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

[Frederic Riss]

>>! In D6497#4, @dblaikie wrote:
> 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.

In my opinion this is the only place where we can safely do that, because it's the only place where we are sure that Instructions are at their final place in the asm flow.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:48
@@ +47,3 @@
+  if (SeenInstructionInRange.back())
+    SeenInstructionInRange.push_back(false);
+  Ranges.push_back(
----------------
dblaikie wrote:
> 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?
Basically each entry in SeenInstructionInRange represents the status of a series of DBG_VALUEs that start at the same PC. If the last entry is false, we can just reuse it, our DBG_VALUE starts at the same PC that all the current still-empty ranges. If the last entry is true, we need to add a new one that will get false till the nest real instruction.
Thus the container always contains only 'true' elements except maybe the last one that indicates that there are newly opened ranges that haven't seen an instruction yet.

And the fact that a range is opened first doesn't mean it'll get closed first. We need to keep the old 'true' values that are pointed to live. 

It's a bit subtle, but it's the only way I could think of that keeps the pass algorithmic  complexity the same. I fear that iterating the InstrRanges could lead to some really bad pathological cases.

If you want to store the flag directly in the InstrRange, you need a way to iterate only the non-validated ones to mark them non-empty. This isn't trivial as the primary storage of the InstrRange is in a SmallVector, thus their address isn't stable (I haven't investigated if there would be a drawback to storing them in a list instead).

I prefer the simplicity of just toggling the status for all the current entries by just modifying a bool value that everybody points to.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:50
@@ -48,1 +49,3 @@
+  Ranges.push_back(
+    DbgValueHistoryMap::InstrRange{&MI, nullptr, &SeenInstructionInRange.back()});
 }
----------------
dblaikie wrote:
> The pointer to the SmallVector element could end up dangling if the SmallVector is reallocated.
Bummer... I knew I had a good reason for having implemented that as a list first. And then I added the call to SeenInstructionInRange.resize() and thought that this would definitely be more efficient with a vector. 

================
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?
+  };
----------------
dblaikie wrote:
> 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)
We have multiple Ranges in the Same vector with different statuses. Today it's due to what I consider a bug: we never close ranges that aren't described by a register (i.e. a constant variable). But ultimately, even for registers it might make sense to have overlapping ranges for the same variable (the fact that a new DBG_VALUE describes the variable doesn't mean that the value mysteriously disappeared from the previous register).

So yes, I explicitly designed this solution to support that scenario.

================
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
----------------
dblaikie wrote:
> 'validate' seems a bit vague, but maybe sufficient
In an earlier iteration the added field in InstrRange was called 'Valid'. I could rename that to markOpenRangesNonEmpty().

================
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: *
----------------
dblaikie wrote:
> 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)
I thought this was implicit due to the XFAIL, but I can make it even more visible.

http://reviews.llvm.org/D6497