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

Remove Legacy ABI Compatibility #298

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

Remove Legacy ABI Compatibility #298

wants to merge 1 commit into from

Conversation

hmelder
Copy link
Collaborator

@hmelder hmelder commented Aug 26, 2024

We are carrying a lot of dead-weight just by supporting the old GCC ABI which probably no one uses in combination with libobjc2.

There are still lot more ugly hacks which can be removed.

Copy link
Member

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t there more to be deleted from the upgrade logic?

legacy.c Outdated Show resolved Hide resolved
@hmelder
Copy link
Collaborator Author

hmelder commented Aug 27, 2024

Isn’t there more to be deleted from the upgrade logic?

Yeah but I was not sure how much to delete. Do we want to leave the gsv1 upgrade logic in place or remove it as well?

@hmelder
Copy link
Collaborator Author

hmelder commented Aug 27, 2024

And this would fall under OLDABI_COMPAT right?

@davidchisnall
Copy link
Member

Isn’t there more to be deleted from the upgrade logic?

Yeah but I was not sure how much to delete. Do we want to leave the gsv1 upgrade logic in place or remove it as well?

I'm not sure. We currently don't support the new ABI on Mach-O in clang and it's quite useful to be able to run tests on macOS so that we can compare their behaviour against the Apple runtime. I've not tried WebAssembly, but it probably also needs some custom work.

The main reason I'd drop the GCC ABI is that anyone using GCC should probably use their runtime. Ideally we'd get every target supported by the v1 GNUstep ABI onto v2 and then drop v1 as well.

@hmelder
Copy link
Collaborator Author

hmelder commented Aug 28, 2024

We currently don't support the new ABI on Mach-O in clang

I did not know that we support v1 on Macho-O. It would be very interesting to support v2 as well just for benchmarking purposes.

@hmelder
Copy link
Collaborator Author

hmelder commented Aug 28, 2024

I'll remove the GCC upgrade logic then.

@hmelder
Copy link
Collaborator Author

hmelder commented Sep 8, 2024

Google recently updated the clang prebuilds for the Android SDK. The fast path test now executes and fails instead of being skipped.

This commit removes the GCC ABI (gcc_abi = 8 in abi_version.c) including
the class and protocol upgrade logic and other quirks related
to the GCC ABI.

This should speed up selector type comparison significantly.
@hmelder
Copy link
Collaborator Author

hmelder commented Nov 12, 2024

@davidchisnall can you take another look at it? I've rebased it onto the latest changes in master.

Comment on lines -297 to -300
// We can't use all of the values in the type encoding for the hash,
// because our equality test is a bit more complex than simple string
// encoding (for example, * and ^C have to be considered equivalent, since
// they are both used as encodings for C strings in different situations)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure clang never does this? I vaguely remember things like char* and [un]signed char* encoded differently.

Comment on lines -305 to -310
switch (c)
{
case '@': case 'i': case 'I': case 'l': case 'L':
case 'q': case 'Q': case 's': case 'S':
hash = hash * 33 + c;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this change is correct with respect to structs and so on.

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