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

[Alexey Samsonov]

In http://reviews.llvm.org/D6497#100223, @friss wrote:

> In http://reviews.llvm.org/D6497#12, @samsonov wrote:
>
> > Finally looked at this patch, sorry for the delay.
> >
> > First of all, I dislike the extension of `InstrRange`  structure, and adding the extra list to DbgValueHistoryMap. After we returned from `calculateDbgValueHistory()`, we don't need and never use InstrRange::NonEmpty pointers, and the list, so it's not nice we leave this extra data lying around.
>
>
> I can understand that and especially the liveness of the NonEmpty field bothers me. However, let me restate my goal: I wanted to find a way to keep the pass at the same algorithmic complexity. DbgValueHistoryCalculator has already shown up on profiles and I wanted to avoid introducing new potentially costly bookkeeping logic. That being said, I might be too careful. (And I even failed at that, because the destructor of the list<> will do a O(<number of Dbg_VALUE>) walk anyway).
>
> > The comment about InstrRange explicitly tells that instruction range *may not* be terminated - it means the range is assumed to be valid either until the start of the next range,
>
>
> (not really relating to that actual patch: I do not see why the beginning of a variable range should close the previous one for that variable. A variable might be present in 2 registers at some point it is even explicitly stated in the Dwarf standard. I even think you can't get the end result right if you try to prevent this from happening.)


I don't say that ranges should be non-overlapping - they shouldn't in general case. I was just telling that apparently it was the case when this code was refactored, and this assumption seems to be baked in there. For instance, DbgValueHistoryMap::getRegisterForVar() assumes returns a single value, apparently assuming that a variable can be stored in a single register. We'd need to carefully audit the code, see how it works with new `DwarfDebug::buildLocationList` method, etc. I'm sort of surprised that the latter works with the current
structure of DbgValueHistoryMap::InstrRanges.

> 

> 

> > or until the end of function. However, you pop_back() "empty" instruction ranges only in endInstrRange() function. It means you may still end up with empty ranges if they were never closed.

> 

> 

> This is true, however the empty ranges could only appear at the very end of the function which is less of an issue. For an empty range to happen there, something would need to have generated a DBG_VALUE after the last instruction of the function (Is that even possible? isn't there a terminator requirement like at the higher level?). But yes, this pass runs so late (meaning the DBG_VALUEs could have been mishandled by so many things) that we need to take care of that case if we want to be exhaustive.

> 

> > Can we do the following instead: have another member:

> 

> > 

> 

> >   std::map<const MDNode *, InstrRange> emptyRanges;

> 

> > 

> 

> > which would describe the current, opened and empty, range for a given variable.

> 

> 

> You can have multiple open ranges for a variable (range entries describing constants will never be closed for example). I do not like the limitation to only one pending range. Thus we'd need to make the map value type a list.

> 

> >   It means that smth. like

> 

> >    const InstrRange& getCurrentRangeForVar(const MDNode *Var);

> 

> > 

> 

> > would return either the entry from `emptyRanges`, or the last value of `VarInstrRanges` vector.

> 

> >  Then `startInstrRange` would simply add new range, and `markOpenRangesNonEmpty()` would move the contents of `emptyRanges` to `VarInstrRanges`. At the end of calculateDbgValueHistory, we can just discard the contents of `emptyRanges`.

> 

> 

> I'll try to use a simpler approach with a map<MDnode *, list<InstrRange>> if everybody agrees that the additional bookkeeping shouldn't be an issue.





http://reviews.llvm.org/D6497

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/