This is an archive of the discontinued LLVM Phabricator instance.

Add modules support for lldb headers in include/
ClosedPublic

Authored by teemperor on Jun 7 2018, 11:27 PM.

Details

Summary

This patch adds a modulemap which allows compiling the lldb headers into C++ modules
(for example in builds with LLVM_ENABLE_MODULES=On).

Even though most of the affected code has been cleaned up to work with the more strict
C++ module semantics, there are still some workarounds left in the current modulemap
(the most obvious one is the big lldb wrapper module).

It also moves the Obj-C++ files in lldb to their own subdirectories. This was necessary
because we need to filter out the modules flags for this code.

Note: With the latest clang and libstdc++ it seems necessary to have a STL C++ module
to get a working LLVM_ENABLE_MODULES build for lldb. Otherwise clang will falsely
detect ODR violations in the textually included STL code inside the lldb modules.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Jun 7 2018, 11:27 PM

I generated a report for the remaining cyclic dependencies in the lldb module here.

Note that I'll provide a modulemap for the source/ headers in another patch, so this is out of scope for this review. The reason is that this modulemap seems to barely affect compilation time and is quite verbose (as I can't use umbrellas with the mixed source/header files in the same directory).

That's awesome! May I ask why you chose to use lldb_Component modules instead of doing submodules? Is this for build performance?

bruno added a comment.Jun 8 2018, 9:49 AM

Very nice!

I generated a report for the remaining cyclic dependencies in the lldb module here.

Do you plan to fix headers at some point to break out the dependencies?

include/lldb/module.modulemap
66 ↗(On Diff #150451)

Will this trick be enough for local submodules visibility mode as well?

@aprantl It's good for build times and more importantly it's consistent with the way Clang/LLVM are naming/organizing their modules. But I actually don't have a strong opinion on how the modules are named/organized.

@bruno Breaking up the lldb module in the future is the plan. I'm just not sure when I get around to do that. But the modulemap in the current state should bring most of the module benefits to LLDB (build time, better code checks), so I just opened a review for it.

Someone else with more LLDB experience could also break the cyclic dependencies. The report I linked should point out quite well what exactly the problematic includes are, so that person wouldn't even need to enable modules for that.

include/lldb/module.modulemap
66 ↗(On Diff #150451)

From my (very limited) experience with disabled local submodule visibility I would say yes, but I can't test it on my current system (compilation already fails at the first LLVM module without LSV).

teemperor updated this revision to Diff 150604.Jun 8 2018, 8:35 PM
  • Removed some inconsistent white space.
bruno accepted this revision.Jun 11 2018, 11:35 AM

LGTM

This revision is now accepted and ready to land.Jun 11 2018, 11:35 AM
teemperor planned changes to this revision.Jun 12 2018, 7:47 AM

It seems the compilation fails on OS X with this modulemap (or any modulemap). The reason seems to be that on OS X we compile Obj-C++ as part of lldb which assumes that Clang/LLVM is valid Obj-C++. However, the Obj-C parsing code in Clang is *not* valid Obj-C++ as some parts of the code use variables named like IBAction for describing, well, IBAction in Obj-C.

So we either try to disable modules for the Obj-C++ code or we make Clang's source code Obj-C++ compatible.

teemperor updated this revision to Diff 151004.Jun 12 2018, 1:04 PM
teemperor retitled this revision from Add modulemap to lldb include directory to Add modules support for lldb headers in include/.
teemperor edited the summary of this revision. (Show Details)
  • All Obj-C files are now in their own subdirectory. This way we can filter out the modules flags for them.
This revision is now accepted and ready to land.Jun 12 2018, 1:04 PM
teemperor added inline comments.Jun 12 2018, 1:05 PM
include/lldb/module.modulemap
66 ↗(On Diff #150451)

It seems to work!

aprantl accepted this revision.Jun 12 2018, 1:25 PM
aprantl added inline comments.
source/Host/CMakeLists.txt
7 ↗(On Diff #151004)

ObjC++ or Objective C++

107 ↗(On Diff #151004)

This is really Objective C++, so ObjCXX or objcxx would be a more appropriate directory name.

109 ↗(On Diff #151004)

macosx

(technically MacOS X is now macOS, but you don't need to change that)

teemperor updated this revision to Diff 151023.Jun 12 2018, 2:11 PM
  • Obj-C -> Obj-C++
teemperor updated this revision to Diff 151025.Jun 12 2018, 2:13 PM
teemperor marked an inline comment as done.
  • Fixed a typo.
aprantl added inline comments.Jun 12 2018, 2:44 PM
source/Host/CMakeLists.txt
8 ↗(On Diff #151025)

s/Obj-C++/Objective C++/

108 ↗(On Diff #151025)

either ...objcxx or ...ObjCXX

7 ↗(On Diff #151004)

s/Obj-C++/Objective C++/

teemperor updated this revision to Diff 151077.Jun 12 2018, 5:54 PM
  • The regex that removes the modules cache now actually removes the whole flag + path.
  • Fixed more typos.
teemperor marked 6 inline comments as done.Jun 12 2018, 5:54 PM
teemperor updated this revision to Diff 151181.Jun 13 2018, 8:51 AM
  • Finally found the one and only way to spell "Objective-C++"
This revision was automatically updated to reflect the committed changes.

I'm getting a new warning now, can you also reproduce this?

In file included from <module-includes>:21:
In file included from ../tools/lldb/include/lldb/Host/MainLoop.h:13:
tools/lldb/include/lldb/Host/Config.h:33:9: warning: 'HAVE_LIBCOMPRESSION' macro redefined [-Wmacro-redefined]
#define HAVE_LIBCOMPRESSION
        ^
<command line>:1:9: note: previous definition is here
#define HAVE_LIBCOMPRESSION 1
        ^
1 warning generated.
lldb/trunk/source/Plugins/Platform/MacOSX/CMakeLists.txt