Page MenuHomePhabricator

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

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

Diff Detail

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

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

1154

that's a separate change, but OK

1426

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

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

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

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

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.