[PATCH] D27912: [XRay] [compiler-rt] Include argv[0] in the log file name.

[Martin Pelikán via Phabricator via llvm-commits]

pelikan marked an inline comment as done.
pelikan added a comment.

Mark as done.



================
Comment at: lib/xray/xray_inmemory_log.cc:116
+  auto Argv = GetArgv();
+  const char *Progname = Argv[0] == nullptr ? "UNKNOWN_PROGNAME" : Argv[0];
+  const char *LastSlash = internal_strrchr(Progname, '/');
----------------
dberris wrote:
> How about instead of "UNKNOWN_PROGNAME" just 'unknown'?
I was actually thinking of "(unknown)", similar to when printf(3) prints "(null)" on NULL strings.  I don't have a strong preference but wanted something that immediately suggests something's gone wrong.


================
Comment at: lib/xray/xray_inmemory_log.cc:121
+    Progname = LastSlash + 1;
+  }
+
----------------
dberris wrote:
> No braces for one-liners?
I tried to find it in the coding standards, I promise! :-)


================
Comment at: lib/xray/xray_inmemory_log.cc:124-125
+  int Need = internal_snprintf(TmpFilename, sizeof(TmpFilename), "%.*s%.*s.%s",
+                               120, flags()->xray_logfile_base,
+                               120, Progname,
+                               TmpWildcardPattern);
----------------
dberris wrote:
> What's the 120?
The buffer is 256 characters long; 120 is a decent bound for not overflowing the string.  Should I bind it with sizeof(TmpFilename) somehow?  Its size is a magic number too, and they're close to one another for people to see.


================
Comment at: lib/xray/xray_inmemory_log.cc:127
+                               TmpWildcardPattern);
+  if (Need > int(sizeof(TmpFilename))) {
+    Report("XRay log file name too long (%d): %s\n", Need, TmpFilename);
----------------
dberris wrote:
> Is there a better name than "Need"? How about Overflow?
The function returns how many bytes would you need to satisfy the operation; "overflow" would indicate it's the amount of characters over 256 (sizeof TmpFilename) IMHO.  Do you still want me to change it?


https://reviews.llvm.org/D27912