Skip to content

fix flattening of ID_invalid_pointer #2630

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

Merged
merged 2 commits into from
Jul 30, 2018
Merged

Conversation

kroening
Copy link
Member

This fixes an inconsistency between the SMT2 encoding and the simplifier on one hand, and the bit-level flattening on the other: NULL is not considered invalid.

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: e40e87c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/80301713

@kroening kroening force-pushed the invalid-pointer-flattening branch from e40e87c to f98842a Compare July 30, 2018 14:46
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth cleaning up language, otherwise does what it says on the tin (though I'm not familiar enough to quickly know if there are other corresponding changes that would be needed)

@@ -991,7 +991,8 @@ void goto_checkt::pointer_validity_check(
allocs=disjunction(disjuncts);
}

if(flags.is_unknown() || flags.is_null())
if(flags.is_unknown() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point of curiosity: all of these is_ tests are may_be_ tests, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed.

@kroening kroening force-pushed the invalid-pointer-flattening branch 2 times, most recently from dce6c8b to ea1a244 Compare July 30, 2018 16:46
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the combination of changes I'm finding it hard to see whether the change that the commit message talks about is actually being tested in any way. Please break apart the changes into separate commits.

@@ -7,6 +7,6 @@ pointer outside dynamic object bounds in \*p: FAILURE
pointer outside dynamic object bounds in \*p: FAILURE
pointer outside dynamic object bounds in p2\[.*1\]: FAILURE
pointer outside dynamic object bounds in p2\[.*0\]: FAILURE
\*\* 4 of 36 failed
\*\* 4 of .* failed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal (and obviously similar changes happened elsewhere): could this please be [0-9]+ instead of .*?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

\[main.assertion.1\] assertion \*p==42: SUCCESS
\[main.pointer_dereference.[0-9]+\] dereference failure: pointer invalid in p\[.*1\]: FAILURE
\[main.assertion.2\] assertion \*\(p\+1\)==42: SUCCESS
^\[main\.pointer_dereference\.2\] dereference failure: invalid integer address in \*p: SUCCESS$
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? I'm finding it hard to reconcile this with the commit message.

expr.find_source_location(),
expr,
guard);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this change and what I mourned above just need to be moved into a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@tautschnig tautschnig assigned kroening and unassigned tautschnig Jul 30, 2018
@kroening kroening force-pushed the invalid-pointer-flattening branch from ea1a244 to 060b59c Compare July 30, 2018 17:13
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR failed Diffblue compatibility checks (cbmc commit: ea1a244).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/80397583
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: 060b59c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/80401170

@kroening kroening merged commit 44ef8d5 into develop Jul 30, 2018
@kroening kroening deleted the invalid-pointer-flattening branch July 30, 2018 17:52
NathanJPhillips added a commit to NathanJPhillips/cbmc that referenced this pull request Aug 22, 2018
6a10f1a Merge pull request diffblue#2462 from tautschnig/vs-goto-inline
96fc7b1 Merge pull request diffblue#2654 from peterschrammel/update-jml8
1847066 Update jbmc/lib/java-models-library to java-models-library#8 (remove sun.* imports)
c157ba7 Revert "CMake version.cpp: switch back to add_custom_target"
7a009d6 Merge pull request diffblue#2650 from diffblue/aws-codebuild-clcache
63652fc add clcache to Windows build
6f72b3b Merge pull request diffblue#2657 from smowton/smowton/fix/cmake-version-cpp
35d09d5 CMake version.cpp: switch back to add_custom_target
80331d8 Merge pull request diffblue#2638 from diffblue/CBMC_VERSION_string
7c066f9 Merge pull request diffblue#2618 from owen-jones-diffblue/doc/move-irep-docs-from-util-to-irep
ad5c375 use a string instead of macro for version number
b6258db Merge pull request diffblue#2509 from danpoe/feature/sharing-map-stats
eb71a01 Merge pull request diffblue#2639 from thk123/array-element-type
c5519ec Merge pull request diffblue#2640 from allredj/support-for-load-containing-class-only
0855872 Address review comments diffblue#2
eaa7664 Wrap lines properly
e682eb9 Add \ref in lots of places
3023cca Address review comments
758f069 Move dstringt documentation to above dstringt
a54c82d Move documentation of irept to be above irept
b827ea4 Use type equality check in unit tests
77185fd Merge pull request diffblue#2607 from jeannielynnmoulton/jeannie/ParseThrownExceptions2
1f4ef40 Add class loader debug output
4ae8eb6 Move sharing map friends declarations to unit tests
186897c Sharing stats for the sharing map
4438b43 Fix sharing map internal assertion
332febe Remove wrong sharing map internal assertions
be7e140 Activate internal checks for the sharing map unit tests
713d3fe Merge pull request diffblue#2636 from polgreen/fix_function_map
87f90ee Add replace_all string utility
ea2d393 Make test work on windows
4fa9943 Make array element type be not a comment
655248a Add unit test for when there are no exceptions.
a6e7c4b Refactors interface for exceptions to not use irepts.
1134bba Creates java_method_typet which extends code_typet
aa83622 Unit tests method get_super_class
565c999 Unit tests throws exceptions parsing.
eb88509 Use parsed information for thrown exceptions.
5c7dcac Parses the exception attribute
7fcc42d Adds const to get/set_outer_class
5994dd8 Add method to get super class from java class type.
fbad2d9 Rename variable extends to super_class
6d0776f if function is not in the function map, treat as if it has no body
da86bdb Merge pull request diffblue#2602 from diffblue/__CPROVER_r/w_ok
0202f34 refactor pointer_validity_check using address_check
4a24ad4 use __CPROVER_r/w_ok in string.c library
732ce2a expand __CPROVER_r/w_ok in goto_check
acfea65 __CPROVER_r_ok and __CPROVER_w_ok added to ANSI-C front-end
0618f7d Merge pull request diffblue#2628 from diffblue/clang-extensions
5e43131 Merge pull request diffblue#2608 from diffblue/ms_cl_int64
7c56091 Merge pull request diffblue#2634 from qaphla/local_bitvector_analysis_regression
44ef8d5 Merge pull request diffblue#2630 from diffblue/invalid-pointer-flattening
f74c161 test for __float80 and __float128
8288a72 __float80 is a typedef, not a keyword
5495625 FreeBSD: default flavor is now CLANG
e63402e added _Null_unspecified clang extension
16a49a7 bugfix: __float128
3849bb0 rename APPLE flavor to CLANG
060b59c separate pointer check for integer addresses
9b5847e fix flattening of ID_invalid_pointer
432dcf1 Added a regression test checking that --pointer-check does not generate excess checks if local_bitvector_analysis can gather information on the pointer being checked.
cbfcc5c added support for _int64 keyword
0148347 Avoid signed/unsigned casts and conversion in goto_inline

git-subtree-dir: cbmc
git-subtree-split: 6a10f1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants