[PATCH] D19298: [profile] Support for memory-mapping counters to a raw profile

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/profile/InstrProfilingFile.c:431
@@ +430,3 @@
+
+  if (!lprofCheckIfAligned((size_t)CountersBegin, PageSize))
+    return -1;
----------------
This is another reason there is need to let driver teach the linker to align the combined counter variable section properly. Silently failing like this is not user friendly.

================
Comment at: lib/profile/InstrProfilingFile.c:436
@@ +435,3 @@
+   * Expect a locked file stream in \p Stream. */
+  if (writeFileImpl(PageSize, false, false, &Stream, &HeaderOffset))
+    return -1;
----------------
I think we should try to avoid bloating the writer interfaces (which makes it really hard to read)

1) the header offset can be obtained using 'stat' call before the write
2) There is no need to reuse the stream -- this can get rid of two other parameters.

3) can probably also use a static variable to set Write alignment -- so that pagesize parameter can be removed.

================
Comment at: lib/profile/InstrProfilingFile.c:442
@@ +441,3 @@
+
+  FD = fileno(Stream);
+
----------------
Can you explain what are these file lockings all about?

Is it to support multi-process concurrent update? or multi-threads with cocurrent dlopen/dlclose?  Note that file synchronization is not supported with existing atexit file writer. For this reason, I think I suggest removing file locking code in this patch and we can discuss as a follow up.   (Note the to be submitted online profile merging support also needs file locking, so there should be some sync up there).

================
Comment at: lib/profile/InstrProfilingWriter.c:175
@@ +174,3 @@
+  uint64_t PaddingAfterCounters =
+      lprofAlignTo(CountersSizeInBytes, CounterSectionAlignment) -
+      CountersSizeInBytes;
----------------
Since tail paddings are explicitly written out here, why is there a need to emit padding bytes in Instrumentation ?

================
Comment at: lib/profile/InstrProfilingWriter.c:177
@@ +176,3 @@
+      CountersSizeInBytes;
+  uint64_t PaddingAfterNames =
+      lprofAlignTo(NamesSize + Padding, CounterSectionAlignment) -
----------------
This is to make sure the next header is properly aligned?


http://reviews.llvm.org/D19298