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

Fix maximum structure alignment on Android i386 #1080

Merged

Conversation

BugsBeGone
Copy link
Contributor

Fixes #1079

@BugsBeGone BugsBeGone marked this pull request as ready for review March 30, 2019 15:59
@matthiasblaesing
Copy link
Member

I had a quick look. What I noticed so far:

General

  • Please make sure you work from a recent state of the repository, your fork is around four months old
  • Please make sure a valid author + email is in the commit. I won't merge commits from random aliases.

Changeset

Your title "Fix maximum structure alignment on Android i386" implies an android only - change. So I would suggest:

            MAX_ALIGNMENT = Platform.isSPARC() || Platform.isWindows()
                || (Platform.isLinux() && (Platform.isARM() || Platform.isPPC() || Platform.isMIPS()))
                || Platform.isAIX()
                || (Platform.isAndroid() && (! (Platform.isIntel() && !Platform.is64Bit()))
                ? 8 : LONG_SIZE;

Then a single test on i386 is enough to validate the change. What do you think?

@BugsBeGone
Copy link
Contributor Author

Thanks for looking into this. Sorry abut the old fork, I didn't have a clean master so spun off a branch from what I thought was the last upstream commit. It turns out I was still in the native subdir, hence the 4 month old branch.

Going to do some rebasing anyway to get the branch up to date, so I'll add the author info then. Does it matter if the commit itself is not verified, so long as the pull request has come from a GitHub account that is?

@matthiasblaesing
Copy link
Member

Thank you for the update. I managed to setup a minimal test in an android emulator and verified, that the alignment on x86 32Bit correctly changes from 8 to 4. This looks right to me.

One last question: Would it make sense to align linux and android? I would expect the ABI to be identical on both platforms? Currently linux is covered by explicitly including arm, ppc and mips (line 227), while android uses an exclude based pattern: not intel (line 229). Given that sparc is handled globally, the two might already be practically identical.

@BugsBeGone
Copy link
Contributor Author

The Linux and Android ABIs should be the same. However, the supported architectures for Android are more limited: so far only x86, ARM and MIPS (and MIPS is already deprecated). I doubt any other architectures will be supported on Android in the near future, but I wouldn't want to make the same assumption about Linux.

So I guess treating Android as a subset of Linux makes perfect sense here. I just wanted to get the smallest functional diff as possible using the exclude based pattern.

@matthiasblaesing
Copy link
Member

Ok - I'll merge as is. Thank you for taking care!

@matthiasblaesing matthiasblaesing merged commit 12c6a92 into java-native-access:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants