<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, May 3, 2017 at 1:32 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D32821#745213" rel="noreferrer" target="_blank">https://reviews.llvm.org/D32821#745213</a>, @dblaikie wrote:<br>
<br>
> I haven't followed some of the design direction Adrian's been pointing this work in - so all of this is subject to discussion, etc:<br>
><br>
> 1. I'd expect not to build an entire representation of all ranges up-front, then verify them (uses more memory, etc) - but rather verify during a traversal of the DIEs instead<br>
<br>
<br>
The whole reason we need this is to look for overlapping functions between all CUs. This helps us to identify exactly which DIEs overlap.</blockquote><div><br>If it's true that no DIEs should overlap, and all DIEs should be a subset of their parent - then there's no need to compare two DIEs that aren't siblings* because they can't overlap. The only comparisons that should be needed would be between immediate siblings, and siblings and parents - not all possible pairs of nodes. Right?<br><br>* for a slightly broad definition of siblings - ignoring any nodes that don't have ranges (eg: A:R { B { C:R { } } D:R { } } - D and C are siblings in this sense)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It is also just a DIE (2 pointers) + vector<pair<uint64_t, uint64_t>. Not too costly.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 2. for/for loops to check if one range is a subset of another's probably rather inefficient. Not sure if LLVM already has better tools for this, though I suspect not.<br>
<br>
It currently works and I have seen many errors introduced by the compilers when they optimize debug info and they get it wrong.</blockquote><div><br>Sure enough - I'm not suggesting the check is a bad idea.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> We only check blocks and inline functions against the containing ranges.</blockquote><div><br>Optimized code can produce /many/ blocks & inline functions, FWIW.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> We can make sure the ranges are coalesced when needed, but other than that, what is innefficient about what is there?</blockquote><div><br>RangeInfoType::DoesIntersect is O(N^2) - if the ranges are sorted it should be O(N), I would think?<br><br>& it looks like verifyDebugInfoTag, for example, calls CheckForRangeErrors against all other nodes at the same level (so, for example - if it was only looking at subprograms, each subprogram would be compared against all other subprograms that came before it - even those in other CUs, even when that other CU's ranges had been proven not to intersect with this CU - similarly one layer down, all the top level lexical_scopes would be compared against all other top level lexical scopes, even when the subprogram ranges for the two subprograms were distinct)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 99% of the time it is one range seeing if one other range is contained.<br>
<br>
><br>
><br>
> 3. I'm not sure that function ranges need to be special/separate - I'd expect any nested ranges (eg: the range of two scopes in a function) to be distinct as well, so I'd expect a more general algorithm that walks nodes and their children looking for ranges and checking that children are subsets of parents, and children are distinct from each other.<br>
<br>
Again, not sure what is wrong with the way it is doing it. We just keep a stack >= depth of the current DIE with ranges filled in. If a block or inlined function is found, it verifies it is in the parent. The DWARF doesn't store this information in any useful way we can access, so I believe this approach work well.<br></blockquote><div><br>I didn't mean to suggest it won't work - I'm confused by why there is a separate "verifyDebugInfoOVerlappingFunctionRanges" - I would've expected that to fall out naturally from the DIE walk range checking.<br><br>At the top level, I could imagine since CU DIEs don't have a common parent, the CU range overlap check might need to be special-cased rather than falling out naturally from the DIE walk - but only that.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D32821" rel="noreferrer" target="_blank">https://reviews.llvm.org/D32821</a><br>
<br>
<br>
<br>
</blockquote></div></div>