Skip to content

Support LLVM 23 - #1098

Open
Vipul-Cariappa wants to merge 3 commits into
compiler-research:mainfrom
Vipul-Cariappa:dev/llvm23
Open

Support LLVM 23#1098
Vipul-Cariappa wants to merge 3 commits into
compiler-research:mainfrom
Vipul-Cariappa:dev/llvm23

Conversation

@Vipul-Cariappa

Copy link
Copy Markdown
Collaborator

This PR enables CppInterOp to be used with LLVM 23.

Review each commit separately; that might be easy. Reading the commit message for the diagnostics change would make it easy to review.

Note: CppJIT currently fails with LLVM 23; it requires llvm/llvm-project#218149. Otherwise, CppJIT itself can be patched temporarily until the LLVM PR lands.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.52%. Comparing base (9bbdbb2) to head (09fb8c8).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
- Coverage   87.74%   87.52%   -0.22%     
==========================================
  Files          23       23              
  Lines        6429     6116     -313     
==========================================
- Hits         5641     5353     -288     
+ Misses        788      763      -25     
Files with missing lines Coverage Δ
lib/CppInterOp/Compatibility.h 84.54% <100.00%> (+2.36%) ⬆️
lib/CppInterOp/CppInterOp.cpp 90.59% <100.00%> (+0.04%) ⬆️
lib/CppInterOp/DynamicLibraryManagerSymbol.cpp 67.96% <ø> (-2.38%) ⬇️

... and 17 files with indirect coverage changes

Files with missing lines Coverage Δ
lib/CppInterOp/Compatibility.h 84.54% <100.00%> (+2.36%) ⬆️
lib/CppInterOp/CppInterOp.cpp 90.59% <100.00%> (+0.04%) ⬆️
lib/CppInterOp/DynamicLibraryManagerSymbol.cpp 67.96% <ø> (-2.38%) ⬇️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions


#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/ADT/SmallVector.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: included header STLForwardCompat.h is not used directly [misc-include-cleaner]

Suggested change
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/SmallVector.h"

// Print anonymous tags as a bare "(anonymous struct)" without the trailing
// source location. Clang 23 replaced the AnonymousTagLocations bit with the
// AnonymousTagMode enum stored in AnonymousTagNameStyle.
inline void SuppressAnonymousTagLocations(clang::PrintingPolicy& Policy) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "clang::PrintingPolicy" is directly included [misc-include-cleaner]

lib/CppInterOp/Compatibility.h:12:

- #if CLANG_VERSION_MAJOR < 21
+ #include <clang/AST/PrettyPrinter.h>
+ #if CLANG_VERSION_MAJOR < 21

#ifdef CPPINTEROP_VALUE_BOX_MOVE
auto* Payload = new detail::ValueRefCount(std::move(V));
#else
auto* Payload = new detail::ValueRefCount(V);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: initializing non-owner 'detail::ValueRefCount *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]

  auto* Payload = new detail::ValueRefCount(V);
  ^

clang::DiagnosticsEngine& fDiagEngine;
bool fOldDiagValue;
clang::DiagnosticConsumer* fOldClient;
std::unique_ptr<clang::DiagnosticConsumer> fOldOwnedClient;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: member variable 'fOldClient' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

sticConsumer* fOldClient;
              ^

bool fOldDiagValue;
clang::DiagnosticConsumer* fOldClient;
std::unique_ptr<clang::DiagnosticConsumer> fOldOwnedClient;
clang::IgnoringDiagConsumer fIgnoringClient;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: member variable 'fOldOwnedClient' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

onsumer> fOldOwnedClient;
         ^

std::unique_ptr<clang::DiagnosticConsumer> fOldOwnedClient;
clang::IgnoringDiagConsumer fIgnoringClient;
};
} // namespace

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: member variable 'fIgnoringClient' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

sumer fIgnoringClient;
      ^

Comment thread CMakeLists.txt
@@ -96,14 +96,14 @@ include(GNUInstallDirs)
## Define supported version of clang and llvm

set(CLANG_MIN_SUPPORTED 20.0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we now have a policy of only supporting the 3 most recent llvm versions. An llvm update PR is normally accompanied with a PR which drops the oldest llvm version.

Comment thread .github/workflows/main.yml Outdated
@@ -50,6 +50,7 @@ jobs:
# Ubuntu Arm
- { name: ubu24-arm-gcc12-llvm22-vg, os: ubuntu-24.04-arm, compiler: gcc-12, clang-runtime: '22', Valgrind: On }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably update the Valgrind jobs to llvm 23. I believe this may have been done in a separate PR though for the llvm 22 upgrade.

Comment thread .github/workflows/main.yml Outdated
Comment on lines 55 to 56
- { name: ubu24-x86-clang22-llvm22-asan-ubsan, os: ubuntu-24.04, compiler: clang-22, clang-runtime: '22', sanitizer: "Address;Undefined", flavor: asan }
- { name: ubu24-x86-clang22-llvm22-msan, os: ubuntu-24.04, compiler: clang-22, clang-runtime: '22', sanitizer: "Memory", flavor: msan }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will also want updating to llvm 23

@mcbarton mcbarton left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't update the Emscripten jobs, so we don't know if there has been any regressions there. There was between llvm 21 and 22, and we ended up disabling tests and just accepted the regressions.

@mcbarton

Copy link
Copy Markdown
Collaborator

I just realised the only job which has been updated to llvm 23 in the ci is the Ubuntu x86 job, so there is no check that nothing broke for llvm 23 on MacOS or Windows.

@anutosh491

Copy link
Copy Markdown
Collaborator

I think llvm 23 supports last value printing through clang-repl !

Maybe we should take care of last value printing being supported through cppinterop here ? (so that xeus-cpp can make use of it ?)

What say @Vipul-Cariappa ?

Silence diagnostic *output* by swapping in a discarding consumer rather than
setting SuppressAllDiagnostics. Suppression returns from
DiagnosticsEngine::ProcessDiag before ErrorOccurred/UnrecoverableErrorOccurred
are set, and both CodeGen (ModuleBuilder's HandleTopLevelDecl) and
IncrementalParser gate on those flags: with suppression on, an ill-formed
input is handed to IRGen anyway, which crashes on the placeholder types Sema
leaves in recovery ASTs (e.g. a '<dependent type>' RecoveryExpr from
`*p = new Unknown{};`), and the failed PTU is never cleaned up. Replacing the
client keeps the error accounting intact while printing nothing.

The new client-swap keeps the error accounting honest, CleanUpPTU runs and
CleanUpPTU erases the whole lookup entry for a namespace the failed input
re-opened (llvm/llvm-project#201844), because re-opening replaces the
predecessor in the enclosing lookup list. Every earlier declaration of that
namespace goes out of reach and the next re-opening quietly starts a fresh one.
@mcbarton

mcbarton commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@Vipul-Cariappa you didn't update the deployment ci to llvm 23. When you do that update the xeus-cpp branch to cppinterop-v2.0 like Vassil did to the Emscripten ci. That way CppInterOp will have a llvm 23 deployment when this PR goes in.

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