[llvm] r258188 - Fix a coverage reading bug

[Sean Silva via llvm-commits]

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.

-- 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
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160120/1d300b41/attachment.html>