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

[Richard Earnshaw (lists)]

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.