Page MenuHomePhabricator

[asan_symbolize] Use proper logging infrastructure.
ClosedPublic

Authored by delcypher on Apr 5 2019, 3:40 PM.

Details

Summary

The previous logging infrastructure had several problems:

  • Debugging output was emitted to standard output which is also where the symbolized output would go. Interleaving these two separate bits of information makes inspecting the output difficult and could potentially break tests.
  • Enabling debugging output requires modifying the script which is not very conveninent.
  • When debugging it isn't immediately obvious where the output is coming from.

This patch uses the Python standard library logging infrastructure
which fixes all of the above problems. Logging is controlled using
two new options.

  • --log-level - Sets the logging level, default is

info.

  • --log-dest - Set the logging destination, default

is standard error.

Some simple test cases for the feature are included.

rdar://problem/49476995

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Apr 5 2019, 3:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 5 2019, 3:40 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Apr 5 2019, 5:17 PM
lib/asan/scripts/asan_symbolize.py
18 ↗(On Diff #193979)

I'd prefer we avoid env variables here
I guess you can use

args, unknown_args = parser.parse_known_args()

to parse args before pluggins just to setup logging
and then parse them again with additional flags.

delcypher marked an inline comment as done.Apr 7 2019, 3:03 AM
delcypher added inline comments.
lib/asan/scripts/asan_symbolize.py
18 ↗(On Diff #193979)

Good point. I hadn't thought of doing it that way. I'll try it.

On a related note the way I was planning on consuming the list of plug-ins to load (future patch) is via an environment variable. The reasons are the same as this patch but also because it's more convenient (e.g. I can just set the environment variable in my shells environment and avoid having to type long commands). Do you also have the same preference for command line arguments here too?

vitalybuka added inline comments.Apr 7 2019, 1:52 PM
lib/asan/scripts/asan_symbolize.py
18 ↗(On Diff #193979)

yes, I would prefer switch there as well
you can achieve same with something like
(export LIST="a,b,c,d" ; echo $LIST)

delcypher updated this revision to Diff 194097.Apr 8 2019, 12:59 AM
  • Use command line arguments instead of environment variables
  • Add tests
vitalybuka accepted this revision.Apr 8 2019, 10:33 AM

LGTM

test/asan/TestCases/Posix/asan_symbolize_script/logging_options_in_help.cc
3 ↗(On Diff #194097)

does lit pickup test in subdir?

This revision is now accepted and ready to land.Apr 8 2019, 10:33 AM

Could you please update the summary?

delcypher edited the summary of this revision. (Show Details)Apr 8 2019, 3:20 PM

Could you please update the summary?

Oops. I updated the commit message but forgot to update the phabricator summary. It's fixed now.

delcypher marked an inline comment as done.Apr 8 2019, 3:24 PM
delcypher added inline comments.
test/asan/TestCases/Posix/asan_symbolize_script/logging_options_in_help.cc
3 ↗(On Diff #194097)

Yes. Running lit with --no-execute shows that the tests I added get picked up.

This revision was automatically updated to reflect the committed changes.