Page MenuHomePhabricator

[analyzer] LoopUnrolling: fix crash when a parameter is a loop counter
ClosedPublic

Authored by AbbasSabra on May 18 2020, 4:20 PM.

Details

Summary

When loop counter is a function parameter "isPossiblyEscaped" will not find the variable declaration "VD" which lead to "llvm_unreachable". Parameters should be escaped like global variables to avoid a crash.

Diff Detail

Event Timeline

AbbasSabra created this revision.May 18 2020, 4:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2020, 4:20 PM

Thanks for finding this bug! Adding some reviewers.

I think it would be perfectly fine to unroll loops where the loop counter is a pass-by-value parameter. That should not be considered escaped.

I think in case of parameters isPossiblyEscaped should return false for values and true for references.

What do you think?

xazax.hun added inline comments.May 19 2020, 5:47 AM
clang/test/Analysis/loop-unrolling.cpp
503

nit: we usually use arg for actual arguments at the call site, and use param for formal parameters. The test function should be renamed accordingly. Also, it would be great to add clang_analyzer_numTimesReached() to demonstrate whether the loop is actually unrolled or not.

Thanks for taking your time to find and resolve the bug.
I agree that this solution seems a bit too harsh. Technically there is no difference between local variable declaration and a parameter declaration (if we are talking about value types as pointed out by @xazax.hun).
I believe that we should actually fix the will not find the variable declaration "VD" which lead to "llvm_unreachable" part.

Fix code review

AbbasSabra accepted this revision.May 19 2020, 5:26 PM
AbbasSabra marked an inline comment as done.
This revision is now accepted and ready to land.May 19 2020, 5:26 PM
AbbasSabra requested review of this revision.May 19 2020, 5:30 PM

What you both said makes sense. Now, reference parameters are escaped and value parameters are treated as the local variables.
Thanks!

vsavchenko added inline comments.May 20 2020, 12:42 AM
clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
167

getKind function is only an implementation detail for isa/cast/dan_cast functions (docs). So, this condition would be better in the following form: isa<ParmVarDecl>(VD).

NOTE: One good motivation here is that maybe in the future there will be some sort of new type of function parameters and it will be derived from ParmVarDecl. In this situation, direct comparison with the kind of node will not work and probably won't be fixed by developers who introduced the new node, but isa approach will stay correct.

Fix code review 2

AbbasSabra marked 2 inline comments as done.May 20 2020, 8:06 AM
AbbasSabra added inline comments.
clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
167

Thanks for the detailed explanation.

AbbasSabra marked an inline comment as done.May 20 2020, 8:09 AM
NoQ accepted this revision.May 20 2020, 10:39 AM

Looks great, thanks!

This is probably not the precise solution (i.e., we might be able to see where exactly is the reference taken and proceed from there) but neither is the original code - it's always been super pattern-matchy,

This revision is now accepted and ready to land.May 20 2020, 10:39 AM
vsavchenko accepted this revision.May 20 2020, 10:41 AM

LGTM! Thanks again!

Great! Can someone take care of merging it? I believe I don't have access.

This revision was automatically updated to reflect the committed changes.