This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference
ClosedPublic

Authored by AbbasSabra on May 11 2021, 12:46 PM.

Diff Detail

Event Timeline

AbbasSabra created this revision.May 11 2021, 12:46 PM
AbbasSabra requested review of this revision.May 11 2021, 12:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 12:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm gonna have a look at this tomorrow.

Great job on the patch! Thanks!

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
165–200

One thought here.
I think it's better to not have implicit connections between parameters that are not obvious to the user of the function (or to the person who is going to modify this function in the future). Here we always have VD = DR->getDecl()->getCanonicalDecl(). So, IMO, the interface of this function will be much cleaner if we accept only a DeclRefExpr.

184–185

But actually, it's a bit different with this one. I don't know exact details and rules how clang sema fills in the class for lambda.
According to cppreference:

For the entities that are captured by reference (with the default capture [&] or when using the character &, e.g. [&a, &b, &c]), it is unspecified if additional data members are declared in the closure type

It can be pretty much specified in clang, that's true, but it looks like in DeadStoreChecker we have a very similar situation and we do not assume that captured variable always have a corresponding field.

187–190

There is a general rule in LLVM's style guide of trying to minimize nestedness of if's, forms and so on.
In this particular situation this assertion can be more local: assert(MD && MD->getParent()->isLambda() & ...);.
This way it is more obvious what it checks and we avoid confusion while reading this function because if (condition) automatically means that !condition is something that might happen.
The same applies to another assertion here.

203–204

I think that this comment has to be updated now.

203–204

I think this whole section is less obvious now and requires one good solid comment explaining what are the different situations here, what is "entry" and so on.

Godin added a subscriber: Godin.May 12 2021, 3:14 AM

By checking the line coverage of the LoopUnrolling.cpp test file, looks like all lines are covered you touched.

There are only two return statements uncovered though: L200, L251.
We should consider extending this test file to cover them as well in a follow-up patch.

AbbasSabra marked 4 inline comments as done.

Updating D102273: [analyzer] Apply code review

AbbasSabra added inline comments.May 14 2021, 7:31 AM
clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
184–185

Yes, I based this on the fact that DeadStoreChecker considers it possible, but I have never faced a case where it does not have a corresponding field.

Thanks for addressing my comments! I still have some left though

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
170

dyn_cast_or_null also tells the reader that D can be null and can be not CXXMethodDecl. So it's better to use cast here if you are sure about D and have assertions about it.

174–177

Body of this function seems to be too dense. It is usually hard to read something that doesn't have rest places for eyes. You can think of it as paragraphs in the written text.
In this particular situation, you can split all these statements and declarations in groups of 3-4. It would be perfect if they are grouped semantically (like good paragraphs).

178

We can just do cast here

184–185

I still think that we should do something about this else

191–192

I think it's a great idea to list all the cases. I think we can go even further: enumerate them and actually pinpoint in the implementation where we cover which case. With numbers it can be much easier.

193

Can we not introduce new terms and simply call it a reference?

196

This should be also modified then

vsavchenko added inline comments.May 14 2021, 7:48 AM
clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
184–185

It still would be good to have some proof that it is indeed like this or simply fallback into returning true (which you already do when in doubt).

AbbasSabra marked 7 inline comments as done.

Updating D102273: [analyzer] Apply code review part 2

AbbasSabra added inline comments.May 14 2021, 10:07 AM
clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
184–185

As I said, I believe it cannot happen but I assumed based on the other checker that there is something I don't know.
I think based on getCaptureFields the implementation we are iterating over all captures. For that algorithm to work number of captures should be equal to number of fields

void CXXRecordDecl::getCaptureFields(
       llvm::DenseMap<const VarDecl *, FieldDecl *> &Captures,
       FieldDecl *&ThisCapture) const {
  Captures.clear();
  ThisCapture = nullptr;

  LambdaDefinitionData &Lambda = getLambdaData();
  RecordDecl::field_iterator Field = field_begin();
  for (const LambdaCapture *C = Lambda.Captures, *CEnd = C + Lambda.NumCaptures;
       C != CEnd; ++C, ++Field) {
    if (C->capturesThis())
      ThisCapture = *Field;
    else if (C->capturesVariable())
      Captures[C->getCapturedVar()] = *Field;
  }
  assert(Field == field_end());
}

I dropped the defensive code

Updating D102273: [analyzer] Update comments + fix typos

NoQ added a comment.May 18 2021, 11:11 AM

I've just been patching up clang-tidy's infinite loop checker and the problem sounds soooo similar. Maybe we should move clang-tidy's alias analysis into libAnalysis and re-use it?

I've just been patching up clang-tidy's infinite loop checker and the problem sounds soooo similar. Maybe we should move clang-tidy's alias analysis into libAnalysis and re-use it?

I'm not familiar with clang-tidy's alias analysis. Do you think it should be part of this patch? or a follow-up one?

Note: I don't have the right to re-run the failed build/test. I assume that it is not related to my change since the test compile and runs omp code(Static analyzer doesn't run on that test)

@vsavchenko any update on this?

vsavchenko accepted this revision.Jul 9 2021, 2:25 AM

Great! Thanks for addressing all of the comments!

clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
184–185

OK, sounds reasonable!

This revision is now accepted and ready to land.Jul 9 2021, 2:25 AM

Great! Thanks for addressing all of the comments!

Thank you for the review! Can you take care of merging it? I don't have the required permission.

This revision was landed with ongoing or failed builds.Jul 12 2021, 7:06 AM
This revision was automatically updated to reflect the committed changes.