[Openmp-commits] [PATCH] D38185: Implementation of OMPT as specified in OpenMP 5.0 Preview 1

[Hansang Bae via Phabricator via Openmp-commits]

hbae added inline comments.


================
Comment at: runtime/src/ompt-general.cpp:124
+  ompt_start_tool_t next_tool = NULL;
+  *(void **)(&next_tool) = dlsym(RTLD_NEXT, "ompt_start_tool");
+  if (next_tool)
----------------
Hahnfeld wrote:
> Hahnfeld wrote:
> > `next_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool");` looks much more readable IMO
> ping
I am fine with the change.


================
Comment at: runtime/src/ompt-general.cpp:219
+  // Try tool-libraries-var ICV
+  const char *tool_libs = getenv("OMP_TOOL_LIBRARIES");
+  if (tool_libs) {
----------------
Hahnfeld wrote:
> Hasn't this been parsed to `__kmp_tool_libraries`?
`__kmp_tool_libraries` is defined when the runtime reads environment variables, but `ompt_pre_init()` is called before that happens, so I think `__kmp_tool_libraries` is just used for printing the values of OMP_TOOL_LIBRARIES.


================
Comment at: runtime/src/ompt-general.cpp:228
+      if (h) {
+        *(void **)(&start_tool) = dlsym(h, "ompt_start_tool");
+#elif KMP_OS_WINDOWS
----------------
Hahnfeld wrote:
> `start_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool");` as above
Looks good to me as above.


https://reviews.llvm.org/D38185