[2/3] gas/gen-sframe: avoid gcc extension using __VA_ARGS__

  We shouldn't be using extensions when we don't have a suitable fallback in
place. Introducing a format-argument-less counterpart macro would feel a
little odd here. Instead make the sole use site have a (fake) argument
(the non-translatable part of the string).
  

On 12/5/2025 2:32 PM, Jan Beulich wrote:
> We shouldn't be using extensions when we don't have a suitable fallback in
> place. Introducing a format-argument-less counterpart macro would feel a
> little odd here. Instead make the sole use site have a (fake) argument
> (the non-translatable part of the string).
> 
> --- a/gas/dw2gencfi.c
> +++ b/gas/dw2gencfi.c
> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>  #endif
>  	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>  	   -32 on x86_64.  */
> -	sframe_as_bad (_(".sframe not supported for target"));
> +	sframe_as_bad (_("%s not supported for target"), ".sframe");

Couldn't this change be omitted, when changing the definition of
sframe_as_bad as follows?

>      }
>  
>    if ((all_cfi_sections & CFI_EMIT_debug_frame) != 0)
> --- a/gas/gen-sframe.h
> +++ b/gas/gen-sframe.h
> @@ -28,7 +28,7 @@
>  #define sframe_as_bad(format, ...) \

#define sframe_as_bad(...) \

>    do {					       \
>      if (flag_gen_sframe == GEN_SFRAME_ENABLED) \
> -      as_bad (format, ##__VA_ARGS__);	       \
> +      as_bad (format, __VA_ARGS__);            \

      as_bad (__VA_ARGS__); \

>    } while (0)
>  
>  #define SFRAME_FRE_ELEM_LOC_REG		0
> 

IIRC this is how Indu resolved a similar case in libsframe.

Regards,
Jens
  

On 05.12.2025 16:05, Jens Remus wrote:
> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>> We shouldn't be using extensions when we don't have a suitable fallback in
>> place. Introducing a format-argument-less counterpart macro would feel a
>> little odd here. Instead make the sole use site have a (fake) argument
>> (the non-translatable part of the string).
>>
>> --- a/gas/dw2gencfi.c
>> +++ b/gas/dw2gencfi.c
>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>  #endif
>>  	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>  	   -32 on x86_64.  */
>> -	sframe_as_bad (_(".sframe not supported for target"));
>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");
> 
> Couldn't this change be omitted, when changing the definition of
> sframe_as_bad as follows?
> 
>>      }
>>  
>>    if ((all_cfi_sections & CFI_EMIT_debug_frame) != 0)
>> --- a/gas/gen-sframe.h
>> +++ b/gas/gen-sframe.h
>> @@ -28,7 +28,7 @@
>>  #define sframe_as_bad(format, ...) \
> 
> #define sframe_as_bad(...) \
> 
>>    do {					       \
>>      if (flag_gen_sframe == GEN_SFRAME_ENABLED) \
>> -      as_bad (format, ##__VA_ARGS__);	       \
>> +      as_bad (format, __VA_ARGS__);            \
> 
>       as_bad (__VA_ARGS__); \
> 
>>    } while (0)
>>  
>>  #define SFRAME_FRE_ELEM_LOC_REG		0

Yes, but see what you and I think about the similar aspect in objcopy.c. One
of the problems with this form is that the macro then also permits for no
arguments at all. Sure, the compiler will flag a missing argument to as_bad()
then, but still.

Jan
  

Hello Jan!

On 12/8/2025 8:27 AM, Jan Beulich wrote:
> On 05.12.2025 16:05, Jens Remus wrote:
>> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>>> We shouldn't be using extensions when we don't have a suitable fallback in
>>> place. Introducing a format-argument-less counterpart macro would feel a
>>> little odd here. Instead make the sole use site have a (fake) argument
>>> (the non-translatable part of the string).
>>>
>>> --- a/gas/dw2gencfi.c
>>> +++ b/gas/dw2gencfi.c
>>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>>  #endif
>>>  	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>>  	   -32 on x86_64.  */
>>> -	sframe_as_bad (_(".sframe not supported for target"));
>>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");

Maybe change as follows to ease grepping for the error message?

	sframe_as_bad ("%s", _(".sframe not supported for target"));

>>
>> Couldn't this change be omitted, when changing the definition of
>> sframe_as_bad as follows?
>>
>>>      }
>>>  
>>>    if ((all_cfi_sections & CFI_EMIT_debug_frame) != 0)
>>> --- a/gas/gen-sframe.h
>>> +++ b/gas/gen-sframe.h
>>> @@ -28,7 +28,7 @@
>>>  #define sframe_as_bad(format, ...) \
>>
>> #define sframe_as_bad(...) \
>>
>>>    do {					       \
>>>      if (flag_gen_sframe == GEN_SFRAME_ENABLED) \
>>> -      as_bad (format, ##__VA_ARGS__);	       \
>>> +      as_bad (format, __VA_ARGS__);            \
>>
>>       as_bad (__VA_ARGS__); \
>>
>>>    } while (0)
>>>  
>>>  #define SFRAME_FRE_ELEM_LOC_REG		0
> 
> Yes, but see what you and I think about the similar aspect in objcopy.c. One
> of the problems with this form is that the macro then also permits for no
> arguments at all. Sure, the compiler will flag a missing argument to as_bad()
> then, but still.

Regards,
Jens
  

On 08.12.2025 11:08, Jens Remus wrote:
> On 12/8/2025 8:27 AM, Jan Beulich wrote:
>> On 05.12.2025 16:05, Jens Remus wrote:
>>> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>>>> We shouldn't be using extensions when we don't have a suitable fallback in
>>>> place. Introducing a format-argument-less counterpart macro would feel a
>>>> little odd here. Instead make the sole use site have a (fake) argument
>>>> (the non-translatable part of the string).
>>>>
>>>> --- a/gas/dw2gencfi.c
>>>> +++ b/gas/dw2gencfi.c
>>>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>>>  #endif
>>>>  	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>>>  	   -32 on x86_64.  */
>>>> -	sframe_as_bad (_(".sframe not supported for target"));
>>>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");
> 
> Maybe change as follows to ease grepping for the error message?
> 
> 	sframe_as_bad ("%s", _(".sframe not supported for target"));

Possible as well. I actually like pulling out the non-translatable directive,
but I also can see your point about grep-ability (which would go away though
anyway if true parametrization would be necessary). Indu's call, in the end.

Jan
  

On 12/8/25 2:16 AM, Jan Beulich wrote:
> On 08.12.2025 11:08, Jens Remus wrote:
>> On 12/8/2025 8:27 AM, Jan Beulich wrote:
>>> On 05.12.2025 16:05, Jens Remus wrote:
>>>> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>>>>> We shouldn't be using extensions when we don't have a suitable fallback in
>>>>> place. Introducing a format-argument-less counterpart macro would feel a
>>>>> little odd here. Instead make the sole use site have a (fake) argument
>>>>> (the non-translatable part of the string).
>>>>>
>>>>> --- a/gas/dw2gencfi.c
>>>>> +++ b/gas/dw2gencfi.c
>>>>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>>>>   #endif
>>>>>   	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>>>>   	   -32 on x86_64.  */
>>>>> -	sframe_as_bad (_(".sframe not supported for target"));
>>>>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");
>>
>> Maybe change as follows to ease grepping for the error message?
>>
>> 	sframe_as_bad ("%s", _(".sframe not supported for target"));
> 
> Possible as well. I actually like pulling out the non-translatable directive,
> but I also can see your point about grep-ability (which would go away though
> anyway if true parametrization would be necessary). Indu's call, in the end.
> 

I think sframe_as_bad ("%s", _(".sframe not supported for target")); may 
be slightly better. We may have a usecase for sframe_as_warn () later. 
There may not be a clear non-translatable subset of the string in some 
cases for sframe_as_warn () like:
    as_warn (_("no SFrame FDE emitted; XYZ"));

In which case, switching over to:
      sframe_as_warn ("%s", _("no SFrame FDE emitted; XYZ"));

will at least look more homogeneous (and grep-ability is not adversely 
affected).

Thanks for taking care of this,
Indu
  

On 08.12.2025 19:56, Indu Bhagat wrote:
> On 12/8/25 2:16 AM, Jan Beulich wrote:
>> On 08.12.2025 11:08, Jens Remus wrote:
>>> On 12/8/2025 8:27 AM, Jan Beulich wrote:
>>>> On 05.12.2025 16:05, Jens Remus wrote:
>>>>> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>>>>>> We shouldn't be using extensions when we don't have a suitable fallback in
>>>>>> place. Introducing a format-argument-less counterpart macro would feel a
>>>>>> little odd here. Instead make the sole use site have a (fake) argument
>>>>>> (the non-translatable part of the string).
>>>>>>
>>>>>> --- a/gas/dw2gencfi.c
>>>>>> +++ b/gas/dw2gencfi.c
>>>>>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>>>>>   #endif
>>>>>>   	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>>>>>   	   -32 on x86_64.  */
>>>>>> -	sframe_as_bad (_(".sframe not supported for target"));
>>>>>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");
>>>
>>> Maybe change as follows to ease grepping for the error message?
>>>
>>> 	sframe_as_bad ("%s", _(".sframe not supported for target"));
>>
>> Possible as well. I actually like pulling out the non-translatable directive,
>> but I also can see your point about grep-ability (which would go away though
>> anyway if true parametrization would be necessary). Indu's call, in the end.
>>
> 
> I think sframe_as_bad ("%s", _(".sframe not supported for target")); may 
> be slightly better. We may have a usecase for sframe_as_warn () later. 
> There may not be a clear non-translatable subset of the string in some 
> cases for sframe_as_warn () like:
>     as_warn (_("no SFrame FDE emitted; XYZ"));
> 
> In which case, switching over to:
>       sframe_as_warn ("%s", _("no SFrame FDE emitted; XYZ"));
> 
> will at least look more homogeneous (and grep-ability is not adversely 
> affected).

If the XYZ is intended to vary, an alternative (saving on consumed .rodata
space) might be

	sframe_as_warn (_("no SFrame FDE emitted; %s), _("XYZ"));

And in any event, if the argument-less pattern was to occur more frequently,
I would anyway prefer to introduce a 2nd macro along the lines of what patch
1 here does, e.g. sframe_as_bad0() / sframe_as_warn0().

Thoughts? (Ftaod, unless I hear otherwise, I'll switch as you request above
for the patch here and commit presumably by the end of the week. The aspects
discussed can easily be addressed later on.)

Jan
  

On 12/8/25 11:23 PM, Jan Beulich wrote:
> On 08.12.2025 19:56, Indu Bhagat wrote:
>> On 12/8/25 2:16 AM, Jan Beulich wrote:
>>> On 08.12.2025 11:08, Jens Remus wrote:
>>>> On 12/8/2025 8:27 AM, Jan Beulich wrote:
>>>>> On 05.12.2025 16:05, Jens Remus wrote:
>>>>>> On 12/5/2025 2:32 PM, Jan Beulich wrote:
>>>>>>> We shouldn't be using extensions when we don't have a suitable fallback in
>>>>>>> place. Introducing a format-argument-less counterpart macro would feel a
>>>>>>> little odd here. Instead make the sole use site have a (fake) argument
>>>>>>> (the non-translatable part of the string).
>>>>>>>
>>>>>>> --- a/gas/dw2gencfi.c
>>>>>>> +++ b/gas/dw2gencfi.c
>>>>>>> @@ -2622,7 +2622,7 @@ cfi_finish (void)
>>>>>>>    #endif
>>>>>>>    	/* Avoid erroring with DEFAULT_SFRAME for non-default options, like
>>>>>>>    	   -32 on x86_64.  */
>>>>>>> -	sframe_as_bad (_(".sframe not supported for target"));
>>>>>>> +	sframe_as_bad (_("%s not supported for target"), ".sframe");
>>>>
>>>> Maybe change as follows to ease grepping for the error message?
>>>>
>>>> 	sframe_as_bad ("%s", _(".sframe not supported for target"));
>>>
>>> Possible as well. I actually like pulling out the non-translatable directive,
>>> but I also can see your point about grep-ability (which would go away though
>>> anyway if true parametrization would be necessary). Indu's call, in the end.
>>>
>>
>> I think sframe_as_bad ("%s", _(".sframe not supported for target")); may
>> be slightly better. We may have a usecase for sframe_as_warn () later.
>> There may not be a clear non-translatable subset of the string in some
>> cases for sframe_as_warn () like:
>>      as_warn (_("no SFrame FDE emitted; XYZ"));
>>
>> In which case, switching over to:
>>        sframe_as_warn ("%s", _("no SFrame FDE emitted; XYZ"));
>>
>> will at least look more homogeneous (and grep-ability is not adversely
>> affected).
> 
> If the XYZ is intended to vary, an alternative (saving on consumed .rodata
> space) might be
> 
> 	sframe_as_warn (_("no SFrame FDE emitted; %s), _("XYZ"));
> 
> And in any event, if the argument-less pattern was to occur more frequently,
> I would anyway prefer to introduce a 2nd macro along the lines of what patch
> 1 here does, e.g. sframe_as_bad0() / sframe_as_warn0().
> 

OK.

> Thoughts? (Ftaod, unless I hear otherwise, I'll switch as you request above
> for the patch here and commit presumably by the end of the week. The aspects
> discussed can easily be addressed later on.)
> 

Sounds reasonable to me.

Thanks