Page MenuHomePhabricator

[Sanitizer] Allow setting the report path to create directory
ClosedPublic

Authored by tejohnson on Tue, Sep 14, 4:39 PM.

Details

Summary

When setting the report path, recursively create the directory as
needed. This brings the profile path support for memprof on par with
normal PGO. The code was largely cloned from __llvm_profile_recursive_mkdir
in compiler-rt/lib/profile/InstrProfilingUtil.c.

Diff Detail

Event Timeline

tejohnson created this revision.Tue, Sep 14, 4:39 PM
tejohnson requested review of this revision.Tue, Sep 14, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 14, 4:39 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Tue, Sep 14, 6:00 PM
compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
87

to avoid #ifs here can you please create CreateDir() and provide two implementations:
in sanitizer_win and sanitizer_posix

93

please clang-format as it asks

Move mkdir calls to OS specific helper.

Undo formatting change in unrelated existing code

tejohnson marked 2 inline comments as done.Tue, Sep 14, 10:33 PM
vitalybuka added inline comments.Wed, Sep 15, 5:23 PM
compiler-rt/lib/sanitizer_common/sanitizer_common.h
263 ↗(On Diff #372633)

can we avoid #if?
we don't have one e.g. for GetProcessName

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

Maybe we can add ReportFile::SetReportPath unittest here:
sanitizer_common/tests/sanitizer_common_test.cpp

The function is little bit unconventional and does not create path for the last component. Maybe rename into something RecursiveCreateParentDirs(char* path)

82

We don't need two vars outside the loop.
Also if we start with 1 we can overflow on empty string.

84

if (!IsPathSeparator) ?

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
154

int -> bool, to avoid exposing platform specific error errors.

tejohnson marked 4 inline comments as done.Thu, Sep 16, 4:05 PM
tejohnson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common.h
263 ↗(On Diff #372633)

GetProcessName has a single def in sanitizer_common.cpp. A closer analogy would be Abort(), which has implementations in all of these 4 places:
sanitizer_posix_libcdep.cpp
sanitizer_common_nolibc.cpp
sanitizer_win.cpp
sanitizer_fuchsia.cpp

Since CreateDir calls mkdir/_mkdir it also needs to be defined in similar places, and I have defs in the first 3 of the above. I believe for things to link properly on Fuchsia I would need to provide one there too. However, sanitizer_file.cpp which uses this new function has its code entirely guarded by #if !SANITIZER_FUCHSIA, so I wasn't sure it made sense. WDYT?

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

I ended up adding a new unittest to sanitizer_common/tests/sanitizer_libc_test.cpp instead, since it is already testing some sanitizer_file interfaces, and it probably only makes sense to test this under libc where we can mkdir anyway.

82

We start at 1 because if the path starts with "/" it doesn't make sense to try to mkdir an empty string. I added a check for the path being empty above this loop.

84

Woops, yes, I introduced this during some cleanup before sending the patch.

tejohnson updated this revision to Diff 373098.Thu, Sep 16, 4:06 PM
tejohnson marked 3 inline comments as done.

Address comments

vitalybuka accepted this revision.Mon, Sep 20, 8:58 AM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common.h
263 ↗(On Diff #372633)

I guess you can just move CreateDir next to IsPathSeparator into sanitizer_file.h without any defines. That header is better place for the definition anyway.

This revision is now accepted and ready to land.Mon, Sep 20, 8:58 AM
vitalybuka added inline comments.Mon, Sep 20, 8:59 AM
compiler-rt/lib/sanitizer_common/sanitizer_common.h
263 ↗(On Diff #372633)

"without any defines" -> "without any ifdefs"

tejohnson marked an inline comment as done.Tue, Sep 21, 1:51 PM
tejohnson updated this revision to Diff 374025.Tue, Sep 21, 1:52 PM

Address comments

vitalybuka accepted this revision.Tue, Sep 21, 2:50 PM
This revision was landed with ongoing or failed builds.Tue, Sep 21, 4:43 PM
This revision was automatically updated to reflect the committed changes.