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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D75795#1918579 <https://reviews.llvm.org/D75795#1918579>, @EricWF wrote:

> In D75795#1918353 <https://reviews.llvm.org/D75795#1918353>, @rprichard wrote:
>
> > 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 <https://reviews.llvm.org/D36446> and D36447 <https://reviews.llvm.org/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/
> >    */
> >
>
>
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75795