[PATCH 1/3] gas: sframe: guard block with support_sframe_p

[Jan Beulich]

On 07.07.2025 22:43, Indu Bhagat wrote:
> On 7/6/25 11:19 PM, Jan Beulich wrote:
>> On 07.07.2025 08:10, Indu Bhagat via Binutils wrote:
>>> SFrame is generated for ELF targets only.  Guard the block with
>>> support_sframe_p like others for consistency.
>>
>> The block was guarded already, just not ...
>>
>>> @@ -2606,6 +2601,7 @@ cfi_finish (void)
>>>   	- .sframe in the .cfi_sections directive.  */
>>>     if (flag_gen_sframe || (all_cfi_sections & CFI_EMIT_sframe) != 0)
>>>       {
>>> +#ifdef support_sframe_p
>>
>> ... by a pre-processor conditional, just ...
>>
>>>         if (support_sframe_p () && !SUPPORT_FRAME_LINKONCE)
>>
>> ... the invocation (plus the stub that you now remove). Hence the
>> description doesn't really make clear what the problem as and why a
>> change would be needed.
>>
> 
> In a later commit, we would like to do a:
>    elf_section_type (sframe_seg) = SHT_GNU_SFRAME;
> 
> This cannot be done for non-ELF targets, hence the need to guard with 
> some pre-processor conditional to enable it for only OBJ_ELF.

This is the crucial detail that's missing from the description. With
that added the patch is okay. In particular ...

>  Guarding 
> with support_sframe_p works for now, because those targets that support 
> SFrame define support_sframe_p:
>    - x86_64 and aarch64 define support_sframe_p when OBJ_ELF is defined
>    - s390x has no non-LEF target.
> 
> The other option (B) is to be explicit and say
> #if defined(support_sframe_p) && defined(OBJ_ELF)
> 
> Also, we can absolutely just use (option C):
> 
> #ifdef OBJ_ELF
>    if (support_sframe_p ()...)
>    {
>      ...
>    }
>    else
> #endif

... I think we would be better off avoiding use of OBJ_ELF where an
alternative exists, just for the (seemingly remote at this point) case
of non-ELF wanting to gain support as well.

Jan

> the last one  (option C) has to to be with the initial definition in place:
> /* Targets which support SFrame format will define this and return true.  */
> #ifndef support_sframe_p
> # define support_sframe_p() false
> #endif
> 
>