This is the mail archive of the ecos-discuss@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Some bugs in ECOS kernel


Кондратенко Антон Александров wrote:
Hello, all:

I think, i found a few bugs in ECOS kernel.

1.
Please take look at folowing source code:

"packages\kernel\current\src\common\kapi.cxx"
externC cyg_bool_t cyg_thread_delete( cyg_handle_t thread )
{
    Cyg_Thread *th = (Cyg_Thread *)thread;
    if( th->get_state() != Cyg_Thread::EXITED )
        th->kill(); // encourage it to terminate
    if( th->get_state() != Cyg_Thread::EXITED )
        return false; // it didn't run yet, leave it up to the app to fix
    th->~Cyg_Thread();
    return true;
}

At first in ECOS Docs, this function described as returning void.
And second is it:
when we trying to delete not EXITED thread, 90% that th->kill() will not switch thread to EXITED state immediately (whereas if thread in SLEEPING state, kill() only wake up thread and exit ), and thread will not deleted.
So, if we wrote code in accordance with ECOS docs, we'll have a troubles if after deleting thread, we freeing thread resources.

You mean there's an omission in the documentation, not a bug in the kernel :-). So I've corrected the documentation, thanks.


2.
see "packages\kernel\current\src\common\thread.cxx"
Function Cyg_Thread::kill(), didn't check a state of thread before check "sleep_reason". So if thread is running but "sleep_reason" not NONE (whereas after last sleeping sleep_reason not resetting to NONE), kill() will not really kill a running thread. This is a big problem if we want to delete thread in function of thread destructor ( via new thread created in destructor). See also bug 1.

The thread will be put into the EXIT state and be woken up (although in your case it already is, but the point is that its state will then be cleared of SLEEPING) and if you check the return value of cyg_thread_delete you'll see the thread delete failed - if the new thread is lower priority than the thread to be killed then the thread to be killed will complete the exit() function and end up in the EXITED state. If the new thread is higher priority than the thread to be killed then the thread to be killed wouldn't be able to run to call exit() anyway so the thread delete would fail anyway.



3. see "packages\kernel\current\src\common\thread.cxx"

Cyg_Thread::kill().

Try to kill thread that in RUNNING state, will not invoke thread destructors whereas destructors calling in Cyg_Thread::exit(), but kill() invoke exit() only if thread was in SLEEPING state ( via "wake_reason=EXIT")

Hmm... I'm not decided about this one. It's true the behaviour isn't very symmetric, but loosely part of the point is that a kill is very much *forcing* a thread to exit. As the comment about Cyg_Thread::kill says:


// Kill thread. Force the thread into EXITED state externally, or
// make it wake up and call exit().

And the documentation says:

"However killing a thread
is generally rather dangerous because no attempt is made to unlock any
synchronization primitives currently owned by that thread or release
any other resources that thread may have claimed. Therefore use of
this function should be avoided, and cyg_thread_exit is preferred."

I would consider thread destructors to be part and parcel of that. At the same time the assymetry irks me as the caller arguably won't know if the thread is sleeping or not and therefore whether the thread destructors get called or not is non-deterministic. I've CC'd Nick for his opinion on this, but I don't see any easy solution that would make the thread destructors be called in the context of the killed thread. Short of setting the PC to &Cyg_Thread::exit anyway! Therefore I'm tempted to go along with the documentation saying that resources may not be released and this function is dangerous and to be avoided.

If you would care to propose a simple deterministic solution then I'm sure we'd consider it.

Please fix me if I wrong.

I don't think I've ever patched a human before ;-).


And fix ECOS otherwise.

I've fixed the doc issue anyway.


Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
>>>>> Visit us in booth 2527 at the Embedded Systems Conference 2004 <<<<<
March 30 - April 1, San Francisco http://www.esconline.com/electronicaUSA/
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine


-- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]