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