[PATCH 03/12] libsframe: refactor code for dumping section flags

[Indu Bhagat]

On 6/2/25 3:03 AM, Jens Remus wrote:
> On 29.05.2025 18:38, Indu Bhagat wrote:
>> On 5/28/25 1:52 AM, Jens Remus wrote:
>>> On 28.05.2025 07:40, Indu Bhagat via Binutils wrote:
>>>> To prepare code for accommodating new flag additions easily as the
>>>> format evolves.
>>>
>>>> diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
> 
>>>>    static void
>>>> -dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
>>>> +dump_sframe_header_flags (sframe_decoder_ctx *sfd_ctx)
>>>>    {
>>>
>>>> +  /* PS: Keep SFRAME_FLAGS_STR_MAX_LEN in sync if adding more members to
>>>> +     this array.  */
>>>> +#define SFRAME_FLAGS_STR_MAX_LEN 50
>>>
>>> I would prefer if we could get rid of this define, by using an approach
>>> similar to the one found in binutils/objdump.c, dump_section_header.
>>>
>>>> +#define MAX_NUM_FLAGS 2
>>>
>>> #define MAX_NUM_FLAGS (sizeof (flags_helper) / sizeof (flags_helper[0]))
> 
>>> Using the approach from binutils/objdump.c, dump_section_header none of
>>> this would be required.
>>>
>>
>> Hmm. OK I think something like the following may be more agreeable ?
> 
> Thanks!  Looks good to me.
> 
>>
>>    const char *prefix = "Flags: ";
>>    const char *comma = ",";
> 
> Nit: Could be removed, as comma never gets re-assigned. (see below)
> 
>>
>>    flags = sframe_decoder_get_flags (sfd_ctx);
>>    if (!flags)
>>      {
>>        printf ("%11sNONE\n", prefix);
>>        return;
>>      }
>>
>> #define PRINT_FLAG(x, y) \
>>    if (flags & x) \
>>      { flags = (flags & ~x); \
>>        printf ("%11s%s%s\n", prefix, y, flags ? comma : ""); \
> 
>        printf ("%11s%s%s\n", prefix, y, flags ? "," : ""); \
> 
> That would make clear, that a trailing comma or nothing gets printed and
> never something else.
> 

Sure. Done.

>>        prefix = " "; \
>>      }
>>
>>    PRINT_FLAG (SFRAME_F_FDE_SORTED, "SFRAME_F_FDE_SORTED");
>>    PRINT_FLAG (SFRAME_F_FRAME_POINTER, "SFRAME_F_FRAME_POINTER");
>> #undef PRINT_FLAG
> 
> 
> Would it make sense to print any remaining "unknown" flags in hexadecimal?
> 
>    if (flags)
>      printf ("%11s%#x\n", prefix, flags);
> 

There are sanity checks for SFrame header in 
sframe_header_sanity_check_p (). The intention has been to enforce the 
absence of any unknown flags and error out at decode time, so adding the 
code to dump them will not be useful.

Thanks for reviewing.