[cling] The LookupHelper routines need the ROOT lock.

[pcanal]

Those routines are at the very least looking up information into Clang and thus need to prevent concurrent updates. Some of the routines can also sometimes induces changes in Clang (eg. template instantiation).

This fixes #18520 and #18519

[github-actions]

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit d70a2f4.

[dpiparo]

The change looks fine to me, thanks, provided that I understood correctly what was the role of the RAII (the header cannot be found)

[vgvassilev]

Why not locking the underlying interface in the LookupHelper?

[pcanal]

Why not locking the underlying interface in the LookupHelper?

Because one need to lock both the (potential) write access and the read accesses.

For example in GetPartiallyDesugaredNameWithScopeHandling we have:

   clang::QualType t = lh.findType(tname.c_str(), ToLHDS(WantDiags()));
   // Technically we ought to try:
   //   if (t.isNull()) t =  lh.findType(TClassEdit::InsertStd(tname), ToLHDS(WantDiags()));
   // at least until the 'normalized name' contains the std:: prefix.
   if (!t.isNull()) {

The lock needs to be held during both the 1st (because of potential modification by this thread) and the last line (because of potential modification by another thread)

We could improve the scaling further by leveraging the fact that the ROOT lock is a read-write lock but that level of granularity is not available from Cling yet.

[pcanal]

the role of the RAII (the header cannot be found)

The header is interpreter/cling/lib/Interpreter/EnterUserCodeRAII.h and the RAII does the lock taking and releasing as needed. The name of the RAII is nowadays a misnomer as it is now used past its original usage intent.

[vgvassilev]

Why not locking the underlying interface in the LookupHelper?

Because one need to lock both the (potential) write access and the read accesses.

Why the read access? Do you mean because read access sometimes mutates the AST?

[pcanal]

Why the read access? Do you mean because read access sometimes mutates the AST?

No. Because another thread might mutate the area you are reading for example:

thread 1 time 1: Take lock
thread 1 time 2:   Load header file with class A
thread 1 time 3:   Find class A
thread 1 time 4: Release lock
thread 1 time 7: Iterate through class A's content

and

thread 2 time 4: Take lock
thread 2 time 5:    Unload header file class A (revert Transaction)
thread 2 time 6: Release lock

Then during time 7, thread 1 is using data/memory that is either deleted or least no longer what it was meant to be.

As long as there is 'destructive' or changing operation left in Cling (eg. unloading transaction) then we do need to have at least a Read Lock when reading data.

The general rule we have been trying (and only incompletely succeeding) to always hold the ROOT lock when accessing Clang/Cling information.