[WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Michael137]

Summary

By default LLDB currently runs an API test with 3 variants,
one of which, depending on platform, is gmodules. However,
most tests don't actually make use of any specific gmodules
feature since they compile a single main.cpp file without
importing types from other modules.

Instead of running all tests an extra time with -gmodules
enabled, we plan to test gmodules features with dedicated
API tests that explicitly opt-into compiling with -gmodules.
One of the main benefits of this will be a reduction in total
API test-suite run-time (by around 1/3).

This patch adds the @gmodules_test decorator which test cases
will be decorated with to specify that we're running a gmodules
test. The decorator serves following purposes:

1. Will skip the test on unsupported platforms
2. Add a single debug-info variant to the test-category such that we don't run a gmodules test multiple times

To enable compilation with -gmodules, the MAKE_GMODULES
flag will be needed in the test's Makefile.

Some simple savings stats for ninja check-lldb-api total run-time:

• With patch: ~955 seconds
• Without patch: ~1300 seconds

So ~26% reduction in run-time

[Michael137]

An alternative would be to keep the debug_info category and simply flag it as not to be run by default unless specified explicitly in the test. That way we wouldn't need the MAKE_GMODULES in the Makefile and the special MAKE_DSYM rule in https://reviews.llvm.org/D134345

[Michael137]

• Fix typo in comment

[labath]

I like this a lot. Incidentally, I believe the main reason these tests don't work on non-darwin platforms is because their system libraries are not modularized. If you make the gmodules tests self-contained (which I would recommend, for the sake of reproducibility, anyway), then I think a lot of these tests could run elsewhere as well.

In D134344#3805270, @Michael137 wrote:

An alternative would be to keep the debug_info category and simply flag it as not to be run by default unless specified explicitly in the test. That way we wouldn't need the MAKE_GMODULES in the Makefile and the special MAKE_DSYM rule in https://reviews.llvm.org/D134345

Avoiding the MAKE_GMODULES repetition would definitely be nice, but I might try(*) do it slightly differently:

• keep gmodules as a category, but not a *debug info* category. Among other things this enables running all gmodules tests with the --category gmodules flag.
• teach the debug info replication to ignore tests with the gmodules category (just like it does for @no_debug_info_test_case tests). This step wouldn't be necessary if we made debug info replication opt-in instead of opt-out, as discussed on one of the previous patches (@JDevlieghere might remember which one it was)
• teach buildDefault to build with gmodules enabled in case the test is annotated with the gmodules category.

(*) I'm not entirely sure how this would work out, but I think it should be fine.

[Michael137]

Thanks for reviewing

• keep gmodules as a category, but not a *debug info* category. Among other things this enables running all gmodules tests with the --category gmodules flag.

This should be easy to arrange and was the other alternative I considered. Just removing gmodules from the debug_info_categories list in test_categories.py should suffice

teach buildDefault to build with gmodules enabled in case the test is annotated with the gmodules category.

I assume you're referring to getBuildCommand in builder.py. Currently we're passing it the debug_info category and infer the Makefile flags from it. Will have to see if we can extract the categories attribute there too.

teach the debug info replication to ignore tests with the gmodules category (just like it does for @no_debug_info_test_case tests). This step wouldn't be necessary if we made debug info replication opt-in instead of opt-out, as discussed on one of the previous patches (@JDevlieghere might remember which one it was)

That's an interesting idea. @JDevlieghere @aprantl How much appetite is there for changing the replication to be opt-in (that would require an audit of each API test right?). Otherwise, an alternative that comes to mind without hard-coding a category == gmodules into the replication logic would be to make debug_info_categories a dictionary<category: string, replicable: bool> and keep gmodules in there. Then we wouldn't need to make changes to getBuildCommand either.

[aprantl]

Thanks! As the one who introduced this feature back in the day (and against Pavel's resistance) I've come around to seeing more value in targeted tests than a spray-paint approach of running the entire testsuite. It doesn't catch many interesting issues, since most tests try to avoid external dependencies (that could be built as modules) anyway, and we can write dedicated tests for libcxx and Objective-C modules like Foundation.

[aprantl]

In D134344#3805953, @Michael137 wrote:

teach the debug info replication to ignore tests with the gmodules category (just like it does for @no_debug_info_test_case tests). This step wouldn't be necessary if we made debug info replication opt-in instead of opt-out, as discussed on one of the previous patches (@JDevlieghere might remember which one it was)

That's an interesting idea. @JDevlieghere @aprantl How much appetite is there for changing the replication to be opt-in (that would require an audit of each API test right?). Otherwise, an alternative that comes to mind without hard-coding a category == gmodules into the replication logic would be to make debug_info_categories a dictionary<category: string, replicable: bool> and keep gmodules in there. Then we wouldn't need to make changes to getBuildCommand either.

That's such a big change (we also need to make the change in all downstream branches like swift-lldb) that I probably wouldn't want to roll it into this patch series right now, but I'm open to having a separate discussion about it. But I'm also missing the context as to why this would be desirable, so if there's a good reason, let me know!

[Michael137]

In D134344#3806509, @aprantl wrote:

In D134344#3805953, @Michael137 wrote:

teach the debug info replication to ignore tests with the gmodules category (just like it does for @no_debug_info_test_case tests). This step wouldn't be necessary if we made debug info replication opt-in instead of opt-out, as discussed on one of the previous patches (@JDevlieghere might remember which one it was)

That's an interesting idea. @JDevlieghere @aprantl How much appetite is there for changing the replication to be opt-in (that would require an audit of each API test right?). Otherwise, an alternative that comes to mind without hard-coding a category == gmodules into the replication logic would be to make debug_info_categories a dictionary<category: string, replicable: bool> and keep gmodules in there. Then we wouldn't need to make changes to getBuildCommand either.

That's such a big change (we also need to make the change in all downstream branches like swift-lldb) that I probably wouldn't want to roll it into this patch series right now, but I'm open to having a separate discussion about it. But I'm also missing the context as to why this would be desirable, so if there's a good reason, let me know!

Just to clarify, any solution will have to support the following points (which Pavel mentioned in his suggestion):
(1) Make sure we *don't* run a test with all debug-info variants if we only want gmodules
(2) Be able to explicitly compile with gmodules support when we add the gmodules category to a test

The options that come to mind are:
(A) Currently the test-replication works by checking all the categories on a test-case and if it finds no debug_info categories it will replicate the test once for each variant. If we made gmodules not a debug_info category this means tagging a test with add_test_categories(["gmodules"]) would not work as expected. Pavel suggests making it opt-in, in which case the replication logic would never do the unexpected thing. We would still have to solve (2), but we would get (1) as a consequence of the opt-in (Correct me if I misunderstood @labath).

(B) Keep the gmodules category in the debug_info categories but add an indicator (e.g., by making the debug_info_categories a dictionary) that will skip replication if set. That would solve (1). And (2) would work as it does today without changes.

(C) The approach in this patch. Remove the gmodules category entirely and add a decorator that is responsible for skipping the test on unsupported platforms and adding a debug_info category to the decorated test-case (to prevent replication from kicking in); this addresses issue (1). Unfortunately this also requires the user to add MAKE_GMODULES/MAKE_DSYM to the test's Makefile; that's how this patch addresses the issue of (2), i.e., it bypasses it entirely

[Michael137]

(B) Keep the gmodules category in the debug_info categories but add an indicator (e.g., by making the debug_info_categories a dictionary) that will skip replication if set. That would solve (1). And (2) would work as it does today without changes.

Uploaded alternative diff that implements this option. Seems simpler since tests in the gmodules category Just Work and we don't need to special-case gmodules in several places

https://reviews.llvm.org/D134524

[labath]

In D134344#3805953, @Michael137 wrote:

that would require an audit of each API test right?

Not really. I think this could be a purely mechanical change which replaces NO_DEBUG_INFO_TESTCASE = False with something different. Ideally I'd make that something a "inheriting from a different class". So we could have something like APITestCase and a DebugInfoTestCase (inheriting from the first), and the tests which want debug info replication (one can also imagine different kinds of replication for some other tests) would inherit from the latter.

In D134344#3806509, @aprantl wrote:

But I'm also missing the context as to why this would be desirable, so if there's a good reason, let me know!

I have two reasons for that:
The first is that from a simply engineering perspective, doing it the other way around seems cleaner, as now we kind of have two ways to avoid replication. Either you don't inherit from the replicated test base clase (all lldb-server and lldb-vscode tests do that), or you do, but then mark yourself as NO_DEBUG_INFO_TESTCASE.
Secondly, it seems to be that no-replication is a better default. We have a lot of features that don't (or shouldn't) depend on the kind of debug info we're using, and we're probably forgetting to add this attribute to some of them. It's possible those tests are adding some marginal debug info coverage, but it's hard to rely on that, because noone know what that is. So I'd say that an opt-in is a better default (particularly for the tests we're adding nowadays), and the replication should be done when you know you're doing something debug-heavy.
I also think that having a more opt-in mechanism could enable us to do *more* replication. For example, I think that running the some tests in both DWARF v4 and v5 modes would be interesting, but I definitely wouldn't want to run all of them, all the time.

In D134344#3811091, @Michael137 wrote:

(B) Keep the gmodules category in the debug_info categories but add an indicator (e.g., by making the debug_info_categories a dictionary) that will skip replication if set. That would solve (1). And (2) would work as it does today without changes.

Uploaded alternative diff that implements this option. Seems simpler since tests in the gmodules category Just Work and we don't need to special-case gmodules in several places

https://reviews.llvm.org/D134524

It's not exactly how I was imagining this, but I like it. :)

[Michael137]

Abandoning in favour of https://reviews.llvm.org/D134524

[Michael137]

In D134344#3811143, @labath wrote:

In D134344#3805953, @Michael137 wrote:

that would require an audit of each API test right?

Not really. I think this could be a purely mechanical change which replaces NO_DEBUG_INFO_TESTCASE = False with something different. Ideally I'd make that something a "inheriting from a different class". So we could have something like APITestCase and a DebugInfoTestCase (inheriting from the first), and the tests which want debug info replication (one can also imagine different kinds of replication for some other tests) would inherit from the latter.

In D134344#3806509, @aprantl wrote:

But I'm also missing the context as to why this would be desirable, so if there's a good reason, let me know!

I have two reasons for that:
The first is that from a simply engineering perspective, doing it the other way around seems cleaner, as now we kind of have two ways to avoid replication. Either you don't inherit from the replicated test base clase (all lldb-server and lldb-vscode tests do that), or you do, but then mark yourself as NO_DEBUG_INFO_TESTCASE.
Secondly, it seems to be that no-replication is a better default. We have a lot of features that don't (or shouldn't) depend on the kind of debug info we're using, and we're probably forgetting to add this attribute to some of them. It's possible those tests are adding some marginal debug info coverage, but it's hard to rely on that, because noone know what that is. So I'd say that an opt-in is a better default (particularly for the tests we're adding nowadays), and the replication should be done when you know you're doing something debug-heavy.
I also think that having a more opt-in mechanism could enable us to do *more* replication. For example, I think that running the some tests in both DWARF v4 and v5 modes would be interesting, but I definitely wouldn't want to run all of them, all the time.

In D134344#3811091, @Michael137 wrote:

(B) Keep the gmodules category in the debug_info categories but add an indicator (e.g., by making the debug_info_categories a dictionary) that will skip replication if set. That would solve (1). And (2) would work as it does today without changes.

Uploaded alternative diff that implements this option. Seems simpler since tests in the gmodules category Just Work and we don't need to special-case gmodules in several places

https://reviews.llvm.org/D134524

It's not exactly how I was imagining this, but I like it. :)

We could perhaps move the discussion to discourse. I think the points raised are worth exploring further

lldb/packages/Python/lldbsuite/test/decorators.py

 import unittest2
 
 # LLDB modules
 import lldb
 from . import configuration
 from . import test_categories
 from . import lldbtest_config
 from lldbsuite.support import funcutils
+from lldbsuite.support import gmodules
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbplatformutil
 
-
 class DecorateMode:
     Skip, Xfail = range(2)
 
 
 # You can use no_match to reverse the test of the conditional that is used to match keyword
 # arguments in the skip / xfail decorators.  If oslist=["windows", "linux"] skips windows
 # and linux, oslist=no_match(["windows", "linux"]) skips *unless* windows
 # or linux.
def should_skip_simulator_test():
                 return None
             else:
                 return "%s simulator is not supported on this system." % platform
         except subprocess.CalledProcessError:
             return "Simulators are unsupported on this system (xcodebuild failed)"
 
     return skipTestIfFn(should_skip_simulator_test)
 
+def gmodules_test(func):
+    """
+    Use this decorator for 'gmodules'-specific tests. This decorator
+    makes sure we skip the test if 'gmodules' is unsupported on the
+    host platform and adds a single debug_info category to the test
+    so we don't run it multiple times (i.e., once for each debug_info variant).
+    """
+    def should_skip_gmodules_test(self):
+        supported_platforms = ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]
+        platform = lldbplatformutil.getHostPlatform()
+        compiler_path = self.getCompiler()
+        if platform not in supported_platforms:
+            return "Tests not supported on " % lldbplatformutil.getHostPlatform()
+        elif not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+            return "-gmodules option is not supported on " % compiler_path
+        else:
+            return None
+
+    return skipTestIfFn(should_skip_gmodules_test)(add_test_categories(["dwarf"])(func))
 
 def debugserver_test(func):
     """Decorate the item as a debugserver test."""
     return add_test_categories(["debugserver"])(func)
 
 
 def llgs_test(func):
     """Decorate the item as a lldb-server test."""