Page MenuHomePhabricator

Add pretty printers for llvm::PointerIntPair and llvm::PointerUnion.
ClosedPublic

Authored by csigg on Jan 11 2020, 9:11 AM.

Diff Detail

Event Timeline

csigg created this revision.Jan 11 2020, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2020, 9:11 AM

Unit tests: pass. 61771 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

csigg updated this revision to Diff 237588.Jan 13 2020, 12:20 AM

Fix formatting.

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:10 AM
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb
42–47

If you aren't testing the pointer value anyway, you could make it null & then you can test directly for that?

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

Any chance of avoiding stringification/parsing here? could this be written as val.type.template_argument(3)['NumLowBitsAvailable'] instead? (I'm guessing not, but figured I'd check)

Oh, looks like maybe that does work: https://sourceware.org/gdb/current/onlinedocs/gdb/Types-In-Python.html - though it seems maybe gdb.Fields can only provide their value if they're an enum (enumval), maybe not if they're static? Even if the final name has to be stringified and eval'd - it'd still be nice to avoid string concatenation to create the name, if reasonably feasible.

328–329

Might be best just to say we can't print this thing if the trait isn't available, to err on the safe side of not giving possibly incorrect/misleading information?

dblaikie added inline comments.Jan 14 2020, 8:11 AM
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb
42–47

Alternatively (a bit dodgy) you could hardcode a pointer value with a reinterpret cast, to check that the specific value does round-trip through the pretty printer.

Or you could dereference the pointer value in the pretty printing test to demonstrate you can reach the thing it's meant to point to.

Given the bit twiddling, it does seem like it might be nice to verify that the specific pointer value comes out unharmed.

csigg marked an inline comment as done.Jan 14 2020, 12:56 PM
csigg added inline comments.
llvm/utils/gdb-scripts/prettyprinters.py
325

Do you know if gdb.lookup_symbol(<string>).value() would be any better than gdb.parse_and_eval(<string>)?

I experimented for a while (disclaimer: I'm a Python GDB newbie) but couldn't get the enum to show up in PtrTraits.fields(), no matter whether the enum is named or anonymous.
However, the named enum can be found with gdb.lookup_type(), and the fields() do contain the enumerators. That would be the third option.

dblaikie added inline comments.Jan 14 2020, 1:40 PM
llvm/utils/gdb-scripts/prettyprinters.py
325

Sorry, the mention of enums was a red herring - not related to the existing enum usage here, but just something that was in the docs.

Nah, gdb.lookup_symbol(<string>).value() V gdb.parse_and_eval(<string>) doesn't make much difference to me.

Just looking at the constexpr static int member version of the code, not the enum - is there a way to avoid stringification to build the name of the constant? Can the static member variable be found in the fields list of the class? & then evaluated?

csigg updated this revision to Diff 238111.Jan 14 2020, 2:46 PM

Applying dblaikie's review comments.

  • Checking pointer address
  • Switching from parse_and_eval to lookup_symbol
  • Removing the fallback requires including D72590

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

clang-tidy: unknown.

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-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

csigg marked an inline comment as done.Jan 15 2020, 5:42 AM
csigg added inline comments.
llvm/utils/gdb-scripts/prettyprinters.py
325

There is no way to get to the static members of a struct without stringification, only enumerators of an enum.

Enums are no win because getting to the nested enum requires gdb.lookup_type(<stringification>). It would require a standardized enum name.

Instantiating the traits or info class (e.g. as empty base class) to get to the static member would add the requirement that PointerTy is complete. That seems worse.

I'm out of ideas for other approaches.

csigg updated this revision to Diff 238228.Jan 15 2020, 5:44 AM

clang-format

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

clang-tidy: unknown.

clang-format: pass.

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

csigg updated this revision to Diff 238271.Jan 15 2020, 8:20 AM

Switch from lookup_symbol of PtrTraits's 'NumBitsAvailable' to lookup_type of Info::MaskAndShiftConstants.
The latter is not (or less) part of the public interface of PointerIntPair, and requires less code changes.

csigg marked 4 inline comments as done.Jan 15 2020, 8:23 AM
csigg added inline comments.
llvm/utils/gdb-scripts/prettyprinters.py
325

On second thought, gdb.lookup_type does look a little cleaner. Switched to that.

csigg updated this revision to Diff 238274.Jan 15 2020, 8:26 AM

Remove debug hack.

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

clang-tidy: unknown.

clang-format: pass.

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

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

clang-tidy: unknown.

clang-format: pass.

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

dblaikie accepted this revision.Jan 16 2020, 3:42 PM

Looks good - thanks!

This revision is now accepted and ready to land.Jan 16 2020, 3:42 PM
csigg updated this revision to Diff 239146.Jan 20 2020, 8:41 AM

Use factory methods for printeres to catch exceptions.

Unit tests: pass. 62017 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

csigg updated this revision to Diff 239159.Jan 20 2020, 10:00 AM

Revert merge accident.

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
csigg updated this revision to Diff 239160.Jan 20 2020, 10:01 AM

Fix base. I'm so terrible at this.

Hi David. Thanks a lot for you review. I did another pass to switch to factory methods that catch exceptions without spewing common errors on the GDB console. Would you mind taking another quick peek? Thanks a lot for your patience!

Unit tests: pass. 62026 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

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-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 62026 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

dblaikie added inline comments.Jan 25 2020, 4:17 PM
llvm/utils/gdb-scripts/prettyprinters.py
349

Is this an idiomatic way to handle the "function that returns a thing or nothing"? It looks a bit quirky (it's a loop that's only ever intended to iterate once) & I wouldn't mind understanding better how/if this is idiomatic python (if you've got a link to some references about this) before committing it.

csigg updated this revision to Diff 240500.Jan 27 2020, 1:58 AM
csigg marked an inline comment as done.

Getting rid of for loop to handle optional return value.

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

I don't know, and I couldn't find anything online. It did look pretty clean to me, but I'm not attached to it.

There is this discussion whether std::optional could be modeled as container of 0 or 1 element. And quite a few people were concerned that using it in a for loop is confusing:
https://www.reddit.com/r/cpp/comments/6uw5ny/stdoptional_and_container_interface/

Changing it to return a tuple of None.

Unit tests: fail. 62021 tests passed, 6 failed and 782 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_class/try_lock.pass.cpp

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.

This revision was automatically updated to reflect the committed changes.

Hmm - coming back to why these errors occur/need to be suppressed... why do these errors occur?

If it's only because of some missing refactorings/fixes to the APIs, I think maybe we should fix the APIs (since we can maintain the pretty printers in lock step with the APIs as they're committed to the same repository - we don't need backwards or forwards compatibility, etc)? Then we can skip the error handling & let it error out if we later break these APIs or add new pointer like type traits that are missing the relevant members, etc.