<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 Mar 9, 2017, at 10: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="font-family: Helvetica; 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;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Thu, Mar 9, 2017 at 10:49 AM Paul Robinson 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">probinson added inline comments.<br class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg">================<br class="gmail_msg">Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:82-89<br class="gmail_msg">+ if (!dwarf::isValidFormForVersion(AttrData.getForm(),<br class="gmail_msg">+ AP->getDwarfVersion())) {<br class="gmail_msg">+ std::string msg;<br class="gmail_msg">+ raw_string_ostream Msg(msg);<br class="gmail_msg">+ Msg << "Invalid form " << format("0x%x", AttrData.getForm())<br class="gmail_msg">+ << " for DWARF version " << AP->getDwarfVersion();<br class="gmail_msg">+ report_fatal_error(Msg.str());<br class="gmail_msg">----------------<br class="gmail_msg">dblaikie wrote:<br class="gmail_msg">> Should this be an assertion instead?<br class="gmail_msg">I dithered on that. An assertion would tell you "something bad happened somewhere" with no details, because there's no way I know of to report a runtime value in an assertion. So you don't know what FORM is causing the problem. Doing it this way was more valuable when figuring out the test failures that I had to fix.<br class="gmail_msg">But, it's true if we detect a problem here, that indicates an internal inconsistency rather than a problem with our external input, so on that basis an assertion would be more the thing.<br class="gmail_msg"></blockquote><div class=""><br class="">I believe there are various forms of debug logging that are used for adding more complex messages near/with an assert. Perhaps you can use that? (I'd search for 'DEBUG' and 'dbg' in the codebase)<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;"><br class="gmail_msg"><br class="gmail_msg">================<br class="gmail_msg">Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:918-919<br class="gmail_msg"> addFlag(Buffer, dwarf::DW_AT_declaration);<br class="gmail_msg">- return addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);<br class="gmail_msg">+ if (getDwarfVersion() >= 4)<br class="gmail_msg">+ addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);<br class="gmail_msg">+ return;<br class="gmail_msg">----------------<br class="gmail_msg">dblaikie wrote:<br class="gmail_msg">> This is probably incorrect (short of the broader discussion I started about how references to type units should be implemented - I should ping that thread).<br class="gmail_msg">><br class="gmail_msg">> If the user requests type units, either it should fail if asking for a DWARF version that doesn't support type units - or ignore the conflict and produce the signature anyway?<br class="gmail_msg">The signature attribute was coming out even without type units. This was added by Adrian for "external" types, something to do with modules.<br class="gmail_msg">I would be okay with emitting the v4 attribute in earlier versions, but not the v4 FORM. We'd have to pick something other than FORM_ref_sig8 for that case.<br class="gmail_msg"></blockquote><div class=""><br class="">Adrian - want to weigh in here?<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>[Sorry, I was out for a few days.]</div><div>We don't use ref_sig8 (or any signatures) for "externally defined" types any more, so what David says sound fine to me.</div><div><br class=""></div><div>-- adrian</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; 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;" class=""><div class="gmail_quote"><div class=""><br class="">Much like type units, though, I think the same should hold - rather than silently omitting it, what's probably required is to either emit it anyway because the user asked for the feature and must have a compatible consumer to cope with it, or reject the combination of features that appear to be contradictory.<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;"><br class="gmail_msg"><br class="gmail_msg"><a href="https://reviews.llvm.org/D30785" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30785</a></blockquote></div></div></div></blockquote></div><br class=""></body></html>