Skip to content

4.0 - #5

Merged
andrewdalpino merged 55 commits into
masterfrom
4.0
Sep 24, 2026
Merged

4.0#5
andrewdalpino merged 55 commits into
masterfrom
4.0

Conversation

@andrewdalpino

@andrewdalpino andrewdalpino commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Swaps PHP array for C array for all tensors.

@andrewdalpino
andrewdalpino merged commit 4f59636 into master Sep 24, 2026
10 checks passed
@torchello

torchello commented Sep 25, 2026 •

Copy link
Copy Markdown

Post-merge review of #5 (4.0: PHP arrays → C buffers)

Thanks for this. The direction is right and the speedups are real. I built the PR head (522dfef) against PHP 8.1, 8.3, 8.4 and 8.5 (Docker, OpenBLAS + LAPACKE). I ran the suite as-is, then again with the extension built under ASan, UBSan and LSan. I also exercised edge cases by hand and benchmarked against 3.1.1. The code behind every finding below is unchanged on current master (1003b1c only touched fromArray()), so all of them still apply.

What's good

  • Big wins where it matters. Compared with 3.1.1, element-wise ops are 25–50× faster, transpose ~20×, row reductions ~10×, and matmul/inverse ~3× (numbers in the perf comment).
  • The arithmetic, comparison, reduction and linear-algebra kernels re-derive dimensions and check them against the real buffer length. Because of that, most "shape lies about the buffer" cases end in an exception rather than memory corruption. I tried to break them through the public constructor, fromArray(..., false) and unserialize(), and they held.
  • No leaks. LSan over the full suite, plus a 20k-iteration loop over the C exception paths, shows nothing that comes from the new C code.
  • TensorBuffer slice, strided slice, split and repeat handle overflow carefully and are tested for it.
  • The committed generated C matches the .zep sources: all 178 throw sites map to the right lines.
  • The suite passes on 8.1, 8.3, 8.4 and 8.5, and also under ASan/UBSan. The only UBSan report is a zero-length VLA in Zephir's kernel/fcall.c:535, which is upstream code.

What needs follow-up (details inline)

# Severity Issue
1 High Matrix::repeat(): no overflow checks in tensor_matrix_repeat → heap-buffer-overflow write / SEGV
2 High quantile(NAN) passes validation → heap-buffer-overflow read (new in 4.0)
3 High ref() / rref() / rank() on m > n (including m×0) → heap over-read of pivots (carried over from 3.x into the rewritten helper)
4 Medium Serialized form is incompatible with rubix/tensor 4.0 (data vs a) and with every 3.x payload; __unserialize() trusts m/n
5 Medium The shape invariant m·n = count(buffer) is not enforced (public ctor, fromArray(..., false), __unserialize())
6 Medium Immutability leak: asTensorBuffer() + TensorBuffer::set()/sort() mutate every tensor that shares the buffer
7 Medium augment*() on an empty matrix gives a wrong shape or silently drops data (regression)
8 Medium The round() port matches PHP ≤ 8.3 only and differs from round() on 8.4 and 8.5
9 Medium repeat($m, 0) returns an empty matrix (Rubix ML's Hyperplane with one coefficient hits it)
10 Medium Errors across the C boundary: SPL exceptions instead of Tensor\Exceptions\*, and optimizers don't check EG(exception)
11 Medium Breaking changes are not in the CHANGELOG
12 Medium (perf) Slower element access, augmentLeft/Right and row iteration
13–18 Low / Nit FAILURE returned without an exception, BLAS xerbla output, empty shapes, NaN ordering, signed-overflow UB, tooling and docs

I can turn any of these into PRs.


Inline comments

1 · ext/include/reductions.c · L608–L627 (see also tensor/matrix.zep L1927–L1945)

[High] tensor_matrix_repeat: n * (times + 1) can overflow unchecked → heap-buffer-overflow write

rows, cols and rows * cols are computed without overflow checks. The copy loop then runs t_n + 1 times per row, using the count before wrapping. When n_hat * (t_n + 1) wraps, the allocation is small and the memcpy loop writes far past it:

// 3 * 6148914691236517206 = 2^64 + 2, so cols wraps to 2. The buffer holds 2 doubles; the loop writes 3 per iteration.
Matrix::fromArray([[1.0, 2.0, 3.0]])->repeat(0, 6148914691236517205);
ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 24
    #1 tensor_matrix_repeat ext/include/reductions.c:626
... located 0 bytes after 16-byte region allocated by tensor_matrix_repeat ext/include/reductions.c:613

->repeat(0, PHP_INT_MAX) on a 1×2 matrix wraps cols to 0. That gives a zero-length (NULL) buffer, and the process SEGVs writing to address 0. UBSan also reports the signed overflows of t_n + 1 and n_hat * (t_n + 1) at L609.

tensor_buffer_repeat() in buffer.c already gets this right (timesHat > ZEND_LONG_MAX / len, with repeatRejectsOverflowingTimes). The same guard is needed here:

if (UNEXPECTED(t_m == ZEND_LONG_MAX || t_n == ZEND_LONG_MAX
        || (m > 0 && t_m + 1 > ZEND_LONG_MAX / m)
        || (n_hat > 0 && t_n + 1 > ZEND_LONG_MAX / n_hat))) {
    zephir_throw_exception_string(tensor_exceptions_invalidargumentexception_ce,
        SL("Repeat counts overflow the matrix dimensions."));
    return;
}

zend_long rows = m * (t_m + 1);
zend_long cols = n_hat * (t_n + 1);

if (UNEXPECTED(cols > 0 && rows > ZEND_LONG_MAX / cols)) {
    /* same exception */
}

Matrix::repeat() also computes this->m * (m + 1) and this->n * (n + 1) in Zephir, which can overflow the same way. A test that mirrors TensorBufferTest::repeatRejectsOverflowingTimes would lock this in.


2 · ext/include/reductions.c · L549–L568 (see also tensor/vector.zep L1246, tensor/matrix.zep L1649)

[High] quantile(NAN) passes validation and reads before the row buffer

q < 0.0 || q > 1.0 is false for NaN, so NaN reaches C. (zend_long) NaN is undefined behaviour: it becomes 0 on arm64 and INT64_MIN on x86-64. Either way, row[x_hat - 1] ends up reading row[-1]:

Vector::fromArray([1.0, 2.0, 3.0, 4.0])->quantile(NAN);
Matrix::fromArray([[1.0, 2.0], [3.0, 4.0]])->quantile(NAN);
ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 8
    #0 tensor_quantile ext/include/reductions.c:565
... located 8 bytes before 32-byte region allocated by tensor_quantile ext/include/reductions.c:534

This is new in 4.0. 3.1.1 returns NAN without touching memory (checked under ASan). I'd use a NaN-safe check on both sides:

if unlikely !(q >= 0.0 && q <= 1.0) {

In C, validate before allocating anything:

double q_hat = zephir_get_doubleval(q);

if (UNEXPECTED(!(q_hat >= 0.0 && q_hat <= 1.0))) {   /* also rejects NaN */
    zephir_throw_exception_string(tensor_exceptions_invalidargumentexception_ce,
        SL("Q must be between 0 and 1."));
    return;
}

A test with NAN would keep it fixed.


3 · ext/include/linear_algebra.c · L992 and L1012–L1016

[High] tensor_ref_step: pivots has MIN(m, n) slots but is read for every i < m

For any matrix with more rows than columns (including m×0), the non-singular branch reads past the end of the pivots allocation:

Matrix::fromArray([[1.0], [2.0], [3.0], [4.0], [5.0], [6.0], [7.0], [8.0]])->ref();
Matrix::fromArray([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0], [7.0, 8.0]])->rank();
Matrix::fromArray([[], []])->rank();
ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 4
    #0 tensor_ref_step ext/include/linear_algebra.c:1013
... located 0 bytes after 4-byte region allocated by tensor_ref_step ext/include/linear_algebra.c:992

Beyond the out-of-bounds read, swaps for tall matrices is computed from garbage. The bug already existed in 3.x's tensor_ref (3.1.1 gives the same ASan report at linear_algebra.c:396). This PR rewrote that code, and the helper is now shared by rref() and therefore by rank() on any tall data matrix, so this is a good time to fix it:

unsigned int k = MIN(m, n);

for (i = 0; i < k; ++i) {
    if (i + 1 != (unsigned int) pivots[i]) {
        ++swaps;
    }
}

4 · tensor/vector.zep · L2100–L2123 (same in tensor/matrix.zep L2997–L3022)

[Medium] The serialized form doesn't match rubix/tensor 4.0 or 3.x payloads, and __unserialize() trusts n/m

  1. The rubix/tensor 4.0 CHANGELOG says "Standardized serial representation with extension". However, the PHP library writes ['a' => …, 'n' => …] (Matrix: a, m, n), while the extension writes ['data' => …, 'n' => …]. Loading a payload from one in the other fails in both directions:

    # written by rubix/tensor 4.0, read with tensor_ext 4.0
    Notice: Undefined index: data
    TypeError: Tensor\Vector::fromArray(): Argument #1 ($a) must be of type array, null given
    
    # written by tensor_ext 4.0, read with rubix/tensor 4.0
    Warning: Undefined array key "a" in src/Vector.php on line 2355
    TypeError: Cannot assign null to property Tensor\Vector::$a of type array
    

    Rubix ML persists models that contain tensors (NN Parameters, Loda::$r, …). A model trained with the extension can't be loaded where the extension isn't installed, and the other way around.

  2. Nothing serialized by 3.x can be unserialized at all. That covers both the extension and the PHP library, which use the default protected-property layout ("\0*\0a", "\0*\0n"). The result is the same notice and TypeError.

  3. this->n = data["n"] and this->m = data["m"] are taken as-is. The object can end up with shape() disagreeing with its buffer, or with a string n that breaks the strict !== dimension checks:

    $v = unserialize('O:13:"Tensor\Vector":2:{s:4:"data";a:1:{i:0;d:1;}s:1:"n";i:4096;}');
    $v->size();  // 4096, but the buffer holds 1 element
    $v->sum();   // Error: Call to a member function get() on null  (see comment 10)

Suggestion: use the PHP library's keys (a, m, n), accept the older layouts for a migration period, and take the shape from the data instead of the payload:

public function __unserialize(const array data)
{
    var values, legacy;

    let legacy = chr(0) . "*" . chr(0) . "a";   // 3.x default layout

    if isset data["a"] {
        let values = data["a"];
    } elseif isset data[legacy] {
        let values = data[legacy];
    } elseif isset data["data"] {               // 4.0.0 extension layout
        let values = data["data"];
    } else {
        throw new InvalidArgumentException("Invalid serialized vector.");
    }

    let this->a = self::fromArray(values)->a;
    let this->n = this->a->count();
}

For Matrix, rebuild with Matrix::fromArray($rows) (with validation) and copy m/n from the rebuilt matrix. A fixture test holding a string produced by rubix/tensor would keep the two implementations in step.


5 · tensor/matrix.zep · L417–L427 (see also fromArray() L359–L407)

[Medium] Matrix never checks m * n === count($a)

The public constructor accepts any TensorBuffer together with any non-negative m/n. fromArray(..., false) flattens ragged rows but still reports the shape as count($a) × count($a[0]):

$m = Matrix::fromArray([[1.0, 2.0], [3.0]], false);
$m->shape();                    // [2, 2]
$m->asTensorBuffer()->count();  // 3
$m->sum();                      // LengthException: Matrix and row dimensions must agree.

$m = new Matrix(Vector::fromArray([1.0, 2.0, 3.0])->asTensorBuffer(), 1000, 1000);
$m->shape();                    // [1000, 1000]

The kernels re-validate lengths, which is the only reason this isn't a memory-safety bug today. Nice. But:

  • The object is invalid from the moment it's created. It fails later, far from the cause, and with an SPL LengthException rather than a Tensor\Exceptions\*.
  • The C-side checks multiply the untrusted dimensions (ma * pa, m * nHat, …) without overflow protection, so a crafted shape can still slip through (comment 17).
  • MatrixTest::fromArraySkipsValidationWhenValidateFalse in this PR asserts the inconsistent state: shape [2, 2] over 3 elements. On master, 1003b1c deleted it together with the other fromArray() validation tests. I'd bring back the two "throws" tests.

Suggestion: enforce the invariant in the constructor, with an overflow-safe check:

if unlikely m < 0 || n < 0 || (n > 0 && m > intdiv(PHP_INT_MAX, n)) || m * n !== a->count() {
    throw new InvalidArgumentException("A " . m . " x " . n . " matrix needs " . (m * n)
        . " elements, " . a->count() . " given.");
}

rubix/tensor 4.0 also made the constructors non-public ("Tensor constructors are no longer public"), and an @internal static factory would let the Zephir classes do the same. fromArray(..., false) can keep skipping the per-row checks. A single count(flat) === rows * n check after flattening costs almost nothing.


6 · tensor/vector.zep · L367–L370 (see also tensor/tensorbuffer.zep L34–L47, L85–L99)

[Medium] asTensorBuffer() exposes the internal buffer, which is shared and mutable

Vector and Matrix are otherwise immutable (offsetSet() throws "cannot be mutated directly"). Many operations are zero-copy views over the same TensorBuffer: reshape, asRowMatrix, asColumnMatrix, flatten, Vector::transpose and ColumnVector::transpose. asTensorBuffer() returns that same instance. TensorBuffer has public in-place mutators (set(), sort()), and asBuffer() gives out the raw Tensor\Buffer with ArrayAccess, fill() and a callable __construct(). So:

$v = Vector::fromArray([3.0, 1.0, 2.0]);
$m = $v->reshape(1, 3);
$t = $v->transpose();

$v->asTensorBuffer()->set(0, 100.0);
$m->asTensorBuffer()->sort();

// $v, $m and $t are now all [1, 2, 100]

TensorBuffer::fromBuffers([$x]) also returns $x itself (L42–L44), so the "new" tensor aliases its source.

Options:

  • Make set() and sort() return new buffers, or take them off the public surface.
  • Have asTensorBuffer() return a copy, or mark it @internal and use this->a internally.
  • Copy in the single-element branch of fromBuffers().

Related:

  • Make TensorBuffer final. The C side requires the exact class (Z_OBJCE_P(obj) != tensor_tensorbuffer_ce), so a subclass passes the Zephir type hint and then fails in C. The class is also @internal in the source, yet documented in docs/index.md / docs/TensorBuffer.md as public.
  • Check the element type in the Vector/Matrix constructors. new Vector(new TensorBuffer(new Buffer(4, Buffer::TYPE_LONG))) is accepted. sum() then works, because the reductions accept LONG, but abs() throws Argument must wrap a buffer of type double.

7 · tensor/matrix.zep · L1822–L1917

[Medium] augment*() on an empty matrix: wrong shape or silently dropped data

The this->m > 0 && guard signals that augmenting an empty matrix is supported. In 3.1.1, augmentAbove/Below returned b in that case. Now:

$empty = Matrix::fromArray([]);
$b = Matrix::fromArray([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]]);

$empty->augmentBelow($b);   // shape [2, 0] over 6 elements; ->sum() then throws LengthException (3.1.1: [2, 3])
$empty->augmentAbove($b);   // shape [2, 0] over 6 elements
$empty->augmentRight($b);   // shape [0, 3] with an empty buffer: b's data silently dropped (3.1.1 threw a TypeError)
$empty->augmentLeft($b);    // shape [0, 3] with an empty buffer

// the usual accumulate-in-a-loop pattern
$acc = Matrix::fromArray([]);
foreach ([$b, $b] as $chunk) {
    $acc = $acc->augmentBelow($chunk);
}
// DimensionalityMismatch: Matrix A requires0 columns but Matrix B has 3.

augmentAbove/Below use this->n for the result even when this is empty. augmentLeft/Right loop only over this->m rows. Suggest adding this at the top of all four:

if this->m < 1 {
    return new self(b->a, b->m(), b->n());
}

Also, the message is missing a space: "Matrix A requires ".


8 · ext/include/unary.c · L100–L242

[Medium] round() matches PHP's round() only on PHP ≤ 8.3

The comment says this mirrors ext/standard/math.c "so that results match PHP's round() exactly". What it mirrors is the pre-8.4 algorithm, which pre-rounds to 15 significant digits. PHP 8.4 replaced that algorithm. So on the 8.4 and 8.5 CI targets, the extension now disagrees with round(), and with 3.x, which called the running PHP's own implementation. I compared Vector::round($p) with round() over 200k values × precisions 0–3:

PHP mismatches (of 800k)
8.1.34 0
8.3.33 0
8.4.26 17,498
8.5.11 17,498
round(0.49999999999999994, 0): php=0                 ext=1
round(1.4999999999999998, 0):  php=1                 ext=2
round(178.54999999999998, 1):  php=178.5             ext=178.6
round(4503599627370495.5, 0):  php=4503599627370496  ext=4503599627370495.5

The simplest fix that is correct on every version is to call the engine's implementation. It is PHPAPI on 8.1 through 8.5, and Zephir's kernel/math.c already uses it:

#include <ext/standard/php_math.h>
...
vc[i] = _php_math_round(va[i], places, PHP_ROUND_HALF_UP);

The local port and its lookup tables can then go.


9 · tensor/matrix.zep · L1927–L1945

[Medium] repeat($m, 0) returns an empty matrix

The docblock and the C kernel agree that the result is m·(times_m+1) × n·(times_n+1), but the early return short-circuits every call with n < 1:

$a = Matrix::fromArray([[1.0], [2.0]]);

$a->repeat(0, 0);   // 0×0; expected 2×1 (the same matrix)
$a->repeat(1, 0);   // 0×0; expected 4×1
$a->repeat(1, -1);  // 0×0, silently
$a->repeat(-1, 1);  // \InvalidArgumentException (SPL) "Times must be non-negative." raised from C

This hits real code. Rubix ML's Hyperplane generator does ->asColumnMatrix()->repeat(0, $d - 1), so new Hyperplane([2.5]) gets a 0×0 matrix. The next ->multiply($coefficients) then throws DimensionalityMismatch: Matrix A expects 0 columns but Vector B has 1.

The bug is inherited: 3.x wrapped the row loop in if n > 0, and rubix/tensor's Matrix::repeat() has the same if ($n > 0). Since this PR rewrites the method anyway, I'd fix it here (and in the PHP library) instead of making it explicit:

if unlikely m < 0 || n < 0 {
    throw new InvalidArgumentException("Repeat counts must be"
        . " greater than or equal to 0.");
}

if unlikely this->m < 1 || this->n < 1 {
    return new self(tensor_buffer_from_array([]), this->m * (m + 1), this->n * (n + 1));
}

10 · optimizers/TensorReduceSumOptimizer.php · L66–L68 (the same pattern is in every optimizer; see also ext/include/arithmetic.c L26–L30)

[Medium] Errors raised in C: SPL exception classes, and the generated code keeps running after them

  1. The kernels throw \LengthException, SPL \InvalidArgumentException and \OutOfBoundsException, while the Zephir layer and AGENTS.md use Tensor\Exceptions\*. catch (TensorException $e) no longer catches a dimension mismatch that only the kernel detects:

    $a = Matrix::fromArray([[1.0, 2.0], [3.0, 4.0]]);
    $b = new Matrix(Vector::fromArray([1.0, 2.0, 3.0])->asTensorBuffer(), 2, 2);
    
    try {
        $a->multiplyMatrix($b);
    } catch (Tensor\Exceptions\TensorException $e) {
        // not reached: LengthException: Input buffers must be the same length.
    }

    I'd suggest tensor_exceptions_invalidargumentexception_ce / tensor_exceptions_dimensionalitymismatch_ce. tensor_buffer_repeat() already uses the former in one place.

  2. The optimizers emit a bare tensor_xxx(&result, ...); with no exception check. After a kernel throws, the method keeps running with result still NULL. If the next statement calls a method on result, the real error is buried under an Error:

    // Vector::sum(): tensor_reduce_sum() throws, then result->get(0) runs on NULL
    Error: Call to a member function get() on null
      previous: LengthException: Matrix and row dimensions must agree.
    

    Having the optimizer template emit the same exception check that Zephir emits after its own calls (for example if (UNEXPECTED(EG(exception))) { RETURN_MM(); }) would fix every call site at once.


11 · CHANGELOG.md · L3–L4

[Medium] The 4.0.0 changelog doesn't list the breaking changes

The only entry is the fromArray() speed-up, and on master (1003b1c) even that line is gone, leaving 4.0.0 empty. People upgrading the extension need to know at least:

  • Vector/ColumnVector/Matrix build() and quick() were removed; use fromArray($a, $validate) instead.
  • Constructors now take (TensorBuffer $a) or (TensorBuffer $a, int $m, int $n).
  • New Tensor\Buffer and Tensor\TensorBuffer classes, and new asTensorBuffer(), asRowBuffers(), asColumnBuffers(), argmin() and argmax() methods.
  • mod*() now uses fmod(), so results are no longer truncated to integers. Divisors in (-1, 1) used to throw DivisionByZeroError, because they truncate to 0. Now fmod() returns a value for them, and a zero divisor gives NAN. For example, [5.5, 7.9] mod 2 is [1.5, 1.9] (3.1.1: [1, 1]), and [5.5] mod 0.5 is [0.0] (3.1.1: DivisionByZeroError). rubix/tensor 4.0 documents this as "Modulus results no longer rounded to nearest integer".
  • The Special interface gained argmin()/argmax(), so third-party Tensor implementations must add them.
  • The serialization format changed (comment 4).
  • fromArray() is positional and discards keys.
  • Numeric-string offsets such as $vector['0'] now throw (3.1.1 returned the element).
  • Matrix::asArray() on an m×0 matrix returns [] (3.1.1: m empty rows), and reductions on 0-row matrices throw (3.1.1: an empty ColumnVector).
  • min()/max() with NaN now depend on its position: [NAN, 1.0]->min() is NAN (3.1.1: 1.0).
  • Errors raised by the kernels are SPL exceptions.
  • round() behaves differently on PHP ≥ 8.4, unless comment 8 is fixed.

Keeping quick() and build() for one major version as deprecated one-line aliases (fromArray($a, false) / fromArray($a)) would make the upgrade painless for anyone not already on Rubix ML master.


12 · tensor/matrix.zep · L1862–L1917 (see also tensor/vector.zep L2074–L2082)

[Medium, perf] Slower than 3.1.1 on paths that cross the PHP/C boundary

These are rough single runs (PHP 8.4, arm64, OpenBLAS single-threaded), in µs per call. The kernel paths are a huge win, but several common paths got slower:

operation 3.1.1 4.0
A + A (2000×64) 1500 28 53× faster
transpose (2000×64) 850 44 19× faster
row sum (2000×64) 238 20 12× faster
variance (2000×64) 2720 204 13× faster
matmul (256×256) 2807 963 2.9× faster
$v[$i] loop, 100k 5777 19801 3.4× slower
$m[$i][$j] loop, 2000×64 7627 29657 3.9× slower
augmentRight (2000×64) 330 3994 12× slower
foreach ($matrix as $row) 509 1207 2.4× slower
asVectors() 544 1184 2.2× slower
Matrix::fromArray vs quick (2000×64) 33 882 O(1) → O(n)
asArray() (2000×64) 0.1 589 O(1) → O(n)

Ideas:

  • augmentLeft/Right build m slices, then m concats, then call fromBuffers(). A C kernel that interleaves the rows with two memcpys per row would remove all of that. Rubix ML's Ridge::train() calls augmentLeft() on the whole dataset.
  • Vector::offsetGet() goes through a Zephir method, then TensorBuffer::get() (another method call), then the kernel dimension read. An optimizer that reads the double straight from tensor_tensorbuffer_doubles() would save two method calls per element. Matrix::offsetGet() has the same problem and allocates a Buffer, a TensorBuffer and a Vector per row.
  • zeros(), ones(), fill(), identity() and diagonal() build PHP arrays and then flatten them. new Buffer($n) is already zero-filled, and Buffer::fill() exists.
  • Matrix::fromArray() still builds a flat PHP array before copying it into the buffer. Walking the nested array straight into the buffer in C would avoid creating an m·n zval array.
  • The fromArray benchmarks added here were removed on master in 1003b1c. I'd keep them and add benchmarks for element access, iteration and augment*, so regressions like these show up.

13 · ext/include/buffer.h · L14–L19 (see also ext/kernel/buffer.h L81–L85, ext/include/signal_processing.c L37–L44)

[Low] "Returns FAILURE (and throws)", but nothing is thrown

zephir_buffer_create() and tensor_tensorbuffer_create() are documented to throw on failure, but for a negative length they just return FAILURE. Every kernel then does return;, with no exception and no return value, and Zephir passes NULL to the next constructor:

Vector::fromArray([])->convolve(Vector::fromArray([]));
// TypeError: Tensor\Vector::__construct(): Argument #1 ($a) must be of type Tensor\TensorBuffer, null given

Here nc = na + nb - 1 = -1 in tensor_convolve_1d. Either throw from tensor_tensorbuffer_create() on failure, or guard the callers. convolve() with an empty kernel should probably be an InvalidArgumentException; [1, 2, 3]->convolve([]) currently returns [0, 0].


14 · ext/include/linear_algebra.c · L97

[Low] BLAS gets called with lda = 0 for empty dimensions

Matrix::fromArray([[], []])->dot(Vector::fromArray([]));
// printed by OpenBLAS:  ** On entry to DGEMV  parameter number  6 had an illegal value

OpenBLAS's xerbla writes straight to the process's stdout, bypassing PHP's output layer and error handling. Returning early when ma == 0 || pc == 0 avoids it. The other BLAS/LAPACK calls are worth checking for zero-sized dimensions (or passing MAX(1, ld)) too.


15 · tensor/matrix.zep · L599–L606 (see also ext/include/reductions.c L244–L249)

[Low] Degenerate shapes behave differently from 3.1.1

Matrix::fromArray([[], [], []])->asArray();  // []  (3.1.1: [[], [], []])
Matrix::fromArray([])->sum();                 // \InvalidArgumentException "Number of groups must be greater than 0." (3.1.1: empty ColumnVector)
// mean(), l2Norm(), covariance() and median() on 0-row matrices throw as well
Matrix::fromArray([[], []])->covariance();    // LengthException: Matrix and vector dimensions must agree.

The n < 1 early returns drop m. For the reductions, tensor_reduce_apply() could return an empty buffer when groups == 0 instead of throwing. (transpose() of m×0 → 0×0 was already the case in 3.1.1.)


16 · ext/include/buffer.c · L140–L151 (see also ext/include/reductions.c L181–L215, L360–L366)

[Low] NaN handling depends on where the NaN sits

(da > db) - (da < db) returns 0 for any comparison with NaN, so it isn't a strict weak ordering. Once a NaN is present, qsort() results are arbitrary, and with them median(), quantile() and TensorBuffer::sort(). min, max, argmin and argmax keep the NaN only if it's the first element:

Vector::fromArray([NAN, 1.0, 3.0, 2.0])->median();  // 1.5
Vector::fromArray([NAN, 1.0])->min();               // NAN  (3.1.1: 1.0)
Vector::fromArray([1.0, NAN])->min();               // 1.0

A NaN-aware comparator (for example, NaN sorts last) and a documented rule for the extrema (propagate NaN, or ignore it) would make this deterministic. The two comparators are identical, so one shared helper would do. A consistent comparator is also worth having for its own sake: older glibc qsort() fallbacks had memory-safety bugs with non-transitive comparators on very large arrays.


17 · ext/include/arithmetic.c · L413 (see also ext/include/linear_algebra.c L41, L386–L394)

[Low] Dimension checks multiply zend_longs without overflow checks (UB), and sizes use unsigned int

Checks such as total != m * nHat, na != ma * pa || nbb != pa * nb and total != ma * na multiply values taken from the unvalidated shape (comment 5). A signed overflow there is undefined behaviour, and a wrapped product can make the check pass. In the decomposition kernels, the dimensions are cast to unsigned int and the allocation sizes are computed as ma * na * sizeof(double) in 32-bit arithmetic before widening. None of this is reachable with sane shapes, but these checks are the last line of defence for comment 5. tensor_buffer_slice_strided() already shows the fix (divide instead of multiply), and safe_emalloc(ma, (size_t) na * sizeof(double), 0) covers the allocation side.


18 · composer.json · L34 (see also .github/workflows/ci.yml)

[Low] Build reproducibility and CI coverage

  • "phalcon/zephir": "dev-development" with no lock file. The committed ext/ depends on an unreleased Zephir branch: the new kernel/buffer.c, kernel-classes, and the tensor_buffer_ce alias workaround in config.json. Regenerating later will silently pick up whatever development is by then. Pinning a commit (dev-development#<sha>) until a Zephir release ships Buffer would make composer compile reproducible.
  • CI only triggers on ext/**, tests/** and the workflow itself. A PR that changes tensor/*.zep, optimizers/** or config.json without regenerating ext/ therefore passes. A job that runs zephir generate followed by git diff --exit-code ext/ would catch that. For what it's worth, I checked heuristically that the committed C matches the .zep at 522dfef: all 178 zephir_throw_exception_debug sites point at the matching throw lines.
  • PHPStan was removed from CI and composer, but stubs/TensorBuffer.php still says it exists for scanFiles in phpstan.neon, which has been deleted. Either restore the analysis (the stub was written to make it work) or drop the stub.
  • An ASan job would be cheap: build with -fsanitize=address, run with USE_ZEND_ALLOC=0, and preload a tiny shim that strips RTLD_DEEPBIND from PHP's dlopen(). Together with edge-case tests (empty and tall matrices, NaN, huge repeat counts), it would have flagged all three High findings above.

19 · Nits (one comment, or spread across the files)

  • AGENTS.md L79: extension=iconv is missing its -d, so PHP takes it as the script path and PHPUnit never runs. Also: "TEnsor" (L76); the "Full build" row (L35) still lists analyze, but composer build no longer runs it; "Bump the version in both config.json" (L70) now names only one file.
  • scripts/compare-serialization.php L27: the hint points at ext/modules/tensor.so, but the module is tensor_ext.so.
  • docs/TensorBuffer.md:
    • get() and set() throw SPL RuntimeException / OutOfBoundsException (from the kernel Buffer), not Tensor\Exceptions\RuntimeException.
    • fromBuffers() is undocumented.
    • concat() with an element that isn't a TensorBuffer raises Error: Call to a member function asBuffer(), not InvalidArgumentException.
  • tensor/tensorbuffer.zep L130–L182: sliceStrided() hand-rolls an overflow-safe multiplication. The C side already re-checks with (length - 1) > last / stride, so the Zephir side could use the same one-liner: length - 1 > intdiv(limit, stride).
  • ext/php_tensor.h, ext/tensor.c and ext/tensor.h are leftovers from the old module name and aren't in config.m4. Even so, php_tensor.h received the new ZEPHIR_BUFFER_* defines, which makes it easy to edit the wrong file.

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.

2 participants