On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote: > --- /dev/null > +++ b/config/sframe.m4 > @@ -0,0 +1,16 @@ > +# SFRAME_CHECK_AS_SFRAME > +# ---------------------- > +# Check whether the assembler supports generation of SFrame > +# unwind information. > +# > +# Defines: > +# ac_cv_have_sframe > + you should be using `dnl` for comments in m4 files so they aren't copied into the generated output. > +AC_DEFUN([SFRAME_CHECK_AS_SFRAME],[ space after the , > + ac_save_CFLAGS="$CFLAGS" > + CFLAGS="$CFLAGS -Wa,--gsframe" > + AC_MSG_CHECKING([for as that supports --gsframe]) > + AC_TRY_COMPILE([], [return 0;], [ac_cv_have_sframe=yes], [ac_cv_have_sframe=no]) > + AC_MSG_RESULT($ac_cv_have_sframe) > + CFLAGS="$ac_save_CFLAGS" > +]) you call it "ac_cv_have_sframe" which implies it's an autoconf cached var, but you aren't actually using the AC_CACHE_CHECK macro. i'm guessing this isn't actually coming from autoconf, or will be merged there, so shouldn't this be using a "gcc_cv_" prefix instead ? i'm not sure what the policy is on config/ when it comes to home-grown cache vars. similarly, should the macro name lacks scoping ... > --- a/libsframe/configure.ac > +++ b/libsframe/configure.ac > > COMPAT_DEJAGNU=$ac_cv_dejagnu_compat > AC_SUBST(COMPAT_DEJAGNU) > > +dnl The libsframebt library needs to be built with SFrame info. > +dnl If the build assembler is not capable of generate SFrame then > +dnl the library is not built. > + > +SFRAME_CHECK_AS_SFRAME > +AM_CONDITIONAL([HAVE_SFRAME_AS], [test "x$ac_cv_have_sframe" = "xyes"]) hmm, is this macro only used by libsframe/ ? if no one else is going to use this macro, config/ isn't the right place for it. you should put it into libsframe/acinclude.m4 instead. > --- /dev/null > +++ b/libsframe/sframe-backtrace-err.c > > +/* SFrame backtrace error messages. */ > +static const char *const sframe_bt_errlist[] = > +{ > + "", > + "File does not contain SFrame data", > + "Iterating shared object reading error", > + "Failed to malloc memory space", > + "Failed to realloc memory space", > + "Failed to open file", > + "Failed on resolve canonical file name", > + "Failed to reposition file offset", > + "Failed to read from a file descriptor", > + "Failed to get the user context", > + "Failed to set up decode data", > + "Illegal CFA offset" > +}; > + > +/* Return the error message associated with the error code. */ > + > +const char * > +sframe_bt_errmsg (enum sframe_bt_errcode ecode) > +{ > + return sframe_bt_errlist[ecode]; > +} this needs to make sure the ecode is within range, and if it isn't, at the very least perform an assert/abort. assert((unsigned int)ecode < ARRAY_SIZE (sframe_bt_errlist)) > --- /dev/null > +++ b/libsframe/sframe-backtrace.c > > +#ifndef _GNU_SOURCE > +#define _GNU_SOURCE > +#endif i thought your configure script already took care of this ? > +#ifndef PT_SFRAME > +#define PT_SFRAME 0x6474e554 /* FIXME. */ > +#endif what excatly is the fix ? that it should be defined in include/elf/common.h first (among other places) ? you should do that then instead of keeping this. > +#define _sf_printflike_(string_index,first_to_check) \ > + __attribute__ ((__format__ (__printf__, (string_index), (first_to_check)))) use ATTRIBUTE_PRINTF from ansidecl instead of defining your own > +static int _sframe_unwind_debug; /* Control for printing out debug info. */ this should be a proper bool instead of an int-pretending-to-be-bool > +static int no_of_entries = 32; shouldn't this be a const ? > +static int > +sframe_bt_errno (int *errp) > +{ > + if (errp == NULL) > + return 0; > + > + return (*errp != SFRAME_BT_OK); > +} shouldn't errp be const ? > +static int > +sframe_fd_open (int *errp) > +{ > + char filename[PATH_MAX]; don't rely on PATH_MAX. use asprintf. > + pid = getpid (); > + snprintf (filename, sizeof filename, "/proc/%d/task/%d/mem", pid, pid); /proc//task// is the same thing as /proc// and /proc// is simply /proc/self/ so can't you use the constant path /proc/self/mem ? am i missing something obvious here ? > + if ((fd = open (filename, O_RDONLY)) == -1) you should always use O_CLOEXEC when calling open(). if you don't want that flag, you should always leave a comment explaining why. > +static int > +sframe_callback (struct dl_phdr_info *info, this seems to be returning a bool, so use a bool instead of an int > + for (i = 0; i < info->dlpi_phnum; i++) > + { > + debug_printf(" %2d: [%14p; memsz:%7lx] flags: 0x%x; \n", i, GNU style puts space before the ( > + (void *) info->dlpi_phdr[i].p_vaddr, p_vaddr is an Elf_Addr right ? you can't assume that sizeof(void*) is >= sizeof(Elf_Addr). a 64-bit BFD on a 32-bit host will break this. cast it to a proper uint64_t and use proper stdint.h PRIx64 types. > + info->dlpi_phdr[i].p_memsz, i think this also has broken assumptions -- that sizeof(long) == sizeof(p_memsz). cast it to 64-bit too. > + sf->sui_ctx.sfdd_data = (char *) malloc (info->dlpi_phdr[i].p_memsz); malloc returns void*. in C, we don't have to cast that. so you can omit it. > +static void > +sframe_unwind (struct sframe_unwind_info *sf, void **ra_lst, > + int *ra_size, int *errp) > +{ > ... > +#ifdef __x86_64__ this really needs better delegation for porting than inlined in the middle of a large portable file. can you at least factor it out into a header ? > + pc = cp->uc_mcontext.gregs[REG_RIP]; > + rsp = cp->uc_mcontext.gregs[REG_RSP]; > + rfp = cp->uc_mcontext.gregs[REG_RBP]; > +#else > +#ifdef __aarch64__ use `#elif defined` to avoid an ever-growing set of nested #else/#endif -mike