[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Core/ModuleList.cpp:94
 
-  llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
-  SetClangModulesCachePath(path);
+  assert(!g_default_clang_modules_cache_path.empty());
+  SetClangModulesCachePath(g_default_clang_modules_cache_path);
----------------
zturner wrote:
> zturner wrote:
> > aprantl wrote:
> > > zturner wrote:
> > > > zturner wrote:
> > > > > zturner wrote:
> > > > > > aprantl wrote:
> > > > > > > zturner wrote:
> > > > > > > > aprantl wrote:
> > > > > > > > > zturner wrote:
> > > > > > > > > > I don't think this should be an assert.  After all, if the whole point is to make LLDB usable in situations where clang is not present, then someone using it in such an environment would probably never call the static function to begin with.  So I think we should just remove the assert and set it to whatever the value happens to be (including empty)
> > > > > > > > > The assertion enforces that ModuleListProperties::Initialize() has been called. If we want to make it more convenient, we can add a default argument `= "dummy"` for clients that don't link against clang.
> > > > > > > > I was actually thinking that instead of calling it `Initialize` (which sounds generic and like it's required) we would just call it `SetDefaultClangModulesCachePath` and have the user directly call that.  With a name like `Initialize`, it makes the user think that it's required, but in fact the only thing it's initializing is something that is optional, so it shouldn't be required.
> > > > > > > > 
> > > > > > > > It's possible I'm misunderstanding something though.
> > > > > > > My point was that this *is* required (for all clients of lldb that also link against clang). When LLDB initializes clang it must set a module cache path because clang doesn't implement a fallback.
> > > > > > If there's a client of LLDB using the public API and/or clang then that client would also be using `SystemInitializerFull` (or at the very least, would be responsible for initializing the set of things they need, one of which would be this path).
> > > > > > 
> > > > > > My point is that `Core` should ultimately have no knowledge that something called clang even exists, and it definitely shouldn't be limiting the use of itself based on the needs of a specific client since it something that is useful to all clients.  If a particular client requires clang, that client should initialize clang.
> > > > > > 
> > > > > > With an assert, this is requiring a non clang-based client to run some initialization code that is only required for a clang-based client, which doesn't seem like a reasonable restriction (imagine if every downstream developer using every possible set of random 3rd party libraries started asserting in low-level debugger code that their optional component had been initialized).
> > > > > In short, `Core` is too low level to be making any assumptions whatsoever about the needs of a particular client.  It may be required for all clients of lldb that use clang, but `Core` is not the right place to be making decisions based on whether a client of lldb uses clang (or any other optional external library / component).
> > > > To put this in perspective, imagine if LLVM's optimization pass library had something like `assert(driverIsClang());`
> > > The assertion is not supposed to check that Clang has been initialized. It is supposed to check that ModuleListProperties::Initialize() has been called. The fact that in order to call this function a client may want to get a string from the Clang Driver is an (ugly) implementation detail. And clients that don't use clang (such as the confusingly named unit tests) can pass in any nonempty string (which as I offered earlier could be made into a default argument).
> > But why must it even be a nonempty string?  And for that matter, if they're not going to use clang anyway, why even require the function to be called in the first place?  If it were an initialization function that did multiple things, it might be a stronger argument.  But as it stands, its only purpose is, in fact, to set a value for this path, which people who aren't using clang shouldn't be required to do.
> > 
> > This is making a decision in a low level library for the purpose of 1 specific client, which doesn't seem right.  I'm not entirely opposed to an assert, but it should only happen in clients that are using clang, otherwise this is effectively 'assert that the user has executed a no-op', which doesn't make sense.
> > I'm not entirely opposed to an assert, but it should only happen in clients that are using clang
> (and hence not in `Core` but in something higher level like ClangASTContext, or the place where you actually make use of this path).
For example, the only place this appears to be used is in `ClangModulesDeclVendor.cpp` line 595.  It looks like this:

```
    props.GetClangModulesCachePath().GetPath(path);
```

How about adding `assert(!path.empty());` after that?


https://reviews.llvm.org/D47235