diff mbox series

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

Message ID 20250310175131.1217374-10-matthieu.longo@arm.com
State New
Headers
Series AArch64 AEABI build attributes (a.k.a. object attributes v2) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Matthieu Longo March 10, 2025, 5:51 p.m. UTC 
  ---
 bfd/elf-attrs.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Comments

Richard Ball March 20, 2025, 3:05 p.m. UTC | #1 
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?

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

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

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

>  /* Determine whether a GNU object attribute tag takes an integer, a
>     string or both.  */
>  static int
Matthieu Longo March 20, 2025, 3:44 p.m. UTC | #2 
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
Richard Ball March 20, 2025, 4:25 p.m. UTC | #3 
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
>
Matthieu Longo March 21, 2025, 4:58 p.m. UTC | #4 
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
>>
diff mbox series

Patch

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)
     }
 }
 
+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;
+
+  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);
+      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);
+}
+
 /* Determine whether a GNU object attribute tag takes an integer, a
    string or both.  */
 static int