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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 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++)
         ;
----------------
protze.joachim wrote:
> 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!).
I got your point. That is a nice catch! Thanks. I'll come up with a good way to fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98838