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

[Jan Beulich]

On 25.07.2025 08:02, Indu Bhagat wrote:
> On 7/24/25 10:32 PM, Jan Beulich wrote:
>> On 24.07.2025 18:14, Indu Bhagat wrote:
>>> On 7/24/25 8:52 AM, Jan Beulich wrote:
>>>> On 24.07.2025 07:52, Indu Bhagat via Binutils wrote:
>>>>> over gas directive to emit .sframe section.
>>>>>
>>>>> Fix PR gas/33175 sframe: --gsframe=no does not disable when
>>>>> .cfi_sections directive with .sframe
>>>>>
>>>>> --gsframe=no should also disable generation of SFrame section when
>>>>> explicit CFI directive:
>>>>>     .cfi_sections .sframe
>>>>> is specified in the input.  This means we need to track whether SFrame
>>>>> generation was explcitly disabled by the user.  Introduce a new enum to
>>>>> facilitate disambiguation between GEN_SFRAME_CONFIG_DISABLED and
>>>>> GEN_SFRAME_DISABLED.
>>>>>
>>>>> While fixing the bug by adding the enum, keep the upcoming requirement
>>>>> in mind: we will also need to 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 (
>>>>> GEN_SFRAME_CONFIG_DISABLED, GEN_SFRAME_CONFIG_ENABLED,
>>>>> GEN_SFRAME_DISABLED, GEN_SFRAME_ENABLED)
>>>>
>>>> Before I look at the change in detail: What is the supposed behavior for
>>>> every one of these? (A code comment on the enumerators would have been
>>>> nice.)
>>>>
>>>
>>> I can add code comments to reflect that.  Perhaps like following (hopefully that helps answer your question too).
>>
>> Yes, it addresses my question.
>>
>>> /* PR gas/33175.
>>>     Add enumerators to disambiguate between configure-time
>>>     enablement/disablement vs user-specficied enablement/disablement (the latter
>>>     via command line).  The expected usage of these states is:  command line
>>>     takes precedence over configure time setting.  */
>>
>> This could do with making things explicit as to auto-generation vs.
>> .cfi_* directive induced handling.
>>
> 
> OK.
> 
> Now I have:
> 
> /* PR gas/33175.
>    Add enumerators to disambiguate between configure-time
>    enablement/disablement vs user-specficied enablement/disablement (the latter
>    via command line).  The expected usage of these states is:
>      - user-specified command line takes precedence over configure-time
>        setting and .cfi_sections directive usage.
>      - .cfi_sections usage takes precedence over configure-time setting.  */
> 
>>> enum gen_sframe_option
>>> {
>>>    /* SFrame generation disabled at configure time.  Default.  */
>>
>> And the behavior then is? By the wording, it would mean no SFrame at all,
>> command line option or not. Just that there is the comment ahead of the
>> enumeration. Personally, as written I'd read these as contradictory with
>> one another.
>>
>>>    GEN_SFRAME_CONFIG_DISABLED,
>>
>> Maybe GEN_SFRAME_CONFIG_DISABLED simply isn't a good name, as it suggests
>> stronger disabling than there is. Maybe GEN_SFRAME_DEFAULT_OFF or
>> GEN_SFRAME_DEFAULT_DISABLED?
>>
> 
> Right. As such there will be a --enable-default-sframe or such, not really a configure-time disablement.  So, GEN_SFRAME_DEFAULT_OFF is good.  Also GEN_SFRAME_DEFAULT_NONE is OK ?

Yes, that's also fine with me.

> (I can send V3 soon after this is settled).  But this is how it is now after addressing your comments :

Much better indeed, thanks.

Jan

> /* PR gas/33175.
>    Add enumerators to disambiguate between configure-time
>    enablement/disablement vs user-specficied enablement/disablement (the latter
>    via command line).  The expected usage of these states is:
>      - user-specified command line takes precedence over configure-time
>        setting and .cfi_sections directive usage.
>      - .cfi_sections usage takes precedence over configure-time setting.  */
> enum gen_sframe_option
> {
>   /* Default. SFrame generation not enabled at configure time.  GNU as will
>      not generate SFrame sections by default, unless enabled by user via
>      command line.  */
>   GEN_SFRAME_DEFAULT_NONE,
>   /* SFrame generation enabled at configure time.  GNU as will generate SFrame
>      sections for all objects, unless disabled by user via command line.  */
>   GEN_SFRAME_CONFIG_ENABLED,
>   /* User specified disablement via --gsframe=no.  */
>   GEN_SFRAME_DISABLED,
>   /* User specified enablement via --gsframe or --gsframe=yes.  */
>   GEN_SFRAME_ENABLED,
> };
> 
> 
> 
> 
> 
>>>    /* SFrame generation enabled at configure time.  GNU as will generate SFrame
>>>       sections for all objects, unless disabled by user via command line.  */
>>
>> I think the equivalent of the 2nd sentence here would want to be in the
>> earlier comment.
>>
>> Jan
>>
> 
> OK.
> 
>>>    GEN_SFRAME_CONFIG_ENABLED,
>>>    /* User specified disablement via --gsframe=no.  */
>>>    GEN_SFRAME_DISABLED,
>>>    /* User specified enablement via --gsframe or --gsframe=yes.  */
>>>    GEN_SFRAME_ENABLED,
>>> };
>>
>>
>