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

[Indu Bhagat]

On 7/22/25 4:38 AM, Sam James wrote:
> 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.
> 

Thanks for clarification.

So what is needed is to also disambiguate between 
--enable-default-sframe and user-specified --gsframe/--gsframe=yes.  The 
intent is to not display SFrame related warnings or errors like:

   as_bad (_(".sframe not supported for target"));

for unsupported targets if --enable-default-sframe is in effect.

This implies we need to have a four state enum:

enum gen_sframe_option
{
   GEN_SFRAME_CONFIG_DISABLED,
   GEN_SFRAME_CONFIG_ENABLED,
   GEN_SFRAME_DISABLED,
   GEN_SFRAME_ENABLED,
};

I can add the four enums now, and with a later patch for configure time 
enablement or warning/error hiding, we can start to exercise the 
GEN_SFRAME_CONFIG_ENABLED.

Thanks