libobjc2 icon indicating copy to clipboard operation
libobjc2 copied to clipboard

Refactor ABI check to allow the functions to execute in either order

Open minusbat opened this issue 6 years ago • 6 comments

As dynamic and static linking call the old and new load functions in the opposite order then the ABI mix check code needs to handle both cases. We take the approach of setting a flag if a (non Protocol) new class has been loaded and if any old class has been loaded, and at the end of each function we error if both flags are set to YES. This passes the basic check that a simple pieces of objectve C can be compiled and run both staticly and dynamicly using both the old and the new ABI.

minusbat avatar May 25 '19 18:05 minusbat

Wouldn't it be simpler to just set a flag for whether the Protocol class has been loaded and, if it hasn't, allow one thing to load with the wrong ABI, failing if it doesn't contain the Protocol class?

davidchisnall avatar May 26 '19 10:05 davidchisnall

Possibly, I started off trying something like that, but in the end just decided to take the lazy approach and set a couple of flags, one for each type. In the what needs to happen is that the Protocol module is exepted from the checks, regardless of load order, so whatever you think is easiest and clearest to achieve that. Am just happy it works :-)

minusbat avatar May 26 '19 20:05 minusbat

Would you mind having a go at making the test suite run with static linking? That would help catch a lot of potential bugs like this (add a _static variant for each test).

davidchisnall avatar May 28 '19 09:05 davidchisnall

Thats a good idea, yes. Not sure I will find time in the next few days, but will trying give it a go. Could this be merged in the meantime though ? My aim at the moment is to find and fix the specific coredumps I have found already (or at least log them as an issue). AM having a couple of crashes with protocols which I haven't got to the bottom of - one which occurs on a pure 2.0 ABI compile.

BTW, the tests don't all pass for me on freeBSD 12, and I notice some of them fail depending on the machine architecture chosen (if set arch to 'znver1' when some fail which don't with 'core2' which worries me a bit).

minusbat avatar May 29 '19 09:05 minusbat

So, I haven't had time to try and make static tests pass yet. Could this one be merged anyway, or do you want me to refactor it to be closer to your original code first ?

minusbat avatar Jun 28 '19 20:06 minusbat

Please can you refactor it to be closer to the original and make the test suite run with static linking? Until this is in CI, I'm not happy that it won't be broken again by the next commit...

davidchisnall avatar Jul 10 '19 17:07 davidchisnall