Fix for issue #4547

[alexcrichton]

It looks like the else if clause isn't really necessary when tearing down the task because having TLS shouldn't mean that you don't necessarily fail the sched loop.

That being said, I'm definitely as familiar with this code as I'm sure someone else is, so this may be breaking something that I'm not seeing. At the very least the test case fails before the patch and succeeds after the patch, so I could re-open pull request with it as an xfail test if the change isn't the right one.

[brson]

It looks to me like this line in core::task::spawn is the one that is intended to be handling this case (when TLS has been initialized in the main task): https://github.com/mozilla/rust/blob/e51ec26dd7d915cfdfabcf5b6e32c001e30bd244/src/libcore/task/spawn.rs#L419

The problem though is that that case is triggered from the TCB destructor, which is called when TLS is destroyed, and the TCB for the main task is not initialized at the same time as TLS (it is initialized on the first call to spawn), so the condition in rust_task that is checking whether TLS is initialized is incorrect (it should actually care about when the TCB is constructed).

So, by my reading, this fix will not work correctly if the test is changed to task::spawn(||()); assert false;, since spawn will initialize the main task's TCB and both kill_task_group and cleanup_task will try to call fail_sched_loop.

I think the easiest thing to do here is to keep your change, making cleanup_task always responsible for calling fail_sched_loop in the main task, and remove that condition from kill_task_group along with the rust_task_kill_all runtime function that it uses (and the corresponding entry in rt/rustrt.in).

@alexcrichton If my description sounds correct to you, do you mind making the above change and verifying that it works for the entire test suite with make check?

[alexcrichton]

Certainly, I'll look some more into this, thanks for the pointers!

[alexcrichton]

So here's some stuff that I've uncovered. I'll call the version of the test without task::spawn (the one in this pull request) A, and the one with task span B. B looks like this:

pub fn main() {              
  os::args();                
  task::spawn(||());
  fail!();
}                            

• I went back to rust 0.5, and A has a 0 exit status, B has a nonzero exit status.
• I used this pull request, A has a nonzero exit status, B has a nonzero exit status
• Using this request plus removing rust_task_kill_all both A/B have a nonzero exit status.

With just this pull request gdb says that fail_sched_loop is called both from kill_taskgroup and cleanup_task. I didn't see any assertions trip or anything like that, so apart from this being odd behavior, this may not be causing any problems. All tests also pass when running make check

With this pull request plus removing rust_task_kill_all, tests like run-fail/morestack4, run-fail/linked-failure2 and others all have a 0 exit status when they should have nonzero.

I'm going to keep thinking about this, but I figured I'd at least post an update.

[brson]

Oh, that's funny. The two calls to fail_sched_loop both have independent assertions that they shouldn't be called twice - and sure enough they are each only being called once, but they are both being called.

[catamorphism]

Closing due to lack of activity and wanting to clear out the queue, but please submit a new PR if you make progress on this, @alexcrichton !