PR#7405: s390x: Fix address of caml_raise_exn in native dynlink modules. #903

Merged
merged 1 commit into from Nov 11, 2016

Projects

None yet

3 participants

@gasche
Member
gasche commented Nov 9, 2016

See PR#7405

This commit comes from Fedora patch e732c39340e86939530a087744caa8d8f1247878.

asmcomp/s390x/emit.mlp
@@ -611,7 +611,7 @@ let emit_instr i =
| Lraise k ->
begin match k with
| Cmm.Raise_withtrace ->
- ` brasl %r14, {emit_symbol "caml_raise_exn"}\n`;
+ ` brasl %r14, {emit_symbol "caml_raise_exn"}@PLT\n`;
@xavierleroy
xavierleroy Nov 10, 2016 Contributor

I think this is correct (using @PLT whether pic_code is requested or not), but it would be more consistent with the remainder of this file to do emit_call "caml_raise_exn".

@gasche
Member
gasche commented Nov 10, 2016

I updated the patch -- it also matches the way caml_raise_exn is handled by other architectures.

@rwmjones
Contributor

I think the updated version is missing a \n or something. The generated asm looks like this:

        lgr     %r2, %r5
        .loc    1       32
brasl   %r14, caml_raise_exn@PLT.L138:
.L136:  brasl   %r14, caml_call_gc@PLT
.L137:  brcl    15, .L135
        .globl  camlPervasives__invalid_arg_1007

and that causes errors like:

/tmp/camlasmc1decc.s: Assembler messages:
/tmp/camlasmc1decc.s:408: Error: junk at end of line: `.L138:'
/tmp/camlasmc1decc.s:435: Error: junk at end of line: `.L143:'
/tmp/camlasmc1decc.s:1286: Error: junk at end of line: `.L251:'
/tmp/camlasmc1decc.s:1476: Error: junk at end of line: `.L284:'
File "/home/rjones/d/fedora/fedora-ocaml/stdlib/pervasives.ml", line 1:
Error: Assembler error, input left in file /tmp/camlasmc1decc.s
Makefile.shared:105: recipe for target 'pervasives.cmx' failed
@rwmjones
Contributor

This fixes the immediate problems above. I am still testing it.

diff --git a/asmcomp/s390x/emit.mlp b/asmcomp/s390x/emit.mlp
index 5d233a3..f99380a 100644
--- a/asmcomp/s390x/emit.mlp
+++ b/asmcomp/s390x/emit.mlp
@@ -611,7 +611,7 @@ let emit_instr i =
     | Lraise k ->
         begin match k with
         | Cmm.Raise_withtrace ->
-          `    brasl   %r14, {emit_symbol "caml_raise_exn"}\n`;
+          `    {emit_call "caml_raise_exn"}\n`;
           let lbl = record_frame Reg.Set.empty true i.dbg in
           `{emit_label lbl}:\n`
         | Cmm.Raise_notrace ->
@rwmjones
Contributor

My updated version above seems to work fine with the simple test case from the Mantis bug.

@gasche
Member
gasche commented Nov 10, 2016

@rwmjones thanks, I will update the patch as you suggest.

It looks like most architectures either embed a newline in the emit_call definition (power) or have an abstracted assembly layer that does not require textual newline (i386, amd64). s390x and arm are the only one to require explicit newlines after emit_call, and it makes it a bit harder for non-expert to take inspiration from other architectures.

@xavierleroy, would you be interested in a second commit to make newline's handling by emit_call consistent across text-emitting architectures? What is the right choice, to have emit_call include a newline or not? (Other emit_{label,reg,...} functions don't include a newline, but they are expression-level producers while emit_call is a statement producer. So I would guess that emit_call should include a newline.)

@gasche
Member
gasche commented Nov 10, 2016

(Looking at the code some more, there are other small inconsistencies that one could consider fixing. For example, most architectures have a record_frame_label function that just returns a label, and a record_frame function that emits the label; but s390x only has a function that returns a label, and it is called record_frame.)

@rwmjones @gasche rwmjones PR#7405: s390x: Fix address of caml_raise_exn in native dynlink modules.
This commit started as Fedora patch e732c39340e86939530a087744caa8d8f1247878.
d6f24c5
@gasche
Member
gasche commented Nov 10, 2016

Looking at the code some more, I notice that when loading global adresses, the code generator is careful to use lgrl <reg>, <symbol>@GOTENT in PIC mode (as opposed to larl <reg>, <symbol>), but that the assembly code in s390.S just uses larl when calling caml_array_bound_error. Is this a bug?

@rwmjones
Contributor

Don't know if that was directed at me, but I don't know the answer. I can however test things on s/390 machines. Do you think that a native module which accesses an array out of bounds would be sufficient to check that? For more substantial answers you'll probably need to talk to the IBM folk who AIUI contributed this work originally.

@rwmjones
Contributor

Well FWIW both a native module which accesses an array member out of bounds, and a normal program doing the same, both throw

Invalid_argument("index out of bounds")

and nothing bad appears to happen.

@xavierleroy
Contributor

Apologies for my imprecise mention of emit_call, and thanks to all for your good will. It's just that we have a weird workflow, with Rich debugging and testing, Gabriel editing the code of the PR, and I driving from the backseat...

Concerning the direct symbol reference to caml_array_bound_error in s390.S, I think this is OK because the reference and its definition are both in libasmrun.a, hence will be statically linked at the same time in the same "linking unit". (By "linking unit", I mean the main executable, or a DLL.) If I understand the ELF linking model correctly, indirect @GOT and @PLT references are needed only for references that could cross linking unit boundaries, i.e. between a DLL and the main program.

Concerning variations between code emitters for various platforms: I'm glad that there are so few given that those emitters were written across a 20+ year period of time... There is no urgent need to make things more uniform across platforms as @gasche suggests: nothing is really broken, and changing code generators that have very few users (like ARM64 or S390x) can create nasty bugs that we detect late.

I'm actually more concerned that in the S390x code emitter we still have places where brasl is used directly instead of emit_call. In other words, consistency within one code emitter is more important than consistency across emitters.

@gasche gasche merged commit ee06d24 into ocaml:trunk Nov 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche
Member
gasche commented Nov 11, 2016

I went ahead and merged.

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