Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static analysis cleanup and open issues #24

Closed
wants to merge 6 commits into from

Conversation

daniel-j-h
Copy link
Contributor

I used LLVM's (3.6) static analyzer and clang's (3.6) Weverything flag in order to check the source.

There are minor issues I could fix myself, e.g. undefined behavior when doing 1 << 31.
(Interestingly this is not UB anymore in C++14, see: http://stackoverflow.com/q/26331035)

This increment looks suspicious, as it is dead code:

z3/src/ast/used_vars.cpp

Lines 106 to 112 in d01c349

unsigned r = 0;
unsigned num = m_found_vars.size();
for (unsigned i = 0; i < num; i++) {
if (m_found_vars[i])
return r++;
}
return r;

Clarify operator precedence: && trumps ||: Is this correct?

m_bv_util.get_bv_size(args[0]) == 3 &&
m_arith_util.is_int(args[1]) ||

Undefined behavior in reinterpret_cast's, one example is this, but there are more:

uint64 r = (RAW(s.value) & 0x800FFFFFFFFFFFFFull) | ((exp + 1023) << 52);

"dereference of type 'const uint64 *' (aka 'const unsigned long long *') that was reinterpret_cast from type 'double *' has undefined behavior"

-Wundefined-reinterpret-cast finds those.

This loop is executed only a single time. Always. Is this correct?

Note: based on the comment by wintersteiger in #11 this pull request is against the unstable branch.

Would you please be so kind and take a look at those issues -- maybe if you have the time do a static analysis and Weverything build yourself and check the left over warnings.

Thanks,
Daniel

NikolajBjorner pushed a commit that referenced this pull request Apr 3, 2015
Signed-off-by: Nikolaj Bjorner <nbjorner@hotmail.com>
@wintersteiger wintersteiger force-pushed the unstable branch 4 times, most recently from 52e29de to 30eb461 Compare June 24, 2015 16:07
@wintersteiger
Copy link
Contributor

Thanks for the patches, those all look good to me. I've moved some of the other issues into separate issues to be looked at later.

@wintersteiger
Copy link
Contributor

@daniel-j-h if it's not too much trouble, could I get you to sign a CLA before we merge this?

@daniel-j-h
Copy link
Contributor Author

@wintersteiger signed.

NikolajBjorner added a commit that referenced this pull request Oct 4, 2015
Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>
wintersteiger added a commit to wintersteiger/z3 that referenced this pull request Oct 5, 2015
wintersteiger pushed a commit to wintersteiger/z3 that referenced this pull request Oct 5, 2015
Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>
@wintersteiger
Copy link
Contributor

Sorry for the premature closure of this. I'm now merging it into master.

@wintersteiger wintersteiger reopened this Oct 19, 2015
@msftclas
Copy link

Hi @daniel-j-h, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@wintersteiger
Copy link
Contributor

Merged into master as of 6749c19

NikolajBjorner added a commit to NikolajBjorner/z3 that referenced this pull request Aug 7, 2016
* fix bound bug

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>

* fix bounds check

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>

* for solver call in final check

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>

* update logging code

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>

* fix compiler error, tabs

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>

* using variable bounds an term facility

Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>
@levnach levnach mentioned this pull request Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants