[PATCH] elf: Avoid nested functions in the loader (x86-64) [BZ #27220]

[Fangrui Song]

Thanks for the reply!

On 2021-08-20, Joseph Myers wrote:
>On Fri, 20 Aug 2021, Fangrui Song via Libc-alpha wrote:
>
>> This patch just fixes x86-64: x86-64 uses the `#ifndef NESTING` branch
>> to avoid nested functions. The `#ifdef NESTING` branch is used by all
>> other ports whose dl-machine.h files haven't been migrated.
>>
>> For the time being, there is some duplicated code. `#ifdef NESTING`
>> dispatches can be removed in the future when all arches are migrated.
>
>The code certainly looks a lot messier after this patch.  Could you please
>send a patch (working for x86_64 only, I suppose) showing what things
>would look like without those conditionals?  There are some key things a
>nested function removal patch ought to satisfy:
>
>* The source code looks at least as clean, to human readers, after the
>patch as before.  If anything is factored out to a new function or macro,
>there should be the usual API comments on that function or macro
>definition explaining its semantics; if any existing function or macro
>gets new parameters to avoid the implicit passing of data to nested
>functions, the API comments on that function or macro need updating to
>describe the semantics of those parameters.

Pushed the clean form (without NESTING dispatches) to
https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/maskray/unnest

8 files changed, 101 insertions(+), 85 deletions(-)

>* The generated object code is of similar or better quality (have you
>compared it before and after the patch?).

The object file is slightly larger.

% readelf -WS /tmp/c/ld.so.old | grep text
   [12] .text             PROGBITS        0000000000001050 001050 023bae 00  AX  0   0 16
% readelf -WS /tmp/c/ld.so.new | grep text
   [12] .text             PROGBITS        0000000000001050 001050 023cce 00  AX  0   0 16

In dl-reloc.c, accessing the 3 internal linkage global variables takes more
instructions.  The file has 28 more instructions. But the cost looks negligible
when considering the function call to _dl_lookup_symbol_x.

>Also, when proposing a patch that only updates some architectures for an
>issue where all architectures ought to be updated, please give the
>proposed text you would add to
><https://sourceware.org/glibc/wiki/PortStatus> if the patch is accepted,
>with instructions for port maintainers on what would be involved in
>updating their ports.

Thanks for the instructions.

>> +#ifndef NESTING
>> +
>> +    /* String table object symbols.  */
>> +
>> +static struct link_map *glob_l;
>> +static struct r_scope_elem **glob_scope;
>> +static const char *glob_strtab;
>
>Writable static variables generally look suspect; there should be as
>little writable global (as opposed to thread-local) data in glibc as
>possible.  At least I'd expect comments on each variable describing its
>semantics, and some kind of comment explaining why it's thread-safe to use
>such variables (e.g. saying what lock, acquired where, is used to prevent
>multiple threads from accessing them at once, or what other mechanism is
>used for thread safety).

I renamed the variables and added comments.

+/* Used by RESOLVE_MAP. _dl_relocate_object is either called at init time or
+ * by dlopen with a global lock, so the variables cannot be accessed
+ * concurrently.  */
+static struct link_map *cur_l;
+static struct r_scope_elem **cur_scope;
+static const char *cur_strtab;

The new commit is on https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/maskray/nesting