[libcxx-commits] [PATCH] D72543: [libcxxabi] Insert padding in __cxa_exception struct for compatibility

John McCall via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jan 20 18:16:05 PST 2020


rjmccall added a comment.

In D72543#1829870 <https://reviews.llvm.org/D72543#1829870>, @steven_wu wrote:

> In D72543#1825496 <https://reviews.llvm.org/D72543#1825496>, @smeenai wrote:
>
> > In D72543#1825454 <https://reviews.llvm.org/D72543#1825454>, @rjmccall wrote:
> >
> > > In D72543#1825422 <https://reviews.llvm.org/D72543#1825422>, @smeenai wrote:
> > >
> > > > In D72543#1825178 <https://reviews.llvm.org/D72543#1825178>, @rjmccall wrote:
> > > >
> > > > > In D72543#1825150 <https://reviews.llvm.org/D72543#1825150>, @smeenai wrote:
> > > > >
> > > > > > From spelunking through history, it looks like D33030 <https://reviews.llvm.org/D33030> tried to solve the same problem, and then rL319123 <https://reviews.llvm.org/rL319123> undid that solution and came up with a different one to solve the same underlying issue. rL319123 <https://reviews.llvm.org/rL319123> is still in place; does it need to be adjusted for the change in this diff?
> > > > >
> > > > >
> > > > > It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the `_Unwind_Exception` type itself.  rL319123 <https://reviews.llvm.org/rL319123> became inadequate after `_Unwind_Exception` became an aligned type because that change still affected the layout of the C++ exception types.  What we're fixing here is essentially the same problem that was introduced by D33030 <https://reviews.llvm.org/D33030>.
> > > >
> > > >
> > > > Makes sense. Should rL319123 <https://reviews.llvm.org/rL319123> be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.
> > >
> > >
> > > If `__cxa_exception` is already sufficiently aligned, I believe that patch does not allocate any extra space.
> >
> >
> > You're right; I understand the case rL319123 <https://reviews.llvm.org/rL319123> was intended for now.
>
>
> Sorry I was on vacation. I intended to keep rL319123 <https://reviews.llvm.org/rL319123> as least for now. The current setup for MacOS is that clang header is setup to use unwind.h from SDK, which it will currently find the unaligned exception struct. After this patch, rL319123 <https://reviews.llvm.org/rL319123> is a no-op for people building libcxxabi against TOT llvm libunwind but it will help people building against the old header to get the correct alignment fro exception struct.
>
> @rjmccall are you suggesting to put the static_assert directly in the header so users can get build failures if they expect different layout? That can help but I need to figure out what is the correct layout different users are expecting.


That makes sense.

> Since the unwind change is a while back, is there any system that picked up the `intermediate` layout and expected to keep compatibility with that layout?

I wouldn't think so, but of course I can't know for certain.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72543/new/

https://reviews.llvm.org/D72543





More information about the libcxx-commits mailing list