[PATCH v0 09/15] bfd: add support for copying object attributes v2

[Matthieu Longo]

On 2025-03-20 15:05, Richard Ball wrote:
> 
> On 3/10/25 17:51, Matthieu Longo wrote:
>> ---
>>   bfd/elf-attrs.c | 35 ++++++++++++++++++++++++++++++++---
>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
>> index bd8ae56678a..12a81181c11 100644
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -541,9 +541,8 @@ bfd_elf_add_obj_attr_int_string (bfd *abfd, int vendor, unsigned int tag,
>>     return elf_add_obj_attr_int_string (abfd, vendor, tag, i, s, NULL);
>>   }
>>   
>> -/* Copy the object attributes from IBFD to OBFD.  */
>> -void
>> -_bfd_elf_copy_obj_attributes (bfd *ibfd, bfd *obfd)
>> +static void
>> +_bfd_elf_copy_obj_attributes_v1 (bfd *ibfd, bfd *obfd)
>>   {
>>     obj_attribute *in_attr;
>>     obj_attribute *out_attr;
>> @@ -604,6 +603,36 @@ _bfd_elf_copy_obj_attributes (bfd *ibfd, bfd *obfd)
>>       }
>>   }
> 
> Why was the comment for this function removed in the renaming?
> 

The comment was moved to the new _bfd_elf_copy_obj_attributes (see below).

>>   
>> +static void
>> +_bfd_elf_copy_obj_attributes_v2 (bfd *ibfd, bfd *obfd)
>> +{
>> +  if (bfd_get_flavour (ibfd) != bfd_target_elf_flavour
>> +      || bfd_get_flavour (obfd) != bfd_target_elf_flavour)
>> +    return;
> 
> This if statement could be moved to the bfd_elf_copy_obj_attributes to avoid code duplication.
> 

Good catch. Fixed in the new revision.

>> +
>> +  obj_attr_subsection_list *in_attr_subsecs = &elf_obj_attr_subsections (ibfd);
>> +  obj_attr_subsection_list *out_attr_subsecs = &elf_obj_attr_subsections (obfd);
>> +
>> +  for (obj_attr_subsection_v2* isubsec = in_attr_subsecs->first_;
>> +       isubsec != NULL;
>> +       isubsec = isubsec->next)
>> +    {
>> +      obj_attr_subsection_v2* osubsec =
>> +	_bfd_elf_obj_attr_subsection_v2_copy (isubsec);
> 
> Naming convention bfd_elf_copy_obj_attributes_v2 vs _bfd_elf_obj_attr_subsection_v2_copy, perhaps _bfd_elf_copy_obj_attr_subsection_v2?
> 

The naming of this function is different because 
_bfd_elf_copy_obj_attributes_v2 is semantically different from 
_bfd_elf_obj_attr_subsection_v2_copy.

_bfd_elf_copy_obj_attributes_v2 copies OAv2 in the input object to the 
output objects.
_bfd_elf_obj_attr_subsection_v2_copy copies a OAv2 subsection object. It 
is basically equivalent to a copy constructor in a OAv2 subsection class 
if we were developing in C++.

I tried to follow a naming that corresponds to [module]_[target_format]_ 
[object class]_method for the second case.
This is what I followed in most of the places. If it is not the case, 
this is others places that should be fixed :)

>> +      LINKED_LIST_APPEND(obj_attr_subsection_v2) (out_attr_subsecs, osubsec);
>> +    }
>> +}
>> +
>> +/* Copy the object attributes from IBFD to OBFD.  */
>> +void
>> +_bfd_elf_copy_obj_attributes (bfd *ibfd, bfd *obfd)
>> +{
>> +  if (get_elf_backend_data (ibfd)->obj_attrs_version == 2)
>> +    _bfd_elf_copy_obj_attributes_v2 (ibfd, obfd);
>> +  else
>> +    _bfd_elf_copy_obj_attributes_v1 (ibfd, obfd);
>> +}
>> +
> 
> Missing function comment for this function.
> 

The doc is there, so not sure what you meant here.
What do you mean by function comment ?

>>   /* Determine whether a GNU object attribute tag takes an integer, a
>>      string or both.  */
>>   static int