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 typo in arm64 assembler directives #1150

Merged
merged 2 commits into from Apr 15, 2017

Conversation

Projects
None yet
5 participants
@kayceesrk
Contributor

kayceesrk commented Apr 14, 2017

PR fixes a minor typo in the function epilogue of caml_alloc3 in the arm64 backend.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Apr 14, 2017

Contributor

FWIW, is there a possible test case (or cases) for this? I think it's worth a Changes entry too?

Contributor

dra27 commented Apr 14, 2017

FWIW, is there a possible test case (or cases) for this? I think it's worth a Changes entry too?

@kayceesrk

This comment has been minimized.

Show comment
Hide comment
@kayceesrk

kayceesrk Apr 15, 2017

Contributor

Added a changes entry. Not aware of a way to test the correctness of these directives. I'm open to suggestions.

Contributor

kayceesrk commented Apr 15, 2017

Added a changes entry. Not aware of a way to test the correctness of these directives. I'm open to suggestions.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Apr 15, 2017

Contributor

I agree - is this code which retries allocation after running the Gc?

Contributor

dra27 commented Apr 15, 2017

I agree - is this code which retries allocation after running the Gc?

@kayceesrk

This comment has been minimized.

Show comment
Hide comment
@kayceesrk

kayceesrk Apr 15, 2017

Contributor

caml_alloc3 is the out-of-line code for allocating 3 word objects implemented in assembly used when -compact is enabled.

Interestingly, this particular typo would have generated correct symbol table info since the .size and .type for caml_alloc3 happens to be the same as caml_alloc2.

Contributor

kayceesrk commented Apr 15, 2017

caml_alloc3 is the out-of-line code for allocating 3 word objects implemented in assembly used when -compact is enabled.

Interestingly, this particular typo would have generated correct symbol table info since the .size and .type for caml_alloc3 happens to be the same as caml_alloc2.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Apr 15, 2017

Contributor

Ah, I see! It looks fine to me.

Contributor

dra27 commented Apr 15, 2017

Ah, I see! It looks fine to me.

@dra27 dra27 merged commit 27fccad into ocaml:trunk Apr 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Apr 15, 2017

Contributor

@damiendoligez - 4.05?

Contributor

dra27 commented Apr 15, 2017

@damiendoligez - 4.05?

@kayceesrk

This comment has been minimized.

Show comment
Hide comment
@kayceesrk

kayceesrk Apr 15, 2017

Contributor

Thanks @dra27.

Contributor

kayceesrk commented Apr 15, 2017

Thanks @dra27.

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Apr 19, 2017

Member

It would be useful to cherrypick this one into 4.05 if possible -- I'm about to activate some aarch64 tests on opam-repository.

Member

avsm commented Apr 19, 2017

It would be useful to cherrypick this one into 4.05 if possible -- I'm about to activate some aarch64 tests on opam-repository.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Apr 19, 2017

Contributor

I'm in favor of the cherry-picking. Nonetheless, I think this mistake on .type and .size directives does not prevent the code from running correctly: the most damage it can do is break debuggers, disassemblers, stack unwinders, and other tools that rely on "meta" information about the code. So, aarch64 testing should be effective (and will be very welcome!) even before the cherry-pick.

Contributor

xavierleroy commented Apr 19, 2017

I'm in favor of the cherry-picking. Nonetheless, I think this mistake on .type and .size directives does not prevent the code from running correctly: the most damage it can do is break debuggers, disassemblers, stack unwinders, and other tools that rely on "meta" information about the code. So, aarch64 testing should be effective (and will be very welcome!) even before the cherry-pick.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez May 29, 2017

Member

Cherry-picked to 4.05 (b4287d9).

Member

damiendoligez commented May 29, 2017

Cherry-picked to 4.05 (b4287d9).

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