subagent transcript

session 18e25685 · workflow wf_23df5b1c
← 18e25685 · archive
delegated task
You are an adversarial verifier for a FreeCAD-wasm code review. A reviewer claims: SEVERITY: low CATEGORY: stale-state FILE: /home/magik6k/lcad-wasm/f
▸ Magik
You are an adversarial verifier for a FreeCAD-wasm code review. A reviewer claims: SEVERITY: low CATEGORY: stale-state FILE: /home/magik6k/lcad-wasm/freecad-port/FreeCAD/src/Gui/WasmGLFixedFunc.cpp SUMMARY: ff_setup_and_draw reuses a stale F._maxIndex when a client normal/color array is drawn with an indexed draw but the vertex array is a VBO. DETAIL/EVIDENCE: WasmGLFixedFunc.cpp: _maxIndex is only recomputed when (isElements && vertClient) (lines 172-183, where vertClient requires the VERTEX array to be client memory). The client-array gather at line 149 uses maxV = F._maxIndex+1 for ANY client array (normal/color). If vertex is a VBO but normal or color is a client array in an indexed draw, _maxIndex is not refreshed this call and a stale value from a prior draw sizes the gather — under-sizing drops vertices, over-sizing reads past the client buffer. Coin typically keeps all arrays in the same residency so this is an edge case, but it is a real correctness gap. RECOMMENDATION: Compute _maxIndex whenever isElements is true and ANY enabled array is client-side (not just the vertex array), or derive per-array bounds. Independently verify by reading the actual file(s) and running git/grep as needed (repo at /home/magik6k/lcad-wasm/freecad-port/FreeCAD, toolchain at /opt/toolchains, deploy at /home/magik6k/lcad-wasm/freecad-port/deploy-gui). Default to REFUTED if you cannot confirm the evidence. Adjust severity to what the evidence actually supports.
Fable
thinking
I'll check the file to confirm that.
Fable
⚙ Read {"file_path": "/home/magik6k/lcad-wasm/freecad-port/FreeCAD/src/Gui/WasmGLFixedFunc.cpp", "limit": 220}
tool result
1	/* SPDX-License-Identifier: LGPL-2.1-or-later
2	 *
3	 * WebAssembly build only. A compact fixed-function OpenGL 1.x emulation over
4	 * WebGL2/GLES3, so Coin3D (which renders FreeCAD geometry with legacy GL) can
5	 * draw into the WebGL2 context Qt hands us. Emscripten's LEGACY_GL_EMULATION
6	 * is WebGL1-only and conflicts with Qt's forced WebGL2, so we implement the
7	 * subset Coin actually exercises ourselves.
8	 *
9	 * Design: the legacy entry points are C functions here; the state and dr  …[truncated]
Fable
thinking
Looking at the issue more carefully, `_maxIndex` only gets recomputed under specific conditions at line 172 when both `isElements` and `vertClient` are true, meaning the vertex array must be in client memory. But the gather operation at line 149 uses `_maxIndex` for any client-side array through the `bind` function, which gets called whenever an array isn't GPU-backed—so it applies to vertex, normal, and color arrays regardless of where the vertex data lives.

This creates the stale value problem: if the vertex array is a VBO (GPU-backed), the recomputation block never runs and `_maxIndex` keeps whatever value it had from a previous draw call. Yet if normal or color arrays are client-side, t  …
Fable
⚙ Bash {"command": "grep -n \"_maxIndex\\|_count\\|glbuf\" /home/magik6k/lcad-wasm/freecad-port/FreeCAD/src/Gui/WasmGLFixedFunc.cpp"}
tool result
140:    if (spec.glbuf) {
141:      g.bindBuffer(g.ARRAY_BUFFER, spec.glbuf);
149:      const maxV = isElements ? F._maxIndex+1 : nVerts;
169:  const vertClient = F.arrays.vertex.on && !F.arrays.vertex.glbuf;
174:    F._maxIndex = 0;
179:      F._maxIndex = F.arrays.vertex._count ? F.arrays.vertex._count-1 : count-1;
181:      for (let i=0;i<count;i++){ const x=iheap[(idxPtr/div)+i]; if(x>F._maxIndex)F._maxIndex=x; }
357:  spec.glbuf = g ? g.getParameter(g.ARRAY_BUFFER_BINDING) : null;
Fable
thinking
So `_maxIndex` only gets set when both vertex data is in a VBO and the normal/color data is client-side, which means in indexed draws with mixed storage types, that value stays stale or undefined and causes the gather to size incorrectly. Now I'm looking at the per-array residency logic around line 357 to confirm that arrays can independently be stored as either client data or VBOs.
Fable
⚙ Read {"file_path": "/home/magik6k/lcad-wasm/freecad-port/FreeCAD/src/Gui/WasmGLFixedFunc.cpp", "limit": 15, "offset": 345}
tool result
345	/* ---- client vertex arrays ---- */
346	EM_JS(void, ffClientState, (GLenum arr, int on), {
347	  const A=globalThis.__ff.arrays;
348	  if(arr===0x8074)A.vertex.on=!!on; else if(arr===0x8075)A.normal.on=!!on;
349	  else if(arr===0x8076)A.color.on=!!on;
350	})
351	EM_JS(void, ffPointer, (int which, GLint size, GLenum type, GLsizei stride, GLintptr ptr), {
352	  const F=globalThis.__ff; const A=F.arrays; const g=F.gl();
353	  const spec = which===0?A.vertex:which===1?A.normal:A.color;
354	  sp  …[truncated]
Fable
thinking
Each array's `glbuf` gets snapshotted independently when the pointer is set, based on whatever ARRAY_BUFFER was bound at that moment, so residency is tracked per-array. This means vertex data could be in a VBO while normal or color data remains client-side, or any other combination. I'm tracing through the code to confirm this scenario: when doing an element draw with the vertex buffer set to a VBO, `vertClient` becomes false, which skips the recomputation block at line 172, leaving `_maxIndex` stale.

Then when normal or color data is client-side, the bind operation takes the client path and uses that stale `_maxIndex` value. If `_maxIndex` was never set initially, it's undefined, which bec  …
Fable
The claim is accurate. Verified: - Line 149 (`maxV = isElements ? F._maxIndex+1 : nVerts`) sizes the client-array gather for ANY array taking the client path (`!spec.glbuf`), inside the shared `bind` helper called for vertex, normal, and color. - Line 169/172: `vertClient` requires the VERTEX array specifically to be client (`!F.arrays.vertex.glbuf`), and `_maxIndex` is recomputed only under `if (isElements && vertClient)`. - `grep` confirms `_maxIndex` is assigned nowhere else (only lines 174/179/181, all inside that guarded block). - Lines 351-357 (`ffPointer`) snapshot `glbuf` per-array from the currently bound `ARRAY_BUFFER`, so residency is independent per array — vertex can be a VBO while normal/color are client memory. So in an indexed draw where vertex is a VBO but normal or color is a client array, the guard is skipped and the gather at line 149 uses a stale (or `undefined` → NaN → zero-length) `F._maxIndex` from a prior call. Under-sizing drops vertices; over-sizing reads past the client buffer. Real gap, correctly bounded as an edge case since Coin usually keeps all arrays in the same residency. Low severity is appropriate.
Fable
⚙ StructuredOutput {"verdict": "CONFIRMED", "reason": "Code matches the claim exactly. The shared bind() helper (line 149) sizes the client-array gather with F._maxIndex+1 for ANY enabled client array (normal or color), but _maxIndex is recomputed only under …
tool result
Structured output provided successfully