[PATCH v2] gas: support --gsframe=no

[Indu Bhagat]

On 7/12/25 1:59 PM, Sam James wrote:
> Indu Bhagat <indu.bhagat@oracle.com> writes:
> 
>> On 7/11/25 9:45 AM, Sam James wrote:
>>> Being able to explicitly disable SFrames on the command line is useful,
>>> especially when looking at a gas that enables SFrames by default. The
>>> binutils testsuite will benefit from this as there's testcases that don't
>>> expect their presence.
>>> 	PR gas/33125
>>> 	* gas/as.c (parse_args): Accept --gsframe=no, --gsframe=yes.
>>> ---
>>> v2: Make clear what the default is and tweak help text.
>>>
>>
>> One comment below for the help text for default.
>>
>>> (Note: I do have another patch for controlling the default, but that
>>> will come later with testsuite adjustments and it exposed some bugs
>>> that Indu is looking at.)
>>>    gas/as.c | 15 ++++++++++++---
>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>> diff --git a/gas/as.c b/gas/as.c
>>> index 78970ff2714..5c56e0fd7eb 100644
>>> --- a/gas/as.c
>>> +++ b/gas/as.c
>>> @@ -311,7 +311,8 @@ Options:\n\
>>>      fprintf (stream, _("\
>>>                              generate GNU Build notes if none are present in the input\n"));
>>>      fprintf (stream, _("\
>>> -  --gsframe               generate SFrame stack trace information\n"));
>>> +  --gsframe=[no|yes]      whether to generate SFrame stack trace information\n\
>>> +                          (default: no)\n"));
>>
>> (default: yes)
> 
> The default is no if you pass nothing, but the default is yes if
> you pass --gsframe..
> 
OK.  So the default of --gsframe is equivalent to --gsframe=yes. 
Meaning the default is yes if you pass --gsframe.  Agreed.  But this 
contradicts ...

> In my other (yet-unposted) patch to allow default-enabling of SFrames
> via configure, I make the 'no' dependent on whatever I chose at
> configure-time.
> 

> If we interpret the 'default: XXX' value as being "what does --gsframe
> do", then in that case, it'll always say no, I think?
> 

...this one.

If we interpret 'default: XXX' value as "what does --gsframe do", the 
default is a yes.  In your patch, we do:

>  	case OPTION_SFRAME:
> -	  flag_gen_sframe = 1;
> +	  if (optarg)
> +	    if (strcasecmp (optarg, "no") == 0)
> +	      flag_gen_sframe = 0;
> +	    else if (strcasecmp (optarg, "yes") == 0)
> +	      flag_gen_sframe = 1;
> +	    else
> +	      as_fatal (_("Invalid --gsframe option: `%s'"), optarg);
> +	  else
> +	    flag_gen_sframe = 1;
>  	  break;

Also, FYI, note in the gas/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)
>         {
>              // create and emit SFrame info
>         }
>       else
> #endif
>         as_bad (_(".sframe not supported for target"));
>     }

IOW, I am not clear how the current patch will fulfill the desired 
configure time setting the default to yes/no later.

--gsframe needs to continue default to --gsframe=yes.

> WDYT?
>