Add ARM EHABI-related constants to unwind.h.

[timonvo]

Summary

Adds a number of constants, defined in the ARM EHABI spec, to the Clang
lib/Headers/unwind.h header. This is prerequisite for landing
http://reviews.llvm.org/D15781, as previously discussed there.

[rengolin]

Tests?

[timonvo]

In D15883#319445, @rengolin wrote:

Tests?

Sure, I could add some. But is it common practice to test constants? The tests would end up being a duplication of the definition, I'm not sure how valuable that would be. I also couldn't find tests for any of the existing constants, at least not through a quick search under Clang's test/ directory.

Or were you thinking of some other type of test?

[rengolin]

Well, I only saw later that these are propositions from another patch...

I don't see why this can't be part of the original patch, but I'm ok with no tests if they're used (and tested) on the final patch.

I'll defer to Logan to decide. :)

[timonvo]

Logan, how should we proceed here? Can you approve the patch?

I also have no committer rights to the project, so does that mean you (or someone else) need to commit it on my behalf?

[timonvo]

Could someone please take a look at this patch as well as http://reviews.llvm.org/D15781? Thanks!

[compnerd]

These should be guarded by a check to ensure that they are not defined when EHABI is not in effect at @nbjoerg pointed out. You could use the same check as libunwind:

defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__ARM_DWARF_EH__)

[timonvo]

Guard ARM EHABI enums using \#ifs (compnerd/nbjoerg's advice).

[timonvo]

Hi compnerd, thanks for getting back to me. I've updated the code.

I was wondering though, where did you see @nbjoerg's comment? I couldn't find any reference to it here or on cfe-commits.

[compnerd]

It was on the mailing list, which won't show up on phabricator (email is still the defacto review system).

Why not create a local macro and use that to make this easier to read?

#if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__ARM_DWARF_EH__)
#define _UNWIND_USE_AEABI 1
#else
#define _UNWIND_USE_AEABI 0
#endif

[timonvo]

Added local macro for enhanced readability.

[timonvo]

Note that I avoided using a local macro at first because I didn't want to export any additional symbols/macros from this file. But I've moved to use one now. Note that I changed the name slightly to be more in line with libunwind's naming (it uses _LIBUNWIND_ARM_EHABI as the macro name).

[logan]

In general, it looks good to me if the comments are addressed.

Sorry for not replying responsively. Not sure why I missed the follow-up e-mails after @rengolin's reply on Jan 5th.

lib/Headers/unwind.h

61

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

89

#ifdef __ARM_EABI_UNWINDER__

166

#ifdef __ARM_EABI_UNWINDER__

[compnerd]

This change seems fine to me as is, just waiting to iron out the macro situation with @logan before accepting it.

lib/Headers/unwind.h

61

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.

[logan]

lib/Headers/unwind.h

61

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.

[compnerd]

lib/Headers/unwind.h

61

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.

[logan]

lib/Headers/unwind.h

61

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, a small build system for small linux distribution, is maintaining a local patch. Yet another example, Aboriginal Linux has another patch that requires this macro. Someone even sent a patch to uClibc++ mailing list.

For Boost, I am referring to this thread, although it seems not being committed.

For libc++abi, I am referring to the earlier version (roughly 3.4.) You won't find __ARM_EABI_UNWINDER__ in libc++abi master branch because I removed it in 05d51bcf07 when I was implementing the ARM EHABI support. I remembered in the review comments 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__.

[logan]

lib/Headers/unwind.h

61

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.

[compnerd]

lib/Headers/unwind.h

61

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.

[logan]

lib/Headers/unwind.h

61

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.

[timonvo]

That's basically the Diff 48015 I had at some point. Reverted back to it now. Now this patch doesn't declare any macros anymore.

[logan]

LGTM. It is good to go now.

@timonvo, do you have commit access? Or, do you need some assistance?

[timonvo]

I don't have commit access. Can I land this CL myself somehow without commit access (e.g. using arc), or will you have to submit it for me?

[logan]

Hi @timonvo,

I have committed this change as rL262178. Thanks for your work.

Let's move forward to D15781.

lib/Headers/unwind.h

 struct _Unwind_Exception;
 typedef enum {
   _URC_NO_REASON = 0,
+#if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \
+    !defined(__ARM_DWARF_EH__)
+  _URC_OK = 0, /* used by ARM EHABI */
+#endif
   _URC_FOREIGN_EXCEPTION_CAUGHT = 1,
 
   _URC_FATAL_PHASE2_ERROR = 2,

   _URC_END_OF_STACK = 5,
   _URC_HANDLER_FOUND = 6,
   _URC_INSTALL_CONTEXT = 7,
-  _URC_CONTINUE_UNWIND = 8
+  _URC_CONTINUE_UNWIND = 8,
+#if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \
+    !defined(__ARM_DWARF_EH__)
+  _URC_FAILURE = 9 /* used by ARM EHABI */
+#endif
 } _Unwind_Reason_Code;
 
 typedef enum {

   _UVRSR_FAILED = 2
 } _Unwind_VRS_Result;
 
+#if !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__ARM_DWARF_EH__)
+typedef uint32_t _Unwind_State;
+#define _US_VIRTUAL_UNWIND_FRAME  ((_Unwind_State)0)
+#define _US_UNWIND_FRAME_STARTING ((_Unwind_State)1)
+#define _US_UNWIND_FRAME_RESUME   ((_Unwind_State)2)
+#define _US_ACTION_MASK           ((_Unwind_State)3)
+#define _US_FORCE_UNWIND          ((_Unwind_State)8)
+#endif
+
 _Unwind_VRS_Result _Unwind_VRS_Get(struct _Unwind_Context *__context,
   _Unwind_VRS_RegClass __regclass,
   uint32_t __regno,