Page MenuHomePhabricator

[Sanitizer] Allow setting the report path to create directory

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



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

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


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
263 ↗(On Diff #372633)

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


Maybe we can add ReportFile::SetReportPath unittest here:

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


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


if (!IsPathSeparator) ?


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.
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:

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?


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.


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.


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.
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
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.