[Openmp-commits] [PATCH] D98838: [OpenMP] Fixed a crash in hidden helper thread

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3654-3656
       for (gtid = __kmp_hidden_helper_threads_num + 1;
            TCR_PTR(__kmp_threads[gtid]) != NULL; gtid++)
         ;
----------------
tianshilei1992 wrote:
> protze.joachim wrote:
> > This for-loop implicitly assumes, that it will find an empty space in `[0:__kmp_threads_capacity)`, i.e. implicitly assumes `gtid < __kmp_threads_capacity` , as stated in the assertion below - but you skip the first few index in the range. Line 3620 fails to provide this guarantee for your modified numbering scheme.
> > 
> Because we already take the number of hidden helper thread into account when setting `__kmp_threads_capacity`, this assumption holds, right?
No, as I showed with my reproducer, your patched code will never be reached in specific cases.
Also in the case, I annotated in your test-case: when the application asks for `N = __kmp_threads_capacity` threads, your patch is never reached, but the assertion still triggers (an prevents access beyond allocated memory!).


================
Comment at: openmp/runtime/test/tasking/hidden_helper_task/num_threads.cpp:12
+  const int __kmp_threads_capacity =
+      std::min(std::max(std::max(32, 4 * omp_get_num_threads()),
+                        4 * omp_get_num_procs()),
----------------
tianshilei1992 wrote:
> protze.joachim wrote:
> > `omp_get_num_threads()` will always return 1 in serial context.
> This logic is from code to set the capacity, specifically in the comment. It uses `$OMP_NUM_THREADS` originally so here I changed to `omp_get_num_threads()`.
Right, `omp_get_max_threads()` is the function, which gives you $OMP_NUM_THREADS. `omp_get_num_threads()` does something different.

Please don't rely on the names of the functions, but check the OpenMP spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98838