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

VR: use fixed and refactored version #65

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

GiraffeReversed
Copy link
Contributor

Known issues:

  • due to splitting the analysis source files to .h and .cpp, I changed the way the resulting libraries are linked to the ValueRelationsPlugin and also to the new vr_tests; the way is probably not the desired one, since it requires the variable CMAKE_INSTALL_LIBDIR to be set with the path to the dg install directory (which probably misuses the CMake in several ways that I know little about)

Related PR to dg:
mchalupa/dg#403

@GiraffeReversed GiraffeReversed changed the title VR: use fixed and refactored versionDevelopment VR: use fixed and refactored version Oct 16, 2021
lzaoral
lzaoral previously requested changes Nov 7, 2021
Copy link
Contributor

@lzaoral lzaoral left a comment

Choose a reason for hiding this comment

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

Remarks:

  • Some files do not end with a newline.
  • Format the changes files using the clang-format config file from dg.

analyses/value_relations_plugin.cpp Outdated Show resolved Hide resolved
tests/tests.cpp Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 23 to 24
../src/jsoncpp.cpp
${CMAKE_SOURCE_DIR}/analyses/value_relations_plugin.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ../src/jsoncpp.cpp -- What if jsoncpp installed by a package manager is used?
  • ${CMAKE_SOURCE_DIR}/analyses/value_relations_plugin.cpp - Link with the corresponding plug-in so that the code that's actually going to be used by the instrumenter is tested.

Copy link
Contributor Author

@GiraffeReversed GiraffeReversed Nov 10, 2021

Choose a reason for hiding this comment

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

I was not able to test the fix as the libjsoncpp-dev on Ubuntu 20.04 contains version 1.7.4, but at least 1.9.2 is required (or at least I hope I interpret the problem correctly).

As for linking ValueRelationsPlugin directly, when I tried the naive solution of dropping the value_relations_plugin.cpp and linking ValueRelationsPlugin as library instead, I got the following error:

CMake Error at tests/CMakeLists.txt:39 (target_link_libraries):
Target "ValueRelationsPlugin" of type MODULE_LIBRARY may not be linked into
another target. One may link only to INTERFACE, OBJECT, STATIC or SHARED
libraries, or to executables with the ENABLE_EXPORTS property set.

I do not feel like it's my call to change the ValueRelationsPlugin library's type and no attempt to set ENABLE_EXPORTS helped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sorry. I forgot that the plug-ins are not meant to be directly linked to other targets. You can re-use the libdl-like machinery of Analyzer::analyse (OT: This is a really deceitful name for a method that just loads and initializes a dynamic library on-demand...):

unique_ptr<InstrPlugin> Analyzer::analyze(const string &path,
llvm::Module* module)
{
if (path.empty())
return nullptr;
string err;
auto DL = llvm::sys::DynamicLibrary::getPermanentLibrary(path.c_str(),
&err);
if (!DL.isValid()) {
cerr << "Cannot open library: " << path << "\n";
cerr << err << endl;
return nullptr;
}
InstrPlugin* (*create)(llvm::Module*);
void *symbol = DL.getAddressOfSymbol("create_object");
if (!symbol) {
cerr << "Cannot load symbol 'create_object' from " << path << endl;
return nullptr;
}
create = reinterpret_cast<InstrPlugin *(*)(llvm::Module*)>(symbol);
unique_ptr<InstrPlugin> plugin(create(module));
return plugin;
}

tests/tested-files.json Outdated Show resolved Hide resolved
tests/tests.cpp Outdated
std::string targetFile = "tmp/" + benchmark + ".ll";

if (! fileExists(targetFile)) {
std::string command = "clang-10 -S -emit-llvm " + path + "/" + benchmark + ".i -o " + targetFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

Clang version should be configured by the build system to use version compatible with LLVM that this project was compiled with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I solved this, but the solution feels pretty hacked.

Copy link
Member

@mchalupa mchalupa left a comment

Choose a reason for hiding this comment

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

I haven't went through the commits, but I have tested them repeatedly (and they ran in SV-COMP'22), so once all cosmetic issues are solved, I think they are good to be merged.

Copy link
Contributor

@trtikm trtikm left a comment

Choose a reason for hiding this comment

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

This is quite big PR. I will need some assistance to fully understand the changes.

@lzaoral lzaoral dismissed their stale review May 19, 2023 08:51

Dismissing because I'm no longer associated with this project in any way for few last months..

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.

None yet

4 participants