[PATCH] D44485: [MC] Always emit relocations for same-section function references
Rafael Avila de Espindola via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 14 11:29:09 PDT 2018
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 @@
> # %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 @@
> # %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
More information about the llvm-commits
mailing list