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.
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?
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.
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.
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,