Re: [PATCH,V3 03/15] gas: generate .sframe from CFI directives

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

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 = \
>  	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 = \
>  	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,
>  
>    /* 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 == X86_64_ABI);
> +  return (offsetT)-8;

spaces around the -

> --- a/gas/dw2gencfi.c
> +++ b/gas/dw2gencfi.c
>  
> +  // 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) != 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) != 0)
> +    {
> +#ifdef support_sframe_p
> +      if (support_sframe_p ())
> +	{
> +	  segT sframe_seg;
> +	  int alignment = ffs (DWARF2_ADDR_SIZE (stdoutput)) - 1;
> +
> +	  if (!SUPPORT_FRAME_LINKONCE)
> +	    sframe_seg = 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
> 
> +/* 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 = 0;
> +
> +  if (strcmp (target_arch, "aarch64") == 0)
> +    {
> +      sframe_abi_arch = big_endian_p
> +	? SFRAME_ABI_AARCH64_ENDIAN_BIG
> +	: SFRAME_ABI_AARCH64_ENDIAN_LITTLE;
> +    }
> +  else if (strcmp (target_arch, "x86-64") == 0)
> +    {
> +      gas_assert (!big_endian_p);
> +      sframe_abi_arch = 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 = 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 = 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 == SFRAME_CFA_FP_REG)
> +      || (cur_fre->cfa_base_reg == SFRAME_CFA_SP_REG))
> +    {
> +      sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.i);
> +      cur_fre->merge_candidate = 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]