This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] fix bug with 1-bit APSInt types in Z3ConstraintManager
ClosedPublic

Authored by ddcc on May 31 2018, 12:55 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ddcc created this revision.May 31 2018, 12:55 PM
george.karpenkov requested changes to this revision.May 31 2018, 1:23 PM

Thanks! Looks good with minor changes.

Would it be possible to add tests? I know we have very few unit tests, but I assume you could actually use an integration test to exercise this path?

lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
1044 ↗(On Diff #149344)

What does L stand for here? It's confusing because L/R usually stand for left/right-hand-side in this context.

1154 ↗(On Diff #149344)

that's a separate change, but OK

1426 ↗(On Diff #149344)

It's redundant to mutate the argument passed by reference and also return it.
Could we take a single APSInt parameter by value and return std::pair<APSInt, QualType> ?

1640 ↗(On Diff #149344)

that's a separate change, but OK

This revision now requires changes to proceed.May 31 2018, 1:23 PM

Would it be possible to add tests? I know we have very few unit tests, but I assume you could actually use an integration test to exercise this path?

I tested this change and it fixes PR37622. There's a simple crash reproducer included there.

ddcc added a comment.May 31 2018, 1:47 PM

Would it be possible to add tests? I know we have very few unit tests, but I assume you could actually use an integration test to exercise this path?

I tested this change and it fixes PR37622. There's a simple crash reproducer included there.

Cool, thanks for the repro! It's been long enough since I've touched this code that I don't recall the original failing testcase. I'll add the test to this revision.

lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
1044 ↗(On Diff #149344)

They correspond to the inferred left/right hand-side inferred types, but inside the subsequent Z3Expr LHS and RHS variables. This is confusing, so I'll rename them to FromTy and ToTy.

1154 ↗(On Diff #149344)

Yeah, this is one of several small miscellaneous changes that didn't make the original commit. It seemed a bit excessive to open separate revisions for each, so I've just been merging them into the next patch. I'm not sure which is preferable?

1426 ↗(On Diff #149344)

Sure.

ddcc updated this revision to Diff 149356.May 31 2018, 2:13 PM

Add test, address comments

ddcc edited the summary of this revision. (Show Details)May 31 2018, 2:15 PM
This revision is now accepted and ready to land.May 31 2018, 3:23 PM
This revision was automatically updated to reflect the committed changes.