[PATCH v1 1/5] ld: fix segfault caused by untagged stub sections

[Jan Beulich]

On 13.10.2025 19:01, Matthieu Longo wrote:
> On 2025-10-13 16:30, Jan Beulich wrote:
>> On 13.10.2025 17:22, Matthieu Longo wrote:
>>> On 2025-09-26 14:13, Jan Beulich wrote:
>>>> On 18.09.2025 17:06, Matthieu Longo wrote:
>>>>> --- a/bfd/bfd-in2.h
>>>>> +++ b/bfd/bfd-in2.h
>>>>> @@ -834,6 +834,12 @@ typedef struct bfd_section
>>>>>        const char *linked_to_symbol_name;
>>>>>      } map_head, map_tail;
>>>>>    
>>>>> +  /* Indicate that the section contains branch veneers.  This is used when
>>>>> +     support for non-contiguous memory regions is enabled.  The veneers have
>>>>> +     to be allocated to the same memory region as the code they are refered
>>>>> +     by, i.e. they cannot be moved to a subsequent memory region.  */
>>>>> +  bool veneer;
>>>>
>>>> Why would you put a bool between two pointer-sized fields, thus introducing
>>>> yet more padding, when in fact existing padding could be used. There's a set
>>>> of bitfields further up from here, and you could simply add a single-bit
>>>> field there, for example. Generic code changes are okay with that change,
>>>> but arch-specific ones will need arch maintainer approval.
>>>
>>> Here is the new attribute as a flag:
>>>
>>> diff --git a/bfd/section.c b/bfd/section.c
>>> index 5f0cf6e71cb..f110ed77363 100644
>>> --- a/bfd/section.c
>>> +++ b/bfd/section.c
>>> @@ -379,6 +379,12 @@ CODE_FRAGMENT
>>>    .     when memory read flag isn't set. *}
>>>    .#define SEC_COFF_NOREAD            0x40000000
>>>    .
>>> +.  {* Indicate that the section contains branch veneers.  This is used when
>>> +.     support for non-contiguous memory regions is enabled.  The
>>> veneers have
>>> +.     to be allocated to the same memory region as the code they are
>>> refered
>>> +.     by, i.e. they cannot be moved to a subsequent memory region.  *}
>>> +.#define SEC_VENEER                 0x80000000
>>> +.
>>>    .  {*  End of section flags.  *}
>>>    .
>>>    .  {* Some internal packed boolean fields.  *}
>>>
>>>
>>> With the tagging of the stub section on AArch64:
>>>
>>> diff --git a/ld/emultempl/aarch64elf.em b/ld/emultempl/aarch64elf.em
>>> index 91d58d8fe5a..b00c9460da2 100644
>>> --- a/ld/emultempl/aarch64elf.em
>>> +++ b/ld/emultempl/aarch64elf.em
>>> @@ -193,7 +193,7 @@ elf${ELFSIZE}_aarch64_add_stub_section (const char
>>> *stub_sec_name,
>>>      lang_output_section_statement_type *os;
>>>      struct hook_stub_info info;
>>>
>>> -  flags = (SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_CODE
>>> +  flags = (SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_CODE | SEC_VENEER
>>>              | SEC_HAS_CONTENTS | SEC_RELOC | SEC_IN_MEMORY | SEC_KEEP);
>>>      stub_sec = bfd_make_section_anyway_with_flags (stub_file->the_bfd,
>>>                                                    stub_sec_name, flags);
>>>
>>> Same thing on AArch32 and PowerPC.
>>>
>>> Is this what you suggested ?
>>
>> No, sorry. I said "bit field", not "section flag". I'd rather not see you
>> use the last available section flag. Right below where you put the new
>> #define is a comment "Some internal packed boolean fields" - that's where
>> I thought your new flag would go.
> 
> What about the below ?

Yes, with one cosmetic request though:

> diff --git a/bfd/section.c b/bfd/section.c
> index 5f0cf6e71cb..424e53029d9 100644
> --- a/bfd/section.c
> +++ b/bfd/section.c
> @@ -428,6 +428,12 @@ CODE_FRAGMENT
>   .  {* Nonzero if section contents should not be freed.  *}
>   .  unsigned int alloced:1;
>   .
> +.  {* Indicate that the section contains branch veneers.  This is used when
> +.     support for non-contiguous memory regions is enabled.  The 
> veneers have
> +.     to be allocated to the same memory region as the code they are 
> refered
> +.     by, i.e. they cannot be moved to a subsequent memory region.  *}
> +.  unsigned int veneer : 1;
> +.
>   .  {* Bits used by various backends.  The generic code doesn't touch
>   .     these fields.  *}
>   .
> @@ -723,11 +729,11 @@ INTERNAL
>   .  {* segment_mark, sec_info_type, use_rela_p, mmapped_p, alloced, 
> *}        \
>   .     0,            0,             0,          0,         0,           \
>   .                                                                      \
> -.  {* sec_flg0, sec_flg1, sec_flg2, sec_flg3, sec_flg4, sec_flg5,    *} 

Please can these all stay on one line, while you ...

>         \
> +.  {* veneer, sec_flg0, sec_flg1, sec_flg2, sec_flg3, sec_flg4,      *} 

... add a new line to put the new initializer on? (Overall the macro likely
wants switching to C99 dedicated initializers, at which point the comments
can all go away, as can the many 0s and NULL-s. But that's for another day,
I guess.)

Jan