Initialising a thread_local object with ID of thread

[Frederick Gotham]

Before I begin, I have deliberately left out necessary code in order to make this as simplistic as possible (for example I've neglected to join all std::thread objects before destroying them).

I have the following global constant, along with an atomic counter variable:

unsigned constexpr g_N_threads = 6u;

std::atomic<unsigned> g_counter{0};

And then I have the following global array to store the thread ID's:

std::array< std::atomic<std::thread::id>, g_N_threads > g_ids_for_threads{};

In my main thread, I have this code in 'main':

int main(void)
{
array<std::thread,g_N_threads> threads; /* Create std::thread objects without starting threads yet */

for ( unsigned i = 0; i != g_N_threads; ++i )
{
threads[i] = std::thread(Thread_Entry_Point);
}

//Give threads a chance to start
std::this_thread::sleep_for(std::chrono::seconds(2));

//Right now, g_counter is still 0
for ( unsigned i = 0; i != g_N_threads; ++i )
{
assert( std::thread::id{} != threads[i].get_id() );

g_ids_for_threads[i] = threads[i].get_id();
}

++g_counter;
}

And then for each of the 6 threads, I first have this helper function:

unsigned Get_Thread_Index_From_StdID(std::thread::id const arg_id)
{
assert( std::thread::id{} != arg_id );

for (unsigned i = 0; i != g_N_threads; ++i)
{
if ( arg_id == g_ids_for_threads[i] )
return i;
}

assert( nullptr == "Control should never reach here!" );
}

And here's the entry point to the thread:

void Thread_Entry_Point(void)
{
unsigned counter = 0;

while ( counter >= g_counter ); //spin until g_counter increments

static thread_local SomeType some_object{
Get_Thread_Index_From_StdID( std::this_thread::get_id() )
};

for (; /* ever */ ;)
{
/* Do Something */
}
}

When I build and run this, the error I'm getting is as follows:

gotham: gotham.cpp:147: unsigned int Get_Thread_Index_From_StdID(std::thread::id): Assertion `nullptr == "Control should never reach here!"' failed.

This means that there's a problem with how I'm retrieving and storing the thread ID's.

Can anyone see what's wrong here?

[Paavo Helde]

12.08.2020 13:09 Frederick Gotham kirjutas:
> Before I begin, I have deliberately left out necessary code in order to make this as simplistic as possible (for example I've neglected to join all std::thread objects before destroying
them).

[random uncompilable code snippets clipped]

> When I build and run this, the error I'm getting is as follows:
>
> gotham: gotham.cpp:147: unsigned int Get_Thread_Index_From_StdID(std::thread::id): Assertion `nullptr == "Control should never reach here!"' failed.
>
> This means that there's a problem with how I'm retrieving and storing the thread ID's.
>
> Can anyone see what's wrong here?
>

There are lots of things wrong with your code (hint: random sleeping and
spin-waiting are pretty bad ideas), but the most obvious is that you
have not provided a compilable and runnable example which would
demonstrate the problem.

I copy-pasted the pieces in some order which compiled, fixed some syntax
errors, added needed includes and missing thread joining. The final
program worked without problems with two different compilers. I suspect
the problem is in your code which you have not shown or have altered
while posting.

[Ben Bacarisse]

It would be helpful if you posted something we could build and run.
When I make minimal additions to make it a whole program, I don't get
the behaviour that you see:

> gotham: gotham.cpp:147: unsigned int Get_Thread_Index_From_StdID(std::thread::id): Assertion `nullptr == "Control should never reach here!"' failed.
>
> This means that there's a problem with how I'm retrieving and storing
> the thread ID's.
>
> Can anyone see what's wrong here?

I haven't looked at the program because it's often hard (and sometimes
pointless) to debug code when you can't re-recreate the problem.

--
Ben.

[Frederick Gotham]

On Wednesday, August 12, 2020 at 11:54:54 AM UTC+1, Paavo Helde wrote:

> I copy-pasted the pieces in some order which compiled, fixed some syntax
> errors, added needed includes and missing thread joining. The final
> program worked without problems with two different compilers. I suspect
> the problem is in your code which you have not shown or have altered
> while posting.

I've put it all together as compilable code:

#include <cassert>
#include <array>
#include <atomic>
#include <thread>
#include <iostream>

using namespace std;

unsigned constexpr g_N_threads = 6u;

std::atomic<unsigned> g_counter{0};

std::array< std::atomic<std::thread::id>, g_N_threads > g_ids_for_threads{};

unsigned Get_Thread_Index_From_StdID(std::thread::id const arg_id)
{
assert( std::thread::id{} != arg_id );

for (unsigned i = 0; i != g_N_threads; ++i)
{
if ( arg_id == g_ids_for_threads[i] )
return i;
}

assert( nullptr == "Control should never reach here!" );
}

void Thread_Entry_Point(void)
{
struct SomeType { unsigned i; SomeType(unsigned arg) : i(arg) {} };

unsigned counter = 0;

while ( counter >= g_counter ); //spin until g_counter increments

static thread_local SomeType some_object{
Get_Thread_Index_From_StdID( std::this_thread::get_id() )
};

for (; /* ever */ ;)
{
/* Do Something */
}
}

int main(void)
{
array<std::thread,g_N_threads> threads; /* Create std::thread objects without starting threads yet */

for ( unsigned i = 0; i != g_N_threads; ++i )
{
threads[i] = std::thread(Thread_Entry_Point);
}

//Give threads a chance to start
std::this_thread::sleep_for(std::chrono::seconds(2));

//Right now, g_counter is still 0
for ( unsigned i = 0; i != g_N_threads; ++i )
{
assert( std::thread::id{} != threads[i].get_id() );

g_ids_for_threads[i] = threads[i].get_id();
}

++g_counter;

cout << "Last line of main\n"; // expect error about unjoined threads after this
}


And unfortunately it works fine.

I therefore don't know where the problem is in my code, I'll have to look more closely.

[Frederick Gotham]

On Wednesday, August 12, 2020 at 12:33:23 PM UTC+1, Frederick Gotham wrote:

> I therefore don't know where the problem is in my code, I'll have to look more closely.

I found out the problem by doing this:

std::thread::id g_main_thread_id;

int main() {
g_main_thread_id = std::this_thread::get_id();
/* */
}

It turns out that the main thread was calling a function which I didn't think it was calling (so the ID of the main thread didn't exist in the array of thread ID's).

[James Kuyper]

On 8/12/20 6:54 AM, Paavo Helde wrote:
> 12.08.2020 13:09 Frederick Gotham kirjutas:
>> Before I begin, I have deliberately left out necessary code in order to make this as simplistic as possible (for example I've neglected to join all std::thread objects before destroying
> them).

...

> I copy-pasted the pieces in some order which compiled, fixed some syntax
> errors, added needed includes and missing thread joining. The final
> program worked without problems with two different compilers. I suspect
> the problem is in your code which you have not shown or have altered
> while posting.

It seems paradoxical, but empirically, people who post only the part of
their code that they think is relevant to the problem are very likely to
have removed the part that caused the problem.

If you can't post the entire program where you ran into your problem,
either because it's too big and complicated, or proprietary or
classified, what you should do is put together simplified version (with
any proprietary or classified features removed) that is complete and
compilable.
Most importantly, before posting your simplified version - confirm that
it still demonstrates the problem.

Sometimes it seems impossible to put together such a simplified version,
but there's a straightforward approach to systematically simplifying a
program that demonstrates a problem into a much simpler program that
still demonstrates it:

You'll need to have two copies of your code, WORK, which you do your
work on, and FAIL, which has been confirmed to fail. Initialize both of
them by copying the original program.

1. Identify a portion of WORK that should be removable without
interfering with the failure, preferably as large as possible. A good
starting candidate is all code that should have executed after the code
which produced the first symptom that something had gone wrong, but
toward the end of the process you might be stuck with removing only a
single line of code. If you can't find anything more to remove, you're
done - you should post the entire code of FAIL along with your question.
2. Remove that portion from WORK.
3. MOST IMPORTANT: check that WORK still demonstrates the problem. If it
does, replace FAIL with a copy of WORK. If not, think very carefully
about the part you just removed. You thought that it wouldn't remove the
problem, but it did - that's a very important clue. Replace WORK with a
copy of FAIL, and go back to step 1 - but make sure to remove a
different part of the program the next time, generally a smaller one.

It's quite common for people following this procedure to end up finding
the problem by themselves, because of the clues they accumulate from
changes that fixed the problem.

Once you're ready to post your simplified example, there's a few
additional facts you need to provide with your question:

1. Identify the precise tool chain you used to create the code,
including which options you chose. It's often the case that people will
be unable to duplicate your problem if they use a different tool chain
or make a different choices for one or more of the options.

2. Identify which result you expected to get. That might seem obvious to
you, but I've frequently seen people post code that contained no obvious
defect, and which produced exactly the results that I would expect it to
produce - the problem was that the person who wrote it was expecting
different results.

3. Identify the actual results you got. If at all possible, they should
be cut-and-pasted from the actual output. You don't want us to waste
time trying to figure out a typo that was introduced by you when you
retyped the output.

Warning: this process is not guaranteed to work. The worst problem I
ever used this approach on, I was only able to remove 52% of the code,
everything else I tried to remove made the problem disappear (and it was
unacceptable to deliver the code with that part removed). I was unable
to remove dependencies on three different third-party libraries. I sent
the simplified version to the help desk for the library that seemed most
relevant, and worked together with them for three months trying to track
down the problem - without success. The program still periodically fails
due to that problem.

[Öö Tiib]

Yes, usually by making code that really compiles and runs (and
demonstrates the problem that they think there is) people find
out what they really did wrongly.

But when they post some kind of pseudo-code without ever
trying it then it is as rule about something that does not
demonstrate the issue.

There was very similar post only recently:
<https://groups.google.com/d/msg/comp.lang.c++/Y1J8W4q4Vo8/VLUBXiR3BAAJ>

[Chris M. Thomasson]

Strange!

>
> static thread_local SomeType some_object{
> Get_Thread_Index_From_StdID( std::this_thread::get_id() )
> };
>
> for (; /* ever */ ;)
> {
> /* Do Something */
> }
> }
>
> int main(void)
> {
> array<std::thread,g_N_threads> threads; /* Create std::thread objects without starting threads yet */
>
> for ( unsigned i = 0; i != g_N_threads; ++i )
> {
> threads[i] = std::thread(Thread_Entry_Point);
> }
>
> //Give threads a chance to start
> std::this_thread::sleep_for(std::chrono::seconds(2));

This is very strange to me.

>
> //Right now, g_counter is still 0
> for ( unsigned i = 0; i != g_N_threads; ++i )
> {
> assert( std::thread::id{} != threads[i].get_id() );
>
> g_ids_for_threads[i] = threads[i].get_id();
> }
>
> ++g_counter;
>
> cout << "Last line of main\n"; // expect error about unjoined threads after this

You need to join the threads.



Also, why not pass in a thread id that you created before you even
create a thread?


// pseudo code
void thread_entry(unsigned long id)
{
//[...]
}


Why are you trying to extract their ids after they are created?

[Juha Nieminen]

Frederick Gotham <cauldwel...@gmail.com> wrote:
> unsigned counter = 0;
>
> while ( counter >= g_counter ); //spin until g_counter increments

Spin loops are generally a really bad idea (unless you are, like, targeting
some small 8-bit embedded CPU that doesn't really care, because it runs
at 100% load all the time anyway. But then, you probably aren't running
many threads in such a CPU anyway, so the only conclusion is that spin loops
are bad.)

Also, I don't understand the rationale in that 'counter' variable at all.
Nothing modifies it. What is its purpose?

> //Give threads a chance to start
> std::this_thread::sleep_for(std::chrono::seconds(2));

How do you know it takes 2 seconds for the threads to "start"?
Why 2 seconds and not some other completely arbitrary time?

[Juha Nieminen]

Chris M. Thomasson <chris.m.t...@gmail.com> wrote:
> You need to join the threads.

Or alternatively detach them.

(There may be situations where you genuinely don't join threads.
Doing a create-and-forget thread like "std::thread(func).detach();"
can be useful sometimes.)

[Chris M. Thomasson]

Detached threads can be sketchy, however they can be handled for sure.

[Chris M. Thomasson]

On 8/12/2020 10:50 PM, Juha Nieminen wrote:
> Frederick Gotham <cauldwel...@gmail.com> wrote:
>> unsigned counter = 0;
>>
>> while ( counter >= g_counter ); //spin until g_counter increments
>
> Spin loops are generally a really bad idea

Think of a special spin loop... Where it tries to do something else
instead of spinning. In other words, if it can do some other real work,
then that can be used as a backoff, in a sense.

[Chris M. Thomasson]

On 8/13/2020 12:07 AM, Chris M. Thomasson wrote:
> On 8/12/2020 10:50 PM, Juha Nieminen wrote:
>> Frederick Gotham <cauldwel...@gmail.com> wrote:
>>>          unsigned counter = 0;
>>>
>>>          while ( counter >= g_counter );  //spin until g_counter
>>> increments
>>
>> Spin loops are generally a really bad idea
>
> Think of a special spin loop... Where it tries to do something else
> instead of spinning. In other words, if it can do some other real work,
> then that can be used as a backoff, in a sense.

pseudo code... Perhaps something along the lines of:
________________________
void foo_lock(...)
{
while (! try_lock(...)) // a check...
{
// failed to grab the lock...
if (try_to_do_something_else()) continue;
lock(); //blocking
return;
}
}
________________________