This is an archive of the discontinued LLVM Phabricator instance.

[unittests] Split DWARF tests out of PDB, fix standalone build
ClosedPublic

Authored by mgorny on Nov 2 2016, 9:04 AM.

Details

Summary

Split the PDB tests into DWARF test and actual PDB tests, the latter
requiring DIA SDK. Use the new LLVMConfig.cmake LLVM_ENABLE_DIA_SDK
symbol to enable the PDB tests rather than relying on
llvm/Config/config.h private include file that is not available when
building standalone.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 76715.Nov 2 2016, 9:04 AM
mgorny retitled this revision from to [unittests] Avoid the dependency on private LLVM headers outside MSVC.
mgorny updated this object.
mgorny added reviewers: zturner, labath, k8stone.
mgorny added a subscriber: lldb-commits.
zturner edited edge metadata.Nov 2 2016, 9:09 AM

Is this necessary because of a standalone build, or for some other reason? If this is to get Standalone build working, then it seems like this isn't really a complete solution, because standalone build still wouldn't work on Windows since it would still be trying to include the private header. What about making a separate unittest file called SymbolFilePDBDIATests.cpp and excluding it at the CMake level?

mgorny added a comment.Nov 2 2016, 9:46 AM

Is this necessary because of a standalone build, or for some other reason? If this is to get Standalone build working, then it seems like this isn't really a complete solution, because standalone build still wouldn't work on Windows since it would still be trying to include the private header.

Yes, it is. However, since I don't have a Windows environment, I can't really test any Windows solution. With the very limited support for stand-alone builds right now, I see this as a feasible improvement to make tests work on Gentoo ;-).

What about making a separate unittest file called SymbolFilePDBDIATests.cpp and excluding it at the CMake level?

I don't mind doing that; however, it won't fix the issue in question. CMake doesn't know the state of HAVE_DIA either since it is available only during LLVM build. If we want to have Windows support there, we probably need to copy the check result to the public LLVMConfig.h.

What if we added a function to llvm/DebugInfo/PDB/PDB.h called isDiaSupported(), and in the implementation it returns true if the definition is present and false otherwise. Then instead of using the pre-processor to decide whether to run the test, we run all of them but just return true if isDiaSupported() returns false?

I can't say it wouldn't work but I think exposing that as part of public API would be ugly-ish. So I'd rather go for either exposing it in llvm-config.h, or LLVMConfig.cmake (and then redeclaring inside LLDB) -- with preference on the former as requiring least work.

That sounds reasonable.

mgorny updated this revision to Diff 76766.Nov 2 2016, 12:21 PM
mgorny retitled this revision from [unittests] Avoid the dependency on private LLVM headers outside MSVC to [unittests] Split DWARF tests out of PDB, fix standalone build.
mgorny updated this object.
mgorny edited edge metadata.

Here's an updated patch. I've decided to go for the initial idea of splitting the tests, combined with LLVMConfig update.

D26255 updates LLVM to include LLVM_ENABLE_DIA_SDK in LLVMConfig.cmake (it already provides similar variables). The variable is also still used to set HAVE_DIA_SDK in config.h, so old versions of LLDB will continue to build fine in-tree.

This one splits the previous test into DWARF and actual PDB tests. The DWARF test is run unconditionally, while the other is run only when DIA SDK is available. As I said, I don't have Windows handy, so I wasn't able to test the latter part. However, unless I failed at syntax when modifying it, it should work (I only removed the include, the DWARF test and file, and removed the DIA SDK magic).

zturner accepted this revision.Nov 4 2016, 10:43 PM
zturner edited edge metadata.

looks good, sorry for the delay was busy with the LLVM dev conference.

unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
33–36 ↗(On Diff #76766)

This can go away, along with all the other preprocessor definitions. (only for the dwarf-specific test file).

This revision is now accepted and ready to land.Nov 4 2016, 10:43 PM
mgorny updated this revision to Diff 82676.Dec 29 2016, 9:51 AM
mgorny edited edge metadata.
mgorny marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
lldb/trunk/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp