-
Notifications
You must be signed in to change notification settings - Fork 273
First iteration of C++ CBMC API #7274
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
428ea04
to
5f51ce6
Compare
Shouldn't this PR be setting up codeowners? |
I currently get the following error when attempting to build locally -
|
cpp_api/call_bmc.cpp
Outdated
@@ -0,0 +1,40 @@ | |||
// Test file to try loading a GOTO-model into memory and running a sample verification run on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we aren't using catch
for this test?
Shouldn't tests be in a separate directory from the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason why we can't have unit tests, but for a first iteration I planned to use this as a binary driver for a regression test, which I thought might be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to think about testing from the start, both unit and regression tests, and rust integration tests.
cpp_api/api.h
Outdated
|
||
// A struct containing the API dependencies (some of them, at least) | ||
struct api_depst { | ||
message_handlert *msg_handler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define a new STL based type for message handling rather than exposing the implementation details of message_handlert
. I am thinking along the lines of a new api_messaget
and corresponding std::function<void(const api_messaget &)> message_handler
.
@NlightNFotis We need to add a |
CMakeLists.txt
Outdated
@@ -215,6 +216,8 @@ cprover_default_properties( | |||
cbmc | |||
cbmc-lib | |||
cpp | |||
api_bin | |||
bmc_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_
does not follow the current naming convention for modules.
Suggestion: cprover-api
cpp_api/api.cpp
Outdated
|
||
#include "api.h" | ||
|
||
api_depst api_deps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope these are temporary. We definitely don't want to have globals. Also see below wrt sessions.
cpp_api/call_bmc.cpp
Outdated
@@ -0,0 +1,40 @@ | |||
// Test file to try loading a GOTO-model into memory and running a sample verification run on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to think about testing from the start, both unit and regression tests, and rust integration tests.
cpp_api/api.cpp
Outdated
api_depst api_deps; | ||
extern configt config; | ||
|
||
void initialize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that we discussed about introducing a session object to encapsulate all the memory owned by the library so that the client has control about the session/memory lifetime.
f0ac9f0
to
a1058d6
Compare
Codecov ReportBase: 78.35% // Head: 78.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #7274 +/- ##
=========================================
Coverage 78.35% 78.36%
=========================================
Files 1645 1651 +6
Lines 190208 190009 -199
=========================================
- Hits 149040 148896 -144
+ Misses 41168 41113 -55
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
ae6c9e2
to
21ddf34
Compare
CMakeLists.txt
Outdated
@@ -206,6 +206,7 @@ option(WITH_MEMORY_ANALYZER | |||
add_subdirectory(src) | |||
add_subdirectory(regression) | |||
add_subdirectory(unit) | |||
add_subdirectory(cpp_api) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpp_api, cprover_api, cprover_api_cpp, cbmc_api, api-... The naming is very inconsistent. We need to settle on a single name that is used throughout. I'd say let's go with cprover-api-cpp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cpp_api
is the directory name, which I've left for now because while I'm playing around with the rust part, I was thinking of renaming the folder into api/cpp
and move the rust bit into api/rust
.
I guess this also refers to the class name, which I can look into naming more appropriately.
cpp_api/api.cpp
Outdated
// Default initialise the api_dependencies object | ||
cbmc_api::api_dependenciest::api_dependenciest() | ||
: // We don't print anything to the console as part of the API | ||
message_handler(new null_message_handlert()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 Unique pointers should be constructed via util_make_unique
rather than directly using new
like this.
cpp_api/api.h
Outdated
// Initialising constructor | ||
explicit cbmc_api(); | ||
cbmc_api(const api_optionst &options); | ||
~cbmc_api(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we put a comment on this destructor in the header file, noting that it is defaulted in the .cpp
file for compatibility with implementing the "pointer to implementation" pattern with unique_ptr
. This saves checking the .cpp
file to work this out, as the prospect of a hand-written destructor implementation can be concerning.
cpp_api/options.h
Outdated
|
||
class optionst; | ||
|
||
class api_optionst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what stage does validation of the supplied options happen? The singular bool option defined now doesn't really need validating. But I foresee a need to report errors if options are specified which don't make sense in combination, or to report warnings if a deprecated option is used.
cpp_api/call_bmc.cpp
Outdated
std::vector<std::string> arguments(argv + 1, argv + argc); | ||
|
||
// Create API options object, to pass to initialiser of API object. | ||
auto api_options = api_optionst::create().simplify(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unique pointer to constant instance makes more sense.
c2ab3ff
to
a888249
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok (minor changes proposed, but nothing to block on).
cprover-cpp-api/readme.md
Outdated
This folder includes the interface and the implementation of a new C++-based | ||
API for the CProver libraries. The aim is for this to be able to be used by | ||
clients to drive all of the verification pipeline without the need to invoke | ||
any of the associated tools. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This [folder/directory] includes the interface and implementation of a C++-based API for the CProver libraries. This allows direct linking to key CProver functionality via C++ function calls. This (will eventually) allow the use of all aspects of the CProver verification pipeline via direct function calls and deprecate the need for invoking separate tools/programs.
cprover-cpp-api/src/api.cpp
Outdated
// Initialise C language mode | ||
register_language(new_ansi_c_language); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a first implementation this is ok I think, but do we want to force/hard-code that the language is C?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed not ideal, but for a first implementation we need to have at least one language mode bootstrapped, and none other than C
seem to fit in this particular scenario.
What I can do is potentially add a todo
comment to make this configurable in the future.
cprover-cpp-api/src/options.cpp
Outdated
return api_optionst{}; | ||
} | ||
|
||
api_optionst &api_optionst::simplify(bool on) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on
-> enable_simplify
?
CODEOWNERS
Outdated
@@ -38,7 +38,7 @@ | |||
/jbmc/src/java_bytecode/ @romainbrenguier @peterschrammel | |||
/src/analyses/ @martin-cs @peterschrammel @chris-ryder | |||
/src/pointer-analysis/ @martin-cs @peterschrammel @chris-ryder | |||
|
|||
/cpp_api/ @diffblue/diffblue-opensource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this explicit with people rather than simply the team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not sure I understand where we are planning to perform error checking on the options. Lets hypothetically say that we add the object-bits
option to the API at some point in the future. This option is required to be positive and less than the number of bits configured for the pointer width. It is currently checked in the parse_object_bits_encoding
function which is used by the command line entry points. How would the error state/string be communicated to users of the new API in the case where an invalid value was set for this option? Which of the new API functions will trigger the validation currently performed by parse_object_bits_encoding
to be carried out?
cpp_api/options.h
Outdated
|
||
class optionst; | ||
|
||
class api_optionst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we declare all classes (and structs) which are part of the API as final
. If users of the API inherit from these classes then it could cause surprising maintenance issues later on. I think this has been prevented for this particular optionst
class by making the constructor private. But marking the class as final
would be more definitive.
cpp_api/api.h
Outdated
|
||
// An object in the pattern of Session Facade - owning all of the memory | ||
// the API is using and being responsible for the management of that. | ||
struct cbmc_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Should the name of this object include the word "session" given that it was added it response to Peter's comment asking for a session object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Maybe it would also make sense to use the letters "api" a little like a namespace. So this would become something like api_sessiont
to match the naming of api_optionst
. If the letters "api" are being used mainly to avoid naming clashes with things that are inside cbmc and excluded from the API, then I think it is confusing for them to be used as the end of some names and at the start of others.
cpp_api/api.h
Outdated
std::unique_ptr<optionst> options; | ||
}; | ||
|
||
std::unique_ptr<goto_modelt> model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 We can actually reduce the number of internal classes which are forward declared in this file. We could have something like an api_session_implementationt
struct and a single line std::unique_ptr<api_session_implementationt> implementation;
in the header file, with the corresponding implementation of the class/struct in the .cpp
for the api_sessiont
class. The fields inside that class can then be values (or unique pointers) as we wish because the .cpp
file includes the implementation headers. The advantage of this is are -
-
Improved compile times. Because the definition of the
api_session_implementationt
would be in the.cpp
we can change what fields this contains and only recompile one file + relink, rather than having to recompile every file that includes the API header. -
Reduced naming clashes. Forward declaring the
message_handlert
/optionst
/goto_modelt
means that users of the API don't need to include the headers defining these classes, but these names could still clash with the names of other unrelated classes of the same names in code bases consuming our api.
This comment is just an idea. So possibly something for a follow-up PR if we think it is worth-while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A worthwhile idea, for sure, but given the in-flux nature of the API (given the rust work being concurrent, requiring changes to the C++ side to facilitate it) I would rather not make any significant structural changes to it right now - at least not until both the C++ side and the Rust side take more concrete form.
cprover-cpp-api/src/api.h
Outdated
@@ -0,0 +1,36 @@ | |||
#ifndef CPROVER_API_API_H | |||
#define CPROVER_API_API_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving these files into a src/libcprover/
sub directory -
- api.h
- api.cpp
- options.h
- options.cpp
Then the work on the rust side of the API could go in a separate src/cprover-rust/
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should all be inside /src
cprover-cpp-api/src/api.h
Outdated
// Initialising constructor | ||
cprover_api(); | ||
explicit cprover_api(const api_optionst &options); | ||
~cprover_api(); // default constructed in the .cpp file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would read better to me if the word constructed
were to be replaced with definition
.
There's currently no designation for a particular function on the API options to do the error checking, but the vision is that there can be a new function called (for example) With regards to communication of this, for now there's no such infrastructure (for simplicity's sake on this first iteration), but the way I picture it is demanding the |
f025c91
to
16b5d7c
Compare
CODEOWNERS
Outdated
@@ -38,7 +38,7 @@ | |||
/jbmc/src/java_bytecode/ @romainbrenguier @peterschrammel | |||
/src/analyses/ @martin-cs @peterschrammel @chris-ryder | |||
/src/pointer-analysis/ @martin-cs @peterschrammel @chris-ryder | |||
|
|||
/cpp_api/ @NlightNFotis @thomasspriggs @esteffin @TGWDB @peterschrammel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to match the actual name
@@ -0,0 +1,39 @@ | |||
file(GLOB_RECURSE api_sources "../src/*.cpp" "../src/*.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the best way to find the files?
std::vector<std::string> arguments(argv + 1, argv + argc); | ||
|
||
// Create API options object, to pass to initialiser of API object. | ||
auto api_options = api_optionst::create().simplify(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why hardcoded to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a sample configuration option - this only set as false in the API binary driver that we use for testing.
This is to be initialised as needed by the users of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is a single option for now, while we set up the scaffold for further work - we intend to expand this with way more options).
// Create API options object, to pass to initialiser of API object. | ||
auto api_options = api_optionst::create().simplify(false); | ||
|
||
// Initialise API dependencies and global configuration in one step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What dependencies are being initialized here?
api_sessiont api(api_options); | ||
|
||
// Demonstrate the loading of a goto-model from the command line arguments | ||
api.load_model_from_files(arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this fail? How are error conditions handled here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As an example, if the filename passed doesn't exist it throws an exception.
// Demonstrate the loading of a goto-model from the command line arguments | ||
api.load_model_from_files(arguments); | ||
|
||
std::cout << "Successfully initialised goto_model" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we actually call bmc? Looks like we load the model, but then I don't see BMC being called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual call to the BMC will follow up later - I have some code for that but it had some wrinkles that I need to iron. Will probably come up as another PR.
int main(int argc, char *argv[]) | ||
{ | ||
int a[] = {0, 1, 2, 3, 4}; | ||
assert(a[4] != 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected to fail, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter - we want to make sure a particular string (that shows up if everything works wrt model loading) is present in the output.
cprover-cpp-api/src/api.cpp
Outdated
options(util_make_unique<optionst>(optionst{})) | ||
{ | ||
// Needed to initialise the language options correctly | ||
cmdlinet cmdline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the CBMC commandline? Doesn't that make a weird dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - hence why it's handled internally :)
cprover-cpp-api/src/api.h
Outdated
#include "options.h" | ||
|
||
// An object in the pattern of Session Facade - owning all of the memory | ||
// the API is using and being responsible for the management of that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this memory released? When the object drops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - this owns unique pointers to all of its dependencies, so as soon as the object gets out of scope all of the memory the object uses should be released.
cprover-cpp-api/src/api.h
Outdated
/// \param files: A vector<string> containing the filenames to be loaded | ||
/// \param options: An options object, to be passed on to analysis or | ||
/// transformation passes. | ||
void load_model_from_files(const std::vector<std::string> &files); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails, what happens? Throws an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - which bubbles up to the call stack.
The test driver we have is doing the exception handling at this point - we are in the process of researching how to best expose this to Rust.
b1f3f83
to
1cbf707
Compare
This also adds a regression testing binary driver under `regression/libcprover-cpp/call_bmc.cpp`, for testing the API. The library being built is `lib/libcprover-api-cpp.a`.
1cbf707
to
d0ab60c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm obviously too late to the party, but I'd please ask for some follow-up cleanup to happen.
@@ -215,6 +215,7 @@ cprover_default_properties( | |||
cbmc | |||
cbmc-lib | |||
cpp | |||
cprover-api-cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not match the directory name?
# TODO: Should be fixed for the proper include form. | ||
${CMAKE_CURRENT_SOURCE_DIR}/../src/libcprover-cpp/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this tracked/when will it be fixed?
macro(add_test_pl_profile name cmdline flag profile) | ||
add_test( | ||
NAME "${name}-${profile}" | ||
COMMAND perl ${test_pl_path} -e -p -c "${cmdline}" ${flag} ${ARGN} | ||
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" | ||
) | ||
set_tests_properties("${name}-${profile}" PROPERTIES | ||
LABELS "${profile};CBMC" | ||
) | ||
endmacro(add_test_pl_profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a new one and cannot re-use the existing one?
# For the best possible utilisation of multiple cores when | ||
# running tests in parallel, it is important that these directories are | ||
# listed with decreasing runtimes (i.e. longest running at the top) | ||
add_subdirectory(model_loading) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we actually get parallelisation within parallel runs here?
#include <string> | ||
#include <vector> | ||
|
||
extern configt config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are including config.h
, this is redundant.
class optionst; | ||
|
||
#include "options.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of these should be removed.
private: | ||
std::unique_ptr<goto_modelt> model; | ||
std::unique_ptr<message_handlert> message_handler; | ||
std::unique_ptr<optionst> options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you use PIMPL here: have a single pointer to the actual implementation, and completely hide internals like class message_handlert
from the API header file.
// Author: Fotis Koutoulakis for Diffblue Ltd. | ||
|
||
#include "options.h" | ||
|
||
#include <util/make_unique.h> | ||
#include <util/options.h> | ||
|
||
api_optionst api_optionst::create() | ||
{ | ||
return api_optionst{}; | ||
} | ||
|
||
api_optionst &api_optionst::simplify(bool on) | ||
{ | ||
simplify_enabled = on; | ||
return *this; | ||
} | ||
|
||
std::unique_ptr<optionst> api_optionst::to_engine_options() const | ||
{ | ||
optionst engine_options; | ||
engine_options.set_option("simplify", simplify_enabled); | ||
return util_make_unique<optionst>(engine_options); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this file please be called api_options.h to avoid confusion?
// Private interface methods | ||
api_optionst() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this comment is helpful?
public: | ||
static api_optionst create(); | ||
|
||
api_optionst &simplify(bool on); | ||
|
||
std::unique_ptr<optionst> to_engine_options() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strictly need comments. This is supposed to be an API to be used, after all.
@tautschnig Hi Michael, I've made a note of all of your comments and I will raise a PR with them addressed as soon as I can. |
This is the first iteration of a C++ CBMC API.
It includes:
api-binary-driver
, for use to validate this first iteration by exercising the API in the form of a regression test.src/libcprover-cpp
, which is built and exposed aslibcprover-api-cpp.a
by CMake and CPack.