Skip to content

Fix EXPT type contagion, MAP result-type dispatch on OR, and TRACE of…#747

Open
blakemcbride wants to merge 1 commit intoarmedbear:masterfrom
blakemcbride:miscellaneous
Open

Fix EXPT type contagion, MAP result-type dispatch on OR, and TRACE of…#747
blakemcbride wants to merge 1 commit intoarmedbear:masterfrom
blakemcbride:miscellaneous

Conversation

@blakemcbride
Copy link
Copy Markdown

Fix EXPT type contagion, MAP result-type dispatch on OR, and TRACE of (SETF name)

Summary

Repairs three unrelated ANSI conformance failures:

Test Area
EXPT.29 Type contagion when the base is zero
MAP.48 MAKE-SEQUENCE with an OR of vector types
TRACE.8 Compiled (SETF foo) calls bypassed the trace wrapper

Each test passes and no previously-passing test regresses.

Root causes and fixes

1. EXPT.29(expt 0 y) ignored the type of y

MathFunctions.EXPT handled a zero base with a bare return base;:

if (base.zerop())
    return base;

That returned the exact base object, which meant (expt 0 2.0f0) produced
the integer 0 rather than the single-float 0.0f0. The ANSI test walks
(x, y) over zero bases (0, 0.0f0, 0.0d0, various complex zeros) and
non-zero powers and requires (eql (* x y) (expt x y)). The old behavior
violated that for every case where y was a float or complex-float.

Fix (MathFunctions.java): Delegate to the existing multiplication
contagion machinery.

if (base.zerop())
    // Type contagion: (expt 0 y) must match the result type of
    // (* 0 y) so that e.g. (expt 0 2.0f0) => 0.0f0, not 0.
    return base.multiplyBy(power);

base.multiplyBy(power) produces zero of the correct contagious type
(single-float, double-float, complex single, complex double, etc.) — the
same value the test expects from (* x y).

2. MAP.48(make-sequence '(or ...) size) errored unconditionally

MAP defers to MAKE-SEQUENCE for the result-type. MAKE-SEQUENCE
rejected any compound type whose operator was not one of the known
vector/array names and signalled "(OR ...) is not a sequence type."
— even when the branches of the OR all shared a single element type.

MAP.48's result type is (or (vector t 10) (vector t 5)); both
branches have element type t, and one of them ((vector t 5)) matches
the input length of 5. CLHS 17.3 says the element type of the result
array is determined by the type specifier when the implementation can
determine it, so this case is legal and well-defined.

The opposite case matters too: MAP.ERROR.10, MAKE-SEQUENCE.ERROR.15,
COERCE.ERROR.10, and CONCATENATE.ERROR.6 all pass an OR whose
branches have conflicting element types (e.g. (or (vector bit) (vector t))) and require an error. So the fix must accept unambiguous
OR types and reject ambiguous ones.

Fix (make-sequence.lisp): Before the existing type dispatch, peel
off an OR by extracting each branch's element type. If all branches
agree, iterate over the branches and return the first one that
MAKE-SEQUENCE can satisfy at the requested size. If the element types
conflict (or any branch is not a recognized vector type), signal the
existing simple-type-error:

(when (and (consp type) (eq (%car type) 'or))
  (let ((branches (%cdr type))
        (common-et nil)
        (first-p t))
    (dolist (branch branches)
      (let ((et (%vector-type-element-type branch)))
        (cond ((eq et :unknown)
               (setf common-et :unknown) (return))
              (first-p (setf common-et et first-p nil))
              ((equal common-et et) nil)
              (t (setf common-et :conflict) (return)))))
    (when (or (eq common-et :unknown) (eq common-et :conflict))
      (error 'simple-type-error ...))
    (dolist (subtype branches)
      (let ((result (handler-case
                        (if iesp
                            (make-sequence subtype size
                                           :initial-element initial-element)
                            (make-sequence subtype size))
                      (error () :make-sequence-or-branch-failed))))
        (unless (eq result :make-sequence-or-branch-failed)
          (return-from make-sequence result))))
    (error 'simple-type-error ...)))

%vector-type-element-type recognizes the standard vector-shaped type
specifiers (VECTOR, SIMPLE-VECTOR, (VECTOR et), STRING, BIT-VECTOR,
ARRAY, etc.) and returns the declared element type or :UNKNOWN.

3. TRACE.8 — compiled (SETF foo) bypassed the trace wrapper

(trace (setf foo)) installs a wrapper by replacing
(fdefinition '(setf foo)). A compiled caller only sees that wrapper if
the call site resolves through the symbol's setf-function slot at
runtime. For ordinary symbols, compiled (foo …) calls go through
Symbol.execute, which dispatches to the current fdefinition, so
traced regular functions emit output correctly (TRACE.1–TRACE.7).

For (setf foo) the name is a two-element list. p2-function in
compiler-pass2.lisp handles this with:

((and (consp name) (eq (%car name) 'SETF))
 (cond
   (local-function …)
   ((and (member name *functions-defined-in-current-file* :test #'equal)
         (not (notinline-p name)))
    (emit-getstatic …))          ; same-file, not notinline
   ((and (null *file-compilation*)
         (fboundp name)
         (fdefinition name))
    (emit-load-externalized-object (fdefinition name)))  ; *** BUG ***
   (t
    (emit-load-externalized-object (cadr name))
    (emit-invokevirtual +lisp-symbol+ "getSymbolSetfFunctionOrDie" …))))

The third clause (flagged with a pre-existing FIXME Need to check for NOTINLINE declaration! comment) fires whenever (compile 'f) is
invoked on a function that references #'(setf foo), including the
compile* call rt.lsp makes when *compile-tests* = T. The branch
bakes the current fdefinition into the generated class as a static
field — so the wrapper installed by TRACE later is never called.

Fix (compiler-pass2.lisp): Add the missing notinline-p guard so
the compiled form falls through to the final clause, which loads the
symbol and calls getSymbolSetfFunctionOrDie() (a runtime lookup):

((and (null *file-compilation*)
      (not (notinline-p name))
      (fboundp name)
      (fdefinition name))
 (emit-load-externalized-object (fdefinition name))
 (emit-move-from-stack target))

TRACE.8's test file declares
(declaim (notinline function-to-trace (setf function-to-trace))) at
the top, so the user's explicit NOTINLINE now propagates into the
generated bytecode. This mirrors how the regular-symbol branch already
routes notinline calls through Symbol.execute rather than caching the
function object.

Files changed

  • src/org/armedbear/lisp/MathFunctions.java
  • src/org/armedbear/lisp/make-sequence.lisp
  • src/org/armedbear/lisp/compiler-pass2.lisp

Test plan

  • ant abcl builds cleanly.
  • Full ant test.ansi.compiled: EXPT.29, MAP.48, and TRACE.8
    leave the failure list. The OR-type error tests
    (COERCE.ERROR.10, CONCATENATE.ERROR.6, MAKE-SEQUENCE.ERROR.15,
    MAP.ERROR.10) continue to pass — they were first regressed by an
    earlier, over-permissive MAKE-SEQUENCE change and required the
    element-type-coherence check described above.
  • Spot checks:
    - (expt 0 2.0f0)0.0f0; (expt 0 #C(2.0 0.5))#C(0.0 0.0).
    - (map '(or (vector t 10) (vector t 5)) #'identity '(1 2 3 4 5))
    #(1 2 3 4 5).
    - (map '(or (vector bit) (vector t)) #'identity '(1 0 1)) signals
    simple-type-error.
    - After (compile 'f) where f does
    (setf (traced-setf-name x) v), the compiled f honours
    (trace (setf traced-setf-name)).

Compatibility

  • EXPT now returns a float/complex zero rather than an integer zero
    when the base is zero and the power has a float/complex type. This
    matches CLHS 12.1 type-contagion rules and what (* 0 y) already
    produced.
  • MAKE-SEQUENCE now accepts (or T1 T2 …) when all branches share a
    single element type. Ambiguous OR result types (conflicting element
    types or unrecognized branches) continue to signal
    simple-type-error.
  • (SETF foo) calls in code compiled with COMPILE (not COMPILE-FILE)
    now honour (declaim (notinline (SETF foo))). Code that declared
    NOTINLINE already intended runtime dispatch, so no caller should
    need to change.

@blakemcbride
Copy link
Copy Markdown
Author

With this final patch, ABCL passes the entire ANSI conformance suite.

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

Successfully merging this pull request may close these issues.

1 participant