some addtions

[tvb377]

Hi Freddie, here are some additions. Time was short, so only minor stuff. I changed the owner of ThreadControlBlock to ThreadCommon. I find it better to pass internal interfaces instead of user interfaces within distortos.

I tried to minimize extra memory consumption for exit(). I think now it should be an additional entry in the vtable of ThreadCommon. Unfortunately, gcc cannot believe in virtual noreturn functions. This makes it a bit ugly here and there.

Regards, Titus

[FreddieChopin]

I changed the owner of ThreadControlBlock to ThreadCommon. I find it better to pass internal interfaces instead of user interfaces within distortos.

Does this solve any particular problem which existed with previous version of the code?

In case of THREAD_DETACH_ENABLE ThisThread::get should return reference to outer DynamicThread

What's the purpose of this change? I think the previous behaviour is correct, there's no point in returning the reference to the "outer" thread, as you can do everything with the "inner" thread...

If there is an use case where the "outer" thread is useful, then let me know.

However letting "inner" thread access "outer" thread is dangerous, as "outer" thread can be deleted at any moment after it gets detached.

[tvb377]

The goal was to have ThisThread::exit(). The rationale is to minimize additional overhead for this type of exit(). I think this can best be done with a virtual member function. However, there must be no Thread::exit() for obvious reasons. So, exit() is part of ThreadCommon. And, as said, it does not make much sense to pass around user interfaces within distortos, anyway. Within distortos, there simply is no Thread that isn't a ThreadCommon. If there would be, ThreadCommon would be gratuitous.

The current problem with ThisThread::get is that the Thread object is not the one having been created. The currently running thread cannot find out which (user created) thread object it is represented by. That is, the child object has a different identity than what its parent thinks it has. However. preserving identity must be an implicit guarantee of ThisThread::get. It simply doesn't make too much sense otherwise since all of what a Thread could do via ThisThread::get().xxx() can already be done in ThisThread directly. In other words, currently get() is an exposed implementation detail of ThisThread that does not do what one could rightfully expect. Yes, the patch helps to create a race condition. The remedy would be to use the previous get() implementation in ThisThread's other functions, and document that the exposed get() must not be used when the Thread should be detached (which is implicitly meaningless, anyway: see above).

[FreddieChopin]

The current problem with ThisThread::get is that the Thread object is not the one having been created. The currently running thread cannot find out which (user created) thread object it is represented by.

Why don't you just pass this info to thread's function (as a simple argument)? Why would it need such info?

I just don't see a problem here - with detachment enabled, the "identity" of thread is not necessarily the same as the constructed object, but this is just how it is... If there is an inherent problem here, we can solve it. But the situation you describe seems to be caused by the application code and seems to be entirely solvable there. If your thread needs to know what is it's "outer" object, then it's really tempting to say that it's the thread's problem, not one of the entire RTOS.

Please describe the problem you are trying to solve in more detail so we can work something out.

Is this particular change (patch nr 3) also related to the "exit" functionality from patch nr 2?

[ThisThread::get()] simply doesn't make too much sense otherwise...

It does. How else would you pass a reference to "this thread" to another thread (or a timer). There could be multiple reasons for that - for example if "another thread" will do something and notify "this thread" about that with a signal. Or maybe "another thread" wishes to control "this thread" priority.

Currently ThisThread::get() will return a reference to the "real thread", which is valid as long as the thread is running. This behaviour does not depend on whether detachment is enabled or not. The result of this function - as seen by the running thread - is always identical and doesn't ever change. With your proposed change the returned value may be invalidated at any moment in time (even when ThisThread::get() did not fully return yet)! The value returned may be different between two calls. With all that in mind using this function in a dynamic thread seems like asking for trouble. So I would say that with this change this function should never be used, which is completely opposite to what you say.

From my perspective this change brings serious problems while solving no issues (which I'm aware of), so please try to understand my concerns here. If there is a real problem here, we will solve it - just not this way. But first I have to understand the real issue you are facing.

Maybe a proper solution would be to implement unique thread IDs, which could then be mapped internally by the scheduler to the real objects? The ID would be identical in both "inner" and "outer" thread, and would never change for the running thread.

[tvb377]

More general: in an OO design, every object has its identity. In a sensible C++ design, this should be the address of the object. Introducing an artificial additional id is a kludge. Currently, distortos’s thread identity is broken by ThisThread::get. This is like pthread_self() returning something else than pthread_create. Having to pass a thread identity from the parent to the child by means of the thread parameters would be an Evil Workaround, not a cure.

Currently, I’m unable to think of a valid example why, how and to whom else one should pass around the results of ThisThread::get(). I would not use get() in the scenarios depicted by your previous mail. Anyway, even if these examples really justify using ThisThread::get(), all you’d need is proper documentation, like so: \warning Do not use ThisThread::get() from a Thread to be detached until it actually has been detached. I don’t see a problem with that.

BTW: AFAICS, the current design is also broken in this area since you declared DynamicThread’s move constructor default. This leaves DynamicThreadBase::owner_ dangling, and a call to detach() on a moved DynamicThread results in undefined behaviour.

Regards, Titus

[FreddieChopin]

More general: in an OO design, every object has its identity. In a sensible C++ design, this should be the address of the object. Introducing an artificial additional id is a kludge. Currently, distortos’s thread identity is broken by ThisThread::get.

But what rule says that a dynamic thread must be composed of a single object which has single identity? In distortos such thread is composed of two objects, outer being used only as a (disposable) wrapper from which the inner one can be detached. I really see no problem that they have two different identities...

Whether or not "id" is a kludge is debatable, however it solves a problem which is not solveable by other means (without MMU). "id" will be required for pthread implementation anyway (returning an address is too dangerous), so please reconsider whether it solves your original issue.

Another idea - add "get()" (or some other name) to Thread - for all typical instances it would return this, while for DynamicThread it would return the address of the inner object or nullptr.

This is like pthread_self() returning something else than pthread_create.

However do note that these functions return an ID, not an address. That's why they always return the same thing.

Also note that pthread_self() returns the same thing for the whole lifetime of the thread - just like current behaviour of ThisThread::get().

Having to pass a thread identity from the parent to the child by means of the thread parameters would be an Evil Workaround, not a cure.

I prefer an "Evil Workaround" which is guaranteed to work than a "cure" which is broken in several edge cases which cannot be solved/controlled by any means.

all you’d need is proper documentation, like so: \warning Do not use ThisThread::get() from a Thread to be detached until it actually has been detached. I don’t see a problem with that.

The thread has no way of knowing that it was detached "externally".

We can argue about which way is more broken or more correct or more expected, but I still don't know the scenario that lead you to this particular change... If there is not such a scenario, I would say that the current behaviour should stay and the docs should be improved saying that the address of the DynamicThread is not necessarily the same as the address returned by ThisThread::get().

I will certainly not say that "this is not a bug - it's a feature", but I will say that "this is not a bug". This is a limitation of the embedded environment which has no MMU. I see no way in which this can be solved when you really do want to use the addresses of the objects and don't want your program to be vulnerable to hundreds of nasty bugs caused by using object after it was already destroyed.

BTW: AFAICS, the current design is also broken in this area since you declared DynamicThread’s move constructor default. This leaves DynamicThreadBase::owner_ dangling, and a call to detach() on a moved DynamicThread results in undefined behaviour.

Now this probably is a real problem which should be really fixed (; However explicitly moving threads is also another thing which you should not be doing (; But then move constructors are necessary for functions like makeStaticThread to allow return value optimization (https://en.wikipedia.org/wiki/Return_value_optimization). I'll investigate that, but it's also possible that with the big API change of threads which I'm planning, the moves won't be required anymore.

[tvb377]

Again, there is no valid thread id in distortos. This is a deficiency. Since you don’t have an explicit Id (which I welcome), the implicit assumption is that get() returns the right address, which it doesn’t. And since you don’t have an id, this compares to the pthread_self() example I gave.

It does not have anything to do with an MMU. Also, I don’t see a problem that a DynamicThread is a pimpl which leads to an ID change after detach. I’d assume that anyone using a DynamicThread can work with that. Calling detach is like saying farewell. I see it as side effect of the artificial id that pthread allows you to keep a valid pthread_t after detach.

The thread itself or the ThisThread implementation does not need to know it has been detached externally. I don’t understand what you mean here.

All I say is that this should be clearly documented which it should be, regardless of what’s going to be implemented.

The scenario where you’d want to have a correct id is finding/managing/deleting dynamically allocated thread objects from either the thread itself or another thread. The first case (find/manage/delete the objects from the thread itself) always fails. (And, in addition, currently it fails if you have moved these objects).

If you don’t see a valid scenario here, and do not want to change get(), we could implement a Thread::compare() function. In DynamicThread this would compare it to the pimpl object.

Let me know if that’s OK for you.

[tvb377]

Moving around DynamicThreads is a perfectly valid scenario. If you don't want that, specify the move constructor as deleted. The makeDynamicThread functions are unneeded extra candy, anyway.

Curing the pimpl problem with a move constructor should be fairly trivial. You simply change the owner of the inner object. If you want to pamper the user you can introduce a lock with detach. However, moving around these objects when you plan to detach them is NOT a valid scenario. Simply remove the gratuitous make functions and you're done here. Also here I'd prefer decent documentation about UB lurking ahead over pampering the user. Maybe with some two examples of the canonical ways of detached and undetached DynamicThread setup.

[FreddieChopin]

Again, there is no valid thread id in distortos. This is a deficiency. Since you don’t have an explicit Id (which I welcome)

So adding unique ID would solve the issue you were facing?

I would expect this to be implemented as ThisThread::getIdentifier() and Thread::getIdentifier(), which would return an unique 32-bit number. Both "inner" and "outer" thread of DynamicThread would return the same value. After the "inner" thread is detached, "outer" thread doesn't return anything (or some sentinel value marked as "invalid identifier").

If you don’t see a valid scenario here, and do not want to change get(), we could implement a Thread::compare() function. In DynamicThread this would compare it to the pimpl object.

Unique identifier is a much better solution, as it maps to what is found in POSIX, while Thread::compare() does not.

The scenario where you’d want to have a correct id is finding/managing/deleting dynamically allocated thread objects from either the thread itself or another thread. The first case (find/manage/delete the objects from the thread itself) always fails. (And, in addition, currently it fails if you have moved these objects).

In my opinion this is not very optimal design, as there's absolutely no point in "finding" which thread is which. In the place where you construct the thread you already know exactly which object from the pool you used. Pass that info to the thread if it really needs to know it and be done with that. Thus the problem you describe is easily solvable with zero changes in the kernel.

Just let me know whether unique thread identifier suits your use case, as this is something I was planning to implement anyway.

[FreddieChopin]

Hello Titus!

I have implemented distortos::ThisThread::exit() - the functionality works and is tested. I'll now try to implement unique thread identifiers.

Please let me know what other features you are missing for your project (possibly with their "priority").

[FreddieChopin]

If you are still interested, I have pushed some commits which implement distortos::ThreadIdentifier type and two new functions - distortos::Thread::getIdentifier() and distortos::ThisThread::getIdentifier(). These can be reliably used for the purpose of comparing the thread objects which you mentioned in one of your previous messages.

This change took so long, because I was on (long deserved) holidays and I did not manage to finish it before leaving, but it's ready now (;