Re: [PATCH] elf: Support DT_RELR relative relocation format [BZ #27924]

[Jan Beulich <jbeulich@suse.com>]

On 13.10.2021 08:18, Fāng-ruì Sòng wrote:
> On Tue, Oct 12, 2021 at 11:13 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>>
>> On Tue, Oct 12, 2021 at 11:00 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 12.10.2021 18:07, Fāng-ruì Sòng wrote:
>>>> On Tue, Oct 12, 2021 at 7:10 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>
>>>>> On Tue, Oct 12, 2021 at 1:18 AM Jan Beulich via Libc-alpha
>>>>> <libc-alpha@sourceware.org> wrote:
>>>>>>
>>>>>> On 11.10.2021 20:43, Fāng-ruì Sòng wrote:
>>>>>>> On Mon, Oct 11, 2021 at 12:48 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 08.10.2021 08:57, Fangrui Song via Binutils wrote:
>>>>>>>>> --- a/elf/dynamic-link.h
>>>>>>>>> +++ b/elf/dynamic-link.h
>>>>>>>>> @@ -192,6 +192,33 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
>>>>>>>>>  #  define ELF_DYNAMIC_DO_RELA(map, scope, lazy, skip_ifunc) /* Nothing to do.  */
>>>>>>>>>  # endif
>>>>>>>>>
>>>>>>>>> +# define ELF_DYNAMIC_DO_RELR(map)                                          \
>>>>>>>>> +  do {                                                                             \
>>>>>>>>> +    ElfW(Addr) l_addr = (map)->l_addr, base = 0, start;                            \
>>>>>>>>> +    const ElfW(Relr) *r = 0, *end = 0;                                             \
>>>>>>>>> +    if (!(map)->l_info[DT_RELR])                                           \
>>>>>>>>> +      break;                                                               \
>>>>>>>>> +    start = D_PTR((map), l_info[DT_RELR]);                                 \
>>>>>>>>> +    r = (const ElfW(Relr) *)start;                                         \
>>>>>>>>> +    end = (const ElfW(Relr) *)(start + (map)->l_info[DT_RELRSZ]->d_un.d_val); \
>>>>>>>>> +    for (; r < end; ++r) {                                                 \
>>>>>>>>> +      ElfW(Relr) entry = *r;                                               \
>>>>>>>>> +      if ((entry & 1) == 0) {                                                      \
>>>>>>>>> +     *((ElfW(Addr) *)(l_addr + entry)) += l_addr;                          \
>>>>>>>>> +     base = entry + sizeof(ElfW(Addr));                                    \
>>>>>>>>> +     continue;                                                             \
>>>>>>>>> +      }                                                                            \
>>>>>>>>> +      ElfW(Addr) offset = base;                                                    \
>>>>>>>>> +      do {                                                                 \
>>>>>>>>> +     entry >>= 1;                                                          \
>>>>>>>>> +     if ((entry & 1) != 0)                                                 \
>>>>>>>>> +       *((ElfW(Addr) *)(l_addr + offset)) += l_addr;                       \
>>>>>>>>> +     offset += sizeof(ElfW(Addr));                                         \
>>>>>>>>> +      } while (entry != 0);                                                \
>>>>>>>>> +      base += (8 * sizeof(ElfW(Relr)) - 1) * sizeof(ElfW(Addr));           \
>>>>>>>>
>>>>>>>> While in line with the proposed spec additions I'm afraid the uses of
>>>>>>>> ElfW(Addr) here aren't universally correct: You assume that ELF
>>>>>>>> container type (size) expresses an aspect of the ABI. While this is
>>>>>>>> indeed the case for several arch-es, I think this has been a mistake.
>>>>>>>> IA-64, while meanwhile mostly dead, is (was) an example where 64-bit
>>>>>>>> code can validly live in a 32-bit ELF container (at least as far as
>>>>>>>> the psABI is concerned; I have no idea whether glibc actually
>>>>>>>> followed the spec). There's a separate ELF header flag indicating the
>>>>>>>> ABI, and hence the size of a pointer.
>>>>>>>
>>>>>>> Thanks for chiming in.
>>>>>>>
>>>>>>> As of ia64 buildability, it works for me:
>>>>>>>
>>>>>>> scripts/build-many-glibcs.py /tmp/glibc-many compilers ia64-linux-gnu
>>>>>>> mkdir -p out/ia64; cd out/ia64
>>>>>>> ../../configure --prefix=/tmp/glibc/ia64 --host=ia64-linux-gnu
>>>>>>> CC=/tmp/glibc-many/install/compilers/ia64-linux-gnu/bin/ia64-glibc-linux-gnu-gcc
>>>>>>> CXX=/tmp/glibc-many/install/compilers/ia64-linux-gnu/bin/ia64-glibc-linux-gnu-g++
>>>>>>> make -j 50
>>>>>>
>>>>>> I didn't suggest the build would fail. What I said is that I don't
>>>>>> think the code is correct there.
>>>>>>
>>>>>>> As of the actual functionality, ugh, I cannot find ia64 in my Debian
>>>>>>> testing's qemu-user-static package:( So I cannot test.
>>>>>>>
>>>>>>> That said, gold and LLD don't support ia64.
>>>>>>> If we have a concern that ia64 may not work, the GNU ld maintainers
>>>>>>> can simply not add ia64 support:)
>>>>>>
>>>>>> But you realize that I took ia64 only as example, as that's where
>>>>>> I know ABI (pointer size) and ELF container size aren't connected.
>>>>>> As per my looking at merely EF_MIPS_* in context of reading
>>>>>> Joseph's reply, it might be that MIPS is another such example. But
>>>>>> I lack sufficient knowledge of MIPS ...
>>>>>>
>>>>>
>>>>> The new code should be tested and verified on all supported
>>>>> targets.  That is another reason to implement this in binutils
>>>>> ld first.
>>>>
>>>> --pack-dyn-relocs=relr is well tested on arm, aarch64, and x86, and
>>>> works on popular arches like ppc64 as well.
>>>> For mips, it is no harm to keep the DT_RELR code path. Its
>>>> elf_machine_rel_relative is empty and it has no relative relocation
>>>> anyway.
>>>> I wish that our reasonable design decisions are not restricted by the
>>>> few retrocomputing architectures, especially when the concern is still
>>>> at the theoretical stage.
>>>
>>> For ia64 it's not theoretical at all, as long as you leave aside the
>>> fact the deprecation state of that architecture. I also have to admit
>>> that I have trouble seeing why the design can't be adjusted to fit
>>> original ELF intentions rather than (as said, imo bad) decisions
>>> taken by a few (popular) architectures. Besides adjusting the wording
>>> accordingly, all it takes for your implementation is to parameterize
>>> word (pointer) size.
>>
>> I re-read your first reply. Did you mean 64-bit small code model code
>> in an ELFCLASS32 ET_EXEC/ET_DYN file?
>> OK. I considered it as a size optimization for a different task.
>> "AArch64 and x86-64 define ILP32 ABIs and use ELFCLASS32, but
>> technically they can use ELFCLASS32 for small code model with regular
>> ABIs, if the kernel allows." (from
>> https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order)
>>
>> I assume that switching *((ElfW(Addr) *)(l_addr + entry)) += l_addr;
>> to `unsigned long` makes this use case work, though I doubt glibc's
>> ia64 implementation supports this as sysdeps/ia64/dl-machine.h uses
>> Elf64.
> 
> It won't. sysdeps/ia64/dl-machine.h:elf_machine_rela_relative writes
> to an Elf64_Addr.
> l_addr and many other representations assume that ElfW can not be an
> Elf32 container for a 64-bit architecture.

Okay, so maybe the code you have is fine within the constraints of glibc.
But I'd still hope for the text addition to the standard to have removed
the ABI implications from ELF container size, before it actually gets
merged.

Jan