Page MenuHomePhabricator

Add GDB pretty printers for llvm::ilist, llvm::simple_ilist, and llvm::ilist_node.
ClosedPublic

Authored by csigg on Jan 13 2020, 12:18 AM.

Diff Detail

Event Timeline

csigg created this revision.Jan 13 2020, 12:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 12:18 AM

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

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

clang-format: pass.

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

dblaikie added inline comments.Jan 14 2020, 8:52 AM
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp
64

(looks like this capture should be removed, according to the lint check)

llvm/utils/gdb-scripts/prettyprinters.py
371–374

Under what conditions does this fallback occur? Are those conditions tested?

csigg updated this revision to Diff 240649.Jan 27 2020, 11:47 AM

Reworking things and improving after PointerIntPair printer has landed.

csigg updated this revision to Diff 240654.Jan 27 2020, 12:01 PM
csigg marked 2 inline comments as done.

Remove capture from lambda.

csigg marked 2 inline comments as done.Jan 27 2020, 12:02 PM
csigg added inline comments.
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp
64

Indeed, don't need to capture globals.

llvm/utils/gdb-scripts/prettyprinters.py
371–374

I changed it to inspect the template parameter, instead of blindly trying both.

It's not possible to check the 'Prev' code path without rebuilding LLVM with different #defines.

Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dblaikie added inline comments.Jan 27 2020, 12:06 PM
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp
64

Ping on this - please remove the capture (remove the "&" in the "[&]") since it doesn't capture anything. (global variables are accessible directly, without capturing them)

llvm/utils/gdb-scripts/prettyprinters.py
380–381

Drop the else-after-return, per LLVM style:

if x:
  return y()
return z()
399

Do you think the make_printer usage is sufficiently more readable than having a standalone pretty printer type (like IlistPrinter below)?

I think there might be some value in just always writing these as standalone pretty printer classes rather than having some of them as factories and some as classes.

csigg updated this revision to Diff 240780.Jan 27 2020, 11:30 PM
csigg marked an inline comment as done.

Switch from factory method to printer class.

csigg updated this revision to Diff 240782.Jan 27 2020, 11:31 PM

Switch from factory method to printer class.

Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Harbormaster completed remote builds in B45090: Diff 240782.
csigg marked 5 inline comments as done.Jan 28 2020, 12:18 AM
csigg added inline comments.
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp
64

Sorry, I think the update was in flight while you were reviewing.

llvm/utils/gdb-scripts/prettyprinters.py
399

I changed it to a class, which makes sense because initialization should not fail.

But for what it's worth, I don't really see the 'some value'. I think it was perfectly readable and concise before.

dblaikie added inline comments.Jan 28 2020, 11:20 AM
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb
48–129

Could you check these with the surrounding syntax too to give a better sense of how the printing would look to the user?

(I guess they'll print in {}, is there a type prefix like with the other pretty printers (eg: "llvm::ArrayRef of", etc)?

csigg updated this revision to Diff 240981.Jan 28 2020, 1:18 PM
csigg marked 2 inline comments as done.

Add detailed CHECKs.

Thanks for the details! I'd be /OK/ with this, but it doesn't seem ideal to be printing the next/prev when printing the container? Do you think it's worth/there's any nice way to avoid that significant redundancy/verbosity in the printing & just print the payload?

csigg marked 2 inline comments as done.Jan 28 2020, 1:33 PM
csigg added inline comments.
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb
48–129

Done.

No, there is no prefix because the printer has no to_string method. That's intentional because gdb does not print the type for ordinary values either. There is ptype for that. CLion watches show the result of both print and ptype. I should probably go back and remove it from the PointerIntPair as well, and add individual printer classes while I'm at it.

Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

csigg marked an inline comment as done.Jan 28 2020, 11:41 PM

Thanks for the details! I'd be /OK/ with this, but it doesn't seem ideal to be printing the next/prev when printing the container? Do you think it's worth/there's any nice way to avoid that significant redundancy/verbosity in the printing & just print the payload?

Well, it's an intrusive list. I'm not aware of any way to hide the ilist_node base class when printing the list, that would be the responsibility of the derived class' printer (which probably wouldn't want to do that, in case someone prints an individual node).
children can only return a list of gdb.Value, and gdb picks the printers for those internally. And to make things a little more complicated, like in the test here, there can be multiple ilist_node base classes and you would only want to silence the one with the corresponding tag.

dblaikie accepted this revision.Jan 29 2020, 3:36 PM

Thanks for the details! I'd be /OK/ with this, but it doesn't seem ideal to be printing the next/prev when printing the container? Do you think it's worth/there's any nice way to avoid that significant redundancy/verbosity in the printing & just print the payload?

Well, it's an intrusive list. I'm not aware of any way to hide the ilist_node base class when printing the list, that would be the responsibility of the derived class' printer (which probably wouldn't want to do that, in case someone prints an individual node).
children can only return a list of gdb.Value, and gdb picks the printers for those internally. And to make things a little more complicated, like in the test here, there can be multiple ilist_node base classes and you would only want to silence the one with the corresponding tag.

Yup. Better than nothing & no doubt something better than this would be non-trivial.

This revision is now accepted and ready to land.Jan 29 2020, 3:36 PM
This revision was automatically updated to reflect the committed changes.