libc++abi: update comment in cxa_thread_atexit.cpp. NFC
#18501
Conversation
| @@ -113,8 +113,8 @@ extern "C" { | |||
| return __cxa_thread_atexit_impl(dtor, obj, dso_symbol); | |||
| #else | |||
| #ifndef __EMSCRIPTEN__ | |||
| // Emscripten doesn't fully support weak undefined symbols yet | |||
| // https://github.com/emscripten-core/emscripten/issues/12819 | |||
| // musl doesn't provide __cxa_thread_atexit_impl | |||
There was a problem hiding this comment.
Can we instead remove the ifndef block now since I think we do support weak undefined symbols fully now?
There was a problem hiding this comment.
I think we could remove this ifndef-block, but I'm not sure if it will ever reach the __cxa_thread_atexit_impl() call (given that we always build against musl). Hence I only updated the comment.
Do you think we should remove this even though it always falls in the else block?
There was a problem hiding this comment.
I guess my argument would be that this ifdef was only added because in the past we had bad support for weak undefined symbols.. now that we don't we can remove the ifdef.
However, I do agree that the if block is dead code in the emscripten case... with static linking + LTO think should be completely removed, but I guess it makes sense to keep the ifdef for the non-LTO or dynamic linking case.
I don't really see why the bug you link to is relevant. How about updating the comment to say We know emscripten doesn't implement __cxa_thread_atexit_impl so we can simply avoid this check.
|
Test failures in the |
Noticed while investigating issue #15722.