Skip to content

Jb/stack checks #55

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.

Sign up for GitHub

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

Open
jackbackrack wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Jb/stack checks #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eceb169
turn off stack check for small functions without non-tail-calls and a…
jackbackrack Feb 3, 2020
75d0466
cleanup code ready for pull request
jackbackrack Feb 5, 2020
f4c4a3a
merge master
jackbackrack Feb 6, 2020
14c1aa2
comment code
jackbackrack Feb 6, 2020
d73aa54
remove debug
jackbackrack Feb 6, 2020
5b2d6e1
move parameter to top, and remove old timer debug code
jackbackrack Feb 7, 2020
41d95cd
use SectionMarkerIns to improve robustness in stripping stack-check p…
jackbackrack Feb 7, 2020
97fecf8
get robust SectionMarkerIns working in bootstrapped compiler
jackbackrack Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/stz-compiler-main.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,4 +341,4 @@ public defstruct CompilationResult :
compiled-packages: Tuple<Symbol>
binding-packages: Tuple<Symbol>
package-stamps: Tuple<PackageStamp>
output-pkgs: Tuple<FileStamp>
output-pkgs: Tuple<FileStamp>
2 changes: 2 additions & 0 deletions compiler/stz-config.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ defsyntax stanza-config :
STANZA-MAX-COMPILER-HEAP-SIZE = sz
defrule entry = (experimental = ?b:#bool!) :
EXPERIMENTAL = b
defrule entry = (strip-stack-checks? = ?b:#bool!) :
STRIP-STACK-CHECKS? = b
fail-if entry = () :
PE(closest-info(), "Invalid configuration rule.")

Expand Down
1 change: 1 addition & 0 deletions compiler/stz-params.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public var OUTPUT-PLATFORM:Symbol = `platform
public var STANZA-PKG-DIRS:List<String> = List()
public val STANZA-PROJ-FILES = Vector<String>()
public var EXPERIMENTAL:True|False = false
public var STRIP-STACK-CHECKS?:True|False = false

;====== Compiler Configuration =====
public var STANZA-MAX-COMPILER-HEAP-SIZE = 4L * 1024L * 1024L * 1024L
Expand Down
61 changes: 49 additions & 12 deletions compiler/stz-reg-alloc.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ defpackage stz/reg-alloc :
import stz/basic-blocks with :
prefix(Block) => Basic

;============================================================
;=================== Parameters =============================
;============================================================

;Size of smap which can be called without checking stack-size.
val SMALL-SMAP-SIZE = 16

;============================================================
;===================== Driver ===============================
;============================================================
Expand Down Expand Up @@ -249,10 +256,10 @@ public defn allocate-registers (ins:VMFunction, emitter:CodeEmitter, backend:Bac
print-prog()

val smap = stack-map()
;if print? :
; println("==== Stack Map ====")
; println(smap)
;; if print? :
;; println("==== Stack Map ====")
;; println(smap)

collapse-blocks()
if print? :
println("==== Collapse Blocks ====")
Expand Down Expand Up @@ -356,6 +363,10 @@ public defstruct MethodDispatch <: Ins :
amb?: Int|False with: (default => false)
killed?: False|List<Int> with: (default => false, as-method => true)

public defstruct SectionMarker <: Ins :
marker: Symbol
killed?: False|List<Int> with: (default => false, as-method => true)

public defstruct Save <: Ins :
x: Var

Expand Down Expand Up @@ -616,6 +627,7 @@ defmethod print (o:OutputStream, i:Ins) :
(i:Label) : "label %_" % [n(i)]
(i:Goto) : "goto %_" % [n(i)]
(i:Break) : "break %_ when %_(%,)" % [n(i), op(i), xs(i)]
(i:SectionMarker) : "section: %~" % [marker(i)]

defmethod print (o:OutputStream, c:FnContext) :
print{o, _} $ match(c) :
Expand Down Expand Up @@ -655,6 +667,7 @@ defn do-defined (f: Var -> ?, e:Ins) :
(e:Load) : f(x(e))
(e:Match) : false
(e:MethodDispatch) : false
(e:SectionMarker) : false
false

defn do-used (gv: Var -> ?, e:Ins) :
Expand All @@ -671,6 +684,7 @@ defn do-used (gv: Var -> ?, e:Ins) :
(e:Load) : false
(e:Match) : do(g, xs(e))
(e:MethodDispatch) : false
(e:SectionMarker) : false
false

defn used-vars (e:Ins) :
Expand Down Expand Up @@ -716,6 +730,8 @@ defn reverse-sweep (e:Ins, define:Var -> ?, emit:Ins -> ?, use-var:Var -> ?) :
do(use, xs(e))
(e:MethodDispatch) :
emit(e)
(e:SectionMarker) :
emit(e)

defn attach-killed (e:Ins, ks:List<Int>) :
match(e) :
Expand All @@ -728,6 +744,7 @@ defn attach-killed (e:Ins, ks:List<Int>) :
(e:Branch) : Branch(op(e), xs(e), ks)
(e:Match) : Match(dispatch?(e), type-lists(e), xs(e), ks, ns?(e))
(e:MethodDispatch) : MethodDispatch(multi(e), num-header-args(e), default?(e), amb?(e), ks)
(e:SectionMarker) : SectionMarker(marker(e), ks)

;============================================================
;=================== Unique Labels ==========================
Expand Down Expand Up @@ -942,6 +959,8 @@ defn load-instructions (function:VMFunc, backend:Backend) :
push(Op(SaveCContextOp(), List(), List()))
(e:CommentIns) :
false
(e:SectionMarkerIns) :
push(SectionMarker(marker(e)))
(e:UnreachableIns) :
false
val next = to-list $
Expand Down Expand Up @@ -1805,6 +1824,8 @@ defn register-assignment (blk:Block, b:Int, backend:Backend) :
do(free-var, killed(e))
(e:MethodDispatch) :
emit(e)
(e:SectionMarker) :
emit(e)
(e:Op) :
defn assign-prefs (pref:List<Imm> -> Loc|List<Loc>) :
val ys* = map(annotate, ys(e))
Expand Down Expand Up @@ -2391,8 +2412,8 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :
;==============
;==== Body ====
;==============
for e in ins(BLOCKS[0]) do :
;println("//Assembling: %_" % [e])

defn emit (e:Ins) :
match(e) :
(e:Set) :
match(y(e)) :
Expand Down Expand Up @@ -2436,7 +2457,7 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :
(t:ExtendStack) :
val return-lbl = unique-id(stubs)
E $ SetL(R0, LM(return-lbl))
E $ SetL(R1, INT(size(stackmap) + 8))
E $ SetL(R1, INT(size(stackmap) + (8 + SMALL-SMAP-SIZE)))
Copy link
Copy Markdown
Collaborator

@CuppoJava CuppoJava Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the right logic, but before I can merge it I need to refresh my head about the details of the stack layout. It's tricky, and if we get it wrong, it manifests as random crashes every few weeks, so it's important we get it right on the first try.

To do that, I need to know what is the stack layout, what is enforced by this instruction, why that +8 is there, etc.

Copy link
Copy Markdown
Contributor Author

@jackbackrack jackbackrack Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the +8 is yours. i just added SMALL-SMAP-SIZE.

E $ asm-Goto(M(extend-stack(stubs)))
E $ asm-Label(return-lbl)
(t:CollectGarbage) :
Expand All @@ -2459,10 +2480,10 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :

;Save RSP
E $ StoreL(M(stack-pointer(stubs)), RSP)

;Restore the C context:
E $ LoadL(RSP, M(saved-c-rsp(stubs)))

;Call C function with right convention
val num-float-args = for y in ys(e) count : reg?(y as Var) is FReg
E $ SubL(RSP, RSP, INT(frame-size(num-mem-args(t), backend(stubs))))
Expand Down Expand Up @@ -2498,7 +2519,7 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :
E $ LoadL(TMP2, F, size-offset)
E $ AddL(TMP, TMP, TMP2)
E $ StoreL(M(stack-limit(stubs)), TMP)

E $ LoadL(TMP, F, pc-offset)
E $ asm-Goto(TMP)
E $ asm-Label(lbl, info(t))
Expand Down Expand Up @@ -2629,7 +2650,7 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :
defn cmp-stack-limit (le-op:asm-Op) :
val TMP = R0
val TMP2 = R1
E $ AddL(TMP, RSP, INT(size(stackmap) + 8))
E $ AddL(TMP, RSP, INT(size(stackmap) + (8 + SMALL-SMAP-SIZE)))
E $ LoadL(TMP2, M(stack-limit(stubs)))
E $ BreakL(label-table[n(e)], le-op, TMP, TMP2)
defn cmp-heap-limit (le-op:asm-Op) :
Expand Down Expand Up @@ -2681,6 +2702,22 @@ defn assemble (emitter:CodeEmitter, stackmap:StackMap, stubs:AsmStubs) :
val amb-branch = label-table[amb(e)]
E $ asm-MethodDispatch(multi(e), num-header-args(e), no-branch, amb-branch)
(e) : fatal("Not yet implemented: %_" % [e])

Copy link
Copy Markdown
Collaborator

@CuppoJava CuppoJava Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is too fragile. Let's introduce a new instruction called:

SectionMarker :
  name: Symbol

And have the preamble be surrounded by a pair of SectionMarker instructions. Then strip away everything in that section.

Copy link
Copy Markdown
Contributor Author

@jackbackrack jackbackrack Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

;When size(stackmap) <= SMALL-SMAP-SIZE and no non-tail-calls in body:
val stripping-preamble?:True|False = if size(stackmap) <= SMALL-SMAP-SIZE and STRIP-STACK-CHECKS? :
for e in ins(BLOCKS[0]) any? :
match(e:Call) : type(e) is-not StanzaTCall
;Strip ins between preamble SectionMarkerIns'.
var stripping?:True|False = false
for e in ins(BLOCKS[0]) do :
if stripping? :
match(e:SectionMarker) :
if marker(e) == `preamble: stripping? = false
else :
match(e:SectionMarker) :
stripping? = stripping-preamble? and (marker(e) == `preamble)
else :
emit(e)

defn asm-type (x:Imm) :
to-asm-type(type(x))
Expand Down Expand Up @@ -2757,4 +2794,4 @@ defn to-asm-op (op:VMOp) -> asm-Op :
;============================================================

defn keys<?T> (xs:Vector<KeyValue<?T,?>>) : to-list(seq(key,xs))
defn values<?T> (xs:Vector<KeyValue<?,?T>>) : to-list(seq(value,xs))
defn values<?T> (xs:Vector<KeyValue<?,?T>>) : to-list(seq(value,xs))
7 changes: 6 additions & 1 deletion compiler/stz-vm-ir.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,10 @@ public defstruct LiveIns <: VMIns :
xs: Tuple<VMImm>
public defstruct CommentIns <: VMIns :
message: String
public defstruct SectionMarkerIns <: VMIns :
marker: Symbol
public defstruct UnreachableIns <: VMIns


public deftype VMValue :
Char <: VMValue
Expand Down Expand Up @@ -676,6 +679,7 @@ defmethod print (o:OutputStream, i:VMIns) :
(i:RecordLiveIns) : P $ "live (%,)" % [live(i)]
(i:LiveIns) : P $ "mark-live (%,)" % [xs(i)]
(i:CommentIns) : P $ "comment: %~" % [message(i)]
(i:SectionMarkerIns) : P $ "section: %~" % [marker(i)]
(i:MatchIns) :
print(o, "match(%,) :" % [ys(i)])
val o2 = IndentedStream(o)
Expand Down Expand Up @@ -828,6 +832,7 @@ defsyntax vmcode :
defrule ins = (live (?xs:#local ...)) : RecordLiveIns(to-tuple(xs))
defrule ins = (mark-live (?xs:#imm! ...)) : LiveIns(to-tuple(xs))
defrule ins = (comment: ?x:#string!) : CommentIns(x)
defrule ins = (section: ?x:#symbol!) : SectionMarkerIns(x)
defrule ins = (match(?ys:#imm! ...) : (?bs:#vmbranch ... ?d:#defaultbranch)) : MatchIns(to-tuple(ys), to-tuple(bs), d)
defrule ins = (dispatch(?ys:#imm! ...) : (?bs:#vmbranch ... ?d:#defaultbranch ?a:#ambbranch)) :
DispatchIns(to-tuple(ys), to-tuple(bs), d, a)
Expand Down Expand Up @@ -1057,4 +1062,4 @@ public defn dump (p:VMPackage, suffix:String|False) :
dump(p, ".", suffix)

public defn dump (p:VMPackage) :
dump(p, false)
dump(p, false)
6 changes: 5 additions & 1 deletion compiler/stz-vm-normalize.stanza
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ defn normalize (f0:VMFunction, databuffer:DataBuffer, backend:Backend, iotable:I
;Check stack extension
val has-stack-lbl = make-label(buffer)
val no-stack-lbl = make-label(buffer)
load-instruction(SectionMarkerIns(`preamble))
load-instruction(Branch0Ins(has-stack-lbl, no-stack-lbl, HasStackOp()))
load-instruction(LabelIns(no-stack-lbl))
load-instruction(ExtendStackIns())
load-instruction(GotoIns(has-stack-lbl))
load-instruction(LabelIns(has-stack-lbl))
load-instruction(SectionMarkerIns(`preamble))

;Get arguments
val arg-rec = arg-extern-records(buffer, args(f), c-rsp-arg, backend)
Expand Down Expand Up @@ -133,11 +135,13 @@ defn normalize (f0:VMFunction, databuffer:DataBuffer, backend:Backend, iotable:I
;Check stack extension
val has-stack-lbl = make-label(buffer)
val no-stack-lbl = make-label(buffer)
load-instruction(SectionMarkerIns(`preamble))
load-instruction(Branch0Ins(has-stack-lbl, no-stack-lbl, HasStackOp()))
load-instruction(LabelIns(no-stack-lbl))
load-instruction(ExtendStackIns())
load-instruction(GotoIns(has-stack-lbl))
load-instruction(LabelIns(has-stack-lbl))
load-instruction(SectionMarkerIns(`preamble))

;Get arguments
within ret = normalize(ret-records(buffer, args(f), backend)) :
Expand Down Expand Up @@ -1438,4 +1442,4 @@ defn InsBuffer (function:VMFunction) :
else : i
def-table[n] = VMDef(n, t)
Local(n)