[PATCH] D43278: Add Xray instrumentation support to FreeBSD

[Kamil Rytarowski via Phabricator via llvm-commits]

krytarowski added inline comments.


================
Comment at: lib/xray/tests/CMakeLists.txt:28
           -lstdc++ -lm ${CMAKE_THREAD_LIBS_INIT}
-          -lpthread
-          -ldl -lrt)
+          ${CMAKE_DL_LIBS} -lrt)
       set_target_properties(XRayUnitTests PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
----------------
I think this is a broken syntax with ${CMAKE_DL_LIBS}. It evaluates to "" on BSDs but on Linux it will evaluate to "dl" on Haiku to "root be" etc.

Please in newer commits use diff: `-U9999` to help to reviewers to get wider context.

```
set(CMAKE_DL_LIBS_INIT "")
foreach(lib ${CMAKE_DL_LIBS})
    list(APPEND CMAKE_DL_LIBS_INIT -l${lib})
endforeach()
```

And reuse `${CMAKE_DL_LIBS_INIT}` in the place of the current `${CMAKE_DL_LIBS}`.


================
Comment at: lib/xray/tests/CMakeLists.txt:28
           -lstdc++ -lm ${CMAKE_THREAD_LIBS_INIT}
           -lpthread
+          ${CMAKE_DL_LIBS} -lrt)
----------------
krytarowski wrote:
> while there - `${CMAKE_THREAD_LIBS_INIT}` is equivalent to `-lpthread`
Is there need to add `find_package(Threads)` in this file?


================
Comment at: lib/xray/xray_x86_64.cc:286
+#else
+  __asm__ __volatile__("cpuid" : "=a"(EAX), "=b"(EBX), "=c"(ECX), "=d"(EDX)
+    : "0"(0x80000001));
----------------
devnexen wrote:
> krytarowski wrote:
> > This should be used for all OSes. `__get_cpuid` is GLIBC specific.
> I m not too keen for this change (not changing even in the slighest the Linux behavior) but if everyone is backing you up on this I ll change.
I think these two `asm` parts are important to get done in order to not desync platforms for no good reason. Also nobody likes `ifdefs`.

Alternatively we can add a fallback implementation of `__get_cpuid` and `__rdtscp` for !Linux.


https://reviews.llvm.org/D43278