Page MenuHomePhabricator

Add test for GDB pretty printers.
ClosedPublic

Authored by csigg on Jan 7 2020, 1:56 AM.

Diff Detail

Event Timeline

csigg created this revision.Jan 7 2020, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 1:56 AM

Unit tests: pass. 61175 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

csigg marked an inline comment as done.Jan 7 2020, 2:15 AM
csigg added inline comments.
llvm/utils/gdb-scripts/prettyprinters.py
322 ↗(On Diff #236535)

Note: this change is from https://reviews.llvm.org/D72136, which I will merge first.

csigg updated this revision to Diff 236538.Jan 7 2020, 2:21 AM

Fix variable name casing.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Seems OK to me, but @aprantl - could you take a look too since I don't often use/interact with the debuginfo-tests. Do you think this is something worth having/appropriate to have there?

debuginfo-tests/gdb-tests/prettyprinters.cpp
20 ↗(On Diff #236538)

attribute((optnone)) might be tidier

csigg updated this revision to Diff 236831.Jan 8 2020, 8:03 AM

O0 > optnone

Unit tests: pass. 61175 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This is cute. We should do something similar with the LLDB pretty printers.

  1. What do you think about putting this into an llvm subdirectory, to make clear that this is a test of llvm/utils rather than generic debug info as all the other tests?
  2. We are gradually moving away from test_debuginfo.pl in favor of dexter. Have you looked at whether this could be implemented using dexter?
csigg added a comment.Jan 8 2020, 11:43 PM
  1. What do you think about putting this into an llvm subdirectory, to make clear that this is a test of llvm/utils rather than generic debug info as all the other tests?

Do you mean something like debuginfo-tests/llvm-prettyprinters/? Or 'llvm/tests/prettyprinter'?

  1. We are gradually moving away from test_debuginfo.pl in favor of dexter. Have you looked at whether this could be implemented using dexter?

I didn't look any further when I saw that dexter doesn't support GDB (according to the readme).
Not sure if we need to use a debugger abstraction because the pretty printers aren't standardized and therefore the tests will be pretty debugger specific.
If you would like to move away from test_debuginfo.pl, maybe I should split the C++ code and gdb commands+CHECKS into two different files manually. It's not perfect FileCheck methodology, but for a single test in a subdirectory it should be manageable.

aprantl added a subscriber: jmorse.Jan 9 2020, 9:11 AM

What about debuginfo-tests/llvm-prettyprinters/gdb/ and then we'll create an lldb subdirectory next to later?

I didn't look any further when I saw that dexter doesn't support GDB (according to the readme).

CC @jmorse

I forgot about that. I believe there exists a binding but it hasn't been upstreamed yet.

csigg updated this revision to Diff 237308.Jan 10 2020, 6:57 AM

Move to different directory, split file.

csigg updated this revision to Diff 237311.Jan 10 2020, 6:59 AM

Revert test_debuginfo.pl.

Unit tests: pass. 61745 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61745 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

aprantl accepted this revision.Jan 10 2020, 8:59 AM
This revision is now accepted and ready to land.Jan 10 2020, 8:59 AM
This revision was automatically updated to reflect the committed changes.

(Heading through my email backlog),

Adrian wrote:

We are gradually moving away from test_debuginfo.pl in favor of dexter. Have you looked at whether this could be implemented using dexter?

Christian wrote:

Not sure if we need to use a debugger abstraction because the pretty printers aren't standardized and therefore the tests will be pretty debugger specific.

I agree with Christian, while Dexter is good for ensuring that information is read accurately by different DWARF/CodeView consumers, how that's presented to the user is open to choices in the debugger application. IMHO, YMMV.

dblaikie added inline comments.
debuginfo-tests/CMakeLists.txt
5–9

Might need to change the way this is built, @tbosch identified a bug where this test fails on a non-debug build of LLVM, because it doesn't explicitly request debug info, so it gets the build defaults. (either that, or the test should detect this situation - use a REQUIRES or similar to not run in the absence of debug info)

https://bugs.llvm.org/show_bug.cgi?id=45652

dblaikie added inline comments.May 11 2020, 2:37 PM
debuginfo-tests/CMakeLists.txt
5–9

@csigg ping on this ^ issue - could you take a look?

csigg marked an inline comment as done.May 13 2020, 12:59 PM
csigg added inline comments.
debuginfo-tests/CMakeLists.txt
5–9

https://reviews.llvm.org/D79897 should fix it.
Sorry for the delay.