[PATCH] D21056: [profile] in-process profile merging support Part-3

[Vedant Kumar via llvm-commits]

vsk added a comment.

Awesome! Some comments/questions inline --


================
Comment at: lib/profile/InstrProfilingFile.c:99
@@ +98,3 @@
+
+  fseek(ProfileFile, 0L, SEEK_END);
+  ProfileFileSize = ftell(ProfileFile);
----------------
Possible dropped error here

================
Comment at: lib/profile/InstrProfilingFile.c:102
@@ +101,3 @@
+  /* Restore file offset.  */
+  fseek(ProfileFile, 0L, SEEK_SET);
+  if (ProfileFileSize < sizeof(__llvm_profile_header)) {
----------------
Ditto

================
Comment at: lib/profile/InstrProfilingFile.c:104
@@ +103,3 @@
+  if (ProfileFileSize < sizeof(__llvm_profile_header)) {
+    if (ProfileFileSize)
+      PROF_WARN("Unable to merge profile data: %s\n",
----------------
Could we skip these two checks by moving the call to `__llvm_profile_check_compatibility` here?

================
Comment at: lib/profile/InstrProfilingFile.c:110
@@ +109,3 @@
+
+  ProfileBuffer = mmap(0, ProfileFileSize, PROT_READ, MAP_SHARED | MAP_FILE,
+                       fileno(ProfileFile), 0);
----------------
0 -> NULL

================
Comment at: lib/profile/InstrProfilingFile.c:112
@@ +111,3 @@
+                       fileno(ProfileFile), 0);
+  if (ProfileBuffer == (void *)-1) {
+    PROF_ERR("Unable to merge profile data, mmap failed: %s\n",
----------------
MAP_FAILED instead of -1?

================
Comment at: lib/profile/InstrProfilingFile.c:149
@@ +148,3 @@
+  rc = doProfileMerging(ProfileFile);
+  if (rc || COMPILER_RT_FTRUNCATE(ProfileFile, 0L)) {
+    PROF_ERR("Profile Merging of file %s failed: %s\n", ProfileFileName,
----------------
I think any successful call to `doProfileMerging` ought to be followed by COMPILER_RT_FTRUNCATE. Sink it into the function?

================
Comment at: lib/profile/InstrProfilingFile.c:155
@@ +154,3 @@
+  }
+  fseek(ProfileFile, 0L, SEEK_SET);
+  return ProfileFile;
----------------
Ditto, fseek error

================
Comment at: lib/profile/InstrProfilingFile.c:245
@@ +244,3 @@
+        if (MergingEnabled || (FilenamePat[I] == 'm' && FilenamePat[I + 1]) ||
+            (FilenamePat[I + 1] == 'm' && FilenamePat[I + 2])) {
+          PROF_WARN(
----------------
Should this check be: `if (MergingEnabled && (FilenamePat[I] == 'm' || FilenamePat[I + 1] == 'm'))`? Why check whether the character after the 'm' is nil?

================
Comment at: lib/profile/InstrProfilingFile.c:280
@@ -170,2 +279,3 @@
  * filename with PID and hostname substitutions. */
+#define SIGLEN 24
 static int getCurFilenameLength() {
----------------
Could you add a comment explaining that '24' comes from `lprofGetLoadModuleSignature`? In fact, it might be better to hide this detail in a helper function in InstrProfilingMerge.c.

================
Comment at: lib/profile/InstrProfilingFile.c:326
@@ +325,3 @@
+                 (FilenamePat[I] >= '1' && FilenamePat[I] <= '9' &&
+                  FilenamePat[I + 1] == 'm')) {
+        char LoadModuleSignature[SIGLEN];
----------------
Can this predicate be shared with `parseFilenamePattern`? I'd prefer not to have it duplicated.

================
Comment at: test/profile/instrprof-basic.c:18
@@ -5,2 +17,3 @@
+// RUN: %clang_profuse=%t.m.profdata -o - -S -emit-llvm %s | FileCheck %s --check-prefix=COMMON --check-prefix=MERGE
 
 int begin(int i) {
----------------
Is the `lprofGetLoadModuleSignature` code path covered?


http://reviews.llvm.org/D21056