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

Liveness information not rich enough for polling after Iextcall #352

Closed
dra27 opened this issue Jun 3, 2020 · 2 comments
Closed

Liveness information not rich enough for polling after Iextcall #352

dra27 opened this issue Jun 3, 2020 · 2 comments

Comments

@dra27
Copy link
Contributor

dra27 commented Jun 3, 2020

This issue is a brain-dump of investigations into a repeatable failure of tests/weak-ephe-final/weaklifetime.ml on mingw-w64. This test was repeatably failing with the ephemeron at index 259 changing length in the course of execution from 906 to 257 and so causing Weak.check to fail when asked to access index 358 of that ephemeron! @ctk21, @kayceesrk and I chased this down to the ephemeron not being marked and the page subsequently being freed and eventually nailed it down to the poll instruction inserted immediately after the extcall in Weak.create. Having then welcomed @stedolan to the scratching-our-heads-in-confusion mix, we're fairly sure that this isn't Windows-specific, but rather that this case is easier to hit on Windows because of the different ABI.

Brief conclusion: doing this correctly with the present safe points implementation requires knowledge that a hardware register contains an OCaml value (as opposed to just an integer). This information exists at clambda, but is dropped at CMM (but obviously could be propagated). It would then be possible to record in the frame_descriptor just prior to the poll that %rax (from the extcall) is a live value.

For now, the problem can be papered over on Windows by not allowing polling just prior to Istackoffset and the situation will hopefully be irrelevant if/when the analysis of safepoints moves from mach to lambda. The purpose of this issue is to ensure we don't forget that!

The gory details:

This is Weak.create:

external create : int -> 'a t = "caml_weak_create"
let create l =
  if not (0 <= l && l <= Obj.Ephemeron.max_ephe_length) then
    invalid_arg("Weak.create");
  create l

which compiles to:

	.text
	.align	16
	.quad	0
	.quad	1019
	.globl	camlWeak__create_85
camlWeak__create_85:
	leaq	-224(%rsp), %r10
	cmpq	32(%r14), %r10
	jb	.L106
.L105:
	subq	$8, %rsp
.L104:
	movq	%rax, (%rsp)
	cmpq	$1, %rax
	jl	.L103
	movabsq	$36028797018963961, %rbx
	cmpq	%rbx, %rax
	jle	.L102
.L103:
	movabsq	$camlWeak__1, %rax
	call	*__caml_imp_camlStdlib__invalid_arg_9(%rip)
.L100:
.L102:
	subq	$32, %rsp
	movq	32(%rsp), %rcx
	movabsq	$caml_weak_create, %rax
	call	*__caml_imp_caml_c_call(%rip)
.L101:
	cmpq	(%r14), %r15
	jb	.L107
.L108:
	addq	$32, %rsp
	addq	$8, %rsp
	ret
.L107:
	call	*__caml_imp_caml_call_poll(%rip)
.L109:
	jmp	.L108
.L106:
	push	$25
	call	*__caml_imp_caml_call_realloc_stack(%rip)
	popq	%r10
	jmp	.L105

note also the relevant part of the frame table, which contains no entries (but the correct stack):

	.quad	.L109
	.word	48
	.word	0
	.align	8

which is consistent with the liveness info computed just prior to the poll:

  {spilled-l/33*}
  offset stack 32
  { + spilled-l/33}
  l/34 := spilled-l/33 (reload)
  { + l/34}
  R/5[%rcx] := l/34
  { + R/5[%rcx]}
  R/0[%rax] := extcall "caml_weak_create" R/5[%rcx]{weak.ml:31,2-10}
  {R/0[%rax]}
  poll
  {R/0[%rax]}
  offset stack -32
  { + R/0[%rax]}
  V/32 := R/0[%rax]
  { + V/32}
  R/0[%rax] := V/32
  { + R/0[%rax]}
  return R/0[%rax]

(note that if %rax contained a value, it would be marked with an *, but this is not possible since at present Mach treats %rax as always an integer). Below the poll is the V/32 assignment to a virtual register which is the normal mechanism (and which gets optimised out), but is happening too late.

@kayceesrk
Copy link
Contributor

This issue should no longer be there since a new implementation of safe points got merged into trunk: ocaml/ocaml#10039 and multicore: #573.

@dra27
Copy link
Contributor Author

dra27 commented Oct 29, 2021

Yes, indeed - and the commit hacking around it got removed when #351 was rebased over it!

@dra27 dra27 closed this as completed Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants