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

[pelikan]

Summary

If you decide to recompile parts of your Linux distro with XRay, it may
be useful to know which trace belongs to which binary. While there, get
rid of the incorrect strncat() usage; it always returns a pointer to the
start which makes that if() always true. Replace with snprintf which is
bounded so that enough from both strings fits nicely.

[dberris]

lib/xray/xray_inmemory_log.cc

116 ↗

(On Diff #81915)

How about instead of "UNKNOWN_PROGNAME" just 'unknown'?

121 ↗

(On Diff #81915)

No braces for one-liners?

124–125 ↗

(On Diff #81915)

What's the 120?

127 ↗

(On Diff #81915)

Is there a better name than "Need"? How about Overflow?

[pelikan]

address comments

[pelikan]

Mark as done.

lib/xray/xray_inmemory_log.cc

116 ↗

(On Diff #81915)

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.

121 ↗

(On Diff #81915)

I tried to find it in the coding standards, I promise! :-)

124–125 ↗

(On Diff #81915)

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.

127 ↗

(On Diff #81915)

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?

[dberris]

lib/xray/xray_inmemory_log.cc

116 ↗

(On Diff #81915)

"(unknown)" seems strictly better if not annoying to refer to in the filesystem. Either that or just keeping it empty?

124–125 ↗

(On Diff #81915)

Yeah, you can probably do something about making it relative to the sizeof(TmpFilename).

127 ↗

(On Diff #81915)

Yeah, so "Need" isn't the best name. Is "Length" a better name?

[pelikan]

address comments

[dberris]

Did the tests need updating, so that we know the "default" would do the right thing?

Currently we set the pattern explicitly. It'd be great to see a test that makes sure that the default pattern including argv[0] actually shows up.

[pelikan]

add a test

[dberris]

I was thinking it might be better if you made a specific test just for this -- that the "default" (i.e. not setting the pattern with the XRAY_OPTIONS environment variable) would be better, so that we can isolate the feature in a specific test.

test/xray/TestCases/Linux/fixedsize-logging.cc

5 ↗

(On Diff #81928)

I'm not sure we can rely on shell expansion to work here. Can you think of a different way of doing this, where you look for the filename on the "default" (i.e. non-configured explicitly) name here?

[pelikan]

sync; test changes still missing

[pelikan]

rebase this properly this time?

[pelikan]

• make a bespoke test for argv[0] being in the file name
• make it more complicated and not use $() and ??????

[dberris]

LGTM

compiler-rt/trunk/lib/xray/xray_inmemory_log.cc

 }
 
 static int __xray_OpenLogFile() XRAY_NEVER_INSTRUMENT {
   // FIXME: Figure out how to make this less stderr-dependent.
   SetPrintfAndReportCallback(PrintToStdErr);
   // Open a temporary file once for the log.
   static char TmpFilename[256] = {};
   static char TmpWildcardPattern[] = "XXXXXX";
-  auto E = internal_strncat(TmpFilename, flags()->xray_logfile_base,
-                            sizeof(TmpFilename) - 10);
-  if (static_cast<size_t>((E + 6) - TmpFilename) > (sizeof(TmpFilename) - 1)) {
-    Report("XRay log file base too long: %s\n", flags()->xray_logfile_base);
+  auto Argv = GetArgv();
+  const char *Progname = Argv[0] == nullptr ? "(unknown)" : Argv[0];
+  const char *LastSlash = internal_strrchr(Progname, '/');
+
+  if (LastSlash != nullptr)
+    Progname = LastSlash + 1;
+
+  const int HalfLength = sizeof(TmpFilename) / 2 - sizeof(TmpWildcardPattern);
+  int NeededLength = internal_snprintf(TmpFilename, sizeof(TmpFilename),
+                                       "%.*s%.*s.%s",
+                                       HalfLength, flags()->xray_logfile_base,
+                                       HalfLength, Progname,
+                                       TmpWildcardPattern);
+  if (NeededLength > int(sizeof(TmpFilename))) {
+    Report("XRay log file name too long (%d): %s\n", NeededLength, TmpFilename);
     return -1;
   }
-  internal_strncat(TmpFilename, TmpWildcardPattern,
-                   sizeof(TmpWildcardPattern) - 1);
   int Fd = mkstemp(TmpFilename);
   if (Fd == -1) {
     Report("XRay: Failed opening temporary file '%s'; not logging events.\n",
            TmpFilename);
     return -1;
   }
   if (Verbosity())
     fprintf(stderr, "XRay: Log file in '%s'\n", TmpFilename);

compiler-rt/trunk/test/xray/TestCases/Linux/argv0-log-file-name.cc

+// Check to make sure argv[0] is contained within the (randomised) XRay log file
+// name.
+
+// RUN: %clangxx_xray -std=c++11 %s -o %t
+// RUN: %run %t > xray.log.file.name 2>&1
+// RUN: ls | FileCheck xray.log.file.name
+// RUN: rm xray-log.* xray.log.file.name
+
+#include <cstdio>
+#include <libgen.h>
+
+[[clang::xray_always_instrument]] int main(int argc, char *argv[]) {
+  printf("// CHECK: xray-log.%s.{{.*}}\n", basename(argv[0]));
+}