-
Notifications
You must be signed in to change notification settings - Fork 12
CU-2083 Update scip-clang to newer Clang version #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
| case Kind::NamespaceAlias: | ||
| case Kind::Global: | ||
| case Kind::Super: | ||
| case Kind::TypeSpecWithTemplate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeSpecWithTemplate was removed in LLVM 21 - the TypeSpec case now handles both template and non-template type specifiers
1ca46e8 to
41926df
Compare
| llvm::dyn_cast<clang::CXXRecordDecl>(baseType->getDecl()); | ||
| if (!baseRecord || !baseRecord->hasDefinition()) | ||
| continue; | ||
| auto baseResults = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CXXRecordDecl::lookupDependentName() was removed from the Clang API in LLVM 21. This is a local reimplementation with the same semantics.
indexer/Indexer.cc
Outdated
| auto symbol = optSymbol.value(); | ||
|
|
||
| if (functionDecl.isPure() || functionDecl.isThisDeclarationADefinition()) { | ||
| if (functionDecl.isPureVirtual() || functionDecl.isThisDeclarationADefinition()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPure() was renamed to isPureVirtual() in LLVM 21 for clarity.
| } | ||
| DERIVE_CMP_ALL(FileLocalSourceRange) | ||
| DERIVE_EQ_ALL(FileLocalSourceRange) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DERIVE_CMP_ALL now generates operator== via C++20 operator<=>, making DERIVE_EQ_ALL redundant and causing a compile error.
| auto end = llvm::sys::path::end(this->value); | ||
| for (auto it = start; it != end; ++it) { | ||
| if (it->equals(".") || it->equals("..")) { | ||
| if (*it == "." || *it == "..") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringRef::equals() was deprecated/removed in LLVM 21; use operator== instead.
| clang::CharSourceRange fileNameRange, | ||
| clang::OptionalFileEntryRef optFileEntry, clang::StringRef /*searchPath*/, | ||
| clang::StringRef /*relativePath*/, const clang::Module * /*importModule*/, | ||
| clang::StringRef /*relativePath*/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM 21 added moduleImported parameter to the PPCallbacks::InclusionDirective signature.
| @@ -0,0 +1,29 @@ | |||
| diff --git a/clang/lib/Tooling/Tooling.cpp b/clang/lib/Tooling/Tooling.cpp | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cherry-pick fix for CUDA assertion failure from llvm/llvm-project#173762. Can be removed once LLVM merges the fix.
| # LLVM 21 uses new naming convention: LLVM-VERSION-Platform.tar.xz | ||
| # with strip prefix LLVM-VERSION-Platform | ||
| grailbio_llvm_toolchain( | ||
| name = name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM 21 changed release archive naming from clang+llvm-VERSION-TRIPLE to LLVM-VERSION-Platform.
| #include "boost/interprocess/ipc/message_queue.hpp" | ||
| #pragma clang diagnostic pop | ||
|
|
||
| #include "llvm/ADT/Optional.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm::Optional was removed in favor of std::optional. This include is no longer needed.
|
|
||
| template <> | ||
| struct SpecializedBase<int> {}; | ||
| // ^^^^^^^^^^^^^^^ reference [..] SpecializedBase# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit template specializations (template <> struct SpecializedBase<int> and template <> struct DerivedFromSelf<int *>) no longer emit both a reference and a definition—now just a definition.
This is correct behavior. An explicit specialization defines a new complete type; it doesn't reference the primary template in the usage sense. A reference would be SpecializedBase<int> x; where you instantiate/use the type. LLVM 21 is more precise here.
88b71cb to
3480a7a
Compare
| fp<<<1, 1>>>(42); // expected-error {{must have void return type}} | ||
| // ^^ reference local 5 | ||
|
|
||
| g1<<<undeclared, 1>>>(42); // expected-error {{use of undeclared identifier 'undeclared'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the line g1<<<undeclared, 1>>>(42);, when the kernel configuration contains an undeclared identifier (undeclared), Clang 21 no longer generates a DeclRefExpr for the callee function g1
3480a7a to
0fb5ccf
Compare
.bazelrc
Outdated
| common --enable_platform_specific_config | ||
|
|
||
| # Disable bzlmod for now - WORKSPACE dependencies not yet migrated | ||
| common --enable_bzlmod=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to bazel 7 but we still use WORKSPACE not modules
0fb5ccf to
329783f
Compare
| # Based on release notes from since we're on Bazel < 7 | ||
| # https://github.com/bazel-contrib/toolchains_llvm/releases/tag/0.10.3 | ||
| build --features=-supports_dynamic_linker | ||
| build --features=-libtool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use bazel 7+ so no longer needed
| build --incompatible_enable_cc_toolchain_resolution | ||
|
|
||
| build --repo_env BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 | ||
| build --@llvm_zstd//:llvm_enable_zstd=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use bazel 7+ so no longer needed
faff6c7 to
235fa63
Compare
ea1763e to
3333e18
Compare
jupblb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Release workflow triggered for this PR failed. This must be fixed before the PR can be merged. That said, the error doesn't seem to be caused by the code changes ("No space left on device," and I was able to build scip-clang manually on both Linux and macOS).
While not a hard requirement right now, in the future it would be nice to split such PRs into separate scopes (in this case, I imagine something like a Bazel update and a Clang update).
Overall, this looks good to me. Once we have the Release pipeline working, this should be easy to merge.
| @@ -1,3 +1,7 @@ | |||
| # Disable bzlmod since this project uses WORKSPACE | |||
| # Docs: https://bazel.build/external/migration#disable-bzlmod | |||
| common --noenable_bzlmod | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be moved to the root .bazelrc. We probably don't want MODULE.bazel to be autogenerated in any case.
| rosetta steps | buildkite-agent pipeline upload | ||
| agents: | ||
| queue: aspect-small-ubuntu-lts | ||
| # - label: "build and test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this?
| static int zero_int = zero<int>; | ||
| // ^^^^^^^^ definition [..] i/zero_int. | ||
| // ^^^^ reference [..] i/j/zero. | ||
| // ^^^^ reference [..] i/zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a regression to me.
Amp-Thread-ID: https://ampcode.com/threads/T-019b7003-fbd1-73b0-84e1-30b1483d9aed Co-authored-by: Amp <[email protected]>
Amp-Thread-ID: https://ampcode.com/threads/T-019b702d-0fc2-77ed-83b3-ba3cb6095db7 Co-authored-by: Amp <[email protected]>
LLVM 21 now wraps template names in QualifiedTemplateName more often, even when there's no explicit qualification in the source. The indexer was not handling this case, causing template name references to be silently dropped in base class specifiers like 'struct Derived : Base<T>'. This commit unwraps QualifiedTemplateName (and DeducedTemplateStorage) at the start of saveTemplateSpecializationTypeLoc to get the underlying template before processing.
When looking up member names via this-> in template bodies, the previous code only handled RecordType base classes. LLVM 21 represents dependent base types like T0<T> as TemplateSpecializationType, which was being skipped. Add tryFindDeclForBaseType helper to handle: - Direct RecordType (non-dependent bases) - InjectedClassNameType (inside template definitions) - TemplateSpecializationType (dependent bases like T0<T>) This restores proper reference emission for inherited member calls.
LLVM 21 binaries require glibc 2.34+ which is not available on older CI agents (Ubuntu 20.04 has glibc 2.31). Fall back to system clang-format if the LLVM toolchain version is not compatible.
ed33a2f to
5aab1f5
Compare
5aab1f5 to
2d95b5c
Compare
Update scip-clang to newer Clang 21 version