Skip to content

Conversation

@zwuis
Copy link
Contributor

@zwuis zwuis commented May 12, 2025

#134522 triggers compilation error with libstdc++, in which primary variable template std::format_kind is defined like

template <typename R>
constexpr auto format_kind =
__primary_template_not_defined(
  format_kind<R>
);

See #139067 or https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120190.

This PR disables checking template id in initializer of primary variable template std::format_kind in libstdc++ (by checking __GLIBCXX__).

Fixes #139067

…le template `std::format_kind` with libstdc++
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 12, 2025
@llvmbot
Copy link
Member

llvmbot commented May 12, 2025

@llvm/pr-subscribers-clang

Author: Yanzuo Liu (zwuis)

Changes

Fixes #139067


Full diff: https://github.com/llvm/llvm-project/pull/139560.diff

1 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplate.cpp (+5)
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 7940340064eda..19142d7c16abb 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4383,6 +4383,11 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
     };
 
     if (VarDecl *Var = Template->getTemplatedDecl();
+        // Skipping std::format_kind in libstdc++ is a hack for
+        // GH139067 / https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120190
+        !(Var->getName() == "format_kind" &&
+          Var->getDeclContext()->isStdNamespace() &&
+          PP.isMacroDefined("__GLIBCXX__")) &&
         ParsingInitForAutoVars.count(Var) &&
         llvm::equal(
             CTAI.CanonicalConverted,

@cor3ntin cor3ntin requested a review from erichkeane May 12, 2025 17:04
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

Can you add a test for this too?

// GH139067 / https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120190
!(Var->getName() == "format_kind" &&
Var->getDeclContext()->isStdNamespace() &&
PP.isMacroDefined("__GLIBCXX__")) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if we could find some sort of way to check the actual VERSION of the hack once they fix this, but it doesn't look like they have a fix in flight.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clang you add something like // HACK workaround libstdc++ 14.x (2025-02) so it's easier to find?

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

Can you please describe the issue in the summary instead of just saying it fixes .... Also add details such as this is fixing an issue introduced by ...

Also please as Erich asked, add a test too

@zyn0217
Copy link
Contributor

zyn0217 commented May 13, 2025

@cor3ntin @erichkeane I question if a hack is really necessary here. We had run into a similar situation when we broke libstdc++ 14.1 (#92439), and the consensus then was to wait for the next dot release. I'd expect they'll fix this issue in the next dot release too?

@jwakely Will the next GCC release come before Clang 21, i.e. before September?

@cor3ntin
Copy link
Contributor

@zyn0217 At the very minimum, it breaks compiler explorer and anyone who uses trunk; it is disruptive

@cor3ntin
Copy link
Contributor

I think we should

  • 1/ Land this patch as is
  • 2/ Extract the parsing of __GLIBCXX__ in a separate function
  • 3/ have a function, probably in Preprocessor (so that it is available from Sema and Parser) such as IsLibStdCxxBefore(_version_) that let us do these checks uniformly and are easy to grep for. Ditto for libc++.

I don't have time at the moment but it would be a useful improvement :)

@erichkeane
Copy link
Collaborator

I think we should

* 1/ Land this patch as is

* 2/ Extract the parsing of `__GLIBCXX__` in a separate function

* 3/ have a function, probably in Preprocessor (so that it is available from Sema and Parser) such as `IsLibStdCxxBefore(_version_)` that let us do these checks uniformly and are easy to grep for. Ditto for libc++.

I don't have time at the moment but it would be a useful improvement :)

*4/ Do an audit of all our libcxx hackery in place right now, and apply the new function to it :D And perhaps instead of IsLibStdCxxBefore, be IsLibStdCxxBetween :D Even better if we make it possible to be 'open-ended' in cases where it hasn't been fixed/starts at the beginning of time.

@cor3ntin
Copy link
Contributor

@zwuis Can you land that? Thanks!

@zwuis
Copy link
Contributor Author

zwuis commented May 14, 2025

@cor3ntin I don't have commit access. Please help me land this.

By the way, should I request commit access?

@cor3ntin
Copy link
Contributor

@cor3ntin I don't have commit access. Please help me land this.

By the way, should I request commit access?

It might be a good idea ! https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

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

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[clang] fails to compile print("{}", 5s) when using libstdc++

6 participants