[llvm] r258188 - Fix a coverage reading bug

[Xinliang David Li via llvm-commits]

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.

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
>> >
>> >
>
>