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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

Sorry about the delay, I had comments on this but I never submitted them!...

Structure is good. Just a few nits about cleaning up using std::numeric_limits<uint64_t>::max() and -1ULL for max address and cleaning the code up by not having multiple locations that are implementing SectionAddress::contains() and SectionAddress::operator <= manually.



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:95
         ClangModuleName(ClangModuleName) {
+
     Info.resize(OrigUnit.getNumDIEs());
----------------
Remove whitespace only change.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1475-1476
   auto OrigUnitDie = OrigUnit.getUnitDIE(false);
-  uint64_t OrigLowPc =
-      dwarf::toAddress(OrigUnitDie.find(dwarf::DW_AT_low_pc), -1ULL);
+  uint64_t OrigLowPc = dwarf::toAddress(OrigUnitDie.find(dwarf::DW_AT_low_pc),
+                                        std::numeric_limits<uint64_t>::max());
   // Ranges addresses are based on the unit's low_pc. Compute the
----------------
use dwarf::toSectionedAddress here? We don't want to assume anything about the low PC section here do we?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1629-1633
+    if (CurrRange == InvalidRange ||
+        Row.Address.SectionIndex != CurrRange.start().SectionIndex ||
+        Row.Address.Address < CurrRange.start().Address ||
+        Row.Address.Address > CurrRange.stop().Address ||
+        (Row.Address.Address == CurrRange.stop().Address && !Row.EndSequence)) {
----------------
If we had a object::SectionAddressRange this code would look a lot cleaner:

```
if (!CurrRange.contains(Row.Address) || (Row.Address.Address == CurrRange.stop().Address && !Row.EndSequence))
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:451
 
-  Optional<uint64_t> HighPc = DIE.getHighPC(*LowPc);
+  Optional<uint64_t> HighPc = DIE.getHighPC(LowPc->Address);
   if (!HighPc) {
----------------
do we need to use dwarf::toSectionedAddress() and verify sections are the same?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1034
       // the low_pc from the input DIE if relocations have been applied.
       Addr = (Info.OrigLowPc != std::numeric_limits<uint64_t>::max()
                   ? Info.OrigLowPc
----------------
Can we make a constexpr MAX_ADDR or INVALID_ADDR for the max address and use it here instead of std::numeric_limits<uint64_t>::max() being inlined everywhere?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1040
       Addr = Unit.getLowPc();
       if (Addr == std::numeric_limits<uint64_t>::max())
         return 0;
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1099
       Die.getTag() == dwarf::DW_TAG_compile_unit) {
     if (Unit.getLowPc() == -1ULL)
       return 0;
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1477
   uint64_t OrigLowPc =
       dwarf::toAddress(OrigUnitDie.find(dwarf::DW_AT_low_pc), -1ULL);
   // Ranges addresses are based on the unit's low_pc. Compute the
----------------
constexpr MAX_ADDR or INVALID_ADDR and do we need to use dwarf::toSectionedAddress() now?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1481
   int64_t UnitPcOffset = 0;
   if (OrigLowPc != -1ULL)
     UnitPcOffset = int64_t(OrigLowPc) - Unit.getLowPc();
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1497-1504
+          First.SectionIndex != CurrRange.start().SectionIndex ||
+          First.StartAddress + OrigLowPc < CurrRange.start().Address ||
+          First.StartAddress + OrigLowPc >= CurrRange.stop().Address) {
+        CurrRange = FunctionRanges.find(
+            {First.StartAddress + OrigLowPc, First.SectionIndex});
+        if (CurrRange == InvalidRange || CurrRange == FunctionRanges.end() ||
+            CurrRange.start().SectionIndex != First.SectionIndex ||
----------------
This code would be much cleaner if we add a SectionedAddress::contains() method.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1639
+                                 ? CurrRange.stop().Address + CurrRange.value()
                                  : -1ULL;
+      CurrRange = FunctionRanges.find(Row.Address);
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1642-1644
+          CurrRange != InvalidRange &&
+          CurrRange.start().SectionIndex == Row.Address.SectionIndex &&
+          CurrRange.start().Address <= Row.Address.Address;
----------------
This code would be much cleaner if we add a SectionedAddress::operator <=() method.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1647
         CurrRange = InvalidRange;
         if (StopAddress != -1ULL) {
           // Try harder by looking in the Address ranges map.
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1659-1660
+          if (Range != Ranges.end() &&
+              Range->first.SectionIndex == Row.Address.SectionIndex &&
+              Range->first.Address <= Row.Address.Address &&
               Range->second.HighPC >= Row.Address.Address) {
----------------
This code would be much cleaner if we add a SectionedAddress::operator <=() method.




================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1666
       }
       if (StopAddress != -1ULL && !Seq.empty()) {
         // Insert end sequence row with the computed end address, but
----------------
constexpr MAX_ADDR or INVALID_ADDR


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:305-307
+    if (!(Range.SectionIndex == FuncRange.start().SectionIndex &&
+          Range.StartAddress + OrigLowPc >= FuncRange.start().Address &&
+          Range.EndAddress + OrigLowPc <= FuncRange.stop().Address))
----------------
This code would be much cleaner if we add a SectionedAddress::contains() method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76085