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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

That sounds good to me as well.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Moving the assertion into `ClangModulesDeclVendor` seems like a good idea to 
me. If that's the only place that actually requires the value to be non-empty, 
then it should be the thing asserting it (we can put a helpful message in the 
assert statement telling the user what he needs to do). But, we need to be 
careful there to make sure that the user cannot trip that assertion by manually 
setting the value of the setting to "".

**However**, what would be an even better idea in my mind is to completely move 
the setting into the ClangExpressionParser plugin. I don't want to sound like a 
broken record, but I feel like noone has responded to that proposal of mine, 
positively or negatively. I'll try to elaborate on it more.

I think this could be modelled the same way as SymbolFileDWARF injects the 
`plugin.symbol-file.dwarf.comp-dir-symlink-paths` setting. The flow there is:

- SystemInitializerFull calls SymbolFileDWARF::Initialize
- SymbolFileDWARF::Initialize calls PluginManager::RegisterPlugin, passing it a 
DebuggerInitialize function
- when a Debugger object is created, it calls PluginManager::DebuggerInitialize 
 (from Debugger::InstanceInitialize)
- PluginManager::DebuggerInitialize calls SymbolFileDWARF::Initialize, passing 
it the debugger instance
- SymbolFileDWARF::Initialize calls 
PluginManager::CreateSettingForSymbolFilePlugin which creates the actual setting
- finally, when SymbolFileDWARF wants to read the setting it calls 
SymbolFileDWARF::GetGlobalPluginProperties()->GetSymLinkPaths()

this is all very enterprise-y, but it allows us to keep the knowledge of the 
comp-dir-symlink-paths setting (even it's existence) completely hidden inside 
the plugin, which I think is what this discussion was all about in the first 
place.

So my question is, is there a reason a flow like this would not work for this 
setting as well?


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via lldb-commits]

> On May 23, 2018, at 1:33 PM, Zachary Turner  wrote:
> 
> Actually, now I’m starting to wonder why can’t ClangModulesDeclVendor.cpp 
> just call this function in clangDriver to get the default if the accessor 
> returns an empty string? That sidesteps all of this initialization funny 
> business and lets the client just handle it.

Since it is surfaced as a property, users expect that "settings list" returns a 
proper value. ClangModulesDeclVendor is only initialized once a Target exists. 
Xcode may also use this property to set the module cache path and initializing 
the property later than at LLDB initialization time could create a race 
condition.
ClangModulesDeclVendor also isn't the only user of the property. The Swift 
Plugin also needs to know it and I'd like to avoid duplicating the code.


-- adrian


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Actually, now I’m starting to wonder why can’t
ClangModulesDeclVendor.cpp just call this function in clangDriver to get
the default if the accessor returns an empty string? That sidesteps all of
this initialization funny business and lets the client just handle it.

Would that work?


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

Actually, now I’m starting to wonder why can’t
ClangModulesDeclVendor.cpp just call this function in clangDriver to get
the default if the accessor returns an empty string? That sidesteps all of
this initialization funny business and lets the client just handle it.

Would that work?
On Wed, May 23, 2018 at 1:25 PM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> 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 

[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 

[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:
> 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 

[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);

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.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl 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:
> > 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).


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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:
> > > > 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());`


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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:
> 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).


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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);

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


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via Phabricator via lldb-commits]

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


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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);

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.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via Phabricator via lldb-commits]

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


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 148265.
aprantl added a comment.

Remove unused include.


https://reviews.llvm.org/D47235

Files:
  include/lldb/API/SystemInitializerFull.h
  include/lldb/Core/ModuleList.h
  source/API/SystemInitializerFull.cpp
  source/Core/ModuleList.cpp
  tools/lldb-test/CMakeLists.txt
  tools/lldb-test/lldb-test.cpp
  unittests/Host/SymbolsTest.cpp
  unittests/Target/ModuleCacheTest.cpp

Index: unittests/Target/ModuleCacheTest.cpp
===
--- unittests/Target/ModuleCacheTest.cpp
+++ unittests/Target/ModuleCacheTest.cpp
@@ -5,7 +5,9 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/SymbolContext.h"
@@ -67,6 +69,7 @@
 void ModuleCacheTest::SetUpTestCase() {
   HostInfo::Initialize();
   ObjectFileELF::Initialize();
+  ModuleListProperties::Initialize("dummy");
 
   FileSpec tmpdir_spec;
   HostInfo::GetLLDBPath(lldb::ePathTypeLLDBTempSystemDir, s_cache_dir);
Index: unittests/Host/SymbolsTest.cpp
===
--- unittests/Host/SymbolsTest.cpp
+++ unittests/Host/SymbolsTest.cpp
@@ -9,20 +9,24 @@
 
 #include "gtest/gtest.h"
 
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 
 using namespace lldb_private;
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   FileSpec symbol_file_spec = Symbols::LocateExecutableSymbolFile(module_spec);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
 }
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   // using a GUID here because the symbol file shouldn't actually exist on disk
   module_spec.GetSymbolFileSpec().SetFile(
Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -34,6 +34,9 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
+
+#include "clang/Driver/Driver.h"
+
 #include 
 
 using namespace lldb;
@@ -467,6 +470,11 @@
 
   cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n");
 
+  // Retrieve the default modulecache path from clang.
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  ModuleListProperties::Initialize(path);
+
   SystemLifetimeManager DebuggerLifetime;
   DebuggerLifetime.Initialize(llvm::make_unique(),
   nullptr);
Index: tools/lldb-test/CMakeLists.txt
===
--- tools/lldb-test/CMakeLists.txt
+++ tools/lldb-test/CMakeLists.txt
@@ -19,6 +19,7 @@
 lldbUtility
 ${LLDB_ALL_PLUGINS}
 ${host_lib}
+clangDriver
 
   LINK_COMPONENTS
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -34,7 +34,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for shared_ptr
@@ -79,15 +78,20 @@
 
 enum { ePropertyEnableExternalLookup, ePropertyClangModulesCachePath };
 
+std::string g_default_clang_modules_cache_path;
 } // namespace
 
+void ModuleListProperties::Initialize(
+llvm::StringRef clang_modules_cache_path) {
+  g_default_clang_modules_cache_path = clang_modules_cache_path.str();
+}
+
 ModuleListProperties::ModuleListProperties() {
   m_collection_sp.reset(new OptionValueProperties(ConstString("symbols")));
   m_collection_sp->Initialize(g_properties);
 
-  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);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
Index: source/API/SystemInitializerFull.cpp
===
--- source/API/SystemInitializerFull.cpp
+++ source/API/SystemInitializerFull.cpp
@@ -119,6 +119,7 @@
 #endif
 
 #include "llvm/Support/TargetSelect.h"
+#include "clang/Driver/Driver.h"
 
 #include 
 
@@ -385,6 +386,13 @@
   // AFTER PluginManager::Initialize is called.
 
   Debugger::SettingsInitialize();
+  InitializeClang();
+}
+
+void 

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 148264.
aprantl added a comment.

Remove whitespace changes.


https://reviews.llvm.org/D47235

Files:
  include/lldb/API/SystemInitializerFull.h
  include/lldb/Core/ModuleList.h
  source/API/SystemInitializerFull.cpp
  source/Core/ModuleList.cpp
  tools/lldb-test/CMakeLists.txt
  tools/lldb-test/lldb-test.cpp
  unittests/Host/SymbolsTest.cpp
  unittests/Target/ModuleCacheTest.cpp

Index: unittests/Target/ModuleCacheTest.cpp
===
--- unittests/Target/ModuleCacheTest.cpp
+++ unittests/Target/ModuleCacheTest.cpp
@@ -5,7 +5,9 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/SymbolContext.h"
@@ -67,6 +69,7 @@
 void ModuleCacheTest::SetUpTestCase() {
   HostInfo::Initialize();
   ObjectFileELF::Initialize();
+  ModuleListProperties::Initialize("dummy");
 
   FileSpec tmpdir_spec;
   HostInfo::GetLLDBPath(lldb::ePathTypeLLDBTempSystemDir, s_cache_dir);
Index: unittests/Host/SymbolsTest.cpp
===
--- unittests/Host/SymbolsTest.cpp
+++ unittests/Host/SymbolsTest.cpp
@@ -9,20 +9,24 @@
 
 #include "gtest/gtest.h"
 
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 
 using namespace lldb_private;
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   FileSpec symbol_file_spec = Symbols::LocateExecutableSymbolFile(module_spec);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
 }
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   // using a GUID here because the symbol file shouldn't actually exist on disk
   module_spec.GetSymbolFileSpec().SetFile(
Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -34,6 +34,9 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
+
+#include "clang/Driver/Driver.h"
+
 #include 
 
 using namespace lldb;
@@ -467,6 +470,11 @@
 
   cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n");
 
+  // Retrieve the default modulecache path from clang.
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  ModuleListProperties::Initialize(path);
+
   SystemLifetimeManager DebuggerLifetime;
   DebuggerLifetime.Initialize(llvm::make_unique(),
   nullptr);
Index: tools/lldb-test/CMakeLists.txt
===
--- tools/lldb-test/CMakeLists.txt
+++ tools/lldb-test/CMakeLists.txt
@@ -19,6 +19,7 @@
 lldbUtility
 ${LLDB_ALL_PLUGINS}
 ${host_lib}
+clangDriver
 
   LINK_COMPONENTS
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/Property.h"
@@ -34,7 +35,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for shared_ptr
@@ -79,15 +79,20 @@
 
 enum { ePropertyEnableExternalLookup, ePropertyClangModulesCachePath };
 
+std::string g_default_clang_modules_cache_path;
 } // namespace
 
+void ModuleListProperties::Initialize(
+llvm::StringRef clang_modules_cache_path) {
+  g_default_clang_modules_cache_path = clang_modules_cache_path.str();
+}
+
 ModuleListProperties::ModuleListProperties() {
   m_collection_sp.reset(new OptionValueProperties(ConstString("symbols")));
   m_collection_sp->Initialize(g_properties);
 
-  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);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
Index: source/API/SystemInitializerFull.cpp
===
--- 

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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Can you revert the changes to `HostInfoBase.h`?  It looks like now it's only 
whitespace changes.




Comment at: source/Core/ModuleList.cpp:16
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"

I think we don't need this include anymore.



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);

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)


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 148248.
aprantl added a comment.

Forgot to update unit tests.


https://reviews.llvm.org/D47235

Files:
  include/lldb/API/SystemInitializerFull.h
  include/lldb/Core/ModuleList.h
  include/lldb/Host/HostInfoBase.h
  source/API/SystemInitializerFull.cpp
  source/Core/ModuleList.cpp
  tools/lldb-test/CMakeLists.txt
  tools/lldb-test/lldb-test.cpp
  unittests/Host/SymbolsTest.cpp
  unittests/Target/ModuleCacheTest.cpp

Index: unittests/Target/ModuleCacheTest.cpp
===
--- unittests/Target/ModuleCacheTest.cpp
+++ unittests/Target/ModuleCacheTest.cpp
@@ -5,7 +5,9 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/SymbolContext.h"
@@ -67,6 +69,7 @@
 void ModuleCacheTest::SetUpTestCase() {
   HostInfo::Initialize();
   ObjectFileELF::Initialize();
+  ModuleListProperties::Initialize("dummy");
 
   FileSpec tmpdir_spec;
   HostInfo::GetLLDBPath(lldb::ePathTypeLLDBTempSystemDir, s_cache_dir);
Index: unittests/Host/SymbolsTest.cpp
===
--- unittests/Host/SymbolsTest.cpp
+++ unittests/Host/SymbolsTest.cpp
@@ -9,20 +9,24 @@
 
 #include "gtest/gtest.h"
 
+#include "lldb/API/SystemInitializerFull.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 
 using namespace lldb_private;
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   FileSpec symbol_file_spec = Symbols::LocateExecutableSymbolFile(module_spec);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
 }
 
 TEST(SymbolsTest,
  LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile) {
+  ModuleListProperties::Initialize("dummy");
   ModuleSpec module_spec;
   // using a GUID here because the symbol file shouldn't actually exist on disk
   module_spec.GetSymbolFileSpec().SetFile(
Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -34,6 +34,9 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
+
+#include "clang/Driver/Driver.h"
+
 #include 
 
 using namespace lldb;
@@ -467,6 +470,11 @@
 
   cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n");
 
+  // Retrieve the default modulecache path from clang.
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  ModuleListProperties::Initialize(path);
+
   SystemLifetimeManager DebuggerLifetime;
   DebuggerLifetime.Initialize(llvm::make_unique(),
   nullptr);
Index: tools/lldb-test/CMakeLists.txt
===
--- tools/lldb-test/CMakeLists.txt
+++ tools/lldb-test/CMakeLists.txt
@@ -19,6 +19,7 @@
 lldbUtility
 ${LLDB_ALL_PLUGINS}
 ${host_lib}
+clangDriver
 
   LINK_COMPONENTS
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/Property.h"
@@ -34,7 +35,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for shared_ptr
@@ -79,15 +79,20 @@
 
 enum { ePropertyEnableExternalLookup, ePropertyClangModulesCachePath };
 
+std::string g_default_clang_modules_cache_path;
 } // namespace
 
+void ModuleListProperties::Initialize(
+llvm::StringRef clang_modules_cache_path) {
+  g_default_clang_modules_cache_path = clang_modules_cache_path.str();
+}
+
 ModuleListProperties::ModuleListProperties() {
   m_collection_sp.reset(new OptionValueProperties(ConstString("symbols")));
   m_collection_sp->Initialize(g_properties);
 
-  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);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
Index: source/API/SystemInitializerFull.cpp
===

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 148241.
aprantl added a comment.

Updated patch according to Zachary's suggestion.


https://reviews.llvm.org/D47235

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Host/HostInfoBase.h
  source/API/SystemInitializerFull.cpp
  source/Core/ModuleList.cpp
  source/Host/CMakeLists.txt
  tools/lldb-test/CMakeLists.txt
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -34,6 +34,9 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
+
+#include "clang/Driver/Driver.h"
+
 #include 
 
 using namespace lldb;
@@ -467,6 +470,11 @@
 
   cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n");
 
+  // Retrieve the default modulecache path from clang.
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  ModuleListProperties::Initialize(path);
+
   SystemLifetimeManager DebuggerLifetime;
   DebuggerLifetime.Initialize(llvm::make_unique(),
   nullptr);
Index: tools/lldb-test/CMakeLists.txt
===
--- tools/lldb-test/CMakeLists.txt
+++ tools/lldb-test/CMakeLists.txt
@@ -19,6 +19,7 @@
 lldbUtility
 ${LLDB_ALL_PLUGINS}
 ${host_lib}
+clangDriver
 
   LINK_COMPONENTS
 Support
Index: source/Host/CMakeLists.txt
===
--- source/Host/CMakeLists.txt
+++ source/Host/CMakeLists.txt
@@ -185,7 +185,7 @@
 lldbUtility
 ${LLDB_PLUGINS}
 ${EXTRA_LIBS}
-  
+
   LINK_COMPONENTS
 Object
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/Property.h"
@@ -34,7 +35,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for shared_ptr
@@ -79,15 +79,20 @@
 
 enum { ePropertyEnableExternalLookup, ePropertyClangModulesCachePath };
 
+std::string g_default_clang_modules_cache_path;
 } // namespace
 
+void ModuleListProperties::Initialize(
+llvm::StringRef clang_modules_cache_path) {
+  g_default_clang_modules_cache_path = clang_modules_cache_path.str();
+}
+
 ModuleListProperties::ModuleListProperties() {
   m_collection_sp.reset(new OptionValueProperties(ConstString("symbols")));
   m_collection_sp->Initialize(g_properties);
 
-  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);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
Index: source/API/SystemInitializerFull.cpp
===
--- source/API/SystemInitializerFull.cpp
+++ source/API/SystemInitializerFull.cpp
@@ -119,6 +119,7 @@
 #endif
 
 #include "llvm/Support/TargetSelect.h"
+#include "clang/Driver/Driver.h"
 
 #include 
 
@@ -385,6 +386,11 @@
   // AFTER PluginManager::Initialize is called.
 
   Debugger::SettingsInitialize();
+
+  // Retrieve the default modulecache path from clang.
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  ModuleListProperties::Initialize(path);
 }
 
 void SystemInitializerFull::InitializeSWIG() {
Index: include/lldb/Host/HostInfoBase.h
===
--- include/lldb/Host/HostInfoBase.h
+++ include/lldb/Host/HostInfoBase.h
@@ -89,7 +89,7 @@
   /// ArchSpec object.
   //---
   static ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
-
+  
 protected:
   static bool ComputeSharedLibraryDirectory(FileSpec _spec);
   static bool ComputeSupportExeDirectory(FileSpec _spec);
Index: include/lldb/Core/ModuleList.h
===
--- include/lldb/Core/ModuleList.h
+++ include/lldb/Core/ModuleList.h
@@ -79,6 +79,10 @@
 public:
   ModuleListProperties();
 
+  /// Set the default clang modules cache path.
+  /// This avoids Core depending on Clang.
+  static void Initialize(llvm::StringRef clang_modules_cache_path);
+
   FileSpec GetClangModulesCachePath() const;
   bool SetClangModulesCachePath(llvm::StringRef 

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

[Zachary Turner via lldb-commits]

SystemInitializerFull.cpp, in the
function SystemInitializerFull::Initialize().

On Wed, May 23, 2018 at 9:44 AM Adrian Prantl  wrote:

>
>
> On May 23, 2018, at 8:51 AM, Zachary Turner  wrote:
>
> There's not really a diagram, because we don't have an exact vision of
> what the final layering is going to look like (some things will need to be
> split up, entirely new targets will need to be introduced, etc).  Mostly
> it's just built from experience based on what the primary logical function
> of a target is, and then asking whether or not someone who wishes to use
> that functionality should be required to link in all of that target's
> current dependencies.  And hoping that a layering emerges from this
> process, at which point we can then make a diagram as well as enforce it
> through CMake / LLVMBuild / etc.
>
> Core is the fundamental target that contains the basic data structures
> representing a generic debugger.  A generic debugger shouldn't need to
> depend on clang.  Someone should be able to (in theory) link against Core
> (and perhaps a few other targets) and build a Java debugger.  Or a debugger
> for an embedded platform.  So a dependency on clang doesn't belong there.
>
> LLDB is a specific debugger which is built on top of Core and other
> libraries, and so that is why I think it makes sense to expose a static
> function in Module which allows you to set the clang modules path, and then
> we do that during LLDB initialization.
>
>
> Could you point me at a specific source file? It's not obvious to me what
> "LLDB initialization" means to you.
>
> -- adrian
>
>   For the record, that still isn't correct from a purity standpoint,
> because there is a method in Core which claims to have something to do with
> clang modules.  But at least it's relatively benign.
>
> In any case, what do you think about that approach?
>
> On Wed, May 23, 2018 at 8:42 AM Adrian Prantl  wrote:
>
>>
>>
>> > On May 22, 2018, at 6:57 PM, Zachary Turner  wrote:
>> >
>> > Yea I don’t think this addresses the problem. We should be able to link
>> against parts of lldb without a dependency on clang. Since this is about
>> configuring something related to clang, it seems like it should be isolated
>> to some part of lldb that interfaces with clang
>>
>> And this is the point where I need some help :-)
>> The intended layering of LLDB is not at all clear to me. Which LLDB
>> libraries are allowed to link against clang? Do we have a diagram somewhere?
>>
>> -- adrian
>>
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via lldb-commits]

> On May 23, 2018, at 8:51 AM, Zachary Turner  wrote:
> 
> There's not really a diagram, because we don't have an exact vision of what 
> the final layering is going to look like (some things will need to be split 
> up, entirely new targets will need to be introduced, etc).  Mostly it's just 
> built from experience based on what the primary logical function of a target 
> is, and then asking whether or not someone who wishes to use that 
> functionality should be required to link in all of that target's current 
> dependencies.  And hoping that a layering emerges from this process, at which 
> point we can then make a diagram as well as enforce it through CMake / 
> LLVMBuild / etc.
> 
> Core is the fundamental target that contains the basic data structures 
> representing a generic debugger.  A generic debugger shouldn't need to depend 
> on clang.  Someone should be able to (in theory) link against Core (and 
> perhaps a few other targets) and build a Java debugger.  Or a debugger for an 
> embedded platform.  So a dependency on clang doesn't belong there.
> 
> LLDB is a specific debugger which is built on top of Core and other 
> libraries, and so that is why I think it makes sense to expose a static 
> function in Module which allows you to set the clang modules path, and then 
> we do that during LLDB initialization.

Could you point me at a specific source file? It's not obvious to me what "LLDB 
initialization" means to you.

-- adrian

>   For the record, that still isn't correct from a purity standpoint, because 
> there is a method in Core which claims to have something to do with clang 
> modules.  But at least it's relatively benign.
> 
> In any case, what do you think about that approach?  
> 
> On Wed, May 23, 2018 at 8:42 AM Adrian Prantl  > wrote:
> 
> 
> > On May 22, 2018, at 6:57 PM, Zachary Turner  > > wrote:
> > 
> > Yea I don’t think this addresses the problem. We should be able to link 
> > against parts of lldb without a dependency on clang. Since this is about 
> > configuring something related to clang, it seems like it should be isolated 
> > to some part of lldb that interfaces with clang 
> 
> And this is the point where I need some help :-)
> The intended layering of LLDB is not at all clear to me. Which LLDB libraries 
> are allowed to link against clang? Do we have a diagram somewhere?
> 
> -- adrian
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D47235#1109580, @zturner wrote:

> In https://reviews.llvm.org/D47235#1109219, @labath wrote:
>
> > I guess it would be nice to encapsulate this in some sort of a plugin 
> > (since the setting is used from the clang expression parser plugin, I guess 
> > this would be the natural home for it) , but I haven't looked in detail at 
> > could that work. What I do know is that we already have the ability to 
> > inject settings from within a plugin (see 
> > SymbolFileDWARF::DebuggerInitialize). Maybe that would work here too?
>
>
> I agree that a clang plugin seems like the "real" solution, I had come to the 
> same conclusion yesterday.  But that is a significant amount of work 
> obviously.


Is it that much work? We already have a clang (well, "clang expression parser" 
plugin). Getting **all** of the clang dependencies into that plugin is a 
completely different story, but I am hoping that simply getting this particular 
setting to live there would be just a matter of cargo-culting some code from 
SymbolFileDWARF.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

There's not really a diagram, because we don't have an exact vision of what
the final layering is going to look like (some things will need to be split
up, entirely new targets will need to be introduced, etc).  Mostly it's
just built from experience based on what the primary logical function of a
target is, and then asking whether or not someone who wishes to use that
functionality should be required to link in all of that target's current
dependencies.  And hoping that a layering emerges from this process, at
which point we can then make a diagram as well as enforce it through CMake
/ LLVMBuild / etc.

Core is the fundamental target that contains the basic data structures
representing a generic debugger.  A generic debugger shouldn't need to
depend on clang.  Someone should be able to (in theory) link against Core
(and perhaps a few other targets) and build a Java debugger.  Or a debugger
for an embedded platform.  So a dependency on clang doesn't belong there.

LLDB is a specific debugger which is built on top of Core and other
libraries, and so that is why I think it makes sense to expose a static
function in Module which allows you to set the clang modules path, and then
we do that during LLDB initialization.  For the record, that still isn't
correct from a purity standpoint, because there is a method in Core which
claims to have something to do with clang modules.  But at least it's
relatively benign.

In any case, what do you think about that approach?

On Wed, May 23, 2018 at 8:42 AM Adrian Prantl  wrote:

>
>
> > On May 22, 2018, at 6:57 PM, Zachary Turner  wrote:
> >
> > Yea I don’t think this addresses the problem. We should be able to link
> against parts of lldb without a dependency on clang. Since this is about
> configuring something related to clang, it seems like it should be isolated
> to some part of lldb that interfaces with clang
>
> And this is the point where I need some help :-)
> The intended layering of LLDB is not at all clear to me. Which LLDB
> libraries are allowed to link against clang? Do we have a diagram somewhere?
>
> -- adrian
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via lldb-commits]

> On May 22, 2018, at 6:57 PM, Zachary Turner  wrote:
> 
> Yea I don’t think this addresses the problem. We should be able to link 
> against parts of lldb without a dependency on clang. Since this is about 
> configuring something related to clang, it seems like it should be isolated 
> to some part of lldb that interfaces with clang 

And this is the point where I need some help :-)
The intended layering of LLDB is not at all clear to me. Which LLDB libraries 
are allowed to link against clang? Do we have a diagram somewhere?

-- adrian

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D47235#1109219, @labath wrote:

> In a way, this makes sense, but in a lot of other ways, it actually makes 
> things worse :)
>
> My long-term goal is to be able to build lldb-server without even having 
> clang checked out (because it doesn't really need anything clang-related), 
> and this would certainly make that impossible. Having Core depend on clang 
> does not worry me much because it depends on so many things already, so the 
> way I was thinking of resolving that is to move low-level things out of it 
> (my tentative list includes things like Event/Listener/Broadcaster, State, 
> Communication). That would leave Core with containing only the high-level 
> stuff for which a clang dependency is not that surprising (although I agree 
> that a ModuleList is not the best place for such a dependency).


Agree, but just because `Core` is already a problem doesn't mean we should just 
ignore it IMO.  At some point we're going to have to do something about it, 
even if it's not that surprising for `Core` to have a dependency on clang at 
some point in the future, it will *almost always* be surprising for two things 
to have a dependency on each other.  So from that angle, we need to be vigilant 
in outgoing dependencies from `Core`.

> I guess it would be nice to encapsulate this in some sort of a plugin (since 
> the setting is used from the clang expression parser plugin, I guess this 
> would be the natural home for it) , but I haven't looked in detail at could 
> that work. What I do know is that we already have the ability to inject 
> settings from within a plugin (see SymbolFileDWARF::DebuggerInitialize). 
> Maybe that would work here too?

I agree that a clang plugin seems like the "real" solution, I had come to the 
same conclusion yesterday.  But that is a significant amount of work obviously.

That's why I proposed in the other thread the idea of having a function called 
`Module::setDefaultClangModulesPath(const Twine&)` and just call that method in 
`SystemInitializerFull`.  That's very similar in spirit to how the plugins work 
anyway.  During initialization we call global functions on all of the plugins 
to have them initialize themselves, and they're free to muck with the world 
during this initialization.  So this solution is, IMO, an incremental step 
towards the "real" solution while still not making the problem worse.  If/when 
someone makes a real clang plugin, they just copy this code into that plugins 
initialize method.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In a way, this makes sense, but in a lot of other ways, it actually makes 
things worse :)

My long-term goal is to be able to build lldb-server without even having clang 
checked out (because it doesn't really need anything clang-related), and this 
would certainly make that impossible. Having Core depend on clang does not 
worry me much because it depends on so many things already, so the way I was 
thinking of resolving that is to move low-level things out of it (my tentative 
list includes things like Event/Listener/Broadcaster, State, Communication). 
That would leave Core with containing only the high-level stuff for which a 
clang dependency is not that surprising (although I agree that a ModuleList is 
not the best place for such a dependency).

I guess it would be nice to encapsulate this in some sort of a plugin (since 
the setting is used from the clang expression parser plugin, I guess this would 
be the natural home for it) , but I haven't looked in detail at could that 
work. What I do know is that we already have the ability to inject settings 
from within a plugin (see SymbolFileDWARF::DebuggerInitialize). Maybe that 
would work here too?

PS: This patch would make the clang modules path similar to the python 
directory computation, but this is also something that I've been meaning to 
change somehow (lldb-server does not need python either).




Comment at: source/Core/ModuleList.cpp:88
 
-  llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
-  SetClangModulesCachePath(path);
+  SetClangModulesCachePath(HostInfoBase::GetDefaultClangModuleCachePath());
 }

You should reference this through the `HostInfo` typedef to get the "static 
polymorphism" to work.


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a subscriber: aprantl.
zturner added a comment.

Yea I don’t think this addresses the problem. We should be able to link
against parts of lldb without a dependency on clang. Since this is about
configuring something related to clang, it seems like it should be isolated
to some part of lldb that interfaces with clang


https://reviews.llvm.org/D47235



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

Yea I don’t think this addresses the problem. We should be able to link
against parts of lldb without a dependency on clang. Since this is about
configuring something related to clang, it seems like it should be isolated
to some part of lldb that interfaces with clang
On Tue, May 22, 2018 at 4:32 PM Adrian Prantl via Phabricator <
revi...@reviews.llvm.org> wrote:

> aprantl created this revision.
> aprantl added reviewers: zturner, jingham.
> Herald added subscribers: jkorous, MaskRay, ioeric, ilya-biryukov, mgorny.
>
> @zturner wrote:
>
> > This change has introduced a dependency from Core -> clang Driver (due
> to #include "clang/Driver/Driver.h" in ModuleList.cpp).  Can you please try
> to find a way to remove this dependency?
>
> This is implementing Jim's suggestions in
> http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180521/041010.html
> .
> This certainly makes sense from a logical perspective, but I'm not sure
> whether this addresses Zachary's concerns. Let me know what you think.
>
>
> https://reviews.llvm.org/D47235
>
> Files:
>   include/lldb/Host/HostInfoBase.h
>   source/Core/ModuleList.cpp
>   source/Host/CMakeLists.txt
>   source/Host/common/HostInfoBase.cpp
>
>
> Index: source/Host/common/HostInfoBase.cpp
> ===
> --- source/Host/common/HostInfoBase.cpp
> +++ source/Host/common/HostInfoBase.cpp
> @@ -25,6 +25,8 @@
>  #include "llvm/Support/Threading.h"
>  #include "llvm/Support/raw_ostream.h"
>
> +#include "clang/Driver/Driver.h"
> +
>  #include 
>  #include 
>
> @@ -387,3 +389,9 @@
>  break;
>}
>  }
> +
> +std::string HostInfoBase::GetDefaultClangModuleCachePath() {
> +  llvm::SmallString<128> path;
> +  clang::driver::Driver::getDefaultModuleCachePath(path);
> +  return path.str();
> +}
> Index: source/Host/CMakeLists.txt
> ===
> --- source/Host/CMakeLists.txt
> +++ source/Host/CMakeLists.txt
> @@ -185,7 +185,8 @@
>  lldbUtility
>  ${LLDB_PLUGINS}
>  ${EXTRA_LIBS}
> -
> +clangDriver
> +
>LINK_COMPONENTS
>  Object
>  Support
> Index: source/Core/ModuleList.cpp
> ===
> --- source/Core/ModuleList.cpp
> +++ source/Core/ModuleList.cpp
> @@ -13,6 +13,7 @@
>  #include "lldb/Core/ModuleSpec.h"
>  #include "lldb/Host/FileSystem.h"
>  #include "lldb/Host/Symbols.h"
> +#include "lldb/Host/HostInfoBase.h"
>  #include "lldb/Interpreter/OptionValueProperties.h"
>  #include "lldb/Interpreter/OptionValueFileSpec.h"
>  #include "lldb/Interpreter/Property.h"
> @@ -34,7 +35,6 @@
>  #include "llvm/Support/FileSystem.h"
>  #include "llvm/Support/Threading.h"
>  #include "llvm/Support/raw_ostream.h" // for fs
> -#include "clang/Driver/Driver.h"
>
>  #include  // for operator!=, time_point
>  #include  // for shared_ptr
> @@ -85,9 +85,7 @@
>m_collection_sp.reset(new
> OptionValueProperties(ConstString("symbols")));
>m_collection_sp->Initialize(g_properties);
>
> -  llvm::SmallString<128> path;
> -  clang::driver::Driver::getDefaultModuleCachePath(path);
> -  SetClangModulesCachePath(path);
> +
> SetClangModulesCachePath(HostInfoBase::GetDefaultClangModuleCachePath());
>  }
>
>  bool ModuleListProperties::GetEnableExternalLookup() const {
> Index: include/lldb/Host/HostInfoBase.h
> ===
> --- include/lldb/Host/HostInfoBase.h
> +++ include/lldb/Host/HostInfoBase.h
> @@ -90,6 +90,9 @@
>
>  //---
>static ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
>
> +  /// Return the default path for the Clang module cache.
> +  static std::string GetDefaultClangModuleCachePath();
> +
>  protected:
>static bool ComputeSharedLibraryDirectory(FileSpec _spec);
>static bool ComputeSupportExeDirectory(FileSpec _spec);
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: zturner, jingham.
Herald added subscribers: jkorous, MaskRay, ioeric, ilya-biryukov, mgorny.

@zturner wrote:

> This change has introduced a dependency from Core -> clang Driver (due to 
> #include "clang/Driver/Driver.h" in ModuleList.cpp).  Can you please try to 
> find a way to remove this dependency?

This is implementing Jim's suggestions in 
http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180521/041010.html .
This certainly makes sense from a logical perspective, but I'm not sure whether 
this addresses Zachary's concerns. Let me know what you think.


https://reviews.llvm.org/D47235

Files:
  include/lldb/Host/HostInfoBase.h
  source/Core/ModuleList.cpp
  source/Host/CMakeLists.txt
  source/Host/common/HostInfoBase.cpp


Index: source/Host/common/HostInfoBase.cpp
===
--- source/Host/common/HostInfoBase.cpp
+++ source/Host/common/HostInfoBase.cpp
@@ -25,6 +25,8 @@
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include "clang/Driver/Driver.h"
+
 #include 
 #include 
 
@@ -387,3 +389,9 @@
 break;
   }
 }
+
+std::string HostInfoBase::GetDefaultClangModuleCachePath() {
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  return path.str();
+}
Index: source/Host/CMakeLists.txt
===
--- source/Host/CMakeLists.txt
+++ source/Host/CMakeLists.txt
@@ -185,7 +185,8 @@
 lldbUtility
 ${LLDB_PLUGINS}
 ${EXTRA_LIBS}
-  
+clangDriver
+
   LINK_COMPONENTS
 Object
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/Property.h"
@@ -34,7 +35,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for shared_ptr
@@ -85,9 +85,7 @@
   m_collection_sp.reset(new OptionValueProperties(ConstString("symbols")));
   m_collection_sp->Initialize(g_properties);
 
-  llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
-  SetClangModulesCachePath(path);
+  SetClangModulesCachePath(HostInfoBase::GetDefaultClangModuleCachePath());
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
Index: include/lldb/Host/HostInfoBase.h
===
--- include/lldb/Host/HostInfoBase.h
+++ include/lldb/Host/HostInfoBase.h
@@ -90,6 +90,9 @@
   //---
   static ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
 
+  /// Return the default path for the Clang module cache.
+  static std::string GetDefaultClangModuleCachePath();
+  
 protected:
   static bool ComputeSharedLibraryDirectory(FileSpec _spec);
   static bool ComputeSupportExeDirectory(FileSpec _spec);


Index: source/Host/common/HostInfoBase.cpp
===
--- source/Host/common/HostInfoBase.cpp
+++ source/Host/common/HostInfoBase.cpp
@@ -25,6 +25,8 @@
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include "clang/Driver/Driver.h"
+
 #include 
 #include 
 
@@ -387,3 +389,9 @@
 break;
   }
 }
+
+std::string HostInfoBase::GetDefaultClangModuleCachePath() {
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  return path.str();
+}
Index: source/Host/CMakeLists.txt
===
--- source/Host/CMakeLists.txt
+++ source/Host/CMakeLists.txt
@@ -185,7 +185,8 @@
 lldbUtility
 ${LLDB_PLUGINS}
 ${EXTRA_LIBS}
-  
+clangDriver
+
   LINK_COMPONENTS
 Object
 Support
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Symbols.h"
+#include "lldb/Host/HostInfoBase.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/Property.h"
@@ -34,7 +35,6 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include  // for operator!=, time_point
 #include  // for