Page MenuHomePhabricator

[sanitizer_common] Add facility to get the full report path
ClosedPublic

Authored by tejohnson on Nov 18 2020, 9:58 PM.

Details

Summary

Add a new interface sanitizer_get_report_path which will return the
full path to the report file if
sanitizer_set_report_path was
previously called (otherwise it returns null). This is useful in
particular for memory profiling handlers to access the path which
was specified at compile time (and passed down via
__memprof_profile_filename), including the pid added to the path when
the file is opened.

There wasn't a test for __sanitizer_set_report_path, so I added one
which additionally tests the new interface.

Diff Detail

Event Timeline

tejohnson created this revision.Nov 18 2020, 9:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 9:58 PM
Herald added subscribers: Restricted Project, phosek. · View Herald Transcript
tejohnson requested review of this revision.Nov 18 2020, 9:58 PM

It mentions the test, but it's not here

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
100

why reopen is needed?

Add the test

It mentions the test, but it's not here

Woops, forgot to git add it. It's there now.

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
100

Because the actual full_path (with pid suffix) is populated only on open.

vitalybuka accepted this revision.Nov 18 2020, 10:30 PM

LGTM with /tmp/ fix

compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_test.cpp
12

it should not work on android

maybe this without FileCHeck?
sprintf(buff, "%s.report_path", arg[0]);
sanitizer_set_report_path(buff);
assert(strncmp(buff,
sanitizer_get_report_path(), strlen(buff)) == 0)

This revision is now accepted and ready to land.Nov 18 2020, 10:30 PM

BTW mktemp should work, but build dir from arg[0] looks better

tejohnson added inline comments.Nov 18 2020, 11:24 PM
compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_test.cpp
12

Good idea. But the assert won't work, because a ".${pid}" suffix will get added. But I can do a partial match using FileCheck.

Fix test to avoid using /tmp

vitalybuka accepted this revision.Nov 18 2020, 11:40 PM
vitalybuka added inline comments.
compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_test.cpp
12

Hm, here we suppose to match only strlen(buff) first byte
assert(strncmp(buff, sanitizer_get_report_path(), strlen(buff)) == 0)

either ways is fine

tejohnson added inline comments.Nov 18 2020, 11:44 PM
compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_test.cpp
12

Ah yeah, you're right. I'll add the assert in as well before I submit tomorrow.

tejohnson updated this revision to Diff 306440.Nov 19 2020, 9:19 AM

Beef up test as suggested to check the full path via an assert

This revision was landed with ongoing or failed builds.Nov 19 2020, 9:19 AM
This revision was automatically updated to reflect the committed changes.