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 Istore_symbol (plus some Win64 fixes) #955

Merged
merged 8 commits into from Dec 27, 2016

Conversation

Projects
None yet
4 participants
@mshinwell
Contributor

mshinwell commented Dec 8, 2016

This used to be MPR#6594, where the original poster describes assembly errors resulting from the use of an instruction with an out-of-range operand, namely attempting to move a 64-bit immediate (only 32 bits are allowed) into a 64-bit memory address. The instruction is believed to be generated by Istore_symbol. This patch disables that pattern for Win64. On other systems, when not compiling for PIC (which is probably not that common anyway), it would be highly unusual to exceed the 4Gb barrier that would trigger this.

@oandrieu Would you please test this?

@mshinwell mshinwell added the bug label Dec 8, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 8, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 8, 2016

Contributor

This doesn't seem to be working - it fails with set.ml instead. I wondered if the equivalent place for the Arch.win64 in amd64/reload.ml was the problem, but then it fails at complex.ml

At which point I'm getting out of my depth - but happy to help debugging further. AppVeyor seems very unhappy with the generated code, though I haven't looked in detail as to why...

Contributor

dra27 commented Dec 8, 2016

This doesn't seem to be working - it fails with set.ml instead. I wondered if the equivalent place for the Arch.win64 in amd64/reload.ml was the problem, but then it fails at complex.ml

At which point I'm getting out of my depth - but happy to help debugging further. AppVeyor seems very unhappy with the generated code, though I haven't looked in detail as to why...

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 8, 2016

Contributor

By the way - did we only just enable the "Reviewers" feature today, or was GitHub listening to the dev meeting on Tuesday and psychically added a new feature?

Contributor

dra27 commented Dec 8, 2016

By the way - did we only just enable the "Reviewers" feature today, or was GitHub listening to the dev meeting on Tuesday and psychically added a new feature?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 8, 2016

Contributor

OK, out of my depth, but still trying! This seems to fix things. I haven't fully confirmed the testsuite, because 4.04 changes aren't yet on trunk (something I may do something about later...) but how's this:

diff --git a/asmcomp/amd64/emit.mlp b/asmcomp/amd64/emit.mlp
index 85b4cee31..5b69920f3 100644
--- a/asmcomp/amd64/emit.mlp
+++ b/asmcomp/amd64/emit.mlp
@@ -992,7 +992,7 @@ let begin_assembly() =
   end;


-  if !Clflags.dlcode then begin
+  if !Clflags.dlcode || true then begin
     (* from amd64.S; could emit these constants on demand *)
     begin match system with
     | S_macosx -> D.section ["__TEXT";"__literal16"] None ["16byte_literals"]
diff --git a/asmcomp/amd64/reload.ml b/asmcomp/amd64/reload.ml
index 2e29de4c1..690e01651 100644
--- a/asmcomp/amd64/reload.ml
+++ b/asmcomp/amd64/reload.ml
@@ -94,7 +94,7 @@ method! reload_operation op arg res =
       then (arg, res)
       else super#reload_operation op arg res
   | Iconst_symbol _ ->
-      if !Clflags.pic_code || !Clflags.dlcode
+      if !Clflags.pic_code || !Clflags.dlcode || Arch.win64
       then super#reload_operation op arg res
       else (arg, res)
   | _ -> (* Other operations: all args and results in registers *)

I imagine that my || true should possibly be || win64 but I'm wondering if Cflags.pic_code should be in there too? Basically, I don't really understand why it works 😄

Contributor

dra27 commented Dec 8, 2016

OK, out of my depth, but still trying! This seems to fix things. I haven't fully confirmed the testsuite, because 4.04 changes aren't yet on trunk (something I may do something about later...) but how's this:

diff --git a/asmcomp/amd64/emit.mlp b/asmcomp/amd64/emit.mlp
index 85b4cee31..5b69920f3 100644
--- a/asmcomp/amd64/emit.mlp
+++ b/asmcomp/amd64/emit.mlp
@@ -992,7 +992,7 @@ let begin_assembly() =
   end;


-  if !Clflags.dlcode then begin
+  if !Clflags.dlcode || true then begin
     (* from amd64.S; could emit these constants on demand *)
     begin match system with
     | S_macosx -> D.section ["__TEXT";"__literal16"] None ["16byte_literals"]
diff --git a/asmcomp/amd64/reload.ml b/asmcomp/amd64/reload.ml
index 2e29de4c1..690e01651 100644
--- a/asmcomp/amd64/reload.ml
+++ b/asmcomp/amd64/reload.ml
@@ -94,7 +94,7 @@ method! reload_operation op arg res =
       then (arg, res)
       else super#reload_operation op arg res
   | Iconst_symbol _ ->
-      if !Clflags.pic_code || !Clflags.dlcode
+      if !Clflags.pic_code || !Clflags.dlcode || Arch.win64
       then super#reload_operation op arg res
       else (arg, res)
   | _ -> (* Other operations: all args and results in registers *)

I imagine that my || true should possibly be || win64 but I'm wondering if Cflags.pic_code should be in there too? Basically, I don't really understand why it works 😄

@oandrieu

This comment has been minimized.

Show comment
Hide comment
@oandrieu

oandrieu Dec 9, 2016

Contributor

@mshinwell I still get the error with your patch.

Same observations as @dra27 :

  • Iconst_symbol triggers the error, fixed in reload.ml
  • caml_negf_mask also, fixed Emit.begin_assembly

Then it seems to work !

Contributor

oandrieu commented Dec 9, 2016

@mshinwell I still get the error with your patch.

Same observations as @dra27 :

  • Iconst_symbol triggers the error, fixed in reload.ml
  • caml_negf_mask also, fixed Emit.begin_assembly

Then it seems to work !

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

@dra27 @oandrieu I've incorporated those changes; perhaps you could try again. Can you try building something large to see if any further errors of this form occur?

Contributor

mshinwell commented Dec 9, 2016

@dra27 @oandrieu I've incorporated those changes; perhaps you could try again. Can you try building something large to see if any further errors of this form occur?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 9, 2016

Contributor

Will do - but not until tomorrow.

Contributor

dra27 commented Dec 9, 2016

Will do - but not until tomorrow.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

All things considered, I'd rather remove the Iconst_symbol specific operation altogether, because 1- it is rarely usable (only in completely static code), and 2- it assumes that static symbols are in the low 4Gb of the addressing space, an assumption that currently holds in Linux and perhaps MacOS X as well, but could be invalidated at any time.

Sometimes removing code is the best solution..

Contributor

xavierleroy commented Dec 9, 2016

All things considered, I'd rather remove the Iconst_symbol specific operation altogether, because 1- it is rarely usable (only in completely static code), and 2- it assumes that static symbols are in the low 4Gb of the addressing space, an assumption that currently holds in Linux and perhaps MacOS X as well, but could be invalidated at any time.

Sometimes removing code is the best solution..

@oandrieu

This comment has been minimized.

Show comment
Hide comment
@oandrieu

oandrieu Dec 9, 2016

Contributor

@mshinwell I recompiled my not-so small application that exhibited the error in the first place — works fine now.

@xavierleroy you mean Istore_symbol (rather than Iconst_symbol) ?

Contributor

oandrieu commented Dec 9, 2016

@mshinwell I recompiled my not-so small application that exhibited the error in the first place — works fine now.

@xavierleroy you mean Istore_symbol (rather than Iconst_symbol) ?

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

@xavierleroy I presume you meant Istore_symbol, which I've now removed.

I don't yet understand why the changes to Iconst_symbol in Reload and the emitter are required. @dra27 @oandrieu Can you confirm those are still needed (I think they will be)?

Contributor

mshinwell commented Dec 9, 2016

@xavierleroy I presume you meant Istore_symbol, which I've now removed.

I don't yet understand why the changes to Iconst_symbol in Reload and the emitter are required. @dra27 @oandrieu Can you confirm those are still needed (I think they will be)?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

I meant Istore_symbol, as you correctly guessed. Sorry about that.

The change in Reload for Iconst_symbol forces the result to be a register (and not a stack slot) in more cases. This is safe, and necessary in cases where Iconst_symbol is turned into a movabsq or into a PC-relative load. I'm not sure what went wrong before, but in any case the change looks sound to me.

Contributor

xavierleroy commented Dec 9, 2016

I meant Istore_symbol, as you correctly guessed. Sorry about that.

The change in Reload for Iconst_symbol forces the result to be a register (and not a stack slot) in more cases. This is safe, and necessary in cases where Iconst_symbol is turned into a movabsq or into a PC-relative load. I'm not sure what went wrong before, but in any case the change looks sound to me.

@mshinwell mshinwell changed the title from Don't use Istore_symbol on x86-64/Win64 platforms to Remove Istore_symbol (plus some Win64 fixes) Dec 9, 2016

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

The latest version (sans Istore_symbol) looks good to me.

Contributor

xavierleroy commented Dec 9, 2016

The latest version (sans Istore_symbol) looks good to me.

@xavierleroy xavierleroy added the approved label Dec 9, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

I'll wait for David / Olivier to try it once more before merging.

Contributor

mshinwell commented Dec 9, 2016

I'll wait for David / Olivier to try it once more before merging.

@oandrieu

This comment has been minimized.

Show comment
Hide comment
@oandrieu

oandrieu Dec 9, 2016

Contributor

Still works for me, thx.

Contributor

oandrieu commented Dec 9, 2016

Still works for me, thx.

dra27 added a commit to dra27/ocaml that referenced this pull request Dec 15, 2016

@mshinwell mshinwell merged commit f206344 into ocaml:trunk Dec 27, 2016

2 checks passed

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

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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