[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

balazske added a comment. I did not like this solution so I like it if we can omit this change and apply another fix (D69726 ). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

vabridgers added a comment. I pulled https://reviews.llvm.org/D69726, brought it up to date and pushed to github at https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. Everything seems ok on this branch (LITs pass, reproducers from

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

vabridgers added a comment. @baloghadamsoftware , these changes do seem to help the case described. This patch isn't quite up to date, and needs to be integrated with changes from @balazske (my integration is hacky and needs to be cleaned up). I'll continue working on this, and get back to

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

chrish_ericsson_atx added a comment. Is anything planned or in progress that would address this issue? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/ https://reviews.llvm.org/D86743 ___

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

NoQ added a comment. But that's for a different bug, no? Even if that patch somehow dodges *this* crash, the underlying problem of us being unable to account for mutability of VLA sizes still remains to be addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

balazske added a comment. I do not know if it got overlooked but there is another patch D81061 that removes the crash. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/ https://reviews.llvm.org/D86743

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

baloghadamsoftware requested changes to this revision. baloghadamsoftware added a comment. This revision now requires changes to proceed. In D86743#2303922 , @NoQ wrote: > One slightly redeeming thing about this crash is that it's assertion-only. > When

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

NoQ added a comment. > Yup, that's pretty bad. One slightly redeeming thing about this crash is that it's assertion-only. When built without assertions clang doesn't crash and this patch doesn't really change its behavior (adding transition to a null state is equivalent to adding no

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

baloghadamsoftware added a comment. @vabridgers Please try to apply D69726 and check whether it solves this crash! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/ https://reviews.llvm.org/D86743

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

baloghadamsoftware added a comment. In D86743#2303910 , @NoQ wrote: >> The last comment for that bug is D69726 , >> but the bug is not closed to it seems to me that D69726 >> does not

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

NoQ added a comment. > The last comment for that bug is D69726 , > but the bug is not closed to it seems to me that D69726 > does not solve it, just takes a single step > towards the solution. It might as well be that the

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

baloghadamsoftware added a comment. Yes, @NoQ is right. If the constraint manager cannot reason about a value, then then `ProgramState::assume()` will return the same state with the new assumption in the constraints. Whenever `ProgramState::assume()` returns `nullptr` it means that the

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

NoQ added a comment. A VLA in a loop may have different size on each iteration of the loop. This looks very much related to https://bugs.llvm.org/show_bug.cgi?id=28450. > I do not know how these changes can appear You know the node. Conditional breakpoint on the node and step-by-step

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

baloghadamsoftware added a comment. I agree here with @Szelethus. We should investigate first why the assumption fails. Then we can decide about the best possible fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

Szelethus added a comment. Herald added a subscriber: danielkiss. I'm not thrilled by this solution. As I understand it, the assertion was put there to enforce our ability to create a new assumption, and its great that we had it, because we managed to catch a fault in that. Seems like this

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

balazske added a comment. I do not know how these changes can appear: F12796482: Screenshot from 2020-08-28 16-06-02.png The checker makes only 2 assumptions, about the array dimension being positive and about the size of the array and the extent. (This

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

vabridgers added a comment. Thanks Balazs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86743/new/ https://reviews.llvm.org/D86743 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

balazske added a comment. The fix is probably OK but I could not find out what causes the problem in this case and not in other (similar) ones. Why is not possible to assume `SVB.evalEQ(State, DynSize, *ArraySizeNL)` to true: DynSize: `extent_$1{e}` *ArraySizeNL: `8 U64b` The problem occurs

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

vabridgers created this revision. vabridgers added reviewers: balazske, NoQ, martong, baloghadamsoftware, Szelethus, gamesh411. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun. Herald added a project: clang.