Page MenuHomePhabricator

[libc++abi] Fix forced_unwind tests failures on ARM/EHABI targets.
ClosedPublic

Authored by vvereschaka on Feb 9 2021, 3:25 PM.

Details

Summary

Added __cxxabi_config.h includes to resolve _LIBCXXABI_ARM_EHABI and proper building the forces_unwindX.cpp tests for the ARM/EHABI targets.

Diff Detail

Event Timeline

vvereschaka created this revision.Feb 9 2021, 3:25 PM
vvereschaka requested review of this revision.Feb 9 2021, 3:25 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 9 2021, 3:25 PM

Why is #include <cxxabi.h> needed?

vvereschaka edited the summary of this revision. (Show Details)

You are right, we don't need it. I did a copy/paste from another test.
Updated diff without cxxabi.h.

MaskRay accepted this revision.Feb 10 2021, 10:24 AM

The int main() {} change is unnecessary but I don't know the libc++ specific code style well.

You still need an owner's approval.

ldionne accepted this revision.Feb 10 2021, 10:26 AM
ldionne added a subscriber: ldionne.

The int main() {} change is unnecessary but I don't know the libc++ specific code style well.

You still need an owner's approval.

LGTM except the nitpick about how main() is declared. When testing under freestanding mode, main isn't considered special. It needs to be declared as int main(int, char**) so it mangles the right way and we can call it explicitly from the test harness. It's kind of nitpicky.

libcxxabi/test/forced_unwind1.pass.cpp
21

Can you use int main(int, char**)?

This revision is now accepted and ready to land.Feb 10 2021, 10:26 AM
vvereschaka added inline comments.Feb 10 2021, 10:34 AM
libcxxabi/test/forced_unwind1.pass.cpp
21

yes, I'll update it accordingly. Should we do the same for main() of the original test? I can update it there either.

vvereschaka marked an inline comment as not done.Feb 10 2021, 10:35 AM

Updated main() -> main(int, char**) for the tests.

vvereschaka marked an inline comment as done.Feb 11 2021, 10:08 AM

@ldionne would you take a look at the updated diff?

ldionne accepted this revision.Feb 12 2021, 1:34 PM

Still LGTM, my comment was a nitpick. Thanks :-)