logan added inline comments.
================
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
----------------
compnerd wrote:
> 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.
> We could also duplicate the check to eschew the selection of a name.
OK. I agree that this is a fine compromise to reach out.
@timonvo: would you mind to update the patch by replacing `_UNWIND_ARM_EHABI`
with following preprocessor condition? I will accept the revised patch as soon
as possible.
```
#if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \
!defined(__ARM_DWARF_EH__)
...
#endif
```
> 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.
Yeah. I wish to move forward as soon as possible as well.
http://reviews.llvm.org/D15883
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
