[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)
----------------
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.


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

https://reviews.llvm.org/D157267