[PATCH] D49522: [DWARF v5] Don't emit duplicate DW_AT_rnglists_base and address review comments from previous review

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:788
     DwarfCompileUnit &U = SkCU ? *SkCU : TheCU;
     if (unsigned NumRanges = TheCU.getRanges().size()) {
+      if (getDwarfVersion() >= 5 && !useSplitDwarf() &&
----------------
dblaikie wrote:
> This doesn't look like the right place to add the rnglists_base - it's possible for there to be ranges without the CU itself having ranges. (eg: a CU could have one function containing one scope - the CU would have a low/high pc (not ranges) but would still need rnglists_base so the scope could use rnglistx), I think?
> 
> So it probably can be pulled out of this conditional (& a test case added to cover/demonstrate this situation)
Right. All CU ranges are added to the range list vector by attachRangesOrLowHighPC() (unless the CU just uses low/high so we can use the size of the vector to decide whether to add the attribute.

There seem to be 3 distinct cases:

- at least one scope range: requires a range list table (and hence the attribute), even if there is only one CU range. This is covered by fission-ranges.ll
- otherwise, if there is more than one CU range, we need a table & attribute. The new test case rnglists_curanges.ll covers this.
- if there is only one CU range, no list or attribute is needed. This is covered by a mod to cu-ranges.ll


================
Comment at: test/DebugInfo/X86/rnglists_base_attr.ll:9
+; void f1() {
+;  if (bool b = x) {}
+; }
----------------
dblaikie wrote:
> total side note: (& I stumbled across the same code sample as you did here while making some range tests) this code probably shoudln't produce a DW_AT_ranges at -O0. For some reason the bool initialization is done inside the new/nested scope (which seems good/proper/correct) but the boolean test is not (which seems a bit weird/bogus)... 
> 
> Fine for this test case (though I'd tend to err on the side of something a bit more intentional, though it's more work - like introducing a scope & then manually reordering some instructions to introduce a "hole" & force ranges to be used) because even if clang is fixed, the IR here will remain as-is.
I modified the test like you suggested. Not that much work, really.


https://reviews.llvm.org/D49522