This is an archive of the discontinued LLVM Phabricator instance.

[Dexter] Remove false requirement of lldb for dexter regression tests on Windows
ClosedPublic

Authored by Orlando on Jan 24 2022, 8:18 AM.

Details

Summary

Not quite NFC because a little work was required to configure some tests to run
on Windows at all.

Before this patch on Windows:

$ llvm-lit cross-project-tests\debuginfo-tests\dexter\feature-tests
   Unsupported: 49
   Passed     : 23

After this patch on Windows:

$ llvm-lit cross-project-tests\debuginfo-tests\dexter\feature-tests
   Unsupported      : 27
   Passed           : 39
   Expectedly failed:  6

There are 3 main changes here. The first is to add a few more substitutions in
cross-project-tests/lit.cfg.py so that tests need to use specific flags can
still use the dexter regression test defaults for the native platform. These
are:

%dexter_regression_test_debugger
%dexter_regression_test_builder
%dexter_regression_test_cflags
%dexter_regression_test_ldflags

Tests that now use these options and therefore can be run on Windows too
(though the second is still failing for unknown reasons):

cross-project-tests/debuginfo-tests/dexte/feature_tests
    /subtools/clang-opt-bisect/clang-opt-bisect.cpp
    /subtools/test/source-root-dir.cpp

The second change is to remove spurious REQUIRES: system-linux, lldb and
UNSUPPORTED: system-windows directives, and make changes to lit.local.cfg
files that have the same effect. I've also added comments to the genuine
REQUIRES, UNSUPPORTED, and XFAIL directives so it's easier to understand
requirements at a glance. The most common reason for a test to not be supported
on Windows is that it uses DexLimitSteps, DexDeclareAddress, or DexCommandLine,
none of which are supported in the dbgeng driver.

There are two failures on Windows that were previously hidden, which I've
XFAILed:

cross-project-tests/debuginfo-tests/dexter/feature_tests
    /commands/perfect/dex_finish_test/default_conditional.cpp
    /commands/perfect/dex_finish_test/default_conditional_hit_count.cpp

And two that were easy to fix:

cross-project-tests/debuginfo-tests/dexter/feature_tests
    /commands/perfect/dex_finish_test/default_simple.cpp
    /commands/perfect/dex_finish_test/default_hit_count.cpp

Lastly, I've set three directories as unsupported.

cross-project-tests/debuginfo-tests/dexter/feature_tests
    /commands/perfect/limit_steps
    /commands/perfect/dex_declare_address
    /commands/perfect/dex_declare_file

The first two are unsupported on Windows because they contains tests for the
DexLimitSteps and DexDeclareAddress commands which aren't supported in the
dbgeng driver. The third is unsupported on all platforms as the tests involve
invoking clang directly, which isn't currently a supported way of building
tests for dexter in lit (it can cause problems for cross compilers that can
target the host, as the tests use the default triple and linker, which may
be aligned for the default target, not host).

Tested on Windows and Linux.

Diff Detail

Event Timeline

Orlando created this revision.Jan 24 2022, 8:18 AM
Orlando requested review of this revision.Jan 24 2022, 8:18 AM
ormris removed a subscriber: ormris.Jan 24 2022, 10:58 AM

LGTM, with a couple of nits - good work!

cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/lit.local.cfg
2

substituion -> substitution

cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/clang-opt-bisect/clang-opt-bisect.cpp
8–9

Very tiny nit, but is there a reason for the "=" on these lines and not the others?

cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_limit_steps_no_values.cpp
10

Nit, keep the newline unless there's an important reason for it.

jmorse accepted this revision.Jan 26 2022, 2:29 AM

SGTM with a question inline.

cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/lit.local.cfg
2–4

To be clear, we're making this unsupported everywhere because it won't work on our public buildbot, yes? This seems like a reasonable trade-off to me (some public coverage now vs no public coverage now), just checking the reasoning.

This revision is now accepted and ready to land.Jan 26 2022, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 3:34 AM

Folded nit-fixes into the commit, thanks both.

cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/lit.local.cfg
2–4

Yep, that's right

cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/lit.local.cfg