[profile] clean up file handling code

[davidxl]

Summary

1. Split file name setting code into two parts -- parsing code and the getter code
2. better warning/error handling
3. Removed the last dependence on malloc in profile runtime (the default path)

[davidxl]

Add check in caller of file name parser.

[vsk]

Nice, thanks.

lib/profile/InstrProfilingFile.c

88

nit, This sounds like a name for a boolean. How about FilenameBuf?

112–117

This is a behavior change, right? Do we really only want to truncate profiles if the filename pattern changes?

126

Could you use strncpy?

129

^ Same comment

152

nit, Use -> Using

161

^ Same nit

[davidxl]

lib/profile/InstrProfilingFile.c

112–118

there is no change. If the pattern str changes, the filename will also change which requires truncation in previous implementation.

[davidxl]

Addressed review feedbacks.

[vsk]

lib/profile/InstrProfilingFile.c

112–118

Got it, silly mistake on my part.

[MatzeB]

lib/profile/InstrProfilingFile.c

129

Note that strncpy is a really silly function: If you actually happen to write n or more characters you will not have a terminating '\0' character... My personal rule is to never use strncpy() but something like snprintf(buf, bufsize, "%s%s", one, two);

[vsk]

Done in compiler-rt/r270617

lib/profile/InstrProfilingFile.c

 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingUtil.h"
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #ifdef _MSC_VER
-/* For _alloca */
+/* For _alloca. */
 #include <malloc.h>
 #endif
 
+#define MAX_PID_SIZE 16
+/* Data structure holding the result of parsed filename pattern.  */
+typedef struct lprofFilename {
+  /* File name string possibly with %p or %h specifiers. */
+  const char *FilenamePat;
+  char PidChars[MAX_PID_SIZE];
+  char Hostname[COMPILER_RT_MAX_HOSTLEN];
+  unsigned NumPids;
+  unsigned NumHosts;
+} lprofFilename;
+
+lprofFilename lprofCurFilename = {0};
 
-#define UNCONST(ptr) ((void *)(uintptr_t)(ptr))
+int getpid(void);
+static int getCurFilenameLength();
+static const char *getCurFilename(char *FilenameBuf);
 
 /* Return 1 if there is an error, otherwise return  0.  */
 static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,
                            void **WriterCtx) {
   uint32_t I;
   FILE *File = (FILE *)*WriterCtx;
   for (I = 0; I < NumIOVecs; I++) {
     if (fwrite(IOVecs[I].Data, IOVecs[I].ElmSize, IOVecs[I].NumElm, File) !=
static void setupIOBuffer() {
   const char *BufferSzStr = 0;
   BufferSzStr = getenv("LLVM_VP_BUFFER_SIZE");
   if (BufferSzStr && BufferSzStr[0]) {
     VPBufferSize = atoi(BufferSzStr);
     DynamicBufferIOBuffer = (uint8_t *)calloc(VPBufferSize, 1);
   }
 }
 
-static int writeFile(FILE *File) {
-  FreeHook = &free;
-  setupIOBuffer();
-  return lprofWriteData(fileWriter, File, lprofGetVPDataReader());
-}
-
-static int writeFileWithName(const char *OutputName) {
+static int writeFile(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
-  if (!OutputName || !OutputName[0])
-    return -1;
 
   /* Append to the file to support profiling multiple shared objects. */
   OutputFile = fopen(OutputName, "ab");
   if (!OutputFile)
     return -1;
 
-  RetVal = writeFile(OutputFile);
+  FreeHook = &free;
+  setupIOBuffer();
+  RetVal = lprofWriteData(fileWriter, OutputFile, lprofGetVPDataReader());
 
   fclose(OutputFile);
   return RetVal;
 }
 
-COMPILER_RT_WEAK int __llvm_profile_OwnsFilename = 0;
-COMPILER_RT_WEAK const char *__llvm_profile_CurrentFilename = NULL;
-
 static void truncateCurrentFile(void) {
   const char *Filename;
+  char *FilenameBuf;
   FILE *File;
+  int Length;
 
-  Filename = __llvm_profile_CurrentFilename;
-  if (!Filename || !Filename[0])
+  Length = getCurFilenameLength();
+  FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
+  Filename = getCurFilename(FilenameBuf);
+  if (!Filename)
     return;
 
   /* Create the directory holding the file, if needed. */
   if (strchr(Filename, '/') || strchr(Filename, '\\')) {
-    char *Copy = (char *)COMPILER_RT_ALLOCA(strlen(Filename) + 1);
-    strcpy(Copy, Filename);
+    char *Copy = (char *)COMPILER_RT_ALLOCA(Length + 1);
+    strncpy(Copy, Filename, Length + 1);
     __llvm_profile_recursive_mkdir(Copy);
   }
 
   /* Truncate the file.  Later we'll reopen and append. */
   File = fopen(Filename, "w");
   if (!File)
     return;
   fclose(File);
 }
 
-static void setFilename(const char *Filename, int OwnsFilename) {
+/* Set the result of the file name parsing. If \p FilenamePat pattern is seen
+ * the first time, also truncate the file associated with that name.
+ */
+static void setFilename(const char *FilenamePat, const char *PidStr,
+                        unsigned NumPids, const char *HostStr,
+                        unsigned NumHosts) {
   /* Check if this is a new filename and therefore needs truncation. */
-  int NewFile = !__llvm_profile_CurrentFilename ||
-      (Filename && strcmp(Filename, __llvm_profile_CurrentFilename));
-  if (__llvm_profile_OwnsFilename)
-    free(UNCONST(__llvm_profile_CurrentFilename));
-
-  __llvm_profile_CurrentFilename = Filename;
-  __llvm_profile_OwnsFilename = OwnsFilename;
+  int NewFile =
+      !lprofCurFilename.FilenamePat ||
+      (FilenamePat && strcmp(FilenamePat, lprofCurFilename.FilenamePat));
+
+  lprofCurFilename.FilenamePat = FilenamePat;
+  lprofCurFilename.NumPids = NumPids;
+  if (NumPids)
+    strncpy(lprofCurFilename.PidChars, PidStr, MAX_PID_SIZE);
+  lprofCurFilename.NumHosts = NumHosts;
+  if (NumHosts)
+    strncpy(lprofCurFilename.Hostname, HostStr, COMPILER_RT_MAX_HOSTLEN);
 
   /* If not a new file, append to support profiling multiple shared objects. */
   if (NewFile)
     truncateCurrentFile();
 }
 
-static void resetFilenameToDefault(void) { setFilename("default.profraw", 0); }
+static void resetFilenameToDefault(void) {
+  setFilename("default.profraw", 0, 0, 0, 0);
+}
 
-int getpid(void);
-static int setFilenamePossiblyWithPid(const char *Filename) {
-#define MAX_PID_SIZE 16
-  char PidChars[MAX_PID_SIZE] = {0};
-  int NumPids = 0, PidLength = 0, NumHosts = 0, HostNameLength = 0;
-  char *Allocated;
-  int I, J;
+static int parseFilenamePattern(const char *FilenamePat) {
+  int NumPids = 0, NumHosts = 0, I;
+  char PidChars[MAX_PID_SIZE];
   char Hostname[COMPILER_RT_MAX_HOSTLEN];
 
-  /* Reset filename on NULL, except with env var which is checked by caller. */
-  if (!Filename) {
-    resetFilenameToDefault();
-    return 0;
-  }
-
   /* Check the filename for "%p", which indicates a pid-substitution. */
-  for (I = 0; Filename[I]; ++I)
-    if (Filename[I] == '%') {
-      if (Filename[++I] == 'p') {
+  for (I = 0; FilenamePat[I]; ++I)
+    if (FilenamePat[I] == '%') {
+      if (FilenamePat[++I] == 'p') {
         if (!NumPids++) {
-          PidLength = snprintf(PidChars, MAX_PID_SIZE, "%d", getpid());
-          if (PidLength <= 0)
+          if (snprintf(PidChars, MAX_PID_SIZE, "%d", getpid()) <= 0) {
+            PROF_WARN(
+                "Unable to parse filename pattern %s. Using the default name.",
+                FilenamePat);
             return -1;
           }
-      } else if (Filename[I] == 'h') {
+        }
+      } else if (FilenamePat[I] == 'h') {
         if (!NumHosts++)
-          if (COMPILER_RT_GETHOSTNAME(Hostname, COMPILER_RT_MAX_HOSTLEN))
+          if (COMPILER_RT_GETHOSTNAME(Hostname, COMPILER_RT_MAX_HOSTLEN)) {
+            PROF_WARN(
+                "Unable to parse filename pattern %s. Using the default name.",
+                FilenamePat);
             return -1;
-          HostNameLength = strlen(Hostname);
+          }
       }
     }
 
-  if (!(NumPids || NumHosts)) {
-    setFilename(Filename, 0);
-    return 0;
-  }
+  setFilename(FilenamePat, PidChars, NumPids, Hostname, NumHosts);
 
-  /* Allocate enough space for the substituted filename. */
-  Allocated = malloc(I + NumPids*(PidLength - 2) +
-                     NumHosts*(HostNameLength - 2) + 1);
-  if (!Allocated)
-    return -1;
+static int getCurFilenameLength() {
+  if (!lprofCurFilename.FilenamePat || !lprofCurFilename.FilenamePat[0])
+    return 0;
+  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts))
+    return strlen(lprofCurFilename.FilenamePat);
+
+  return strlen(lprofCurFilename.FilenamePat) +
+         lprofCurFilename.NumPids * (strlen(lprofCurFilename.PidChars) - 2) +
+         lprofCurFilename.NumHosts * (strlen(lprofCurFilename.Hostname) - 2);
+}
+
+static const char *getCurFilename(char *FilenameBuf) {
+  int I, J, PidLength, HostNameLength;
+  const char *FilenamePat = lprofCurFilename.FilenamePat;
+
+  if (!lprofCurFilename.FilenamePat || !lprofCurFilename.FilenamePat[0])
+    return 0;
+
+  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts))
+    return lprofCurFilename.FilenamePat;
 
+  PidLength = strlen(lprofCurFilename.PidChars);
+  HostNameLength = strlen(lprofCurFilename.Hostname);
   /* Construct the new filename. */
-  for (I = 0, J = 0; Filename[I]; ++I)
-    if (Filename[I] == '%') {
-      if (Filename[++I] == 'p') {
-        memcpy(Allocated + J, PidChars, PidLength);
+  for (I = 0, J = 0; FilenamePat[I]; ++I)
+    if (FilenamePat[I] == '%') {
+      if (FilenamePat[++I] == 'p') {
+        memcpy(FilenameBuf + J, lprofCurFilename.PidChars, PidLength);
         J += PidLength;
-      }
-      else if (Filename[I] == 'h') {
-        memcpy(Allocated + J, Hostname, HostNameLength);
+      } else if (FilenamePat[I] == 'h') {
+        memcpy(FilenameBuf + J, lprofCurFilename.Hostname, HostNameLength);
         J += HostNameLength;
       }
       /* Drop any unknown substitutions. */
     } else
-      Allocated[J++] = Filename[I];
-  Allocated[J] = 0;
+      FilenameBuf[J++] = FilenamePat[I];
+  FilenameBuf[J] = 0;
 
-  /* Use the computed name. */
-  setFilename(Allocated, 1);
-  return 0;
+  return FilenameBuf;
 }
 
-static const char *getFilenameFromEnv(void) {
+static const char *getFilenamePatFromEnv(void) {
   const char *Filename = getenv("LLVM_PROFILE_FILE");
   if (!Filename || !Filename[0])
     return 0;
   return Filename;
 }
 
 /* This method is invoked by the runtime initialization hook
  * InstrProfilingRuntime.o if it is linked in. Both user specified
  * profile path via -fprofile-instr-generate= and LLVM_PROFILE_FILE
  * environment variable can override this default value. */
 COMPILER_RT_VISIBILITY
 void __llvm_profile_initialize_file(void) {
-  const char *Filename;
+  const char *FilenamePat;
   /* Check if the filename has been initialized. */
-  if (__llvm_profile_CurrentFilename)
+  if (lprofCurFilename.FilenamePat)
     return;
 
   /* Detect the filename and truncate. */
-  Filename = getFilenameFromEnv();
-  if (!Filename || setFilenamePossiblyWithPid(Filename))
+  FilenamePat = getFilenamePatFromEnv();
+  if (!FilenamePat || parseFilenamePattern(FilenamePat))
     resetFilenameToDefault();
 }
 
 /* This API is directly called by the user application code. It has the
  * highest precedence compared with LLVM_PROFILE_FILE environment variable
  * and command line option -fprofile-instr-generate=<profile_name>.
  */
 COMPILER_RT_VISIBILITY
-void __llvm_profile_set_filename(const char *Filename) {
-  setFilenamePossiblyWithPid(Filename);
+void __llvm_profile_set_filename(const char *FilenamePat) {
+  if (!FilenamePat || parseFilenamePattern(FilenamePat))
+    resetFilenameToDefault();
 }
 
-/* 
+/*
  * This API is invoked by the global initializers emitted by Clang/LLVM when
  * -fprofile-instr-generate=<..> is specified (vs -fprofile-instr-generate
  *  without an argument). This option has lower precedence than the
  *  LLVM_PROFILE_FILE environment variable.
  */
 COMPILER_RT_VISIBILITY
-void __llvm_profile_override_default_filename(const char *Filename) {
+void __llvm_profile_override_default_filename(const char *FilenamePat) {
   /* If the env var is set, skip setting filename from argument. */
-  const char *Env_Filename = getFilenameFromEnv();
+  const char *Env_Filename = getFilenamePatFromEnv();
   if (Env_Filename)
     return;
-  setFilenamePossiblyWithPid(Filename);
+  if (!FilenamePat || parseFilenamePattern(FilenamePat))
+    resetFilenameToDefault();
 }
 
 COMPILER_RT_VISIBILITY
 int __llvm_profile_write_file(void) {
-  int rc;
+  int rc, Length;
+  const char *Filename;
+  char *FilenameBuf;
+
+  Length = getCurFilenameLength();
+  FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
+  Filename = getCurFilename(FilenameBuf);
 
-  GetEnvHook = &getenv;
   /* Check the filename. */
-  if (!__llvm_profile_CurrentFilename) {
+  if (!Filename) {
     PROF_ERR("Failed to write file : %s\n", "Filename not set");
     return -1;
   }
 
   /* Check if there is llvm/runtime version mismatch.  */
   if (GET_VERSION(__llvm_profile_get_version()) != INSTR_PROF_RAW_VERSION) {
     PROF_ERR("Runtime and instrumentation version mismatch : "
              "expected %d, but get %d\n",
              INSTR_PROF_RAW_VERSION,
              (int)GET_VERSION(__llvm_profile_get_version()));
     return -1;
   }
 
-  /* Write the file. */
-  rc = writeFileWithName(__llvm_profile_CurrentFilename);
+  /* Write profile data to the file. */
+  rc = writeFile(Filename);
   if (rc)
-    PROF_ERR("Failed to write file \"%s\": %s\n",
-            __llvm_profile_CurrentFilename, strerror(errno));
+    PROF_ERR("Failed to write file \"%s\": %s\n", Filename, strerror(errno));
   return rc;
 }
 
 static void writeFileWithoutReturn(void) { __llvm_profile_write_file(); }
 
 COMPILER_RT_VISIBILITY
 int __llvm_profile_register_write_file_atexit(void) {
   static int HasBeenRegistered = 0;