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

[sgraenitz]

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.

[sgraenitz]

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.

[sgraenitz]

This comment was removed by sgraenitz.

[sgraenitz]

Fix NULL entry in symbol table

[sgraenitz]

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?

[sgraenitz]

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.

[sgraenitz]

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.

[sgraenitz]

Drop unrelated switch case R_X86_64_32S

[sgraenitz]

Polish comments

[sgraenitz]

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]

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

[sgraenitz]

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!

[sgraenitz]

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]

Add test ELF_x86-64_common

[sgraenitz]

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?

[sgraenitz]

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]

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

[sgraenitz]

With movl we have to target eax

[sgraenitz]

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

llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp

for (auto SecRef : sections) {
         //   Type != ELF::STT_COMMON) {
         //     continue;
         //   }
         std::pair<Linkage, Scope> bindings;
         auto Name = SymRef.getName(*StringTable);
         // I am not sure on If this is going to hold as an invariant. Revisit.
         if (!Name)
           return Name.takeError();
+
+        // Allocate uninitialized common blocks.
+        if (SymRef.isCommon()) {
+          // st_value holds alignment constraints for SHN_COMMON symbols
+          Symbol &S =
+              G->addCommonSymbol(*Name, Scope::Default, getCommonSection(), 0,
+                                 SymRef.st_size, SymRef.getValue(), false);
+          JITSymbolTable[SymbolIndex] = &S;
+          continue;
+        }
+
         // TODO: weak and hidden
         if (SymRef.isExternal())
           bindings = {Linkage::Strong, Scope::Default};
         else
           bindings = {Linkage::Strong, Scope::Local};
 
         if (SymRef.isDefined() &&
             (Type == ELF::STT_FUNC || Type == ELF::STT_OBJECT ||
for (auto SecRef : sections) {
               *B, SymRef.getValue(), *Name, SymRef.st_size, bindings.first,
               bindings.second, SymRef.getType() == ELF::STT_FUNC, false);
           JITSymbolTable[SymbolIndex] = &S;
         } else if (SymRef.isUndefined() && SymRef.isExternal()) {
           Symbol &S = G->addExternalSymbol(*Name, SymRef.st_size, bindings.first);
           JITSymbolTable[SymbolIndex] = &S;
         }
 
-        //  }
         // TODO: The following has to be implmented.
         // leaving commented out to save time for future patchs
         /*
           G->addAbsoluteSymbol(*Name, SymRef.getValue(), SymRef.st_size,
           Linkage::Strong, Scope::Default, false);
-
-          if(SymRef.isCommon()) {
-            G->addCommonSymbol(*Name, Scope::Default, getCommonSection(), 0, 0,
-          SymRef.getValue(), false);
-          }
         */
       }
     }
     return Error::success();
   }
 
 public:
   ELFLinkGraphBuilder_x86_64(std::string filename,
                              const object::ELFFile<object::ELF64LE> &Obj)