From efb7e1aa2988079c4a65effbe3d9391f875b4561 Mon Sep 17 00:00:00 2001 From: Rui Ueyama Date: Sat, 20 Jun 2015 07:21:57 +0000 Subject: [PATCH] COFF: Fix a common symbol bug. This is a case that one mistake caused a very mysterious bug. I made a mistake to calculate addresses of common symbols, so each common symbol pointed not to the beginning of its location but to the end of its location. (Ouch!) Common symbols are aligned on 16 byte boundaries. If a common symbol is small enough to fit between the end of its real location and whatever comes next, this bug didn't cause any harm. However, if a common symbol is larger than that, its memory naturally overlapped with other symbols. That means some uninitialized variables accidentally shared memory. Because totally unrelated memory writes mutated other varaibles, it was hard to debug. It's surprising that LLD was able to link itself and all LLD tests except gunit tests passed with this nasty bug. With this fix, the new COFF linker is able to pass all tests for LLVM, Clang and LLD if I use MSVC cl.exe as a compiler. Only three tests are failing when used with clang-cl. llvm-svn: 240216 --- lld/COFF/InputFiles.cpp | 2 +- lld/COFF/Symbols.cpp | 30 ++++++++----- lld/COFF/Symbols.h | 28 +++++++++++-- lld/test/COFF/Inputs/common.yaml | 91 ++++++++++++++++++++++++++++++++++++++++ lld/test/COFF/common.test | 10 +++++ 5 files changed, 146 insertions(+), 15 deletions(-) create mode 100644 lld/test/COFF/Inputs/common.yaml create mode 100644 lld/test/COFF/common.test diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp index 4499395..df57da0 100644 --- a/lld/COFF/InputFiles.cpp +++ b/lld/COFF/InputFiles.cpp @@ -184,7 +184,7 @@ SymbolBody *ObjectFile::createSymbolBody(COFFSymbolRef Sym, const void *AuxP, if (Sym.isCommon()) { Chunk *C = new (Alloc) CommonChunk(Sym); Chunks.push_back(C); - return new (Alloc) DefinedRegular(COFFObj.get(), Sym, C); + return new (Alloc) DefinedCommon(COFFObj.get(), Sym, C); } if (Sym.isAbsolute()) { COFFObj->getSymbolName(Sym, Name); diff --git a/lld/COFF/Symbols.cpp b/lld/COFF/Symbols.cpp index aaaf74c..af2cf0d 100644 --- a/lld/COFF/Symbols.cpp +++ b/lld/COFF/Symbols.cpp @@ -29,21 +29,21 @@ int DefinedRegular::compare(SymbolBody *Other) { auto *R = dyn_cast(Other); if (!R) return 1; - - // Common symbols are weaker than other types of defined symbols. - if (isCommon() && R->isCommon()) - return (getCommonSize() < R->getCommonSize()) ? -1 : 1; - // TODO: we are not sure if regular defined symbol and common - // symbols are allowed to have the same name. - if (isCommon()) - return -1; - if (R->isCommon()) - return 1; if (isCOMDAT() && R->isCOMDAT()) return 1; return 0; } +int DefinedCommon::compare(SymbolBody *Other) { + if (Other->kind() < kind()) + return -Other->compare(this); + if (auto *D = dyn_cast(Other)) + return getSize() > D->getSize() ? 1 : -1; + if (isa(Other)) + return -1; + return 1; +} + int DefinedBitcode::compare(SymbolBody *Other) { assert(Other->kind() >= kind()); if (!isa(Other)) @@ -63,10 +63,12 @@ int DefinedBitcode::compare(SymbolBody *Other) { // resolution will be done accurately after lowering bitcode symbols // to regular symbols in addCombinedLTOObject(). if (auto *R = dyn_cast(Other)) { - if (!R->isCommon() && !R->isCOMDAT() && !Replaceable) + if (!R->isCOMDAT() && !Replaceable) return 0; return -1; } + if (isa(Other)) + return -1; return 0; } @@ -113,6 +115,12 @@ StringRef DefinedRegular::getName() { return Name; } +StringRef DefinedCommon::getName() { + if (Name.empty()) + COFFFile->getSymbolName(Sym, Name); + return Name; +} + ErrorOr> Lazy::getMember() { auto MBRefOrErr = File->getMember(&Sym); if (auto EC = MBRefOrErr.getError()) diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h index 7f4088b..62fbd54 100644 --- a/lld/COFF/Symbols.h +++ b/lld/COFF/Symbols.h @@ -49,6 +49,7 @@ public: DefinedAbsoluteKind, DefinedImportDataKind, DefinedImportThunkKind, + DefinedCommonKind, DefinedRegularKind, DefinedLast, LazyKind, @@ -130,11 +131,32 @@ public: bool isCOMDAT() const { return Data->isCOMDAT(); } int compare(SymbolBody *Other) override; - // Returns true if this is a common symbol. - bool isCommon() const { return Sym.isCommon(); } - uint32_t getCommonSize() const { return Sym.getValue(); } +private: + StringRef Name; + COFFObjectFile *COFFFile; + COFFSymbolRef Sym; + Chunk *Data; +}; + +class DefinedCommon : public Defined { +public: + DefinedCommon(COFFObjectFile *F, COFFSymbolRef S, Chunk *C) + : Defined(DefinedCommonKind), COFFFile(F), Sym(S), Data(C) {} + + static bool classof(const SymbolBody *S) { + return S->kind() == DefinedCommonKind; + } + + StringRef getName() override; + uint64_t getRVA() override { return Data->getRVA(); } + bool isExternal() override { return Sym.isExternal(); } + void markLive() override { Data->markLive(); } + uint64_t getFileOff() override { return Data->getFileOff(); } + int compare(SymbolBody *Other) override; private: + uint64_t getSize() { return Sym.getValue(); } + StringRef Name; COFFObjectFile *COFFFile; COFFSymbolRef Sym; diff --git a/lld/test/COFF/Inputs/common.yaml b/lld/test/COFF/Inputs/common.yaml new file mode 100644 index 0000000..53e5307 --- /dev/null +++ b/lld/test/COFF/Inputs/common.yaml @@ -0,0 +1,91 @@ +--- +header: + Machine: IMAGE_FILE_MACHINE_AMD64 + Characteristics: [] +sections: + - Name: .text + Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ] + Alignment: 4 + SectionData: b800000000b800000000b800000000b800000000b800000000 + Relocations: + - VirtualAddress: 1 + SymbolName: bssdata4 + Type: IMAGE_REL_AMD64_ADDR32 + - VirtualAddress: 6 + SymbolName: bsspad1 + Type: IMAGE_REL_AMD64_ADDR32 + - VirtualAddress: 11 + SymbolName: bssdata64 + Type: IMAGE_REL_AMD64_ADDR32 + - VirtualAddress: 16 + SymbolName: bsspad2 + Type: IMAGE_REL_AMD64_ADDR32 + - VirtualAddress: 21 + SymbolName: bssdata16 + Type: IMAGE_REL_AMD64_ADDR32 + - Name: .data + Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ] + Alignment: 4 + SectionData: 03000000 +symbols: + - Name: .text + Value: 0 + SectionNumber: 1 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_STATIC + SectionDefinition: + Length: 0 + NumberOfRelocations: 5 + NumberOfLinenumbers: 0 + CheckSum: 0 + Number: 0 + - Name: .data + Value: 0 + SectionNumber: 2 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_STATIC + SectionDefinition: + Length: 4 + NumberOfRelocations: 0 + NumberOfLinenumbers: 0 + CheckSum: 0 + Number: 0 + - Name: mainCRTStartup + Value: 0 + SectionNumber: 1 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_FUNCTION + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: bssdata4 + Value: 4 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: bsspad1 + Value: 1 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: bssdata64 + Value: 64 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: bsspad2 + Value: 1 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: bssdata16 + Value: 16 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_NULL + StorageClass: IMAGE_SYM_CLASS_EXTERNAL +... diff --git a/lld/test/COFF/common.test b/lld/test/COFF/common.test new file mode 100644 index 0000000..b9c0a2b --- /dev/null +++ b/lld/test/COFF/common.test @@ -0,0 +1,10 @@ +# RUN: yaml2obj %p/Inputs/common.yaml > %t.obj +# RUN: lld -flavor link2 /out:%t.exe %t.obj %t.obj +# RUN: llvm-objdump -d %t.exe | FileCheck %s + +# Operands of B8 (MOV EAX) are common symbols +CHECK: 3000: b8 00 10 00 40 +CHECK: 3005: b8 10 10 00 40 +CHECK: 300a: b8 20 10 00 40 +CHECK: 300f: b8 60 10 00 40 +CHECK: 3014: b8 70 10 00 40 -- 2.7.4