The return type of __cxa_finalize is documented as void in the Itanium
C++ ABI, and it is void in various C libraries.
Details
Details
- Reviewers
EricWF compnerd mclow.lists MaskRay rprichard - Group Reviewers
Restricted Project - Commits
- rG8bd0dc5bfe23: [libc++abi] Do not declare __cxa_finalize and __cxa_atexit in <cxxabi.h>
rGfde9d33f7101: [libc++abi] Change __cxa_finalize return type to void
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Itanium C++ ABI and Linux Standard Base Core Specification define it as returning a void. The declarations in glibc/musl/FreeBSD libc return a void.
| libcxxabi/include/cxxabi.h | ||
|---|---|---|
| 140–142 | void* -> void * | |
Comment Actions
Restore original formatting: void* -> void *
| libcxxabi/include/cxxabi.h | ||
|---|---|---|
| 140–142 | Ok, I can put that back. I had run git clang-format HEAD~1, and that changed it to void*, but void * is more consistent with the rest of the file. | |
Comment Actions
Folks, it's not okay to commit changes to libc++ without having a libc++ or libc++abi code owner review them. If you wait for us to review the change and a review is not coming, you can ping the patch. But you can't commit it just because 2 business days have passed and we haven't said anything.
@MaskRay This isn't the first time this happens. The LLVM community is very loose by giving global commit access to everybody, but we're expecting people to act in good faith and not bypass the usual review process.
Comment Actions
Reverted in 0b017c85ca2ea55131a18ee783fd39a9ac155063.
This is not only for the sake of following the process -- I actually want to consult with linker folks on my side before moving forward with this patch.
Comment Actions
Was phabricator-friendly approach considered?
Why not have a herald rule to auto-put someone from libc++ as blocking reviewer on new libc++ patches instead of such trigger-happy reverts?
Comment Actions
@EricWF is automatically added to these reviews. It's not marked as "blocking" though, since I guess that would create a huge bottleneck on all reviews.
Usually, someone who has several libc++/libc++abi commits will chime in and review these changes. I'm thinking about folks like @compnerd , @phosek , @mstorsjo, and there's others. Whenever one of these folks sign off, I think most people are comfortable and you won't see anyone complain. But this isn't the case here.
This has rarely been a problem so far, but I guess we should consider a libcxxabi/CODE_OWNERS.txt file (@EricWF and I were talking about it just now).
Comment Actions
llvm/CODE_OWNERS.txt lists @mclow.lists as the code owner of libc++, but in reality, many of us know that the triumvirates exists. You and @EricWF can feel free committing and can approve others' changes. By the way, I very much appreciate your reviews. You are working really hard to make libcxx better.
I shall put up one point, libcxxabi is not libcxx. Well, we all know that the number of commits cannot summarize one's contribution to the project, but it at least can commute some interesting facts.
179 Howard Hinnant 135 Eric Fiselier 65 Erik Pilkington 62 Saleem Abdulrasool 53 Petr Hosek 36 Marshall Clow 34 Nick Kledzik 29 Dan Albert 29 Louis Dionne 27 Jonathan Roelofs 24 Logan Chien 23 Nico Weber 17 Richard Smith 14 Shoaib Meenai 11 Asiri Rathnayake 9 Joerg Sonnenberger
I think people will not complain if any of @howard.hinnant @erik.pilkington @compnerd @phosek approves a change. I have close to zero contribution to libcxxabi, I don't deny. However, for this case, I am very confident that Ryan was making the correct change (my only complaint would be [1]). I wrote enough justification why it is correct. I think your action is a bit over the top. People probably don't mind if the realm of libc++ is extended to libc++abi as well, but unnecessarily reverting a change to make it your territory is just not nice. If there is any omission from this patch, please just state it. You could still do a post-commit review.
There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted.
[1]: it'd be nice to strip some Phabricator metadata tags in the commit description:
arcfilter () {
git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}Comment Actions
Hi Louis,
That's fine and I would encourage an abi code owners file if you'd like. That said, from the commit guidelines:
"Smaller patches (or patches where the developer owns the component) that meet likely-community-consensus requirements (as apply to all patch approvals) can be committed prior to an explicit review. In situations where there is any uncertainty, a patch should be reviewed prior to being committed."
Honestly, this should have been a fairly uncontroversial patch. __cxa_finalize is described in the itanium abi (and fwiw here as well: https://opensource.apple.com/source/Libc/Libc-1158.50.2/stdlib/FreeBSD/atexit.c ) as having a particular return type and is well documented as having that return just about everywhere I've seen. This is the sort of thing that likely-community-consensus very much applies towards.
Thoughts? Thanks!
-eric
Comment Actions
No, it’s not, but they are intimately tied.
I think your action is a bit over the top. People probably don't mind if the realm of libc++ is extended to libc++abi as well, but unnecessarily reverting a change to make it your territory is just not nice. If there is any omission from this patch, please just state it. You could still do a post-commit review.
My goal was very much not to make libc++abi my territory by reverting your patch. I’m actually much happier letting it be other people’s territory as much as possible.
I have to agree my reverting was prompt and I could have asked you to do it instead (or commented on this review). I apologize for that, my intent was not to come out as territorial.
What triggered me is that it’s at least the second time we’re calling you out for putting up a review and committing without giving time to respond. Last time I remember having a discussion with other maintainers on Slack and we were all annoyed, even though I was the one to revert your commit (and hence take the flak).
There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted.
Post commit reviews don’t work very well in a world where I don’t even have time to properly review everything in my queue. I can’t speak for other maintainers, but personally when something is committed, I wont review it again unless I need to or have a specific concern about it, because I just don’t have time.
So, from my point of view (again, I can’t speak for other reviewers), post-commit reviews for libc++ and libc++abi don’t work. Just today I spent significant time reading through code that was committed 6 months ago in libc++abi without review and broke something on a platform I maintain. I learned about the existence of that change just today, cause I missed the original commit email.
(Sent from phone)
Comment Actions
I don't understand the argument. I reviewed the patch, not authored it. It was not me who pushed the commit. Even though, I think I should defend for this patch. It apparently met the likely-community-consensus requirements. As a contributor of binutils/gcc/glibc/Linux/lld/MC/etc, I am not illiterate about these low-level stuff. I had checked two standards and three implementations before accepting it. Isn't that sufficient?
docs/CodeReview.rst says:
If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays.
First, my review was responsible. Second, Ryan waited for an extended period of time, nearly 90 hours. It would argue that he did not have to wait for so long. Then you suddenly jumped into the review and reverted the change.
There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted.
Post commit reviews don’t work very well in a world where I don’t even have time to properly review everything in my queue. I can’t speak for other maintainers, but personally when something is committed, I wont review it again unless I need to or have a specific concern about it, because I just don’t have time.
I agree. There are some components in LLVM which I don't want random commits coming in. I also agree that our bar for a permission bit is too low.
So, from my point of view (again, I can’t speak for other reviewers), post-commit reviews for libc++ and libc++abi don’t work. Just today I spent significant time reading through code that was committed 6 months ago in libc++abi without review and broke something on a platform I maintain. I learned about the existence of that change just today, cause I missed the original commit email.
(Sent from phone)
If you think my review for this patch is unqualified, please give concrete reasons. You may be discussing general policies regarding libc++ and libc++abi, but your words "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" was apparently irritating and targeting me. If you wanted to deprive me of the right to approve, I would not agree. If there is any other place other than D52401 where you think I offended you, please point it out. "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" made me very unpleasant.
Comment Actions
@rprichard has no commits to libc++/libc++abi and you both seem to be working together, so I assumed you were mentoring him through -- that's why I @'d you in my original comment. If that's not the case, then I don't understand why we're even discussing this, I should be having this discussion with @rprichard instead.
It apparently met the likely-community-consensus requirements. As a contributor of binutils/gcc/glibc/Linux/lld/MC/etc, I am not illiterate about these low-level stuff. I had checked two standards and three implementations before accepting it. Isn't that sufficient?
I didn't mean to imply you were not qualified to make these changes. It's possible you have more experience with this low-level stuff than I have, for example. That's just not the point I'm trying to make.
docs/CodeReview.rst says:
If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays.
First, my review was responsible. Second, Ryan waited for an extended period of time, nearly 90 hours. It would argue that he did not have to wait for so long. Then you suddenly jumped into the review and reverted the change.
If you're familiar with the libc++/libc++abi review process, you know that waiting 90 hours doesn't mean much. Given our bandwidth, some patches have been stuck for longer than that. I think this is (another) instance where the reality of the project doesn't necessarily align perfectly with the LLVM-wide guidelines. From your perspective, waiting 90 hours on a "simple" change might have looked okay, but from my perspective I didn't even see the change until it went in, without approvals from the usual reviewers.
If you think my review for this patch is unqualified, please give concrete reasons. You may be discussing general policies regarding libc++ and libc++abi, but your words "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" was apparently irritating and targeting me. If you wanted to deprive me of the right to approve, I would not agree. If there is any other place other than D52401 where you think I offended you, please point it out. "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" made me very unpleasant.
Let's get that clear, I have no desire (or even authority) to deprive you of approval rights. What I meant here by "triggered" is that had it been the first time it happened, I would probably have reached out privately or been nicer about it. That one's on me, I apologize. But the change would still have been reverted.
I don't think 6897f99314452 was reviewed either.
I want to make it clear this is not about me thinking you (or @rprichard) not being qualified to make that change. This is an open community, the only way to go about is to judge the contributions themselves, not who's making them. The reason why I reverted the change is that @rprichard committed it without having one of the usual reviewers vet the change. To me, that was a sufficient reason to revert. However, concretely, here's what I would have done on the review had the committer given more time. The Itanium C++ ABI says
When linking any DSO containing a call to __cxa_atexit, the linker should define a hidden symbol __dso_handle, with a value which is an address in one of the object's segments. (It does not matter what address, as long as they are different in different DSOs.) It should also include a call to the following function in the FINI list (to be executed first):
extern "C" void __cxa_finalize ( void *d );
Howard implemented __cxa_finalize originally, and I wanted to both ask Howard whether he recalled a specific reason for the return type differing from the spec, and also look at whether our linker relied/used the int return type. Looking at specs and other implementations is nice, but sometimes things are the way they are because of actual historical reasons, and I always do some archeology before making/vetting API-facing changes that are seemingly trivial. Remember that <cxxabi.h> is a public header shipped by vendors.
Now that I've checked our linker and it doesn't look like it's relying on that, I'm fine with this change.
Next time, try giving folks a heads up by pinging the patch if they don't review it quickly enough, but wait for a frequent reviewer to approve it. We should setup a Phabricator team for libc++abi and automatically block the review on getting one approval from that team. I believe this would have made the expectations clearer here, and none of this would have happened. I'll look into doing that right now.
Comment Actions
We are not working on the same team. The last (uncancelled) LLVM meetup is the only time I saw Ryan.
If that's not the case, then I don't understand why we're even discussing this, I should be having this discussion with @rprichard instead.
I just happened to notice this patch on Recent Activity. I happened to have studied some implementations of __cxa_finalize, so I commented.
Yes, I probably should not be involved. Even though, I don't think violently reverting this change was justified when
It apparently met the likely-community-consensus requirements. As a contributor of binutils/gcc/glibc/Linux/lld/MC/etc, I am not illiterate about these low-level stuff. I had checked two standards and three implementations before accepting it. Isn't that sufficient?
I didn't mean to imply you were not qualified to make these changes. It's possible you have more experience with this low-level stuff than I have, for example. That's just not the point I'm trying to make.
docs/CodeReview.rst says:
If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays.
First, my review was responsible. Second, Ryan waited for an extended period of time, nearly 90 hours. It would argue that he did not have to wait for so long. Then you suddenly jumped into the review and reverted the change.
If you're familiar with the libc++/libc++abi review process, you know that waiting 90 hours doesn't mean much. Given our bandwidth, some patches have been stuck for longer than that. I think this is (another) instance where the reality of the project doesn't necessarily align perfectly with the LLVM-wide guidelines. From your perspective, waiting 90 hours on a "simple" change might have looked okay, but from my perspective I didn't even see the change until it went in, without approvals from the usual reviewers.
I don't disagree with this statement. However,
- nowhere documents the libcxx/ triumvirate
- nowhere says the power of the libcxx/ triumvirate automatically extends to libcxxabi/
- nowhere says the reality of the libcxx/ doesn't necessarily align perfectly with the LLVM-wide guidelines. In particular, a change has to wait for many days, with an approval from the triumvirate.
- nowhere says the libcxx/ culture automatically extends to libcxxabi/.
One contributor has to know all 4 facts to understand that their change could be reverted.
If you just wanted to exercise the power, why not pick a really-misbehaved patch? Why was this innocent patch selected? Reverted, Requested Changes, then accepted again.
If you think my review for this patch is unqualified, please give concrete reasons. You may be discussing general policies regarding libc++ and libc++abi, but your words "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" was apparently irritating and targeting me. If you wanted to deprive me of the right to approve, I would not agree. If there is any other place other than D52401 where you think I offended you, please point it out. "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" made me very unpleasant.
Let's get that clear, I have no desire (or even authority) to deprive you of approval rights. What I meant here by "triggered" is that had it been the first time it happened, I would probably have reached out privately or been nicer about it. That one's on me, I apologize. But the change would still have been reverted.
I don't understand "But the change would still have been reverted."
I don't think 6897f99314452 was reviewed either.
OK, I don't think be52ff95063aa3a5f6784d1c3479511d333c7fd6 d03068c3e1fbc8b8aa24af8e2a806fafa8a92e26 04501a22a073d0f64e980aaa8c895a6e86c0a103 were reviewed either. Some could be risky.
I want to make it clear this is not about me thinking you (or @rprichard) not being qualified to make that change. This is an open community, the only way to go about is to judge the contributions themselves, not who's making them. The reason why I reverted the change is that @rprichard committed it without having one of the usual reviewers vet the change. To me, that was a sufficient reason to revert. However, concretely, here's what I would have done on the review had the committer given more time. The Itanium C++ ABI says
The sentence seems to conflict with your action and previous words? You were judging contributions by who made the change.
When linking any DSO containing a call to __cxa_atexit, the linker should define a hidden symbol __dso_handle, with a value which is an address in one of the object's segments. (It does not matter what address, as long as they are different in different DSOs.) It should also include a call to the following function in the FINI list (to be executed first):
extern "C" void __cxa_finalize ( void *d );Howard implemented __cxa_finalize originally, and I wanted to both ask Howard whether he recalled a specific reason for the return type differing from the spec, and also look at whether our linker relied/used the int return type. Looking at specs and other implementations is nice, but sometimes things are the way they are because of actual historical reasons, and I always do some archeology before making/vetting API-facing changes that are seemingly trivial. Remember that <cxxabi.h> is a public header shipped by vendors.
Now that I've checked our linker and it doesn't look like it's relying on that, I'm fine with this change.
Next time, try giving folks a heads up by pinging the patch if they don't review it quickly enough, but wait for a frequent reviewer to approve it. We should setup a Phabricator team for libc++abi and automatically block the review on getting one approval from that team. I believe this would have made the expectations clearer here, and none of this would have happened. I'll look into doing that right now.
Howard resigned as the libc++ code owner in "r201432 - Remove myself as owner of libc++". His had one activity on average in recent years. Blocking this patch on his review would create a huge bottleneck.
Traditionally, __dso_handle is defined by crtbegin. The linker does not define it. lld optionally defines it because Fuchsia needs it. GNU ld and gold don't do anything special with the symbol __cxa_finalize or __dso_handle.
See also my comment at https://reviews.llvm.org/D28791#1420914 I confess that I didn't check ld64.
Comment Actions
Anyway, I'll sign off from the discussion because we don't need to have it in the first place. I just want to show that I cannot bear an unjustifiable revert, and hope all these misunderstandings will not affect our future collaboration.
Comment Actions
Just FYI because I think it's interesting -- we do have a private Slack channel where maintainers hang out, and we sometimes ask Howard questions about weird historical stuff. He's always incredibly helpful and often has surprising things to teach us. Of course, we almost never block reviews on his input, but we do reach out when other forms of archeology fail.
Anyway, I'll sign off from the discussion because we don't need to have it in the first place. I just want to show that I cannot bear an unjustifiable revert,
Indeed, I don't think anything productive is coming out of this discussion, as you don't find my reason for reverting justifiable. There's not much I can do on my end to change that perception.
Judging from your previous reply, it seems clear to me that you were personally offended by my revert. So I apologize for that, that wasn't my intention. My intention was to make sure contributors follow the process that we traditionally have for reviews, even though (as you point out) that process is not textbook the same as documented in the commit guidelines.
and hope all these misunderstandings will not affect our future collaboration.
Speaking for myself, it will not.
Comment Actions
Sorry about this -- I'll try to be more careful in the future about getting my changes vetted.
@rprichard has no commits to libc++/libc++abi
Pedantic: I do have a couple libcxxabi patches, but @EricWF committed them for me (D36446 and D36447).
Why are we declaring this function at all?
I don't know. __cxa_finalize and __cxa_atexit are part of the IA-64 C++ ABI, so I suppose the notice at the top of the file applies. They're implemented elsewhere, though, not in libcxxabi.
/* * This header provides the interface to the C++ ABI as defined at: * https://itanium-cxx-abi.github.io/cxx-abi/ */
Comment Actions
The function cannot be called directly by any user, and isn't implemented by the library. So we don't own it and can't call it.
Just because it's a part of the ABI spec doesn't mean we need to provide a declaration.
Even after this patch the declaration is still wrong. Because we mark it with _LIBCPP_FUNC_VIS, but the spec specifically says the function should have hidden visibility.
My vote is to remove it.
Comment Actions
Agreed. In practice (libgcc) only crtbeginS.o (not crtbegin.o/crtbeginT.o) calls __cxa_finalize (in compiler-rt, __cxa_finalize is an undefined weak symbol). No user code calls it.
Even after this patch the declaration is still wrong. Because we mark it with _LIBCPP_FUNC_VIS, but the spec specifically says the function should have hidden visibility.
__dso_handle is hidden. __cxa_finalize is not.
My vote is to remove it.
+1
Comment Actions
I just noticed that GNU libsupc++'s cxxabi.h declares __cxa_finalize with an int return type.
Comment Actions
SGTM, but we could leave a comment saying that __cxa_finalize isnt' declared here for <reasons>. AFAICT, __cxa_atexit is in the same boat, by the way, so should we remove that one too?
Comment Actions
Trying to clean up the review queue. Commandeering to propose what seems to be the consensus here.
Comment Actions
I think I'm fine with this revision. I was wondering if the two functions should be removed from the GNU libsupc++'s cxxabi.h as well, which also declares both of the functions with an int return type. (i.e. It has the same issue as LLVM's cxxabi.h.)
Aside: I noticed that libcxxrt, another implementation of the C++ ABI library, has a cxxabi.h without __cxa_atexit and __cxa_finalize declarations. libcxxrt does have implementations of those two APIs, but only for __sun__ targets. The __sun__ __cxa_finalize has a void return type, as expected.
| libcxxabi/include/cxxabi.h | ||
|---|---|---|
| 141 | The return type of __cxa_atexit should remain as int. | |
Comment Actions
Change __cxa_atexit back to returning int
| libcxxabi/include/cxxabi.h | ||
|---|---|---|
| 141 | Uh, yeah, sorry. I don't know why I changed it. | |
The return type of __cxa_atexit should remain as int.