[profile] Do not cache __llvm_profile_get_filename result

[vsk]

Summary

When the %m filename pattern is used, the filename is unique to each
image, so the cached value is wrong.

It struck me that the full filename isn't something that's recomputed
often, so perhaps it doesn't need to be cached at all.

@davidxl pointed out we can go further and just hide lprofCurFilename.

rdar://55137071

[davidxl]

shared lib should have different profile name from the main executable so the test should check that two file names are not identical?

[vsk]

The test was not written clearly. I've added a clarifying comment. The test should exit with code 1 if the two filenames compare equal.

[davidxl]

I see.

Even without the patch, I saw two different files are created with %m. How is the bug triggered?

[vsk]

In D69137#1713779, @davidxl wrote:

I see.

Even without the patch, I saw two different files are created with %m. How is the bug triggered?

An internal adopter is making use of the libprofile APIs to dump profile data when a message is received in a server. I'm not sure what the exact API usage looks like (why not just call __llvm_profile_dump directly?). Perhaps the filename is used for logging.

[davidxl]

maybe we should make lprofCurFilename hidden?

[vsk]

In D69137#1713899, @davidxl wrote:

maybe we should make lprofCurFilename hidden?

Oh, you mean to stop relying on the dynamic loader coalescing all copies of the symbol? I think that would be nice.

Currently the runtime only relies on a coalesced lprofCurFilename symbol to avoid truncating a raw profile twice (IIRC, if FilenamePat is set => do not truncate). But we could easily use getenv/setenv to accomplish the same thing. This might simplify the code, the only cost is parsing the filename repeatedly.

[davidxl]

If we can clean up the code, that will be great.

[vsk]

Hide lprofCurFilename, per David's suggestion. This requires deleting 'instrprof-set-filename-shared.test'.

[davidxl]

Thanks. lgtm

[vsk]

FTR, I'm a bit conflicted about the latest version of this patch. On one hand, hiding lprofCurFilename decouples all of the copies of the profiling runtime in a process. On the other, it may cause a regression for users who use the set-filename API with instrumented DSOs. A quick search shows that we have no internal adopters who rely on the behavior, so I'm inclined to go ahead with this.

compiler-rt/lib/profile/InstrProfilingFile.c

 /* Data structure holding the result of parsed filename pattern. */
 typedef struct lprofFilename {
   /* File name string possibly with %p or %h specifiers. */
   const char *FilenamePat;
   /* A flag indicating if FilenamePat's memory is allocated
    * by runtime. */
   unsigned OwnsFilenamePat;
   const char *ProfilePathPrefix;
-  const char *Filename;
   char PidChars[MAX_PID_SIZE];
   char Hostname[COMPILER_RT_MAX_HOSTLEN];
   unsigned NumPids;
   unsigned NumHosts;
   /* When in-process merging is enabled, this parameter specifies
    * the total number of profile data files shared by all the processes
    * spawned from the same binary. By default the value is 1. If merging
    * is not enabled, its value should be 0. This parameter is specified
    * by the %[0-9]m specifier. For instance %2m enables merging using
    * 2 profile data files. %1m is equivalent to %m. Also %m specifier
    * can only appear once at the end of the name pattern. */
   unsigned MergePoolSize;
   ProfileNameSpecifier PNS;
 } lprofFilename;
 
-COMPILER_RT_WEAK lprofFilename lprofCurFilename = {0,   0, 0, 0, {0},
+COMPILER_RT_WEAK lprofFilename lprofCurFilename = {0,   0, 0, {0},
                                                    {0}, 0, 0, 0, PNS_unknown};
 
 static int ProfileMergeRequested = 0;
 static int isProfileMergeRequested() { return ProfileMergeRequested; }
 static void setProfileMergeRequested(int EnableMerge) {
   ProfileMergeRequested = EnableMerge;
 }
 
static int parseFilenamePattern(const char *FilenamePat,
   int NumPids = 0, NumHosts = 0, I;
   char *PidChars = &lprofCurFilename.PidChars[0];
   char *Hostname = &lprofCurFilename.Hostname[0];
   int MergingEnabled = 0;
 
   /* Clean up cached prefix and filename.  */
   if (lprofCurFilename.ProfilePathPrefix)
     free((void *)lprofCurFilename.ProfilePathPrefix);
-  if (lprofCurFilename.Filename)
-    free((void *)lprofCurFilename.Filename);
 
   if (lprofCurFilename.FilenamePat && lprofCurFilename.OwnsFilenamePat) {
     free((void *)lprofCurFilename.FilenamePat);
   }
 
   memset(&lprofCurFilename, 0, sizeof(lprofCurFilename));
 
   if (!CopyFilenamePat)

 }
 
 COMPILER_RT_VISIBILITY
 const char *__llvm_profile_get_filename(void) {
   int Length;
   char *FilenameBuf;
   const char *Filename;
 
-  if (lprofCurFilename.Filename)
-    return lprofCurFilename.Filename;
-
   Length = getCurFilenameLength();
   FilenameBuf = (char *)malloc(Length + 1);
   if (!FilenameBuf) {
     PROF_ERR("Failed to %s\n", "allocate memory.");
     return "\0";
   }
   Filename = getCurFilename(FilenameBuf, 1);
   if (!Filename)
     return "\0";
 
-  lprofCurFilename.Filename = FilenameBuf;
   return FilenameBuf;
 }
 
 /* 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

compiler-rt/test/profile/Inputs/instrprof-get-filename-dso.c

+const char *__llvm_profile_get_filename(void);
+
+const char *get_filename_from_DSO(void) {
+  return __llvm_profile_get_filename();
+}

compiler-rt/test/profile/instrprof-get-filename-merge-mode.c

+// Test __llvm_profile_get_filename when the on-line merging mode is enabled.
+//
+// RUN: %clang_pgogen -dynamiclib -o %t.dso %p/Inputs/instrprof-get-filename-dso.c
+// RUN: %clang_pgogen -o %t %s %t.dso
+// RUN: env LLVM_PROFILE_FILE="%t-%m.profraw" %run %t
+
+#include <string.h>
+
+const char *__llvm_profile_get_filename(void);
+extern const char *get_filename_from_DSO(void);
+
+int main(int argc, const char *argv[]) {
+  const char *filename1 = __llvm_profile_get_filename();
+  const char *filename2 = get_filename_from_DSO();
+  return strcmp(filename1, filename2) == 0;
+}