[lldb][test] Link certain libc++ tests with the whole library #118986

dzhidzhoev · 2024-12-06T15:16:28Z

Some tests from 'import-std-module' used to fail on the builder https://lab.llvm.org/staging/#/builders/195/builds/4470, since libcxx is set up to be linked statically with test binaries on it.

Thus, they were temporarily disabled in #112530. Here, this commit is reverted.

When libcxx is linked with a program as an archive of static libraries, functions that aren't used in the program are omitted. Then, jitted expressions from the tests try calling several functions that aren't present in the process image, which causes the failure.

Here, --whole-archive linker flags are added to tests' Makefiles to ensure that the whole libc++ is included during static linking. It's applied only to -lc++ and -lc++abi (via --push-state/--pop-state), since for some reason clang Gnu toolchain driver on certain Linux configurations adds -libclang_rt.builtins twice, so we can't just apply --whole-archive to all static libraries.

Flags in Makefile.rules and tests' Makefiles are changed to supported the configuration when libc++, libc++abi and libunwind are linked separately (this configuration will be enabled on lldb remote buildbots to be able to control what we link with and avoid duplicate symbols errors).

'-nostdlib++ -nostdinc' are added to LDFLAGS in Makefile.rules to avoid duplicate symbols error. Applying them only to CXXFLAGS is not enough since thus they are not passed to the linker call.

'--libcxx-include-dir' flag passing has been fixed on Windows host for remote platform in lldb/test/API/lit.cfg.py.

Some tests from 'import-std-module' used to fail on the builder https://lab.llvm.org/staging/#/builders/195/builds/4470, since libcxx is set up to be linked statically with test binaries on it. Thus, they were temporarily disabled in llvm#112530. Here, this commit is reverted. When libcxx is linked with a program as an archive of static libraries, functions that aren't used in the program are omitted. Then, jitted expressions from the tests try calling several functions that aren't present in the process image, which causes the failure. Here, --whole-archive linker flags are added to tests' Makefiles to ensure that the whole libc++ is included during static linking. It's applied only to -lc++ and -lc++abi (via --push-state/--pop-state), since for some reason clang Gnu toolchain driver on certain Linux configurations adds -libclang_rt.builtins twice, so we can't just apply --whole-archive to all static libraries. Flags in Makefile.rules and tests' Makefiles are changed to supported the configuration when libc++, libc++abi and libunwind are linked separately (this configuration will be enabled on lldb remote buildbots to be able to control what we link with and avoid duplicate symbols errors). '-nostdlib++ -nostdinc' are added to LDFLAGS in Makefile.rules to avoid duplicate symbols error. Applying them only to CXXFLAGS is not enough since thus they are not passed to the linker call. '--libcxx-include-dir' flag passing has been fixed on Windows host for remote platform in lldb/test/API/lit.cfg.py.

llvmbot · 2024-12-06T15:17:03Z

@llvm/pr-subscribers-lldb

Author: Vladislav Dzhidzhoev (dzhidzhoev)

Changes

Some tests from 'import-std-module' used to fail on the builder https://lab.llvm.org/staging/#/builders/195/builds/4470, since libcxx is set up to be linked statically with test binaries on it.

Thus, they were temporarily disabled in #112530. Here, this commit is reverted.

When libcxx is linked with a program as an archive of static libraries, functions that aren't used in the program are omitted. Then, jitted expressions from the tests try calling several functions that aren't present in the process image, which causes the failure.

Here, --whole-archive linker flags are added to tests' Makefiles to ensure that the whole libc++ is included during static linking. It's applied only to -lc++ and -lc++abi (via --push-state/--pop-state), since for some reason clang Gnu toolchain driver on certain Linux configurations adds -libclang_rt.builtins twice, so we can't just apply --whole-archive to all static libraries.

Flags in Makefile.rules and tests' Makefiles are changed to supported the configuration when libc++, libc++abi and libunwind are linked separately (this configuration will be enabled on lldb remote buildbots to be able to control what we link with and avoid duplicate symbols errors).

'-nostdlib++ -nostdinc' are added to LDFLAGS in Makefile.rules to avoid duplicate symbols error. Applying them only to CXXFLAGS is not enough since thus they are not passed to the linker call.

'--libcxx-include-dir' flag passing has been fixed on Windows host for remote platform in lldb/test/API/lit.cfg.py.

Full diff: https://github.com/llvm/llvm-project/pull/118986.diff

8 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/make/Makefile.rules (+2-2)
(modified) lldb/test/API/commands/expression/import-std-module/array/Makefile (+5)
(modified) lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile (+5)
(modified) lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile (+5)
(modified) lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile (+5)
(modified) lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile (+5)
(modified) lldb/test/API/lit.cfg.py (+2-1)
(modified) lldb/test/Shell/helper/toolchain.py (+1)

diff --git a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
index d0045ac9f91a77..3972fa5da7faf5 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -368,7 +368,7 @@ ifeq (,$(filter 1, $(USE_LIBSTDCPP) $(USE_LIBCPP) $(USE_SYSTEM_STDLIB)))
     ifneq "$(LIBCPP_INCLUDE_TARGET_DIR)" ""
       CXXFLAGS += -cxx-isystem $(LIBCPP_INCLUDE_TARGET_DIR)
     endif
-    LDFLAGS += -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++
+    LDFLAGS += -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++ -lc++abi
   else
     USE_SYSTEM_STDLIB := 1
   endif
@@ -389,7 +389,7 @@ ifeq (1,$(USE_LIBCPP))
 		ifneq "$(LIBCPP_INCLUDE_TARGET_DIR)" ""
 				CXXFLAGS += -cxx-isystem $(LIBCPP_INCLUDE_TARGET_DIR)
 		endif
-		LDFLAGS += -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++
+		LDFLAGS += -nostdlib++ -nostdinc -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++ -lc++abi
 	else
 		ifeq "$(OS)" "Android"
 				# Nothing to do, this is already handled in
diff --git a/lldb/test/API/commands/expression/import-std-module/array/Makefile b/lldb/test/API/commands/expression/import-std-module/array/Makefile
index f938f7428468ab..6f0d77235b59de 100644
--- a/lldb/test/API/commands/expression/import-std-module/array/Makefile
+++ b/lldb/test/API/commands/expression/import-std-module/array/Makefile
@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LDFLAGS := -Xlinker --push-state -Xlinker --whole-archive -lc++ -lc++abi -Xlinker --pop-state
+endif
+
 include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile b/lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
index f938f7428468ab..6f0d77235b59de 100644
--- a/lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
+++ b/lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile
@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LDFLAGS := -Xlinker --push-state -Xlinker --whole-archive -lc++ -lc++abi -Xlinker --pop-state
+endif
+
 include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile b/lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
index f938f7428468ab..6f0d77235b59de 100644
--- a/lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
+++ b/lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile
@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LDFLAGS := -Xlinker --push-state -Xlinker --whole-archive -lc++ -lc++abi -Xlinker --pop-state
+endif
+
 include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile b/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
index f938f7428468ab..6f0d77235b59de 100644
--- a/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
+++ b/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile
@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LDFLAGS := -Xlinker --push-state -Xlinker --whole-archive -lc++ -lc++abi -Xlinker --pop-state
+endif
+
 include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile b/lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile
index f938f7428468ab..6f0d77235b59de 100644
--- a/lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile
+++ b/lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile
@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LDFLAGS := -Xlinker --push-state -Xlinker --whole-archive -lc++ -lc++abi -Xlinker --pop-state
+endif
+
 include Makefile.rules
diff --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index 06c685ebc3f5a5..320e6c94976ffc 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -4,6 +4,7 @@
 
 import os
 import platform
+import re
 import shlex
 import shutil
 import subprocess
@@ -211,7 +212,7 @@ def delete_module_cache(path):
 
 # If we have a just-built libcxx, prefer it over the system one.
 if is_configured("has_libcxx") and config.has_libcxx:
-    if platform.system() != "Windows":
+    if platform.system() != "Windows" or re.match(r".*-linux.*", config.target_triple):
         if is_configured("libcxx_include_dir") and is_configured("libcxx_libs_dir"):
             dotest_cmd += ["--libcxx-include-dir", config.libcxx_include_dir]
             if is_configured("libcxx_include_target_dir"):
diff --git a/lldb/test/Shell/helper/toolchain.py b/lldb/test/Shell/helper/toolchain.py
index 42968128f27026..a62f6f4ec52cbf 100644
--- a/lldb/test/Shell/helper/toolchain.py
+++ b/lldb/test/Shell/helper/toolchain.py
@@ -239,6 +239,7 @@ def use_support_substitutions(config):
         host_flags += [
             "-L{}".format(config.libcxx_libs_dir),
             "-lc++",
+            "-lc++abi",
         ]
 
     host_flags = " ".join(host_flags)

labath

It's definitely better, but I'm still not particularly thrilled with this approach. IIUC, you made this work by changing the buildbot configuration, which is fine if you control all the buildbots that build this way, but it's definitely not ideal. The linker flags are skipped on darwin because their linker doesn't understand them, which works because darwin currently always builds libc++ dynamically, but if that changed, we'd need a darwin-specific version of this. The same goes for windows, which currently works only because we're always using lld.

OTOH, pulling in a symbol by referencing it from a C++ file should work under pretty much any configuration.

labath · 2024-12-09T15:38:23Z

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

@@ -389,7 +389,7 @@ ifeq (1,$(USE_LIBCPP))
 		ifneq "$(LIBCPP_INCLUDE_TARGET_DIR)" ""
 				CXXFLAGS += -cxx-isystem $(LIBCPP_INCLUDE_TARGET_DIR)
 		endif
-		LDFLAGS += -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++
+		LDFLAGS += -nostdlib++ -nostdinc -L$(LIBCPP_LIBRARY_DIR) -Wl,-rpath,$(LIBCPP_LIBRARY_DIR) -lc++ -lc++abi


Is -nostdinc here really needed here? Headers shouldn't be accessed during the link step (and even if they were, this should probably be -nostdinc++)

dzhidzhoev · 2025-01-09T20:06:04Z

It's definitely better, but I'm still not particularly thrilled with this approach. IIUC, you made this work by changing the buildbot configuration, which is fine if you control all the buildbots that build this way, but it's definitely not ideal. The linker flags are skipped on darwin because their linker doesn't understand them, which works because darwin currently always builds libc++ dynamically, but if that changed, we'd need a darwin-specific version of this. The same goes for windows, which currently works only because we're always using lld.

OTOH, pulling in a symbol by referencing it from a C++ file should work under pretty much any configuration.

Thank you! Could you take a look at this #122358?

dzhidzhoev requested review from DavidSpickett, labath and Michael137 December 6, 2024 15:16

dzhidzhoev requested a review from JDevlieghere as a code owner December 6, 2024 15:16

dzhidzhoev self-assigned this Dec 6, 2024

dzhidzhoev added the lldb label Dec 6, 2024

dzhidzhoev mentioned this pull request Dec 6, 2024

[lldb][test] Skip libc++ tests if it is linked statically #113935

Closed

dzhidzhoev mentioned this pull request Dec 6, 2024

[lldb][test] Enable static linking with libcxx for import-std-module tests #98701

Closed

labath reviewed Dec 9, 2024

View reviewed changes

dzhidzhoev closed this Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][test] Link certain libc++ tests with the whole library #118986

[lldb][test] Link certain libc++ tests with the whole library #118986

Uh oh!

dzhidzhoev commented Dec 6, 2024

Uh oh!

llvmbot commented Dec 6, 2024

Uh oh!

labath left a comment

Uh oh!

labath Dec 9, 2024

Uh oh!

dzhidzhoev commented Jan 9, 2025

Uh oh!

Uh oh!

[lldb][test] Link certain libc++ tests with the whole library #118986

[lldb][test] Link certain libc++ tests with the whole library #118986

Uh oh!

Conversation

dzhidzhoev commented Dec 6, 2024

Uh oh!

llvmbot commented Dec 6, 2024

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

labath Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

dzhidzhoev commented Jan 9, 2025

Uh oh!

Uh oh!