Prefix temporary variable names with function id #2077
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
peterschrammel
requested review from
cesaro,
chrisr-diffblue,
kroening,
martin-cs,
mgudemann,
smowton,
tautschnig and
thk123
as code owners
peterschrammel
force-pushed
the
stable-tmp-var-names
branch
from
c29b584 to
4c542c0
Compare
tautschnig
requested changes
Apr 16, 2018
There was a problem hiding this comment.
I believe this is very helpful, thinking about the code that dump-c happily generates. Just one problem (as noted below): 5832ffd got lost.
| ^a=&tmp\$\d+!0, tmp\$\d+=&tmp\$\d+!0, tmp\$\d+=4$ | ||
| ^a=\(\(signed int \*\*\)NULL\), tmp(|\$\d+)=[^,]*, tmp(|\$\d+)=[^,]*$ | ||
| ^a=&tmp(|\$\d+)!0, tmp(|\$\d+)=\(\(signed int \*\)NULL\), tmp(|\$\d+)=[^,]*$ | ||
| ^a=&tmp(|\$\d+)!0, tmp(|\$\d+)=&tmp(|\$\d+)!0, tmp(|\$\d+)=([012356789][0-9]*|4[0-9]+)$ |
There was a problem hiding this comment.
I believe this lost the change from 5832ffd
| @@ -2,8 +2,8 @@ CORE | |||
| main.c | |||
|
|
|||
| ^\*\* Results:$ | |||
| ^\[main.assertion.1\] assertion tmp_if_expr\$1: FAILURE$ | |||
| ^\[main.assertion.2\] assertion tmp_if_expr\$2: SUCCESS$ | |||
| ^\[main.assertion.1\] assertion tmp_if_expr(|\$\d+): FAILURE$ | |||
There was a problem hiding this comment.
I think (\$\d+)? is more idiomatic than (|...)
peterschrammel
force-pushed
the
stable-tmp-var-names
branch
from
4c542c0 to
8ac3fc1
Compare
|
@tautschnig, I've addressed your comments. |
tautschnig
approved these changes
Apr 17, 2018
tautschnig
reviewed
Apr 17, 2018
| @@ -114,6 +114,7 @@ static void create_clinit_wrapper_symbols( | |||
| symbolt wrapper_method_symbol; | |||
| code_typet wrapper_method_type; | |||
| wrapper_method_type.return_type() = void_typet(); | |||
| wrapper_method_type.parameters(); // to ensure the property is there | |||
There was a problem hiding this comment.
Why is this necessary?! The only impact that I can think of is the hash value and comparison between irepts.
There was a problem hiding this comment.
Exactly for that reason.
There was a problem hiding this comment.
So what code is it that is impacted by changes in the ordering resulting from those differences here?
There was a problem hiding this comment.
But then the same program should always yield the same irept (previously without an ID_parameters in named_sub and now with such an entry). Hence I'm still surprised that this impacts goto-diff in some way.
There was a problem hiding this comment.
In principle, yes. The root cause is likely an instability elsewhere in the code. I found this issue only after diffing two 200MB irep dumps. We'll have to investigate this further by shrinking the large example that I have. Any similar sources of nondeterminism will cause further trouble in future. For now, this fix will solve the problem for the customer.
There was a problem hiding this comment.
You might just want to log this in an issue to ensure immediate progress while not forgetting about the problem. As the issue surfaces in goto-diff, however, which data structure in goto-diff is it that is being traversed and found to be spuriously different?
peterschrammel
force-pushed
the
stable-tmp-var-names
branch
2 times, most recently
from
f1dbe6a to
d1d4ea8
Compare
Temporary variable names must be globally unique and changing the number of temporaries in one function must not change a variable name in another function. Otherwise, this has adverse effects on goto-diff. This commit gets rid of various global temporary variables counters that do not meet above criteria. Instead, we now always use the function id to prefix temporaries to eliminate cross-function effects and increment the numeric suffix only when the entire variable name already exists, using the rename facility.
Many accesses to the function type would add an ID_parameters if it is not there, which can cause inconsistencies in irep comparisons.
peterschrammel
force-pushed
the
stable-tmp-var-names
branch
from
d1d4ea8 to
0b3170d
Compare
smowton
added a commit
to smowton/cbmc
that referenced
this pull request
May 9, 2018
ad62682 Merge pull request diffblue#2071 from thk123/refactor/split-string-unit-tests fc8ba88 Revert to aborting precondition for function inputs 3e2ab6f Merge pull request diffblue#2080 from diffblue/java-bytecode-dependency 6ff1eec cbmc: remove dependency on java_bytecode 0bff19b Merge pull request diffblue#2049 from karkhaz/kk-factor-goto-model-processing 79e3b25 Merge pull request diffblue#2084 from tautschnig/has_subtype-test cd45b0b Test has_subtype on recursive data types 85ac315 Merge pull request diffblue#2082 from thomasspriggs/default_dstring_hash 28c2e8b Merge pull request diffblue#2065 from tautschnig/be-constructor afa6023 Merge pull request diffblue#2061 from tautschnig/simplify-extractbits 014d151 Factor out getting & processing goto-model 06b3adc Merge pull request diffblue#2077 from peterschrammel/stable-tmp-var-names 0b3170d Stabilize clinit wrapper function type parameters 3cd8bf4 Temporary vars tests for goto-diff 9f0626c Prefix temporary var names with function id ca678aa More permissive regression tests regarding tmp var suffixes 47951ca Merge pull request diffblue#2079 from romainbrenguier/bugfix/has-subtype-recursion dd73b1a Specify default hash function of `dstringt` to STL. fe8e589 Avoid infinite recursion in has_subtype 00b9bf6 Merge pull request diffblue#2051 from svorenova/generics_tg2717 cd4bfc3 Merge pull request diffblue#2078 from romainbrenguier/bool-literal-in-while-loop 67ea889 Use bool literal in while loop d229ad9 Merge pull request diffblue#2056 from diffblue/fix-regression-cbmc-memcpy1 506faf0 Refactor a function for base existence 617d388 Utility functions for generic types c07e6ca Update generic specialization map when replacing pointers ed26d0a Merge pull request diffblue#2058 from peterschrammel/stable-disjuncts b663734 Simplify extractbits(concatenation(...)) b091560 Typing and refactoring of simplify_extractbits 49ad1bd Merge pull request diffblue#974 from tautschnig/fix-assert-encoding 16e9599 Merge pull request diffblue#2063 from tautschnig/has-subtype 950f58b Merge pull request diffblue#2060 from tautschnig/opt-local-map 4222a94 Regression tests for unstable instanceof and virtual method disjuncts b44589e Make disjuncts in instanceof removal independent of class loading 3afff86 Make disjuncts in virtual method removal independent of class loading a385d9b Allowed split_string to be used on whitespace if not also trying to strip fe4a642 Merge pull request diffblue#2062 from tautschnig/no-has-deref 145f474 Adding tests for empty string edge cases 07009d4 Refactored test to run all combinations 252c24c Migrate old string utils unit tests e87edbf Removing wrong elements from the make file b165c52 make test work on 32-bit Linux b804570 Merge pull request diffblue#2048 from JohnDumbell/improvement/adding_null_object_id 61f14d8 Merge pull request diffblue#1962 from owen-jones-diffblue/owen-jones-diffblue/simplify-replace-java-nondet fdee7e8 Merge pull request diffblue#2059 from tautschnig/generalise-test 4625cc5 Extend global has_subexpr to take a predicate as has_subtype does e9ebd59 has_subtype(type, pred, ns) to search for contained type matching pred 1f1f67f Merge pull request diffblue#1889 from hannes-steffenhagen-diffblue/develop-feature_generate_function_bodies 048c188 Add unit test for java_replace_nondet 0fe48c9 Make remove_java_nondet work before remove_returns bcc4dc4 Use byte_extract_exprt constructor a1814a3 Get rid of thin (and duplicate) has_dereference wrapper 4122a28 Test to demonstrate assert bug on alpine d44bfd3 Also simplify assert structure found on alpine linux c5cde18 Do not generate redundant if statements in assert expansions 4fb0603 Make is_skip publicly available and use constant argument 5832ffd Negative numbers should also pass the test 3c23b28 Consistently disable simplify_exprt::local_replace_map da63652 Merge pull request diffblue#2054 from romainbrenguier/bugfix/clear-equations d77f6a2 Merge pull request diffblue#1831 from NathanJPhillips/feature/class-annotations 60c8296 Clear string_refinement equations (not dependencies) 314ed53 Correcting the value of ID_null_object 751a882 Factor out default CBMC options to static method 6f24009 Can now test for an option being set in optionst 9a8d937 Add to_annotated_type and enable type_checked_cast for annotated_typet ca77b4e Add test for added annotations b06a27d Introduce abstract qualifierst base class e6fb3bf Pretty printing of java_class_typet e22b95b Fix spurious virtual function related keywords 3ac6d17 Add type_dynamic_cast and friends for java_class_typet ce1f4d2 Add annotations to java_class_typet, its methods and fields f84753d Merge pull request diffblue#2042 from hannes-steffenhagen-diffblue/add_deprecate_macro 7a38669 Merge pull request diffblue#2017 from NathanJPhillips/feature/overlay-classes 75a4aec Revert "the deprecation will need to wait until codebase is clean" 67735b5 Disable deprecation warnings by default 0764f77 Merge pull request diffblue#2036 from romainbrenguier/id_array_list 690b606 Merge pull request diffblue#2039 from peterschrammel/fix-duplicate-msg-json-ui bba17d9 the deprecation will need to wait until codebase is clean 822c757 Regression test for redundant JSON message output de0644d Only force end of previous message if there actually is one. 5a637bf Merge pull request diffblue#2037 from hannes-steffenhagen-diffblue/add_deprecate_macro bff456a Merge pull request diffblue#2040 from tautschnig/remove-swp 87ebe90 Remove vim temp file 228c019 Fix duplicate message output in json-ui 0a2c43e Add DEPRECATED to functions documented as \deprecated 47f4b35 interval_sparse_arrayt constructor from array-list 026c4ca Declare an array_list_exprt class 50a2696 Define ID_array_list 513b67a Merge pull request diffblue#2038 from romainbrenguier/bugfix/assign-at-malloc-site c207106 Test with array of strings 60183a3 Assign string at malloc site 116fffd Add DEPRECATED macro to mark deprecated functions and variables 7952f2c Add option to generate function body to goto-instrument 3d4183a Add ability to overlay classes with new definitions of existing methods dbc2f71 Improve code and error message in infer_opaque_type_fields 7c0ea4d Tidied up java_class_loader_limitt git-subtree-dir: cbmc git-subtree-split: ad62682
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Temporary variable names must be globally unique and
changing the number of temporaries in one function
must not change a variable name in another function.
Otherwise, this has adverse effects on goto-diff.
This PR gets rid of various global temporary
variables counters that do not meet above criteria.
Instead, we now always use the function id to prefix
temporaries to eliminate cross-function effects
and increment the numeric suffix only when the
entire variable name already exists, using the rename
facility that has already been used for the same purpose
in other places.