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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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() &&
----------------
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)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2103-2106
+    return (llvm::all_of(
+        CUMap, [](const decltype(CUMap)::const_iterator::value_type &Pair) {
+          return Pair.second->getRangeLists().empty();
+        }));
----------------
Probably drop the extra () around the return expression?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2140-2141
+
+  if (getDwarfVersion() >= 5) {
+    assert(TableEnd && "End label for rnglists table not created");
+    Asm->OutStreamer->EmitLabel(TableEnd);
----------------
Rather than testing the dwarf version and then asserting table end, maybe just use TableEnd as the test:

  if (TableEnd)
    Asm->OutStreamer->EmitLabel(TableEnd);


================
Comment at: test/DebugInfo/X86/rnglists_base_attr.ll:9
+; void f1() {
+;  if (bool b = x) {}
+; }
----------------
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.


https://reviews.llvm.org/D49522