[PATCH] [Profile] compiler-rt support for setting profile output from command line

[Justin Bogner]

Teresa Johnson <tejohnson at google.com> writes:
> This change is the first of 3 patches to add support for specifying
> the profile output from the command line via -fprofile-instr-generate=<path>,
> where the specified output path/file will be overridden by the
> LLVM_PROFILE_FILE environment variable.
>
> Several changes are made to the runtime to support this:
> 1) Add a new interface __llvm_profile_set_filename_env_override that will
> set the profile output filename, but allows the LLVM_PROFILE_FILE to override.
> This is the interface used by the new option.
> 2) Refactor the pid-expansion done for LLVM_PROFILE_FILE into a separate
> routine that can be shared by the various filename setting routines
> (so that the filename from the option can also use the "%p" syntax).
> 3) Move the truncation into setFilename, and only truncate if there is a
> new filename specified (to maintain support for appending to the same
> profile file in the case of multiple shared objects built with profiling).
> 4) Move the handling for a NULL filename passed to __llvm_profile_setfilename*
> into the new setFilenamePossiblyWithPid routine. The handling for a null
> LLVM_PROFILE_FILE (which should not reset) is done by caller
> setFilenameFromEnvironment.

Cool. Thanks for working on this! I have a few comments, below.

> http://reviews.llvm.org/D9323
>
> Files:
>   lib/profile/InstrProfiling.h
>   lib/profile/InstrProfilingFile.c
>   test/profile/instrprof-set-filename-env-override.c
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
> Index: lib/profile/InstrProfiling.h
> ===================================================================
> --- lib/profile/InstrProfiling.h
> +++ lib/profile/InstrProfiling.h
> @@ -77,6 +77,18 @@
>   */
>  void __llvm_profile_set_filename(const char *Name);
>  
> +/*!
> + * \brief Set the filename for writing instrumentation data, unless the
> + * \c LLVM_PROFILE_FILE environment variable was set.
> + *
> + * Unless overridden, 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.

I'm not sure that passing NULL does what you say it does here. It looks
to me like it'll end up setting __llvm_profile_CurrentFilename to NULL,
but __llvm_profile_initialize_file has almost certainly already run, so
we'll just fail when __llvm_profile_write_file gets called later.

> + */
> +void __llvm_profile_set_filename_env_override(const char *Name);

I find this function name confusing - it sounds to me like it overrides
the environment, which is the exact opposite of what happens. I don't
have a particularly good alternative though, as something like
__llvm_profile_set_filename_unless_env_is_set is a bit verbose.

Maybe setDefaultFilename should be renamed to "resetFilenameToDefault",
and this can be set_default_filename or override_default_filename?

> +
>  /*! \brief Register to write instrumentation data to file at exit. */
>  int __llvm_profile_register_write_file_atexit(void);
>  
> Index: lib/profile/InstrProfilingFile.c
> ===================================================================
> --- lib/profile/InstrProfilingFile.c
> +++ lib/profile/InstrProfilingFile.c
> @@ -76,14 +76,6 @@
>  __attribute__((weak)) int __llvm_profile_OwnsFilename = 0;
>  __attribute__((weak)) const char *__llvm_profile_CurrentFilename = NULL;
>  
> -static void setFilename(const char *Filename, int OwnsFilename) {
> -  if (__llvm_profile_OwnsFilename)
> -    free(UNCONST(__llvm_profile_CurrentFilename));
> -
> -  __llvm_profile_CurrentFilename = Filename;
> -  __llvm_profile_OwnsFilename = OwnsFilename;
> -}
> -
>  static void truncateCurrentFile(void) {
>    const char *Filename;
>    FILE *File;
> @@ -99,19 +91,36 @@
>    fclose(File);
>  }
>  
> +static void setFilename(const char *Filename, int OwnsFilename) {
> +  /* 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;
> +
> +  /* If not a new file, append to support profiling multiple shared objects. */
> +  if (NewFile)
> +    truncateCurrentFile();
> +}
> +
>  static void setDefaultFilename(void) { setFilename("default.profraw", 0); }
>  
>  int getpid(void);
> -static int setFilenameFromEnvironment(void) {
> -  const char *Filename = getenv("LLVM_PROFILE_FILE");
> +static int setFilenamePossiblyWithPid(const char *Filename) {
>  #define MAX_PID_SIZE 16
>    char PidChars[MAX_PID_SIZE] = {0};
>    int NumPids = 0, PidLength = 0;
>    char *Allocated;
>    int I, J;
>  
> -  if (!Filename || !Filename[0])
> -    return -1;
> +  /* Reset filename on NULL, except for env var which is checked by caller. */
> +  if (!Filename) {
> +    setFilename(Filename, 0);

I guess this should be setDefaultFilename, as per my comment above.

> +    return 0;
> +  }
>  
>    /* Check the filename for "%p", which indicates a pid-substitution. */
>    for (I = 0; Filename[I]; ++I)
> @@ -148,6 +157,15 @@
>    return 0;
>  }
>  
> +static int setFilenameFromEnvironment(void) {
> +  const char *Filename = getenv("LLVM_PROFILE_FILE");
> +
> +  if (!Filename || !Filename[0])
> +    return -1;
> +
> +  return setFilenamePossiblyWithPid(Filename);
> +}
> +
>  static void setFilenameAutomatically(void) {
>    if (!setFilenameFromEnvironment())
>      return;
> @@ -163,13 +181,20 @@
>  
>    /* Detect the filename and truncate. */
>    setFilenameAutomatically();
> -  truncateCurrentFile();
>  }
>  
>  __attribute__((visibility("hidden")))
>  void __llvm_profile_set_filename(const char *Filename) {
> -  setFilename(Filename, 0);
> -  truncateCurrentFile();
> +  setFilenamePossiblyWithPid(Filename);
> +}
> +
> +__attribute__((visibility("hidden")))
> +void __llvm_profile_set_filename_env_override(const char *Filename) {
> +  /* If the env var is set, skip setting filename from argument. */
> +  const char *Env_Filename = getenv("LLVM_PROFILE_FILE");
> +  if (Env_Filename && Env_Filename[0])
> +    return;
> +  setFilenamePossiblyWithPid(Filename);
>  }
>  
>  __attribute__((visibility("hidden")))
> Index: test/profile/instrprof-set-filename-env-override.c
> ===================================================================
> --- /dev/null
> +++ test/profile/instrprof-set-filename-env-override.c
> @@ -0,0 +1,14 @@
> +// RUN: %clang_profgen -o %t -O3 %s
> +// RUN: env LLVM_PROFILE_FILE=%t.good.profraw %run %t %t.bad.profraw
> +// RUN: llvm-profdata merge -o %t.profdata %t.good.profraw
> +// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s
> +
> +void __llvm_profile_set_filename_env_override(const char *);
> +int main(int argc, const char *argv[]) {
> +  // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
> +  if (argc < 2)
> +    return 1;
> +  __llvm_profile_set_filename_env_override(argv[1]);
> +  return 0;
> +}
> +// CHECK: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}

Please also test that this DTRT when LLVM_PROFILE_FILE isn't set, and
that calling it with NULL resets to the default correctly.

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits