[libcxx-commits] [PATCH] D75795: [libc++abi] Change __cxa_finalize return type to void
Fangrui Song via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Mar 11 10:46:06 PDT 2020
MaskRay added a comment.
In D75795#1917132 <https://reviews.llvm.org/D75795#1917132>, @ldionne wrote:
> In D75795#1916232 <https://reviews.llvm.org/D75795#1916232>, @MaskRay wrote:
> > 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.
> @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.
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 <https://reviews.llvm.org/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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits