[llvm] r258188 - Fix a coverage reading bug

[Xinliang David Li via llvm-commits]

On Wed, Jan 20, 2016 at 4:18 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Wed, Jan 20, 2016 at 4:11 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> On Wed, Jan 20, 2016 at 4:01 PM, Sean Silva <chisophugis at gmail.com> wrote:
>> >
>> >
>> > On Wed, Jan 20, 2016 at 3:56 PM, Xinliang David Li <davidxl at google.com>
>> > wrote:
>> >>
>> >> It was added but got reverted in r258314 due to an ARM build bot
>> >> failure. Briefly looking at the bot failure looks like some BFD linker
>> >> bug (old version perhaps). I need more investigation before re-install
>> >> the tests.
>>
>> It needs covmap with duplicate entries across multiple modules, so
>> adding a test in compile-rt is more natural. I can of course add a
>> test case with binary data.
>> >
>> >
>> > Why does adding a test for this require putting something in
>> > compiler-rt?
>> >
>> > 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.
>>
>> It will be investigated. In the mean time, I will add a case in llvm.
>
>
> 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.
>

yes -- that is my plan -- the compile-rt test will also be added back
once the root cause is nailed.

David

> -- Sean Silva
>
>>
>>
>> David
>>
>> >
>> > -- Sean Silva
>> >
>> >>
>> >>
>> >> David
>> >>
>> >> On Wed, Jan 20, 2016 at 3:49 PM, Sean Silva <chisophugis at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Tue, Jan 19, 2016 at 1:18 PM, Xinliang David Li via llvm-commits
>> >> > <llvm-commits at lists.llvm.org> wrote:
>> >> >>
>> >> >> Author: davidxl
>> >> >> Date: Tue Jan 19 15:18:12 2016
>> >> >> New Revision: 258188
>> >> >>
>> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=258188&view=rev
>> >> >> Log:
>> >> >> Fix a coverage reading bug
>> >> >>
>> >> >> function record pointer is not advanced when
>> >> >> duplicate entry is found.
>> >> >>
>> >> >> Test case to be added.
>> >> >
>> >> >
>> >> > Did this ever get added? In LLVM the general rule is that if your
>> >> > commit
>> >> > fixes a bug then the test case should be included in the same commit
>> >> > (or
>> >> > there should be an explanation for why it was impractical to add).
>> >> >
>> >> > -- Sean Silva
>> >> >
>> >> >>
>> >> >>
>> >> >> Modified:
>> >> >>     llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp
>> >> >>
>> >> >> Modified: llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp
>> >> >> URL:
>> >> >>
>> >> >>
>> >> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp?rev=258188&r1=258187&r2=258188&view=diff
>> >> >>
>> >> >>
>> >> >>
>> >> >> ==============================================================================
>> >> >> --- llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp (original)
>> >> >> +++ llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp Tue Jan 19
>> >> >> 15:18:12 2016
>> >> >> @@ -396,8 +396,10 @@ public:
>> >> >>        // function name. This is useful to ignore the redundant
>> >> >> records
>> >> >> for the
>> >> >>        // functions with ODR linkage.
>> >> >>        NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
>> >> >> -      if (!UniqueFunctionMappingData.insert(NameRef).second)
>> >> >> +      if (!UniqueFunctionMappingData.insert(NameRef).second) {
>> >> >> +        CFR++;
>> >> >>          continue;
>> >> >> +      }
>> >> >>
>> >> >>        StringRef FuncName;
>> >> >>        if (std::error_code EC =
>> >> >>
>> >> >>
>> >> >> _______________________________________________
>> >> >> llvm-commits mailing list
>> >> >> llvm-commits at lists.llvm.org
>> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >> >
>> >> >
>> >
>> >
>
>