[PATCH] D153771: [BOLT][Instrumentation] Fix hash table memory corruption and append-pid option

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks for working on fixing these issues, @treapster! I have some suggestions below.



================
Comment at: bolt/runtime/instr.cpp:254
   uint64_t Val;
+  void report(const char *Msg = nullptr) {
+    char Buf[BufSize];
----------------
report -> dump

see Graph::dump() for an easier way to write (and read) this code

also include a call to this function inside a DEBUG() macro to showcase how/where you want this printed. If you don't want to always print it in debug, leave it commented out, so at least we know how to use it when we need it.


================
Comment at: bolt/test/runtime/instrumentation-indirect-2.c:90-91
+
+RUN: child_pid=$(cat %t.output | grep func1: | awk '{print $2;}')
+RUN: par_pid=$(cat %t.output | grep func2: | awk '{print $2;}')
+
----------------
Here I would rather have a command that moves the child/parent fdata to fixed file names such as:

mv $t.$child_pid.fdata child.fdata
mv $t.$par_pid.fdata parent.fdata

The reason is because if we don't do that, each time a developer runs "ninja check-bolt", it will create a new unique file (with a different PID attached to the file name) in the Output folder and won't replace the previous one (the expected behavior), unnecessarily using more disk space over time.


================
Comment at: bolt/test/runtime/instrumentation-indirect-2.c:96-97
+# process and vice versa.
+RUN: cat %t.$child_pid.fdata | FileCheck %s --check-prefix=CHECK-CHILD
+RUN: cat %t.$par_pid.fdata | FileCheck %s --check-prefix=CHECK-PARENT
+
----------------
This can accidentally match other entries in the profile, such as the activity in funcX calling printf(). I imagine what we want to match is the indirect call itself, right? For that I would rather use:


  RUN: llvm-bolt %t.exe -data %t.child.fdata \
  RUN:   -print-finalized -print-only=main -o /dev/null | FileCheck %s --check-prefix=CHECK_CHILD

And then match this string:
    {{.*}}:   callq   *%rax # CallProfile: 8 (0 misses) :
        { func1: 1 (0 misses) },
        { func3: 1 (0 misses) },
...


Actually, I ran BOLT like that and surprisingly the profile is incorrect and it is printing this:
.LFT15 (8 instructions, align : 1)
  Exec Count : 8
  CFI State : 3
  Predecessors: .Ltmp10
    000000d1:   movslq  -0x94(%rbp), %rax
    000000d8:   movq    -0x90(%rbp,%rax,8), %rax
    000000e0:   movl    -0x98(%rbp), %edi
    000000e6:   callq   *%rax # CallProfile: 8 (0 misses) :
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) },
        { <unknown>: 1 (0 misses) }
    000000e8:   movl    -0x94(%rbp), %eax
    000000ee:   addl    $0x2, %eax
    000000f1:   movl    %eax, -0x94(%rbp)
    000000f7:   jmp     .Ltmp10
  Successors: .Ltmp10 (mispreds: 0, count: 8)



I'm not sure why yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153771