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

[Matthieu Longo]

On 2025-03-20 16:25, Richard Ball wrote:
> 
> 
> 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.
> 

Fixed in the next revision.

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

ACK

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