[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.

clang/lib/Driver/ToolChains/Darwin.cpp

   if (hasExportSymbolDirective(Args)) {
     if (needsGCovInstrumentation(Args)) {
       addExportedSymbol(CmdArgs, "___gcov_flush");
       addExportedSymbol(CmdArgs, "_flush_fn_list");
       addExportedSymbol(CmdArgs, "_writeout_fn_list");
     } else {
       addExportedSymbol(CmdArgs, "___llvm_profile_filename");
       addExportedSymbol(CmdArgs, "___llvm_profile_raw_version");
-      addExportedSymbol(CmdArgs, "_lprofCurFilename");
     }
     addExportedSymbol(CmdArgs, "_lprofDirMode");
   }
 }
 
 void DarwinClang::AddLinkSanitizerLibArgs(const ArgList &Args,
                                           ArgStringList &CmdArgs,
                                           StringRef Sanitizer,

clang/test/Driver/darwin-ld.c

 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate -Wl,-exported_symbols_list,/dev/null -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=PROFILE_EXPORT %s < %t.log
 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate -Wl,-exported_symbol,foo -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=PROFILE_EXPORT %s < %t.log
 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate -Xlinker -exported_symbol -Xlinker foo -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=PROFILE_EXPORT %s < %t.log
 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate -Xlinker -exported_symbols_list -Xlinker /dev/null -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=PROFILE_EXPORT %s < %t.log
-// PROFILE_EXPORT: "-exported_symbol" "___llvm_profile_filename" "-exported_symbol" "___llvm_profile_raw_version" "-exported_symbol" "_lprofCurFilename"
+// PROFILE_EXPORT: "-exported_symbol" "___llvm_profile_filename" "-exported_symbol" "___llvm_profile_raw_version"
 //
 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate --coverage -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=NO_PROFILE_EXPORT %s < %t.log
 // NO_PROFILE_EXPORT-NOT: "-exported_symbol"
 //
 // RUN: %clang -target x86_64-apple-darwin12 --coverage -exported_symbols_list /dev/null -### %t.o 2> %t.log
 // RUN: FileCheck -check-prefix=GCOV_EXPORT %s < %t.log
 // RUN: %clang -target x86_64-apple-darwin12 -fprofile-arcs -Wl,-exported_symbols_list,/dev/null -### %t.o 2> %t.log

compiler-rt/lib/profile/InstrProfiling.h

 /*!
  * \brief Set the filename for writing instrumentation data.
  *
  * Sets the filename to be used for subsequent calls to
  * \a __llvm_profile_write_file().
  *
  * \c Name is not copied, so it must remain valid.  Passing NULL resets the
  * filename logic to the default behaviour.
+ *
+ * Note: There may be multiple copies of the profile runtime (one for each
+ * instrumented image/DSO). This API only modifies the filename within the
+ * copy of the runtime available to the calling image.
  */
 void __llvm_profile_set_filename(const char *Name);
 
 /*!
  * \brief Set the FILE object for writing instrumentation data.
  *
  * Sets the FILE object to be used for subsequent calls to
  * \a __llvm_profile_write_file(). The profile file name set by environment
  * variable, command-line option, or calls to \a  __llvm_profile_set_filename
  * will be ignored.
  *
  * \c File will not be closed after a call to \a __llvm_profile_write_file() but
  * it may be flushed. Passing NULL restores default behavior.
  *
  * If \c EnableMerge is nonzero, the runtime will always merge profiling data
  * with the contents of the profiling file. If EnableMerge is zero, the runtime
  * may still merge the data if it would have merged for another reason (for
  * example, because of a %m specifier in the file name).
+ *
+ * Note: There may be multiple copies of the profile runtime (one for each
+ * instrumented image/DSO). This API only modifies the file object within the
+ * copy of the runtime available to the calling image.
  */
 void __llvm_profile_set_file_object(FILE *File, int EnableMerge);
 
 /*! \brief Register to write instrumentation data to file at exit. */
 int __llvm_profile_register_write_file_atexit(void);
 
 /*! \brief Initialize file handling. */
 void __llvm_profile_initialize_file(void);
 
 /*!
  * \brief Return path prefix (excluding the base filename) of the profile data.
  * This is useful for users using \c -fprofile-generate=./path_prefix who do
  * not care about the default raw profile name. It is also useful to collect
  * more than more profile data files dumped in the same directory (Online
  * merge mode is turned on for instrumented programs with shared libs).
  * Side-effect: this API call will invoke malloc with dynamic memory allocation.
  */
 const char *__llvm_profile_get_path_prefix();
 
 /*!
  * \brief Return filename (including path) of the profile data. Note that if the
  * user calls __llvm_profile_set_filename later after invoking this interface,
  * the actual file name may differ from what is returned here.
- * Side-effect: this API call will invoke malloc with dynamic memory allocation.
+ * Side-effect: this API call will invoke malloc with dynamic memory allocation
+ * (the returned pointer must be passed to `free` to avoid a leak).
+ *
+ * Note: There may be multiple copies of the profile runtime (one for each
+ * instrumented image/DSO). This API only retrieves the filename from the copy
+ * of the runtime available to the calling image.
  */
 const char *__llvm_profile_get_filename();
 
 /*! \brief Get the magic token for the file format. */
 uint64_t __llvm_profile_get_magic(void);
 
 /*! \brief Get the version of the file format. */
 uint64_t __llvm_profile_get_version(void);

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},
-                                                   {0}, 0, 0, 0, PNS_unknown};
+static 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 FILE *ProfileFile = NULL;
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/Posix/instrprof-set-filename-shared.test

-# Test that __llvm_profile_set_filename is honored by shared libary too.
-RUN: mkdir -p %t.d
-RUN: %clang_profgen=%t.shared.profraw -fPIC -shared -o %t.d/t.shared %S/../Inputs/instrprof-dlopen-func.c
-RUN: %clang_profgen -DCALL_SHARED -o %t.m -O3 -rpath %t.d %t.d/t.shared %S/../instrprof-set-filename.c
-RUN: %run %t.m %t.main.profraw
-RUN: llvm-profdata show %t.main.profraw | FileCheck --check-prefix=SHARED %s
-
-# SHARED: Total functions: 2

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();
+
+  // Exit with code 1 if the two filenames are the same.
+  return strcmp(filename1, filename2) == 0;
+}