[PATCH v8 12/19] Merge of Object Attributes v2 during linkage (generic logic)

[Matthieu Longo]

On 2025-09-01 08:37, Jan Beulich wrote:
> On 29.08.2025 18:37, Matthieu Longo wrote:
>> On 2025-08-22 10:50, Jan Beulich wrote:
>>> On 15.07.2025 13:39, Matthieu Longo wrote:
>>>> +/* Merge object attributes from object file ABFD into REF_BFD.  */
>>>> +static bool
>>>> +oav2_subsections_merge (struct bfd_link_info *info, bfd *ref_bfd, bfd *abfd)
>>>> +{
>>>> +  bool success = true;
>>>> +  obj_attr_subsection_list *out_frozen_subsecs
>>>> +    = &elf_obj_attr_subsections (info->output_bfd);
>>>> +  obj_attr_subsection_list *abfd_subsecs = &elf_obj_attr_subsections (abfd);
>>>> +  obj_attr_subsection_list *ref_subsecs = &elf_obj_attr_subsections (ref_bfd);
>>>> +
>>>> +  obj_attr_subsection_v2 *s_frozen_first = out_frozen_subsecs->first;
>>>> +  obj_attr_subsection_v2 *s_abfd = abfd_subsecs->first;
>>>> +  obj_attr_subsection_v2 *s_ref = ref_subsecs->first;
>>>> +
>>>> +  /* Translate object attributes from abfd to GNU properties if they have an
>>>> +     equivalence.  */
>>>> +  _bfd_elf_translate_relevant_obj_attrs_to_gnu_props (abfd);
>>>
>>> How does this call fit here? Below you're only dealing with attributes,
>>> afaict.
>>
>> ABFD is an input file. It might have either GNU properties only, object
>> attributes only, or both.
>>
>> Before the merge of object attributes occur, we need to make sure that
>> the GNU properties are translated to their object attributes equivalents
>> (if they exists).
>>
>> In the same way, before the merge of GNU properties, we need to make
>> sure that the object attributes are translated to their GNU properties
>> equivalents (if they exists).
>>
>> In this implementation, the merge of object attributes always occurs
>> before the merge of GNU properties.
> 
> May I then ask that you extend the comment some, to mention not only what
> is done, but also why it's done here?
> 

ABFD is an input file that may contain GNU properties, object 
attributes, or both.  Before merging object attributes, we must first 
translate any GNU properties into their equivalent object attributes (if 
such equivalents exist) since they may not already be present.

Note: object attributes are always merged before GNU properties. 
Ideally, there would be a single internal representation, with an 
abstraction level flexible enough to capture both GNU properties and 
object attributes without loss.  In such a design, merge order would be 
irrelevant, and translation would occur only at the I/O boundaries 
during deserialization of GNU properties and object attributes.


Is this clear enough ?

>>>> +      else /* (subsec->encoding == NTBS)  */
>>>> +	{
>>>> +	  if (strcmp (a->vals.string_val, a->next->vals.string_val) != 0)
>>>
>>> Code in obj_attr_v2_tag_merge_ADD() suggests that vals.string_val can be NULL,
>>> in which case it would be invalid to pass to strcmp().
>>
>> oav2_file_scope_merge_subsections(), which calls oav2_compact_tags(), is
>> called before oav2_merge_attrs().
>> It compacts the content of subsections before the merge process starts,
>> because the merge process has for the following requirements on an input
>> object file:
>> - the subsections have to be sorted in alphabetical order.
>> - the attributes inside a subsection have to be sorted in ascending order.
>> - the subsections and their attributes have to be unique.
>>
>> The value of a string attribute can only be NULL if this attribute was
>> created by the merge process.
>> Since this code runs before the merge process, no worry about the string
>> being NULL and strcmp() can work assuming that the string will never be
>> NULL.
> 
> As that's far from obvious here, may I ask that you add a comment and maybe
> also an assertion?
> 

What about this ?

/* For NTBS encoding, ensure that the string value of the attributes is 
  not NULL.
    Note: a string attribute can only be NULL if it was created during 
the merge process.  Since tag compaction occurs before merging begins, 
this assertion guarantees that the function is never called in a context 
where the assumption does not hold.  */
   BFD_ASSERT
     (subsec->encoding == OA_ENC_ULEB128 ||
      (a->val.string_val != NULL && a->next->val.string_val != NULL));

>>>> @@ -1146,9 +2619,9 @@ oav2_parse_subsection (bfd *abfd,
>>>>        ? OA_SUBSEC_PUBLIC
>>>>        : OA_SUBSEC_PRIVATE;
>>>>    
>>>> -  obj_attr_subsection_v2 *subsec =
>>>> -    _bfd_elf_obj_attr_subsection_v2_init (subsection_name, scope, optional_raw,
>>>> -					  attr_type_raw);
>>>> +  obj_attr_subsection_v2 *subsec
>>>> +    = _bfd_elf_obj_attr_subsection_v2_init (subsection_name, scope,
>>>> +					    optional_raw, attr_type_raw);
>>>>      while (cursor < end)
>>>>        {
>>>>          BufferReadOp_t op_ = oav2_parse_attr (abfd, cursor, end, attr_type_raw);
>>>
>>> Misplaced format adjustment?
>>
>> The format looks good, the issue is probably triggered by the diff.
>> I changed the formatting to the below, maybe it will be better.
>>
>>     *subsec = _bfd_elf_obj_attr_subsection_v2_init
>>       (subsection_name, scope, comprehension_raw, value_encoding);
> 
> I didn't ask that you do things differently. My comment was about you
> applying a format change here which likely should be got right when the
> code is introduced, not by touching the code here (again) for no other
> reason than formatting (unless I'm overlooking something).
> 

I fixed the formatting in the patch the introduced the code,
i.e. [PATCH v8 09/19] bfd: parse Object Attributes v2's section in input 
object files

Matthieu