Skip to content

Fix String type initialisation when --refine-strings is not active #2188

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

Conversation

smowton
Copy link
Contributor

@smowton smowton commented May 16, 2018

Previously this would incorrectly leave a branch on which the object was not initialised at all. Now it is always initialised, falling back to conventional initialisation when string-specific init is not possible.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Please check the removal of the else

// case we fall back to normal pointer target init.
if(!java_string_library_preprocesst::implements_java_char_sequence_pointer(
expr.type()) ||
add_nondet_string_pointer_initialization(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap multi-line body in braces

symbol_table,
loc,
object_factory_parameters.function_id,
assignments))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the else intentionally deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no I see - I think this way of structuring it is very confusing to follow - suggest using above suggestion to make it more obvious

assignments);
}
else
// Note string-type-specific initialization might fail, e.g. if java.lang.CharSequence does not
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of using critical side effect functions in the if statement as they tend to get lost. I think it would be clearer to:

if(!java_string_library_preprocesst::implements_java_char_sequence_pointer(
  expr.type()))
{
  bool error_adding_string_pointer = add_nondet_string_pointer_initialization(...);
  if(error_adding_string_pointer)
  {
    // tring-type-specific initialization failed, e.g. if java.lang.CharSequence does not ...
    gen_pointer_target_init(...);
  }
}
else 
{
  get_pointer_target_init(...);
}

Previously this would incorrectly leave a branch on which the object was not
initialised at all. Now it is always initialised, falling back to conventional
initialisation when string-specific init is not possible.
@smowton smowton force-pushed the smowton/fix/init-string-types-without-refine-strings branch from d9a74a9 to 57988fc Compare May 16, 2018 12:50
@smowton
Copy link
Contributor Author

smowton commented May 16, 2018

@thk123 restructured as suggested

Copy link
Contributor

@mgudemann mgudemann left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Pending CI failure of course

@smowton smowton merged commit ead0aa3 into diffblue:develop May 16, 2018
NathanJPhillips pushed a commit to NathanJPhillips/cbmc that referenced this pull request Aug 22, 2018
31ef182 Merge pull request diffblue#2208 from diffblue/cleanout-goto-programs
0e84d3e Merge pull request diffblue#2195 from smowton/smowton/feature/smarter-reachability-slicer
c801126 Merge pull request diffblue#2209 from diffblue/travis-no-osx-g++
3487bf7 Reachability slicer: mark reachable code more precisely
85e8451 Merge pull request diffblue#2066 from tautschnig/bv-endianness
5e5eafb Merge pull request diffblue#2191 from smowton/smowton/feature/java-fatal-assertions
8a68ab8 remove g++ build on OS X; this is identical to the clang++ build
48e5b25 Amend faulty long-string test
3f718ba Java: make null-pointer and similar checks fatal
821eb1d remove basic_blocks and goto_program_irep
d87c2db Merge pull request diffblue#2203 from diffblue/typed-lookup
d77dea3 strongly type the lookup() methods in namespacet
2382f27 Merge pull request diffblue#2193 from martin-cs/feature/correct-instruction-documentation
c314272 Merge pull request diffblue#2181 from diffblue/format-expr-constants
514a0a5 Merge pull request diffblue#2167 from diffblue/parse_unwindset_options
0102452 move escape(s) to string_utils
f1b6174 use unwindsett in goto-instrument
dcc907a introduce unwindsett for unwinding limits
10aeae8 format_expr now does index, c_bool constants, string constants
92b92d6 Correct the documentation of ASSERT : it does not alter control flow.
a11add8 Expand the documentation of the goto-program instructions.
a06503b Merge pull request diffblue#2197 from tautschnig/fix-help
05e4bc3 Remove stray whitespace previously demanded by clang-format
3261f4d Fix help output of --generate-function-body-options
7c67b23 Merge pull request diffblue#2110 from tautschnig/type-mismatch-exception
18fb262 Merge pull request diffblue#2025 from tautschnig/stack-depth-fix
9191b6a Merge pull request diffblue#2199 from tautschnig/simplifier-typing
f99e631 Merge pull request diffblue#2198 from tautschnig/fix-test-desc
1a79a11 stack depth instrumentation: __CPROVER_initialize may be empty
a7690ba Merge pull request diffblue#2185 from smowton/smowton/fix/tolerate-ts18661-types
fc02e8f Restore returns before writing the simplified binary
49333eb Make restore_returns handle simplified programs
46f7987 Fix perl regular expressioons in regression test descriptions
9fe432b Merge pull request diffblue#1899 from danpoe/sharing-map-catch-unit-tests
9cc6aae Merge pull request diffblue#2081 from hannes-steffenhagen-diffblue/floating_point_simplificiation
da19abe Tolerate TS 18661 (_Float32 et al) types
a055454 Try all rounding modes when domain is unknown
5f7bc15 Add float support to constant propagator domain
3dc2244 Move adjust_float_expressions to goto-programs
06d8bea Merge pull request diffblue#2187 from diffblue/move-convert-nondet
6d8c3fa Merge pull request diffblue#2189 from thk123/bugfix/json-parser-restart
2ad157f Merge pull request diffblue#2186 from smowton/smowton/fix/call-graph-uninit-field
cd54ad7 Corrected state persistence in the json parser
4447be0 Fix uninitialised collect_callsites field in call_grapht
ead0aa3 Merge pull request diffblue#2188 from smowton/smowton/fix/init-string-types-without-refine-strings
57988fc Fix String type initialisation when --refine-strings is not active
6a76aff Move convert_nondet to java_bytecode
ac6eb21 Merge pull request diffblue#1858 from danpoe/dependence-graph-fix
10b8b09 Merge pull request diffblue#2011 from thomasspriggs/final_classes
a154593 Merge pull request diffblue#2087 from danpoe/feature/small-map
7002909 More dependence graph tests
66263ea Make dependence graph merge() return true when abstract state changes
3aa6dca Control dependency computation fix
a408423 Simplified state merging in the dependence graph
0315816 Merge pull request diffblue#2180 from thomasspriggs/json_id2string
8941567 Merge pull request diffblue#2124 from smowton/smowton/feature/fallback-function-provider
cd04e70 JBMC: use simple method stubbing pass
a6b2cda Add Java simple method stubbing pass
ec1127c Lazy goto model: hook for driver program to replace or stub functions
b6a4382 Remove `id2string` from inside calls to the `json_stringt` constructor.
b673ebb Add storage of final modifier status of java classes in `java_class_typet`.
a2ad909 Small map
808dade Provide suitable diagnostics for equality-without-matching-types
89cf6fe Throw appropriate exceptions when converting constraints
2ae66c2 Produce a proper error message when catching a std::runtime_error at top level
e7b3ade Workaround for Visual Studio loss of CV qualifier bug
1f661f5 Move sharing map and sharing node unit tests to util
47463a3 Use std::size_t instead of unsigned in the sharing map
3e22217 Sharing map documentation
e54f740 Fix sharing map compiler warnings
bcc489c Move sharing map unit tests to catch
34114b8 Use a specialised endianness map for flattening

git-subtree-dir: cbmc
git-subtree-split: 31ef182
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.

4 participants