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

Refactor and correct the "is pure" and "can raise" (port upstream PR#10354 and PR#10387) #555

Merged
merged 9 commits into from
Apr 15, 2022
Prev Previous commit
Next Next commit
Fix handling of exception-raising specific operations during spilling
The ARM and ARM64 architectures have `Ishiftcheckbound` specific instructions
that can raise an Invalid_argument exception.  This exception can, in turn,
be handled in the same function (see PR#10339 for an example).

However, these specific instructions were not taken into account
during insertion of spill code in asmcomp/spill.ml.  The consequence
is a variable that is reloaded from stack in the exception handler,
yet has not been spilled to stack before.

This commit fixes the issue by using the recently-fixed
`Mach.operation_can_raise` predicate to handle operations during spill
code insertion, instead of an ad-hoc pattern-matching.

Fixes: PR#10339
  • Loading branch information
xavierleroy authored and gretay-js committed Apr 15, 2022
commit a60671ca1916bd90c3f6d0bb393503dfed321df6
12 changes: 4 additions & 8 deletions backend/spill.ml
Original file line number Diff line number Diff line change
Expand Up @@ -494,17 +494,13 @@ let rec spill :
let before1 = Reg.diff_set_array after i.res in
k (instr_cons i.desc i.arg i.res new_next)
(Reg.add_set_array before1 i.res))
| Iop _ ->
| Iop op ->
spill env i.next finally (fun new_next after ->
let before1 = Reg.diff_set_array after i.res in
let before =
match i.desc with
Iop Icall_ind | Iop(Icall_imm _) | Iop(Iextcall _) | Iop(Ialloc _)
| Iop(Iintop Icheckbound) | Iop(Iintop_imm(Icheckbound, _))
| Iop (Iprobe _) ->
Reg.Set.union before1 env.at_raise
| _ ->
before1 in
if operation_can_raise op
then Reg.Set.union before1 env.at_raise
else before1 in
k (instr_cons_debug i.desc i.arg i.res i.dbg
(add_spills env (Reg.inter_set_array after i.res) new_next))
before)
Expand Down
12 changes: 4 additions & 8 deletions ocaml/asmcomp/spill.ml
Original file line number Diff line number Diff line change
Expand Up @@ -301,17 +301,13 @@ let rec spill i finally =
let before1 = Reg.diff_set_array after i.res in
(instr_cons i.desc i.arg i.res new_next,
Reg.add_set_array before1 i.res)
| Iop _ ->
| Iop op ->
let (new_next, after) = spill i.next finally in
let before1 = Reg.diff_set_array after i.res in
let before =
match i.desc with
Iop(Icall_ind) | Iop(Icall_imm _) | Iop(Iextcall _) | Iop(Ialloc _)
| Iop (Iprobe _)
| Iop(Iintop (Icheckbound)) | Iop(Iintop_imm(Icheckbound, _)) ->
Reg.Set.union before1 !spill_at_raise
| _ ->
before1 in
if operation_can_raise op
then Reg.Set.union before1 !spill_at_raise
else before1 in
(instr_cons_debug i.desc i.arg i.res i.dbg
(add_spills (Reg.inter_set_array after i.res) new_next),
before)
Expand Down
12 changes: 12 additions & 0 deletions ocaml/testsuite/tests/asmcomp/try_checkbound.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
(* TEST *)

(* See PR#10339 *)

let access (a: string array) n =
try
ignore (a.(n)); -1
with _ ->
n

let _ =
assert (access [||] 1 = 1)