Re: [PATCH, V3 08/15] unwinder: generate backtrace using SFrame format

[Weimin Pan <weimin.pan@oracle.com>]

Thanks for your comments.

On 10/30/2022 7:03 AM, Mike Frysinger via Binutils wrote:
> 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.

OK.

>> +AC_DEFUN([SFRAME_CHECK_AS_SFRAME],[
> space after the ,

OK.

>> +  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.

Indu should be able to address issue better.

> 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.

Looks like libsframe/ is the only place that uses this macro.

>> --- /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))

OK, added code to validate ecode. It prints "Unknown error" for invalid 
ecode.

>> --- /dev/null
>> +++ b/libsframe/sframe-backtrace.c
>>
>> +#ifndef _GNU_SOURCE
>> +#define _GNU_SOURCE
>> +#endif
> i thought your configure script already took care of this ?

Yes,  the above code is taken out.

>> +#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.

Since <link.h>, which is needed for dl_iterate_phdr, also includes <elf.h>.
using include/elf/common.h will cause multiple defined warnings, e.g. PT_HIPROC.
Looks like it should be defined in /usr/include/elf.h.

>> +#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

OK.

>> +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

Change its type to bool.

>> +static int no_of_entries = 32;
> shouldn't this be a const ?

OK, done.

>> +static int
>> +sframe_bt_errno (int *errp)
>> +{
>> +  if (errp == NULL)
>> +    return 0;
>> +
>> +  return (*errp != SFRAME_BT_OK);
>> +}
> shouldn't errp be const ?

Done.

>> +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/<pid>/task/<pid>/ is the same thing as /proc/<pid>/
>
> and /proc/<getpid>/ is simply /proc/self/
>
> so can't you use the constant path /proc/self/mem ?
> am i missing something obvious here ?

Nice. We can get rid of asprintf and filename[] altogether and just do:

         if ((fd = open ("/proc/self/mem", O_CLOEXEC)) == -1)
>> +  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

Function sframe_callback is the "callback" of


        int dl_iterate_phdr(
                  int (*callback) (struct dl_phdr_info *info,
                                   size_t size, void *data), void *data);

which returns an int: 0 for success and 1 otherwise.

>> +  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 (

OK

>> +		   (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.

OK, cast it to uint64_t and use PRIu64 for both p_vaddr and p_memsz.

>> +	  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.

OK, done.

>
>> +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 ?

Sorry, not sure I follow this comment. Would you please elaborate on 
"factoring 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

OK.

Thanks again.

> -mike