[libcxx-commits] [PATCH] D73584: [libunwind] Rename __personality_routine to _Unwind_Personality_Fn

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd added a comment.

In D73584#1862820 <https://reviews.llvm.org/D73584#1862820>, @steven_wu wrote:

> In D73584#1862464 <https://reviews.llvm.org/D73584#1862464>, @zero9178 wrote:
>
> > In D73584#1862110 <https://reviews.llvm.org/D73584#1862110>, @steven_wu wrote:
> >
> > > I hope we are not renaming everything because it add complexity on compatibility.
> > >
> > > Rename only the refs from libunwind is not enough since there are refs from clang header and libc++abi as well. unwind.h and corresponding header from clang are both shipping products which add compatibility problem.
> >
> >
> > Git grepping for __ personality_routine after the patch yields only three results. One is the typedef inside of the clang Headers, Two are inside of tests of llvm that I am not sure are relevant here. libc++abi uses the name _Unwind_Personality_Fn instead of __personality_routine that libunwind has used prior to this patch. _Unwind_Personality_Fn is also what GCC calls it. So this patch actually makes it possible to compile libc++abi with libunwind (Build errors only show up on windows since only windows has actually used _Unwind_Personality_Fn ).
> >
> > Regarding compatibility I am not sure, but I thought users are expected to include unwind.h from clang Headers anyways.
>
>
> Won't the typedef in clang header turns into an error? That is also the compatibility problem. If you have a mismatch between clang header and libunwind header, it is going to break one way or the other.


I don't think that is a real concern - the typedef is local, and references a local definition (that is, clang's `unwind.h` defines both `__Unwind_Personality_Fn` and `__personality_routine`).

Furthermore, @zero9178 didn't mention that the local definition that exists actually is under a preprocessor macro for ARM EHABI specifically.  So you are looking at a mismatch on ARM (GNU)EABI environments where it would have previously caused an error already in K&R, ANSI, or C89 due to the type redefinition.  In C99 or newer, the type redefinition is permitted and then the type alias will reference the locally declared function type.  This is all ignoring the fact that mixing and matching the headers like that is already prone to errors.

I believe that the patch in the current incarnation is the right approach.


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

https://reviews.llvm.org/D73584