[libunwind] Ensure enough alignment for unw_cursor_t for SEH build configurations

[mstorsjo]

Summary

When built in SEH mode, UnwindCursor contains a CONTEXT struct, which is aligned to 16 bytes in most configurations, causing the whole UnwindCursor object to have 16 byte alignment.

This fixes backtraces using _Unwind_Backtrace on x86_64 mingw, where an unw_cursor_t allocated on the stack was misaligned before.

This is an ABI break for this struct for this configuration, but very few callers call libunwind directly (and even fewer directly allocate an unw_cursor_t anyway).

[mstorsjo]

@compnerd What do you think about this one?

[compnerd]

libunwind/include/libunwind.h

78

Can you extract this into a separate declaration and use that please? Its purely a readability concern:

#if defined(_WIN32) && defined(__SEH__)
#define _LIBUNWIND_CURSOR_ALIGNMENT_ATTR __attribute__((__aligned__(16)))
#else
#define _LIBUNWIND_CURSOR_ALIGNMENT_ATTR
#endif

[mstorsjo]

libunwind/include/libunwind.h

78

Sure, can do. But doing it without the leading underscore, to keep it consistent with an existing such define further up in the file.

[mstorsjo]

Moved the attribute to a separate define, as suggested by @compnerd.

[mstorsjo]

@hans - What do you think about this for the release branch? It fixes the use of _Unwind_Backtrace in SEH build configurations on x86_64 - but it's rather late in the release process already, so it's not a must.

[rprichard]

After this change, I see a compile error with GCC. There's something broken with libunwind's static_assert emulation. I'd guess the fix is to remove that emulation and simply use the C++11 static_assert.

Regarding static_assert: there are two compile_time_assert_failed[] declarations within a single scope, and GCC thinks the declarations are incompatible. Somehow it matters that the array condition depends on a template parameter. Here's a simple demo that compiles with Clang but not GCC, https://godbolt.org/z/M3sc4x.

The libunwind error:

/x/llvm-upstream/llvm-project/libunwind/src/config.h:27:18: error: conflicting declaration ‘int libunwind::compile_time_assert_failed [((__alignof__ (libunwind::UnwindCursor<A, R>) <= 8) ? 1 : -1)]’
   27 |       extern int compile_time_assert_failed[ ( __b ) ? 1 : -1 ]  \
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
/x/llvm-upstream/llvm-project/libunwind/src/UnwindCursor.hpp:1187:3: note: in expansion of macro ‘static_assert’
 1187 |   static_assert((alignof(UnwindCursor<A, R>) <= alignof(unw_cursor_t)),
      |   ^~~~~~~~~~~~~
/x/llvm-upstream/llvm-project/libunwind/src/config.h:27:18: note: previous declaration as ‘int libunwind::compile_time_assert_failed [(check_fit<libunwind::UnwindCursor<A, R>, unw_cursor_t>::does_fit ? 1 : -1)]’
   27 |       extern int compile_time_assert_failed[ ( __b ) ? 1 : -1 ]  \
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
/x/llvm-upstream/llvm-project/libunwind/src/UnwindCursor.hpp:1185:3: note: in expansion of macro ‘static_assert’
 1185 |   static_assert((check_fit<UnwindCursor<A, R>, unw_cursor_t>::does_fit),
      |   ^~~~~~~~~~~~~

[mstorsjo]

In D86102#2232290, @rprichard wrote:

After this change, I see a compile error with GCC. There's something broken with libunwind's static_assert emulation. I'd guess the fix is to remove that emulation and simply use the C++11 static_assert.

Regarding static_assert: there are two compile_time_assert_failed[] declarations within a single scope, and GCC thinks the declarations are incompatible. Somehow it matters that the array condition depends on a template parameter. Here's a simple demo that compiles with Clang but not GCC, https://godbolt.org/z/M3sc4x.

Ouch, sorry about this - I hadn't really anticipated such an error.

@hans - I guess that makes it too messy for backporting. Or I guess it could be done by leaving out the static asserts...

[hans]

@hans - I guess that makes it too messy for backporting. Or I guess it could be done by leaving out the static asserts...

It's also late in the process, so I'd rather pass this time.

[mstorsjo]

In D86102#2234174, @hans wrote:

@hans - I guess that makes it too messy for backporting. Or I guess it could be done by leaving out the static asserts...

It's also late in the process, so I'd rather pass this time.

Ok, very well!

libunwind/include/libunwind.h

#else
     #else
       #define LIBUNWIND_AVAIL __attribute__((unavailable))
     #endif
   #endif
 #else
   #define LIBUNWIND_AVAIL
 #endif
 
+#if defined(_WIN32) && defined(__SEH__)
+  #define LIBUNWIND_CURSOR_ALIGNMENT_ATTR __attribute__((__aligned__(16)))
+#else
+  #define LIBUNWIND_CURSOR_ALIGNMENT_ATTR
+#endif
+
 /* error codes */
 enum {
   UNW_ESUCCESS      = 0,     /* no error */
   UNW_EUNSPEC       = -6540, /* unspecified (general) error */
   UNW_ENOMEM        = -6541, /* out of memory */
   UNW_EBADREG       = -6542, /* bad register number */
   UNW_EREADONLYREG  = -6543, /* attempt to write read-only register */
   UNW_ESTOPUNWIND   = -6544, /* stop unwinding */

 
 struct unw_context_t {
   uint64_t data[_LIBUNWIND_CONTEXT_SIZE];
 };
 typedef struct unw_context_t unw_context_t;
 
 struct unw_cursor_t {
   uint64_t data[_LIBUNWIND_CURSOR_SIZE];
-};
+} LIBUNWIND_CURSOR_ALIGNMENT_ATTR;
 typedef struct unw_cursor_t unw_cursor_t;
 
 typedef struct unw_addr_space *unw_addr_space_t;
 
 typedef int unw_regnum_t;
 typedef uintptr_t unw_word_t;
 #if defined(__arm__) && !defined(__ARM_DWARF_EH__)
 typedef uint64_t unw_fpreg_t;
 #else

libunwind/src/UnwindCursor.hpp

 };
 
 
 template <typename A, typename R>
 UnwindCursor<A, R>::UnwindCursor(unw_context_t *context, A &as)
     : _addressSpace(as), _unwindInfoMissing(false) {
   static_assert((check_fit<UnwindCursor<A, R>, unw_cursor_t>::does_fit),
                 "UnwindCursor<> does not fit in unw_cursor_t");
+  static_assert((alignof(UnwindCursor<A, R>) <= alignof(unw_cursor_t)),
+                "UnwindCursor<> requires more alignment than unw_cursor_t");
   memset(&_info, 0, sizeof(_info));
   memset(&_histTable, 0, sizeof(_histTable));
   _dispContext.ContextRecord = &_msContext;
   _dispContext.HistoryTable = &_histTable;
   // Initialize MS context from ours.
   R r(context);
   _msContext.ContextFlags = CONTEXT_CONTROL|CONTEXT_INTEGER|CONTEXT_FLOATING_POINT;
 #if defined(_LIBUNWIND_TARGET_X86_64)

 
 
 template <typename A, typename R>
 UnwindCursor<A, R>::UnwindCursor(unw_context_t *context, A &as)
     : _addressSpace(as), _registers(context), _unwindInfoMissing(false),
       _isSignalFrame(false) {
   static_assert((check_fit<UnwindCursor<A, R>, unw_cursor_t>::does_fit),
                 "UnwindCursor<> does not fit in unw_cursor_t");
+  static_assert((alignof(UnwindCursor<A, R>) <= alignof(unw_cursor_t)),
+                "UnwindCursor<> requires more alignment than unw_cursor_t");
   memset(&_info, 0, sizeof(_info));
 }
 
 template <typename A, typename R>
 UnwindCursor<A, R>::UnwindCursor(A &as, void *)
     : _addressSpace(as), _unwindInfoMissing(false), _isSignalFrame(false) {
   memset(&_info, 0, sizeof(_info));
   // FIXME