[lldb] Make the LLDB version a first class citizen

[JDevlieghere]

Summary

Instead of trying to squeeze GetVersion into lldb-private.h, give it its own header and implementation file in its own directory. I called it Basic, partially because the library was already called lldbBase (?) but mostly to align with clang/Basic/Version.h (and swift/Basic/Version.h) which might explain the current library name. I'm pretty flexible as far as naming goes, I think Version/Version.h would work pretty well too.

Additional benefits of this patch include that we can get rid of the quoting macros and that other place of LLDB can read the version number from Version.inc. A downstream change requiring the latter was the main motivation for this patch. I considered adding a few other functions similar to what clang offers but decided against it since there would be no existing uses for them.

[labath]

I am definitely not sad to see lldbBase go away, but I am wondering if we really need a separate library for this functionality (function).

The only reason I can think of is that this function forces a dependency on clang. If we put it in say Utility, then all of lldb would depend on a (very tiny) piece of clang. I don't think that most of lldb would care about that, but this isn't technically correct for lldb-server. It doesn't use clang, and only depends on it because of the general tangledness of lldb dependencies. In an ideal world one would even be able to build lldb-server without having clang checked out. However, given that:

• we are pretty far from an ideal world, and if we ever reach it, we could make this small dependency conditional on the existence of clang
• it's really not clear (to me) what else should belong in the Basic library

I am thinking that we could just put this stuff into Utility. It's true that the clang version stuff lives in a library called "Basic", but that's because this is the name clang chose for its lowest level library. In lldb the lowest level library is Utility.

WDYT?

[tschuett]

I believe the Basic library in Clang does not depend on other Clang libraries. It is a tail library. Do you envision a Basic, Utility, ... library that does not depend on Clang and provides basic utilities.

Maybe 2 tail libraries with utilities. One with dependencies on Clang and one without.

[JDevlieghere]

In D115211#3175581, @labath wrote:

I am definitely not sad to see lldbBase go away, but I am wondering if we really need a separate library for this functionality (function).

The only reason I can think of is that this function forces a dependency on clang. If we put it in say Utility, then all of lldb would depend on a (very tiny) piece of clang. I don't think that most of lldb would care about that, but this isn't technically correct for lldb-server. It doesn't use clang, and only depends on it because of the general tangledness of lldb dependencies. In an ideal world one would even be able to build lldb-server without having clang checked out. However, given that:

• we are pretty far from an ideal world, and if we ever reach it, we could make this small dependency conditional on the existence of clang
• it's really not clear (to me) what else should belong in the Basic library

I am thinking that we could just put this stuff into Utility. It's true that the clang version stuff lives in a library called "Basic", but that's because this is the name clang chose for its lowest level library. In lldb the lowest level library is Utility.

WDYT?

Yep, the clang dependency is the reason I kept it separate, and the problem gets worse downstream where you might have other compilers (i.e. Swift, but maybe Rust one day :-) that want to be part of the version number string too. Generally speaking we are pretty far from an ideal world dependency-wise, but I think the clang dependencies is one place where Alex has made quite a bit of progress over the past years. If we don't need to have the version be available in the "lowest level library", we can do it relatively nicely by having each TypeSystem provide a compilers-specific version string and incorporating that in the final version. That stuff lives in Target, which is a bit of an awkward place for an LLDB version number. That approach wouldn't work today anyway, because we need the version in Utility for the reproducers, and while I have different plants for those, a version number is going to remain pretty critical. I can imagine other things like Greg's statistics wanting to include a version number too.

What's your main concern about making it a separate library? I'm asking because I think the tradeoff between that and linking clang/swift into Utility leans in favor of a separate library, but I'd like to better understand your perspective.

In D115211#3175792, @tschuett wrote:

I believe the Basic library in Clang does not depend on other Clang libraries. It is a tail library. Do you envision a Basic, Utility, ... library that does not depend on Clang and provides basic utilities.

Maybe 2 tail libraries with utilities. One with dependencies on Clang and one without.

No, other than the version I can't imagine another utility that would have to rely on clang. I picked Basic just for consistency in the header path, I don't envision it being anything more than the version (which is why I floated Version as an alternative).

[labath]

The main reason I wanted to avoid a separate library is because I could not imagine how would it fit into the general library landscape in lldb. It's also moderately strange to have a library for just a single function, but that is something I can live with.

If the idea is that this library would consist of just the single function ~forever, then I think that Version is a better name, and I don't really have a problem with that -- that is pretty much the status quo anyway.

One disadvantage of that is that the version number becomes hard to get from lower level parts of lldb. So far the only use we had for that were the reproducers, so if you say you can handle that, then I guess it should be fine.

One advantage of that is that it opens up the possibility for different components of lldb to report different version string. For example, lldb-server could write its own GetVersion function (which wouldn't include clang), and that would look a lot more natural if the "main" version function lives in a library that it does not even use.

[JDevlieghere]

Rename Basic -> Version

[thakis]

Thanks for this change! I always wondered why this strange lldbBase target existed in lldb/source instead of what's now done in this patch.

lldb/source/CMakeLists.txt

4

should lldb.cpp be deleted too?

lldb/source/Version/Version.cpp

18 ↗

(On Diff #392948)

does this intentionally use CLANG_VERSION_STRING instead of LLDB_VERSION_STRING?

(if not: remove clang/Basic/Version.h include too)

[thakis]

lldb/source/CMakeLists.txt

4

fixed in D115438

lldb/source/Version/Version.cpp

18 ↗

(On Diff #392948)

This is still wrong, I think.

(The include is needed for the clang::getclangRevision() call in lldb_private::GetVersion() though.)

[JDevlieghere]

lldb/source/Version/Version.cpp

18 ↗

(On Diff #392948)

Yes, I think this is a remnant from before we computed LLDB_VERSION_* correctly in CMake. I'll put up a patch.

lldb/include/lldb/Basic/Version.h

+//===-- Version.h -----------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_BASIC_VERSION_H
+#define LLDB_BASIC_VERSION_H
+
+#include <string>
+
+namespace lldb_private {
+
+/// Retrieves a string representing the complete LLDB version, which includes
+/// the lldb version number, as well as embedded compiler versions and the
+/// vendor tag.
+const char *GetVersion();
+
+} // namespace lldb_private
+
+#endif // LLDB_BASIC_VERSION_H

lldb/include/lldb/Basic/Version.inc.in

+#define LLDB_VERSION @LLDB_VERSION@
+#define LLDB_VERSION_STRING "@LLDB_VERSION@"
+#define LLDB_VERSION_MAJOR @LLDB_VERSION_MAJOR@
+#define LLDB_VERSION_MINOR @LLDB_VERSION_MINOR@
+#define LLDB_VERSION_PATCHLEVEL @LLDB_VERSION_PATCHLEVEL@
+#cmakedefine LLDB_FULL_VERSION_STRING "@LLDB_FULL_VERSION_STRING@"

lldb/include/lldb/lldb-private.h

 #if defined(__cplusplus)
 
 #include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
 #include "lldb/lldb-public.h"
 
-namespace lldb_private {
-
-const char *GetVersion();
-
-} // namespace lldb_private
-
 #endif // defined(__cplusplus)
 
 #endif // LLDB_LLDB_PRIVATE_H

lldb/source/API/CMakeLists.txt

add_lldb_library(liblldb SHARED ${option_framework}
   SBVariablesOptions.cpp
   SBWatchpoint.cpp
   SBUnixSignals.cpp
   SystemInitializerFull.cpp
   ${lldb_python_wrapper}
   ${lldb_lua_wrapper}
 
   LINK_LIBS
-    lldbBase
+    lldbBasic
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbExpression
     lldbHost
     lldbInitialization
     lldbInterpreter
     lldbSymbol

lldb/source/API/SBDebugger.cpp

 //===-- SBDebugger.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SBReproducerPrivate.h"
 #include "SystemInitializerFull.h"
 
 #include "lldb/API/SBDebugger.h"
 
-#include "lldb/lldb-private.h"
-
 #include "lldb/API/SBBroadcaster.h"
 #include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBCommandInterpreterRunOptions.h"
 #include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBFile.h"
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBSourceManager.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBStructuredData.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBTypeCategory.h"
 #include "lldb/API/SBTypeFilter.h"
 #include "lldb/API/SBTypeFormat.h"
 #include "lldb/API/SBTypeNameSpecifier.h"
 #include "lldb/API/SBTypeSummary.h"
 #include "lldb/API/SBTypeSynthetic.h"
 
+#include "lldb/Basic/Version.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Progress.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/XML.h"

lldb/source/API/SBReproducer.cpp

 #include "lldb/API/SBCommandInterpreterRunOptions.h"
 #include "lldb/API/SBData.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBDeclaration.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBHostOS.h"
 #include "lldb/API/SBReproducer.h"
+#include "lldb/Basic/Version.h"
 #include "lldb/Host/FileSystem.h"
-#include "lldb/lldb-private.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::repro;
 
 SBReplayOptions::SBReplayOptions()
     : m_opaque_up(std::make_unique<ReplayOptions>()){}
 

lldb/source/Basic/CMakeLists.txt

+if(LLDB_VERSION_STRING)
+  set(LLDB_FULL_VERSION_STRING LLDB_VERSION_STRING)
+endif()
+
+# Configure the VCSVersion.inc file.
+set(vcs_version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
+set(generate_vcs_version_script "${LLVM_CMAKE_DIR}/GenerateVersionFromVCS.cmake")
+
+find_first_existing_vc_file("${LLDB_SOURCE_DIR}" lldb_vc)
+
+if(lldb_vc AND LLVM_APPEND_VC_REV)
+  set(lldb_source_dir ${LLDB_SOURCE_DIR})
+endif()
+
+add_custom_command(OUTPUT "${vcs_version_inc}"
+  DEPENDS "${lldb_vc}" "${generate_vcs_version_script}"
+  COMMAND ${CMAKE_COMMAND} "-DNAMES=LLDB"
+                           "-DLLDB_SOURCE_DIR=${lldb_source_dir}"
+                           "-DHEADER_FILE=${vcs_version_inc}"
+                           -P "${generate_vcs_version_script}")
+
+set_source_files_properties("${vcs_version_inc}"
+  PROPERTIES GENERATED TRUE
+             HEADER_FILE_ONLY TRUE)
+
+# Configure the Version.inc file.
+set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/Version.inc")
+
+configure_file(
+  ${LLDB_SOURCE_DIR}/include/lldb/Basic/Version.inc.in
+  ${version_inc})
+
+set_source_files_properties("${version_inc}"
+  PROPERTIES GENERATED TRUE
+             HEADER_FILE_ONLY TRUE)
+
+include_directories(${CMAKE_CURRENT_BINARY_DIR})
+
+add_lldb_library(lldbBasic
+  Version.cpp
+  ${vcs_version_inc}
+  ${version_inc})

lldb/source/Basic/Version.cpp

-//===-- lldb.cpp ----------------------------------------------------------===//
+//===-- Version.cpp -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "lldb/Basic/Version.h"
 #include "VCSVersion.inc"
-#include "lldb/lldb-private.h"
+#include "Version.inc"
 #include "clang/Basic/Version.h"
 
-using namespace lldb;
-using namespace lldb_private;
-
-// LLDB_VERSION_STRING is set through a define so unlike the other defines
-// expanded with CMake, it lacks the double quotes.
-#define QUOTE(str) #str
-#define EXPAND_AND_QUOTE(str) QUOTE(str)
-
 static const char *GetLLDBVersion() {
-#ifdef LLDB_VERSION_STRING
-  return EXPAND_AND_QUOTE(LLDB_VERSION_STRING);
+#ifdef LLDB_FULL_VERSION_STRING
+  return LLDB_FULL_VERSION_STRING;
 #else
   return "lldb version " CLANG_VERSION_STRING;
 #endif
 }
 
 static const char *GetLLDBRevision() {
 #ifdef LLDB_REVISION
   return LLDB_REVISION;
 #else
-  return NULL;
+  return nullptr;
 #endif
 }
 
 static const char *GetLLDBRepository() {
 #ifdef LLDB_REPOSITORY
   return LLDB_REPOSITORY;
 #else
-  return NULL;
+  return nullptr;
 #endif
 }
 
 const char *lldb_private::GetVersion() {
   static std::string g_version_str;
+
   if (g_version_str.empty()) {
     const char *lldb_version = GetLLDBVersion();
     const char *lldb_repo = GetLLDBRepository();
     const char *lldb_rev = GetLLDBRevision();
     g_version_str += lldb_version;
     if (lldb_repo || lldb_rev) {
       g_version_str += " (";
       if (lldb_repo)
         g_version_str += lldb_repo;
       if (lldb_repo && lldb_rev)
         g_version_str += " ";
       if (lldb_rev) {
         g_version_str += "revision ";
         g_version_str += lldb_rev;
       }
       g_version_str += ")";
     }
 
     std::string clang_rev(clang::getClangRevision());
     if (clang_rev.length() > 0) {
       g_version_str += "\n  clang revision ";
       g_version_str += clang_rev;
     }
+
     std::string llvm_rev(clang::getLLVMRevision());
     if (llvm_rev.length() > 0) {
       g_version_str += "\n  llvm revision ";
       g_version_str += llvm_rev;
     }
   }
+
   return g_version_str.c_str();
 }

lldb/source/CMakeLists.txt

 include_directories(.)
 
-set(lldbBase_SOURCES
-    lldb.cpp
-  )
-
-
-find_first_existing_vc_file("${LLDB_SOURCE_DIR}" lldb_vc)
-
-set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
-set(generate_vcs_version_script "${LLVM_CMAKE_DIR}/GenerateVersionFromVCS.cmake")
-
-if(lldb_vc AND LLVM_APPEND_VC_REV)
-  set(lldb_source_dir ${LLDB_SOURCE_DIR})
-endif()
-
-add_custom_command(OUTPUT "${version_inc}"
-  DEPENDS "${lldb_vc}" "${generate_vcs_version_script}"
-  COMMAND ${CMAKE_COMMAND} "-DNAMES=LLDB"
-                           "-DLLDB_SOURCE_DIR=${lldb_source_dir}"
-                           "-DHEADER_FILE=${version_inc}"
-                           -P "${generate_vcs_version_script}")
-
-# Mark the generated header as being generated.
-set_source_files_properties("${version_inc}"
-  PROPERTIES GENERATED TRUE
-             HEADER_FILE_ONLY TRUE)
-
-list(APPEND lldbBase_SOURCES ${version_inc})
-
-if(LLDB_VERSION_STRING)
-  set_property(SOURCE lldb.cpp APPEND PROPERTY
-               COMPILE_DEFINITIONS "LLDB_VERSION_STRING=${LLDB_VERSION_STRING}")
-endif()
-
-add_lldb_library(lldbBase
-  ${lldbBase_SOURCES}
-  )
-
+add_subdirectory(Basic)
 add_subdirectory(Breakpoint)
 add_subdirectory(Commands)
 add_subdirectory(Core)
 add_subdirectory(DataFormatters)
 add_subdirectory(Expression)
 add_subdirectory(Host)
 add_subdirectory(Initialization)
 add_subdirectory(Interpreter)
 add_subdirectory(Plugins)
 add_subdirectory(Symbol)
 add_subdirectory(Target)
 add_subdirectory(Utility)
 
 # Build API last.  Since liblldb needs to link against every other target, it needs
 # those targets to have already been created.
 add_subdirectory(API)

lldb/source/Commands/CMakeLists.txt

add_lldb_library(lldbCommands
   CommandObjectTrace.cpp
   CommandObjectType.cpp
   CommandObjectVersion.cpp
   CommandObjectWatchpoint.cpp
   CommandObjectWatchpointCommand.cpp
   CommandOptionsProcessLaunch.cpp
 
   LINK_LIBS
-    lldbBase
+    lldbBasic
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbExpression
     lldbHost
     lldbInterpreter
     lldbSymbol
     lldbTarget
     lldbUtility
 
   LINK_COMPONENTS
     Support
   )
 
 add_dependencies(lldbCommands LLDBOptionsGen)

lldb/source/Commands/CommandObjectVersion.cpp

 //===-- CommandObjectVersion.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CommandObjectVersion.h"
 
+#include "lldb/Basic/Version.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
-#include "lldb/lldb-private.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // CommandObjectVersion
 
 CommandObjectVersion::CommandObjectVersion(CommandInterpreter &interpreter)
     : CommandObjectParsed(interpreter, "version",

lldb/source/Initialization/SystemInitializerCommon.cpp

 //===-- SystemInitializerCommon.cpp ---------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Initialization/SystemInitializerCommon.h"
 
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
+#include "lldb/Basic/Version.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ReproducerProvider.h"
 #include "lldb/Utility/Timer.h"
-#include "lldb/lldb-private.h"
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #endif
 
 #if defined(_WIN32)
 #include "Plugins/Process/Windows/Common/ProcessWindowsLog.h"
 #include "lldb/Host/windows/windows.h"

lldb/tools/lldb-server/CMakeLists.txt

add_lldb_tool(lldb-server
     Acceptor.cpp
     lldb-gdbserver.cpp
     lldb-platform.cpp
     lldb-server.cpp
     LLDBServerUtilities.cpp
     SystemInitializerLLGS.cpp
 
     LINK_LIBS
-      lldbBase
+      lldbBasic
       lldbHost
       lldbInitialization
       ${LLDB_PLUGINS}
       lldbPluginInstructionARM
       lldbPluginInstructionMIPS
       lldbPluginInstructionMIPS64
       ${LLDB_SYSTEM_LIBS}
 

lldb/tools/lldb-server/lldb-server.cpp

 //===-- lldb-server.cpp -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SystemInitializerLLGS.h"
+#include "lldb/Basic/Version.h"
 #include "lldb/Initialization/SystemLifetimeManager.h"
-#include "lldb/lldb-private.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 

lldb/tools/lldb-test/CMakeLists.txt

 get_property(LLDB_ALL_PLUGINS GLOBAL PROPERTY LLDB_PLUGINS)
 
 add_lldb_tool(lldb-test
   FormatUtil.cpp
   lldb-test.cpp
   SystemInitializerTest.cpp
 
   LINK_LIBS
-    lldbBase
+    lldbBasic
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbExpression
     lldbHost
     lldbInitialization
     lldbInterpreter
     lldbSymbol