Page MenuHomePhabricator

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

Authored by mysterymath on Fri, Jun 3, 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

Unit TestsFailed

TimeTest
60,070 msx64 debian > Clang.Driver::emit-reproducer.c
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp
60,040 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,030 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest

Event Timeline

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

Clean up patch before code review.

mysterymath published this revision for review.Fri, Jun 3, 1:23 PM
mysterymath added reviewers: phosek, mcgrathr.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 3, 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.