[PATCH] D15883: Add ARM EHABI-related constants to unwind.h.

[Saleem Abdulrasool via cfe-commits]

compnerd added inline comments.

================
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
----------------
logan wrote:
> logan wrote:
> > compnerd wrote:
> > > logan wrote:
> > > > compnerd wrote:
> > > > > logan wrote:
> > > > > > Since this is `unwind.h`, I feel that we can get a step further and use `__ARM_EABI_UNWINDER__` to get more compatibility to GCC's unwind.h.
> > > > > > 
> > > > > > Here's the change:
> > > > > > 
> > > > > > ```
> > > > > > #if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \
> > > > > >     !defined(__ARM_DWARF_EH__)
> > > > > > #define __ARM_EABI_UNWINDER__ 1
> > > > > > #endif
> > > > > > ```
> > > > > I dont know if we really need to imitate GCC's macros here.  Am I mistaken in that they assume that `__ARM_EABI_UNWINDER__` has been set to 1 externally if targeting such an environment?  I think that it is better to use the reserved namespace and intrude into libunwind's namespace as already done here.
> > > > > Am I mistaken in that they assume that `__ARM_EABI_UNWINDER__` has been set to 1 externally if targeting such an environment?
> > > > 
> > > > Although this is an implementation detail, it was defined by `unwind.h` in the implementation of GCC.
> > > > 
> > > > Remark: `__ARM_EABI_UNWINDER__` is not a pre-defined macro in GCC and Clang (can be checked with ` gcc -dM -E - < /dev/null`.)
> > > > 
> > > > BTW, some applications or libraries need this macro to be defined after including `<unwind.h>` (such as uclibc, boost, or libc++abi 3.0.)  I remembered that someone suggested to use `__ARM_EABI_UNWINDER__` instead of  `LIBCXXABI_ARM_EHABI` when I was fixing libc++abi several years ago.  I chose `LIBCXXABI_ARM_EHABI` simply because `__ARM_EABI_UNWINDER__` wasn't provided by clang at that time.
> > > > 
> > > > I am less concerned to namespace pollution, because this is already the de facto implementation in GCC world and the macro names start with underscores are reserved for compiler or standard libraries by convention.
> > > > 
> > > > Since this is file a public header and will be used for a long time, I personally believe that it will be better to use an existing name with the same meaning instead of introducing a new name.  In addition, this will make it easier to port the application between gcc and clang.
> > > I just checked, libc++abi has no use of this macro, nor does boost 1.60.  uclibc only defines `__ARM_EABI_UNWINDER__`, but does not use it.  I also checked glibc and musl, and glibc like uclibc defines it while musl has no references to it.  This is injecting itself into the compiler namespace and is misleading, so I think I would really rather prefer the current patch as is.
> > > I just checked, libc++abi has no use of this macro, nor does boost 1.60. uclibc only defines __ARM_EABI_UNWINDER__, but does not use it. I also checked glibc and musl, and glibc like uclibc defines it while musl has no references to it.
> > 
> > For uClibc++ and Boost I only did a simple Google search while writing the previous reply.  Sorry for the brevity.
> > 
> > Although uClibc++ itself does not use `__ARM_EABI_UNWINDER__`, some third-party ARM ports are using this macro.  For example, [toyroot](https://github.com/luckboy/toyroot), a small build system for small linux distribution, is maintaining a [local patch](https://github.com/luckboy/toyroot/blob/master/patch/uClibc%2B%2B-0.2.4-arm-eabi-unwinder.patch).  Yet another example, [Aboriginal Linux](http://landley.net/aboriginal/) has [another patch](http://www.landley.net/hg/aboriginal/file/tip/sources/patches/uClibc%2B%2B-unwind-cxx.patch) that requires this macro.  Someone even sent a [patch](http://lists.uclibc.org/pipermail/uclibc/2012-June/046915.html) to uClibc++ mailing list.
> > 
> > For Boost, I am referring to [this thread](http://lists.boost.org/Archives/boost/2008/04/136332.php), although it seems not being committed.
> > 
> > For libc++abi, I am referring to the [earlier version](http://llvm.org/klaus/libcxxabi/blob/8b547a338373b6e149d8ceed34bbf6a979a1e10d/src/cxa_exception.hpp) (roughly 3.4.)  You won't find `__ARM_EABI_UNWINDER__` in libc++abi master branch because I removed it in [05d51bcf07](http://llvm.org/klaus/libcxxabi/commit/05d51bcf07d0ec1c40785b4a860fd917410b4be1/) when I was implementing the ARM EHABI support.  I remembered in the [review comments](http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140414/103125.html) Jonathan even suggested me to use `__ARM_EABI_UNWINDER__` instead.  I couldn't do  so because `__ARM_EABI_UNWINDER__` was not defined by `<clang-src>/lib/Headers/unwind.h`.
> > 
> > The main purpose to mention these projects is to demonstrate that `__ARM_EABI_UNWINDER__` is a common knownledge between unwinder or personality developers.  Many of us will come up with `__ARM_EABI_UNWINDER__` when we need to distinguish ARM EHABI code and Itanium code.
> > 
> > > This is injecting itself into the compiler namespace and is misleading, so I think I would really rather prefer the current patch as is.
> > 
> > I have a completely opposite point of view.  Please notice that the subject we are referring to is the unwind.h distributed with clang (`<clang-src>/lib/Headers/unwind.h`) which will usually be installed at `<llvm-install-prefix>/lib/clang/<version>/include/unwind.h`.  This file is a part of compiler and maintained by the compiler developer.  Thus, IMO, we SHOULD keep macros in compiler namespace.
> > 
> > BTW, IMO, both `_UNWIND_ARM_EHABI` and `__ARM_EABI_UNWINDER__` belongs to compiler namespace (both of them start with a underscore), so this criteria is not the reason to flavor one over the other.
> > 
> > ```
> > #if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \
> >     !defined(__ARM_DWARF_EH__)
> > #define _UNWIND_ARM_EHABI 1
> > #else
> > #define _UNWIND_ARM_EHABI 0
> > #endif
> > ```
> > 
> > Let's get back to these `#if` and `#define`.  I have two arguments against the changes in the second revision:
> > 
> > 1. As a public header provided by compiler, I believe it will be better to eliminate every unnecessary macros.  This macro is not a must-have for non-ARM platforms.  We can simply change the upcoming `#if` to `#ifdef` or `#if defined(...)`.  In the other words, IMO, we don't need the `#else` part.
> > 
> > 2. I prefer `__ARM_EABI_UNWINDER__` to `_UNWIND_ARM_EABI` for four reasons:
> > 
> >    a. As mentioned earlier, some application code relies on `__ARM_EABI_UNWINDER__`.  Using `__ARM_EABI_UNWINDER__` can reduce the effort to port the program around.
> > 
> >    b. `__ARM_EABI_UNWINDER__` is battle tested.  If a program which includes `<unwind.h>` has been compiled with `arm-linux-gnueabi-g++`, we can make sure that the program is not using `__ARM_EABI_UNWINDER__` as identifier.  On the contrary, although the possibility is low, someone may name his variable with `_UNWIND_ARM_EHABI` and introducing `_UNWIND_ARM_EHABI` to compiler header will break his program.
> > 
> >    c. Using `__ARM_EABI_UNWINDER__` can reduce the divergence between gcc and clang.
> > 
> >    d. I personally prefer `__ARM_EABI_UNWINDER__` because it looks similar to architecture-specific pre-defined macros, such as `__ARM_EABI__` and `__ARM_ARCH_7A__`.
> Hi @compnerd,
> 
> I know that my arguments for `__ARM_EABI_UNWINDER__` are mainly due to the historical reason and are not inherent to the name itself.  If the history were different (e.g. some GCC developer chose `_UNWIND_ARM_EHABI`), then I will favor `_UNWIND_ARM_EHABI` over `__ARM_EABI_UNWINDER__`.  But, unfortunately, we are living in the world that `__ARM_EABI_UNWINDER__` was coined first.  IMHO, it will really be an advantage to reduce the divergence.
> 
> Or, do you have other concerns that I haven't addressed or thought of?  Thanks for your understanding.
Exactly, its historical only.  There is no good reason to continue with it, and i don't really see how this is a benefit.  How would we deal with a similar macro being defined by the compiler?  I really think that it is significantly safer to use the current proposed name (_UNWIND_ARM_EABI).  We could also duplicate the check to eschew the selection of a name.

Either way, I don't think that we should further hold up this patch on this.  This is reasonable, and I can see it simplifying things for people so we should get this merged.


http://reviews.llvm.org/D15883