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
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.