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

[Indu Bhagat <indu.bhagat@oracle.com>]

Hi Mike,

On 10/30/22 01:03, Mike Frysinger wrote:
> 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
> 

OK.

>> --- 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 ?
> 

I tried to keep the same style as the file originally had.

>> --- 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 -
> 

Fixed.

>> --- 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 ?
> 

This was an obsolete comment. The two ways to generate SFrame sections 
will remain:
   - pass --gsframe command line option to GNU as
   - compiler generates the .cfi_sections .sframe directive

Thanks for pointing out. I fixed the comment.

>> +  // .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
> 

OK.

>> --- 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
> 

Fixed.

>> --- /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 ?
> 

Hmm..on taking a second look, yes, may be I can fold it into the 
config/tc-* headers. I initially started with a mindset to get the 
minimal information from the backends and then translate/use that to 
create SFrame specific constants.

Either approach works.  What you suggest looks easier to read, so I have 
switched to that.

>> +      /* 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
> 

Fixed.

>> +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

The return type is intended to return an error code.  I have now defined 
a set of error codes.  They are limited at this time, but can be used to 
convey information from the callee to caller later.

Thanks for your review.