[PATCH] D147066: [DWARFLinker][DWARFv5] Add handling of DW_OP_addrx and DW_OP_constx expression operands.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:48-62
+// This structure keeps patch for the location attribute and the value
+// of relocation which should be applied: either to the function ranges
+// if location attribute is of type 'loclist', either to the operand of
+// DW_OP_addr/DW_OP_addrx if location attribute is of type 'exprloc'.
+// ASSUMPTION: Location attributes of 'loclist' type containing 'exprloc'
+//             with address expression operands are not supported yet.
+struct PatchWithRelocAdjustment {
----------------
Instead of wrapping `PatchLocation` in another struct, would it make sense to add the `RelocAdjustment` to the `PatchLocation` itself?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1107
+      } else
+        Linker.reportWarning("Cannot read DW_OP_addrx operand.", File);
+    } else if (!Linker.Options.Update && Op.getCode() == dwarf::DW_OP_constx) {
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1126
+          Linker.reportWarning(
+              formatv(("Unsupported address size: {0}."), OrigAddressByteSize),
+              File);
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1140
+      } else
+        Linker.reportWarning("Cannot read DW_OP_constx operand.", File);
     } else {
----------------



================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:1006-1044
+  // Parse 'exprloc' expression.
+  DataExtractor Data(toStringRef(*Expr), U->getContext().isLittleEndian(),
+                     U->getAddressByteSize());
+  DWARFExpression Expression(Data, U->getAddressByteSize(),
+                             U->getFormParams().Format);
+
+  uint64_t CurExprOffset = 0;
----------------
The code to parse the exprloc here is very similar to that in the dsymutil variant. Any chance we could hoist some of this logic into the DWARFLinker? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147066/new/

https://reviews.llvm.org/D147066