diff --git a/NEXT.md b/NEXT.md index 0474117..f87ae35 100644 --- a/NEXT.md +++ b/NEXT.md @@ -237,13 +237,38 @@ Non-local exit is lowered explicitly: `return`, `some` and a failed bounds check are branches, never platform unwinding, so wasm32 needs no exception proposal. -## Sharp edges found and left visible +## Sharp edges + +Most of these are edges the language keeps and you should know about. Two — +the top-level namespace and the shift count, both found by review after +milestone 4 — were bugs that reached LLVM or ran wrong, and are **fixed**; each +says so. They stay written down because each one is now a rule the checker +enforces, and a later change could quietly drop it. - **An index converts from a narrower integer and never from a wider one.** `(nth colors current-color)` with a `u32` index works — anything above 2³¹ truncates to a negative `i32` and the unsigned bounds check rejects it. An `i64` index is refused with the reason: 2³²+5 truncates to 5 and would read the wrong element with no trap at all. +- **There is one top-level namespace, and `check.ml` now enforces it.** The + environment's tables are per-kind — structs, unions, aliases, enums, + functions, externs and globals each have their own — so only a function was + ever checked for a duplicate. `(defn item …)` beside `(defvar item …)` type + checked and then died in LLVM as `redefinition of function '@flan.item'`, a + message about an emitted symbol with no source location left, and two + colliding *type* declarations were not caught anywhere. One pass over + `Ast.declared_name` now runs before every other collection pass and rejects + the second declaration of a name whatever kind either one is. `declared_name` + lives in `ast.ml` because `Load` needs exactly the same set — the names an + import renames — and two copies of that list would drift. +- **A shift count is bounded, two different ways.** A shift by the operand's + own width or more is *poison* in LLVM, not a wrong number: `(defn main [] i32 + (<< 1 32))` compiled at -O2 to a bare `retq`, returning an undefined value. A + literal count out of range is now rejected in `check.ml` — that is the typo + case — and `emit.ml` masks a computed count to `width - 1`, which is what the + hardware does anyway and which LLVM folds away whenever the count is + constant. The prelude's rotate masks its own count; that is now redundant but + harmless. - **A `u64` literal is its 64-bit pattern**, so `0xcbf29ce484222325` is a real `u64` and not an error. The cost is that a negative *decimal* literal is accepted as a `u64` too, because the reader records the value and not how it diff --git a/lib/ast.ml b/lib/ast.ml index 58b9a65..c7ae83d 100644 --- a/lib/ast.ml +++ b/lib/ast.ml @@ -108,3 +108,14 @@ and decl_kind = and variant = { vname : string; vfields : field list; vloc : Loc.t } and init = Zeroed | Uninit | Init of expr + +(* Every top-level name a declaration introduces, whatever kind it is. There is + one top-level namespace, so this is both the set [Load] renames on an import + and the set [Check] refuses to see twice — one definition, so the two cannot + drift apart. *) +let declared_name (d : decl) = + match d.d with + | Defenum (n, _) | Defalias (n, _) | Defstruct (n, _) | Defunion (n, _) + | Defvar (n, _, _) | Defconst (n, _, _) -> Some n + | Declare (fn, _) | Defn fn -> Some fn.name + | Package _ | Import _ -> None diff --git a/lib/check.ml b/lib/check.ml index b554602..4cf4ad8 100644 --- a/lib/check.ml +++ b/lib/check.ml @@ -767,6 +767,19 @@ and named_call ctx ~want loc name args = | Types.Int _ -> () | other -> fail loc "%s takes integers, found %s" name (Types.to_string other)); + (* A shift by the operand's own width or more is poison in LLVM, which at + -O2 turns the whole function into an undefined value rather than into a + wrong number. A literal count is rejected here — that is the typo — and + [emit] masks a computed one, so no shift can reach the hardware out of + range. *) + (match a.Tast.ty, b.Tast.e with + | Types.Int k, Tast.Int (n, _) when p = Tast.Shl || p = Tast.Shr -> + let w = Int64.of_int (Types.bits k) in + if Int64.unsigned_compare n w >= 0 then + fail loc + "%s by %Ld is out of range for %s, which is %d bits wide" name n + (Types.to_string a.Tast.ty) (Types.bits k) + | _ -> ()); prim p a.Tast.ty [ a; b ] (* (min a b) and (max a b) evaluate each operand once — hence the slots — because a min over two calls must not call either of them twice. *) @@ -983,6 +996,22 @@ let rec const_int env (e : Ast.expr) : int64 option = | _ -> None let collect env (decls : Ast.decl list) = + (* One pass over every declaration kind before any of the others, because + the tables below are per-kind — structs, unions, aliases, enums, functions + and globals each have their own — and a collision between two of them + would otherwise be found by LLVM, as [redefinition of function + '@flan.item'], or not at all. A [defn item] and a [defvar item] are two + declarations of one name and are rejected here. *) + let claimed = Hashtbl.create 64 in + List.iter + (fun (d : Ast.decl) -> + match Ast.declared_name d with + | None -> () + | Some n -> + if Hashtbl.mem claimed n then + fail d.Ast.dloc "%s is defined twice" n; + Hashtbl.add claimed n ()) + decls; (* Names first, so a struct may mention one declared below it. *) List.iter (fun (d : Ast.decl) -> @@ -1086,8 +1115,6 @@ let collect env (decls : Ast.decl list) = { Tast.vname = v.Ast.vname; vfields = List.map field v.Ast.vfields }) vs } | Ast.Defn fn -> - if Hashtbl.mem env.fns fn.Ast.name then - fail loc "%s is defined twice" fn.Ast.name; let params = List.map (fun (p : Ast.field) -> resolve env p.Ast.fty) fn.Ast.params in diff --git a/lib/emit.ml b/lib/emit.ml index b7cd29d..9dbbdec 100644 --- a/lib/emit.ml +++ b/lib/emit.ml @@ -555,6 +555,20 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = | Types.Int k, _ -> if Types.signed k then "ashr" else "lshr" | t, _ -> failwith ("bitwise on " ^ Types.to_string t) in + (* The count is masked to the operand's width. LLVM makes an over-wide + shift poison, and a poison return at -O2 is a function that returns + nothing at all; masking is what the hardware does anyway, and LLVM folds + the [and] away whenever the count is a constant. [check] has already + rejected a literal that is out of range, so this only ever fires on a + computed count. *) + let b = + match x.Tast.ty, p with + | Types.Int k, (Tast.Shl | Tast.Shr) -> + let m = fresh f in + ins f "%s = and %s %s, %d" m (ll x.Tast.ty) b (Types.bits k - 1); + m + | _ -> b + in let t = fresh f in ins f "%s = %s %s %s, %s" t op (ll x.Tast.ty) a b; t diff --git a/lib/load.ml b/lib/load.ml index 07bc885..5c86674 100644 --- a/lib/load.ml +++ b/lib/load.ml @@ -228,15 +228,7 @@ let qualify_decl owned alias (d : Ast.decl) : Ast.decl = (* Every top-level name the package declares — types and values alike, since a use site is rewritten by name and the two never collide in one namespace. *) let owned_names (ds : Ast.decl list) = - List.filter_map - (fun (d : Ast.decl) -> - match d.Ast.d with - | Ast.Defenum (n, _) | Ast.Defalias (n, _) | Ast.Defstruct (n, _) - | Ast.Defunion (n, _) | Ast.Defvar (n, _, _) | Ast.Defconst (n, _, _) -> - Some n - | Ast.Declare (fn, _) | Ast.Defn fn -> Some fn.Ast.name - | Ast.Package _ | Ast.Import _ -> None) - ds + List.filter_map Ast.declared_name ds let import ~loc alias dir = let files = entries dir ".flan" in diff --git a/test/test_flan.ml b/test/test_flan.ml index e4a377c..711a3ed 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -552,6 +552,30 @@ let () = (* A folded constant skips [check], so its range check has to be its own. *) rejects_check "a folded constant is still range-checked" "(defconst c u8 300) (defn f [] u8 c)" ~needle:"does not fit in u8"; + (* One top-level namespace, enforced across declaration kinds. Each of these + used to pass the checker — the tables are per-kind — and be caught by LLVM + as a redefinition of an emitted symbol, or not caught at all. *) + rejects_check "a global defined twice" + "(defvar x i32 1) (defvar x i32 2)" ~needle:"defined twice"; + rejects_check "a constant shadowing a variable" + "(defconst c 1) (defvar c i32 2)" ~needle:"defined twice"; + rejects_check "a function and a global" + "(defn item [] i32 1) (defvar item i32 2)" ~needle:"defined twice"; + rejects_check "a struct and an alias" + "(defstruct P [x i32]) (defalias P i32)" ~needle:"defined twice"; + rejects_check "an enum and a struct" + "(defenum E [a 1]) (defstruct E [x i32])" ~needle:"defined twice"; + rejects_check "an extern and a constant" + "(declare cw [] \"flan_cw\") (defconst cw 1)" ~needle:"defined twice"; + (* A shift by the operand's own width or more is poison in LLVM, and at -O2 + a poison return is a function that returns nothing at all. A literal count + is rejected; a computed one is masked in [emit]. *) + rejects_check "a shift past the operand's width" + "(defn f [] i32 (<< 1 32))" ~needle:"out of range"; + rejects_check "a right shift past the operand's width" + "(defn f [] u8 (>> (u8 1) 8))" ~needle:"out of range"; + accepts "a shift by the widest count in range" + "(defn f [] i32 (<< 1 31))"; (* An index converts from a narrower integer and never from a wider one. *) accepts "a u32 index" "(defvar a [4 u32]) (defn f [] u32 (let [i 2] (at a (u32 i))))"; rejects_check "an i64 index"