Home Original page

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

Mike Frysinger vapier@gentoo.org
Sun Oct 30 14:03:36 GMT 2022 
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/<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 ?

> +  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20221030/736098a1/attachment.sig>
More information about the Binutils mailing list