Move ModuleList's dependency on clangDriver into Host

[aprantl]

Summary

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

[zturner]

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

[labath]

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

source/Core/ModuleList.cpp

88

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

[zturner]

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

[labath]

In D47235#1109580, @zturner wrote:

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

[aprantl]

Updated patch according to Zachary's suggestion.

[aprantl]

Forgot to update unit tests.

[zturner]

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

source/Core/ModuleList.cpp

16

I think we don't need this include anymore.

88

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)

[aprantl]

Remove whitespace changes.

[aprantl]

Remove unused include.

[aprantl]

source/Core/ModuleList.cpp

88

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.

[zturner]

source/Core/ModuleList.cpp

88

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.

[aprantl]

source/Core/ModuleList.cpp

88

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.

[zturner]

source/Core/ModuleList.cpp

88

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

[zturner]

source/Core/ModuleList.cpp

88

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

[zturner]

source/Core/ModuleList.cpp

88

To put this in perspective, imagine if LLVM's optimization pass library had something like assert(driverIsClang());

[aprantl]

source/Core/ModuleList.cpp

88

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

[zturner]

source/Core/ModuleList.cpp

88

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.

[zturner]

source/Core/ModuleList.cpp

88

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

[zturner]

source/Core/ModuleList.cpp

88

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?

[zturner]

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?

[labath]

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?

[zturner]

That sounds good to me as well.

include/lldb/Host/HostInfoBase.h

public:
 
   //---------------------------------------------------------------------------
   /// If the triple does not specify the vendor, os, and environment parts, we
   /// "augment" these using information from the host and return the resulting
   /// ArchSpec object.
   //---------------------------------------------------------------------------
   static ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
 
+  /// Return the default path for the Clang module cache.
+  static std::string GetDefaultClangModuleCachePath();
+  
 protected:
   static bool ComputeSharedLibraryDirectory(FileSpec &file_spec);
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static bool ComputeProcessTempFileDirectory(FileSpec &file_spec);
   static bool ComputeGlobalTempFileDirectory(FileSpec &file_spec);
   static bool ComputeTempFileBaseDirectory(FileSpec &file_spec);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);
   static bool ComputeSystemPluginsDirectory(FileSpec &file_spec);

source/Core/ModuleList.cpp

 //===-- ModuleList.cpp ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/FileSpecList.h" // for FileSpecList
 #include "lldb/Core/Module.h"
 #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"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h" // for SymbolContextList, SymbolCon...
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Utility/ArchSpec.h"    // for ArchSpec
 #include "lldb/Utility/ConstString.h" // for ConstString
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h" // for GetLogIfAnyCategoriesSet
 #include "lldb/Utility/UUID.h"    // for UUID, operator!=, operator==
 #include "lldb/lldb-defines.h"    // for LLDB_INVALID_INDEX32
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/PosixApi.h" // for PATH_MAX
 #endif
 
 #include "llvm/ADT/StringRef.h" // for StringRef
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for fs
-#include "clang/Driver/Driver.h"
 
 #include <chrono> // for operator!=, time_point
 #include <memory> // for shared_ptr
 #include <mutex>
 #include <string>  // for string
 #include <utility> // for distance
 
 namespace lldb_private {

 enum { ePropertyEnableExternalLookup, ePropertyClangModulesCachePath };
 
 } // namespace
 
 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);
+  SetClangModulesCachePath(HostInfoBase::GetDefaultClangModuleCachePath());
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
   const uint32_t idx = ePropertyEnableExternalLookup;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_properties[idx].default_uint_value != 0);
 }
 

source/Host/CMakeLists.txt

add_lldb_library(lldbHost
   
   LINK_LIBS
     lldbCore
     lldbSymbol
     lldbTarget
     lldbUtility
     ${LLDB_PLUGINS}
     ${EXTRA_LIBS}
+    clangDriver
     
   LINK_COMPONENTS
     Object
     Support
   )

source/Host/common/HostInfoBase.cpp

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include "clang/Driver/Driver.h"
+
 #include <mutex>
 #include <thread>
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 //----------------------------------------------------------------------
void HostInfoBase::ComputeHostArchitectureSupport(ArchSpec &arch_32,
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
   case llvm::Triple::sparcv9:
   case llvm::Triple::systemz:
     arch_64.SetTriple(triple);
     break;
   }
 }
+
+std::string HostInfoBase::GetDefaultClangModuleCachePath() {
+  llvm::SmallString<128> path;
+  clang::driver::Driver::getDefaultModuleCachePath(path);
+  return path.str();
+}