<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 14, 2014 at 6:36 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@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><br><div class="gmail_quote"><div><div class="h5">On Mon, Apr 14, 2014 at 5:56 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br>
On Apr 11, 2014, at 11:31 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>> wrote:<br>
<br>
> Hi Duncan,<br>
><br>
> Thanks for working on this. The patches look great overall.<br>
> 0001: LGTM<br>
> 0002: There is some inconsistency in creating debug info verifier pass.<br>
> 1><br>
> tools/opt/opt.cpp: if (VerifyEach) PM.add(createVerifierPass());<br>
> Should VerifyEach guard debug info verifier as well given that we are using NoVerify to guard debug info verifier?<br>
> I am okay with not using VerifyEach for debug info verifier, but please add comments there to explain why.<br>
<br>
</div>I just changed it to line up with Verifier for now. Probably it will<br>
be too slow, but if it is, we'll probably want to add a separate flag<br>
so we can --verify-di-each on demand.<br>
<div><br>
> 2> What about these two places? Should we add debug info verifier?<br>
> tools/clang/lib/CodeGen/BackendUtil.cpp: FPM->add(createVerifierPass());<br>
> unittests/Bitcode/BitReaderTest.cpp: passes.add(createVerifierPass());<br>
<br>
</div>I just missed those. Thanks! Note that one of those is in cfe, so<br>
I've added a third patch for that one.<br>
<div><br>
> 3> Could you turn debug info verifier on by default and make sure it is working correctly with "make check-all"?<br>
<br>
</div>I'm seeing 225 failures with make check-all when I turn --verify-di<br>
and --verify-each on by default (configured with X86;ARM;ARM64):<br>
<br>
Expected Passes : 14993<br>
Expected Failures : 58<br>
Unsupported Tests : 2341<br>
Unexpected Failures: 225<br>
<br>
make check gives:<br>
<br>
Expected Passes : 7893<br>
Expected Failures : 39<br>
Unsupported Tests : 2288<br>
Unexpected Failures: 225<br>
<br>
so they're all in LLVM proper.<br>
<br>
Is this expected? Given that I know next to nothing about debug<br>
info, I'm probably not the right person to diagnose them.<br></blockquote><div><br></div></div></div><div>These tests may fail as well with the current debug info verifier when we turn it on by default (without your patch).</div>
<div class="">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Should they be fixed before commit? I'm thinking it would be<br>
better to tackle them incrementally.<br></blockquote><div> </div></div><div>I am okay with committing your patches as long as we see the same set of failures without your patch.</div><div>We should file a bug to track these failing tests though.</div>
</div></div></div></blockquote><div><br></div><div>Since Eric is not worried about these failing cases, these patches LGTM.<br><br>Manman<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>Thanks,</div><div>Manman</div><div class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(For clarity, in case someone else on the list isn't following<br>
closely, check-all is clean unless I *force* these options on.)<br>
<br>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>