[Profile][NFC] Clean up initializeProfileForContinuousMode

[zequanwu]

Summary

Merge two versions of initializeProfileForContinuousMode function into one.

[zequanwu]

Update.

[vsk]

Thanks!

compiler-rt/lib/profile/InstrProfilingFile.c

442

Will these defines be needed in your follow-up patches? If so, it might be worth moving them to the global namespace now. If not, 'static' can be dropped.

466

The difference in fileOpenMode ('a+b' vs. 'w+b') is probably worth a TODO or FIXME. But it makes sense to keep this patch NFC.

497

These checks are useful for debugging, and should be safe to keep in the useBiasVar == 0 path. Any reason to delete them?

497

The new "CountersOffset" cannot be used when useBiasVar == 0, so I suggest renaming it (maybe "CountersOffsetInBiasMode") and sinking it closer to its use.

507–512

I guess warnIfNonZero can also be deleted.

531

Why not keep the 'Grow the profile ...' comment?

558–559

Why not keep the 'so long as there is at least one counter' comment?

[zequanwu]

Address comments.

[zequanwu]

compiler-rt/lib/profile/InstrProfilingFile.c

442

Yes, those checks will still be needed in the follow-up patches.
Moved up.

466

What would the TODO/FIXME about? Are we going to change it in the future?

497

Those checks failed when running profile tests on Linux.
Moved to the useBiasVar == 0 path.

531

Added.

[zequanwu]

Fix errors.

[vsk]

Thanks, lgtm.

compiler-rt/lib/profile/InstrProfilingFile.c

466

If I've understood correctly, fileOpenMode is used in the !doMerging() case, and the append behavior ("a+b") is the correct one.

Consider the case where you have two instrumented DSOs, both using continuous mode with !doMerging(): then as the runtime initializer in the second DSO does initializeProfileForContinuousMode(), it will truncate + clobber the profile written by the initializer in the first DSO.

497

Thanks, moving the checks to the useBiasVar == 0 path is the right thing to do, since that's the path where we require certain profile sections to be page aligned.

[zequanwu]

Capitalize first letter of global variables.

compiler-rt/lib/profile/InstrProfilingFile.c

466

it will truncate + clobber the profile written by the initializer in the first DSO.

I don't understand that. If the file is opened with "a+b", it will just append new profile to the end of existing profile file.

[vsk]

compiler-rt/lib/profile/InstrProfilingFile.c

466

Right, but if the file is opened with "w+b", then the second DSO's initializer will truncate the profile, erasing the data written by the first DSO.

[zequanwu]

Add a TODO.

compiler-rt/lib/profile/InstrProfilingFile.c

466

Oh, I see. You are saying the non-apple platform. It is problematic in that case. Added a TODO.

compiler-rt/lib/profile/InstrProfilingFile.c

#endif
 
   /* Truncate the file.  Later we'll reopen and append. */
   File = fopen(Filename, "w");
   if (!File)
     return;
   fclose(File);
 }
 
-// TODO: Move these functions into InstrProfilingPlatform* files.
-#if defined(__APPLE__)
 /* Write a partial profile to \p Filename, which is required to be backed by
  * the open file object \p File. */
 static int writeProfileWithFileObject(const char *Filename, FILE *File) {
   setProfileFile(File);
   int rc = writeFile(Filename);
   if (rc)
     PROF_ERR("Failed to write file \"%s\": %s\n", Filename, strerror(errno));
   setProfileFile(NULL);
   return rc;
 }
 
-/* Unlock the profile \p File and clear the unlock flag. */
-static void unlockProfile(int *ProfileRequiresUnlock, FILE *File) {
-  if (!*ProfileRequiresUnlock) {
-    PROF_WARN("%s", "Expected to require profile unlock\n");
-  }
-
-  lprofUnlockFileHandle(File);
-  *ProfileRequiresUnlock = 0;
-}
-
 static void initializeProfileForContinuousMode(void) {
-  if (!__llvm_profile_is_continuous_mode_enabled())
+#if defined(__APPLE__)
+  static const int continuousModeSupported = 1;
+  static const int useBiasVar = 0;
+  static const char *fileOpenMode = "a+b";
+  intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+  (void)INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+#elif defined(__ELF__) || defined(_WIN32)
+#if defined(_WIN32)
+  COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+#if defined(_M_IX86) || defined(__i386__)
+#define WIN_SYM_PREFIX "_"
+#else
+#define WIN_SYM_PREFIX
+#endif
+#pragma comment(                                                               \
+    linker, "/alternatename:" WIN_SYM_PREFIX INSTR_PROF_QUOTE(                 \
+                INSTR_PROF_PROFILE_COUNTER_BIAS_VAR) "=" WIN_SYM_PREFIX        \
+                INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))
+#else
+  COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR
+      __attribute__((weak, alias(INSTR_PROF_QUOTE(
+                               INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))));
+#endif
+  static const int continuousModeSupported = 1;
+  static const int useBiasVar = 1;
+  static const char *fileOpenMode = "w+b";
+#define INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR                            \
+  INSTR_PROF_CONCAT(INSTR_PROF_PROFILE_COUNTER_BIAS_VAR, _default)
+  intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR = 0;
+  /* This symbol is defined by the compiler when runtime counter relocation is
+   * used and runtime provides a weak alias so we can check if it's defined.
+   */
+  void *BiasAddr = &INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+  void *BiasDefaultAddr = &INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR;
+  if (BiasAddr == BiasDefaultAddr) {
+    PROF_ERR("%s\n", "__llvm_profile_counter_bias is undefined");
+    return;
+  }
+#else
+  static const int useBiasVar = 0;
+  static const int continuousModeSupported = 0;
+#endif
+  if (!__llvm_profile_is_continuous_mode_enabled() || !continuousModeSupported)
     return;
 
   /* Get the sizes of various profile data sections. Taken from
    * __llvm_profile_get_size_for_buffer(). */
   const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
   const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
   const uint64_t *CountersBegin = __llvm_profile_begin_counters();
   const uint64_t *CountersEnd = __llvm_profile_end_counters();
   const char *NamesBegin = __llvm_profile_begin_names();
   const char *NamesEnd = __llvm_profile_end_names();
   const uint64_t NamesSize = (NamesEnd - NamesBegin) * sizeof(char);
   uint64_t DataSize = __llvm_profile_get_data_size(DataBegin, DataEnd);
   uint64_t CountersSize = CountersEnd - CountersBegin;
-
-  /* Check that the counter and data sections in this image are page-aligned. */
-  unsigned PageSize = getpagesize();
-  if ((intptr_t)CountersBegin % PageSize != 0) {
-    PROF_ERR("Counters section not page-aligned (start = %p, pagesz = %u).\n",
-             CountersBegin, PageSize);
-    return;
-  }
-  if ((intptr_t)DataBegin % PageSize != 0) {
-    PROF_ERR("Data section not page-aligned (start = %p, pagesz = %u).\n",
-             DataBegin, PageSize);
-    return;
-  }
+  const uint64_t CountersOffset =
+      sizeof(__llvm_profile_header) + (DataSize * sizeof(__llvm_profile_data));
 
   int Length = getCurFilenameLength();
   char *FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
   const char *Filename = getCurFilename(FilenameBuf, 0);
   if (!Filename)
     return;
 
   FILE *File = NULL;
   off_t CurrentFileOffset = 0;
-  off_t OffsetModPage = 0;
-
-  /* Whether an exclusive lock on the profile must be dropped after init.
-   * Use a cleanup to warn if the unlock does not occur. */
-  COMPILER_RT_CLEANUP(warnIfNonZero) int ProfileRequiresUnlock = 0;
-
-  if (!doMerging()) {
-    /* We are not merging profiles, so open the raw profile in append mode. */
-    File = fopen(Filename, "a+b");
-    if (!File)
-      return;
-
-    /* Check that the offset within the file is page-aligned. */
-    CurrentFileOffset = ftello(File);
-    OffsetModPage = CurrentFileOffset % PageSize;
-    if (OffsetModPage != 0) {
-      PROF_ERR("Continuous counter sync mode is enabled, but raw profile is not"
-               "page-aligned. CurrentFileOffset = %" PRIu64 ", pagesz = %u.\n",
-               (uint64_t)CurrentFileOffset, PageSize);
-      return;
-    }
-
-    /* Grow the profile so that mmap() can succeed.  Leak the file handle, as
-     * the file should stay open. */
-    if (writeProfileWithFileObject(Filename, File) != 0)
-      return;
-  } else {
+  if (doMerging()) {
     /* We are merging profiles. Map the counter section as shared memory into
      * the profile, i.e. into each participating process. An increment in one
      * process should be visible to every other process with the same counter
      * section mapped. */
     File = lprofOpenFileEx(Filename);
     if (!File)
       return;
 
-    ProfileRequiresUnlock = 1;
-
-    uint64_t ProfileFileSize;
-    if (getProfileFileSizeForMerging(File, &ProfileFileSize) == -1)
-      return unlockProfile(&ProfileRequiresUnlock, File);
-
+    uint64_t ProfileFileSize = 0;
+    if (getProfileFileSizeForMerging(File, &ProfileFileSize) == -1) {
+      lprofUnlockFileHandle(File);
+      fclose(File);
+      return;
+    }
     if (ProfileFileSize == 0) {
-      /* Grow the profile so that mmap() can succeed.  Leak the file handle, as
-       * the file should stay open. */
-      if (writeProfileWithFileObject(Filename, File) != 0)
-        return unlockProfile(&ProfileRequiresUnlock, File);
+      if (writeProfileWithFileObject(Filename, File) != 0) {
+        lprofUnlockFileHandle(File);
+        fclose(File);
+        return;
+      }
     } else {
       /* The merged profile has a non-zero length. Check that it is compatible
        * with the data in this process. */
       char *ProfileBuffer;
       if (mmapProfileForMerging(File, ProfileFileSize, &ProfileBuffer) == -1 ||
-          munmap(ProfileBuffer, ProfileFileSize) == -1)
-        return unlockProfile(&ProfileRequiresUnlock, File);
+          munmap(ProfileBuffer, ProfileFileSize)) {
+        lprofUnlockFileHandle(File);
+        fclose(File);
+        return;
+      }
+    }
+  } else {
+    File = fopen(Filename, fileOpenMode);
+    if (!File)
+      return;
+    /* Check that the offset within the file is page-aligned. */
+    CurrentFileOffset = ftello(File);
+    unsigned PageSize = getpagesize();
+    if (CurrentFileOffset % PageSize != 0) {
+      PROF_ERR("Continuous counter sync mode is enabled, but raw profile is not"
+               "page-aligned. CurrentFileOffset = %" PRIu64 ", pagesz = %u.\n",
+               (uint64_t)CurrentFileOffset, PageSize);
+      return;
+    }
+    if (writeProfileWithFileObject(Filename, File) != 0) {
+      fclose(File);
+      return;
     }
   }
 
-  /* mmap() the profile counters so long as there is at least one counter.
-   * If there aren't any counters, mmap() would fail with EINVAL. */
   if (CountersSize > 0) {
     int Fileno = fileno(File);
+    if (useBiasVar) {
+      /* Get the file size. */
+      uint64_t FileSize = ftell(File);
+
+      /* Map the profile. */
+      char *Profile = (char *)mmap(NULL, FileSize, PROT_READ | PROT_WRITE,
+                                   MAP_SHARED, Fileno, 0);
+      if (Profile == MAP_FAILED) {
+        PROF_ERR("Unable to mmap profile: %s\n", strerror(errno));
+        return;
+      }
+      /* Update the profile fields based on the current mapping. */
+      INSTR_PROF_PROFILE_COUNTER_BIAS_VAR =
+          (intptr_t)Profile - (uintptr_t)CountersBegin + CountersOffset;
 
-    /* Determine how much padding is needed before/after the counters and after
-     * the names. */
+      /* Return the memory allocated for counters to OS. */
+      lprofReleaseMemoryPagesToOS((uintptr_t)CountersBegin,
+                                  (uintptr_t)CountersEnd);
+    } else {
+      /* Determine how much padding is needed before/after the counters and
+       * after the names. */
       uint64_t PaddingBytesBeforeCounters, PaddingBytesAfterCounters,
           PaddingBytesAfterNames;
       __llvm_profile_get_padding_sizes_for_counters(
           DataSize, CountersSize, NamesSize, &PaddingBytesBeforeCounters,
           &PaddingBytesAfterCounters, &PaddingBytesAfterNames);
 
       uint64_t PageAlignedCountersLength =
           (CountersSize * sizeof(uint64_t)) + PaddingBytesAfterCounters;
       uint64_t FileOffsetToCounters =
           CurrentFileOffset + sizeof(__llvm_profile_header) +
           (DataSize * sizeof(__llvm_profile_data)) + PaddingBytesBeforeCounters;
 
-    uint64_t *CounterMmap = (uint64_t *)mmap(
-        (void *)CountersBegin, PageAlignedCountersLength, PROT_READ | PROT_WRITE,
-        MAP_FIXED | MAP_SHARED, Fileno, FileOffsetToCounters);
+      uint64_t *CounterMmap =
+          (uint64_t *)mmap((void *)CountersBegin, PageAlignedCountersLength,
+                           PROT_READ | PROT_WRITE, MAP_FIXED | MAP_SHARED,
+                           Fileno, FileOffsetToCounters);
       if (CounterMmap != CountersBegin) {
         PROF_ERR(
             "Continuous counter sync mode is enabled, but mmap() failed (%s).\n"
             "  - CountersBegin: %p\n"
             "  - PageAlignedCountersLength: %" PRIu64 "\n"
             "  - Fileno: %d\n"
             "  - FileOffsetToCounters: %" PRIu64 "\n",
             strerror(errno), CountersBegin, PageAlignedCountersLength, Fileno,
             FileOffsetToCounters);
       }
     }
-
-  if (ProfileRequiresUnlock)
-    unlockProfile(&ProfileRequiresUnlock, File);
   }
-#elif defined(__ELF__) || defined(_WIN32)
 
-#define INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR                            \
-  INSTR_PROF_CONCAT(INSTR_PROF_PROFILE_COUNTER_BIAS_VAR, _default)
-intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR = 0;
-
-/* This variable is a weak external reference which could be used to detect
- * whether or not the compiler defined this symbol. */
-#if defined(_WIN32)
-COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
-#if defined(_M_IX86) || defined(__i386__)
-#define WIN_SYM_PREFIX "_"
-#else
-#define WIN_SYM_PREFIX
-#endif
-#pragma comment(                                                               \
-    linker, "/alternatename:" WIN_SYM_PREFIX INSTR_PROF_QUOTE(                 \
-                INSTR_PROF_PROFILE_COUNTER_BIAS_VAR) "=" WIN_SYM_PREFIX        \
-                INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))
-#else
-COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR
-    __attribute__((weak, alias(INSTR_PROF_QUOTE(
-                             INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))));
-#endif
-
-static int writeMMappedFile(FILE *OutputFile, char **Profile) {
-  if (!OutputFile)
-    return -1;
-
-  /* Write the data into a file. */
-  setupIOBuffer();
-  ProfDataWriter fileWriter;
-  initFileWriter(&fileWriter, OutputFile);
-  if (lprofWriteData(&fileWriter, NULL, 0)) {
-    PROF_ERR("Failed to write profile: %s\n", strerror(errno));
-    return -1;
-  }
-  fflush(OutputFile);
-
-  /* Get the file size. */
-  uint64_t FileSize = ftell(OutputFile);
-
-  /* Map the profile. */
-  *Profile = (char *)mmap(
-      NULL, FileSize, PROT_READ | PROT_WRITE, MAP_SHARED, fileno(OutputFile), 0);
-  if (*Profile == MAP_FAILED) {
-    PROF_ERR("Unable to mmap profile: %s\n", strerror(errno));
-    return -1;
-  }
-
-  return 0;
-}
-
-static void initializeProfileForContinuousMode(void) {
-  if (!__llvm_profile_is_continuous_mode_enabled())
-    return;
-
-  /* This symbol is defined by the compiler when runtime counter relocation is
-   * used and runtime provides a weak alias so we can check if it's defined. */
-  void *BiasAddr = &INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
-  void *BiasDefaultAddr = &INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR;
-  if (BiasAddr == BiasDefaultAddr) {
-    PROF_ERR("%s\n", "__llvm_profile_counter_bias is undefined");
-    return;
-  }
-
-  /* Get the sizes of various profile data sections. Taken from
-   * __llvm_profile_get_size_for_buffer(). */
-  const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
-  const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
-  const uint64_t *CountersBegin = __llvm_profile_begin_counters();
-  const uint64_t *CountersEnd = __llvm_profile_end_counters();
-  uint64_t DataSize = __llvm_profile_get_data_size(DataBegin, DataEnd);
-  const uint64_t CountersOffset =
-      sizeof(__llvm_profile_header) + (DataSize * sizeof(__llvm_profile_data));
-
-  int Length = getCurFilenameLength();
-  char *FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
-  const char *Filename = getCurFilename(FilenameBuf, 0);
-  if (!Filename)
-    return;
-
-  FILE *File = NULL;
-  char *Profile = NULL;
-
-  if (!doMerging()) {
-    File = fopen(Filename, "w+b");
-    if (!File)
-      return;
-
-    if (writeMMappedFile(File, &Profile) == -1) {
-      fclose(File);
-      return;
-    }
-  } else {
-    File = lprofOpenFileEx(Filename);
-    if (!File)
-      return;
-
-    uint64_t ProfileFileSize = 0;
-    if (getProfileFileSizeForMerging(File, &ProfileFileSize) == -1) {
-      lprofUnlockFileHandle(File);
-      fclose(File);
-      return;
-    }
-
-    if (!ProfileFileSize) {
-      if (writeMMappedFile(File, &Profile) == -1) {
-        fclose(File);
-        return;
-      }
-    } else {
-      /* The merged profile has a non-zero length. Check that it is compatible
-       * with the data in this process. */
-      if (mmapProfileForMerging(File, ProfileFileSize, &Profile) == -1) {
-        fclose(File);
-        return;
-      }
-    }
+  if (doMerging()) {
 
-    lprofUnlockFileHandle(File);
-  }
-
-  /* Update the profile fields based on the current mapping. */
-  INSTR_PROF_PROFILE_COUNTER_BIAS_VAR =
-      (intptr_t)Profile - (uintptr_t)CountersBegin +
-      CountersOffset;
-
-  /* Return the memory allocated for counters to OS. */
-  lprofReleaseMemoryPagesToOS((uintptr_t)CountersBegin, (uintptr_t)CountersEnd);
-}
-#else
-static void initializeProfileForContinuousMode(void) {
-  PROF_ERR("%s\n", "continuous mode is unsupported on this platform");
-}
-#endif
-
 static const char *DefaultProfileName = "default.profraw";
 static void resetFilenameToDefault(void) {
   if (lprofCurFilename.FilenamePat && lprofCurFilename.OwnsFilenamePat) {
     free((void *)lprofCurFilename.FilenamePat);
   }
   memset(&lprofCurFilename, 0, sizeof(lprofCurFilename));
   lprofCurFilename.FilenamePat = DefaultProfileName;
   lprofCurFilename.PNS = PNS_default;