Skip to content

[WIP][Cling] Refactor CIFactory and add incremental action support#21903

Draft
SahilPatidar wants to merge 1 commit intoroot-project:masterfrom
SahilPatidar:incr_action
Draft

[WIP][Cling] Refactor CIFactory and add incremental action support#21903
SahilPatidar wants to merge 1 commit intoroot-project:masterfrom
SahilPatidar:incr_action

Conversation

@SahilPatidar
Copy link
Copy Markdown

This Pull request:

This PR refactors CIFactory and adds support for IncrementalAction (from Clang-Repl). The goal is to reduce the amount of Clang-specific logic handled inside CIFactory and rely more on Clang’s existing infrastructure.

Changes or fixes:

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@devajithvs devajithvs self-assigned this Apr 13, 2026
@github-actions
Copy link
Copy Markdown

Test Results

     6 files       6 suites   1d 8h 23m 4s ⏱️
 3 770 tests  3 757 ✅ 0 💤 13 ❌
22 108 runs  22 033 ✅ 0 💤 75 ❌

For more details on these failures, see this check.

Results for commit 3dd5414.

@SahilPatidar
Copy link
Copy Markdown
Author

@vgvassilev
Most of the failing tests are related to Clad:

TEST FAILURES:
1:cppinterop-CppInterOpTests
185:gtest-hist-hist-TFormulaGradientTests
186:gtest-hist-hist-TFormulaHessianTests
271:gtest-math-mathcore-CladDerivatorTests
309:minuit2_testADMinim
330:gtest-roofit-histfactory-testHistFactory
350:gtest-roofit-roofitcore-testRooFuncWrapper
353:gtest-roofit-roofitcore-testRooMinimizer
361:test-stressroofit-codegen
426:tmva-sofie-test-TestCladAutodiff
430:gtest-tmva-sofie-TestGemmDerivative
922:tutorial-math-fit-exampleFit3D
945:tutorial-math-fit-minuit2GausFit

Some of them show errors like:

error: static assertion failed: clad doesn't appear to be loaded; make sure that you pass clad.so to clang.
        static_assert(false, "clad doesn't appear to be loaded; make sure that "
                      ^~~~~

This may be happening because I made rough changes in FrontendAction::CreateWrappedASTConsumer, where it now directly returns the DeclCollector from IncrementalAction::CreateASTConsumer without adding the plugin consumer.

Previously, I mentioned an issue related to the Clad plugin consumer. There was a problem with CladPlugin::initializeSema adding a SemaExternalSource, which I have resolved.

If we allow the plugin consumer, then during BeginSourceFile, when setASTConsumer is called, it triggers Initialize. This ends up stealing and replacing the CompilerInstance’s consumers. I’m not fully sure why this happens, but it appears to be causing many of the test failures.

In the original Cling implementation, as far as I can see, we never call Plugin::Initialize (so no replacement happens). Instead, we take the plugin consumer and pass it to the DeclCollector consumer.

We need to find a way (to pass plugin into DeclCollector or something else) to solve this first before moving forward.

@vgvassilev
Copy link
Copy Markdown
Member

When clad is OFF we should not run these tests. This means that we need to suppress them in cmake. cc: @guitargeek

Are the related llvm/clang changes really required? I think we should do extra effort to avoid them..

@SahilPatidar
Copy link
Copy Markdown
Author

Those are in CodeGenAction.cpp. On my local system, they were causing a segmentation fault. I’m not sure why, but after commenting them out, the crash stopped:

// PrettyStackTraceDecl CrashInfo(*D.begin(), SourceLocation(),
//                                Context->getSourceManager(),
//                                "LLVM IR generation of declaration");

And another change related to CompilerInstance and SourceManager initialization. As I mentioned, in Cling we use one large virtual source file to keep source locations consistent.

For now, I added a temporary helper:

runIncrementalSourceMgrInitializer

@vgvassilev
Copy link
Copy Markdown
Member

Those are in CodeGenAction.cpp. On my local system, they were causing a segmentation fault. I’m not sure why, but after commenting them out, the crash stopped:

// PrettyStackTraceDecl CrashInfo(*D.begin(), SourceLocation(),
//                                Context->getSourceManager(),
//                                "LLVM IR generation of declaration");

And another change related to CompilerInstance and SourceManager initialization. As I mentioned, in Cling we use one large virtual source file to keep source locations consistent.

For now, I added a temporary helper:

runIncrementalSourceMgrInitializer

Sounds suspicious. Maybe we should run valgrind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants