Runtime Loader Exported Symbols Address Size

[Chris Johns]

On 19/6/22 5:21 am, Joel Sherrill wrote:
> 
> 
> On Fri, Jun 17, 2022 at 8:13 PM Chris Johns <chrisj at rtems.org
> <mailto:chrisj at rtems.org>> wrote:
> 
>     On 18/6/22 10:08 am, Joel Sherrill wrote:
>     > On Fri, Jun 17, 2022, 2:28 PM Alex White <alex.white at oarcorp.com
>     <mailto:alex.white at oarcorp.com>
>     > <mailto:alex.white at oarcorp.com <mailto:alex.white at oarcorp.com>>> wrote:
>     >
>     >     Hi,
>     >
>     >     While testing libdl on AArch64 QEMU, we found a bug where the exported
>     >     symbol table appears to always use 32-bit values for addresses, but where
>     >     the exported symbols table is read in `rtems_rtl_symbol_global_add`, the
>     >     addresses are expected to be of size `sizeof(unsigned long)`.
>     >
>     >     This did not cause a problem on ARM since `sizeof(unsigned long)` is
>     4, but
>     >     with AArch64 `sizeof(unsigned long)` is 8. So it is trying to read the
>     >     address as 64 bits instead of 32.
>     >
>     >     The simple fix is to use an exact-width integer type - like
>     >     `sizeof(uint32_t)`. But this would not allow for 64-bit architectures
>     to use
>     >     the full address space. It would also break on 64-bit RISC-V (see below).
>     >     Perhaps we could have an ifdef to choose a 32 or 64-bit type based on the
>     >     architecture rather than relying on `sizeof(unsigned long)`?
>     >
>     >     It looks like there is an exception in the rtems-syms tool for 64-bit
>     RISC-V
>     >     to emit 64-bit addresses rather than 32-bit addresses. Is this the right
>     >     solution? Should we add another exception for AArch64?
>     >
>     > After having given this some thought, I think the code that is generated
>     and the
>     > parsing code in rtl-sym.c should be conditionalized on the GCC CPP predefine
>     > that indicates the size of a void *.
> 
>     Good idea. The symbol value is held as a `void*` in the symbol table so the key
>     is the generated asm size in rtems-syms.cpp matches the runtime `void*` size.
>     That code maybe conditional on the size selecting the correct variable size.
> 
>     > The code in rtl-sym.c should use a type definition for the type that the
>     sizeof
>     > is operating on. Using unsigned long is just not good enough. Both cases
>     should
>     > be fixed width types.
> 
>     Sure `unsigned long` may not be the best type to use. It was selected because it
>     happened change size with the arch but I am sure there are types that are better
>     suited. The type needs to be the size of a full address space pointer.
> 
> 
> Glad you hear you agree on the basic approach. The cpp predefine is 
> __SIZEOF_POINTER__ and  it should be 4 or 8. If both sides use that, then this
> should be permanently fixed.
> 
> 
>     > There also should be a very good comment above the parsing method in rtl-sym.c
>     > explaining where the file comes from that it is reading and why there is the
>     > need for a type which varies by architecture.
> 
>     Sure, happy to see comments added. Is this something you would like me to
>     handle?
> 
>     The generation and source of the symbols can be handled in a number of ways. I
>     have provided a tool that should cover most use cases however I can imagine
>     more, eg symbol filters for defined exported symbols. I documented things here:
> 
>     https://docs.rtems.org/branches/master/user/exe/loader.html#base-image-symbols
>     <https://docs.rtems.org/branches/master/user/exe/loader.html#base-image-symbols>
> 
>     Does that documentation help cover what you are looking for?
> 
> 
> I'll let Ryan and Alex answer that since they are fixing this. I just was 
> helping Ryan debug the parsing loop and realized it was chopping off the 
> first four bytes of every symbol after the first one. 

OK

>     > Based on our current understanding, I think this address is everything and
>     > leaves sufficient information and organization to cover all the cases in the
>     > future. Very likely without anyone actively having to fix anything.
>     Running into
>     > this issue again and again as architectures are added is not a good long-term
>     > solution.
> 
>     Agreed. Figuring out stable 64bit addressing in libdl is a good idea and
>     welcome. I think the main requirement is for the full address range to be
>     supported.
> 
>     > Chris.. is this a good path to you?
> 
>     Yes this sounds good.
> 
>     > Is there somewhere else in the dynamic loader system with a similar issue?
> 
>     This is the only external interface of this type where static checking does not
>     help. The other is ELF which I hope is OK. I am not sure if other places may
>     have a problem. All care was taken however only using the code with 64bits will
>     determine this.
> 
>     The elftools libelf support was ported and tested with only 32bit targets. I am
>     not sure if there are issues there.
> 
> 
> OK. Ryan and Alex will be fixing this as part of adding aarch64 libdl support. 
> Just having you available to give insight is very helpful.

No problem and Alex knows he can ping me on Discord.

Chris