search

PRUNERS / archer

Archer, a data race detection tool for large OpenMP applications
https://pruners.github.io/archer
Apache License 2.0
62 stars 13 forks source link

do not copy wrappers to llvm #8

Closed lee218llnl closed 7 years ago

lee218llnl commented 7 years ago

I don't think we can/should always assume that the llvm installation directory is writeable. It is probably also not wise to put the clang-archer wrappers in there at configure time anyway.

simoatze commented 7 years ago

@lee218llnl I think I put that in case we install ARCHER within Clang/LLVM otherwise when it runs the tests it won't find the archer wrapper. I forgot to put a check "if not STANDALONE_BUILD" or something like that. Does it make sense?

lee218llnl commented 7 years ago

Yeah, I thinks I can add that to the PR. Should this be an install command though as opposed to a file command? The file command is run at configure time.

simoatze commented 7 years ago

It should be a configure command because one can run the tests before installing. The variable to check should be "${ARCHER_STANDALONE_BUILD}".

lee218llnl commented 7 years ago

OK, let me know if my latest push is what you have in mind. I squashed a few commits into one. I'm still not sure if it makes sense to copy the wrappers at build time. What happens if someone runs cmake, but then fails to run ninja install? Should the tests run in the local build directory instead? I'm not sure what your test setup looks like or how to run it.

simoatze commented 7 years ago

The installation needs to be done anyway (that is done when you run ninja install). The copy has to be done only when ARCHER is build within Clang/LLVM, in this way if one runs cmake and not ninja install can still run the tests. The tests runs with ninja check-libarcher so it will look for the clang-archer wrapper in the build directory. So I think there should be only an if without else and the install needs to be done anyway.

lee218llnl commented 7 years ago

The tests runs with ninja check-libarcher so it will look for the clang-archer wrapper in the build directory

For the file(COPY), what should the DESTINATION be then? You originally had it as LLVM_TOOLS_BINARY_DIR, which I still think is a dangerous thing to do. If you make it local to the build directory and then have the ninja check-libarcher use the copy in the build directory, that would make more sense to me.

simoatze commented 7 years ago

Greg, sorry I was thinking that LLVM_TOOLS_BINARY_DIR was the build dir. You are right, I think the right folder is ${CMAKE_BUILD_DIRECTORY}/bin?

lee218llnl commented 7 years ago

Sorry, this is getting to be a bit more complicated than I initially hoped. I'm probing through the files some more and it looks like your tests are already designed to explicitly call clang -Xclang -load -Xclang LLVMArcher.so, so is it really necessary to even copy the clang-archer* build scripts to the build directory for testing? Thus far, I have only tested this with the standalone build, which does not need the copy.

simoatze commented 7 years ago

Then we can just remove that. Sorry, I forgot all the things I've done, I think in the tests I use that to avoid any problem. So let's just remove the copy to the build directory and leave only the install.

lee218llnl commented 7 years ago

OK, I think the attached is what we want then. Please merge if you agree.