Continuation of D7224.

[kib]

Summary

I spent a time with the patch.

WRT the functional part of the patch, main observation that I have is that there is no reason to leave failed destructor records on the list. It is required to leave them for atexit handler, from where the code seems to be copied, but for the cxa_thread the end result is memory leak. If all records, both succeeded and failed, are removed during the pass, we can get without the list generation at all: restart is needed iff the list is not empty after the iterator finished. For the same reason, if we maxed our attempts count, the list still must be cleared, but now without the destructor calls.

I also reworded and reformatted comments, and did some minor but numerous editorial changes.

I added several more tests. The biggest omission was the lack of the test which does not link libthr into the address space. Another good to have test is the explicit infinite destructor stream, to check the CXA_DTORS_ITERATIONS logic.

Please read and confirm that you are fine with the updated patch. I will check universe build after your confirmation and then commit.

[kib]

Centralize iterator. Use callbacks to specify per-dtor action.

[kib]

I still have one question about the patch.

The __cxa_thread_atexit() signature specifies int as the return value. You return errno as error indicator, similar to the modern POSIX conventions, while typical unix tradition is to return -1 and set errno. Glibs never returns an error, in similar situation of failed malloc() it segfaults. Judging from the code generated by clang, the error returned is ignored.

Is there a spec for __cxa_thread_atexit() anywhere ? Is it allowed to return an error, and if yes, in which way (POSIX/Unix) must it be done ?

[theraven]

Thank you for all of the time that you've spent on this. __cxa_thread_atexit is not in the latest version of the ABI spec, so it's currently an informal convention between the compiler and runtime library. The compiler doesn't check for failure, so it's probably best to abort for dangerous failures. The compiler-generated code probably should be checking for failure, invoking the destructor for the object, and throwing an exception, but that's something for WG21 to decide.

I'm not convinced by CXA_DTORS_ITERATIONS, as I think it would be entirely valid (though spectacularly bad style) C++11 to use registering new thread-local variables to perpetually resurrect a thread.

[kib]

In D7427#154494, @theraven wrote:

Thank you for all of the time that you've spent on this. __cxa_thread_atexit is not in the latest version of the ABI spec, so it's currently an informal convention between the compiler and runtime library. The compiler doesn't check for failure, so it's probably best to abort for dangerous failures. The compiler-generated code probably should be checking for failure, invoking the destructor for the object, and throwing an exception, but that's something for WG21 to decide.

Libraries must not abort, esp.C library.

I'm not convinced by CXA_DTORS_ITERATIONS, as I think it would be entirely valid (though spectacularly bad style) C++11 to use registering new thread-local variables to perpetually resurrect a thread.

I object against a possiblity for user code to delay/deny the thread termination requested by pthread_exit(). POSIX has similar opinion there, with PTHREAD_DESTRUCTOR_ITERATIONS idea.

The key destructors and destructors called during stack unwinding could introduce unterminated loops, but that the only current failure mode, and I do not want to introduce a new one.

I tend to consider the current code as the field experiment. Both CXA_DTORS_ITERATIONS and dso unload issues are made to have the library interfaces reasonable; we will see how that interacts with the real world. In particular, all questionable places issue verbose-so-much-that-it-is-silly diagnostics.

[theraven]

All sounds sensible to me.

[mmokhi]

Thanks for cleaning up code using cb_walk() idea.

I confirm it's okay from my side too.
I also tested the tests you've added, all was okay.

[mmokhi]

In D7427#154489, @kib wrote:

I still have one question about the patch.

The __cxa_thread_atexit() signature specifies int as the return value. You return errno as error indicator, similar to the modern POSIX conventions, while typical unix tradition is to return -1 and set errno. Glibs never returns an error, in similar situation of failed malloc() it segfaults. Judging from the code generated by clang, the error returned is ignored.

Is there a spec for __cxa_thread_atexit() anywhere ? Is it allowed to return an error, and if yes, in which way (POSIX/Unix) must it be done ?

I used ENOMEM value as a convention i respected always when i code, i have no insist on it as the return value is not being checked.

[kib]

In D7427#154514, @mokhi64_gmail.com wrote:

I used ENOMEM value as a convention i respected always when i code, i have no insist on it as the return value is not being checked.

This is not the question. ENOMEM is the reasonable error to return in this case, but there are more than one way to return an error. The question was about the later and not about the former.

[mmokhi]

In D7427#154518, @kib wrote:

In D7427#154514, @mokhi64_gmail.com wrote:

I used ENOMEM value as a convention i respected always when i code, i have no insist on it as the return value is not being checked.

This is not the question. ENOMEM is the reasonable error to return in this case, but there are more than one way to return an error. The question was about the later and not about the former.

Ah, i thought you asked me about return (-1); and/or return (ENOMEM); in the fail cases.

[kib]

In D7427#154522, @mokhi64_gmail.com wrote:

In D7427#154518, @kib wrote:

In D7427#154514, @mokhi64_gmail.com wrote:

I used ENOMEM value as a convention i respected always when i code, i have no insist on it as the return value is not being checked.

This is not the question. ENOMEM is the reasonable error to return in this case, but there are more than one way to return an error. The question was about the later and not about the former.

Ah, i thought you asked me about return (-1); and/or return (ENOMEM); in the fail cases.

Yes, I did asked about

errno = ENOMEM; /* do not rely on malloc(3) errno */
return (-1);

vs. current

return (ENOMEM);

[mmokhi]

Well, I think second one (returning -1 and setting errno) is more compatible with what most of FreeBSD libc is coded than current one.
I guess it'd be better if we change the current one [noting that return value is not checked]

[mmokhi]

I don't have permission to update diff on revision to do this (if you're agree with changing return statement), so i updated D7224 😁😅 to keep a reference for last changes.

[kib]

In D7427#154529, @mokhi64_gmail.com wrote:

Well, I think second one (returning -1 and setting errno) is more compatible with what most of FreeBSD libc is coded than current one.
I guess it'd be better if we change the current one [noting that return value is not checked]

I am surprised to see that gcc libstdc++ implements __cxa_thread_atexit() on its own. There, on error they just return -1, so I changed the patch to do same.

[kib]

Return -1 from __cxa_thread_atexit on malloc error.
Fix gcc (pre-std c++11 compilers) build.

[mmokhi]

As i pointed before, I agree with changing return statement too (that's why i updated old D7224)
So all are sensible IMO 😄.