Re: [PATCH v3] elf: Rewrite long RESOLVE_MAP macro to a debug friendly function

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 22/05/2022 19:24, Fangrui Song via Libc-alpha wrote:
> On 2022-05-15, Siddhesh Poyarekar wrote:
>> On 15/05/2022 04:18, Fangrui Song wrote:
>>> On 2022-05-14, Nicholas Guriev wrote:
>>>> On Пн, 2022-05-09 at 19:08 +0530, Siddhesh Poyarekar wrote:
>>>>> Please use __always_inline instead, which adds the inline keyword as
>>>>> well as __attribute__ ((inline)).  That way gcc ensures that the
>>>>> function is *always* inlined.
>>>>
>>>> That is not necessary. My original intention was to improve the
>>>> debugging experience at this point. And the standard `inline` keyword
>>>> allows one to control optimizations with the -O flag.
>>
>> You can't build glibc with anything less than -O2, so that's a moot point.  If you don't use __always_inline here then you're not only changing the debugging experience, but also potentially adding a performance overhead.  gdb can do well enough in most cases of inlined functions, so __always_inlined should not hamper that.
> 
> Hope that the -O2 requirement can be lifted:)
> 
> The original decision might be related to guaranteed inlining and relocations in -O0
> but it should be revisited nowadays... (Both FreeBSD rtld and musl rtld allow -O0).

I tried some time ago, before the recent refactors, and the loader still fails at
bootstrapping (I have not dig into to pinpoint exactly what has failed).  But 
I agree it would be good to support it, we also might need to fix some objecting
pulling from loader (since -O0 messes with symbols and the script to decide what
to pull).

> 
>>> Looks good to me, I think the always_inline here does not affect
>>> performance, as shown in my testing with a large PIE with many
>>> R_*_RELATIVE (function call overhead is larger with a non-R_*_RELATIVE).
>>
>> If you want to make a decision based on this then please quantify it more precisely with a microbenchmark added to benchtests.  Otherwise just add __always_inline because it is a more minimal change.  TBH I'd prefer the former because it then adds a new microbenchmark that we can measure dynamic linker changes in future but I can live with the latter.
> 
> Thanks. I can add __always_inline.
> (I don't think it matters but I guess I just want to avoid a debate :))

I think the main issue is we build glibc with -fgnu89-inline, which is required
due some optimizations where a function is defined without inline, but then
it has an inline definition to be used internally.

Also, since we don't use -Winline we can't assure that compiler won't emit the
function definition where gcc documents it might [1].  So I think one exercise
we might do it to remove all __always_inline__, and add -Winline to see which
functions, if any, won't be inline by compiler. 

I would like also to eventually remove -fgnu89-inline, since I think we can
restructure the code to not rely on extern inlines nor on the internal inline
optimizations.  Also, it seems that although clang seems to support
-fgnu89-inline, it has subtle different semantics that breaks some internal
glibc assumptions.

[1] https://gcc.gnu.org/onlinedocs/gcc/Inline.html