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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
