From: Mike Frysinger <vapier@gentoo.org>
To: Indu Bhagat <indu.bhagat@oracle.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH,V3 08/15] unwinder: generate backtrace using SFrame format
Date: Sun, 30 Oct 2022 19:48:36 +0545 [thread overview]
Message-ID: <Y16EOFuti76vLfRk@vapier> (raw)
In-Reply-To: <20221030074450.1956074-9-indu.bhagat@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 5879 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2022-10-30 15:17 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-30 7:44 [PATCH,V3 00/15] Definition and support for SFrame unwind format Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 01/15] sframe.h: Add SFrame format definition Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 02/15] gas: add new command line option --gsframe Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 03/15] gas: generate .sframe from CFI directives Indu Bhagat
2022-10-30 8:03 ` Mike Frysinger
2022-11-06 5:30 ` Indu Bhagat
2022-11-06 5:36 ` [PATCH,V3.1 " Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 04/15] gas: testsuite: add new tests for SFrame unwind info Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 05/15] libsframe: add the SFrame library Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 06/15] bfd: linker: merge .sframe sections Indu Bhagat
2022-10-30 13:33 ` Mike Frysinger
2022-10-30 7:44 ` [PATCH,V3 07/15] readelf/objdump: support for SFrame section Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 08/15] unwinder: generate backtrace using SFrame format Indu Bhagat
2022-10-30 14:03 ` Mike Frysinger [this message]
2022-11-01 22:36 ` [PATCH, V3 " Weimin Pan
2022-11-02 15:00 ` Mike Frysinger
2022-11-02 6:23 ` [PATCH,V3 " Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 09/15] unwinder: Add SFrame unwinder tests Indu Bhagat
2022-10-30 14:14 ` Mike Frysinger
2022-10-30 7:44 ` [PATCH,V3 10/15] gdb: sim: buildsystem changes to accommodate libsframe Indu Bhagat
2022-10-30 14:15 ` [PATCH, V3 " Mike Frysinger
2022-10-31 20:39 ` Indu Bhagat
2022-11-02 15:02 ` Mike Frysinger
2022-11-02 19:11 ` Jose E. Marchesi
2022-11-03 15:27 ` Mike Frysinger
2022-11-04 12:14 ` Jose E. Marchesi
2022-10-30 7:44 ` [PATCH,V3 11/15] libctf: add libsframe to LDFLAGS and LIBS Indu Bhagat
2022-10-30 13:27 ` Mike Frysinger
2022-10-31 20:06 ` Indu Bhagat
2022-11-01 18:57 ` Nick Alcock
2022-11-01 21:42 ` Andreas Schwab
2022-11-02 13:16 ` Nick Alcock
2022-11-04 19:02 ` [PATCH,V3.1,11/15] " Indu Bhagat
2022-11-04 21:01 ` [PATCH,V3.2,11/15] " Indu Bhagat
2022-11-05 9:21 ` Andreas Schwab
2022-11-07 22:28 ` [PATCH,V3.3 11/15] " Indu Bhagat
2022-11-08 3:26 ` Mike Frysinger
2022-11-08 19:26 ` Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 12/15] src-release.sh: Add libsframe Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 13/15] binutils/NEWS: add text for SFrame support Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 14/15] gas/NEWS: add text about new command line option and " Indu Bhagat
2022-10-30 8:05 ` [PATCH, V3 " Mike Frysinger
2022-10-31 23:17 ` Indu Bhagat
2022-10-30 7:44 ` [PATCH,V3 15/15] doc: add SFrame spec file Indu Bhagat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y16EOFuti76vLfRk@vapier \
--to=vapier@gentoo.org \
--cc=binutils@sourceware.org \
--cc=indu.bhagat@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).