<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 20, 2016 at 3:56 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">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></blockquote><div><br></div><div>Why does adding a test for this require putting something in compiler-rt?</div><div><br></div><div>Also, for the moment, can you revert this patch until you have time to investigate the test failure? Experience shows that failing to check in tests with the code that fixes the bug results in tests ultimately never getting added.</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>
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>
>> <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>
>> --- 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>
</div></div></blockquote></div><br></div></div>