This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wallace on Apr 3 2020, 7:46 PM.

Details

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.

Diff Detail

Event Timeline

wallace created this revision.Apr 3 2020, 7:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 7:46 PM
wallace updated this revision to Diff 254990.Apr 3 2020, 7:48 PM
wallace edited the summary of this revision. (Show Details)

improve description

labath added a comment.Apr 6 2020, 2:02 AM

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 updated this revision to Diff 255445.Apr 6 2020, 12:34 PM
wallace edited the summary of this revision. (Show Details)

See the new description of the diff for the updates

labath added a subscriber: JDevlieghere.

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
32

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

wallace updated this revision to Diff 256715.Apr 10 2020, 5:21 PM
wallace edited the summary of this revision. (Show Details)
wallace removed a reviewer: JDevlieghere.
wallace removed a subscriber: JDevlieghere.

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

wallace updated this revision to Diff 256716.Apr 10 2020, 5:22 PM

remove blank line

Harbormaster completed remote builds in B52769: Diff 256716.

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 updated this revision to Diff 257450.Apr 14 2020, 12:32 PM

move the skipping logic to the actual test

labath accepted this revision.Apr 15 2020, 12:25 AM
This revision is now accepted and ready to land.Apr 15 2020, 12:25 AM
This revision was automatically updated to reflect the committed changes.