<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 20, 2016 at 4:11 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Jan 20, 2016 at 4:01 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Jan 20, 2016 at 3:56 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> It was added but got reverted in r258314 due to an ARM build bot<br>
>> failure. Briefly looking at the bot failure looks like some BFD linker<br>
>> bug (old version perhaps). I need more investigation before re-install<br>
>> the tests.<br>
<br>
</span>It needs covmap with duplicate entries across multiple modules, so<br>
adding a test in compile-rt is more natural. I can of course add a<br>
test case with binary data.<br>
<span class="">><br>
><br>
> Why does adding a test for this require putting something in compiler-rt?<br>
><br>
> Also, for the moment, can you revert this patch until you have time to<br>
> investigate the test failure? Experience shows that failing to check in<br>
> tests with the code that fixes the bug results in tests ultimately never<br>
> getting added.<br>
<br>
</span>It will be investigated. In the mean time, I will add a case in llvm.<br></blockquote><div><br></div><div>Thanks! In general, we try to add the test case in the repository which contains the code with the bug, but getting extra test coverage in compiler-rt showing the real use case that hit it is of course greatly appreciated.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> -- Sean Silva<br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>> On Wed, Jan 20, 2016 at 3:49 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Tue, Jan 19, 2016 at 1:18 PM, Xinliang David Li via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Author: davidxl<br>
>> >> Date: Tue Jan 19 15:18:12 2016<br>
>> >> New Revision: 258188<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=258188&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=258188&view=rev</a><br>
>> >> Log:<br>
>> >> Fix a coverage reading bug<br>
>> >><br>
>> >> function record pointer is not advanced when<br>
>> >> duplicate entry is found.<br>
>> >><br>
>> >> Test case to be added.<br>
>> ><br>
>> ><br>
>> > Did this ever get added? In LLVM the general rule is that if your commit<br>
>> > fixes a bug then the test case should be included in the same commit (or<br>
>> > there should be an explanation for why it was impractical to add).<br>
>> ><br>
>> > -- Sean Silva<br>
>> ><br>
>> >><br>
>> >><br>
>> >> Modified:<br>
>> >> llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp<br>
>> >><br>
>> >> Modified: llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp?rev=258188&r1=258187&r2=258188&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp?rev=258188&r1=258187&r2=258188&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp (original)<br>
>> >> +++ llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp Tue Jan 19<br>
>> >> 15:18:12 2016<br>
>> >> @@ -396,8 +396,10 @@ public:<br>
>> >> // function name. This is useful to ignore the redundant records<br>
>> >> for the<br>
>> >> // functions with ODR linkage.<br>
>> >> NameRefType NameRef = CFR->template getFuncNameRef<Endian>();<br>
>> >> - if (!UniqueFunctionMappingData.insert(NameRef).second)<br>
>> >> + if (!UniqueFunctionMappingData.insert(NameRef).second) {<br>
>> >> + CFR++;<br>
>> >> continue;<br>
>> >> + }<br>
>> >><br>
>> >> StringRef FuncName;<br>
>> >> if (std::error_code EC =<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@lists.llvm.org">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>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>