From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.gentoo.org (mail.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) by sourceware.org (Postfix) with ESMTP id 635E63858D28 for ; Sun, 30 Oct 2022 15:17:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 635E63858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gentoo.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gentoo.org Received: by smtp.gentoo.org (Postfix, from userid 559) id 03DF7340F1F; Sun, 30 Oct 2022 15:17:58 +0000 (UTC) Date: Sun, 30 Oct 2022 19:48:36 +0545 From: Mike Frysinger To: Indu Bhagat Cc: binutils@sourceware.org Subject: Re: [PATCH,V3 08/15] unwinder: generate backtrace using SFrame format Message-ID: Mail-Followup-To: Indu Bhagat , binutils@sourceware.org References: <20221030074450.1956074-1-indu.bhagat@oracle.com> <20221030074450.1956074-9-indu.bhagat@oracle.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="p1+m13a4pF1djY4g" Content-Disposition: inline In-Reply-To: <20221030074450.1956074-9-indu.bhagat@oracle.com> X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --p1+m13a4pF1djY4g Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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=3D"$CFLAGS" > + CFLAGS=3D"$CFLAGS -Wa,--gsframe" > + AC_MSG_CHECKING([for as that supports --gsframe]) > + AC_TRY_COMPILE([], [return 0;], [ac_cv_have_sframe=3Dyes], [ac_cv_have= _sframe=3Dno]) > + AC_MSG_RESULT($ac_cv_have_sframe) > + CFLAGS=3D"$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=3D$ac_cv_dejagnu_compat > AC_SUBST(COMPAT_DEJAGNU) > =20 > +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" =3D "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[] =3D > +{ > + "", > + "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 th= is. > +#define _sf_printflike_(string_index,first_to_check) \ > + __attribute__ ((__format__ (__printf__, (string_index), (first_to_ch= eck)))) 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 =3D 32; shouldn't this be a const ? > +static int > +sframe_bt_errno (int *errp) > +{ > + if (errp =3D=3D NULL) > + return 0; > + > + return (*errp !=3D 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 =3D 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 =3D open (filename, O_RDONLY)) =3D=3D -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 =3D 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 >=3D 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) =3D=3D sizeof(p_memsz). cast it to 64-bit too. > + sf->sui_ctx.sfdd_data =3D (char *) malloc (info->dlpi_phdr[i].p_memsz= ); malloc returns void*. in C, we don't have to cast that. so you can omit i= t. > +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 =3D cp->uc_mcontext.gregs[REG_RIP]; > + rsp =3D cp->uc_mcontext.gregs[REG_RSP]; > + rfp =3D cp->uc_mcontext.gregs[REG_RBP]; > +#else > +#ifdef __aarch64__ use `#elif defined` to avoid an ever-growing set of nested #else/#endif -mike --p1+m13a4pF1djY4g Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEuQK1JxMl+JKsJRrUQWM7n+g39YEFAmNehDgACgkQQWM7n+g3 9YHfthAApX9byoMLXTJ1slzS+zmKM33DSRDlZJx/2MGg8LcnYykQMDt4QXYl+edz air8UEEqOouXYINmJUgEmGtXvdHuxouD1k2s7WXv8NqMhiYbqvmv9IC4zXgkmQgi /c+VlGZE8yRpL6lGK6xjLjgL6tlPHNND2rJTXlICWOY1hGxFBVQSeWRNV0VLGic2 77MehbItCpJCxQL4Kakz2+GvO8ULeh/zxsY7cF1xsaPVPVFUSlGxnNrqtyrN2CN/ FitaS6cSUzRkNF+lY8x+cy7kyESgGEd6h0pagMWYTr/otmar3je7N4DjUZM6G2SA azUR48SYEBXEU6J7FhZuzN8475IgPY6vOYQhr1Nh2TC/4ZQsnnpV+evx6rmP59Gu 19MwO4UMQi83U2b6ImGfie2F9YoHMKZW0YfEj1NHfq4fq/KYAOZ1ZKiE3PcLoZb8 iwcy6VjHZudqHmSvOK3oAJkg9W8iTeq+kzmzDs3zBv2lrJJh4PlE/YIm47SpFgWg CkLXEvwMphkhPVfWztUkPKkGkyebAezkzsx4zPAidnWpyjwomBpbpgthIuA8Ysha h7RgjjOOCdIMhTe6S5EhLvw9h1InTNNgyaI3zBM/alA9BfJz8+e+ODnqbMR6DWDA OKTccw7FowFj8Q/HG+SKnnL37Bcy9eB/Rk4iWeiqA30xiJML7n8= =U54i -----END PGP SIGNATURE----- --p1+m13a4pF1djY4g--