[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

slydiman · 2024-06-06T19:40:17Z

The different build configuration and target Linux system can load a different number of .so libraries (2 and more). As example, Linux x86_64 host shows the following loaded modules:

Loaded files: ld-linux-x86-64.so.2
Loaded files: [vdso]
Loaded files: a.out
Loaded files: libstdc++.so.6, libm.so.6, libgcc_s.so.1, libc.so.6

avg_solibs_added_per_event = 1.75

But Linux Aarch64 (remote target) with statically linked C++ library (clang) shows:

Loaded files: ld-2.31.so
Loaded files: [vdso]
Loaded files: a.out
Loaded files: libm.so.6, libc.so.6

avg_solibs_added_per_event = 1.25

Increase precision by 1 digit to fix the test.

… of Linux targets. The different build configuration and target Linux system can load a different number of .so libraries (2 and more). As example, Linux x86_64 host shows the following loaded modules: ``` Loaded files: ld-linux-x86-64.so.2 Loaded files: [vdso] Loaded files: a.out Loaded files: libstdc++.so.6, libm.so.6, libgcc_s.so.1, libc.so.6 ``` avg_solibs_added_per_event = 1.75 But Linux Aarch64 (remote target) with statically linked C++ library (clang) shows: ``` Loaded files: ld-2.31.so Loaded files: [vdso] Loaded files: a.out Loaded files: libm.so.6, libc.so.6 ``` avg_solibs_added_per_event = 1.25 Increase precision by 1 digit to fix the test.

llvmbot · 2024-06-06T19:40:50Z

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

The different build configuration and target Linux system can load a different number of .so libraries (2 and more). As example, Linux x86_64 host shows the following loaded modules:

Loaded files: ld-linux-x86-64.so.2
Loaded files: [vdso]
Loaded files: a.out
Loaded files: libstdc++.so.6, libm.so.6, libgcc_s.so.1, libc.so.6

avg_solibs_added_per_event = 1.75

But Linux Aarch64 (remote target) with statically linked C++ library (clang) shows:

Loaded files: ld-2.31.so
Loaded files: [vdso]
Loaded files: a.out
Loaded files: libm.so.6, libc.so.6

avg_solibs_added_per_event = 1.25

Increase precision by 1 digit to fix the test.

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

1 Files Affected:

(modified) lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py (+2-2)

diff --git a/lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py b/lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
index abf761fb3420b..28a1026ae4fcc 100644
--- a/lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
+++ b/lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
@@ -118,6 +118,6 @@ def test_launch_notifications(self):
         # On Linux we get events for ld.so, [vdso], the binary and then all libraries.
 
         avg_solibs_added_per_event = round(
-            float(total_solibs_added) / float(total_modules_added_events)
+            10.0 * float(total_solibs_added) / float(total_modules_added_events)
         )
-        self.assertGreater(avg_solibs_added_per_event, 1)
+        self.assertGreater(avg_solibs_added_per_event, 10)

labath · 2024-06-07T06:59:21Z

lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py

@@ -118,6 +118,6 @@ def test_launch_notifications(self):
        # On Linux we get events for ld.so, [vdso], the binary and then all libraries.

        avg_solibs_added_per_event = round(
-            float(total_solibs_added) / float(total_modules_added_events)
+            10.0 * float(total_solibs_added) / float(total_modules_added_events)


You can't really call it "number of libraries per event" anymore if you multiply by ten. It's more like number of decilibraries/event :P. Maybe you could just remove the round call, and compare to 1.0, but I'm wondering if this is even the right metric for the test. Since what we're trying to check is that we don't send these notifications one by one, I think some check like "number of events with more than one module" or "number of total events" (or both) would be better. @jasonmolenda, what do you think?

Also, this test is very unhermetic in the sense that it depends on the environment to provide enough shared libraries to measure. In the extreme case we could have a totally statically linked binary and zero shared libraries. So, another way to make this test be more resilient is to introduce a couple of shared libraries of our own, so that we can be sure there is something to measure. You could copy the pattern from TestLoadUnload.py to link a bunch of shared libraries to this binary.

You can't really call it "number of libraries per event" anymore if you multiply by ten.

I have updated the patch.

Maybe you could just remove the round call, and compare to 1.0

No, because assertGreater() expects int parameters.

Also, this test is very unhermetic

Right, and it seems that the behavior on Darwin is very different. I'd just check min_modules_per_event >= 1.

@labath

So, another way to make this test be more resilient is to introduce a couple of shared libraries of our own

The patch is updated this way. Please review. Thanks.

You can't really call it "number of libraries per event" anymore if you multiply by ten.

I have updated the patch.

Maybe you could just remove the round call, and compare to 1.0

No, because assertGreater() expects int parameters.

That's strange because assertGreater(47.0001, 47.0000) works just fine for me. So does assertLessEqual(set(["a", "b"]), set(["a", "b"])).

Could it be that you're still running on the version of lldb that has the ancient checked in copy of unittest?

I just checked the documentation for assertGreater() trying to figure out the purpose of round() usage in the original code. But it does not matter anymore, since the patch has been significantly refactored.

github-actions · 2024-06-08T08:55:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

labath

This is great stuff, just a couple of details

labath · 2024-06-10T08:23:42Z

lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py

+        ext = "so"
+        if self.platformIsDarwin():
+            ext = "dylib"


You should be able to get this as self.platformContext.shlib_extension

labath · 2024-06-10T08:26:18Z

lldb/test/API/functionalities/target-new-solib-notifications/a.cpp

+int a_init() { return 234; }
+
+int a_global = a_init();


Just delete all of this stuff, as it's not relevant for what we're testing. (the a_function -> b_function dep is marginally interesting, as it creates a more interesting dependency graph).

labath · 2024-06-10T08:27:11Z

lldb/test/API/functionalities/target-new-solib-notifications/Makefile

-
-include Makefile.rules
+CXX_SOURCES := main.cpp
+LD_EXTRAS := -L. -lloadunload_d -lloadunload_c -lloadunload_a -lloadunload_b


Please rename the modules to something else (even a simple liba is probably fine).

labath · 2024-06-10T08:35:58Z

lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py

+        self.assertGreaterEqual(
+            len(set(max_solib_chunk_per_event).intersection(expected_solibs)),
+            len(expected_solibs),


I think this could be assertLessEqual(set(expected_solibs), set(max_solib_chunk_per_event)) (with the advantage being that you'd bet a better error message than 3 is not less than 4 when it fails.

labath

Thanks.

… of Linux targets (llvm#94672) The different build configuration and target Linux system can load a different number of .so libraries. Add and check own libraries.

slydiman requested review from DavidSpickett, walter-erquinigo and labath June 6, 2024 19:40

slydiman requested a review from JDevlieghere as a code owner June 6, 2024 19:40

llvmbot added the lldb label Jun 6, 2024

labath requested a review from jasonmolenda June 7, 2024 06:50

labath reviewed Jun 7, 2024

View reviewed changes

Keep the avg_solibs_added_per_event name correct.

0518bff

slydiman requested a review from labath June 8, 2024 08:55

slydiman force-pushed the fix-lldb-TestModuleLoadedNotifys branch 3 times, most recently from 0aec72a to d7832a1 Compare June 8, 2024 09:15

Added own so modules.

60671ec

slydiman force-pushed the fix-lldb-TestModuleLoadedNotifys branch from d7832a1 to 60671ec Compare June 8, 2024 09:19

labath reviewed Jun 10, 2024

View reviewed changes

Optimized.

256a245

labath approved these changes Jun 10, 2024

View reviewed changes

slydiman merged commit 3e39328 into llvm:main Jun 10, 2024
5 checks passed

HerrCai0907 mentioned this pull request Jun 13, 2024

tidy #95384

Closed

slydiman deleted the fix-lldb-TestModuleLoadedNotifys branch July 25, 2024 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

slydiman commented Jun 6, 2024

llvmbot commented Jun 6, 2024

labath Jun 7, 2024

slydiman Jun 7, 2024 •

edited

Loading

slydiman Jun 8, 2024

labath Jun 10, 2024

slydiman Jun 10, 2024

github-actions bot commented Jun 8, 2024 •

edited

Loading

labath left a comment

labath Jun 10, 2024

slydiman Jun 10, 2024

labath Jun 10, 2024

slydiman Jun 10, 2024

labath Jun 10, 2024

slydiman Jun 10, 2024

labath Jun 10, 2024 •

edited

Loading

slydiman Jun 10, 2024

labath left a comment

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

Conversation

slydiman commented Jun 6, 2024

llvmbot commented Jun 6, 2024

Choose a reason for hiding this comment

slydiman Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 8, 2024 • edited Loading

labath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labath Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labath left a comment

Choose a reason for hiding this comment

slydiman Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 8, 2024 •

edited

Loading

labath Jun 10, 2024 •

edited

Loading