Make LLDB's clang module cache path customizable
ClosedPublic

Authored by aprantl on Feb 8 2018, 4:04 PM.

Details

Summary

This patch makes LLDB's clang module cache path customizable via settings set target.clang-modules-cache-path <path> and uses it in the LLDB testsuite to reuse the same location inside the build directory for LLDB and clang.

Diff Detail

Repository
rL LLVM
aprantl created this revision.Feb 8 2018, 4:04 PM
jingham requested changes to this revision.Feb 8 2018, 5:03 PM

Think about whether it would be better to have GetClangModulesCachePath calculate the fallback modules path rather than having the client do it?

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
595–601 ↗(On Diff #133527)

Is there a reason not to have GetClangModulesCachePath do this? This is roughly the "default value" of the value. Is there a good reason to make clients compute that?

I presume you are computing this here because clang doesn't offer an API to do that?

This revision now requires changes to proceed.Feb 8 2018, 5:03 PM
aprantl updated this revision to Diff 133636.Feb 9 2018, 9:38 AM

Address most review feedback.

aprantl added inline comments.
source/Plugins/ExpressionParser/Clang/CMakeLists.txt
26 ↗(On Diff #133636)

I checked and this does not affects LLDB's binary size in any measurable way.

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
595–601 ↗(On Diff #133527)

Is there a reason not to have GetClangModulesCachePath do this?

Yes. It is supposed to return the value of the *property*. Also, I assume that Target doesn't necessarily link against Clang, so calling into a Clang API there seems to be a layering violation.

davide accepted this revision.Feb 9 2018, 9:48 AM
davide added a subscriber: davide.

LGTM modulo minor.

source/Plugins/ExpressionParser/Clang/CMakeLists.txt
26 ↗(On Diff #133636)

This doesn't surprise me, as the driver is very small.

source/Target/Target.cpp
3949 ↗(On Diff #133636)

add an assertion message if you don't mind?

jingham accepted this revision.Feb 9 2018, 10:01 AM

This looks fine to me.

This revision is now accepted and ready to land.Feb 9 2018, 10:01 AM
aprantl added inline comments.Feb 9 2018, 10:02 AM
source/Target/Target.cpp
3949 ↗(On Diff #133636)

This code is mechanically duplicated from the other functions in this file, and I was actually planning on making another pass that auto-generates all of the properties and the accessor functions from a .inc file using an llvm-style HANDLE_foo() macro.

davide added inline comments.Feb 9 2018, 10:05 AM
source/Target/Target.cpp
3949 ↗(On Diff #133636)

Sure, even better :) It would be awesome if lldb didn't do its own command line handling but would use something like cl::opt, but I guess that's a battle for another day.

I don't think there's any battle. I wouldn't mind moving the command parsing code into llvm if there's a suitable replacement.

But so far as I can see llvm's command line parsing was primarily intended for parsing args for shell tools. So it lacks features required for a replacement to lldb's command line parser.

The lldb interactive command interpreter absolutely needs completion callbacks. Nobody would be happy with lldb if it didn't complete filenames and symbols and the commands themselves. We also do shortest match rather than exact matches which folks find very handy. And you have to be able to provide our alias system which is also very heavily used. If somebody wants to add all these features to the llvm command line parser, then we could use it in lldb. I'm not sure that that would be a great idea, it might be seen to overly complicate the shell tool only parser.

Anyway, I don't see a drop-in replacement in llvm.

It's probably possible to make it work, but as Jim said, there's no drop in replacement currently. There's bits and pieces of stuff that, with a dedicated effort, could be improved to the point of being sufficient, though.

To me, the determinant here is whether the llvm project sees many more interactive tools in its future. If lldb is the only one, then moving all this functionality to llvm seems like effort better spent elsewhere. But if there are other interactive tools in the offing, it would be great to have all such tools have a common behavior, and so there's good motivation for doing this work.

This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.Feb 12 2018, 2:31 AM

There are several components in the "command line parsing"

  1. argparse: parsing a single string into the individual words (processing quotes, backslashes, etc.)
  2. resolving the "command" part of the command line (the "frame variable" part of "frame variable --raw-output FOO")
  3. resolving the arguments of a command (the "--raw-output FOO" part)

These parts compose quite nicely, and can be individually replaced. I'll briefly speak about each one :

(1) is very specific to our interactive use case (for command line utilities, the shell/os does the parsing for us). There also isn't anything nearly similar in llvm (afaik).
(2) is also quite specific to how lldb commands are structured, and there also isn't a suitable llvm replacement. This is the level at which the "command aliases" are resolved.
(3) is a fairly generic piece of functionality -- we even implement it by delegating to getopt (in a scarily un-thread-safe way). For this there are *two* competing implementations in llvm: llvm::cl and llvm::opt. Of these the first one is not suitable for our use case right now as it relies on global variables. Making it work probably would be a bit or work. However, using the second one should be fairly easy, and it should be generic-enough (it's the thing that handles crazy gcc command lines).

The reason I'm spelling it out this way is because I think you were mostly talking about (3), but some of the difficulties Jim pointed out (command aliases) just aren't relevant there. I don't think llvm::opt handles "shortest match" options yet, but that should be fairly easy to add. On the flip side, the library has recently gained argument suggestion capabilities (error: unknown argument '-gllbb', did you mean '-glldb'), so we would get that for free. Making sure tab completion works there would require a bit of thought, but I think this should not interfere with this library too much (after all, we use getopt now, and it does not support tab completion either).