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

[Jan Beulich]

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?

> +  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()?

> +  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.

> +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".

> +  /* -- <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.)

> +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 ...

> +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?

> --- 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?

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.

Jan