Page MenuHomePhabricator

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

Authored by mstorsjo on Aug 17 2020, 12:52 PM.

Details

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).

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 17 2020, 12:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 17 2020, 12:52 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Aug 17 2020, 12:52 PM
cdavis5x accepted this revision.Aug 17 2020, 4:48 PM

@compnerd What do you think about this one?

compnerd added inline comments.Aug 20 2020, 9:38 AM
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 added inline comments.Aug 20 2020, 12:21 PM
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 updated this revision to Diff 286874.Aug 20 2020, 12:22 PM

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

compnerd accepted this revision.Aug 21 2020, 8:50 AM
This revision is now accepted and ready to land.Aug 21 2020, 8:50 AM
mstorsjo added a subscriber: hans.Aug 22 2020, 1:11 PM

@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.

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),
      |   ^~~~~~~~~~~~~

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 added a comment.Aug 24 2020, 10:36 AM

@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.

@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!