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

amd64: align data section to word boundary #8751

Merged
merged 2 commits into from Jun 24, 2019
Merged

Conversation

@nojb
Copy link
Contributor

@nojb nojb commented Jun 19, 2019

We came across a case where compiling with mingw64 results in misaligned statically allocated constants in the data section. This patch fixes the issue. Incidentally, both arm64 (here) and power (here) explicitly align the data items to a word boundary.

Unfortunately, I haven't been able to come up with a small reproduction case.

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 19, 2019

For the record, something I do not understand is why we never come across this problem when compiling with msvc64.

@dra27
Copy link
Contributor

@dra27 dra27 commented Jun 19, 2019

I think (I can't find it properly stated anywhere) that MASM automatically aligns DATA segments to PARAGRAPH (so ALIGN 16) which means you wouldn't see the effect?

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 19, 2019

I think (I can't find it properly stated anywhere) that MASM automatically aligns DATA segments to PARAGRAPH (so ALIGN 16) which means you wouldn't see the effect?

Spot on! https://docs.microsoft.com/en-us/cpp/assembler/masm/segment?view=vs-2019 says that PARA is used by default.

If you agree, do you care approving the PR? I'll add a Changes entry.

@dra27
Copy link
Contributor

@dra27 dra27 commented Jun 19, 2019

Just to confirm - spotted on mingw64, but your larger fail case didn’t apply on msvc64, but did apply on amd64 on another arch?

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 19, 2019

Just to confirm - spotted on mingw64, but your larger fail case didn’t apply on msvc64, but did apply on amd64 on another arch?

No, just mingw64.

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 19, 2019

It just seems the right thing to do to emit the alignment for all amd64 backends.

@nojb nojb force-pushed the amd64_align_data_section branch from 1c85b7e to 4fca5e0 Jun 24, 2019
@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

I rewrote the Changes entry to be more accurate.

dra27
dra27 approved these changes Jun 24, 2019
@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

Thanks!

@nojb nojb merged commit 40cda4a into ocaml:trunk Jun 24, 2019
2 checks passed
nojb added a commit that referenced this issue Jun 24, 2019
amd64: align data section to word boundary

(cherry-picked from 40cda4a)
@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

4.09 aa70fea

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

Should this go to 4.08 ?

@nojb nojb deleted the amd64_align_data_section branch Jun 24, 2019
@gasche
Copy link
Member

@gasche gasche commented Jun 24, 2019

I see no harm in cherry-picking to 4.08. We don't currently have a bugfix-release planned, but you can never know, and some people may in the future want to use the head of the branch anyway.

nojb added a commit that referenced this issue Jun 24, 2019
amd64: align data section to word boundary

(cherry-picked from 40cda4a)
@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

OK, thanks! I added a new heading for a hypothetical 4.08.1 release: d665e78

@gasche
Copy link
Member

@gasche gasche commented Jun 24, 2019

For 4.06 (for example) we used:

4.06 maintenance branch:
------------------------

which I think is slightly better unless a release is actually planned.

@nojb
Copy link
Contributor Author

@nojb nojb commented Jun 24, 2019

Thanks, I fixed it as suggested.

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

Successfully merging this pull request may close these issues.

None yet

3 participants