logan added inline comments. ================ Comment at: lib/Headers/unwind.h:61 @@ +60,3 @@ +#define _UNWIND_ARM_EHABI 0 +#endif + ---------------- 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__`. http://reviews.llvm.org/D15883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
