[PATCH v9 09/19] bfd: parse Object Attributes v2's section in input object files

[Matthieu Longo]

On 07/11/2025 08:33, Jan Beulich wrote:
> On 06.11.2025 14:35, Matthieu Longo wrote:
>> On 31/10/2025 10:25, Jan Beulich wrote:
>>> On 01.09.2025 18:56, Matthieu Longo wrote:
>>>> +  const uint32_t F_SUBSECTION_LEN = sizeof(uint32_t);
>>>> +  const uint32_t F_SUBSECTION_COMPREHENSION = sizeof(uint8_t);
>>>> +  const uint32_t F_SUBSECTION_ENCODING = sizeof(uint8_t);
>>>> +  /* The minimum subsection length is 7: 4 bytes for the length itself, and 1
>>>> +     byte for an empty NUL-terminated string, 1 byte for the comprehension,
>>>> +     1 byte for the encoding, and no vendor-data.  */
>>>> +  const uint32_t F_MIN_SUBSECTION_DATA_LEN
>>>> +    = F_SUBSECTION_LEN + 1 /* for '\0' */
>>>> +      + F_SUBSECTION_COMPREHENSION + F_SUBSECTION_ENCODING;
>>>> +
>>>> +  /* Similar to the issues reported in PR 17531, we need to check all the sizes
>>>> +     and offsets as we parse the section.  */
>>>> +  if (max_read < F_MIN_SUBSECTION_DATA_LEN)
>>>> +    {
>>>> +      _bfd_error_handler (_("%pB: error: attributes subsection ends "
>>>> +			    "prematurely"), abfd);
>>>> +      goto error;
>>>> +    }
>>>> +
>>>> +  const uint32_t subsection_len = bfd_get_32 (abfd, cursor);
>>>> +  total_read += F_SUBSECTION_LEN;
>>>> +  cursor += F_SUBSECTION_LEN;
>>>> +  if (subsection_len > max_read)
>>>> +    {
>>>> +      _bfd_error_handler (_("%pB: error: bad subsection length (%u > max=%lu)"),
>>>> +			  abfd, subsection_len, max_read);
>>>> +      goto error;
>>>> +    }
>>>> +  else if (subsection_len < F_MIN_SUBSECTION_DATA_LEN)
>>>> +    {
>>>> +      _bfd_error_handler (_("%pB: error: subsection length of %u is too small"),
>>>> +			  abfd, subsection_len);
>>>> +      goto error;
>>>> +    }
>>>> +
>>>> +  const size_t MAX_SUBSECTION_NAME_LEN
>>>> +    = subsection_len - F_SUBSECTION_LEN
>>>> +      - F_SUBSECTION_COMPREHENSION - F_SUBSECTION_ENCODING;
>>>
>>> Other all-capitals identifiers are kind-of #define-s of build-time constants.
>>> This one isn't a constant, though, so the use of such an identifier feels a
>>> little misleading.
>>>
>>
>> Fixed in the next revision.
>>
>>>> +  const size_t subsection_name_len
>>>> +    = strnlen ((char *) cursor, MAX_SUBSECTION_NAME_LEN);
>>>
>>> I think I mentioned earlier that casts would better be used sparingly. Here we
>>> could get away without, if we used memchr() instead. Thoughts?
>>
>> Is it what you had in mind ?
> 
> Looks like so.
> 
>>>> +  const char *vendor_name = get_elf_backend_data (abfd)->obj_attrs_vendor;
>>>> +  obj_attr_subsection_scope_v2 scope
>>>> +    = (strncmp (subsection_name, vendor_name, strlen (vendor_name)) == 0
>>>> +      ? OA_SUBSEC_PUBLIC
>>>> +      : OA_SUBSEC_PRIVATE);
>>>
>>> What is this following from? I'm in particular puzzled that there's no form of
>>> separator required between the part potentially matching the sub-section name
>>> and the rest. This way a sub-section XYZ would be public for vendor XY, which
>>> may or may not be what is intended. Personally I would have expected
>>> separation by e.g. some non-alphanumeric character.
>>
>> I discussed that with the person writing the specs, and he does not have
>> any objection to add a separator between the prefix and the core.
>>
>> So, in the next revision, the parser for a subsection directive only
>> accept [a-zA-Z][a-zA-Z0-9_]* for the subsection name, and '_' is
>> considered a separator.
> 
> Is '_' a good character to use as separator? It's pretty commonly used in
> identifiers. It's also unclear to me why subsection names would need
> constraining to alphanumeric plus underscore. (Sorry, likely lacking some
> background here.)
> 
> Jan

Whether '_' is a good character or not to use as a separator, we don't 
have much choice now since Clang has already implemented the feature by 
following the spec using '_', and already released it. Using '_' instead 
of '-' was making things easier for their parser from what I understood.
Even if the name is following the pattern of an identifier, is it really 
an issue ? What example do you have in mind ?

Matthieu