Page MenuHomePhabricator

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

Authored by rnk on Mar 14 2018, 11:21 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Mar 14 2018, 11:21 AM

LGTM

Reid Kleckner via Phabricator via llvm-commits
<llvm-commits@lists.llvm.org> 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
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

This revision was not accepted when it landed; it landed in state Needs Review.Mar 14 2018, 12:27 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 1:36 AM

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 added a comment.Jan 23 2020, 11:43 AM

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.

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 added a comment.Apr 29 2020, 3:47 PM

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.

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