diff --git a/llvm/include/llvm/DWARFLinker/DWARFLinker.h b/llvm/include/llvm/DWARFLinker/DWARFLinker.h --- a/llvm/include/llvm/DWARFLinker/DWARFLinker.h +++ b/llvm/include/llvm/DWARFLinker/DWARFLinker.h @@ -592,9 +592,12 @@ /// 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 --- a/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h +++ b/llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h @@ -95,6 +95,9 @@ /// 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 --- a/llvm/lib/DWARFLinker/DWARFLinker.cpp +++ b/llvm/lib/DWARFLinker/DWARFLinker.cpp @@ -427,7 +427,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ RelocMgr.getExprOpAddressRelocAdjustment( *U, Op, *AddressOffset, *AddressOffset + DIE.getDwarfUnit()->getAddressByteSize())) - return *RelocAdjustment; + return std::make_pair(HasLocationAddress, *RelocAdjustment); } } break; default: { @@ -509,7 +512,7 @@ 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 @@ // 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 @@ } } -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 @@ 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 @@ // 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 --- 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 @@ 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