[sheepdog] [PATCH] Fix hardcode AMD64 assembly code (RSP)

[YunQiang Su]

On Mon, Jul 29, 2013 at 4:06 PM, Hitoshi Mitake
<mitake.hitoshi at gmail.com> wrote:
> At Mon, 29 Jul 2013 14:41:35 +0800,
> Liu Yuan wrote:
>>
>> On Sun, Jul 28, 2013 at 02:05:49AM +0800, YunQiang Su wrote:
>> > RSP is a register name only for AMD64 (x86_64) platform,
>> > while on other platform it may be another name.
>> >
>> > On i386 (x86_32) platform, it is "esp".
>> > On most of RISC platform (PowerPC, SPARC, MIPS, ARM, etc),
>> > it is "sp".
>> > ---
>> >  include/logger.h | 23 ++++++++++++++++++++++-
>> >  lib/logger.c     |  5 -----
>> >  2 files changed, 22 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/include/logger.h b/include/logger.h
>> > index 79eb108..81691b2 100644
>> > --- a/include/logger.h
>> > +++ b/include/logger.h
>> > @@ -38,10 +38,31 @@ void log_write(int prio, const char *func, int line, const char *fmt, ...)
>> >  void set_thread_name(const char *name, bool show_idx);
>> >  void get_thread_name(char *name);
>> >
>> > +#ifdef __i386__
>> > +#define SP_REG "esp"
>> > +#elif defined(__x86_64__)
>> > +#define SP_REG "rsp"
>> > +#elif defined(__mips__) || defined(__sparc__) || \
>> > +      defined (__arm__) || (defined __powerpc__) || \
>> > +      defined (__s390__) || (defined __s390x__) || \
>> > +      defined (__ia64__)
>> > +#define SP_REG "sp"
>> > +#endif
>> > +
>> > +#ifdef SP_REG
>> > +#define dump_stack_frames() ({                     \
>> > +   register void *current_sp asm(SP_REG);  \
>> > +   __dump_stack_frames(current_sp);        \
>> > +})
>> >  #define sd_dump_variable(var) ({           \
>> > -   register void *current_sp asm("rsp");   \
>> > +   register void *current_sp asm(SP_REG);  \
>> >     __sd_dump_variable(#var, current_sp);   \
>> >  })
>> > +#else
>> > +#define dump_stack_frames() ()
>> > +#define sd_dump_variable(var) ()
>> > +#endif
>> > +
>> >  int __sd_dump_variable(const char *var, const void *base_sp);
>> >  void sd_backtrace(void);
>> >
>> > diff --git a/lib/logger.c b/lib/logger.c
>> > index 253163d..ba55adc 100644
>> > --- a/lib/logger.c
>> > +++ b/lib/logger.c
>> > @@ -742,11 +742,6 @@ notrace int __sd_dump_variable(const char *var, const void *base_sp)
>> >     return 0;
>> >  }
>> >
>> > -#define dump_stack_frames() ({                     \
>> > -   register void *current_sp asm("rsp");   \
>> > -   __dump_stack_frames(current_sp);        \
>> > -})
>> > -
>> >  __attribute__ ((__noinline__))
>> >  static notrace int __dump_stack_frames(const void *base_sp)
>> >  {
>> > --
>>
>> I've come up with a better fix. Which use the gcc builtin function to get the
>> stack pointer.
>>
>> From 44300c3fe580a677757803cc30838923ccc5b748 Mon Sep 17 00:00:00 2001
>> From: Liu Yuan <namei.unix at gmail.com>
>> Date: Mon, 29 Jul 2013 14:33:58 +0800
>> Subject: [PATCH] sheep: use gcc builtin to get stack frame pointer
>>
>> This remove hardcoded assembly which is not portable.
>>
>> Signed-off-by: Liu Yuan <namei.unix at gmail.com>
>>
>> diff --git a/include/logger.h b/include/logger.h
>> index 79eb108..9253548 100644
>> --- a/include/logger.h
>> +++ b/include/logger.h
>> @@ -39,10 +39,9 @@ void set_thread_name(const char *name, bool show_idx);
>>  void get_thread_name(char *name);
>>
>>  #define sd_dump_variable(var) ({             \
>> -     register void *current_sp asm("rsp");   \
>> -     __sd_dump_variable(#var, current_sp);   \
>> +     __sd_dump_variable(#var);               \
>>  })
>> -int __sd_dump_variable(const char *var, const void *base_sp);
>> +int __sd_dump_variable(const char *var);
>>  void sd_backtrace(void);
>>
>>  /* sheep log priorities, comliant with syslog spec */
>> diff --git a/lib/logger.c b/lib/logger.c
>> index 253163d..da1221b 100644
>> --- a/lib/logger.c
>> +++ b/lib/logger.c
>> @@ -697,11 +697,15 @@ static bool check_gdb(void)
>>       return system("which gdb > /dev/null") == 0;
>>  }
>>
>> +#define STACK_POINTER        \
>> +     ((char *)__builtin_frame_address(0) + __SIZEOF_POINTER__ * 2)
>
> Doesn't this depend on the ABI of x86?
> # I don't know ABIs of other architectures supported in YunQiang's
> # patch. Do you know their ABIs?
Will use backtrace(3) be a good idea?
>
> Thanks,
> Hitoshi



-- 
YunQiang Su