[PATCH v5 05/20] gas: implement parsing of object attributes v2

[Matthieu Longo]

On 2025-07-10 14:32, Richard Earnshaw (lists) wrote:
> On 10/07/2025 11:15, Matthieu Longo wrote:
>> On 2025-07-10 10:57, Richard Earnshaw (lists) wrote:
>>> On 09/07/2025 16:05, Matthieu Longo wrote:
>>>> On 2025-07-09 14:55, Richard Earnshaw (lists) wrote:
>>>>> On 07/07/2025 17:49, Matthieu Longo wrote:
>>>>>> diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
>>>>>> index 1ccbcc40d4d..acf93a3cee5 100644
>>>>>> --- a/bfd/elf-attrs.c
>>>>>> +++ b/bfd/elf-attrs.c
>>>>>> +
>>>>>> +/* Return True if the given subsection name is part of the reserved "gnu-testing"
>>>>>> +   namespace.  */
>>>>>> +static bool
>>>>>> +gnu_testing_namespace (const char *subsec_name)
>>>>>> +{
>>>>>> +  return strncmp ("gnu-testing", subsec_name, 11) == 0;
>>>>>
>>>>> What's the reason for using strncmp here rather than strcmp?
>>>>>
>>>>
>>>> The subsection name can be longer than "gnu-testing" (see example in the tests), so we need to compare at most len("gnu-testing"), i.e. 11 bytes.
>>>> In my understanding, you cannot do this with strcmp.
>>>
>>> No, you can't but this would match a section called "gnu-testing-for-christmas", which doesn't really seem correct: it's not the gnu-testing subsection.  If subsec_name isn't null terminated, then it would be reasonable to use strncmp, but then you'd want to pass the length limit as an argument to this function.
>>>
>>>
>>> But maybe I've misunderstood how the naming scheme works here.  Perhaps this *is* supposed to be a prefix to longer strings.  But in that case:
>>> 1) I think the comment could be a bit clearer.
>>> 2) I think you should check that subsec_name[11] is not '\0', since then the string is not a prefix.
>>>
>>>
>>> R.
>>
>> Yes, this is supposed to be a prefix to longer strings, but at the same time, "gnu-testing\0" is acceptable too if for the test case, one gnu-testing subsection is enough, and people don't care about a merge policy (i.e. no linker test).
>>
>> There is already a comment about this at the top of the file, and in the commit message:
>>
>> In order to test this generic logic, AArch64's use cases are not offering enough coverage, so a "GNU testing namespace" which corresponds to the name of the subsection was introduced. It follows the following pattern:
>>       gnu-testing-<XXXXXX>-MERGE-<POLICY>
>>
>> - <XXXXXX>: an arbitrary name for your testing subsection.
>> - <POLICY>: the name of the merging policy to apply on the values in the subsection. The currently supported merge policy are:
>>           * -MERGE-AND: bitwise AND applied on numerical values.
>>           * -MERGE-OR: bitwise OR applied on numerical values.
>>           * -MERGE-ADD: concatenates strings together with a '+' in-between.
>>           Note: "-MERGE-ADD" does not make really sense, and will very likely never be used for a real merge. Its only purpose is to test the correct handling of merges with strings.
>>
>> Matthieu
> 
> 
> Ok, perhaps just clarify the comment before the function then.  Something like:
> 
> /* Return True if the given subsection name is part of the reserved testing
>     namespace, ie SUBSEC_NAME begins with "gnu-testing".  */
> 
> R.

Fixed in the next revision.

Matthieu