[PATCH] D109794: [Sanitizer] Allow setting the report path to create directory

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 4 inline comments as done.
tejohnson added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common.h:263
 
+#if !SANITIZER_FUCHSIA
+int CreateDir(const char *pathname);
----------------
vitalybuka wrote:
> can we avoid #if?
> we don't have one e.g. for GetProcessName
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?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:78
 
+static void RecursiveMkdir(char *path) {
+  int i;
----------------
vitalybuka wrote:
> 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)
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:82
+
+  for (i = start; path[i] != '\0'; ++i) {
+    char save = path[i];
----------------
vitalybuka wrote:
> We don't need two vars outside the loop.
> Also if we start with 1 we can overflow on empty string.
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:84
+    char save = path[i];
+    if (IsPathSeparator(path[i]))
+      continue;
----------------
vitalybuka wrote:
> if (!IsPathSeparator) ?
> 
> 
Woops,  yes,  I introduced this during some cleanup before sending the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109794/new/

https://reviews.llvm.org/D109794