[PATCH] D142317: [Support][LLD] Avoid using uninitialized threadIndex.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D142317#4074043 <https://reviews.llvm.org/D142317#4074043>, @andrewng wrote:

>> I did not find direct specification of that behavior(`thread_local` variables are zero initialized) thus created a patch which initializes it.
>
> Did you see actual issues arising from `threadIndex` being uninitialized? If so, what platform and toolchain were you using?

No, I did not. The problem which I see is data race while accessing the same index from different threads.

I did not see that C++ standard says that thread local data is zero initialized - that is why i decided to initialize it. If we think this is not necessary - we can skip initialization part.

>> Another thing, as mentioned in above quote, is that main thread with zero index clashes with first thread created by  `parallelFor`.
>
> See my suggestion.





================
Comment at: llvm/lib/Support/Parallel.cpp:231-232
     }
     for (; Begin != End; ++Begin)
       Fn(Begin);
     return;
----------------
andrewng wrote:
> andrewng wrote:
> > Perhaps a "simpler" fix would be not to use the main thread here, i.e. should use `TG.spawn`? However, that assumes that `getThreadIndex()` is only for use in the "`parallelFor`" style functions otherwise the other parallel functions would also need to avoid using the main thread too.
> > 
> > I'm surprised that this hasn't been causing more issues.
> My gut feeling is that this would be the "better" solution, i.e. not including the invoking thread (might not be the main thread) as part of the pool of worker threads. It just feels cleaner to me.
> 
> However, can we also assume that the use of `getThreadIndex()` is not supported with regards to `parallel_sort` which I believe can also make use of the invoking thread? I think this would be reasonable.
yes, we can avoid using main thread in this case.

Though, it seems, having the same index for main thread and for one from ThreadPool threads is a potential bug. We would always need to remember to avoid using main thread and ThreadPool threads. Instead, it looks better, to always have different indexes. As this patch does : 0 for main thread, 1... for others.

How about using TG.spawn in this place, but still having different indexes for main thread and others?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142317