This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Diff 25 - Fix test with shared libraries on Windows.
ClosedPublic

Authored by mpividori on Dec 16 2016, 4:12 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 81818.Dec 16 2016, 4:12 PM
mpividori retitled this revision from to [libFuzzer] Diff 25 - Fix test with shared libraries on Windows..
mpividori updated this object.
mpividori added reviewers: zturner, kcc.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
kcc added inline comments.Dec 16 2016, 4:16 PM
lib/Fuzzer/test/CMakeLists.txt
218

That's bad. Please try to find a way to avoid this.

zturner edited edge metadata.Dec 16 2016, 4:27 PM

If

lib/Fuzzer/test/CMakeLists.txt
218

I'm not sure I follow the logic here. Without this patch, where do the .lib and .dll file end up, and is the problem occuring during link time or runtime?

mpividori added inline comments.Dec 16 2016, 5:00 PM
lib/Fuzzer/test/CMakeLists.txt
218

I tried to find a better solution, but didn't succeed.
Without this patch, the .lib ends in: "<CMAKE_BINARY_DIR>/bin" (because the .dll is created there with the rule: CMAKE_CXX_CREATE_SHARED_LIBRARY).
But cmake tries to find it in: "<CMAKE_BINARY_DIR>/lib"
So the problem is at link time.

With this patch, I let cmake knows that the .lib file is in "<CMAKE_BINARY_DIR>/bin".

@zturner, The problem is that cmake will add "lib\LLVMFuzzer-DSO1.lib" to the linker, at it won't find it.

What's causing the .lib file to end up in the bin directory in the first place? We use DLLs in LLDB as well. we compile most of lldb into liblldb.dll, which produces liblldb.lib in the lib/ directory. Then when we link lldb.exe and specify the dependency to the dll's target, it automatically knows that it put the implib in the lib/ folder and links it from there.

Are we overriding that behavior in this CMake somewhere, because LLDB doesn't do anything special and by default the implib ends up in lib/. Is this perhaps a side effect of your earlier patch which overrides CMAKE_CXX_LINK_EXECUTABLE?

mpividori updated this revision to Diff 81841.Dec 17 2016, 1:44 AM
mpividori edited edge metadata.

@zturner I updated the code. I think this final solution is appropiate. Following cmake documentation:
+ I set ARCHIVE_OUTPUT_DIRECTORY as the destination of the import library.
+ I set RUNTIME_OUTPUT_DIRECTORY as the destination of the dll library.
Let me know if you agree.

But isn't the DLL now going into the lib directory too? I thought the DLL should go into the bin directory. This is how it works by default for clang and lldb. clang puts libclang.lib into lib/ and libclang.dll into bin/. Similarly for lldb. What are we doing differently that makes this not possible?

@zturner thanks for your comment.
If I don't add this changes, both the dll and the import library will be created in the bin directory.
I think this is probably happening because of the rule: "CMAKE_CXX_CREATE_SHARED_LIBRARY" that we define.

As previous definitions move the shared library to lib/Fuzzer/lib , I decided it would be reasonable to do the same with the dll and import library.

beanz, do you know anything about how this works? On Windows when you build a shared library we get .lib files in the lib/ directory and .dll files in the bin/ directory, and when linking against a DLL CMake automatically knows to look in the lib/ directory for the .lib file but still put the resulting exe into the bin directory.

Is there some magic that we need to know to make this happen? I'd like for everything in libFuzzer to work the same way as it does in clang and lldb.

beanz edited edge metadata.Dec 19 2016, 3:03 PM

In most of LLVM & Clang this is done inside llvm_add_library. Buried in there we have a call to set_output_directory, which sets everything up.

libFuzzer could add a call to set_output_directory, or it could implement its library add to use llvm_add_library instead of calling add_library directly.

mpividori updated this revision to Diff 82047.Dec 19 2016, 5:24 PM
mpividori edited edge metadata.

@beanz Thanks for that information.

@zturner This libraries are only created for a specific test. So, I think that is the reason why Kostya outputs it to "${CMAKE_BINARY_DIR}/lib/Fuzzer/lib" instead of the main "${CMAKE_BINARY_DIR}/lib" directory.

I modified the cmake file, so now we create the dll in the same folder that the test: "${CMAKE_BINARY_DIR}/lib/Fuzzer/test".
And the import library in: "${CMAKE_BINARY_DIR}/lib/Fuzzer/lib". Would you agree?

Thanks.

beanz added a comment.Dec 20 2016, 9:31 AM

BINARY_DIR is only used for the dynamic library on Windows, not on Unix. On Unix dynamic libraries go in the LIBRARY_DIR.

To make this patch work on Unix systems you would also need to add something like:

set_target_properties(LLVMFuzzer-DSO1 PROPERTIES
            BUILD_WITH_INSTALL_RPATH On
            INSTALL_RPATH "${CMAKE_BINARY_DIR}/lib/Fuzzer/lib")
set_target_properties(LLVMFuzzer-DSO2 PROPERTIES
            BUILD_WITH_INSTALL_RPATH On
            INSTALL_RPATH "${CMAKE_BINARY_DIR}/lib/Fuzzer/lib")

@beanz thanks for your comments.
Yes, I output the shared library in LIBRARY_DIR for Unix, just like previous implementation does. (output the shared library in: "${CMAKE_BINARY_DIR}/lib/Fuzzer/lib")
Surprisingly, the tests works on linux, the loader finds the shared library...

zturner accepted this revision.Dec 21 2016, 9:36 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.Dec 21 2016, 9:36 AM
kcc accepted this revision.Dec 27 2016, 11:07 AM
kcc edited edge metadata.

LGTM, assuming this does not change the behavior on linux

mpividori updated this revision to Diff 85210.Jan 20 2017, 4:00 PM

@kcc @zturner Again here. I was confused but the previous changes didn't fix this for Windows. This is the situation:

  • We need to set BINARY_DIR to: ${CMAKE_BINARY_DIR}/lib/Fuzzer/test , so the dll is placed in the same directory than the test LLVMFuzzer-DSOTest, and is found when executing that test.
  • As we are using CMAKE_CXX_CREATE_SHARED_LIBRARY to link the dll, we can't modify the output directory for the import library. It will be created in the same directory than the dll (in BINARY_DIR), no matter which value we set to LIBRARY_DIR. So, if we set LIBRARY_DIR to a different directory than BINARY_DIR, when linking LLVMFuzzer-DSOTest, cmake will look for the import library LLVMFuzzer-DSO1.lib in LIBRARY_DIR, and won't find it, since it was created in BINARY_DIR. So, for Windows, we need that LIBRARY_DIR and BINARY_DIR are the same directory.
mpividori requested review of this revision.Jan 21 2017, 6:11 PM
mpividori edited edge metadata.
zturner accepted this revision.Jan 21 2017, 6:13 PM
This revision is now accepted and ready to land.Jan 21 2017, 6:13 PM
This revision was automatically updated to reflect the committed changes.