This is an archive of the discontinued LLVM Phabricator instance.

[Symbolize] Add log markup --filter to llvm-symbolizer.
ClosedPublic

Authored by mysterymath on Jun 3 2022, 10:59 AM.

Details

Summary

This adds a --filter option to llvm-symbolizer. This takes log-bearing
symbolizer markup from stdin and writes a human-readable version to
stdout.

For now, this only implements the "symbol" markup tag; all others are
passed through unaltered. This is a proof-of-concept bit of
functionalty; implement the various tags is more-or-less just a matter
of hooking up various parts of the Symbolize library to the architecture
established here.

Diff Detail

Event Timeline

mysterymath created this revision.Jun 3 2022, 10:59 AM
Herald added a project: Restricted Project. · View Herald Transcript

Clean up patch before code review.

mysterymath published this revision for review.Jun 3 2022, 1:23 PM
mysterymath added reviewers: phosek, mcgrathr.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 1:23 PM

Add field size checker.

Add pretty error location reporting.

Remove tryFilter abstraction; unneeded complexity.

Rename coloring utils.

Only a few small comments. May be worth calling Filter.cpp and Filter.h MarkupFilter.cpp and MarkupFilter.h as there could be non-markup based filters in the future. Not a strong opinion as these filenames could be changed later if needed.

Could be worth adding to the release notes as a new feature for llvm-symbolizer.

llvm/docs/CommandGuide/llvm-symbolizer.rst
250

Looking forward, is it possible that the symbolizer might support more than one markup, and would it change the interface? My thoughts are that we could introduce another parameter --filter-markup=<llvm | something-else> which could default to the LLVM Markup syntax, so using --filter isn't a problem.

llvm/include/llvm/DebugInfo/Symbolize/Filter.h
35 ↗(On Diff #439106)

Just a subjective opinion, must be that passed read a bit abruptly. Perhaps something like:
must be the same Line that was passed to parseLine() to ...

llvm/lib/DebugInfo/Symbolize/Filter.cpp
142 ↗(On Diff #439106)
mysterymath marked 2 inline comments as done.

Address review comments.

Only a few small comments. May be worth calling Filter.cpp and Filter.h MarkupFilter.cpp and MarkupFilter.h as there could be non-markup based filters in the future. Not a strong opinion as these filenames could be changed later if needed.

I like MarkupFilter as a name; especially as a pair for MarkupNode and MarkupParser. I've renamed the files and classes to match.

Could be worth adding to the release notes as a new feature for llvm-symbolizer.

Done.

llvm/docs/CommandGuide/llvm-symbolizer.rst
250

I actually like --filter-markup better as the command line option; it's clearer from the invocation what it's going to do. This also allows us to later backwards-compatibly add an --filter-markup= version of the flag, and the non-equals version can default to llvm.

peter.smith accepted this revision.Jun 27 2022, 1:36 AM

LGTM. Thanks very much for the updates.

This revision is now accepted and ready to land.Jun 27 2022, 1:36 AM
This revision was landed with ongoing or failed builds.Jun 27 2022, 10:44 AM
This revision was automatically updated to reflect the committed changes.