This is an archive of the discontinued LLVM Phabricator instance.

[demangler] Node precision dumper
ClosedPublic

Authored by urnathan on Mar 30 2022, 8:50 AM.

Details

Reviewers
dblaikie
bruno
ldionne
Group Reviewers
Restricted Project
Commits
rG4a4d0985d4f0: [demangler] Node precision dumper
Summary

Another fix from fixing up D120905

The empty print(Prec) functions added in fixing that breakage need bodies. This is that.

Diff Detail

Event Timeline

urnathan created this revision.Mar 30 2022, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
urnathan requested review of this revision.Mar 30 2022, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:50 AM

can/should this have test coverage?

can/should this have test coverage?

Doesn't look simple. This is inside NDEBUG and explicitly writes to stderr, so I'm not sure how to capture that?

can/should this have test coverage?

Doesn't look simple. This is inside NDEBUG and explicitly writes to stderr, so I'm not sure how to capture that?

Ah, I actually just was glancing at a commit that tested a situation like this ( rG9a62d9db2e1f6064e6c20344870f5e9d43a8de43 ) - so it can be/is done by REQUIRES: asserts on a lit test and then checking stderr (possibly/usually by 2>&1 to just collapse stderr and stdout together).

I'd expect/hope there is already other test coverage for this general feature that you could extend - or is this whole printing system untested?

can/should this have test coverage?

Doesn't look simple. This is inside NDEBUG and explicitly writes to stderr, so I'm not sure how to capture that?

Ah, I actually just was glancing at a commit that tested a situation like this ( rG9a62d9db2e1f6064e6c20344870f5e9d43a8de43 ) - so it can be/is done by REQUIRES: asserts on a lit test and then checking stderr (possibly/usually by 2>&1 to just collapse stderr and stdout together).

I'd expect/hope there is already other test coverage for this general feature that you could extend - or is this whole printing system untested?

I cannot see existing tests for this. AFAICT the node dumping machinery is accessible via debugging llvm. So I don't really see how a unittest could test it, nor how one could invoke the compiler to get at it.

The only 3 dump member tests I can find in llvm/unittest/...

IR/DominatorTreeBatchUpdatesTest.cpp:
Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp:
Transforms/Vectorize/VPlanTest.cpp:

but they only seem to be checking the dumpers don't crash.

dblaikie accepted this revision.Apr 6 2022, 1:00 PM

Ah, these aren't invoked from any user-accessible codepath (even in an asserts build), fair enough. Thanks!

This revision is now accepted and ready to land.Apr 6 2022, 1:00 PM
This revision was landed with ongoing or failed builds.Apr 6 2022, 2:06 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 2:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript