Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

minusbat
Copy link

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.

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.
@davidchisnall
Copy link
Member

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?

@minusbat
Copy link
Author

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 :-)

@davidchisnall
Copy link
Member

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).

@minusbat
Copy link
Author

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
Copy link
Author

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 ?

@davidchisnall
Copy link
Member

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants