Foundation Classes - Fix Resource_Manager::Debug and Storage_Schema current-data data races

[gsdali]

Description

Fixes #1398.

Resource_Manager::Resource_Manager(const char*, bool) (Resource_Manager.cxx:109) writes a
file-scope static bool Debug on every construction with zero synchronization. Two
Resource_Manager instances constructed concurrently on different threads (e.g. two independent
TDocStd_Application instances each lazily building their own via Resources()/DefineFormat())
race on this write. Fixed by making Debug a std::atomic<bool>: it is a plain process-wide
debug-logging flag, not per-instance intent, so an atomic is sufficient here.

Storage_Schema::ICurrentData() (Storage_Schema.cxx:802) is a function-local static
Handle(Storage_Data), shared by every Storage_Schema in the process and mutated with no
synchronization anywhere. Write() sets it for the duration of one store, and any
Storage_Schema construction calls Clear() -> ICurrentData().Nullify() in its constructor,
including throwaway instances built only to read header info
(PCDM_ReadWriter_1::ReadReferenceCounter, ReadReferences and ReadDocumentVersion, all reached
unconditionally from a plain TDocStd_Application::Open() via CDF_Application::Retrieve). One
instance's construction can therefore null out a different, unrelated instance's in-flight
Write() on another thread, or race a concurrent load's own throwaway construction.

Fixed by removing the global rather than guarding it, per @gkv311's review below: the handle
becomes a mutable Handle(Storage_Data) myCurrentData field on Storage_Schema, so there is no
shared state left to synchronize. Write() assigns the field, Clear() nullifies its own, and
HasTypeBinding()/BindType()/TypeBinding()/AddPersistent()/PersistentToAdd() read it
through *this. The private statics ICurrentData() and ISetCurrentData() are removed.
mutable because all of those methods are const.

Nothing in the tree relies on the state being process-wide:

• Every Storage_Schema is constructed locally by its caller and used only there
  (PCDM_StorageDriver::Write, and PCDM_ReadWriter_1 at three sites). None is cached or shared.
• Storage_CallBack::Add/Write/Read all take the driving schema as an argument, so the
  BindType/AddPersistent/PersistentToAdd calls made from per-type callbacks during a Write()
  run against that same instance.
• The other Storage_Schema users in the tree (BinLDrivers, XmlMDF, StdLDrivers) use only the
  statics CheckTypeMigration() and ICreationDate(), neither of which reads the current data.
• ICurrentData() and ISetCurrentData() were both private with no reference outside the class,
  so removing them is not an API change.

The same commit drops the static from AddPersistent()'s TCollection_AsciiString aTypeName, a
second unsynchronized process-wide object in the same class. It is assigned from the tName
argument and read two lines later, so static only ever saved an allocation, and it was previously
covered only incidentally by the mutex this revision removes.

Both races are invisible under the traditional single shared XCAFApp_Application /
TDocStd_Application usage pattern, because there Resources()'s own lazy-init mutex happens to
serialize Resource_Manager/Storage_Schema construction down to "runs once, ever, for the whole
process." They only become reachable once an application is constructed privately per caller or
thread (the pattern this project's own TDocStd_Application documentation recommends over the
XCAFApp_Application::GetApplication() singleton) and multiple independent instances are built
concurrently. See the linked issue for the full TSan writeup and a standalone reproducer.

No public API signature changes. The revision is a net reduction: 15 insertions, 31 deletions.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

ThreadSanitizer (minimal-module build: FoundationClasses + ModelingData + ModelingAlgorithms +
DataExchange, RelWithDebInfo, -fsanitize=thread -g), each round building a brand-new private
TDocStd_Application, defining formats, populating and saving a document, closing it, then a
separate brand-new private application opening, verifying and closing it. No two threads ever
share an application or schema instance.

• Before the fix: 13 TSan race warnings (2x Resource_Manager::Resource_Manager, 11x
  Storage_Schema::Storage_Schema's Clear() call) at 8 threads x 30 rounds, 0 functional
  failures.
• The mutex revision: 0 races across 4 runs (8x30, 8x50, 10x60, 8x40), 0 functional failures.
• This per-instance revision: 0 races across 8x50, 8x30 and 10x60, 0 save, load or verify failures.
  Re-run after the aTypeName change with the same result.

Also run against the wider concurrency suite that the downstream project maintains for this kernel
(10 scenarios covering the fixes from #1374, #1388, #1390, #1394 and #1397 plus this one): all
clean, no regression. Full downstream test suite (4666 tests) clean against a production build of
the patched kernel.

Reproducer and full writeup:
https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/374-resource-manager-storage-schema-race

Checklist:

• My code follows the code style of this project (clang-format --dry-run --Werror clean on
  every touched file)
• My change requires a change to the documentation
  • I have updated the documentation accordingly (no user-facing documentation covers this
    internal-only change)

[gkv311]

@gsdali I think Storage_Schema::ICurrentData could be moved to Storage_Schema as a mutable Handle(Storage_Data) myCurrentData; class field to get rid of this global variable.

[gsdali]

Done, and thanks for the pointer: ICurrentData()'s global is gone and Storage_Schema now carries mutable Handle(Storage_Data) myCurrentData instead. ICurrentDataMutex() and every lock_guard went with it, so the revision is a net 15 insertions / 31 deletions. Force-pushed onto the same branch.

I checked the assumptions the field relies on before changing it:

• every Storage_Schema in the tree is constructed locally by its caller and used only there (PCDM_StorageDriver::Write, and PCDM_ReadWriter_1 at three sites); none is cached or shared;
• Storage_CallBack::Add/Write/Read all take the driving schema as an argument, so the BindType/AddPersistent/PersistentToAdd calls that per-type callbacks make during a Write() run against that same instance;
• ICurrentData() and ISetCurrentData() were both private with no reference outside the class, so removing them is not an API change;
• the remaining Storage_Schema users (BinLDrivers, XmlMDF, StdLDrivers) touch only the statics CheckTypeMigration() and ICreationDate(), neither of which reads the current data.

It also fixes strictly more than the mutex did. Under the lock, a throwaway schema built by PCDM_ReadWriter_1 during an unrelated Open() still nullified an in-flight Write()'s current data, it just did so without a data race. With the field it cannot reach another instance's data at all.

One extra change rode along in the same commit, because removing the lock re-exposes it: AddPersistent() had a static TCollection_AsciiString aTypeName, a second unsynchronized process-wide object in the same class. It is assigned from the tName argument and read two lines later, so the static only ever saved an allocation, and it is now a local. Glad to move it to its own commit if you would rather keep this one to the field change alone.

ThreadSanitizer: 0 races at 8x50, 8x30 and 10x60 with the field, matching what the mutex revision gave, against 13 race warnings plus an abort on the unpatched kernel at 8x30. Re-run after the aTypeName change with the same result. The wider concurrency suite that the downstream project keeps for this kernel (10 scenarios, covering #1374, #1388, #1390, #1394, #1397 and this one) is clean, and a production build of the patched kernel passes the downstream test suite.

On whether to split: I have kept Resource_Manager and Storage_Schema in one PR, since they share one issue (#1398), one reproducer, and one precondition, in that both only become reachable once applications are constructed privately per caller rather than through the shared singleton. Now that they use two different techniques I am happy to split them into two PRs if you prefer; just say which way you want it.