[sheepdog] [PATCH] Fix hardcode AMD64 assembly code (RSP)
Liu Yuan
namei.unix at gmail.com
Mon Jul 29 10:55:56 CEST 2013
On Mon, Jul 29, 2013 at 04:19:08PM +0800, Liu Yuan wrote:
> On Mon, Jul 29, 2013 at 05:06:37PM +0900, Hitoshi Mitake 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 have no idea of other archs. I am not sure. But if this address is wrong, GDB
> just print nothing about the frames or variables.
I think __builtin_frame_address is arch-netrual, but I didn't find out why we
need '+ __SIZEOF_POINTER__ * 2'. I get this by examining the address of
__builtin_frame_address and previous asm("esp") code.
Thanks
Yuan
More information about the sheepdog
mailing list