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

[Jan Beulich]

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:
>>> +/* Returns True if the given BFD is an ELF object with the current backend
>>
>> What's "current"?
>>
> 
> The "target" backend.
> Fixed in the next revision.
> 
>>> +   machine code, non-dynamic (i.e. not a shared library), and has sections.
>>
>> But an executable is okay? I.e. aren't after relocatable objects here?
>>
> 
> You're right, it is missing.
> I took this condition from _bfd_elf_link_setup_gnu_properties, and 
> EXEC_P is missing.
> 
> However, this raises one question in my head.
> Why would the GNU property code or object attribute one would have to 
> deal with executable ELF as an input file ? Shouldn't ld filter such an 
> executable before reaching this stage ?

Iirc it used to be permitted (maybe that has changed in the meantime), no
matter that ...

> Does it even make sense to add an executable as an input object for the 
> linker ?

... I can't answer this.

>>> +   False otherwise.
>>> +   Note: this function is a convenient encapsulation of the predicate used to
>>> +   search for objects containing object attributes in the list of BFDs.  */
>>> +static bool
>>> +bfd_is_non_dynamic_elf_object (struct bfd_link_info *info,
>>> +			       bfd *abfd)
>>> +{
>>> +  const struct elf_backend_data *output_bfd
>>> +    = get_elf_backend_data (info->output_bfd);
>>
>> "output_bed" may be a better name for this variable. It's hard to see though
>> why you have ...
>>
> 
> Fixed in the next revision.
> 
>>> +  unsigned int elfclass = output_bfd->s->elfclass;
>>> +  int elf_machine_code = output_bfd->elf_machine_code;
>>
>> ... three variables for one side ...
>>
> 
> The line is too long otherwise, that's why I splitted it.
> 
>>> +  return (bfd_get_flavour (abfd) == bfd_target_elf_flavour
>>> +	  && abfd->section_count != 0
>>> +	  && (abfd->flags & DYNAMIC) == 0
>>> +	  && elf_machine_code == get_elf_backend_data (abfd)->elf_machine_code
>>> +	  && elfclass == get_elf_backend_data (abfd)->s->elfclass);
>>
>> ... of the comparisons and none for the other. I would suggest to have "bed"
>> and "output_bed", and no further ones.
>>
> 
> Fixed in the next revision.
> 
>> As this function isn't generic, I'd recommend dropping (or replacing) its
>> bfd_ prefix. (This likely applies elsewhere as well.)
>>
> 
> The issue, if I manually inlining this code inside 
> bfd_linear_search_one_with_build_attributes, is that the filtering 
> condition is really big and makes the purpose of this loop obscure.

I didn't ask for inlining of the code, though.

> I renamed it to elf_may_contain_obj_attrs ().
> 
> Then this loop is relatively readable :)
> 
> for (bfd *abfd = info->input_bfds; abfd != NULL; abfd = abfd->link.next)
>      if (elf_may_contain_obj_attrs (info, abfd))
>        {
> 	res.pbfd = abfd;
> 	if (bfd_has_build_attributes (abfd, &res))
> 	  break;
>        }

Looks okay at the first glance.

>>> +/* Report required attribute A_ABFD mismatching with A_REF.  */
>>> +static void
>>> +report_mismatching_required_obj_attr (struct bfd_link_info *info,
>>> +				      bfd *ref_bfd,
>>> +				      bfd *abfd,
>>> +				      obj_attr_subsection_v2 *s_ref,
>>> +				      obj_attr_v2 *a_ref,
>>> +				      obj_attr_v2 *a_abfd)
>>> +{
>>> +  const struct elf_backend_data *be = get_elf_backend_data (abfd);
>>> +  const char* tag_s = obj_attr_v2_tag_to_string (be, s_ref->name, a_ref->tag);
>>> +  if (s_ref->encoding == OA_ENC_ULEB128)
>>> +    {
>>> +      if (tag_s)
>>> +	info->callbacks->einfo (
>>> +	  _("%X%pB, %pB: error: mismatching values 0x%x and 0x%x for "
>>
>> In case I didn't say so elsewhere already: Please prefer %#x over 0x%x.
>>
> 
> If I change it to %#x, it crashes due to a SEGFAULT with the stack below:
> 
> (gdb) bt
> #0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:76
> #1  0x00007ffff7c8626a in __GI__IO_fputs (str=0x0, fp=0x7ffff7e044e0 
> <_IO_2_1_stderr_>) at ./libio/iofputs.c:33
> #2  0x00005555555baf34 in vfinfo (fp=0x7ffff7e044e0 <_IO_2_1_stderr_>, 
> fmt=0x55555596d243 "' in subsection '%s'\n", ap=0x7fffffffd630, 
> is_warning=true) at ../../ld/ldmisc.c:527
> #3  0x00005555555bb36e in einfo (fmt=0x55555596d1f0 "%X%pB, %pB: error: 
> mismatching values %#x and %#x for required object attribute '%s' in 
> subsection '%s'\n") at ../../ld/ldmisc.c:608
> #4  0x000055555566dfc8 in report_mismatching_required_obj_attr 
> (info=0x5555559d7ce0 <link_info>, ref_bfd=0x555555a14dc0, 
> abfd=0x555555a17160, s_ref=0x555555a19590, a_ref=0x555555a19610, 
> a_abfd=0x555555a1fdf0)
>      at ../../bfd/elf-attrs.c:1077
> #5  0x000055555566e20c in oav2_subsection_perfect_match 
> (info=0x5555559d7ce0 <link_info>, ref_bfd=0x555555a14dc0, 
> abfd=0x555555a17160, s_ref=0x555555a19590, s_abfd=0x555555a1fd70) at 
> ../../bfd/elf-attrs.c:1139
> 
> It does not seem to me that the support for "#" was implemented in vfinfo().

Hmm, I'm sorry for the bad request then. printf()-like functions which aren't
really printf()-like are problematic imo, but of course that's entirely
unrelated top your work here.

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

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

>>> @@ -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).

>> Once again an overly large patch, close to impossible to sensibly review.
> 
> This is a big patch indeed.
> How would you have splitted it in smaller pieces that still make sense ?
> I am interested in your suggestions. Such a situation, where I introduce 
> a lot of code, might occur again. Who knows ? :P

Well, giving suggestions on this matter is pretty hard. You know how the
code is structured much better than I do. Hence you're in a far better
position to determine possible split boundaries.

Jan