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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris requested changes to this revision.
dberris added inline comments.
This revision now requires changes to proceed.


================
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, '/');
----------------
pelikan wrote:
> 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.
"(unknown)" seems strictly better if not annoying to refer to in the filesystem. Either that or just keeping it empty?


================
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);
----------------
pelikan wrote:
> 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.
Yeah, you can probably do something about making it relative to the sizeof(TmpFilename).


================
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);
----------------
pelikan wrote:
> 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?
Yeah, so "Need" isn't the best name. Is "Length" a better name?


https://reviews.llvm.org/D27912