[LLD] [COFF] Warn about pseudo relocations that are too narrow

[mstorsjo]

Summary

In 64 bit mode, any references to symbols that might end up autoimported
must be made via full 64 bit pointers (usually in .refptr stubs
generated by the compiler).

If referenced via e.g. a 32 bit rip relative offset, it might work
as long as DLLs are loaded close together in the 64 bit address
space, but will fail surprisingly later if they happen to be loaded
further apart. Any cases of that happening is usually a toolchain
error, and the sooner we can warn about it, the easier it is to diagnose.

The warning message is a bit odd; normally all lld warnings/errors
start with lower case, but here the error message consists of two
full sentences. Style suggestions for how to handle that are
welcome.

[alvinhochun]

Sounds like a good diagnostic to me. For the warning message perhaps it can be changed a bit to avoid using a full stop?

runtime pseudo relocation in [...] against symbol [...] is too narrow (only 32 bits wide); this will fail at runtime depending on memory layout

I also think it can use a slightly stronger tone than "can fail", maybe even worth being an error by default. With HEASLR being the default nowadays, 32-bit pseudo relocations seems like a recipe for failure, is it not?

[mstorsjo]

In D154777#4483231, @alvinhochun wrote:

Sounds like a good diagnostic to me. For the warning message perhaps it can be changed a bit to avoid using a full stop?

runtime pseudo relocation in [...] against symbol [...] is too narrow (only 32 bits wide); this will fail at runtime depending on memory layout

Nice, thanks!

I also think it can use a slightly stronger tone than "can fail", maybe even worth being an error by default.

Yes, generally whenever you hit this, you have a serious issue anyway. On one hand, I'm worried of breaking cases where things actually do happen to work (for whatever reason - consider things like cygwin, with lots of loader/memory layout trickery - might be doing things differently?). On the other hand, a warning that doesn't break a build is very easily overlooked.

Wrt these issues, we've slowly progressed towards detecting them earlier. Initially, if you struck this, you'd have a strange runtime crash which was mysterious to debug. Nowadays, the mingw-w64 runtime errors out if the actual target symbol is too far away, with a cryptic error message (only giving addresses). With this in place, we'd at least have the symbol name more accessible if someone tries to dig in.

So I'm somewhat undecided between just making it a hard error right away, and merging it as a warning and maybe considering strengthening to an error later.

With HEASLR being the default nowadays, 32-bit pseudo relocations seems like a recipe for failure, is it not?

Not necessarily; while ASLR does affect the load addresses, I think it primarily affects the base addresses where things are loaded, while most DLLs probably still are loaded close together. Liu Hao struck this issue in May (although in a build linked with binutils, so I'm unsure if HEASLR was enabled there or not), and in that case, the mingw-w64 loader only errored out occasionally, while it mostly worked out ok.

[mstorsjo]

Adjusted the warning message to avoid splitting it into two separate sentences.

[mati865]

I'm in favor of the warning. I don't know LLVM process for adding new errors for code that was previously accepted (even if wrongly) but IMO new errors should be introduced in a way that doesn't surprisingly break downstreams.
So I'd be okay if this becomes a warning in next release (N) and an error/hard error (don't think LLD threats them differently though) in release N+1 or N+2.

ASs for the tone, dunno, I think even with HEASLR user can be lucky enough to have it working.

lld/COFF/Chunks.cpp

int sizeInBits =
         getRuntimePseudoRelocSize(rel.Type, file->ctx.config.machine);
     if (sizeInBits == 0) {
       error("unable to automatically import from " + target->getName() +
             " with relocation type " +
             file->getCOFFObj()->getRelocationTypeName(rel.Type) + " in " +
             toString(file));
       continue;
     }
+    int addressSizeInBits = file->ctx.config.is64() ? 64 : 32;
+    if (sizeInBits < addressSizeInBits) {
+      warn("runtime pseudo relocation in " + toString(file) + " against " +
+           "symbol " + target->getName() + " is too narrow; only " +
+           Twine(sizeInBits) + " bits wide. This can fail at runtime " +
+           "depending on memory layout.");
+    }
     // sizeInBits is used to initialize the Flags field; currently no
     // other flags are defined.
     res.emplace_back(target, this, rel.VirtualAddress, sizeInBits);
   }
 }
 
 bool SectionChunk::isCOMDAT() const {
   return header->Characteristics & IMAGE_SCN_LNK_COMDAT;

lld/test/CMakeLists.txt

list(APPEND LLD_TEST_DEPS
     dsymutil
     llc
     llvm-ar
     llvm-as
     llvm-bcanalyzer
     llvm-config
     llvm-cvtres
     llvm-dis
+    llvm-dlltool
     llvm-dwarfdump
     llvm-lib
     llvm-lipo
     llvm-mc
     llvm-nm
     llvm-objcopy
     llvm-objdump
     llvm-otool

lld/test/COFF/autoimport-nowarn.s

+# REQUIRES: x86
+
+# RUN: echo -e "EXPORTS\nvariable" > %t-lib.def
+# RUN: llvm-dlltool -m i386 -d %t-lib.def -D lib.dll -l %t-lib.lib
+
+# RUN: llvm-mc -triple=i386-windows-gnu %s -filetype=obj -o %t.obj
+# RUN: lld-link -lldmingw -out:%t.exe -entry:main %t.obj %t-lib.lib -verbose 2>&1 | FileCheck --allow-empty %s
+
+# CHECK-NOT: runtime pseudo relocation {{.*}} is too narrow
+
+    .global _main
+    .text
+_main:
+    movl _variable, %eax
+    ret
+
+relocs:
+    .long ___RUNTIME_PSEUDO_RELOC_LIST__
+    .long ___RUNTIME_PSEUDO_RELOC_LIST_END__

lld/test/COFF/autoimport-warn.s

+# REQUIRES: x86
+
+# RUN: echo -e "EXPORTS\nvariable1 DATA\nvariable2 DATA" > %t-lib.def
+# RUN: llvm-dlltool -m i386:x86-64 -d %t-lib.def -D lib.dll -l %t-lib.lib
+
+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t.obj
+# RUN: lld-link -lldmingw -out:%t.exe -entry:main %t.obj %t-lib.lib -verbose 2>&1 | FileCheck %s
+
+# CHECK-NOT: runtime pseudo relocation {{.*}} against symbol variable1
+# CHECK: warning: runtime pseudo relocation in {{.*}}.obj against symbol variable2 is too narrow; only 32 bits wide. This can fail at runtime depending on memory layout.
+# CHECK-NOT: runtime pseudo relocation {{.*}} against symbol variable1
+
+    .global main
+    .text
+main:
+    movq .refptr.variable1(%rip), %rax
+    movl (%rax), %eax
+    movl variable2(%rip), %ecx
+    addl %ecx, %eax
+    ret
+
+    .section .rdata$.refptr.variable1,"dr",discard,.refptr.variable1
+    .global .refptr.variable1
+.refptr.variable1:
+    .quad variable1
+
+relocs:
+    .quad __RUNTIME_PSEUDO_RELOC_LIST__
+    .quad __RUNTIME_PSEUDO_RELOC_LIST_END__