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

[Hitoshi Mitake]

At Mon, 29 Jul 2013 16:55:56 +0800,
Liu Yuan wrote:
> 
> 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.

I checked your way in SH4a as an example which YunQiang's way doesn't
cover, and it seems work well. If it works well on other important
architectures, e.g. arm, Yuan's way would be better. Could you verify
it?

Thanks,
Hitoshi