Page MenuHomePhabricator

[libcxxabi] Fix exception address alignment test for EHABI
ClosedPublic

Authored by rmaprath on Mar 21 2017, 3:26 AM.

Details

Summary

This test was failing on our downstream (bare-metal) runs and only now I got around to investigate it.

The problem is, this test assumes the Itanium ABI, whereas EHABI requires the exception address to be 8-byte aligned.

I was a bit puzzled at first because this should've failed on the public arm-linux builder too. I think the reason it passes there is because we don't include libunwind headers in the include path when running the libcxxabi tests, so the system unwind.h gets picked up.

Diff Detail

Repository
rL LLVM

Event Timeline

rmaprath created this revision.Mar 21 2017, 3:26 AM
rmaprath updated this revision to Diff 92450.Mar 21 2017, 3:36 AM

Fixed a few paths.

rengolin edited edge metadata.Mar 21 2017, 3:54 AM

This looks like a simple oversight from my perspective, so looks good.

But I'll let the others have a look at it, as I'm not overly familiar with libcxxabi.

cheers,
--renato

EricWF edited edge metadata.Mar 27 2017, 7:39 PM

For the most part this LGTM other than the nits mentioned.

test/libcxxabi/test/config.py
34 ↗(On Diff #92450)

I think the correct default here is None, not ../libunwind/src, since we might not be using libunwind at all.

93 ↗(On Diff #92450)

I'm not sure if this is default is optimal. I think None might be less surprising.

test/lit.site.cfg.in
9 ↗(On Diff #92450)

I don't think we need the unwind sources anywhere within the test suite. I would remove this option all together and simply depend on libunwind_headers.

10 ↗(On Diff #92450)

I think libunwind_headers should be the empty string when LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL has not been found, instead of expanding to "LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL-NOTFOUND"

test/test_exception_address_alignment.pass.cpp
30 ↗(On Diff #92450)

Shouldn't this be an #ifdef?

rmaprath updated this revision to Diff 94074.Apr 4 2017, 7:13 AM

Addressed review comments from @EricWF .

This revision was automatically updated to reflect the committed changes.