<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 3, 2017, at 1:45 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, May 3, 2017 at 1:32 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="">
<br class="">
In <a href="https://reviews.llvm.org/D32821#745213" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D32821#745213</a>, @dblaikie wrote:<br class="">
<br class="">
> 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 class="">
><br class="">
> 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 class="">
<br class="">
<br class="">
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 class=""><br class="">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 class=""></div></div></div></div></blockquote><div><br class=""></div>That is how this is coded. Only top DW_TAG_subprogram DIEs get added to the AllFunctionRangeInfos. So when checking for overlap, we just check between adjacent DW_TAG_subprogram. Since these are in a std::set, they are ordered already. So we just check:</div><div><br class=""></div><div>for I in AllFunctionRangeInfos.size():</div><div> AllFunctionRangeInfos[I].Contains(AllFunctionRangeInfos[I+1])</div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">* 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 class=""> </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 class="">
> 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 class="">
<br class="">
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 class=""><br class="">Sure enough - I'm not suggesting the check is a bad idea.<br class=""> </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 class=""><br class="">Optimized code can produce /many/ blocks & inline functions, FWIW.<br class=""> </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 class=""><br class="">RangeInfoType::DoesIntersect is O(N^2) - if the ranges are sorted it should be O(N), I would think?<br class=""><br class="">& 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 class=""></div></div></div></div></blockquote><div><br class=""></div>No, CheckForRangeErrors check a single DIEs address ranges and looks for low_pc > high_pc and removes any empty ranges (optimized out function often just have low_pc == high_pc). So CheckForRangeErrors only look at ranges in one single DIE.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> </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 class="">
<br class="">
><br class="">
><br class="">
> 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 class="">
<br class="">
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 class=""></blockquote><div class=""><br class="">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 class=""></div></div></div></div></blockquote><div><br class=""></div>We don't detect overlaps until the end. And the only thing we verify on the fly is if the containing DIE (DW_TAG_subprogram, or DW_TAG_lexical_block or DW_TAG_inlined_subroutine) contains all address ranges for the child. So a child DW_TAG_lexical_block or DW_TAG_inlined_subroutine will check that its ranges are fully contained in the parent's ranges.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">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 class=""></div></div></div></div></blockquote><div><br class=""></div>Child DW_TAG_subprogram DIEs verify that their address ranges are contained in DieRangeInfos[0]. </div><div><br class=""></div><div>So I think everything is happened pretty efficiently. I can improve both DWARFVerifier::RangeInfoType::Contains() and DWARFVerifier::RangeInfoType::DoesIntersect() to use std::lower_bound since we sort all ranges in CheckForRangeErrors if that is what you are worried about?</div></body></html>