[PATCH] Prevent the creation of empty location list ranges.

[David Blaikie]

Yeah, I'm mildly curious to get a better understanding of where this comes up & whether we should just be dropping the dbg.value intrinsics earlier rather than waiting until we find that they describe nothing of interest - but it seems totally plausible that that wouldn't be easy/clean/convenient to do and that this (what you've proposed) is the right place to do it.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:48
@@ +47,3 @@
+  if (SeenInstructionInRange.back())
+    SeenInstructionInRange.push_back(false);
+  Ranges.push_back(
----------------
I don't follow what's going on here - maybe a comment is in order? (not sure why it's only pushing back if the back() is true). I guess whatever the answer is also explains why SeenInstructionInRange starts with size 1 rather than empty.

OK, I think I see what's going on here - rather than having entry in SeenInstructionInRange per live range, you avoid having them in the case where two ranges start together (or are not "valid" before teh second one starts, at least). Bit subtle, but I think I get it - an alternative idea/question:

How do you avoid having to walk all the elements of SeenInstructionInRange when performing validateOpenRanges? I would've imagined/expected that you'd need to set all the elements of the container to true, not just the last one, no?

In any case - could we just keep booleans directly in the InstrRanges - and validateOpenRanges would iterate through InstrRanges and set them all to true?

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:50
@@ -48,1 +49,3 @@
+  Ranges.push_back(
+    DbgValueHistoryMap::InstrRange{&MI, nullptr, &SeenInstructionInRange.back()});
 }
----------------
The pointer to the SmallVector element could end up dangling if the SmallVector is reallocated.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h:38
@@ +37,3 @@
+    const MachineInstr *first, *second;
+    const bool *NonEmpty; ///< Have we seen a real instruction in this range?
+  };
----------------
Do we need one of these on every InstrRange? I suppose this comes up if we have multiple open InstrRanges at the same time? Do we support/implement that scenario? (I imagine we probably don't - in which case we only need "NonEmpty" for the currently open InstrRange - so perhaps InstrRanges should be a struct of SmallVector<InstrRange> + bool *NonEmpty, so we just have it once per InstrRanges, rather than once for every range within the InstrRanges)

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h:55
@@ -40,2 +54,3 @@
   void endInstrRange(const MDNode *Var, const MachineInstr &MI);
+  void validateOpenRanges() { SeenInstructionInRange.back() = true; }
   // Returns register currently describing @Var. If @Var is currently
----------------
'validate' seems a bit vague, but maybe sufficient

================
Comment at: test/DebugInfo/X86/block-capture.ll:9
@@ +8,3 @@
+; location list. Now that we do not generate such invalid (empty) entries anymore,
+; the varible doesn't appear at all.
+; XFAIL: *
----------------
Just because we don't generate any locations doesn't mean we should omit the variable entirely - maybe a comment explaining that the missing variable is a (rather severe) bug, even if it doesn't have any location (another, perhaps less severe, bug, most likely)

http://reviews.llvm.org/D6497