[intel-pt] Improve the way the test determines whether to run

[wallace]

Summary

@labath raised a concern on the way I was skipping this test. I think that was
fair and I found a better way.
Now I'm skipping if the CMAKE flag LLDB_BUILD_INTEL_PT is false.
I added an enabled_plugins entry in the dotest configuration, which gets
set by lit or lldb-dotest. The only available plugin right now is
'intel-pt', but I imagine it will be useful in the future for other
kinds of plugins that get determined at configuration time. I didn't
want to add a new argument option --enable-intel-pt or something or the
sort, as it wouldn't be useful for other cases.

[wallace]

improve description

[labath]

Checking for presence this way seems ok-ish (definitely better than the environment variable). A possible alternative would be to pass some argument to dotest from cmake which would identify the status of the pt support.

Anyway, if you're going to add a new public API for this purpose, please create a separate patch for that with a test.

[wallace]

See the new description of the diff for the updates

[labath]

Adding @JDevlieghere as he is in the same time zone and knows about all this stuff. This is on the right track, but changing lldb-dotest is not enough -- you'll also need to change test/API/lit.cfg.py as that's what's used for regular "check-lldb" runs. Also, instead of environment variables, please add a command line switch to dotest. Environment variables make it hard to reproduce steps when copy-pasting commands.

lldb/utils/lldb-dotest/lldb-dotest.in

33

The usual solution to this is to run llvm_canonicalize_cmake_booleans in cmake.

[wallace]

Addressed comments. The diff description now reflects the latest changes.

[wallace]

remove blank line

[labath]

Heh.. I wasn't an attempt expecting a fully generic solution. Since we don't invoke dotest.py manually these days (we have lldb-dotest for that) making a separate argument specifically for this plugin would be just fine. However, I don't see anything inherently wrong with this generic approach, so I guess we can leave it. The main think I don't like is the introduction of the skipping code into the base test class. These classes are already more complicated than they ought to be, so I'd like to avoid adding things to them, if those things can be implemented elsewhere.

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

736–738 ↗

(On Diff #256716)

Let's put this stuff in TestIntelPTSimpleBinary for now -- it can be moved into a separate base class (along with other common code) once we have more of these tests.

[wallace]

move the skipping logic to the actual test

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

 
 # The names of all tests. Used to assert we don't have two tests with the
 # same base name.
 all_tests = set()
 
 # LLDB library directory.
 lldb_libs_dir = None
 
+# A plugin whose tests will be enabled, like intel-pt.
+enabled_plugins = []
+
 
 def shouldSkipBecauseOfCategories(test_categories):
     if use_categories:
         if len(test_categories) == 0 or len(
                 categories_list & set(test_categories)) == 0:
             return True
 
     for category in skip_categories:

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

else:
         configuration.clang_module_cache_dir = os.path.join(
             configuration.test_build_dir, 'module-cache-clang')
 
     os.environ['CLANG_MODULE_CACHE_DIR'] = configuration.clang_module_cache_dir
 
     if args.lldb_libs_dir:
         configuration.lldb_libs_dir = args.lldb_libs_dir
 
+    if args.enabled_plugins:
+        configuration.enabled_plugins = args.enabled_plugins
+
     # Gather all the dirs passed on the command line.
     if len(args.args) > 0:
         configuration.testdirs = [os.path.realpath(os.path.abspath(x)) for x in args.args]
 
     lldbtest_config.codesign_identity = args.codesign_identity
 
 
 def setupTestResults():

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

group.add_argument(
         dest='clang_module_cache_dir',
         metavar='The clang module cache directory used by Clang',
         help='The clang module cache directory used in the Make files by Clang while building tests. Defaults to <test build directory>/module-cache-clang.')
     group.add_argument(
         '--lldb-libs-dir',
         dest='lldb_libs_dir',
         metavar='path',
         help='The path to LLDB library directory (containing liblldb)')
+    group.add_argument(
+        '--enable-plugin',
+        dest='enabled_plugins',
+        action='append',
+        type=str,
+        metavar='A plugin whose tests will be enabled',
+        help='A plugin whose tests will be enabled. The only currently supported plugin is intel-pt.')
 
     # Configuration options
     group = parser.add_argument_group('Remote platform options')
     group.add_argument(
         '--platform-name',
         dest='lldb_platform_name',
         metavar='platform-name',
         help='The name of a remote platform to use')

lldb/test/API/lit.cfg.py

if config.dsymutil:
   dotest_cmd += ['--dsymutil', config.dsymutil]
 
 if config.filecheck:
   dotest_cmd += ['--filecheck', config.filecheck]
 
 if config.lldb_libs_dir:
   dotest_cmd += ['--lldb-libs-dir', config.lldb_libs_dir]
 
+if config.enabled_plugins:
+  for plugin in config.enabled_plugins:
+    dotest_cmd += ['--enable-plugin', plugin]
+
 # We don't want to force users passing arguments to lit to use `;` as a
 # separator. We use Python's simple lexical analyzer to turn the args into a
 # list. Pass there arguments last so they can override anything that was
 # already configured.
 if config.dotest_lit_args_str:
   dotest_cmd.extend(shlex.split(config.dotest_lit_args_str))
 
 
 # Load LLDB test format.
 sys.path.append(os.path.join(config.lldb_src_root, "test", "API"))
 import lldbtest
 
 # testFormat: The test format to use to interpret tests.
 config.test_format = lldbtest.LLDBTest(dotest_cmd)

lldb/test/API/lit.site.cfg.py.in

 config.test_arch = '@LLDB_TEST_ARCH@'
 config.test_compiler = '@LLDB_TEST_COMPILER@'
 config.dsymutil = '@LLDB_TEST_DSYMUTIL@'
 config.filecheck = '@LLDB_TEST_FILECHECK@'
 # The API tests use their own module caches.
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-api")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-api")
 
+# Plugins
+lldb_build_intel_pt = '@LLDB_BUILD_INTEL_PT@'
+if lldb_build_intel_pt == '1':
+    config.enabled_plugins = ['intel-pt']
+
 # Additional dotest arguments can be passed to lit by providing a
 # semicolon-separates list: --param dotest-args="arg;arg".
 dotest_lit_args_str = lit_config.params.get('dotest-args', None)
 if dotest_lit_args_str:
     config.dotest_lit_args_str = dotest_lit_args_str
 
 # Support substitution of the tools and libs dirs with user parameters. This is
 # used when we can't determine the tool dir at configuration time.

lldb/test/API/tools/intel-features/intel-pt/test/TestIntelPTSimpleBinary.py

 from __future__ import print_function
 
 import os
 import lldb
 import time
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
+from lldbsuite.test import configuration
 
 
 class TestIntelPTSimpleBinary(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
     NO_DEBUG_INFO_TESTCASE = True
 
+    def setUp(self):
+        TestBase.setUp(self)
+
+        if 'intel-pt' not in configuration.enabled_plugins:
+            self.skipTest("The '" + self.plugin + "' test plugin is not enabled")
+
+        plugin_path = os.path.join(os.environ["LLDB_IMPLIB_DIR"], "liblldbIntelFeatures.so")
+        self.runCmd("plugin load " + plugin_path)
+
     @skipIf(oslist=no_match(['linux']))
     @skipIf(archs=no_match(['i386', 'x86_64']))
     @skipIfRemote
     def test_basic_flow(self):
         """Test collection, decoding, and dumping instructions"""
-        if os.environ.get('TEST_INTEL_PT') != '1':
-            self.skipTest("The environment variable TEST_INTEL_PT=1 is needed to run this test.")
-
-        lldb_exec_dir = os.environ["LLDB_IMPLIB_DIR"]
-        lldb_lib_dir = os.path.join(lldb_exec_dir, os.pardir, "lib")
-        plugin_file = os.path.join(lldb_lib_dir, "liblldbIntelFeatures.so")
 
         self.build()
-
-        self.runCmd("plugin load " + plugin_file)
-
         exe = self.getBuildArtifact("a.out")
         lldbutil.run_to_name_breakpoint(self, "main", exe_name=exe)
         # We start tracing from main
         self.runCmd("processor-trace start all")
 
         # We check the trace after the for loop
         self.runCmd("b " + str(line_number('main.cpp', '// Break 1')))
         self.runCmd("c")
 
         # We wait a little bit to ensure the processor has send the PT packets to
         # the memory
         time.sleep(.1)
 
         # We find the start address of the 'fun' function for a later check
         target = self.dbg.GetSelectedTarget()
         fun_start_adddress = target.FindFunctions("fun")[0].GetSymbol() \
             .GetStartAddress().GetLoadAddress(target)
 
         # We print the last instructions
         self.expect("processor-trace show-instr-log -c 100",
             patterns=[
                 # We expect to have seen the first instruction of 'fun'
-                hex(fun_start_adddress),  
+                hex(fun_start_adddress),
                 # We expect to see the exit condition of the for loop
-                "at main.cpp:" + str(line_number('main.cpp', '// Break for loop')) 
+                "at main.cpp:" + str(line_number('main.cpp', '// Break for loop'))
             ])
 
         self.runCmd("processor-trace stop")

lldb/test/CMakeLists.txt

 endif()
 
 if (CMAKE_SIZEOF_VOID_P EQUAL 8)
   set(LLDB_IS_64_BITS 1)
 endif()
 
 # These values are not canonicalized within LLVM.
 llvm_canonicalize_cmake_booleans(
+  LLDB_BUILD_INTEL_PT
   LLDB_ENABLE_PYTHON
   LLDB_ENABLE_LUA
   LLDB_ENABLE_LZMA
   LLVM_ENABLE_ZLIB
   LLVM_ENABLE_SHARED_LIBS
   LLDB_IS_64_BITS)
 
 # Configure the individual test suites.

lldb/utils/lldb-dotest/CMakeLists.txt

 # Make lldb-dotest a custom target.
 add_custom_target(lldb-dotest)
 add_dependencies(lldb-dotest lldb-test-deps)
 set_target_properties(lldb-dotest PROPERTIES FOLDER "lldb utils")
 
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 set(LLDB_LIBS_DIR "${LLVM_LIBRARY_OUTPUT_INTDIR}")
 
+llvm_canonicalize_cmake_booleans(
+  LLDB_BUILD_INTEL_PT
+)
+
 # Generate lldb-dotest Python driver script for each build mode.
 if(LLDB_BUILT_STANDALONE)
   set(config_types ".")
   if(CMAKE_CONFIGURATION_TYPES)
     set(config_types ${CMAKE_CONFIGURATION_TYPES})
   endif()
   foreach(config_type ${config_types})
     # In paths to our build-tree, replace CMAKE_CFG_INTDIR with our actual configuration names.

lldb/utils/lldb-dotest/lldb-dotest.in

 #!@PYTHON_EXECUTABLE@
 import subprocess
 import sys
 
 dotest_path = '@LLDB_SOURCE_DIR_CONFIGURED@/test/API/dotest.py'
 build_dir = '@LLDB_TEST_BUILD_DIRECTORY_CONFIGURED@'
 dotest_args_str = '@LLDB_DOTEST_ARGS_CONFIGURED@'
 arch = '@LLDB_TEST_ARCH@'
 executable = '@LLDB_TEST_EXECUTABLE_CONFIGURED@'
 compiler = '@LLDB_TEST_COMPILER_CONFIGURED@'
 dsymutil = '@LLDB_TEST_DSYMUTIL_CONFIGURED@'
 filecheck = '@LLDB_TEST_FILECHECK_CONFIGURED@'
 lldb_libs_dir = "@LLDB_LIBS_DIR_CONFIGURED@"
+lldb_build_intel_pt = "@LLDB_BUILD_INTEL_PT@"
 
 if __name__ == '__main__':
     wrapper_args = sys.argv[1:]
     dotest_args = dotest_args_str.split(';')
     # Build dotest.py command.
     cmd = [sys.executable, dotest_path]
     cmd.extend(['--arch', arch])
     cmd.extend(dotest_args)
     cmd.extend(['--build-dir', build_dir])
     cmd.extend(['--executable', executable])
     cmd.extend(['--compiler', compiler])
     cmd.extend(['--dsymutil', dsymutil])
     cmd.extend(['--filecheck', filecheck])
     cmd.extend(['--lldb-libs-dir', lldb_libs_dir])
+    if lldb_build_intel_pt == "1":
+        cmd.extend(['--enable-plugin', 'intel-pt'])
     cmd.extend(wrapper_args)
     # Invoke dotest.py and return exit code.
     print(' '.join(cmd))
     sys.exit(subprocess.call(cmd))