[PATCH] gas: sframe: command line option takes precedence

[Sam James]

Jan Beulich <jbeulich@suse.com> writes:

> On 18.07.2025 21:06, Indu Bhagat wrote:
>> On 7/18/25 11:33 AM, Indu Bhagat wrote:
>>> On 7/18/25 1:25 AM, Jan Beulich wrote:
>>>> On 18.07.2025 09:21, Indu Bhagat via Binutils wrote:
>>>>> --- a/gas/as.h
>>>>> +++ b/gas/as.h
>>>>> @@ -355,8 +355,15 @@ COMMON int flag_execstack;
>>>>>   /* TRUE if .note.GNU-stack section with SEC_CODE should be created */
>>>>>   COMMON int flag_noexecstack;
>>>>> -/* TRUE if .sframe section should be created.  */
>>>>> -COMMON int flag_gen_sframe;
>>>>> +enum gen_sframe_option
>>>>> +{
>>>>> +  GEN_SFRAME_NONE = 0,
>>>>
>>>> Maybe better GEN_SFRAME_DEFAULT, seeing Sam's plan to allow controlling the
>>>> default from configure?
>>>>
>>>
>>> I think GEN_SFRAME_DEFAULT will be confusing.  Related response below..
>
> As will be NONE - it can be read as similar/equivalent to DISABLED.
>
>>>>> --- a/gas/dw2gencfi.c
>>>>> +++ b/gas/dw2gencfi.c
>>>>> @@ -2596,10 +2596,13 @@ cfi_finish (void)
>>>>>         flag_traditional_format = save_flag_traditional_format;
>>>>>       }
>>>>> -  /* Generate SFrame section if the user specifies:
>>>>> -    - the command line option to gas, or
>>>>> -    - .sframe in the .cfi_sections directive.  */
>>>>> -  if (flag_gen_sframe || (all_cfi_sections & CFI_EMIT_sframe) != 0)
>>>>> +  /* Generate SFrame section if the user:
>>>>> +    - enables via the command line option, or
>>>>> +    - specifies .sframe in the .cfi_sections directive and does not disable
>>>>> +      via the command line.  */
>>>>> +  if (flag_gen_sframe == GEN_SFRAME_ENABLED
>>>>> +      || ((all_cfi_sections & CFI_EMIT_sframe) != 0
>>>>> +      && flag_gen_sframe != GEN_SFRAME_DISABLED))
>>>>
>>>> With said plan, I'm not sure about this. What's the intended behavior if
>>>> the default is "disabled"? It feels like you need to resolve the default
>>>> anyway after having parsed the command line. At which point I'm not even
>>>> sure anymore that a tristate is needed here.
>>>>
>>>
>>> The intended behavior IMO should be:
>>>    - command line takes precedence over configure time setting and cfi_section directive.
>>>    - .cfi_section directive takes precedence over configure time setting.
>>>
>>> (The command line takes precedence over configure time defaults.) 
>>> So Sam's patch to add configure-time enablement will basically just
>>> initialize the value of flag_gen_sframe to GEN_SFRAME_ENABLED or
>>> GEN_SFRAME_DISABLED based on --enable-sframe/--disable-sframe
>>> respectively at an appropriate time.
>>>
>> 
>> Correction: IMO Sam's patch to add configure-time enablement will
>> basically just initialize the value of flag_gen_sframe to
>> GEN_SFRAME_ENABLED or GEN_SFRAME_NONE based on
>> --enable-sframe/--disable-sframe respectively at an appropriate
>> time.
>> 
>> When --disable-sframe is used, the desired behavior is equivalent to 'passing no --gsframe', and not '--gsframe=no'.
>
> That's one of (at least) two possible behaviors. Sam, can you clarify what
> the plan is?

We want --disable-sframe (or --disable-default-sframe, whatever) at
configure-time to mean "no --gsframe was passed".

i.e. we want --enable-default-sframe to always do something positive (imply
--gsframe/--gsframe=yes when it wasn't otherwise passed), and not for
--disable-default-sframe to ever stop people from enabling SFrames
via a command-line argument.

Now, where a tri-state may be useful (or some way of representing when
we're "default-enabled, not explicitly by the user") is when suppressing
warnings, or an error in this particular place in dw2gencfi.c:


  /* Generate SFrame section if the user specifies:
        - the command line option to gas, or
        - .sframe in the .cfi_sections directive.  */
  if (flag_gen_sframe || (all_cfi_sections & CFI_EMIT_sframe) != 0)
    {
#ifdef support_sframe_p
      if (support_sframe_p () && !SUPPORT_FRAME_LINKONCE)
        {
          segT sframe_seg;
          int alignment = ffs (DWARF2_ADDR_SIZE (stdoutput)) - 1;

          sframe_seg = get_cfi_seg (NULL, ".sframe",
                                    (SEC_ALLOC | SEC_LOAD | SEC_DATA
                                     | DWARF2_EH_FRAME_READ_ONLY),
                                    alignment);
          elf_section_type (sframe_seg) = SHT_GNU_SFRAME;
          output_sframe (sframe_seg);
        }
      else
#endif
        as_bad (_(".sframe not supported for target"));
    }

Having that as_bad call is a real pain for --enable-default-sframe if
`as -32` is used on a x86_64 target.

sam