[PATCH] D157267: [NewGVN] Fix an use after free when updating use count

[Konstantina Mitropoulou via Phabricator via llvm-commits]

kmitropoulou added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:4119
           if (isSSACopy) {
             unsigned &IIUseCount = UseCounts[II];
             if (--IIUseCount == 0)
----------------
kmitropoulou wrote:
> vladimirradosavljevic wrote:
> > kmitropoulou wrote:
> > > The test case does not crash in my workspace. Anyway, I think the problem is here. If II is not in the UseCounts map, then it just adds it and it returns zero. Will the following change solve the problem?
> > > 
> > > if (isSSACopy) {
> > >     auto It = UseCounts.find(II);
> > >     if (It != UseCounts.end()) {
> > >         unsigned &IIUseCount = It->second;
> > >         if (--IIUseCount == 0)
> > >              ProbablyDead.insert(II);
> > >     }
> > > }
> > > 
> > Could you please try to run it with valgrind/sanitizers, because it happens occasionally and this is why it is such a dangerous bug.
> > Yes, proposed solution fixes the problem. Please note that I'm not familiar with the NewGVN pass, but if II is not in UseCounts map, should we also insert it into ProbablyDead?
> I am not expert on this part of NewGVN, but I think that if II is in UseCounts map, then II should be already in ProbablyDead. This logic is towards the bottom of convertClassToDFSOrdered() . 
> 
> Regarding the test, if it crashes with the sanitizer, then it is OK. But, the test is way too big. Please make it smaller.
> 
> Since NewGVN is not enabled by default, we should run the tests from test-suite with NewGVN enabled. Please be aware that there are some tests that are failing due to NewGVN.
I am sorry I meant:

 I think that if II is not in UseCounts map, then II should be already in ProbablyDead. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157267/new/

https://reviews.llvm.org/D157267