[PATCH] D76085: [DWARFLinker][dsymutil][NFC] add section index into address range.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

@aprantl Would probably prefer one of you folks more familiar with dsymutil/DWARFLinker to do final review on this



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1038-1050
     else if (Die.getTag() == dwarf::DW_TAG_compile_unit) {
-      Addr = Unit.getLowPc();
-      if (Addr == std::numeric_limits<uint64_t>::max())
+      const auto &pc = Unit.getLowPc();
+
+      if (pc != std::numeric_limits<uint64_t>::max())
+        Addr = pc;
+      else
         return 0;
----------------
What happened here? (is this change related/necessary for the rest of this patch)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1099-1103
+    if (Unit.getLowPc() != std::numeric_limits<uint64_t>::max())
+      // Dwarf >= 4 high_pc is an size, not an address.
+      Value = Unit.getHighPc() - Unit.getLowPc();
+    else
       return 0;
----------------
Skip unnecessary refactors (if this is unnecessary)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1640
+                                 ? CurrRange.stop().Address + CurrRange.value()
+                                 : std::numeric_limits<uint64_t>::max();
+      CurrRange = FunctionRanges.find(Row.Address);
----------------
(similarly - best to leave out changes that aren't related to this patch, if that's the case here)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1667
       }
-      if (StopAddress != -1ULL && !Seq.empty()) {
+      if (StopAddress != std::numeric_limits<uint64_t>::max() && !Seq.empty()) {
         // Insert end sequence row with the computed end address, but
----------------
Is this change motivated by/necessary/related to the rest of this patch? If not, best to leave it out.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinkerCompileUnit.cpp:138-139
     Ranges.insert(FuncLowPc, FuncHighPc, PcOffset);
-  this->LowPc = std::min(LowPc, FuncLowPc + PcOffset);
-  this->HighPc = std::max(HighPc, FuncHighPc + PcOffset);
+  LowPc = std::min(LowPc, FuncLowPc.Address + PcOffset);
+  HighPc = std::max(HighPc, FuncHighPc.Address + PcOffset);
 }
----------------
Probably commit the "this->" removal as a separate patch (no need to send that for review) to keep this change on-topic.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.h:118
               *Mapping.ObjectAddress + Mapping.Size,
-              int64_t(Mapping.BinaryAddress) - *Mapping.ObjectAddress);
+              int64_t(Mapping.BinaryAddress - *Mapping.ObjectAddress)};
       }
----------------
Why the change in () here?


================
Comment at: llvm/tools/dsymutil/DwarfStreamer.cpp:390-391
   // Offset each range by the right amount.
-  int64_t PcOffset = -Unit.getLowPc();
+  int64_t PcOffset = 0;
+  PcOffset = -Unit.getLowPc();
   // Emit coalesced ranges.
----------------
What's the purpose of this change? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76085