From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.gentoo.org (smtp.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) by sourceware.org (Postfix) with ESMTP id 08CB83857C4F for ; Sun, 30 Oct 2022 09:17:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 08CB83857C4F 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 87BDA340FBA; Sun, 30 Oct 2022 09:17:55 +0000 (UTC) Date: Sun, 30 Oct 2022 13:48:33 +0545 From: Mike Frysinger To: Indu Bhagat Cc: binutils@sourceware.org Subject: Re: [PATCH,V3 03/15] gas: generate .sframe from CFI directives Message-ID: Mail-Followup-To: Indu Bhagat , binutils@sourceware.org References: <20221030074450.1956074-1-indu.bhagat@oracle.com> <20221030074450.1956074-4-indu.bhagat@oracle.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0eszVPStTYVzAF16" Content-Disposition: inline In-Reply-To: <20221030074450.1956074-4-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: --0eszVPStTYVzAF16 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: > --- a/gas/Makefile.am > +++ b/gas/Makefile.am > @@ -70,6 +70,8 @@ GAS_CFILES =3D \ > atof-generic.c \ > compress-debug.c \ > cond.c \ > + sframe-opt.c \ > + gen-sframe.c \ > depend.c \ > dwarf2dbg.c \ > dw2gencfi.c \ > @@ -105,6 +107,7 @@ HFILES =3D \ > bit_fix.h \ > cgen.h \ > compress-debug.h \ > + gen-sframe.h \ > dwarf2dbg.h \ > dw2gencfi.h \ > ecoff.h \ i think both these variables are sorted, so you should keep that > --- a/gas/as.h > +++ b/gas/as.h > @@ -261,7 +261,10 @@ enum _relax_state > rs_cfa, > =20 > /* Cross-fragment dwarf2 line number optimization. */ > - rs_dwarf2dbg > + rs_dwarf2dbg, > + > + /* SFrame FRE type selection optimization. */ > + rs_sframe add a dangling comma ? > --- a/gas/config/tc-i386.c > +++ b/gas/config/tc-i386.c > > +offsetT > +x86_sframe_cfa_ra_offset (void) > +{ > + gas_assert (x86_elf_abi =3D=3D X86_64_ABI); > + return (offsetT)-8; spaces around the - > --- a/gas/dw2gencfi.c > +++ b/gas/dw2gencfi.c > =20 > + // FIXME - remove this commented line once the compiler can specify can you link to an open bug in GCC bugzilla ? or some other tracker ? > + // .sframe for .cfi_sections directive period at the end of sentences. > + // if ((all_cfi_sections & CFI_EMIT_sframe) !=3D 0) i didn't think we used // C++ comments in C code, but stuck to /*...*/ > + if (flag_gen_sframe || (all_cfi_sections & CFI_EMIT_sframe) !=3D 0) > + { > +#ifdef support_sframe_p > + if (support_sframe_p ()) > + { > + segT sframe_seg; > + int alignment =3D ffs (DWARF2_ADDR_SIZE (stdoutput)) - 1; > + > + if (!SUPPORT_FRAME_LINKONCE) > + sframe_seg =3D get_cfi_seg (NULL, ".sframe", > + (SEC_ALLOC | SEC_LOAD | SEC_DATA > + | DWARF2_EH_FRAME_READ_ONLY), > + alignment); > + output_sframe (sframe_seg); > + } > + else > + as_bad (_(".sframe not supported for target")); > + > +#else > + as_bad (_(".sframe not supported for target")); > +#endif rather than interleaving cpp checks in the middle of code (which are hard to keep track of), put something at the top of the file (or a header) like: #ifndef support_sframe_p # define support_sframe_p() false #endif > --- a/gas/dw2gencfi.h > +++ b/gas/dw2gencfi.h > @@ -200,5 +200,6 @@ extern struct fde_entry *all_fde_data; > #define CFI_EMIT_debug_frame (1 << 1) > #define CFI_EMIT_target (1 << 2) > #define CFI_EMIT_eh_frame_compact (1 << 3) > +#define CFI_EMIT_sframe (1 << 4) keep the defines aligned > --- /dev/null > +++ b/gas/gen-sframe.c >=20 > +/* gen-sframe.c - Support for generating SFrame section. > + Copyright (C) 2021 Free Software Foundation, Inc. isn't this code new to 2022 ? > +unsigned char > +sframe_get_abi_arch_callback (const char *target_arch, > + int big_endian_p) > +{ > + unsigned char sframe_abi_arch =3D 0; > + > + if (strcmp (target_arch, "aarch64") =3D=3D 0) > + { > + sframe_abi_arch =3D big_endian_p > + ? SFRAME_ABI_AARCH64_ENDIAN_BIG > + : SFRAME_ABI_AARCH64_ENDIAN_LITTLE; > + } > + else if (strcmp (target_arch, "x86-64") =3D=3D 0) > + { > + gas_assert (!big_endian_p); > + sframe_abi_arch =3D SFRAME_ABI_AMD64_ENDIAN_LITTLE; shouldn't this be in config/tc-* headers ? > + /* Other abi/arch are not supported. Should be unreachable. */ > + printf (_("SFrame Unsupported abi or arch\n")); > + abort (); as_fatal would be better than calling abort. comes up multiple times. > +static void > +sframe_xlate_ctx_cleanup (struct sframe_xlate_ctx *xlate_ctx) > +{ > + struct sframe_row_entry *fre, *fre_next; > + > + if (xlate_ctx->num_xlate_fres) > + { > + fre =3D xlate_ctx->first_fre; > + while (fre) > + { bad GNU style -- braces indent one more level > +static int > +sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx, > + struct cfi_insn_data *cfi_insn) > +{ > + /* The scratchpad FRE currently being updated with each cfi_insn > + being interpreted. This FRE eventually gets linked in into the > + list of FREs for the specific function. */ > + struct sframe_row_entry *cur_fre =3D xlate_ctx->cur_fre; > + > + gas_assert (cur_fre); > + /* Define the current CFA rule to use the provided offset (but to keep > + the old register). However, if the old register is not FP/SP, > + skip creating SFrame unwind info for the function. */ > + if ((cur_fre->cfa_base_reg =3D=3D SFRAME_CFA_FP_REG) > + || (cur_fre->cfa_base_reg =3D=3D SFRAME_CFA_SP_REG)) > + { > + sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.i); > + cur_fre->merge_candidate =3D false; > + } > + else > + return -1; > + > + return 0; > +} is this returning a boolean ? use an actual bool type w/true+false instead of 0/1/-1 integer values. this comes up multiple times in this file. -mike --0eszVPStTYVzAF16 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEuQK1JxMl+JKsJRrUQWM7n+g39YEFAmNeL9UACgkQQWM7n+g3 9YE3Zw//Z1z/1vZP9KZhsXnUo9P389W+o04sbG4GZxNp3c3JPBDJKniZFoNUokEd kKpkyN7nf5dY9hgmyy5t+d5nqERxJY69/kFY6/6deWyNsLGIZAW8G7kE7o/pibku P0Ouw9jnCCY0yocAOvcqU2rKRApxv9gr6lXnKpZsQWMik2SUwZGLoPRq2PxBz5/n klUkrr7DWUh0IMJ7BWUPdbIh9AaCsv9pm4N+A9l2IasrvvRsVoTun8LQ12e6XEFs q5HiqsLexNmTMDtzqKB02gkI0vIMWcPbioYbWBCi3in6Ryjl2W/RI+zgJeSlZ8P6 rxO5cPsKFJZWu6EioyTHEPk8tnOxDtk9P7ZXaKY4zNJ0xDpDu8YCm9WoJXXUMUa8 4ZUtlkNsZbiaBHze2GfpvwM/g3B0qcoKCpQiRn20ZNxSlJ6wAu+dZiw2wm+XSCG5 jy1l8Z3E/C5Thx+oNt/nMLnd5CKsRxe7v2hEQ9BjhRIXFDZXdhnas9ENHJxB9rFp tY1MYZ8RdPxkn9J0VhGAurEBXn8M2QaW36rkUwcR3H56EnseAaPcu8ILS1it2aub iknWn3EXhXF0qeyr0xhtKgFM+GWwBvFdzGCsSq97fSI8oc3qRQnVwh6PKTOhtwxf Z7IrvJNa+YiPPC1tViV4zYA5C8hXZutJv3gXqemLclu2Obdlzxs= =qc+B -----END PGP SIGNATURE----- --0eszVPStTYVzAF16--