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

[Richard Ball]

On 3/20/25 15:44, Matthieu Longo wrote:
> 
> 
> 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).
> 

Ah, I see. I think I would still be in favour of a comment above the v1 and v2 functions as well.

>>>   +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 :)
> 

I see what you're saying, I think trying to come up with a clearer naming structure here would be great, because from the names of these functions they can be hard to differentiate.
I'm not completely sure on what though, and I suspect this is possibly an issue with the pre-existing function names. At any rate, not a huge issue.

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

I meant no comment above the v2 function I put this in the wrong place apologies.

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