Page MenuHomeFreeBSD

Continuation of D7224.
ClosedPublic

Authored by kib on Aug 6 2016, 8:31 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kib retitled this revision from to Continuation of D7224..Aug 6 2016, 8:31 AM
kib updated this object.
kib edited the test plan for this revision. (Show Details)
kib added reviewers: mmokhi, emaste, theraven.
kib set the repository for this revision to rS FreeBSD src repository.
kib updated this revision to Diff 19083.
kib updated this revision to Diff 19084.Aug 6 2016, 8:56 AM

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

kib added a comment.Aug 6 2016, 8:58 AM

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 edited edge metadata.Aug 6 2016, 9:59 AM

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 added a comment.Aug 6 2016, 10:07 AM

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 edited edge metadata.Aug 6 2016, 10:11 AM
theraven accepted this revision.

All sounds sensible to me.

This revision is now accepted and ready to land.Aug 6 2016, 10:11 AM
mmokhi edited edge metadata.Aug 6 2016, 10:23 AM
mmokhi accepted this revision.

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.

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 added a comment.Aug 6 2016, 10:33 AM
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 added a comment.EditedAug 6 2016, 10:36 AM
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 added a comment.Aug 6 2016, 10:40 AM
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 added a comment.EditedAug 6 2016, 10:52 AM

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 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 added a comment.Aug 6 2016, 12:51 PM
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 edited edge metadata.Aug 6 2016, 12:53 PM
kib updated this revision to Diff 19090.

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

This revision now requires review to proceed.Aug 6 2016, 12:53 PM
mmokhi edited edge metadata.Aug 6 2016, 1:30 PM
mmokhi accepted this revision.

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

This revision is now accepted and ready to land.Aug 6 2016, 1:30 PM
kib closed this revision.Aug 6 2016, 1:36 PM