Page MenuHomePhabricator

[RFC] Adopt Dexter and use it to run debuginfo-tests
AbandonedPublic

Authored by jmorse on Oct 9 2019, 8:25 AM.

Details

Reviewers
None
Summary

This is a patch demonstrating the changes we'd like to make for an RFC I'm about to send to llvm-dev@, to use the Dexter tool for running debuginfo tests. Broadly, this patch:

  • Imports the Dexter codebase (written in python) from https://github.com/snsystems/dexter into the debuginfo-tests directory
  • Converts a variety of old {ll,g}db tests to be runnable by Dexter and drops them in the debuginfo-tests/dexter-tests directory
  • Does similar for a bunch of CDB / dbgeng tests
  • Glues these things in so that all (the supported) tests run when one runs 'ninja check-debuginfo' or check-all.

More context in the email, which I'll link here once it's sent.

You can also browse the tree at https://github.com/jmorse/llvm-project/tree/dexter-rfc

Diff Detail

Event Timeline

jmorse created this revision.Oct 9 2019, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 8:25 AM
rnk added a subscriber: rnk.Mon, Oct 14, 3:48 PM

I looked at some of the converted cdb tests, and it looks pretty nice. Thanks for working on this! :)

In the long run, we could try to standardize on using the clang/clang++ command line syntax on Windows if we want to. It would make the tests more similar to the GDB and LLDB tests.

debuginfo-tests/dexter-tests/inline-line-gap.cpp
42–46

This is really cool.

debuginfo-tests/dexter/dex/debugger/dbgeng/README.md
14–15

Nice.

Thanks for working on this!

Is the code in LLVM going to be the new canonical copy of dexter?

Hi Nico ,

Is the code in LLVM going to be the new canonical copy of dexter?

Yes -- I think one of the wins in Dexter is making debug-experience testing common across platforms and formats, permanently contributing it to the community (by making the LLVM copy canonical) helps secure that.

jmorse updated this revision to Diff 226928.Tue, Oct 29, 10:18 AM

This update:

  • Correctly diffs against the base commit. The previous one missed some uninteresting file moves (because I used '..' instead of '...'...), this patch now cleanly applies.
  • Patches up some tests with the wrong ldflags (asan needed to be turned on)
  • XFails one test where, on Ubuntu 18.04 and lldb-8, a memory access by lldb causes an asan signal in the debugged process. I'll dig into that later.
  • Insist on finding a python3 interpreter and uses it.
  • Prefer an LLDB binary in the build tree to anything else in the PATH

I'll post a sitrep comment here shortly and add some people as subscribers.

Add some debuginfo subscribers; please add more people who might be interested, I didn't get everyones name at the conference. Alternately this might be more appropriate on the mailing list, I can move it there.

All the feedback at the conference was very positive, thanks all. The question now is how to land this, on my mind are four topics:

  • Python python python python
  • Buildbots
  • Code maintenance
  • Test convergence

Python: We focused on python 3 only, seeing how python 2 expires in 63 days. This might put Dexter in the unfortunate position of pushing people to upgrade. Please observe the contents of CMakeLists.txt: in its current state, this patch will require python3 to run the debuginfo tests, but should otherwise work alongside LLVM using python2 elsewhere. AFAIUI usage of the debuginfo-tests is currently low, my major concern is the buildbots.

Buildbots: From a quick look, I believe only green-dragon and clang-x64-windows-msvc (@rnk s?) run the debuginfo tests. I know little about the windows buildbot, but it should be straight forwards (TM) for it to use python3. green-dragon definitely has python3 installed because it builds lldb with it. There's a difficulty with green dragon though, I believe it checks debuginfo-tests _before_ building lldb. In its current form, Dexter will pick up the system lldb and use that if there isn't another available. If the system lldb wasn't built for python3, things will go wrong. (I imagine the only way to find all buildbot related problems is going to be pushing up this patch and seeing what breaks).

Code maintenance: Do we need to designate a code owner here? Contributions from the rest of the community is exactly what we want, and tools like llvm-lit don't seem to have code owners. On the other hand having a code owner guarantees someone will review patches, which we / I are happy to do.

Test convergence: right now all the test are pinned to a specific OS and debugger. I'm confident that these limits can be removed and debuggers autoselected, with XFails / unsupporteds added where there are known limitations to platform debuginfo / limitations to the debugger. I'd prefer pushing up the pinned tests as they are then expanding them, as that allows an incremental approach. Note that everything marked "system-linux" is purely because we don't have a mac immediately available to test these things on, we could probably push them up supporting Darwin too.

tl;dr: I think this is good to go, but as we're adding python things there'll probably be buildbot breakage. I'd be happy to push/revert until we've incrementally found all the issues and it sticks.

From the Apple side I have no concerns with this being Python 3 only.

rnk added a comment.Tue, Oct 29, 5:23 PM

Buildbots: From a quick look, I believe only green-dragon and clang-x64-windows-msvc (@rnk s?) run the debuginfo tests. I know little about the windows buildbot, but it should be straight forwards (TM) for it to use python3.

I suspect Python 3 may not be installed there yet. We can easily enable/disable the test suite by removing the check-debuginfo-tests target here:
https://github.com/llvm/llvm-zorg/blob/master/zorg/buildbot/builders/annotated/clang-windows.py#L19
You could land and hope for the best, and if you can't fix it forward, remove the check until later, or we could assume that it will fail and remove that target from the script ahead of time.

Either way, don't let this get blocked on that bot. I'm a bit busy this week, so I'm not sure I'll find time to patch in dexter and run it locally or on the bot in question.

Cool: in that case I'll push up a commit early tomorrow when the buildbots are calm, and will see how it goes. Cheers!

Sitrep: I pushed up f78c236efda8 and the windows buildbot seemed to handle Dexter fine, although I didn't see any evidence that it was actually running it. There are no cmake reports about finding python 3 in the cmake reports here [0] for example, but the cmake report seems suspiciously quiet anyway.

Green dragon appeared to get quite far, but not as far as running the tests, instead some interaction with clang-python integration tests occurred [1], I'll try to replicate that locally.

[0] http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11839
[1] http://green.lab.llvm.org/green/job/clang-stage1-RA/3372/console

Please don't revert it, we're working on fixing green dragon by removing some no longer needed Python 2.7 workarounds.

Curses, I was too quick on the trigger -- without objection I'll push it back up in ~10 minutes

rnk added a comment.Thu, Oct 31, 10:35 AM

The Windows bot must be running stale debug info tests. These logs show only 24 tests, which seems wrong:

[1/4] Running debug info integration tests
-- Testing: 24 tests, 24 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

I'll go debug it.

I got it building now. Here's the first real error:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/3211/console

********************
FAIL: debuginfo-tests :: dexter/feature_tests/subtools/test/err_paren_mline.cpp (38 of 53)
******************** TEST 'debuginfo-tests :: dexter/feature_tests/subtools/test/err_paren_mline.cpp' FAILED ********************
Script:
--
: 'RUN: at line 10';   not /usr/local/bin/python3 /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/debuginfo-tests/dexter/dexter.py test --lldb-executable /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/lldb test --builder 'clang' --debugger "lldb"      --cflags "-O0 -g" -v -- /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/debuginfo-tests/dexter/feature_tests/subtools/test/err_paren_mline.cpp      | /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/FileCheck /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/debuginfo-tests/dexter/feature_tests/subtools/test/err_paren_mline.cpp --match-full-lines --strict-whitespace
--
Exit Code: 2
Hide Details


Command Output (stderr):
--
Hide Details


error: unrecognized argument: '--'
       unrecognized argument: '/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/debuginfo-tests/dexter/feature_tests/subtools/test/err_paren_mline.cpp'
Hide Details


usage: DExTer test [-h] [--no-color-output] [--time-report] [-v] [-V] [-w]
                   [--unittest {off,show-failures,show-all}]
                   [--working-directory <file>] [--save-temps]
                   [--fail-lt <float>] [--binary <file>]
                   [--builder {clang,clang-c}] [--cflags CFLAGS]
                   [--ldflags LDFLAGS] [--lldb-executable <file>] --debugger
                   {dbgeng,lldb,vs2015,vs2017} [--max-steps <int>]
                   [--pause-between-steps <seconds>] [--show-debugger]
                   [--arch <architecture>]
                   [--penalty-variable-optimized <int>]
                   [--penalty-misordered-values <int>]
                   [--penalty-irretrievable <int>]
                   [--penalty-not-evaluatable <int>]
                   [--penalty-missing-values <int>]
                   [--penalty-incorrect-values <int>]
                   [--penalty-unreachable <int>]
                   [--penalty-misordered-steps <int>]
                   [--penalty-missing-step <int>]
                   [--penalty-incorrect-program-state <int>]
                   [--results-directory <directory>]
                   [<test-path>]
FileCheck error: '-' is empty.
FileCheck command line:  /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/FileCheck /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/debuginfo-tests/dexter/feature_tests/subtools/test/err_paren_mline.cpp --match-full-lines --strict-whitespace

@jmorse We are running Python 3.7.0. Is whatever argparse mechanism dexter uses depending on some earlier/newer behavior?

The -- argument is baked into the test.

I don't have time to investigate this right now so I XFAILed the tests on Darwin in 8e406204418895f7b09d1a9a3f8037e741a43968. Perhaps the solution is obvious to you, let me know if it isn't, it would be great to revert the XFAILs again!

gbedwell added a subscriber: gbedwell.EditedThu, Oct 31, 1:57 PM

(quoting Jeremy's on-list reply)

Hi,

I'm having auth issues with phab and google accounts right now, so
can't reply on phab,

Thanks for pushing through the python 3 issues!,

On Thu, Oct 31, 2019 at 6:28 PM Adrian Prantl via Phabricator
<reviews@reviews.llvm.org> wrote:
> I don't have time to investigate this right now so I XFAILed the tests on Darwin in 8e406204418895f7b09d1a9a3f8037e741a43968 https://reviews.llvm.org/rG8e406204418895f7b09d1a9a3f8037e741a43968. Perhaps the solution is obvious to you, let me know if it isn't, it would be great to revert the XFAILs again!

Exciting -- it's just argparse plus a wrapper, and we've been using
python 3.6 so I wouldn't expect python 3.7 to regress. The "--" is the
positional-argument separator (bottom of the section in [0])
indicating that everything following is a positional argument.

Technically all those '--' separators can be deleted as we're not
doing anything advanced with command lines. It's odd that this proves
to be an incompatibility though.

[0] https://docs.python.org/dev/library/argparse.html#arguments-containing

  • Thanks, Jeremy

I suspect what we're seeing is that something earlier in the command line is being consumed as a positional argument, then we get some --optional arguments and so it's unhappy to see further positional arguments later on. E.g.

$ type 1.py
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('positional', nargs='+')
parser.add_argument('--optional', action='store_true')
parser.parse_args()

$ py -3.6 1.py pos1 pos2 --optional -- pos3
usage: 1.py [-h] [--optional] positional [positional ...]
1.py: error: unrecognized arguments: -- pos3

$

Hopefully we should be able to figure out what's going on tomorrow. If worst comes to worst we can temporarily add a bit of logging to the error message easily enough that should shed some light.

@aprantl et al: Thanks for your help on this!

-Greg

jmorse added a comment.Fri, Nov 1, 6:07 AM

The lldb-cmake pipeline is now running those internal tests successfully with 7f738c811ac4, which is great. I'll enable a few of the 'dexter-tests' tests for darwin and see how that goes.

I'm a bit worried about this [1] green-dragon pipeline -- it started failing when dexter landed, with this message:

cd /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/clang/bindings/python && /usr/local/Cellar/cmake/3.12.4/bin/cmake -E env CLANG_LIBRARY_PATH=/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/lib -m unittest discover
cmake -E env: unknown option '-m'

In previous runs a path to a python executable appeared in that command; it's probably not appearing now because I'm unsetting PYTHON_EXECUTABLE after finding python3, similar to Reids problem in D69684. It's not clear to me what the solution may be -- possibly running FindPythonInterp after unsetting, or save/restoring the configuration instead of wiping it? How CMake files interact with each other isn't my area of expertise, alas.

[0] http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/
[1] http://green.lab.llvm.org/green/job/clang-stage1-RA/

jmorse added a comment.Fri, Nov 1, 7:55 AM

I've unmasked the "dexter-tests" tests (REQUIRES: system-linux -> UNSUPPORTED: system-windows) and after a few faults, those Dexter tests run fine on Darwin. Making the Windows tests work on unix-like systems requires a little massaging, and standardising the clang command line / autoselecting debugger still needs implementing, something for next week I think.

In the meantime there's only the python3 concerns that

jmorse added a comment.Fri, Nov 1, 8:07 AM

In the meantime there's only the python3 concerns that

... could lead to this being backed out.

I believe I got that bot working again by just removing debuginfo-tests from the list of projects. It wasn't *running* the debuginfo-tests tests anyway.

jmorse abandoned this revision.Mon, Nov 4, 1:31 AM

Ah well, in that case I think this counts as merged, thanks all!

I'll get our bunch to consider adding integration tests when fixing things in the future.