[DWARFv5][DWARFLinker] Remove dsymutil-classic compatibility feature as it leads to an error.

[avl]

Summary

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.

[JDevlieghere]

LGTM

llvm/include/llvm/DWARFLinker/DWARFLinker.h

595–601

[fdeazeve]

llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h

99

The phrasing here is slightly broken.

Maybe something like: "Is this a variable with a location attribute referencing address?"

[avl]

Thank you for the review. Will update comments in the committed patch.

llvm/include/llvm/DWARFLinker/DWARFLinker.h

private:
 
   unsigned shouldKeepDIE(AddressesMap &RelocMgr, RangesTy &Ranges,
                          const DWARFDie &DIE, const DWARFFile &File,
                          CompileUnit &Unit, CompileUnit::DIEInfo &MyInfo,
                          unsigned Flags);
 
   /// 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<int64_t> 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<bool, std::optional<int64_t>>
+  getVariableRelocAdjustment(AddressesMap &RelocMgr, const DWARFDie &DIE);
 
   /// Check if a variable describing DIE should be kept.
   /// \returns updated TraversalFlags.
   unsigned shouldKeepVariableDIE(AddressesMap &RelocMgr, const DWARFDie &DIE,
                                  CompileUnit::DIEInfo &MyInfo, unsigned Flags);
 
   unsigned shouldKeepSubprogramDIE(AddressesMap &RelocMgr, RangesTy &Ranges,
                                    const DWARFDie &DIE, const DWARFFile &File,
                                    CompileUnit &Unit,

llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h

struct DIEInfo {
     bool InModuleScope : 1;
 
     /// Is ODR marking done?
     bool ODRMarkingDone : 1;
 
     /// 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)
   };
 
   CompileUnit(DWARFUnit &OrigUnit, unsigned ID, bool CanUseODR,
               StringRef ClangModuleName)
       : OrigUnit(OrigUnit), ID(ID), ClangModuleName(ClangModuleName) {

llvm/lib/DWARFLinker/DWARFLinker.cpp

void DWARFLinker::cleanupAuxiliarryData(LinkContext &Context) {
   DIEAlloc.Reset();
 }
 
 static bool isTlsAddressCode(uint8_t DW_OP_Code) {
   return DW_OP_Code == dwarf::DW_OP_form_tls_address ||
          DW_OP_Code == dwarf::DW_OP_GNU_push_tls_address;
 }
 
-std::optional<int64_t>
+std::pair<bool, std::optional<int64_t>>
 DWARFLinker::getVariableRelocAdjustment(AddressesMap &RelocMgr,
                                         const DWARFDie &DIE) {
   assert((DIE.getTag() == dwarf::DW_TAG_variable ||
           DIE.getTag() == dwarf::DW_TAG_constant) &&
          "Wrong type of input die");
 
   const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
 
   // Check if DIE has DW_AT_location attribute.
   DWARFUnit *U = DIE.getDwarfUnit();
   std::optional<uint32_t> 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 =
       Abbrev->getAttributeOffsetFromIndex(*LocationIdx, DIE.getOffset(), *U);
 
   // Get value of the DW_AT_location attribute.
   std::optional<DWARFFormValue> 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<ArrayRef<uint8_t>> 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(),
                      U->getAddressByteSize());
   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) {
     DWARFExpression::iterator NextIt = It;
     ++NextIt;
 
     const DWARFExpression::Operation &Op = *It;
     switch (Op.getCode()) {
     case dwarf::DW_OP_const4u:
     case dwarf::DW_OP_const8u:
     case dwarf::DW_OP_const4s:
     case dwarf::DW_OP_const8s:
       if (NextIt == Expression.end() || !isTlsAddressCode(NextIt->getCode()))
         break;
       [[fallthrough]];
     case dwarf::DW_OP_addr: {
+      HasLocationAddress = true;
       // Check relocation for the address.
       if (std::optional<int64_t> 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<uint64_t> AddressOffset =
               DIE.getDwarfUnit()->getIndexedAddressOffset(
                   Op.getRawOperand(0))) {
         // Check relocation for the address.
         if (std::optional<int64_t> RelocAdjustment =
                 RelocMgr.getExprOpAddressRelocAdjustment(
                     *U, Op, *AddressOffset,
                     *AddressOffset + DIE.getDwarfUnit()->getAddressByteSize()))
-          return *RelocAdjustment;
+          return std::make_pair(HasLocationAddress, *RelocAdjustment);
       }
     } break;
     default: {
       // Nothing to do.
     } break;
     }
     CurExprOffset = Op.getEndOffset();
   }
 
-  return std::nullopt;
+  return std::make_pair(HasLocationAddress, std::nullopt);
 }
 
 /// Check if a variable describing DIE should be kept.
 /// \returns updated TraversalFlags.
 unsigned DWARFLinker::shouldKeepVariableDIE(AddressesMap &RelocMgr,
                                             const DWARFDie &DIE,
                                             CompileUnit::DIEInfo &MyInfo,
                                             unsigned Flags) {
   const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
 
   // Global variables with constant value can always be kept.
   if (!(Flags & TF_InFunctionScope) &&
       Abbrev->findAttributeIndex(dwarf::DW_AT_const_value)) {
     MyInfo.InDebugMap = true;
     return Flags | TF_Keep;
   }
 
   // See if there is a relocation to a valid debug map entry inside this
   // variable's location. The order is important here. We want to always check
   // 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<int64_t> RelocAdjustment =
+  std::pair<bool, std::optional<int64_t>> LocExprAddrAndRelocAdjustment =
       getVariableRelocAdjustment(RelocMgr, DIE);
 
-  if (RelocAdjustment) {
-    MyInfo.AddrAdjust = *RelocAdjustment;
-    MyInfo.InDebugMap = true;
-  }
+  if (LocExprAddrAndRelocAdjustment.first)
+    MyInfo.HasLocationExpressionAddr = true;
+
+  if (!LocExprAddrAndRelocAdjustment.second)
+    return Flags;
+
+  MyInfo.AddrAdjust = *LocExprAddrAndRelocAdjustment.second;
 
-  if (!RelocAdjustment || ((Flags & TF_InFunctionScope) &&
-                           !LLVM_UNLIKELY(Options.KeepFunctionForStatic)))
+  if (((Flags & TF_InFunctionScope) &&
     return Flags;
 
   if (Options.Verbose) {
     outs() << "Keeping variable DIE:";
     DIDumpOptions DumpOpts;
     DumpOpts.ChildRecurseDepth = 0;
     DumpOpts.Verbose = Options.Verbose;
     DIE.dump(outs(), 8 /* Indent */, DumpOpts);
if (OpenParens != StringRef::npos) {
       //        dsymutil-classic also does it this way.
       MethodNameNoCategory.append(std::string(SelectorStart));
       Unit.addNameAccelerator(Die, StringPool.getEntry(MethodNameNoCategory),
                               SkipPubSection);
     }
   }
 }
 
-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;
   case dwarf::DW_AT_low_pc:
   case dwarf::DW_AT_high_pc:
   case dwarf::DW_AT_ranges:
     return !Update && SkipPC;
   case dwarf::DW_AT_addr_base:
shouldSkipAttribute(bool Update,
   case dwarf::DW_AT_loclists_base:
     // In case !Update the .debug_addr table is not generated/preserved.
     // Thus instead of DW_FORM_loclistx the DW_FORM_sec_offset is used.
     // Since DW_AT_loclists_base is used for only DW_FORM_loclistx the
     // DW_AT_loclists_base is removed.
     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;
   }
 }
 
 DIE *DWARFLinker::DIECloner::cloneDIE(const DWARFDie &InputDIE,
                                       const DWARFFile &File, CompileUnit &Unit,
                                       int64_t PCOffset, uint32_t OutOffset,
                                       unsigned Flags, bool IsLittleEndian,
                                       DIE *Die) {
if (Abbrev->getTag() == dwarf::DW_TAG_subprogram) {
     Flags |= TF_InFunctionScope;
     if (!Info.InDebugMap && LLVM_LIKELY(!Update))
       Flags |= TF_SkipPC;
   } else if (Abbrev->getTag() == dwarf::DW_TAG_variable) {
     // Function-local globals could be in the debug map even when the function
     // 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;
     }
 
     DWARFFormValue Val = AttrSpec.getFormValue();
     uint64_t AttrSize = Offset;
     Val.extractValue(Data, &Offset, U.getFormParams(), &U);

llvm/test/tools/dsymutil/X86/dead-stripped.cpp

 // would just drop the CU altogether).
 
 // CHECK: DW_TAG_compile_unit
 // CHECK:   DW_AT_low_pc
 // CHECK:   DW_AT_high_pc
 // 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
 
 int bar(unsigned i) {
   int val = foo();
   if (i)
     return val + bar(i-1);
   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
 // CHECK:   NULL
 // CHECK: NULL
 }
 // CHECK: DW_TAG_base_type
 // CHECK: DW_TAG_imported_module