Workaround bug in test ObjCXXEHInterop_arc

[weliveindetail]

The test failure appears to origin from a Clang bug that is related to the CodeGen passes PreISelIntrinsicLowering and WinEHPrepare. In simple cases like this test, the workaround is effective. In complex cases it can cause a crash in Clang further down the line. Would it make sense to pull it into upstream libobjc2 until we have a fix in mainline Clang and a release that contains it?

The --disable-cleanups flag used here is not as general-purpose as it may sound. It's used in WinEHPrepare.cpp exclusively and only triggers a single branch.

Happy to talk about an actual fix in more detail when time permits!

[weliveindetail]

This aims to workaround the bug tracked in https://github.com/gnustep/libobjc2/issues/222

[weliveindetail]

The bot failed before running tests in build step:

[112/229] Building C object Test\CMakeFiles\PropertyIntrospectionTest2_arc_optimised.dir\PropertyIntrospectionTest2_arc.m.obj

https://dev.azure.com/gnustep/libobjc2/_build/results?buildId=507&view=logs&j=d0cab528-cccd-5180-b7d9-4b8b4ac0b9c3&t=f671b46d-dd84-5da8-a465-97dc8caf7fa9&l=618

[davidchisnall]

The build failure is because newer versions of VS install a non-functional stdatomic.h. These tests should work fine if stdatomic.h doesn't exist. Can disable the attempt to use stdatomic.h on Windows for now?

[weliveindetail]

Hi David, thanks for your quick reply. Right, that seems to be the issue. In fact PropertyIntrospectionTest2_arc is the only test that uses stdatomic.h directly! I dropped the include locally and the test won't compile, because it does use atomic_bool in a few places.

For the sake of getting the bot to run the ObjCXXEHInterop_arc test at least once, do you think it's acceptable to disable PropertyIntrospectionTest2_arc temporarily?

I'd propose to add a commit for that and revert it afterwards. Then a squashing merge could eliminate the two right? What do you think?

[davidchisnall]

Can you guard the #include and the use of atomic_bool with a Windows-specific macro so that we can skip the part of the test but keep running the rest?

[weliveindetail]

The remaining tests passed:

2022-04-28T12:08:53.9584777Z 85/92 Test #87: ObjCXXEHInterop ..........................   Passed    0.05 sec
2022-04-28T12:08:53.9594245Z       Start 91: ObjCXXEHInterop_arc
2022-04-28T12:08:53.9600823Z 86/92 Test #89: ObjCXXEHInteropTwice .....................   Passed    0.04 sec
2022-04-28T12:08:53.9608378Z       Start 92: ObjCXXEHInterop_arc_optimised
2022-04-28T12:08:53.9609288Z 87/92 Test #78: hash_test_optimised ......................   Passed    2.91 sec
2022-04-28T12:08:53.9610161Z 88/92 Test #92: ObjCXXEHInterop_arc_optimised ............   Passed    0.02 sec
2022-04-28T12:08:53.9610852Z 89/92 Test #90: ObjCXXEHInteropTwice_optimised ...........   Passed    0.03 sec
2022-04-28T12:08:53.9611564Z       Start 31: ManyManySelectors
2022-04-28T12:08:53.9672148Z 90/92 Test #91: ObjCXXEHInterop_arc ......................   Passed    0.03 sec
2022-04-28T12:08:56.4648789Z 91/92 Test #31: ManyManySelectors ........................   Passed    2.50 sec
2022-04-28T12:08:56.4651123Z       Start 32: ManyManySelectors_optimised
2022-04-28T12:08:58.8975918Z 92/92 Test #32: ManyManySelectors_optimised ..............   Passed    2.43 sec
2022-04-28T12:08:58.8989639Z 
2022-04-28T12:08:58.8990467Z 100% tests passed, 0 tests failed out of 92

So the workaround is effective for ObjCXXEHInterop_arc, which is what this PR is about.

[weliveindetail]

Can you guard the #include and the use of atomic_bool with a Windows-specific macro so that we can skip the part of the test but keep running the rest?

Ok, I guess the plan would be: guard the respective #includes, make a separate PR, merge it, rebase this one on top (dropping the 2 temporary commits) and let the bot rerun. IIUC this would be a fix for https://github.com/gnustep/libobjc2/commit/7dee32ad1e977ddf085aff70f0f0b1ffc3524ee6 Let me see when I find the time for it.

[weliveindetail]

Hi David, sorry for the late follow-up. Actually, this PR is outdated. Instead of using the workaround, we should enable this test for Clang versions 15+. Let me put together a new PR for that.