Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 6 additions & 6 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ let occtTarget: Target = useLocalBinary
name: "OCCT",
path: "Libraries/OCCT.xcframework"
)
// v1.10.1 rebuild: OCCT 8.0.0p1 + our carried patches — 0001 (ShapeFix_Face guard, #263) and
// 0002 (backport of upstream OCCT#1334, #280). Bump BOTH url and checksum whenever the
// xcframework is rebuilt, or URL-resolving consumers silently keep the previous kernel while
// local sibling builds get the new one.
// v1.12.3 rebuild: OCCT 8.0.0p1 + our carried patches — 0001 (ShapeFix_Face guard, #263),
// 0002 (backport of upstream OCCT#1334, #280), and 0003 (fillet TopOpeBRep thread_local, #298).
// Bump BOTH url and checksum whenever the xcframework is rebuilt, or URL-resolving consumers
// silently keep the previous kernel while local sibling builds get the new one.
: .binaryTarget(
name: "OCCT",
url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.10.1/OCCT.xcframework.zip",
checksum: "a1f816ea9b5308dafc9187933431b3f36793377ed7fa2567692305fac0255927"
url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.12.3/OCCT.xcframework.zip",
checksum: "9daee3da6f0655a927ee8deebfa1306b282f2f0da459b35383ef9c7de0555cc2"
)

let package = Package(
Expand Down
144 changes: 144 additions & 0 deletions Scripts/patches/0003-TopOpeBRep-non-reentrant-globals-fillet-298.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
From: OCCTSwift ecosystem <noreply@secondmouse.au>
Subject: [PATCH] Modeling - make BRepFilletAPI_MakeFillet reentrant across threads

Two independent BRepFilletAPI_MakeFillet builds, on independent shapes, running on
separate threads corrupt each other and produce a wrong-but-plausible solid that
fails BRepCheck -- silent bad geometry, no crash and no thrown error. This violates
OCCT's documented concurrency model ("algorithms on different threads in different
contexts"). The cause is file-scope / function-local `static` state on the fillet
path. ThreadSanitizer on an 8-thread fuse+fillet stress (V8_0_0_p1) pinpoints them.

FUNCTIONAL (causes the geometry corruption):
* TopOpeBRepBuild_Builder.cxx STATIC_SOLIDINDEX -- SplitSolid sets it to 1/2 to
tell FillSolid which operand it is splitting; FillSolid reads it back to pick the
operand shape. Concurrent solid reconstructions (ChFi3d_Builder::Compute ->
TopOpeBRepBuild_HBuilder::MergeSolid) interleave the writes and FillSolid
mis-classifies faces, dropping material. Fixing this one variable makes a stress
of 1600 concurrent fillet builds return correct geometry every time (was ~15-20%
corrupt).
* TopOpeBRep_kpart.cxx STATIC_lastVPind -- same cross-call-cache pattern
("the vp on which was computed the last CPI"), read back to index the VPoint
list; converted for the same reason.

BENIGN DATA RACES on the same path (no geometry corruption observed, but still UB;
converted so concurrent fillet is TSan-clean) -- the constant/evolutive-radius blend
solvers' per-parameter scratch cache and the ChFi3d curve checker's reused adaptor:
* BlendFunc_ConstRad.cxx, BlendFunc_EvolRad.cxx (ComputeValues scratch)
* ChFi3d_Builder_6.cxx (checkcurve)

All are converted to `static thread_local`, which preserves the exact single-thread
behaviour (each thread keeps its own copy of the cross-call state) while making
independent instances on separate threads reentrant. This is the same class of fix,
in the same engine, as Open-Cascade-SAS/OCCT#1180 (19 TKBool globals -> thread_local
across 8 files); STATIC_SOLIDINDEX and these were not covered by it.

Reported downstream as OCCTSwift #298.

diff --git a/src/ModelingAlgorithms/TKBool/TopOpeBRep/TopOpeBRep_kpart.cxx b/src/ModelingAlgorithms/TKBool/TopOpeBRep/TopOpeBRep_kpart.cxx
index 94b16fba..fbe26283 100644
--- a/src/ModelingAlgorithms/TKBool/TopOpeBRep/TopOpeBRep_kpart.cxx
+++ b/src/ModelingAlgorithms/TKBool/TopOpeBRep/TopOpeBRep_kpart.cxx
@@ -38,7 +38,13 @@ extern bool TopOpeBRep_GetcontextNEWKP();

// VP<STATIC_lastVPind> is the vp on which was computed the last CPI.
// if no CPI is computed yet, <STATIC_lastVPind> = 0.
-static int STATIC_lastVPind;
+// #298: `thread_local` (was plain `static`). This carries the "last VPoint index"
+// across calls and is read back to index the VPoint list — per-thread cross-call
+// state. Plain `static` shared it across threads, so concurrent intersections
+// (reachable from concurrent fillets via the TopOpeBRep engine) could read another
+// thread's index. Same functional non-reentrant-global class as STATIC_SOLIDINDEX;
+// fixed defensively (the audit found it, though the #298 repro exercised SOLIDINDEX).
+static thread_local int STATIC_lastVPind;

#define M_FORWARD(st) (st == TopAbs_FORWARD)
#define M_REVERSED(st) (st == TopAbs_REVERSED)
diff --git a/src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_Builder.cxx b/src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_Builder.cxx
index d866d860..55a7b5b6 100644
--- a/src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_Builder.cxx
+++ b/src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_Builder.cxx
@@ -70,7 +70,15 @@ Standard_EXPORT void debspf(const int i)
}
#endif

-static int STATIC_SOLIDINDEX = 0;
+// #298: `thread_local` (was plain `static`). SplitSolid sets this to 1/2 to tell
+// FillSolid which of the two operands it is currently splitting, and FillSolid
+// reads it back. Plain `static` shared that mode flag across threads, so two
+// concurrent solid reconstructions (e.g. two independent fillets, whose
+// ChFi3d_Builder::Compute reconstructs via TopOpeBRepBuild::MergeSolid) clobbered
+// each other's index and mis-classified faces — silent wrong geometry, no crash.
+// Per-thread storage keeps the single-thread flow identical. Matches the TKBool
+// thread_local conversions already landed for this engine.
+static thread_local int STATIC_SOLIDINDEX = 0;

//=================================================================================================

diff --git a/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_ConstRad.cxx b/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_ConstRad.cxx
index 9b099dbc..5bff8677 100644
--- a/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_ConstRad.cxx
+++ b/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_ConstRad.cxx
@@ -138,12 +138,15 @@ bool BlendFunc_ConstRad::ComputeValues(const math_Vector& X,
const bool byParam,
const double Param)
{
- // static declaration to avoid systematic reallocation
-
- static gp_Vec d3u1, d3v1, d3uuv1, d3uvv1, d3u2, d3v2, d3uuv2, d3uvv2;
- static gp_Vec d1gui, d2gui, d3gui;
- static gp_Pnt ptgui;
- static double invnormtg, dinvnormtg;
+ // These persist across calls as a per-parameter cache (the t_OK / myX_OK guards
+ // below skip recomputation when T / X are unchanged and read them back), so they
+ // cannot be made function-local. Plain `static` shared that cache across threads,
+ // so concurrent fillet builds raced on it. `thread_local` keeps the caching while
+ // giving each thread its own copy. (#298 — matches the TKBool thread_local work.)
+ static thread_local gp_Vec d3u1, d3v1, d3uuv1, d3uvv1, d3u2, d3v2, d3uuv2, d3uvv2;
+ static thread_local gp_Vec d1gui, d2gui, d3gui;
+ static thread_local gp_Pnt ptgui;
+ static thread_local double invnormtg, dinvnormtg;
double T = Param, aux;

// Case of implicite parameter
diff --git a/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_EvolRad.cxx b/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_EvolRad.cxx
index faf25679..312350ab 100644
--- a/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_EvolRad.cxx
+++ b/src/ModelingAlgorithms/TKFillet/BlendFunc/BlendFunc_EvolRad.cxx
@@ -196,12 +196,15 @@ bool BlendFunc_EvolRad::ComputeValues(const math_Vector& X,
const bool byParam,
const double Param)
{
- // static declaration to avoid systematic realloc
-
- static gp_Vec d3u1, d3v1, d3uuv1, d3uvv1, d3u2, d3v2, d3uuv2, d3uvv2;
- static gp_Vec d1gui, d2gui, d3gui;
- static gp_Pnt ptgui;
- static double invnormtg, dinvnormtg;
+ // These persist across calls as a per-parameter cache (the t_OK / myX_OK guards
+ // below skip recomputation when T / X are unchanged and read them back), so they
+ // cannot be made function-local. Plain `static` shared that cache across threads,
+ // so concurrent fillet builds raced on it. `thread_local` keeps the caching while
+ // giving each thread its own copy. (#298 — matches the TKBool thread_local work.)
+ static thread_local gp_Vec d3u1, d3v1, d3uuv1, d3uvv1, d3u2, d3v2, d3uuv2, d3uvv2;
+ static thread_local gp_Vec d1gui, d2gui, d3gui;
+ static thread_local gp_Pnt ptgui;
+ static thread_local double invnormtg, dinvnormtg;
double T = Param, aux;

// Case of implicit parameter
diff --git a/src/ModelingAlgorithms/TKFillet/ChFi3d/ChFi3d_Builder_6.cxx b/src/ModelingAlgorithms/TKFillet/ChFi3d/ChFi3d_Builder_6.cxx
index b4d23e9a..106ff49d 100644
--- a/src/ModelingAlgorithms/TKFillet/ChFi3d/ChFi3d_Builder_6.cxx
+++ b/src/ModelingAlgorithms/TKFillet/ChFi3d/ChFi3d_Builder_6.cxx
@@ -641,7 +641,10 @@ bool ChFi3d_Builder::StoreData(occ::handle<ChFiDS_SurfData>& Data,
const bool Reversed)
{
// Small control tools.
- static occ::handle<GeomAdaptor_Curve> checkcurve;
+ // #298: `thread_local` (was plain `static`) — reused scratch adaptor shared across
+ // calls; plain `static` shared it across threads, racing concurrent fillet builds.
+ // Per-thread lazy allocation keeps the reuse while making it reentrant.
+ static thread_local occ::handle<GeomAdaptor_Curve> checkcurve;
if (checkcurve.IsNull())
{
checkcurve = new GeomAdaptor_Curve();
19 changes: 19 additions & 0 deletions Scripts/patches/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,22 @@ long-standing `cone()` failure in `StressFormatRoundTripTests`, which passed in
every full run because `OCCTIOTests` reads a STEP first.

**Retire** once the bundled OCCT moves past upstream `e2de4398ca6bf034074e6921599da76a9941c792`.

## 0003-TopOpeBRep-non-reentrant-globals-fillet-298.patch

**Fixes the upstream OCCT thread-safety defect behind [#298](https://github.com/SecondMouseAU/OCCTSwift/issues/298)** — concurrent `BRepFilletAPI_MakeFillet` builds on independent shapes corrupt each other.

`BRepFilletAPI_MakeFillet` reconstructs its result solid through the legacy `TopOpeBRepBuild` boolean engine (`ChFi3d_Builder::Compute` → `TopOpeBRepBuild_HBuilder::MergeSolid` → `TopOpeBRepBuild_Builder::SplitSolid`), which passes state between methods through **file-scope `static` variables**. That makes it non-reentrant: two fillet builds on separate threads produce a wrong-but-plausible solid that fails `BRepCheck` — silent bad geometry, no crash, no thrown error.

ThreadSanitizer on an 8-thread fuse+fillet stress (V8_0_0_p1) pinpoints the **functional** culprit as `STATIC_SOLIDINDEX` in `TopOpeBRepBuild_Builder.cxx`: `SplitSolid` sets it to 1/2 to tell `FillSolid` which operand it is splitting, and `FillSolid` reads it back to pick the operand shape. Concurrent reconstructions interleave the writes, so `FillSolid` mis-classifies faces and drops material (the ~260-unit volume loss in the repro). Fixing that one variable makes a stress of 1600 concurrent fillet builds return correct geometry every time (was ~15–20% corrupt).

The patch converts the fillet-path shared statics to `static thread_local` (each thread keeps its own copy of the cross-call state; single-thread behaviour is unchanged):

- **Functional** (cause the corruption): `STATIC_SOLIDINDEX` (`TopOpeBRepBuild_Builder.cxx`) and `STATIC_lastVPind` (`TopOpeBRep_kpart.cxx`, same cross-call-cache pattern, converted for the same reason).
- **Benign data races** on the same path (no geometry corruption observed — `STATIC_SOLIDINDEX` alone already gives correct geometry — but still UB; converted so concurrent fillet is TSan-clean): the constant/evolutive-radius blend solvers' scratch cache (`BlendFunc_ConstRad.cxx`, `BlendFunc_EvolRad.cxx`) and the ChFi3d curve checker's reused adaptor (`ChFi3d_Builder_6.cxx`).

This is the same class of fix, in the same engine, as [Open-Cascade-SAS/OCCT#1180](https://github.com/Open-Cascade-SAS/OCCT/pull/1180) (19 TKBool globals → `thread_local` across 8 files); `STATIC_SOLIDINDEX` and these were not covered by it.

**Validation:** the isolated pure-C++ repro (`fuse` two/three prisms → fillet the seam, 8 threads) returns BRepCheck-invalid solids across several wrong volumes on stock p1, and 0/1600 invalid with a single correct volume after the patch. ThreadSanitizer reports the `STATIC_SOLIDINDEX` and `BlendFunc` scratch races on stock p1 and is clean on the fillet path after the patch (only an unrelated benign `BOPAlgo_InitMessages` lazy-init race remains, in the boolean path — orthogonal). While this patch is carried, the in-wrapper `occtFilletMutex` shipped in v1.12.1 serialises fillet/chamfer builds so the corruption cannot reach consumers.

**Retire** once the bundled OCCT includes these `thread_local` conversions (upstream PR pending), at which point the `occtFilletMutex` guard can also be dropped.
7 changes: 0 additions & 7 deletions Sources/OCCTBridge/src/OCCTBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@
void OCCTSerialLockAcquire(void) { occtGlobalMutex().lock(); }
void OCCTSerialLockRelease(void) { occtGlobalMutex().unlock(); }

// #298: serialises 3D fillet/chamfer builds against each other (non-reentrant
// blend statics in OCCT's TKFillet). See OCCTBridge_Internal.h for the full why.
std::recursive_mutex& occtFilletMutex() {
static std::recursive_mutex mutex;
return mutex;
}

// Install OCCT's signal handlers once (issue #175). OSD::SetSignal(false) installs
// SIGSEGV/SIGBUS/SIGFPE handlers without enabling the FPE-trapping FP mask, so that
// signals raised inside OCCT become catchable via OCC_CATCH_SIGNALS instead of aborting
Expand Down
2 changes: 0 additions & 2 deletions Sources/OCCTBridge/src/OCCTBridge_Healing.mm
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,6 @@ OCCTShapeRef OCCTShapeFilletVariable(OCCTShapeRef shape, int32_t edgeIndex,
TopoDS_Edge edge = TopoDS::Edge(edgeMap(edgeIndex + 1)); // OCCT uses 1-based indexing

// Create fillet maker
std::lock_guard<std::recursive_mutex> _filletLock(occtFilletMutex()); // #298
BRepFilletAPI_MakeFillet fillet(shape->shape);

// Add edge with variable radius
Expand Down Expand Up @@ -668,7 +667,6 @@ OCCTShapeRef OCCTShapeBlendEdges(OCCTShapeRef shape,
TopExp::MapShapes(shape->shape, TopAbs_EDGE, edgeMap);

// Create fillet maker
std::lock_guard<std::recursive_mutex> _filletLock(occtFilletMutex()); // #298
BRepFilletAPI_MakeFillet fillet(shape->shape);

// Add each edge with its radius
Expand Down
26 changes: 4 additions & 22 deletions Sources/OCCTBridge/src/OCCTBridge_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,28 +186,10 @@ struct OCCTHistoryStorage {

std::recursive_mutex& occtGlobalMutex();
std::mutex& igesMutex();

// #298: 3D fillet/chamfer serialization lock.
//
// BRepFilletAPI_MakeFillet's constant- and evolutive-radius blend solvers
// (BlendFunc_ConstRad / BlendFunc_EvolRad) and the shared ChFi3d_Builder curve
// checker keep their geometric work variables in function-local `static`s ("to
// avoid systematic reallocation"). That makes the blend evaluation NON-REENTRANT:
// two threads filleting at once interleave writes to the same statics, the solver
// converges on a corrupted surface, and the result is a wrong-but-plausible solid
// that fails BRepCheck — silent bad geometry, not a crash or a thrown error.
// Reproduced in pure OCCT with no wrapper involved (issue #298).
//
// Hold this lock around every 3D fillet/chamfer Build(). It is DISTINCT from
// occtGlobalMutex(): booleans, meshing, and everything else stay fully parallel —
// only fillet/chamfer builds serialise against each other. Recursive so a fillet
// wrapper that nests another cannot self-deadlock. 2D fillets
// (BRepFilletAPI_MakeFillet2d) use the separate analytic ChFi2d toolkit with no
// such statics and are intentionally NOT guarded.
//
// This is a mitigation. The real fix de-statics those OCCT work variables
// (Scripts/patches/0003) so the lock can be dropped. See docs/thread-safety.md.
std::recursive_mutex& occtFilletMutex();
// #298: the fillet/chamfer serialization lock (occtFilletMutex) was removed in
// v1.12.3 — the underlying OCCT non-reentrancy (STATIC_SOLIDINDEX and the blend
// scratch) is now fixed in the pinned kernel via Scripts/patches/0003, so 3D
// fillet/chamfer builds are reentrant and no longer need serialising.

// === OCCT signal handling ===
//
Expand Down
Loading