
<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Apr 30, 2018, at 11:54 AM, 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" style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, Apr 30, 2018 at 11:46 AM Greg Clayton <<a href="mailto:clayborg@gmail.com" class="">clayborg@gmail.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Apr 30, 2018, at 11:41 AM, Jonas Devlieghere <<a href="mailto:jdevlieghere@apple.com" target="_blank" class="">jdevlieghere@apple.com</a>> wrote:</div><br class="m_-8497800645187476353Apple-interchange-newline"><div class=""><div style="word-wrap: break-word; line-break: after-white-space;" class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Apr 30, 2018, at 6:28 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class="m_-8497800645187476353Apple-interchange-newline"><div class=""><div dir="ltr" class="">Sorry - could we revisit this?<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Of course :-) </div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><br class="">I think my concerns expressed still haven't been resolved:<br class=""><br class="">"<span style="color: rgb(33, 33, 33);" class="">Could you check the revision history here? I'm pretty sure the first version of this I reviewed from Greg wasn't recursive - and then it became recursive at some point to handle something needed, but maybe those decisions need to be reexamined?" (& related comments back there)</span></div></div></blockquote><div class=""><br class=""></div><div class="">Greg added the recursiveness in D40156 because he saw DWARFDie::getName() failing “in the wild”. I think it was for some Google project but I’d have to double check the mail thread. </div><div class=""><br class=""></div><div class="">IIRC we all agreed that for “valid” DWARF we shouldn’t need the recursion. The question is whether we want to support the “invalid case or not, as it’s definitely being generated (but maybe not by clang?). If the answer is yes, then r331200 is strictly better than what we had before. Now we don’t crash if there’s a cycle. If the answer is no, we just need to go back to the non-recursive implementation. Personally I don’t have a strong opinion, as long as we don’t crash.</div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class="">I indeed did see a DIE that had a DW_AT_specification and pointed to another die that also had a DW_AT_specification. Regardless of what the DWARF spec says, compilers or post production DWARF tools are are producing DWARF that has these multiple chaining of specifications or abstract origins. There is also no enforcement of the DWARF spec anywhere, so compilers produce a wide variety of different DWARF that may or may not be legal. We have "llvm-dwarfdump --verify" but it isn't exhaustive in what it reports. So I would vote that we have a DWARF parser that can handle what ever is thrown at it when possible. Just my 2 cents.</div></div></blockquote><div class=""><br class="">I'm not suggesting the DWARF parser should crash or be unhelpful on such inputs - but the convenience of following abstract origin/specification DIE references to print names seems OK if it's limited to the situations we know make sense until we see something else?<br class=""><br class="">So if we could see/figure out what producer is generating such DWARF and look an some examples, decide if it's meaningful/useful to support name printing in that case, I'd be all for it. Without that, I'd rather err on the side of simplicity and not support the recursive case (& then have to handle/defend against cycles, etc).<br class=""></div></div></div></div></blockquote><div><br class=""></div>We have many tools here at Facebook that use LLVM's DWARF parser to symbolicate information and rely on the getting the names correctly from any producer of DWARF. I can't tell you exactly which compilers generate DWARF with multiple indirections, but I do know it happens. There are also many post production tools that we have here and take an existing linked binary and produce a new binary and fix the DWARF up. So in these cases, it isn't the compiler itself that ends up vending the final DWARF. So even if we identify the compiler that might produce this DWARF, there is no guarantee that the DWARF won't get changed. Granted, none of these tools will change abbreviation declarations that I know of. </div><div><br class=""></div><div>That being said I would be fine not dealing with recursion as the case that I fixed didn't have recursion. And if the DWARF is invalid I don't know how much we can/should do to try and recover.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div class="gmail_quote"><div class=""><br class="">- Dave<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">Greg</div></div><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Jonas </div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, Apr 30, 2018 at 10:06 AM Jonas Devlieghere via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">This revision was automatically updated to reflect the committed changes.<br class="">Closed by commit rL331200: [DebugInfo] Prevent infinite recursion for malformed DWARF (authored by JDevlieghere, committed by ).<br class=""><br class="">Changed prior to commit:<br class=""> <span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D43092?vs=144485&id=144582#toc" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D43092?vs=144485&id=144582#toc</a><br class=""><br class="">Repository:<br class=""> <span class="Apple-converted-space"> </span>rL LLVM<br class=""><br class=""><a href="https://reviews.llvm.org/D43092" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D43092</a><br class=""><br class="">Files:<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/lib/DebugInfo/DWARF/DWARFDie.cpp<br class=""> <span class="Apple-converted-space"> </span>llvm/trunk/test/tools/llvm-dwarfdump/X86/invalid_abstract_origin.s</blockquote></div></div></blockquote></div></div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div><br class=""></body></html>