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

dzhidzhoev · 2024-07-12T23:36:30Z

--whole-archive and --allow-multiple-definition options has been added to linker flags of these import-std-module tests in order to make them pass with libcxx static linking enabled.

Darwin has been excluded since it doesn't seem to be built in this configuration.

Also, some tests were switched from system stdlib to libcxx since the problem described in https://reviews.llvm.org/D139361 seems to be fixed.

llvmbot · 2024-07-12T23:37:16Z

@llvm/pr-subscribers-lldb

Author: Vladislav Dzhidzhoev (dzhidzhoev)

Changes

--whole-archive and --allow-multiple-definition options has been added to linker flags of these import-std-module tests in order to make them pass with libcxx static linking enabled.

Darwin has been excluded since it doesn't seem to build in this configuration.

Also, some tests were switched from system stdlib to libcxx since the problem described in https://reviews.llvm.org/D139361 seems to be fixed.

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

5 Files Affected:

(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 (+6-3)
(modified) lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/Makefile (+6-3)
(modified) lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/Makefile (+6-3)
(modified) lldb/test/API/commands/expression/import-std-module/vector-of-vectors/Makefile (+5)

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 f938f7428468..b96106a55b85 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)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition
+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 98638c56f0b9..b96106a55b85 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,5 +1,8 @@
-# FIXME: once the expression evaluator can handle std libraries with debug
-# info, change this to USE_LIBCPP=1
-USE_SYSTEM_STDLIB := 1
+USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition
+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 98638c56f0b9..b96106a55b85 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,5 +1,8 @@
-# FIXME: once the expression evaluator can handle std libraries with debug
-# info, change this to USE_LIBCPP=1
-USE_SYSTEM_STDLIB := 1
+USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition
+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 98638c56f0b9..b96106a55b85 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,5 +1,8 @@
-# FIXME: once the expression evaluator can handle std libraries with debug
-# info, change this to USE_LIBCPP=1
-USE_SYSTEM_STDLIB := 1
+USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition
+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 f938f7428468..b96106a55b85 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)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition
+endif
+
 include Makefile.rules

Michael137 · 2024-07-12T23:47:00Z

lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/Makefile

@@ -1,5 +1,8 @@
-# FIXME: once the expression evaluator can handle std libraries with debug
-# info, change this to USE_LIBCPP=1
-USE_SYSTEM_STDLIB := 1


Is this related to the linker flag changes in any way? We rely on this on the macOS bots, so it doesn't sound like the underlying issue the FIXME referred to was resolved. If it's an unrelated change, I'd prefer opening a separate PR for these

Michael137 · 2024-07-12T23:47:59Z

lldb/test/API/commands/expression/import-std-module/array/Makefile

@@ -1,3 +1,8 @@
 USE_LIBCPP := 1
 CXX_SOURCES := main.cpp
+
+ifneq ($(OS),Darwin)
+	LD_EXTRAS := -Xlinker --whole-archive -Xlinker --allow-multiple-definition


Can you elaborate on why this is necessary? Example test failures would be great

…tests --whole-archive and --allow-multiple-definition options has been added to linker flags of these import-std-module tests in order to make them pass with libcxx static linking enabled. Darwin has been excluded since it doesn't seem to build in such configuration. Also, some tests were switched from system stdlib to libcxx since the problem described in https://reviews.llvm.org/D139361 seems to be fixed.

slydiman · 2024-10-15T16:08:03Z

It seems #106885 fixed the issue for
deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
and
list-dbg-info-content/TestDbgInfoContentListFromStdModule.py,
but not for
vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py.

The following 3 tests prevent the buildbot from being green

array/TestArrayFromStdModule.py
vector-of-vectors/TestVectorOfVectorsFromStdModule.py
vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py

Michael137 · 2024-10-15T19:01:45Z

Can you add in the PR description why the --whole-archive and --allow-multiple-definition fixes the issue? And paste the log of the CI test that's failing? Or is @slydiman implying this PR isn't needed anymore?

slydiman · 2024-10-15T20:54:35Z

Or is @slydiman implying this PR isn't needed anymore?

This patch should fix the following tests:
array/TestArrayFromStdModule.py
vector-of-vectors/TestVectorOfVectorsFromStdModule.py
vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py

See the buildbot https://lab.llvm.org/staging/#/builders/195/builds/4464
Note it is the first buildbot for cross build and it may turn green for the first time after fixing these tests.

But probably it is possible to update this patch to keep deque-dbg-info-content/Makefile and list-dbg-info-content/Makefile unchanged.

This patch fixes the error https://lab.llvm.org/staging/#/builders/195/builds/4464 ``` File "llvm-project/lldb/test/API/commands/expression/import-std-module/array/TestArrayFromStdModule.py", line 55, in test self.expect_expr("a.at(0)", result_type=value_type, result_value="3") Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler. ``` Note adding the usage of `a.at(0)` to main.cpp did not help. It is the alternative to llvm#98701.

labath · 2024-10-16T06:48:28Z

I doubt those linker flags are going to work on windows. It might be more "portable" to ensure the symbols are present by using them from inside the program.

slydiman · 2024-10-16T07:05:52Z

Those linker flags work on Windows.
Note I tried to add the usage of a.at(0) to main.cpp and it did not help.
Please see the alternative patch #112485.

labath · 2024-10-16T08:23:02Z

I guess it will with clang+lld (which is the only way we can run the test suite now), though I still find this suboptimal -- in particular, I think the multiple definition thingy plants us firmly into undefined behavior territory.

Did you check that at(0) actually causes the necessary symbols to be pulled into the binary? If yes, why can't we find them? If not, why not?

Michael137 · 2024-10-16T09:21:50Z

I guess it will with clang+lld (which is the only way we can run the test suite now), though I still find this suboptimal -- in particular, I think the multiple definition thingy plants us firmly into undefined behavior territory.

Did you check that at(0) actually causes the necessary symbols to be pulled into the binary? If yes, why can't we find them? If not, why not?

Agree with Pavel here, it would be good to figure out what exactly is going wrong in this test.

FWIW, we don't actually want to emit at into the debug-info. This test specifically tests the case where we don't have the debug-info for a function, but are able to find and call it using the std Clang module. IIRC from the last time we saw this, the problem only occurs when debug symbols for libc++ are available (https://reviews.llvm.org/D139361).

Since this is quite tricky to debug, and has been a known issue for a while and (i think) only happens when we built libc++ with debug symbols, I'd be fine with skipping the test on Linux until we figure out what the root cause is.

This is the alternative to llvm#98701. See for more details: https://reviews.llvm.org/D139361 https://discourse.llvm.org/t/lldb-test-failures-on-linux/80095

slydiman · 2024-10-16T12:24:02Z

Since this is quite tricky to debug, and has been a known issue for a while and (i think) only happens when we built libc++ with debug symbols, I'd be fine with skipping the test on Linux until we figure out what the root cause is.

Agree. Please see #112530.

This is the alternative to #98701. See for more details: https://reviews.llvm.org/D139361 https://discourse.llvm.org/t/lldb-test-failures-on-linux/80095

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 the binary 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. LLD (and GNU ld AFAIK) links a binary with libcxx shared library if libc++.so is present in corresponding library search paths. Otherwise, it tries static linking. Here, a linker flag is added to a clang call in libcxx configuration, that ensures that libc++.so is present and found by linker. Forcing linker to keep all functions from libcxx archive in this way llvm#98701, without checking whether libcxx is linked statically looks like a less clean solution, but I'm not sure about that.

dzhidzhoev · 2024-12-06T15:17:35Z

Closed in favor of #118986

dzhidzhoev requested review from tstellar, mizvekov, DavidSpickett, labath, felipepiovezan and dwblaikie July 12, 2024 23:36

dzhidzhoev requested a review from JDevlieghere as a code owner July 12, 2024 23:36

llvmbot added the lldb label Jul 12, 2024

dzhidzhoev self-assigned this Jul 12, 2024

Michael137 reviewed Jul 12, 2024

View reviewed changes

dzhidzhoev force-pushed the lldb/import-std-module branch from 47f541c to c5332e4 Compare October 10, 2024 15:09

slydiman mentioned this pull request Oct 16, 2024

[lldb][test] Fix tests Test*FromStdModule where called missing at(0) #112485

Closed

slydiman mentioned this pull request Oct 16, 2024

[lldb][test] Skip Test*FromStdModule tests on Linux for now #112530

Merged

dzhidzhoev mentioned this pull request Oct 28, 2024

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

Closed

dzhidzhoev closed this Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

dzhidzhoev commented Jul 12, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 12, 2024

Uh oh!

Michael137 Jul 12, 2024

Uh oh!

Michael137 Jul 12, 2024

Uh oh!

slydiman commented Oct 15, 2024 •

edited

Loading

Uh oh!

Michael137 commented Oct 15, 2024 •

edited

Loading

Uh oh!

slydiman commented Oct 15, 2024

Uh oh!

labath commented Oct 16, 2024

Uh oh!

slydiman commented Oct 16, 2024

Uh oh!

labath commented Oct 16, 2024

Uh oh!

Michael137 commented Oct 16, 2024

Uh oh!

slydiman commented Oct 16, 2024

Uh oh!

dzhidzhoev commented Dec 6, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

dzhidzhoev commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 12, 2024

Uh oh!

Michael137 Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

slydiman commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael137 commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slydiman commented Oct 15, 2024

Uh oh!

labath commented Oct 16, 2024

Uh oh!

slydiman commented Oct 16, 2024

Uh oh!

labath commented Oct 16, 2024

Uh oh!

Michael137 commented Oct 16, 2024

Uh oh!

slydiman commented Oct 16, 2024

Uh oh!

dzhidzhoev commented Dec 6, 2024

Uh oh!

Uh oh!

dzhidzhoev commented Jul 12, 2024 •

edited

Loading

slydiman commented Oct 15, 2024 •

edited

Loading

Michael137 commented Oct 15, 2024 •

edited

Loading