search

secure-software-engineering / phasar

A LLVM-based static analysis framework.
Other
919 stars 140 forks source link

Allow LLVM beeing installed in a custom path #690

Closed flipreverse closed 5 months ago

flipreverse commented 6 months ago

Currently, the custom build of LLVM must be located in /usr/local/llvm-14. If not, CMake will either fail finding LLVMConfig.cmake or use a different LLVM installation, e.g., the system-wide one. This will, for example, result in an include path different from the custom LLVM installation. This commit extends the CMake scripts to force the user to specify the variable PHASAR_LLVM_INSTALL_DIR that points to the LLVM installation directory. This variable is also used to extend CMAKE_PREFIX_PATH which will make CMake use LLVMConfig.cmake from the custom build. (Fixed #689)

fabianbs96 commented 6 months ago

Hi @flipreverse,

as I already commented on #689, PhASAR's CMake Configuration should be able to automatically find the correct LLVM installation.

In addition, the bootstrap script is meant for the very first build of phasar only and it installs all system dependencies of phasar, as well as the correct LLVM version. If you use a custom LLVM that you have built (and installed) already, the bootstrap script is probably not, what you are looking for.

Furthermore, it is not a good idea to force users to pass an additional argument to the cmake command (PHASAR_LLVM_INSTALL_DIR). So, as of now, I am not in favor of merging this PR.

flipreverse commented 6 months ago

Hi @flipreverse,

as I already commented on #689, PhASAR's CMake Configuration should be able to automatically find the correct LLVM installation.

I can ensure you, it doesn't. It looks at system paths, e.g., /usr/lib/..., and at /usr/local/llvm-14/. There is currently no way to make CMake use a custom path. We, for example, want to install that LLVM custom build at /fs/proj/phasar . CMake won't detect that path unless explicitly specified. In addition, the bootstrap script is meant for the very first build of phasar only and it installs all system dependencies of phasar, as well as the correct LLVM version. If you use a custom LLVM that you have built (and installed) already, the bootstrap script is probably not, what you are looking for.

Furthermore, it is not a good idea to force users to pass an additional argument to the cmake command (PHASAR_LLVM_INSTALL_DIR). So, as of now, I am not in favor of merging this PR.

What do you think about an optional argument? PHASAR_LLVM_INSTALL_DIR defaults to /usr/local/llvm-14, and can be overwritten.

fabianbs96 commented 6 months ago

Hi @flipreverse, according to the cmake documentation for find_package you can customize the search procedure. Have you tried setting the cmake- or env variable LLVM_ROOT to your LLVM installation?

flipreverse commented 6 months ago

Hi @flipreverse, according to the cmake documentation for find_package you can customize the search procedure. Have you tried setting the cmake- or env variable LLVM_ROOT to your LLVM installation?

That did the trick. Thx! :) Now I get a compile error on my Debian Testing: use of undeclared identifier 'uint32_t' std::vector<uint32_t> SourceValues;. But that is a different issue.

Out of curiosity: Why are LLVM_INSTALL_DIR and PHASAR_INSTALL_DIR read-only variables? At least for PHASAR_INSTALL_DIR there is a cmdline switch.

What do you think about the following patch? Add a new cmdline switch to change the LLVM install dir. If used, bootstrap.sh will set LLVM_ROOT.

fabianbs96 commented 6 months ago

Hi @flipreverse, to answer your questions:

Out of curiosity: Why are LLVM_INSTALL_DIR and PHASAR_INSTALL_DIR read-only variables? At least for PHASAR_INSTALL_DIR there is a cmdline switch.

We simply did not have the use-case yet of installing LLVM to a custom location; probably PHASAR_INSTALL_DIR should not be readonly then anyway

What do you think about the following patch? Add a new cmdline switch to change the LLVM install dir. If used, bootstrap.sh will set LLVM_ROOT

This sounds reasonable

flipreverse commented 6 months ago

Hi @flipreverse, to answer your questions:

Out of curiosity: Why are LLVM_INSTALL_DIR and PHASAR_INSTALL_DIR read-only variables? At least for PHASAR_INSTALL_DIR there is a cmdline switch.

We simply did not have the use-case yet of installing LLVM to a custom location; probably PHASAR_INSTALL_DIR should not be readonly then anyway

What do you think about the following patch? Add a new cmdline switch to change the LLVM install dir. If used, bootstrap.sh will set LLVM_ROOT

This sounds reasonable

What is your opinion on the new commit?