[SBHostOS] Remove getting the python script interpreter path

[JDevlieghere]

Summary

This is clearly a layering violation and there's no good way to fix this. The value it adds is limited, people can use the $PYTHONPATH or call the corresponding Python API.

[clayborg]

People use this to populate the extra system path in python scripts when "import lldb" fails and throws an exception as you can run "lldb -P" to get the python path for the lldb module for the current lldb in your path.

[davide]

While the intentions of exposing this are good, the implementation leaks too many details. In fact, there shouldn't be anything outside of ScriptInterpreter guarded by LLDB_DISABLE_PYTHON.
You might want also to realize that having ePathTypePythonDir goes against the whole notion of a generic pluggable interpreter.
Imagine tomorrow somebody wants to implement support for another language, what would this call return for it?

If we really want to keep supporting this, maybe the ScriptInterpreter should have a base method that we can override? Still sounds a little bizarre to me because it's really hard to generalize the notion of PythonDir for any other language whatsoever.

[davide]

@clayborg I would also like to point out that some of us fair amount of time cleaning LLDB_DISABLE_PYTHON leaking all over the codebase, and this is basically the last occurrence, so we're motivated to have this going away/fixed in some way.

[jingham]

I agree with Greg, I really don't want to lose the "lldb -P" functionality. That's quite handy.

Seems to me that if lldb wants to provide a scripting module for some scripting language, that language would need to know where the module lived in order to load it. So having a generic "Get the lldb module location" API to help users do that automatically seems entirely appropriate.

Just call it GetLLDBScriptingModuleDir or something and I think that would make total sense.

[jingham]

BTW, congrats on getting rid of all the LLDB_DISABLE_PYTHON uses. That was a goodly chunk of work and removed an obvious wart.

[clayborg]

In D61090#1477709, @davide wrote:

@clayborg I would also like to point out that some of us fair amount of time cleaning LLDB_DISABLE_PYTHON leaking all over the codebase, and this is basically the last occurrence, so we're motivated to have this going away/fixed in some way.

Fine to have a ScriptInterpreterPython that is gutted if disabled. Or call some code that can dynamically detect if python is enabled and do the right thing inside "DefaultScriptInterpreter::GetLLDBModuleDirectory()"

[JDevlieghere]

Deprecate the current method in SBHostOS and provide an alternative through SBCommandInterpreter::GetModuleDirectory

[JDevlieghere]

(I didn't yet, but I'll cleanup the references to Python in the driver if people like this approach.)

[jingham]

It seems weird to be getting the ScriptInterpreter's ScriptModuleDirectory from the CommandInterpreter. I would have expected:

m_debugger.GetScriptInterpreter().GetScriptModuleDirectory()

I don't think there's any reason to have the Command Interpreter know which ScriptInterpreter is being used, so entangling them like this is awkward.

Am I missing some reason why it makes sense to do this on the CommandInterpreter?

[JDevlieghere]

In D61090#1477912, @jingham wrote:

It seems weird to be getting the ScriptInterpreter's ScriptModuleDirectory from the CommandInterpreter. I would have expected:

m_debugger.GetScriptInterpreter().GetScriptModuleDirectory()

I don't think there's any reason to have the Command Interpreter know which ScriptInterpreter is being used, so entangling them like this is awkward.

Am I missing some reason why it makes sense to do this on the CommandInterpreter?

The debugger doesn't know about the script interpreter. The command interpreter is the class that creates it and is in charge of its lifetime. The script interpreter also keeps a reference to the command interpreter, so they're already entangled. The change here just mirrors the relationship as it is in the non-SB variants of the classes.

That being said, I agree that it would be better to not have them entangled. I'll spend some time trying to figure out if we can clean this up, before changing the SB API.

[jingham]

Yeah, I must have not been paying attention at some point in the past. OTOH, I don't know if you can change the access to the ScriptInterpreter in the SB API's, since that seems likely to break people's scripts.

Maybe it's better for this patch to add the GetScriptModuleDirectory to the ScriptInterpreter - which is clearly right - and then get the script interpreter from the command interpreter in the Driver. That way when/if we tackle the larger question, this will still have been the correct choice..

[labath]

I'm going to throw one more opinion into the mix here. :)

While I agree that sprinkling #ifdef PYTHON everywhere is bad, and it's great to be rid of it, I also think that it is good for the user to be able to know how a particular build of lldb was configured. So, I wouldn't say this goes "against the notion of a generic pluggable interpreter". In fact I would say the opposite. In an imaginary future where lldb can be built with either python or say javascript support (or both?), it may be very important for programs to detect whether the library they are using is compatible with them.

The question on my mind is "do we ever want/anticipate the python path to change at runtime?". If we don't, then getting the path value through a command interpreter *instance* seems odd. In fact, I would say that the SBHostOS is the best place to provide this info, as it is really a property of the host, which does not depend on any runtime configuration of anything. So, if the only reason you made this go through the command interpreter class was to break the build dependency, then I think we should do this another way. One way to handle this would be to add one more argument to the PluginManager::RegisterPlugin function which is called from ScriptInterpreterPython::Initialize. This argument could be either a callback which computes the "module path" for the given language, or even the path itself. Then SBHostOS could just get the path by asking the PluginManager.

So, with both backward and forward compatibility in mind, what I'd to is this:

• don't add any new SB interfaces
• implement SBHostOS::GetLLDBPythonPath via a call to PluginManager::GetModulePathFor(eScriptLanguagePython) as described previously
• In the future, once multiple interpreters become a reality, consider a new SBHostOS function, which takes a language type as an argument. This will allow the user to determine which scripting languages are available and leave the door open for a single build of lldb to support multiple languages (which the current proposal doesn't). SBHostOS::GetLLDBPythonPath then just becomes a deprecated variant of this new function.

[clayborg]

I agree with Pavel: no new SB APIs needed as this call is where it is supposed to be on SBHostOS.

[jingham]

I'm a bit confused. If we get to the world where we can support multiple script interpreters, I would expect to add a static method on ScriptInterpreter that enumerates the available interpreters. I would not expect that to be on SBHostOS since this is not a feature of the Host OS but of how lldb was configured. An lldb running on macOS with the Python2 plugin available behaves - w.r.t. macOS - exactly the same as one built with a Python3 plugin available, etc.

So the "available languages" query for sure seems to me to belong as a static method on the ScriptInterpreter class. But then once I've requested a particular script interpreter, asking it where it put the lldb module seems much more natural than going to the HostOS. Under the covers, we will need to cooperate with the HostOS since that knows how lldb is packaged for a particular system. But that doesn't seem like something we need to have users know.

[labath]

In D61090#1479004, @jingham wrote:

I'm a bit confused. If we get to the world where we can support multiple script interpreters, I would expect to add a static method on ScriptInterpreter that enumerates the available interpreters. I would not expect that to be on SBHostOS since this is not a feature of the Host OS but of how lldb was configured. An lldb running on macOS with the Python2 plugin available behaves - w.r.t. macOS - exactly the same as one built with a Python3 plugin available, etc.

So the "available languages" query for sure seems to me to belong as a static method on the ScriptInterpreter class. But then once I've requested a particular script interpreter, asking it where it put the lldb module seems much more natural than going to the HostOS. Under the covers, we will need to cooperate with the HostOS since that knows how lldb is packaged for a particular system. But that doesn't seem like something we need to have users know.

That also sounds pretty reasonable, especially if we want to do other things with the enumerated script interpreters besides getting their "module paths". The only thing is that there isn't a *Script*Interpreter class at the SB level. There is a *Command*Interpreter (which is what this patch is using), but that does not sound completely right to me as there is more to scripting languages than just the "script" command (in fact, I think the main reason that python layering is so hard to get right is that it has a dual role -- python is both embedded into lldb, and it also can be used to drive lldb from the outside).

[jingham]

I really thought there was one at the SB layer, because in terms of design that is what makes sense. I guess we never really needed it until now, so we didn't add it. Once there's more than one hard-coded script interpreter, we will need to add some way to select & direct scripts at the various script interpreters so we will need SBScriptInterpreter at the SB layer. So maybe now is the time to add it in preparation...

Also, the fact that at the lldb_private layer, the ScriptInterpreter is held onto by the CommandInterpreter is clearly wrong. The CommandInterpreter should have a member that tells it the currently selected ScriptInterpreter, but the list of script interpreters should belong to the Debugger. We should probably disentangle that at the same time.

As an aside, IIUC, the current work to support either Python flavor only supports one interpreter at a time because Python doesn't support loading Python 2 & 3 into the same process? That wouldn't be true for any other script interpreter. I get LOTS of requests to add Swift as a script interpreter, BTW, though at present Swift isn't really ready for that so far as I can see...

[clayborg]

I agree with all that was said above.

My reason for leaving it in SBHostOS is due to SBCommandInterpreter not being the right place, since we don't have SBScriptInprereter in the API, and we might as well leave it in SBHostOS until we are going to do it right by having a static call on SBScriptInterpreter that takes a enumeration from lldb-enumerations.h with a list of script interpreter languages that could be supported. Right now we just have python, so no need to add that functionality yet.

[labath]

In D61090#1479049, @jingham wrote:

I really thought there was one at the SB layer, because in terms of design that is what makes sense. I guess we never really needed it until now, so we didn't add it. Once there's more than one hard-coded script interpreter, we will need to add some way to select & direct scripts at the various script interpreters so we will need SBScriptInterpreter at the SB layer. So maybe now is the time to add it in preparation...

Also, the fact that at the lldb_private layer, the ScriptInterpreter is held onto by the CommandInterpreter is clearly wrong. The CommandInterpreter should have a member that tells it the currently selected ScriptInterpreter, but the list of script interpreters should belong to the Debugger. We should probably disentangle that at the same time.

Yes, it sounds like we should have a SBScriptInterpreter at some point. Though, right now, it's not really clear to me what will it's exact role be, so I would tend to agree with Greg that we wait until we have a real use case for it (so we don't commit to a design we may want to change later).

As an aside, IIUC, the current work to support either Python flavor only supports one interpreter at a time because Python doesn't support loading Python 2 & 3 into the same process?

I've heard someone say that, and I can believe that is the official party line of the python project, but I haven't investigated how "true" that statement is. E.g., I'm pretty sure we could get it to work by using some of the fancier flags of dlopen (RTLD_LOCAL, or the new namespace thingies present on linux), but that may take more effort than it's worth..

[JDevlieghere]

Move the function into SBDebugger

[clayborg]

We can't change functionality in our API. Fine to add something to SBDebugger, but we can't just disable a call that used to work in SBHostOS.

lldb/source/API/SBHostOS.cpp

63

So now we don't return anything here? That is bad. this can break older scripts and violates our API. Deprecation is not the same as disabling the functionality that once worked.

lldb/include/lldb/API/SBDebugger.h

public:
   bool GetUseColor() const;
 
   static bool GetDefaultArchitecture(char *arch_name, size_t arch_name_len);
 
   static bool SetDefaultArchitecture(const char *arch_name);
 
   lldb::ScriptLanguage GetScriptingLanguage(const char *script_language_name);
 
+  /// Return the module path for the script interpreter, if one is registered.
+  lldb::SBFileSpec GetScriptDirectory() const;
+
   static const char *GetVersionString();
 
   static const char *StateAsCString(lldb::StateType state);
 
   static SBStructuredData GetBuildConfiguration();
 
   static bool StateIsRunningState(lldb::StateType state);
 

lldb/include/lldb/API/SBFileSpec.h

public:
 
   void AppendPathComponent(const char *file_or_directory);
 
 private:
   friend class SBAttachInfo;
   friend class SBBlock;
   friend class SBCommandInterpreter;
   friend class SBCompileUnit;
+  friend class SBDebugger;
   friend class SBDeclaration;
   friend class SBFileSpecList;
   friend class SBHostOS;
   friend class SBLaunchInfo;
   friend class SBLineEntry;
   friend class SBModule;
   friend class SBModuleSpec;
   friend class SBPlatform;

lldb/include/lldb/API/SBHostOS.h

 #include "lldb/API/SBFileSpec.h"
 
 namespace lldb {
 
 class LLDB_API SBHostOS {
 public:
   static lldb::SBFileSpec GetProgramFileSpec();
 
+  // Deprecated. Use SBCommandInpterpreter::GetScriptModuleDirectory instead.
   static lldb::SBFileSpec GetLLDBPythonPath();
 
   static lldb::SBFileSpec GetLLDBPath(lldb::PathType path_type);
 
   static lldb::SBFileSpec GetUserHomeDirectory();
 
   static void ThreadCreated(const char *name);
 

lldb/include/lldb/Interpreter/ScriptInterpreter.h

public:
   static std::string LanguageToString(lldb::ScriptLanguage language);
 
   static lldb::ScriptLanguage StringToLanguage(const llvm::StringRef &string);
 
   virtual void ResetOutputFileHandle(FILE *new_fh) {} // By default, do nothing.
 
   lldb::ScriptLanguage GetLanguage() { return m_script_lang; }
 
+  virtual FileSpec GetModuleDirectory() { return {}; };
+
 protected:
   Debugger &m_debugger;
   lldb::ScriptLanguage m_script_lang;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ScriptInterpreter_h_

lldb/source/API/SBDebugger.cpp

SBDebugger::GetScriptingLanguage(const char *script_language_name) {
   LLDB_RECORD_METHOD(lldb::ScriptLanguage, SBDebugger, GetScriptingLanguage,
                      (const char *), script_language_name);
 
   if (!script_language_name) return eScriptLanguageDefault;
   return OptionArgParser::ToScriptLanguage(
       llvm::StringRef(script_language_name), eScriptLanguageDefault, nullptr);
 }
 
+lldb::SBFileSpec SBDebugger::GetScriptDirectory() const {
+  if (!m_opaque_sp)
+    return {};
+
+  ScriptInterpreter *script_interpreter =
+      m_opaque_sp->GetScriptInterpreter(true);
+
+  if (!script_interpreter)
+    return {};
+
+  return SBFileSpec(script_interpreter->GetModuleDirectory());
+}
+
 const char *SBDebugger::GetVersionString() {
   LLDB_RECORD_STATIC_METHOD_NO_ARGS(const char *, SBDebugger, GetVersionString);
 
   return lldb_private::GetVersion();
 }
 
 const char *SBDebugger::StateAsCString(StateType state) {
   LLDB_RECORD_STATIC_METHOD(const char *, SBDebugger, StateAsCString,

lldb/source/API/SBHostOS.cpp

case ePathTypeLLDBShlibDir:
     break;
   case ePathTypeSupportExecutableDir:
     fspec = HostInfo::GetSupportExeDir();
     break;
   case ePathTypeHeaderDir:
     fspec = HostInfo::GetHeaderDir();
     break;
   case ePathTypePythonDir:
-#ifndef LLDB_DISABLE_PYTHON
-    fspec = ScriptInterpreterPython::GetPythonDir();
-#endif
+    // Deprecated. Use SBCommandInpterpreter::GetScriptModuleDirectory instead.
     break;
   case ePathTypeLLDBSystemPlugins:
     fspec = HostInfo::GetSystemPluginDir();
     break;
   case ePathTypeLLDBUserPlugins:
     fspec = HostInfo::GetUserPluginDir();
     break;
   case ePathTypeLLDBTempSystemDir:

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

ScriptInterpreterPython(Debugger &debugger)
         IOHandlerDelegateMultiline("DONE") {}
 
   static void Initialize();
   static void Terminate();
   static lldb_private::ConstString GetPluginNameStatic();
   static const char *GetPluginDescriptionStatic();
   static FileSpec GetPythonDir();
 
+  virtual FileSpec GetModuleDirectory() { return GetPythonDir(); };
+
 protected:
   static void ComputePythonDirForApple(llvm::SmallVectorImpl<char> &path);
   static void ComputePythonDirForPosix(llvm::SmallVectorImpl<char> &path);
   static void ComputePythonDirForWindows(llvm::SmallVectorImpl<char> &path);
 };
 } // namespace lldb_private
 
 #endif // LLDB_DISABLE_PYTHON
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTINTERPRETERPYTHON_H

lldb/tools/driver/Driver.cpp

SBError Driver::ProcessArgs(const opt::InputArgList &args, bool &exiting) {
 
   if (m_option_data.m_print_version) {
     llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
     exiting = true;
     return error;
   }
 
   if (m_option_data.m_print_python_path) {
-    SBFileSpec python_file_spec = SBHostOS::GetLLDBPythonPath();
+    SBFileSpec python_file_spec = m_debugger.GetScriptDirectory();
     if (python_file_spec.IsValid()) {
       char python_path[PATH_MAX];
       size_t num_chars = python_file_spec.GetPath(python_path, PATH_MAX);
       if (num_chars < PATH_MAX) {
         llvm::outs() << python_path << '\n';
       } else
         llvm::outs() << "<PATH TOO LONG>\n";
     } else