[rvec] replace malloc/free by operator new/delete

[jblomer]

Replaces #22460
Fixes #22449

[github-actions]

Test Results

    22 files      22 suites   3d 17h 40m 49s ⏱️
 3 886 tests  3 885 ✅ 0 💤 1 ❌
75 886 runs  75 885 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 448c793.

♻️ This comment has been updated with latest results.

[silverweed]

Could you please elaborate on why this change fixes the issue?
The wording in the linked Microsoft article is not very clear on C++-style memory allocation, but it does say:

A related problem can occur when you allocate memory (either explicitly with new or malloc, or [...]

Even the LLM's wording on the original issue is vague:

[...] although Microsoft documentation warns that even global new can cause issues if /MD is mismatched, in a uniform project environment where ROOT strictly mandates the dynamic CRT, C++ global ::operator new seems to ensure a more unified heap context under the process heap [...]

So my question is: did you find any docs on why using operator new/delete fixes the issue? Or is it just empirical?

(Same question for @bellenot who maybe is more familiar with Windows specifics)

[pcanal]

@bellenot Is it possible that the 'real' issue is that on Windows both ucrtbase and msvcrt are loaded and/or used inconsistently by the compiled and interpreted code? This PR seems effective at hiding the issue but could any code that use malloc/free.

[pcanal]

See #22986 (comment) (I.e. need to clearly document the decision on whether to fix this symptoms or to fix the underlying issue).

[bellenot]

@bellenot Is it possible that the 'real' issue is that on Windows both ucrtbase and msvcrt are loaded and/or used inconsistently by the compiled and interpreted code? This PR seems effective at hiding the issue but could any code that use malloc/free.

I don't know, and I don't see how I could figure out... All I can tell is that this PR fixes the issue and allow to re-enable something like 20 tests (or so) that are currently failing...

[wacfrr]

@bellenot Is it possible that the 'real' issue is that on Windows both ucrtbase and msvcrt are loaded and/or used inconsistently by the compiled and interpreted code? This PR seems effective at hiding the issue but could any code that use malloc/free.

I don't know, and I don't see how I could figure out... All I can tell is that this PR fixes the issue and allow to re-enable something like 20 tests (or so) that are currently failing...

Thanks for the suggestion. I gave this direction a try: I attempted to adjust the search-path priority — more specifically, in interpreter/cling/lib/Interpreter/IncrementalJIT.cpp, inside the Builder.setProcessSymbolsJITDylibSetup([&](LLJIT& J) -> Expected { callback, I manually insert dedicated generators for ucrtbase.dll / vcruntime140.dll before HostProcessLookup is added, so that JIT symbol lookup prefers the UCRT runtime:

    auto& JD = J.getExecutionSession().createBareJITDylib("<Process Symbols>");

#if defined(_WIN32)
    // Attempt: ucrtbase added first -> queried first (ORC generator chain:
    // first-added wins)
    auto UcrtLookup = RTDynamicLibrarySearchGenerator::Load(
        "C:\\Windows\\System32\\ucrtbase.dll", LinkerPrefix,
        [this] { return m_CurrentProcessRT; },
        [this](const SymbolStringPtr& Sym) {
          return !m_ForbidDlSymbols.contains(*Sym);
        });
    if (UcrtLookup) {
      JD.addGenerator(std::move(*UcrtLookup));
      errs() << "[DLM] generator: ucrtbase.dll (highest priority)\n";
    } else {
      logAllUnhandledErrors(UcrtLookup.takeError(), errs(), "ucrtbase gen: ");
    }

    // Also prioritize the compiler runtime helpers (new/delete, SEH)
    auto VcrLookup = RTDynamicLibrarySearchGenerator::Load(
        "C:\\Windows\\System32\\vcruntime140.dll", LinkerPrefix,
        [this] { return m_CurrentProcessRT; },
        [this](const SymbolStringPtr& Sym) {
          return !m_ForbidDlSymbols.contains(*Sym);
        });
    if (VcrLookup) {
      JD.addGenerator(std::move(*VcrLookup));
      errs() << "[DLM] generator: vcruntime140.dll\n";
    } else {
      logAllUnhandledErrors(VcrLookup.takeError(), errs(), "vcruntime140 gen: ");
    }
#endif

    // Process symbol resolution
    auto HostProcessLookup =
        RTDynamicLibrarySearchGenerator::GetForCurrentProcess(
            LinkerPrefix, [this] { return m_CurrentProcessRT; },
            [this](const SymbolStringPtr& Sym) {
              return !m_ForbidDlSymbols.contains(*Sym);
            });
    if (!HostProcessLookup) {
      return HostProcessLookup.takeError();
    }
    JD.addGenerator(std::move(*HostProcessLookup));

    // This must come after process resolution, to consistently resolve global
    // symbols (e.g. std::cout) to the same address.
    auto LibLookup = std::make_unique<RTDynamicLibrarySearchGenerator>(
        llvm::sys::DynamicLibrary(ExtraLibHandle), LinkerPrefix,
        [this] { return m_CurrentProcessRT; },
        [this](const SymbolStringPtr& Sym) {
          return !m_ForbidDlSymbols.contains(*Sym);
        });
    JD.addGenerator(std::move(LibLookup));
    return &JD;
  });

What I've observed so far: in df002_dataModel.C, free is indeed resolved to the ucrtbase.dll implementation (confirmed via instrumentation: msvcrt!free → ucrtbase!free), and the tutorial runs fine again — the crash caused by the cross-heap free no longer appears.

However — this is just a draft. I can't say for sure that this is a reasonable solution, and honestly there are several things I haven't fully thought through:

This simply puts ucrt first — it essentially works around msvcrt, rather than understanding and solving the more fundamental question of why the JIT prefers msvcrt in the first place;
The path C:\Windows\System32 is hardcoded — 32-bit processes, custom system directories, etc. are not covered;
It has only been validated on one tutorial; edge cases are completely untested — e.g. legacy code relying on msvcrt-only symbols, other library-loading paths, and interactions with other ROOT components;
I'm also not sure whether this change belongs in Cling (the generic interpreter layer) or in ROOT, and whether it should be made configurable.
So I'd like to hear everyone's thoughts: Is this direction itself reasonable? Or should we go back and look for a solution at a more fundamental level? Maybe we can discuss it together.

[bellenot]

@wacfrr I tried your proposed changes, and it breaks most of the tests...

[wacfrr]

@wacfrr I tried your proposed changes, and it breaks most of the tests...

Sorry. To clarify, my little experiment was only meant to show that another path might exist — but your test results show it breaks most of the tests, so maybe we should think carefully about how to approach this.

[jblomer]

@silverweed @pcanal The PR to me seems to be an improvement over the current situation, even if we are fixing only a symptom and not the cause. What needs to be changed to make it mergable?

[pcanal]

@jblomer My concern is that this shows a fundamental issue that will (in a weird way I sure) bite us later. I would prefer that, if there is time available for this, the root cause would be investigated. If not, then the next best thing I guess is to open an issue recording all we have learned and how to reproduce it so that it can be investigated later ....

[pcanal]

@wacfrr Out of curiosity what LLM are you using for this?

@wacfrr What is the extra information provided in #22986 (comment) that was not already discovered (2 conflicting libraries are being loaded from the compiled code and the interpreted code - but we did not pinpoint where/why they are both loaded).

[wacfrr]

@wacfrr Out of curiosity what LLM are you using for this?

@wacfrr What is the extra information provided in #22986 (comment) that was not already discovered (2 conflicting libraries are being loaded from the compiled code and the interpreted code - but we did not pinpoint where/why they are both loaded).

On the LLM:

For what it's worth, I used DeepSeek V4 Pro — mostly as a translation and formatting aid so I could focus on the investigation rather than the English phrasing. The actual debugging and reasoning was done by hand (windbg, dumpbin, Process Monitor, the instrumentation, etc.).

---

The main purpose of my previous comment was to share my current understanding and to provide a standalone reproducer (test_fakervec) that does not depend on df002_dataModel.C.

Based on the evidence I have, msvcrt.dll appears to come in indirectly through advapi32.dll — though this is not definitive. However, I would emphasize that pinpointing the exact importer, while valuable, has limited relevance to the fix itself — we cannot control which system DLL brings msvcrt into the process.

To address the root cause, I think the more actionable direction is to exclude msvcrt.dll from JIT symbol resolution, or at least lower its priority. It might be worth opening a separate issue on whether and how to exclude msvcrt.dll's symbols (or lower their priority) at JIT time.

Happy to hear your thoughts.

[silverweed]

@wacfrr please stop parroting the LLM by copy-pasting long and unhelpful messages in this PR. It is unwelcome behavior and explicitly violating the LLM policy of the ROOT project.
If you think you have useful information, distill them into succint sentences that are actionable.

[jblomer]

@pcanal @bellenot I created #23713

I don't think we have much diagnostics or a reproducer yet, but the ticket may serve as a reminder that there is a lurking problem.

Can this PR continue? I still believe that using C++ new/delete consistently is an improvement over mixing malloc and new for different objects.

[pcanal]

This is self consistent and we need to incorporate for the upcoming release, so let's merge. However to investigate the underlying issue (recorded at #23713) one probably need to revert this PR.