Page MenuHomePhabricator

[jitlink][ELF] Add zero-fill blocks for symbols in section SHN_COMMON
ClosedPublic

Authored by sgraenitz on Oct 20 2020, 7:51 AM.

Details

Summary

Symbols with special section index SHN_COMMON (0xfff2) haven't been handled so far and caused an invalid section error.

This is a more or less straightforward use of the code commented out at the end of the function. I checked with the ELF spec, that the symbol value gives the alignment.

Diff Detail

Event Timeline

sgraenitz created this revision.Oct 20 2020, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 7:51 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sgraenitz requested review of this revision.Oct 20 2020, 7:51 AM

FYI The issue came up when I was trying to jitlink gzip: https://people.csail.mit.edu/smcc/projects/single-file-programs/gzip.c Next step would be an isolated test case.

This comment was removed by sgraenitz.
sgraenitz updated this revision to Diff 299942.Oct 22 2020, 6:08 AM

Fix NULL entry in symbol table

Relocations to the common section seem to be calculated correctly now, but at runtime I see a SIGSEGV: invalid address. Maybe memory for the section isn't allocated?

It turned out the SIGSEGV: invalid address is caused by not handling the R_X86_64_32S type correctly. This is somewhat unrelated, but must be fixed in order to come up with a test case.

Of course, I must compile with -fPIC to avoid relocation types like R_X86_64_32S in the first place. Looking into the test case now.

Drop unrelated switch case R_X86_64_32S

Polish comments

sgraenitz retitled this revision from [jitlink][ELF] Allocate SHN_COMMON symbols as uninitialized blocks in __common to [jitlink][ELF] Add zero-fill blocks for symbols in section SHN_COMMON.Oct 22 2020, 12:27 PM
sgraenitz edited the summary of this revision. (Show Details)

The ELF spec has an extra paragraph for special handling in dynamic linkers. I am not sure how much it's relevant here:

When the dynamic linker encounters a reference to a symbol that resolves to a definition of type STT_COMMON, it may (but is not required to) change its symbol resolution rules as follows: instead of binding the reference to the first symbol found with the given name, the dynamic linker searches for the first symbol with that name with type other than STT_COMMON. If no such symbol is found, it looks for the STT_COMMON definition of that name that has the largest size.

What do you think?

lhames accepted this revision.Oct 22 2020, 11:32 PM

This looks good to me.

Suggested test case below (this would go in llvm/test/ExecutionEngine/JITLink/X86/). I didn't get a chance to see whether your patch fixes it yet -- I'll try that tomorrow.

# RUN: rm -rf %t && mkdir -p %t
# RUN: llvm-mc -triple=x86_64-unknown-linux -position-independent -filetype=obj -o %t/elf_common.o %s
# RUN: llvm-jitlink -entry=load_common -noexec -check %s %t/elf_common.o

        .text
        .file   "ELF_common.c"
        .globl  load_common
        .p2align        4, 0x90
        .type   load_common,@function
load_common:
# Check that common variable GOT entry is synthesized correctly.
# jitlink-check: decode_operand(load_common, 4) = \
# jitlink-check:   got_addr(elf_common.o, common_data) - next_pc(load_common)
# jitlink-check: *{8}(got_addr(elf_common.o, common_data)) = common_data
        movq    x@GOTPCREL(%rip), %rax
.Lfunc_end0:
        .size   load_common, .Lfunc_end0-load_common

# Check that common is zero-filled.
# jitlink-check: *{4}(common_data) = 0
        .type   common_data,@object
        .comm   common_data,4,4
This revision is now accepted and ready to land.Oct 22 2020, 11:32 PM

Thanks for coming up with a test case. I haven't looked into llvm-jitlink's check code yet. That serves a an excellent starting point!

Yes, that brings up the issue with relaxed R_X86_64_REX_GOTPCRELX (42) relocations you mentioned in Discord: JIT session error: Unsupported x86-64 relocation:42 I will check whether we can map it to PCRel32GOTLoad or otherwise what's the difference.

sgraenitz updated this revision to Diff 300291.Oct 23 2020, 7:48 AM

Add test ELF_x86-64_common

I didn't get the test to work yet. Neither with a R_X86_64_REX_GOTPCRELX relocation for common_data (TOT clang and llvm-mc emit that, llvm-mc-10 too btw) nor with R_X86_64_GOTPCREL (clang-10). Some observations in the following.

Output is:

Expression 'decode_operand(load_common, 4) = got_addr(elf_common.o, common_data) - next_pc(load_common)' is false: 0xff9 != 0x1ff9

Executable memory is:

dis -s 0x00007ffff7ff5000 -e 0x00007ffff7ff5010
0x7ffff7ff5000: leaq   0xff9(%rip), %rax

So movq became leaq. Apparently the optimizeELF_x86_64_GOTAndStubs post-allocation pass does that. The instruction makes sense to me, because the data segment is at 0x00007ffff7ff6000. The pass transforms the original PCRel32GOTLoad into a PCRel32 and the result seems to be correct in this case. So, if we keep the optimization, I had to adjust the test expression right?

Skipping the optimization pass, I had expected to get something like movq 0x1ff9(%rip), %rax but instead it's:

dis -s 0x00007ffff7ff5000 -e 0x00007ffff7ff5010
    0x7ffff7ff5000: movq   (%rip), %rax

It turns out, that there is no actual implementation for PCRel32GOTLoad (and no warning for unhandled relocations). Instead we seem to rely on the optimization here. Is that correct? If so, I see two ways to move forward from here:

(1) Implement PCRel32GOTLoad relocations, add a way to skip passes in llvm-jitlink and use it in the test.
(2) Fix the test expression and keep relying on the optimization pass.

In both cases it might be useful to issue a warning for unhandled relocations. @lhames Did I miss anything? Are there similar cases in the MachO implementation? What would you recommend?
I will have a look at the ELF_x86-64_relocations test in the meantime and see how the GOTPCREL test handles it.

sgraenitz updated this revision to Diff 300399.Oct 23 2020, 1:44 PM

Prevent the optimizer from relaxing the edge by using a movl instruction (anyway it's been only 4 bytes all the time)

sgraenitz updated this revision to Diff 300477.Oct 24 2020, 5:26 AM

With movl we have to target eax

This revision was landed with ongoing or failed builds.Oct 24 2020, 5:56 AM
This revision was automatically updated to reflect the committed changes.

One note on the optimizeELF_x86_64_GOTAndStubs pass that had to be considered for this patch: it currently only relaxes edges for MOVQ instructions to LEA and we avoid that in the test by using a MOVL. If I understand correctly, LLD doesn't have this restriction. It relaxes MOVQ to LEAQ and MOVL to LEAL. For reference see https://reviews.llvm.org/D15779#change-f8vl5AmubNpC