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

[Reid Kleckner via Phabricator via llvm-commits]

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 @@
 # %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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D44485.138421.patch
Type: text/x-patch
Size: 2736 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180314/4449820b/attachment.bin>