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

Wed Mar 14 11:29:19 PDT 2018

rafael added subscribers: junbuml, rafael.
rafael added a comment.

LGTM

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

https://reviews.llvm.org/D44485