From: Alexey Lapshin Date: Tue, 27 Jun 2023 19:27:34 +0000 (+0200) Subject: [DWARFv5][DWARFLinker] Remove dsymutil-classic compatibility feature as it leads... X-Git-Tag: upstream/17.0.6~3439 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=4546015f867ac19823116374bb1fb21f27bd2447;p=platform%2Fupstream%2Fllvm.git [DWARFv5][DWARFLinker] Remove dsymutil-classic compatibility feature as it leads to an error. DWARFLinker has a compatibility feature with dsymutil-classic. It may keep location expression attribute even if does not reference live address. Current llvm-dwarfdump --verify reports a error if variable references an address but is not added into the .debug_names table. error: Name Index @ 0x0: Entry for DIE @ 0xf35 (DW_TAG_variable) with name seed missing. DW_TAG_variable DW_AT_name ("seed") DW_AT_type (0x00000000000047b7 "uint64_t") DW_AT_location (DW_OP_addr 0x9ff8) <<<< dead address DWARFLinker does not add the variable into .debug_names table because it references dead address. To have a valid variable and consistent accelerator table it is necessary to remove location expression referencing dead address. This patch removes dsymutil-classic compatibilty feature. Differential Revision: https://reviews.llvm.org/D153988 --- diff --git a/llvm/include/llvm/DWARFLinker/DWARFLinker.h b/llvm/include/llvm/DWARFLinker/DWARFLinker.h index 8f1d2d0..e24fa2e 100644 --- a/llvm/include/llvm/DWARFLinker/DWARFLinker.h +++ b/llvm/include/llvm/DWARFLinker/DWARFLinker.h @@ -592,9 +592,12 @@ private: /// This function checks whether variable has DWARF expression containing /// operation referencing live address(f.e. DW_OP_addr, DW_OP_addrx...). - /// \returns relocation adjustment value if live address is referenced. - std::optional getVariableRelocAdjustment(AddressesMap &RelocMgr, - const DWARFDie &DIE); + /// \returns first is true if the expression has an operation referencing an + /// address. + /// second is the relocation adjustment value if the live address is + /// referenced. + std::pair> + getVariableRelocAdjustment(AddressesMap &RelocMgr, const DWARFDie &DIE); /// Check if a variable describing DIE should be kept. /// \returns updated TraversalFlags. diff --git a/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h b/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h index 3736033..08ebd4b 100644 --- a/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h +++ b/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h @@ -95,6 +95,9 @@ public: /// Is this a reference to a DIE that hasn't been cloned yet? bool UnclonedReference : 1; + /// Is this a variable with a location attribute referencing address? + bool HasLocationExpressionAddr : 1; + #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) LLVM_DUMP_METHOD void dump(); #endif // if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) diff --git a/llvm/lib/DWARFLinker/DWARFLinker.cpp b/llvm/lib/DWARFLinker/DWARFLinker.cpp index 206ba9d..a954a1a 100644 --- a/llvm/lib/DWARFLinker/DWARFLinker.cpp +++ b/llvm/lib/DWARFLinker/DWARFLinker.cpp @@ -427,7 +427,7 @@ static bool isTlsAddressCode(uint8_t DW_OP_Code) { DW_OP_Code == dwarf::DW_OP_GNU_push_tls_address; } -std::optional +std::pair> DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, const DWARFDie &DIE) { assert((DIE.getTag() == dwarf::DW_TAG_variable || @@ -441,7 +441,7 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, std::optional LocationIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_location); if (!LocationIdx) - return std::nullopt; + return std::make_pair(false, std::nullopt); // Get offset to the DW_AT_location attribute. uint64_t AttrOffset = @@ -451,14 +451,14 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, std::optional LocationValue = Abbrev->getAttributeValueFromOffset(*LocationIdx, AttrOffset, *U); if (!LocationValue) - return std::nullopt; + return std::make_pair(false, std::nullopt); // Check that DW_AT_location attribute is of 'exprloc' class. // Handling value of location expressions for attributes of 'loclist' // class is not implemented yet. std::optional> Expr = LocationValue->getAsBlock(); if (!Expr) - return std::nullopt; + return std::make_pair(false, std::nullopt); // Parse 'exprloc' expression. DataExtractor Data(toStringRef(*Expr), U->getContext().isLittleEndian(), @@ -466,6 +466,7 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, DWARFExpression Expression(Data, U->getAddressByteSize(), U->getFormParams().Format); + bool HasLocationAddress = false; uint64_t CurExprOffset = 0; for (DWARFExpression::iterator It = Expression.begin(); It != Expression.end(); ++It) { @@ -482,15 +483,17 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, break; [[fallthrough]]; case dwarf::DW_OP_addr: { + HasLocationAddress = true; // Check relocation for the address. if (std::optional RelocAdjustment = RelocMgr.getExprOpAddressRelocAdjustment( *U, Op, AttrOffset + CurExprOffset, AttrOffset + Op.getEndOffset())) - return *RelocAdjustment; + return std::make_pair(HasLocationAddress, *RelocAdjustment); } break; case dwarf::DW_OP_constx: case dwarf::DW_OP_addrx: { + HasLocationAddress = true; if (std::optional AddressOffset = DIE.getDwarfUnit()->getIndexedAddressOffset( Op.getRawOperand(0))) { @@ -499,7 +502,7 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, RelocMgr.getExprOpAddressRelocAdjustment( *U, Op, *AddressOffset, *AddressOffset + DIE.getDwarfUnit()->getAddressByteSize())) - return *RelocAdjustment; + return std::make_pair(HasLocationAddress, *RelocAdjustment); } } break; default: { @@ -509,7 +512,7 @@ DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr, CurExprOffset = Op.getEndOffset(); } - return std::nullopt; + return std::make_pair(HasLocationAddress, std::nullopt); } /// Check if a variable describing DIE should be kept. @@ -532,16 +535,20 @@ unsigned DWARFLinker::shouldKeepVariableDIE(AddressesMap &RelocMgr, // if the variable has a valid relocation, so that the DIEInfo is filled. // However, we don't want a static variable in a function to force us to keep // the enclosing function, unless requested explicitly. - std::optional RelocAdjustment = + std::pair> LocExprAddrAndRelocAdjustment = getVariableRelocAdjustment(RelocMgr, DIE); - if (RelocAdjustment) { - MyInfo.AddrAdjust = *RelocAdjustment; - MyInfo.InDebugMap = true; - } + if (LocExprAddrAndRelocAdjustment.first) + MyInfo.HasLocationExpressionAddr = true; - if (!RelocAdjustment || ((Flags & TF_InFunctionScope) && - !LLVM_UNLIKELY(Options.KeepFunctionForStatic))) + if (!LocExprAddrAndRelocAdjustment.second) + return Flags; + + MyInfo.AddrAdjust = *LocExprAddrAndRelocAdjustment.second; + MyInfo.InDebugMap = true; + + if (((Flags & TF_InFunctionScope) && + !LLVM_UNLIKELY(Options.KeepFunctionForStatic))) return Flags; if (Options.Verbose) { @@ -1652,9 +1659,10 @@ void DWARFLinker::DIECloner::addObjCAccelerator(CompileUnit &Unit, } } -static bool shouldSkipAttribute( - bool Update, DWARFAbbreviationDeclaration::AttributeSpec AttrSpec, - uint16_t Tag, bool InDebugMap, bool SkipPC, bool InFunctionScope) { +static bool +shouldSkipAttribute(bool Update, + DWARFAbbreviationDeclaration::AttributeSpec AttrSpec, + bool SkipPC) { switch (AttrSpec.Attr) { default: return false; @@ -1682,15 +1690,7 @@ static bool shouldSkipAttribute( return !Update; case dwarf::DW_AT_location: case dwarf::DW_AT_frame_base: - // FIXME: for some reason dsymutil-classic keeps the location attributes - // when they are of block type (i.e. not location lists). This is totally - // wrong for globals where we will keep a wrong address. It is mostly - // harmless for locals, but there is no point in keeping these anyway when - // the function wasn't linked. - return !Update && - (SkipPC || (!InFunctionScope && Tag == dwarf::DW_TAG_variable && - !InDebugMap)) && - !DWARFFormValue(AttrSpec.Form).isFormClass(DWARFFormValue::FC_Block); + return !Update && SkipPC; } } @@ -1770,11 +1770,15 @@ DIE *DWARFLinker::DIECloner::cloneDIE(const DWARFDie &InputDIE, // is not, e.g., inlined functions. if ((Flags & TF_InFunctionScope) && Info.InDebugMap) Flags &= ~TF_SkipPC; + // Location expressions referencing an address which is not in debug map + // should be deleted. + else if (!Info.InDebugMap && Info.HasLocationExpressionAddr && + LLVM_LIKELY(!Update)) + Flags |= TF_SkipPC; } for (const auto &AttrSpec : Abbrev->attributes()) { - if (shouldSkipAttribute(Update, AttrSpec, Die->getTag(), Info.InDebugMap, - Flags & TF_SkipPC, Flags & TF_InFunctionScope)) { + if (shouldSkipAttribute(Update, AttrSpec, Flags & TF_SkipPC)) { DWARFFormValue::skipValue(AttrSpec.Form, Data, &Offset, U.getFormParams()); continue; diff --git a/llvm/test/tools/dsymutil/X86/dead-stripped.cpp b/llvm/test/tools/dsymutil/X86/dead-stripped.cpp index 16bbbd6..6ea5ef2 100644 --- a/llvm/test/tools/dsymutil/X86/dead-stripped.cpp +++ b/llvm/test/tools/dsymutil/X86/dead-stripped.cpp @@ -18,13 +18,12 @@ // CHECK: DW_TAG_namespace namespace N { int blah = 42; -// This is actually a dsymutil-classic bug that we reproduced // CHECK: DW_TAG_variable -// CHECK: DW_AT_location +// CHECK-NOT: DW_AT_location __attribute__((always_inline)) int foo() { return blah; } // CHECK: DW_TAG_subprogram -// CHECK: DW_AT_frame_base +// CHECK-NOT: DW_AT_frame_base // CHECK: DW_TAG_subprogram @@ -35,7 +34,7 @@ int bar(unsigned i) { return foo(); } // CHECK: DW_TAG_subprogram -// CHECK: DW_AT_frame_base +// CHECK-NOT: DW_AT_frame_base // CHECK: DW_TAG_formal_parameter // CHECK: DW_TAG_variable // CHECK: DW_TAG_inlined_subroutine