[libcxx-commits] [PATCH] D75795: [libc++abi] Change __cxa_finalize return type to void

Wed Mar 11 09:37:12 PDT 2020

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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. 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 <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 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75795/new/

https://reviews.llvm.org/D75795