[MC] Always emit relocations for same-section function references

[rnk]

Summary

We already emit relocations in this case when the "incremental linker
compatible" flag is set, but it turns out these relocations are also
required for /guard:cf. Now that we have two use cases for this
behavior, let's make it unconditional to try to keep things simple.

We never hit this problem in Clang because it always sets the
"incremental linker compatible" flag when targeting MSVC. However, LLD
LTO doesn't set this flag, so we'd get CFG failures at runtime when
using ThinLTO and /guard:cf. We probably don't want LLD LTO to set the
"incremental linker compatible" assembler flag, since this has nothing
to do with incremental linking, and we don't need to timestamp LTO
temporary objects.

Fixes PR36624.

[• rafael]

LGTM

Reid Kleckner via Phabricator via llvm-commits
<[e-mail]> writes:

rnk created this revision.
rnk added reviewers: inglorion, espindola, majnemer.
Herald added subscribers: hiraditya, mehdi_amini.

We already emit relocations in this case when the "incremental linker
compatible" flag is set, but it turns out these relocations are also
required for /guard:cf. Now that we have two use cases for this
behavior, let's make it unconditional to try to keep things simple.

We never hit this problem in Clang because it always sets the
"incremental linker compatible" flag when targeting MSVC. However, LLD
LTO doesn't set this flag, so we'd get CFG failures at runtime when
using ThinLTO and /guard:cf. We probably don't want LLD LTO to set the
"incremental linker compatible" assembler flag, since this has nothing
to do with incremental linking, and we don't need to timestamp LTO
temporary objects.

Fixes PR36624.

https://reviews.llvm.org/D44485

Files:

llvm/lib/MC/WinCOFFObjectWriter.cpp
llvm/test/MC/COFF/diff.s

Index: llvm/test/MC/COFF/diff.s

• llvm/test/MC/COFF/diff.s

+++ llvm/test/MC/COFF/diff.s
@@ -1,19 +1,14 @@
// RUN: llvm-mc -filetype=obj -triple i686-pc-mingw32 %s | llvm-readobj -s -sr -sd | FileCheck %s

+ COFF resolves differences between labels in the same section, unless that
+ label is declared with function type.
+
.section baz, "xr"

• .def X
• .scl 2;
• .type 32;
• .endef .globl X X: mov Y-X+42, %eax retl

• .def Y
• .scl 2;
• .type 32;
• .endef .globl Y Y: retl

@@ -30,6 +25,11 @@

1. %bb.0: ret

+ .globl _baz
+_baz:
+ calll _foobar
+ retl
+

	.data
	.globl	_rust_crate             # @rust_crate
	.align	4

@@ -39,6 +39,15 @@

	.long	_foobar-_rust_crate
	.long	_foobar-_rust_crate

+ Even though _baz and _foobar are in the same .text section, we keep the
+ relocation for compatibility with the VC linker's /guard:cf and /incremental
+ flags, even on mingw.
+
+ CHECK: Name: .text
+ CHECK: Relocations [
+ CHECK-NEXT: 0x12 IMAGE_REL_I386_REL32 _foobar
+ CHECK-NEXT: ]
+
CHECK: Name: .data
CHECK: Relocations [
CHECK-NEXT: 0x4 IMAGE_REL_I386_DIR32 _foobar

Index: llvm/lib/MC/WinCOFFObjectWriter.cpp

• llvm/lib/MC/WinCOFFObjectWriter.cpp

+++ llvm/lib/MC/WinCOFFObjectWriter.cpp
@@ -697,12 +697,14 @@
bool WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(

const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
bool InSet, bool IsPCRel) const {

• // MS LINK expects to be able to replace all references to a function with a
• // thunk to implement their /INCREMENTAL feature. Make sure we don't optimize
• // away any relocations to functions.

+ Don't drop relocations between functions, even if they are in the same text
+ section. Multiple Visual C++ linker features depend on having the
+ relocations present. The /INCREMENTAL flag will cause these relocations to
+ point to thunks, and the /GUARD:CF flag assumes that it can use relocations
+ to approximate the set of all address taken functions. LLD's implementation
+ of /GUARD:CF also relies on the existance of these relocations.

uint16_t Type = cast<MCSymbolCOFF>(SymA).getType();

• if (Asm.isIncrementalLinkerCompatible() &&
• (Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)

+ if ((Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)

  return false;
return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
                                                              InSet, IsPCRel);

Index: llvm/test/MC/COFF/diff.s

• llvm/test/MC/COFF/diff.s

+++ llvm/test/MC/COFF/diff.s
@@ -1,19 +1,14 @@
// RUN: llvm-mc -filetype=obj -triple i686-pc-mingw32 %s | llvm-readobj -s -sr -sd | FileCheck %s

+ COFF resolves differences between labels in the same section, unless that
+ label is declared with function type.
+
.section baz, "xr"

• .def X
• .scl 2;
• .type 32;
• .endef .globl X X: mov Y-X+42, %eax retl

• .def Y
• .scl 2;
• .type 32;
• .endef .globl Y Y: retl

@@ -30,6 +25,11 @@

1. %bb.0: ret

+ .globl _baz
+_baz:
+ calll _foobar
+ retl
+

	.data
	.globl	_rust_crate             # @rust_crate
	.align	4

@@ -39,6 +39,15 @@

	.long	_foobar-_rust_crate
	.long	_foobar-_rust_crate

+ Even though _baz and _foobar are in the same .text section, we keep the
+ relocation for compatibility with the VC linker's /guard:cf and /incremental
+ flags, even on mingw.
+
+ CHECK: Name: .text
+ CHECK: Relocations [
+ CHECK-NEXT: 0x12 IMAGE_REL_I386_REL32 _foobar
+ CHECK-NEXT: ]
+
CHECK: Name: .data
CHECK: Relocations [
CHECK-NEXT: 0x4 IMAGE_REL_I386_DIR32 _foobar

Index: llvm/lib/MC/WinCOFFObjectWriter.cpp

• llvm/lib/MC/WinCOFFObjectWriter.cpp

+++ llvm/lib/MC/WinCOFFObjectWriter.cpp
@@ -697,12 +697,14 @@
bool WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(

const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
bool InSet, bool IsPCRel) const {

• // MS LINK expects to be able to replace all references to a function with a
• // thunk to implement their /INCREMENTAL feature. Make sure we don't optimize
• // away any relocations to functions.

+ Don't drop relocations between functions, even if they are in the same text
+ section. Multiple Visual C++ linker features depend on having the
+ relocations present. The /INCREMENTAL flag will cause these relocations to
+ point to thunks, and the /GUARD:CF flag assumes that it can use relocations
+ to approximate the set of all address taken functions. LLD's implementation
+ of /GUARD:CF also relies on the existance of these relocations.

uint16_t Type = cast<MCSymbolCOFF>(SymA).getType();

• if (Asm.isIncrementalLinkerCompatible() &&
• (Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)

+ if ((Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)

  return false;
return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
                                                              InSet, IsPCRel);

---

llvm-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

[vchuravy]

Just as a note, Julia currently reverts this patch. IIRC it caused issue with gcc/mingw32 which we use for linking.
My notes from back then are fairly limited and another team member did the git bisect.

ERROR: could not load library "C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\lib\julia\sys.dll"
%1 is not a valid Win32 application.

gflags -i julia.exe +sls
cdb julia.exe
> g
0b6c:05cc @ 12495953 - LdrpProcessWork - ERROR: Unable to load DLL: "C:\cygwin\home\vchuravy\julia\usr\lib\julia\sys.dll", Parent Module: "(null)", Status: 0xc000007b

    Status 0xc000007b is STATUS_INVALID_IMAGE_FORMAT

[rnk]

I don't think this has been a problem for mingw, GCC, & ld.bfd. At least @mstorsjo hasn't reported any issues with it. Anyway, if you can reproduce the problem, we can look into it.

[vtjnash]

We emit an offset table of the form:

ptrdiff_t symbol_offset = &f2 - &f1;

since we know this should normally be folded to a constant in the object file. This is normally rejected by a C compiler ("error: initializer element is not a compile-time constant", pointing to the f2 symbol) but LLVM accepts it. It used to be handled here (since the difference is a constant known here), though it seems the mingw-w64 linker later corrupts this expression.

[rnk]

That makes some sense. However, it seems like LLVM MC will correctly report this as an error:

$ cat t.ll
@gv = hidden local_unnamed_addr global i64 sub (i64 ptrtoint (i32 ()* @f1 to i64),
                                                i64 ptrtoint (i32 ()* @f2 to i64)), align 8
define internal i32 @f1() { ret i32 13 }
define internal i32 @f2() { ret i32 42 }

$ llc t.ll -filetype=obj -mtriple=x86_64-windows-gnu -o t.o
LLVM ERROR: Cannot represent this expression
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
...

Seems to be WAI, Julia should not generate such GV initializers.

At the assembly level, the expression looks like this:

gv:
        .quad   f1-f2

llvm-mc reports a nice error diagnostic instead of error spew in that case.

[vchuravy]

Right the issue for is that LLVM6 used to accept and compile it without issue:

➜  v6 tools/llc -version
LLVM (http://llvm.org/):
  LLVM version 6.0.1
  Optimized build.

➜  v6 cat t.ll
@gv = hidden local_unnamed_addr global i64 sub (i64 ptrtoint (i32 ()* @f1 to i64),
                                                i64 ptrtoint (i32 ()* @f2 to i64)), align 8
define internal i32 @f1() { ret i32 13 }
define internal i32 @f2() { ret i32 42 }

➜  v6 tools/llc t.ll -filetype=obj -mtriple=x86_64-windows-gnu -o t.o

As well as a toolchain that has this PR reverted:

➜  v9 tools/llc -version
LLVM (http://llvm.org/):
  LLVM version 9.0.1jl
  Optimized build.
  Default target: x86_64-linux-gnu
  Host CPU: skylake

  Registered Targets:
    amdgcn  - AMD GCN GPUs
    nvptx   - NVIDIA PTX 32-bit
    nvptx64 - NVIDIA PTX 64-bit
    r600    - AMD GPUs HD2XXX-HD6XXX
    wasm32  - WebAssembly 32-bit
    wasm64  - WebAssembly 64-bit
    x86     - 32-bit X86: Pentium-Pro and above
    x86-64  - 64-bit X86: EM64T and AMD64
➜  v9 tools/llc t.ll -filetype=obj -mtriple=x86_64-windows-gnu -o t.o

llvm/trunk/lib/MC/WinCOFFObjectWriter.cpp

void WinCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
   for (const MCSymbol &Symbol : Asm.symbols())
     if (!Symbol.isTemporary())
       DefineSymbol(Symbol, Asm, Layout);
 }
 
 bool WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
     const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
     bool InSet, bool IsPCRel) const {
-  // MS LINK expects to be able to replace all references to a function with a
-  // thunk to implement their /INCREMENTAL feature.  Make sure we don't optimize
-  // away any relocations to functions.
+  // Don't drop relocations between functions, even if they are in the same text
+  // section. Multiple Visual C++ linker features depend on having the
+  // relocations present. The /INCREMENTAL flag will cause these relocations to
+  // point to thunks, and the /GUARD:CF flag assumes that it can use relocations
+  // to approximate the set of all address taken functions. LLD's implementation
+  // of /GUARD:CF also relies on the existance of these relocations.
   uint16_t Type = cast<MCSymbolCOFF>(SymA).getType();
-  if (Asm.isIncrementalLinkerCompatible() &&
-      (Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)
+  if ((Type >> COFF::SCT_COMPLEX_TYPE_SHIFT) == COFF::IMAGE_SYM_DTYPE_FUNCTION)
     return false;
   return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
                                                                 InSet, IsPCRel);
 }
 
 void WinCOFFObjectWriter::recordRelocation(MCAssembler &Asm,
                                            const MCAsmLayout &Layout,
                                            const MCFragment *Fragment,

llvm/trunk/test/MC/COFF/diff.s

 // RUN: llvm-mc -filetype=obj -triple i686-pc-mingw32 %s | llvm-readobj -s -sr -sd | FileCheck %s
 
+// COFF resolves differences between labels in the same section, unless that
+// label is declared with function type.
+
 .section baz, "xr"
-  .def  X
-  .scl  2;
-  .type 32;
-  .endef
   .globl  X
 X:
   mov Y-X+42, %eax
   retl
 
-  .def  Y
-  .scl  2;
-  .type 32;
-  .endef
   .globl  Y
 Y:
   retl
 
   .def   _foobar;
   .scl  2;
   .type 32;
   .endef
   .text
   .long   0
   .globl  _foobar
   .align  16, 0x90
 _foobar:                                # @foobar
 # %bb.0:
   ret
 
+  .globl  _baz
+_baz:
+  calll _foobar
+  retl
+
   .data
   .globl  _rust_crate             # @rust_crate
   .align  4
 _rust_crate:
   .long   0
   .long   _foobar
   .long _foobar-_rust_crate
   .long _foobar-_rust_crate
 
+// Even though _baz and _foobar are in the same .text section, we keep the
+// relocation for compatibility with the VC linker's /guard:cf and /incremental
+// flags, even on mingw.
+
+// CHECK:        Name: .text
+// CHECK:        Relocations [
+// CHECK-NEXT:     0x12 IMAGE_REL_I386_REL32 _foobar
+// CHECK-NEXT:   ]
+
 // CHECK:        Name: .data
 // CHECK:        Relocations [
 // CHECK-NEXT:     0x4 IMAGE_REL_I386_DIR32 _foobar
 // CHECK-NEXT:     0x8 IMAGE_REL_I386_REL32 _foobar
 // CHECK-NEXT:     0xC IMAGE_REL_I386_REL32 _foobar
 // CHECK-NEXT:   ]
 // CHECK:        SectionData (
 // CHECK-NEXT:     0000: 00000000 00000000 0C000000 10000000
 // CHECK-NEXT:   )
 
 // CHECK:        Name: baz
 // CHECK:        Relocations [
 // CHECK-NEXT:   ]
 // CHECK:        SectionData (
 // CHECK-NEXT:     0000: A1300000 00C3C3
 // CHECK-NEXT:   )