[PATCH] D70330: [profile] Fix file contention causing dropped counts on Windows under -fprofile-generate

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Thanks! I think instrprof-multiwrite.test is a great test for this.

I'd suggest dropping instrprof-open-file.c and only keep the other one, which is simpler and more targeted towards testing the profile info rather than the function used internally to implement it.



================
Comment at: compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c:10
+
+void spawn_child(PROCESS_INFORMATION *pi, int child_num) {
+  wchar_t child_str[10];
----------------
Since the child_num doesn't really matter for the child process, I'd suggest maybe dropping that to simplify the code.

I suppose an environment variable is still needed to differentiate between child and parent processes, but it could just be set to a constant string.


================
Comment at: compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c:57
+int foo(int num) {
+  if (num < 5) {
+    return 1;
----------------
I know dxf suggested adding some control-flow here, but I don't see why. Isn't just the function entry counts enough to verify that we've merged the profile info successfully? I'd suggest keeping it simple.


================
Comment at: compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c:65
+
+extern FILE *lprofOpenFileEx(const char *);
+int main(int argc, char *argv[]) {
----------------
This declaration is not needed.


================
Comment at: compiler-rt/test/profile/Windows/Inputs/instrprof-multiwrite.c:69
+  if (!child_str) {
+    PROCESS_INFORMATION child[10];
+    // In parent
----------------
Nit: I'd probably use a #define for the number of children.


================
Comment at: compiler-rt/test/profile/Windows/instrprof-multiwrite.test:2
+RUN: mkdir -p %t.d
+RUN: %clang_profgen %S/Inputs/instrprof-multiwrite.c -o %t
+RUN: rm -f %t_*.profraw 
----------------
Maybe "instrprof-multiprocess.c" would be a better name?


================
Comment at: compiler-rt/test/profile/Windows/instrprof-multiwrite.test:6
+RUN: %run %t
+RUN: llvm-profdata show --counts -function=foo %t_*.profraw | FileCheck %s
+
----------------
Thanks! I think this is a great test.

I'd personally prefer stripping out the expectations below that aren't essential to the test, and only keep the check that foo is entered 10 times, i.e. something like

```
CHECK: Counters:
CHECK:   foo:
CHECK:     Function count: 10
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70330/new/

https://reviews.llvm.org/D70330