[lldb] Remove the requirement for windows clients to specify -DIMPORT_LIBLLDB

[labath]

Summary

This macro was being used to select the proper import/export annotations
on SB classes. Non-windows clients do not have such requirements.

Instead introduce a new macro (LLDB_IN_LIBLLDB), which signals that
we're compiling liblldb itself (and should use dllexport). The default
(no macro) is to use dllimport. I've moved the macro definition to
SBDefines.h, since it only makes sense when building the API library.

[stella.stamenova]

LGTM.

[JDevlieghere]

Neat

[thakis]

Before this change, it was very easy to build liblldb as a static library (just don't define IMPORT_LIBLLDB and EXPORT_LIBLLDB). After this change, this is no longer possible on Windows (but it still works fine everywhere else). Is this intentional?

[labath]

Well.. I wouldn't say it was intentional, but building (lib)lldb as a static library is not really supported either.

Like, there's no way to do that from cmake. Some people using other build systems do that nonetheless, but it comes with all kinds of strings (regardless of the operating system).

So what happens now if you define LLDB_IN_LIBLLDB? Does dllexport explode if used in a static library?

[thakis]

So what happens now if you define LLDB_IN_LIBLLDB? Does dllexport explode if used in a static library?

Do you mean define it for all targets, instead of for just liblldb?

If just for liblldb, then the link fails because (e.g.) driver tries to dllimport the symbols in liblldb then.

If you mean "define for everything", I haven't checked (since it's inconvenient to do).

To be clear, this isn't a huge problem for me, I'll just switch to a dll on Windows. The old system just seemed more flexible than the new setup, without being more complicated. (Maybe you're planning to do some followup that requires this change.)

[labath]

In D117564#3254798, @thakis wrote:

So what happens now if you define LLDB_IN_LIBLLDB? Does dllexport explode if used in a static library?

Do you mean define it for all targets, instead of for just liblldb?

If just for liblldb, then the link fails because (e.g.) driver tries to dllimport the symbols in liblldb then.

If you mean "define for everything", I haven't checked (since it's inconvenient to do).

I see. Thanks for explaining.

To be clear, this isn't a huge problem for me, I'll just switch to a dll on Windows. The old system just seemed more flexible than the new setup, without being more complicated. (Maybe you're planning to do some followup that requires this change.)

The background here is that I found this patch in one of my old WIP branches. The next patch in the series was going to define LLDB_API to __attribute__((visibility("default"))) on the non-windows path (and make everything else hidden), but I have no idea why I wanted to do that. I mean, it does not sounds like an *un*reasonable thing to do, but am not going to do unless I find some benefit to it (maybe it saves size?)

This patch, otoh, seemed useful (and it still does) on its own as it removes the need for windows users to define some extra macros (for the supported configuration, anyway).

Anyway, what would you say if we wrapped this entire LLDB_API-setting block in an #ifndef LLDB_API? That way, things would still work out-of-the-box in the default (supported) configuration, but users who know what they're doing can define it to visibility(default), nothing, or whatever else, and that would take precedence.

[thakis]

The background here is that I found this patch in one of my old WIP branches. The next patch in the series was going to define LLDB_API to __attribute__((visibility("default"))) on the non-windows path (and make everything else hidden), but I have no idea why I wanted to do that. I mean, it does not sounds like an *un*reasonable thing to do, but am not going to do unless I find some benefit to it (maybe it saves size?)

It'd allow getting rid of the .exports files, which would be nice I suppose.

Anyway, what would you say if we wrapped this entire LLDB_API-setting block in an #ifndef LLDB_API? That way, things would still work out-of-the-box in the default (supported) configuration, but users who know what they're doing can define it to visibility(default), nothing, or whatever else, and that would take precedence.

That sounds like a good idea to me.

[labath]

I've done that with rG57ebfea38c03e5cd2d0677eabd2abf761b336097, if you're still interested in static builds. It took me a while to get to that, as I was fighting fires which also had to do with funky lldb builds (unrelated to this patch).

lldb/include/lldb/API/SBDefines.h

 #define LLDB_API_SBDEFINES_H
 
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 #include "lldb/lldb-versioning.h"
 
+#if defined(_WIN32)
+#if defined(LLDB_IN_LIBLLDB)
+#define LLDB_API __declspec(dllexport)
+#else
+#define LLDB_API __declspec(dllimport)
+#endif
+#else // defined (_WIN32)
+#define LLDB_API
+#endif
+
 // Forward Declarations
 namespace lldb {
 
 class LLDB_API SBAddress;
 class LLDB_API SBBlock;
 class LLDB_API SBBreakpoint;
 class LLDB_API SBBreakpointLocation;
 class LLDB_API SBBreakpointName;

lldb/include/lldb/lldb-defines.h

 //===-- lldb-defines.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_LLDB_DEFINES_H
 #define LLDB_LLDB_DEFINES_H
 
 #include "lldb/lldb-types.h"
 
-#if defined(_WIN32)
-#if defined(EXPORT_LIBLLDB)
-#define LLDB_API __declspec(dllexport)
-#elif defined(IMPORT_LIBLLDB)
-#define LLDB_API __declspec(dllimport)
-#else
-#define LLDB_API
-#endif
-#else // defined (_WIN32)
-#define LLDB_API
-#endif
-
 #if !defined(INT32_MAX)
 #define INT32_MAX 2147483647
 #endif
 
 #if !defined(UINT32_MAX)
 #define UINT32_MAX 4294967295U
 #endif
 

lldb/source/API/CMakeLists.txt

-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
-  add_definitions( -DEXPORT_LIBLLDB )
-endif()
-
 get_property(LLDB_ALL_PLUGINS GLOBAL PROPERTY LLDB_PLUGINS)
 
 if(LLDB_BUILD_FRAMEWORK)
   set(option_install_prefix INSTALL_PREFIX ${LLDB_FRAMEWORK_INSTALL_DIR})
   set(option_framework FRAMEWORK)
 endif()
 
 if(LLDB_ENABLE_PYTHON)
if(LLDB_ENABLE_LUA)
   set_source_files_properties(${lldb_lua_wrapper} PROPERTIES GENERATED ON)
 endif()
 
 set_target_properties(liblldb
   PROPERTIES
   VERSION ${LLDB_VERSION}
 )
 
+target_compile_definitions(liblldb PRIVATE LLDB_IN_LIBLLDB)
 if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
   if (NOT LLDB_EXPORT_ALL_SYMBOLS)
     # If we're not exporting all symbols, we'll want to explicitly set
     # the exported symbols here.  This prevents 'log enable --stack ...'
     # from working on some systems but limits the liblldb size.
     MESSAGE("-- Symbols (liblldb): exporting all symbols from the lldb namespace")
     add_llvm_symbol_exports(liblldb ${CMAKE_CURRENT_SOURCE_DIR}/liblldb.exports)
   else()

lldb/tools/driver/CMakeLists.txt

add_lldb_tool(lldb
   LINK_LIBS
     liblldb
 
   LINK_COMPONENTS
     Option
     Support
   )
 
-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
-  add_definitions( -DIMPORT_LIBLLDB )
-endif()
-
 add_dependencies(lldb
   LLDBOptionsTableGen
   ${tablegen_deps}
 )
 
 set_target_properties(LLDBOptionsTableGen PROPERTIES FOLDER "lldb misc")
 
 if(LLDB_BUILD_FRAMEWORK)

lldb/tools/lldb-vscode/CMakeLists.txt

 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD" )
-  add_definitions( -DIMPORT_LIBLLDB )
   list(APPEND extra_libs lldbHost)
 endif ()
 
 if (HAVE_LIBPTHREAD)
   list(APPEND extra_libs pthread)
 endif ()