[PATCH v8 06/19] bfd: write Object Attributes v2

[Matthieu Longo]

On 2025-08-08 13:23, Jan Beulich wrote:
> On 15.07.2025 13:39, Matthieu Longo wrote:
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -131,13 +131,56 @@ vendor_obj_attrs_v1_size (bfd *abfd, int vendor)
>>   }
>>   
>>   static bfd_vma
>> -bfd_elf_obj_attrs_v1_size (bfd *abfd)
>> +oav1_section_size (bfd *abfd)
>>   {
>>     bfd_vma size = 0;
>>     size = vendor_obj_attrs_v1_size (abfd, OBJ_ATTR_PROC);
>>     size += vendor_obj_attrs_v1_size (abfd, OBJ_ATTR_GNU);
>>     if (size > 0)
>> -    size += sizeof(uint8_t); /* <format-version: ‘A’>  */
>> +    size += sizeof (uint8_t); /* <format-version: uint8>  */
>> +  return size;
>> +}
>> +
>> +/* Return the size of a single attribute.  */
>> +static bfd_vma
>> +oav2_attr_size (obj_attr_v2 *attr, obj_attr_encoding_v2 type)
>> +{
>> +  bfd_vma size;
>> +
>> +  size = uleb128_size (attr->tag);
> 
> Have this be the initializer of the variable?
> 

Fixed in the next revision.

>> +  if (type == OA_ENC_ULEB128)
>> +    size += uleb128_size (attr->vals.uint_val);
>> +  if (type == OA_ENC_NTBS)
>> +    size += strlen (attr->vals.string_val) + 1; /* +1 for '\0'.  */
> 
> Use switch()?
> 

Fixed in the next revision.

>> +  return size;
>> +}
>> +
>> +/* Return the size of a subsection.  */
>> +static bfd_vma
>> +oav2_subsection_size (obj_attr_subsection_v2 *subsec)
>> +{
>> +  bfd_vma size = sizeof (uint32_t); /* <subsection-length: uint32>  */
>> +  size += strlen (subsec->name) + 1; /* <subsection-name: NTBS>  so +1 for '\0'.  */
>> +  size += 2 * sizeof (uint8_t); /* <optional: uint8> <encoding: uint8>  */
>> +  /* <attribute>*  */
>> +  for (obj_attr_v2 *attr = subsec->first;
>> +       attr != NULL;
>> +       attr = attr->next)
>> +    size += oav2_attr_size (attr, subsec->encoding);
>> +  return size;
>> +}
>> +
>> +/* Return the size of a build attributes section.  */
>> +static bfd_vma
>> +oav2_section_size (bfd *abfd)
>> +{
>> +  obj_attr_subsection_v2 *subsec = elf_obj_attr_subsections (abfd).first;
> 
> Some constification wants doing here and above (and perhaps also in the
> writing code). You don't alter what the pointers point to.
> 

Fixed in the next revision.

>> +static bfd_byte *
>> +oav2_write_subsection (bfd *abfd,
>> +		       obj_attr_subsection_v2 *subsec,
>> +		       bfd_byte *p)
>> +{
>> +  /* <subsection-length: uint32>  */
>> +  bfd_vma subsec_size = oav2_subsection_size (subsec);
>> +  bfd_put_32 (abfd, subsec_size, p);
>> +  p += sizeof (uint32_t);
>> +
>> +  /* <vendor-name: NTBS>  */
>> +  size_t vendor_name_size = strlen (subsec->name) + 1; /* +1 for '\0'.  */
>> +  memcpy (p, subsec->name, vendor_name_size);
>> +  p += vendor_name_size;
> 
> It's a pity that this can't simply be "p = stpcpy (...) + 1".
> 

Yes, but we need the size, so no choice.

>> +  /* -- <vendor-data: bytes> --  */
>> +  /* <optional: uint8>  */
>> +  p = write_uleb128 (p, subsec->optional);
>> +  /* <encoding: uint8>  */
>> +  p = write_uleb128 (p, obj_attr_encoding_v2_to_u8 (subsec->encoding));
> 
> The comments say "uint8" but you write uleb128? That's also not in line with
> oav2_subsection_size(). (For presently legitimate values there may be no
> difference, but the functions and comments want to all be consistent.)
> 

Yes, you are right. It was a shortcut to have everything written in one 
line but it is confusing.
Replaced by the code below in both cases.

   bfd_put_8 (abfd, ..., p);
   ++p;

>> +static void
>> +oav2_sort_subsections (obj_attr_subsection_list *plist)
>> +{
>> +  for (obj_attr_subsection_v2 *subsec = plist->first;
>> +       subsec != NULL;
>> +       subsec = subsec->next)
>> +    LINKED_LIST_MERGE_SORT(obj_attr_v2) (subsec, _bfd_elf_obj_attr_v2_cmp);
>> +
>> +  LINKED_LIST_MERGE_SORT(obj_attr_subsection_v2)
>> +    (plist, _bfd_elf_obj_attr_subsection_v2_cmp);
>> +}
> 
> Imo this would better move immediately ahead of bfd_elf_obj_attr_finalize_content(),
> such that related functions are close together. This then also puts
> oav2_write_subsection() closer to ...
> 

Fixed in the next revision.

>> +static void
>> +oav2_write_section (bfd *abfd, bfd_byte *buffer, bfd_vma size)
> 
> ... this caller of that function.
> 
>> +{
>> +  bfd_vma section_size = oav2_section_size (abfd);
>> +  if (section_size == 0)
>> +    return;
>> +
>> +  bfd_byte *p = buffer;
>> +
>> +  const struct elf_backend_data *be = get_elf_backend_data (abfd);
>> +  /* <format-version: uint8>  */
>> +  *(p++) = be->obj_attrs_version_enc (elf_obj_attr_version (abfd));
>> +
>> +  /* [ <subsection-length: uint32> <vendor-name: NTBS> <vendor-data: bytes> ]*  */
>> +  for (obj_attr_subsection_v2 *subsec = elf_obj_attr_subsections (abfd).first;
>> +       subsec != NULL;
>> +       subsec = subsec->next)
>> +    p = oav2_write_subsection (abfd, subsec, p);
>> +
>> +  /* We didn't overrun the buffer.  */
>> +  BFD_ASSERT (p <= buffer + size);
>> +  /* We wrote as many data as it was computed by
>> +     vendor_section_obj_attr_using_subsections_size().  */
>> +  BFD_ASSERT (section_size == (long unsigned int) (p - buffer));
>> +}
> 
> Looking at the call site, isn't the requirment that p == buffer + size and
> section_size == size? Or else, don't you need to pass back how much was
> actually populated?
> 

The parameter size is the maximum size we can write, it does not mean 
that it would be the amount of data that the function will actually write.
Even if section_size == size is true, this function should not be aware 
of this detail, and the assertions simply guarantee that:
- we wrote as much as data as we said we would write: section_size == 
(bfd_vma) (p - buffer)
- we didn't overrun the given buffer: p <= buffer + size

You are right. Using this approach, the function should return how much 
characters was actually written (even if it is unused).

Fixed in the next revision.

>> --- a/gas/write.c
>> +++ b/gas/write.c
>> @@ -1915,6 +1915,8 @@ create_obj_attrs_section (void)
>>     bfd_set_section_flags (s, SEC_READONLY | SEC_DATA);
>>     frag_now_fix ();
>>     char *p = frag_more (size);
>> +
>> +  bfd_elf_obj_attr_finalize_content (stdoutput);
> 
> For my own understanding: This isn't at risk of altering the size (due to
> e.g. different padding requirements), not even in the slightest?
> 

No, no padding is required so no risk of changing the size.

> Further, to match ...
> 
>>     bfd_elf_set_obj_attr_contents (stdoutput, (bfd_byte *)p, size);
> 
> ... its sibling, please use ..._contents also in the new function's name.

Fixed in the next revision.

> 
> Jan