<div dir="ltr">Hi Dave,<div><br></div><div>Sorry - I referenced <span style="font-size:13px">r</span>265975, but didn't say explicitly: Since that commit fixed the issue (by moving this directive into Darwin-specific parser code) it also changed these report_fatal_errors back in to llvm_unreachables.</div><div><br></div><div>- Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 12, 2016 at 10:09 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Apr 11, 2016 at 1:13 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Dave,<div><br></div><div>100% code coverage sounds like a good aim, but I'm not sure it applies here: As of this patch there's no code in the ELF or COFF parser for this directive. It just goes in the "unsupported directive" bucket. As long as we're testing one unsupported directive (though to be fair I'm not sure we are) we've covered all of the relevant code.</div></div></blockquote><div><br></div></span><div>What I mean is that the previous unreachable was untested, presumably (because you can't really test an unreachable - it's UB to reach it). So replacing it with a diagnostic (report_fatal_error) means we now have untested code. It'd be nice to test it.</div><div><br></div><div>I'm not sure how this code could be covered by an existing test case - but I haven't looked at the code more broadly, so I may be missing something.<span class="HOEnZb"><font color="#888888"><br><br>- David</font></span></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>- Lang.</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 11, 2016 at 12:48 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Apr 11, 2016 at 12:44 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Rafael,<div><br></div><div>Yep - this was just a temporary hack. It's been fixed as of r<span style="font-size:13px">265975.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">Regarding </span>test cases - what's the bar for something like this. I'd have included a test that the directive was handled correctly on the platforms it's intended for, but testing every (unsupported) directive on every platform seems like overkill.</div></div></blockquote><div><br></div></span><div>FWIW, my bar would be "if it's separate code, it probably deserves a separate test". (100% block coverage is a pretty low bar for testing (& I expect a bit more than that - it's easy to have coverage without verification of the right behavior, for example), and, admittedly, not one (m)any projects really have, especially with C++ & many implicit blocks (maybe that's not so bad with exceptions disabled), but generally something to strive for)<br><br>- Dave</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>- Lang.</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 10, 2016 at 8:35 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks, but:<br>
<br>
* This should have a testcase.<br>
* It is odd to parse and then issue an error. I think the correct fix<br>
is to move the parsing to lib/MC/MCParser/DarwinAsmParser.cpp, that<br>
way we get the natural error on ELF and COFF.<br>
<br>
Cheers,<br>
Rafael<br>
<br>
<br>
<br>
On 8 April 2016 at 13:38, Lang Hames via llvm-commits<br>
<div><div><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: lhames<br>
> Date: Fri Apr 8 12:38:51 2016<br>
> New Revision: 265815<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=265815&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=265815&view=rev</a><br>
> Log:<br>
> [Object] Report an error if .alt_entry is used with ELF or COFF.<br>
><br>
> I'm looking into a better way to do this long-term, but for now at least don't<br>
> crash.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/MC/MCELFStreamer.cpp<br>
> llvm/trunk/lib/MC/WinCOFFStreamer.cpp<br>
><br>
> Modified: llvm/trunk/lib/MC/MCELFStreamer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/MC/MCELFStreamer.cpp (original)<br>
> +++ llvm/trunk/lib/MC/MCELFStreamer.cpp Fri Apr 8 12:38:51 2016<br>
> @@ -285,7 +285,7 @@ bool MCELFStreamer::EmitSymbolAttribute(<br>
> break;<br>
><br>
> case MCSA_AltEntry:<br>
> - llvm_unreachable("ELF doesn't support this attribute");<br>
> + report_fatal_error("ELF doesn't support the .alt_entry attribute");<br>
> }<br>
><br>
> return true;<br>
><br>
> Modified: llvm/trunk/lib/MC/WinCOFFStreamer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/MC/WinCOFFStreamer.cpp (original)<br>
> +++ llvm/trunk/lib/MC/WinCOFFStreamer.cpp Fri Apr 8 12:38:51 2016<br>
> @@ -107,6 +107,8 @@ bool MCWinCOFFStreamer::EmitSymbolAttrib<br>
> case MCSA_Global:<br>
> Symbol->setExternal(true);<br>
> break;<br>
> + case MCSA_AltEntry:<br>
> + report_fatal_error("COFF doesn't support the .alt_entry attribute");<br>
> }<br>
><br>
> return true;<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>