<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jan 9, 2017 at 2:55 PM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Jan 9, 2017, at 2:41 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-5703417138647522469Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Mon, Jan 9, 2017 at 2:29 PM Greg Clayton <<a href="mailto:clayborg@gmail.com" class="gmail_msg" target="_blank">clayborg@gmail.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Jan 9, 2017, at 2:23 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="gmail_msg m_-5703417138647522469m_4706928405125494520Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">I'm not sure optional error handling's ideal - seems errors should always be handled (which is the strong premise behind Lang's work introducing llvm::Error).</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg">What is a DWARF parser during a debug session gonna do with any of these errors? Emit them in the console? If we have borked DWARF we can’t just emit every error we come across, the user will not like that. </div></div></blockquote><div class="gmail_msg"><br class="gmail_msg">Quite possibly, yes. That seems to be what LLDB does already for DWARF it can't handle - and I imagine not reading any attributes would start to look like "not handling" to a user when things show up without names, address ranges, or any other functionality?<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg">I guess after we parse the DWARF and make the skeleton of the DIEs successfully that the input is good and that is kind of an invariant. We wouldn’t be extracting any data from DIEs using a DWARFDie if we weren’t able to parse the debug info already to get the layout of the DWARF. I would like to assume that the DWARF is good at this point and we are just accessing it.</div></div></blockquote><div><br></div><div>But that doesn't seem to be true - we don't parse most of the DWARF we just skip over the bytes to find the next DIE because we know the size. Seems possible some of those bytes could be invalid, perhaps? *looks through the decoding code... * perhaps not - I'm not sure if there are any cases of invalid bit patterns in the attributes</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg">If we add error handling, I don’t really plan on integrating it into the LLDB parser, but you seemed to want this so I added it. I personally don’t think it is useful.</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">What should LLDB do when it encounters broken input? What does it do when it encounters input it can't parse today?</div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg">Again, the DWARF would have failed to parse and no DIEs would be visited if the pass through the compile units died in the first place.</div></div></blockquote><div><br></div><div>Then it sounds like we shouldn't ever stop short because we should never encounter errors - instead we should assert that we don't encounter errors, instead of writing code to handle a case we believe to be an invariant violation.<br><br>Indeed, some of the APIs could be changed to have stronger preconditions rather than handling error cases (getForm/AttrByIndex should probably just assert that the index is valid - same as std::vector[] does, basically? and this caller could assert that DWARFFormValue::extractValue returns true - so long as we check up-front hat we aren't parsing any unknown forms... but I think we want to support unknown forms, so perhaps that operation should be fallible? & thus walking the attributes could fail at that operation (or we could not expose a value there? Seems like a erasonable definition of behavior and not a failure mode - and we can/should easily write a test for that))<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"> We just try to iterate through the attributes and if anything goes wrong we exit the attribute loop. I really don’t see the point of the errors, or if they have to exist then they should be optional.</div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Greg</div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Greg</div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Mon, Jan 9, 2017 at 2:21 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg updated this revision to Diff 83698.<br class="gmail_msg">
clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
Added optional error handling to the attributes iterators. You can now pass an "llvm::Error *" to the attributes():<br class="gmail_msg">
<br class="gmail_msg">
  attribute_iterator DWARFDie::attributes(llvm::Error *Err);<br class="gmail_msg">
<br class="gmail_msg">
If the error is non-NULL, then the error will be filled in. This iteration error is the same method used in Archive.h/Archive.cpp after I spoke with Lang Hames.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D28386" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D28386</a><br class="gmail_msg">
<br class="gmail_msg">
Files:<br class="gmail_msg">
  include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h<br class="gmail_msg">
  include/llvm/DebugInfo/DWARF/DWARFAttribute.h<br class="gmail_msg">
  include/llvm/DebugInfo/DWARF/DWARFDie.h<br class="gmail_msg">
  lib/DebugInfo/DWARF/DWARFDie.cpp<br class="gmail_msg">
  unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>
</div></blockquote></div><br class="gmail_msg"></div></blockquote></div></div>
</div></blockquote></div><br class="gmail_msg"></div></blockquote></div></div>