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

Permit re-setting --object-bits #7858

Merged
merged 4 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions doc/cprover-manual/goto-cc.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,6 @@ most important architectural parameters are:
`sizeof(long int)` on various machines.
- The width of pointers; for example, compare the value of `sizeof(int *)` on
various machines.
- The number of bits in a pointer which are used to differentiate between
different objects. The remaining bits of a pointer are used for offsets
within objects.
- The [endianness](http://en.wikipedia.org/wiki/Endianness) of
the architecture.

Expand All @@ -152,8 +149,6 @@ following command-line arguments can be passed to the CPROVER tools:
- The word-width can be set with `--16`, `--32`, `--64`.
- The endianness can be defined with `--little-endian` and
`--big-endian`.
- The number of bits in a pointer used to differentiate between different
objects can be set using `--object-bits x`. Where `x` is the number of bits.

When using a goto binary, CBMC and the other tools read the
configuration from the binary. The setting when running goto-cc is
Expand Down
7 changes: 4 additions & 3 deletions doc/man/goto-cc.1
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,10 @@ files.
.TP
\fB\-\-print\-rejected\-preprocessed\-source\fR \fIfile\fR
Copy failing (preprocessed) source to \fIfile\fR.
.TP
\fB\-\-object\-bits\fR \fIN\fR
Configure the number of bits used for object numbering in CBMC's pointer encoding.
.SH BACKWARD COMPATIBILITY
.B goto\-cc
will warn and ignore the use of \fB\-\-object\-bits\fR, which previous versions
Copy link
Member

Choose a reason for hiding this comment

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

❓ Where is this warning produced in this PR? Or does that already exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

goto-cc warns about all options that it doesn't explicitly know about, so by virtue of removing it from gcc_cmdline.cpp we get this.

processed. This option now instead needs to be passed to \fBcbmc\fR(1).
.SH ENVIRONMENT
All tools honor the TMPDIR environment variable when generating temporary
files and directories.
Expand Down
1 change: 0 additions & 1 deletion regression/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ else()
add_subdirectory(goto-cl)
endif()
add_subdirectory(goto-cc-cbmc)
add_subdirectory(goto-cc-cbmc-shared-options)
add_subdirectory(cbmc-cpp)
add_subdirectory(goto-cc-goto-analyzer)
add_subdirectory(goto-analyzer-simplify)
Expand Down
1 change: 0 additions & 1 deletion regression/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ DIRS = cbmc-shadow-memory \
goto-gcc \
goto-cl \
goto-cc-cbmc \
goto-cc-cbmc-shared-options \
cbmc-cpp \
goto-cc-goto-analyzer \
goto-analyzer-simplify \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
#include <stdlib.h>
#include <stdint.h>

extern size_t __CPROVER_max_malloc_size;

#if defined(_WIN32) && defined(_M_X64)
int __builtin_clzll(unsigned long long);
#define __nof_symex_objects \
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
CORE dfcc-only
THOROUGH dfcc-only
main.c
--malloc-may-fail --malloc-fail-null --dfcc main --enforce-contract foo _ --pointer-check --pointer-primitive-check
^EXIT=0$
Expand Down
11 changes: 0 additions & 11 deletions regression/goto-cc-cbmc-shared-options/CMakeLists.txt

This file was deleted.

30 changes: 0 additions & 30 deletions regression/goto-cc-cbmc-shared-options/Makefile

This file was deleted.

4 changes: 0 additions & 4 deletions regression/goto-cc-cbmc-shared-options/README.md

This file was deleted.

20 changes: 0 additions & 20 deletions regression/goto-cc-cbmc-shared-options/chain.sh

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
CORE winbug
CORE
test.c
--function main --object-bits 6
^EXIT=10$
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
CORE winbug
CORE
test.c
--function main --object-bits 10
^EXIT=10$
Expand Down
34 changes: 3 additions & 31 deletions src/ansi-c/ansi_c_internal_additions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,34 +124,6 @@ static std::string architecture_string(T value, const char *s)
std::string(s) + "=" + std::to_string(value) + ";\n";
}

/// The maximum allocation size is determined by the number of bits that
/// are left in the pointer of width \p pointer_width.
///
/// The allocation size cannot exceed the number represented by the (signed)
/// offset, otherwise it would not be possible to store a pointer into a
/// valid bit of memory. Therefore, the max allocation size is
/// 2^(offset_bits - 1), where the offset bits is the number of bits left in the
/// pointer after the object bits.
///
/// The offset must be signed, as a pointer can point to the end of the memory
/// block, and needs to be able to point back to the start.
/// \param pointer_width: The width of the pointer
/// \param object_bits : The number of bits used to represent the ID
/// \return The size in bytes of the maximum allocation supported.
static mp_integer
max_malloc_size(std::size_t pointer_width, std::size_t object_bits)
{
PRECONDITION(pointer_width >= 1);
PRECONDITION(object_bits < pointer_width);
PRECONDITION(object_bits >= 1);
const auto offset_bits = pointer_width - object_bits;
// We require the offset to be able to express upto allocation_size - 1,
// but also down to -allocation_size, therefore the size is allowable
// is number of bits, less the signed bit.
const auto bits_for_positive_offset = offset_bits - 1;
return ((mp_integer)1) << (mp_integer)bits_for_positive_offset;
}

void ansi_c_internal_additions(std::string &code, bool support_float16_type)
{
// clang-format off
Expand All @@ -175,9 +147,9 @@ void ansi_c_internal_additions(std::string &code, bool support_float16_type)
CPROVER_PREFIX "size_t size, " CPROVER_PREFIX "bool zero);\n"
"void " CPROVER_PREFIX "deallocate(void *);\n"

CPROVER_PREFIX "size_t " CPROVER_PREFIX "max_malloc_size="+
integer2string(max_malloc_size(config.ansi_c.pointer_width, config
.bv_encoding.object_bits));
CPROVER_PREFIX "thread_local " CPROVER_PREFIX "size_t "
CPROVER_PREFIX "max_malloc_size="+
integer2string(config.max_malloc_size());
if(config.ansi_c.pointer_width==config.ansi_c.long_int_width)
code += "UL;\n";
else if(config.ansi_c.pointer_width==config.ansi_c.long_long_int_width)
Expand Down
1 change: 1 addition & 0 deletions src/ansi-c/cprover_library.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,5 @@ void cprover_c_library_factory_force_load(
const std::set<irep_idt> &functions,
symbol_table_baset &symbol_table,
message_handlert &message_handler);

#endif // CPROVER_ANSI_C_CPROVER_LIBRARY_H
5 changes: 4 additions & 1 deletion src/ansi-c/library/cprover.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,11 @@ extern const void *__CPROVER_deallocated;
extern const void *__CPROVER_memory_leak;

extern int __CPROVER_malloc_failure_mode;
extern __CPROVER_size_t __CPROVER_max_malloc_size;
extern __CPROVER_bool __CPROVER_malloc_may_fail;
// The maximum size of an object that we can handle under the object:offset
// pointer encoding. Marked thread-local as it is a per-analysis constant that
// can safely be constant-propagated even in concurrent execution.
extern __CPROVER_thread_local __CPROVER_size_t __CPROVER_max_malloc_size;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this thread local?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to avoid our concurrency encoding picking the variable as one that can't be constant-propagated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have now added a comment to clarify this.

Copy link
Member

Choose a reason for hiding this comment

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

Would making it const achieve that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't commonly trust const at this time. We could, however, consider a lightweight analysis that establishes that a variable (irrespective of whether it is marked const or not) is only ever written once (and doesn't have its address taken).


// malloc failure modes
extern int __CPROVER_malloc_failure_mode_return_null;
Expand Down
1 change: 0 additions & 1 deletion src/ansi-c/library/cprover_contracts.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#define __CPROVER_contracts_library_defined

// external dependencies
extern __CPROVER_size_t __CPROVER_max_malloc_size;
const void *__CPROVER_alloca_object = 0;
extern const void *__CPROVER_deallocated;
const void *__CPROVER_new_object = 0;
Expand Down
2 changes: 2 additions & 0 deletions src/cbmc/cbmc_parse_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,8 @@ int cbmc_parse_optionst::get_goto_program(

goto_model = initialize_goto_model(cmdline.args, ui_message_handler, options);

update_max_malloc_size(goto_model, ui_message_handler);

if(cmdline.isset("show-symbol-table"))
{
show_symbol_table(goto_model, ui_message_handler);
Expand Down
1 change: 0 additions & 1 deletion src/goto-cc/gcc_cmdline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ const char *goto_cc_options_with_separated_argument[]=
"--native-linker",
"--print-rejected-preprocessed-source",
"--mangle-suffix",
"--object-bits",
nullptr
};

Expand Down
1 change: 0 additions & 1 deletion src/goto-cc/goto_cc_mode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ void goto_cc_modet::help()
"symbols\n"
" {y--print-rejected-preprocessed-source} {ufile} \t "
"copy failing (preprocessed) source to file\n"
" {y--object-bits} {N} \t number of bits used for object addresses\n"
"\n");
}

Expand Down
40 changes: 16 additions & 24 deletions src/goto-instrument/nondet_static.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,17 @@
/// assigned to nondet-initializable static variables with nondeterministic
/// values.
/// \param ns: Namespace for resolving type information.
/// \param [out] goto_functions: Existing goto-functions to be updated.
/// \param [inout] goto_model: Existing goto-functions and symbol table to
/// be updated.
/// \param fct_name: Name of the goto-function to be updated.
static void nondet_static(
const namespacet &ns,
goto_functionst &goto_functions,
goto_modelt &goto_model,
const irep_idt &fct_name)
{
goto_functionst::function_mapt::iterator fct_entry =
goto_functions.function_map.find(fct_name);
CHECK_RETURN(fct_entry != goto_functions.function_map.end());
goto_model.goto_functions.function_map.find(fct_name);
CHECK_RETURN(fct_entry != goto_model.goto_functions.function_map.end());

goto_programt &init = fct_entry->second.body;

Expand All @@ -99,11 +100,11 @@

if(is_nondet_initializable_static(sym, ns))
{
const auto source_location = instruction.source_location();
instruction = goto_programt::make_assignment(
code_assignt(
sym, side_effect_expr_nondett(sym.type(), source_location)),
source_location);
side_effect_expr_nondett nondet{
sym.type(), instruction.source_location()};
instruction.assign_rhs_nonconst() = nondet;
goto_model.symbol_table.get_writeable_ref(sym.get_identifier()).value =
nondet;
Copy link
Contributor

@thomasspriggs thomasspriggs Dec 7, 2023

Choose a reason for hiding this comment

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

This line here seems to be the cause of the failure in the incremental SMT2 decision procedure. The failure happens during the conversion of the y symbol. Before your PR, the symbol value is an expression for the integer 23. After your PR it gains the value

side_effect
  * type: signedbv
      * width: 32
      * #c_type: signed_int
  * #source_location: 
    * file: main.c
    * line: 4
    * function: 
    * working_directory: /home/tspriggs/Development/cbmc/regression/cbmc/no_nondet_static
  * statement: nondet
  * is_nondet_nullable: 1

This isn't supported by the decision procedure. I think the expressions sent to it either directly or from looking up the values of symbols are expected to be side effect free. The only exception would be in the case where we take the address of a code typed expression. The example in this test does not fall into this exception, because y symbol_exprt has a bit vector type.

It could be argued that the decision procedure should not error in this case. However I would say that this change in the value of symbols unrelated to max_malloc_size / object bits appears to be a change unrelated to the intended purpose of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to figure out why the value in the symbol table is used directly. What should happen is that such a value is compiled into an assignment in __CPROVER_initialize, and replace_nondet will resolve this during symex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem appears to have been introduced in f286c07: @thomasspriggs, why is it deemed necessary to lookup a symbol's value in smt2_incremental_decision_proceduret::define_dependent_functions?!

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the API for decision_proceduret is not sufficiently well documented or explicit with regards to how symbol expressions should be treated? I have (incorrectly) assumed that symbol expressions referring to symbols which have not previously been seen, means that these symbols are implicitly defined such that their value is equal to the value in the symbol table. This seemed to work well up to this point.

I can raise a PR which changes this such that symbol expressions are implicitly defined to have a nondeterministic value instead, if that is how a decision procedure should be implemented.

}
}
else if(instruction.is_function_call())
Expand All @@ -113,33 +114,24 @@
// see cpp/cpp_typecheck.cpp, which creates initialization functions
if(fsym.get_identifier().starts_with("#cpp_dynamic_initialization#"))
{
nondet_static(ns, goto_functions, fsym.get_identifier());
nondet_static(ns, goto_model, fsym.get_identifier());

Check warning on line 117 in src/goto-instrument/nondet_static.cpp

View check run for this annotation

Codecov / codecov/patch

src/goto-instrument/nondet_static.cpp#L117

Added line #L117 was not covered by tests
}
}
}

// update counters etc.
goto_functions.update();
}

/// Nondeterministically initializes global scope variables in
/// CPROVER_initialize function.
/// \param ns: Namespace for resolving type information.
/// \param [out] goto_functions: Existing goto-functions to be updated.
void nondet_static(const namespacet &ns, goto_functionst &goto_functions)
{
nondet_static(ns, goto_functions, INITIALIZE_FUNCTION);
goto_model.goto_functions.update();
}

/// First main entry point of the module. Nondeterministically initializes
/// global scope variables, except for constants (such as string literals, final
/// fields) and internal variables (such as CPROVER and symex variables,
/// language specific internal variables).
/// \param [out] goto_model: Existing goto-model to be updated.
/// \param [inout] goto_model: Existing goto-model to be updated.
void nondet_static(goto_modelt &goto_model)
{
const namespacet ns(goto_model.symbol_table);
nondet_static(ns, goto_model.goto_functions);
nondet_static(ns, goto_model, INITIALIZE_FUNCTION);
}

/// Second main entry point of the module. Nondeterministically initializes
Expand Down Expand Up @@ -199,7 +191,7 @@
}
}

nondet_static(ns, goto_model.goto_functions, INITIALIZE_FUNCTION);
nondet_static(ns, goto_model, INITIALIZE_FUNCTION);
}

/// Nondeterministically initializes global scope variables that
Expand Down Expand Up @@ -227,5 +219,5 @@
}

const namespacet ns(goto_model.symbol_table);
nondet_static(ns, goto_model.goto_functions, INITIALIZE_FUNCTION);
nondet_static(ns, goto_model, INITIALIZE_FUNCTION);
}
4 changes: 0 additions & 4 deletions src/goto-instrument/nondet_static.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ bool is_nondet_initializable_static(
const symbol_exprt &symbol_expr,
const namespacet &ns);

void nondet_static(
const namespacet &ns,
goto_functionst &goto_functions);

void nondet_static(goto_modelt &);

void nondet_static(goto_modelt &, const std::set<std::string> &);
Expand Down
29 changes: 29 additions & 0 deletions src/goto-programs/initialize_goto_model.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ Author: Daniel Kroening, kroening@kroening.com

#include "initialize_goto_model.h"

#include <util/arith_tools.h>
#include <util/c_types.h>
#include <util/config.h>
#include <util/exception_utils.h>
#include <util/message.h>
Expand All @@ -22,6 +24,7 @@ Author: Daniel Kroening, kroening@kroening.com
#include <langapi/language.h>
#include <langapi/language_file.h>
#include <langapi/mode.h>
#include <linking/static_lifetime_init.h>

#include "goto_convert_functions.h"
#include "read_goto_binary.h"
Expand Down Expand Up @@ -236,3 +239,29 @@ goto_modelt initialize_goto_model(

return goto_model;
}

void update_max_malloc_size(
goto_modelt &goto_model,
message_handlert &message_handler)
{
if(!goto_model.symbol_table.has_symbol(CPROVER_PREFIX "max_malloc_size"))
return;

const auto previous_max_malloc_size_value = numeric_cast<mp_integer>(
goto_model.symbol_table.lookup_ref(CPROVER_PREFIX "max_malloc_size").value);
const mp_integer current_max_malloc_size = config.max_malloc_size();

if(
!previous_max_malloc_size_value.has_value() ||
*previous_max_malloc_size_value != current_max_malloc_size)
{
symbolt &max_malloc_size_sym = goto_model.symbol_table.get_writeable_ref(
CPROVER_PREFIX "max_malloc_size");
max_malloc_size_sym.value =
from_integer(current_max_malloc_size, size_type());
max_malloc_size_sym.value.set(ID_C_no_nondet_initialization, true);

if(goto_model.can_produce_function(INITIALIZE_FUNCTION))
recreate_initialize_function(goto_model, message_handler);
}
}
Loading
Loading