[PATCH] D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly.
Greg Clayton via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 16 11:38:01 PDT 2017
clayborg added inline comments.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:46
+ bool insert(const DWARFAddressRangesVector &UnsortedRanges);
+ };
+
----------------
Yes this can be moved into DieRangeInfo
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:52
+
+ DieRangeInfo() : Die(), Ranges() {}
+ DieRangeInfo(DWARFDie D, const DWARFAddressRangesVector &R);
----------------
dblaikie wrote:
> I think the "Die(), Ranges()" is redundant/unnecessary & probably best to write this as:
>
> DieRangeInfo() = default;
Will change it.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:74
+ bool insertAndReportErrors(raw_ostream &OS, const DieRangeInfo &RI);
+ };
+
----------------
We need both. NonOverlappingRanges keeps a set of "DIE + ranges" so we can tell which DIE had ranges that don't overlap. DieRangeInfo is ranges for a specific Die. For all function address ranges (AllFunctionRanges below), we need to know which DIE the range came from.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFVerifier.h:83
+ struct NonOverlappingRanges {
+ std::set<DieRangeInfo> RangeSet;
+
----------------
dblaikie wrote:
> Does llvm's IntervalMap make sense here? Keeping an IntervalMap from the address range to the DIE that occupies that range seems good & less code (no need to write a custom op<, etc) I think... probably.
I don't it does. Looking at the IntervalMap::insert() function headerdoc:
```
/// insert - Add a mapping of [a;b] to y, coalesce with adjacent intervals.
/// It is assumed that no key in the interval is mapped to another value, but
/// overlapping intervals already mapped to y will be coalesced.
```
So it is says it can have no keys mapped to different values, and we are watching for possible overlaps and possible DIEs that have the same range.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:51
+ // Insert the range into our sorted list
+ Ranges.insert(InsertPos, R);
+ }
----------------
Longer ranges will appear at the end since the comparison does both the Start and End so this should work right?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:153
+ }
+ return Die.getOffset() < RHS.Die.getOffset();
+}
----------------
Yes, we need to find errors if two DIEs have the same low/high PC.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:180
+ // contained in this DieInfo's address ranges except if this is a compile
+ // unit that doesn't have address range(s).
+ bool IsCompileUnit = RI.Die.getTag() == DW_TAG_compile_unit;
----------------
You got it right. A CU may or may not have ranges and if it doesn't we don't need to check if DW_TAG_subprogram DIEs are contained. Lexical blocks and inline functions must be contained in parent.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:188
+ : "error: DIE has a child DIE whose address ranges "
+ "are not contained in its ranges:";
+ if (CheckContains && RI.reportErrorIfNotContained(OS, DieRI, Error))
----------------
One error message will be fine, I'll change it.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:245
+ ++NumDebugInfoErrors;
+ break;
+ case DW_TAG_subprogram:
----------------
The "NOR" in the UnitDI never gets cleared, so it is doing the cross CU checking. UnitDI.RI gets updated with the current CU info, but UnitDI.NOR never gets cleared.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:117
+ HasErrors = true;
+ }
+ if (HasErrors) {
----------------
This is detecting ranges **within** a Die that overlap or are invalid and the one and only Die is dumped below. This isn't checking if multiple Dies overlap, that is elsewhere. I do need to add a test for this though
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:208
+ const char *Error = "error: CU DIE has child with address ranges that are "
+ "not contained in its ranges:\n";
+ if (!UnitRI.Ranges.empty() &&
----------------
DW_TAG_subprogram DIEs are always direct children of the CU DIE so this special case is needed. We are always passing a parent range info in ParentRI, but that might be a DW_TAG_class_type for a DW_TAG_subprogram. So for DW_TAG_subprogram we always check against the CU and only check against the CU if it has ranges. For blocks we always check if the block has ranges, then the parent must have the ranges. So this needs to stay.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:212
+ ++NumDebugInfoErrors;
+ }
+
----------------
It is ok for the CU to be empty, not when you have a DW_TAG_lexical_block. If it has address range(s), then the parent must contain it. Not true for compile units. So the empty check is saying "it is ok if you just have a DW_AT_low_pc". If the CU has a low/high PC or a DW_AT_ranges, then it must contain all functions.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:216
+ // from any compile unit and make an error if needed.
+ if (AllFunctionRanges.insertAndReportErrors(OS, RI))
+ ++NumDebugInfoErrors;
----------------
This is the easiest way to do it.
> I would expect that functions would be compared to their enclosing CU - if all CUs are not overlapping and all functions are contained within their CU's range then no function could be overlapping (in the same way that if all lexical scopes are contained within their parent, and their parents are non-overlapping (eg: different, non-overlapping functions) then such a pair of lexical scopes couldn't overlap)
So what if a CU has no ranges? Then we need to actually make and keep a list of all CUs and then compare them at the end. And we would still need to tell which DIEs overlapped in the end. This is easier and cleaner IMHO.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:217
+ if (AllFunctionRanges.insertAndReportErrors(OS, RI))
+ ++NumDebugInfoErrors;
+
----------------
It checks 3 things:
1 - if the DIE has bad ranges or ranges that overlap within itself
2 - if the CU has ranges, make sure its ranges are in them
3 - make sure that the function doesn't overlap globally with any other function's address range
This seems pretty simple.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:224-225
+ ++NumDebugInfoErrors;
+ Die.dump(OS, 0);
+ OS << "\n";
+ }
----------------
The above two lines need to be removed.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:238
+ if (NOR.insertAndReportErrors(OS, RI))
+ ++NumDebugInfoErrors;
+ } break;
----------------
Sounds good.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:60
+ raw_ostream &OS) {
+ Ranges.clear();
+ auto UnsortedRanges = Die.getAddressRanges();
----------------
dblaikie wrote:
> Perhaps this should go in 'insert' which is only called from this function anyway?
DieRangeInfo::extractRangesAndReportErrors() is called from two places. This function calls DWARFVerifier::DieRangeInfo::insert() and also the DieRangeInfo constructor calls it for testing.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:70
+ HasErrors = true;
+ return false;
+ });
----------------
dblaikie wrote:
> Guessing this should be 'return true;' - otherwise no elements will ever be removed?
>
> Is that tested? (what happens if elements aren't removed here? what would that do to the rest of the code/validation?)
It didn't affect the tests because DWARFVerifier::DieRangeInfo::insert() was removing them when they were inserted. I have removed the check from insert and added an assert that will fire and catch the above error when the final "return false;" is in, but succeeds when it is true. The DWARFVerifier::DieRangeInfo::insert() is used for testing with the DieRangeInfo constructor and those ranges must to be valid (sorted and non overlapping) and the assert that was added enforces that.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:84
+ OS << "\n";
+ }
+ return HasErrors;
----------------
Again, this is only checking if the ranges within 1 DIE overlap while it is adding these ranges. Only 1 die needs to be printed.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:131
+ return true;
+ }
+ SearchBegin = Iter;
----------------
The logic is simpler now, let me know if it still needs to be broken out.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:160
+ bool IsCompileUnit = RI.Die.getTag() == DW_TAG_compile_unit;
+ bool CheckContains = !IsCompileUnit || !RI.Ranges.empty();
+
----------------
rangless subprograms are fine, but not rangeless subprograms with ranged lexical blocks. So:
1 - Ranged CU with ranged subprogram -> OK as long as contained
2 - Rangless CU with rangeless subprogram -> OK
3 - Ranged CU with rangeless subprogram -> OK
1 - Rangless subprogram/block/inlined with contained rangeless block/inlined -> OK
2 - Rangless subprogram/block/inlined with contained ranged block/inlined -> bad
3 - Ranged subprogram/block/inlined with contained ranged block/inlined -> OK as long as contained
#2 is where they differ
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:216
+ UnitDI.RI.Die = Die;
+ if (UnitDI.RI.extractRangesAndReportErrors(OS))
+ ++NumDebugInfoErrors;
----------------
NOR is used to detect any functions overlapping. So the UnitDI.RI is used to track the Unit DIE and its ranges so each subprogram in the CU can verify it is in the CU ranges if the CU has range(s), and the NOR is never cleared across all CUs so we can verify that no functions overlap within a CU. So this is already happening.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:219
+ break;
+ case DW_TAG_subprogram:
+ // Add the subprogram to the unit DIE and make it is contained.
----------------
overlapping CUs happens below. Each DW_TAG_subprogram will check if it is in the UnitDI.RI's ranges and also add itself to the UnitDI.NOR to verify no functions globally overlap.
https://reviews.llvm.org/D32821
More information about the llvm-commits
mailing list