Repository navigation
[Clang] Support libstdc++ workarounds when using -E - #210802
Conversation
|
@llvm/pr-subscribers-clang Author: Sirraide ChangesWe have a libstdc++ workaround in place to address a hack in libstdc++15 that is used in the definition of Whether this workaround (and others that address libstdc++ hacks) is active depends on the value of In case of this particular workaround, this results in To resolve this problem, this patch enables the workaround irrespective of whether I don’t think this has much potential for causing us issues, and conversely, it enables us to support separate preprocessing and compilation with the affected versions of libstdc++15. Other workarounds are left untouched by this as enabling some of them unconditionally may have broader implications that I haven’t investigated, and moreover, all of them apply to libstdc++ versions from 2016 or earlier, so I don’t believe that enabling these workarounds ten years after the fact would really help anyone... Fixes #160314. Full diff: https://github.com/llvm/llvm-project/pull/210802.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
index 5a07e77076d17..045aac959140d 100644
--- a/clang/docs/ReleaseNotes.md
+++ b/clang/docs/ReleaseNotes.md
@@ -323,6 +323,10 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the
- Fixed a crash when a using-declaration naming an unresolvable member of a
dependent base was shadowed by an invalid using-declaration. (#GH209427)
+- A workaround that was introduced to fix an issue with the `<format>` header present in some versions of
+ libstdc++15 has been extended to support preprocessed input. Previously, splitting the preprocessing and
+ compilation step would result in the fix not being applied. (#GH160314)
+
#### Bug Fixes to AST Handling
- Fixed a non-deterministic ordering of unused local typedefs that made
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 643392833759d..7268a9aa5b4ef 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4586,16 +4586,10 @@ static bool IsLibstdcxxStdFormatKind(Preprocessor &PP, VarDecl *Var) {
!Var->getDeclContext()->isStdNamespace())
return false;
- // Checking old versions of libstdc++ is not needed because 15.1 is the first
- // release in which users can access std::format_kind.
- // We can use 20250520 as the final date, see the following commits.
- // GCC releases/gcc-15 branch:
- // https://gcc.gnu.org/g:fedf81ef7b98e5c9ac899b8641bb670746c51205
- // https://gcc.gnu.org/g:53680c1aa92d9f78e8255fbf696c0ed36f160650
- // GCC master branch:
- // https://gcc.gnu.org/g:9361966d80f625c5accc25cbb439f0278dd8b278
- // https://gcc.gnu.org/g:c65725eccbabf3b9b5965f27fff2d3b9f6c75930
- return PP.NeedsStdLibCxxWorkaroundBefore(2025'05'20);
+ // We don't check the libstdc++ version for this one since that doesn't work
+ // if a user preprocesses the input as a separate step (because __GLIBCXX__
+ // will not be defined).
+ return true;
}
} // end anonymous namespace
diff --git a/clang/test/SemaCXX/libstdcxx_format_kind_hack.cpp b/clang/test/SemaCXX/libstdcxx_format_kind_hack.cpp
index 35611c870b8d1..698b6ccf697e6 100644
--- a/clang/test/SemaCXX/libstdcxx_format_kind_hack.cpp
+++ b/clang/test/SemaCXX/libstdcxx_format_kind_hack.cpp
@@ -1,4 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -std=c++23 -verify %s
+// RUN: %clang_cc1 -E -std=c++23 %s -o %t.ii
+// RUN: echo '// expected-no-diagnostics' >> %t.ii
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -verify %t.ii
// expected-no-diagnostics
|
|
Concretly, this is an issue for us (NVIDIA) in NVCC: during the host compilation pass, we first invoke the host compiler (in this case Clang) to preprocess the CUDA C++ input file, then run some transformations on the preprocessed output, before finally passing the result of that back to Clang. For the reasons described above, this results in There is no good way for NVCC to fix this since we don’t know the value of Also, the fact that we have an open issue for this (#160314) suggests that there are probably other users for whom separate preprocessing is a useful mode that we should support. |
|
Maybe a stupid suggestion, but could we translate (See also #206201, a slightly different flavor of funny preprocessor interactions with libstdc++.) |
|
The flip side is that if any implementation of the standard library touches that declaration incorrectly, neither gcc not clang would warn. I do not think this is really acceptable. I wonder if -E should preserve some constants ( version of clang, glibc, msvc and other things that are useful for diagnostics or that sort of workaround). Because this is not the only place which checks for the value of a macro |
I mean, I considered that too; that would also be an option, but do we want pragmas in the output of |
I'd be opposed to that; we already have options for
Now that's an idea we hadn't thought of -- |
Ah, I didn’t know that; in that case I agree a pragma is probably the best (or I guess, least bad?) solution |
|
I’ve introduced a new pragma for this and updated the pr description to describe how it works |
-E
|
|
||
| #pragma clang glibcxx_version // expected-error {{expected integer after '#pragma clang glibcxx_version'}} | ||
| #pragma clang glibcxx_version foo // expected-error {{expected integer after '#pragma clang glibcxx_version'}} | ||
| #pragma clang glibcxx_version 100000000000000000000000000000 // expected-error {{expected integer after '#pragma clang glibcxx_version'}} |
There was a problem hiding this comment.
Can we get a valid -E test here (with filecheck) so we can see the pragma printed correctly in the output file?
There was a problem hiding this comment.
The other test checks for that already (there is a file check line in there)
There was a problem hiding this comment.
(Also, all the pragmas in this test here are invalid, so they’re not printed)
| } | ||
|
|
||
| void PragmaGLIBCXXVersion(SourceLocation Loc, std::uint64_t Value) override { | ||
| First->PragmaGLIBCXXVersion(Loc, Value); |
There was a problem hiding this comment.
For those wondering WTF is going on here :D This is the 'chained PP Callbacks' thing that lets you do multiple PP callback thingies wrapped in 1.
| /// MacroDefined - This hook is called whenever a macro definition is seen. | ||
| void PrintPPOutputPPCallbacks::MacroDefined(const Token &MacroNameTok, | ||
| const MacroDirective *MD) { | ||
| bool ShouldEmitDefine = true; |
There was a problem hiding this comment.
It isn't clear to me what we're doing here? This change and the one in PrintMacroDefinition seem odd? What am I missign?
There was a problem hiding this comment.
This handles printing macro definitions (if doing that is enabled); basically, the idea here is that even if we don’t want to print macro definitions, we still need to emit a pragma for __GLIBCXX__ when we encounter a #define for it (and conversely, if we’re emitting the #define anyway, we don’t need the pragma).
The changes to PrintMacroDefinition are so we can print just the expansion of __GLIBCXX__ without printing the leading #define (since we want to print #pragma clang glibcxx_version instead)
| // | ||
| // RUN: %clang_cc1 -fsyntax-only -std=c++23 -verify %s -DDEFINE | ||
| // RUN: %clang_cc1 -E -std=c++23 %s -o %t.ii -DDEFINE | ||
| // RUN: echo '// expected-no-diagnostics' >> %t.ii |
There was a problem hiding this comment.
Oh boy, we're doing some particularly werid things in these check lines. I think you're being 100x too clever or something, it isn't clear what is being tested, and we probably shouldn't do it like this.
There was a problem hiding this comment.
I mean, I feel like I’ve seen similarly complicated RUN lines all over the place in the test suite: here, we check (not exactly in that order):
- That defining
__GLIBCXX__enables the workaround. - That using the pragma enables the workaround.
- That the pragma is retained when
-Eis passed. - That we emit a pragma at the point where
__GLIBCXX__is defined when-Eis passed. - That parsing the output of
-Esucceeds in both of these cases.
I could rearrange the RUN lines a bit or add some more comments if that would help (or split out some of this into a separate file)
| // Preserve '__GLIBCXX__' as a pragma if we shouldn't print '#define's; this | ||
| // is required for a number of workarounds in Sema (which are enabled | ||
| // depending on the value of this macro). | ||
| if (MacroNameTok.getIdentifierInfo()->getName() == "__GLIBCXX__") { |
There was a problem hiding this comment.
Instead of adding a special-case to -E, should we just make the preprocessor call the pragma handler when it sees __GLIBCXX__?
There was a problem hiding this comment.
Instead of adding a special-case to -E, should we just make the preprocessor call the pragma handler when it sees
__GLIBCXX__?
As in call PPCallbacks::PragmaGLIBCXXVersion() when we encounter #define __GLIBCXX__? That would also work I think; we’d have to parse the expansion of the macro to see if it contains an integer literal, but we do that anyway for the workaround in Sema, so that’d just be moving that logic there
|
I like this approach a lot, in that in think the approach is workable. can we do |
@Sirraide and I talked about that approach and I would really prefer to avoid it because it opens up all kinds of messy problems that require careful thought. e.g., do we also then need an undefine because users can define and undefine the macros? what happens if the user does We convinced ourselves that it makes more sense to do a non-general solution here because we want this to stay in the realm of "implementation detail hack" and not "something users should use themselves". WDYT? |
I personally definitely fear we're going to end up with something else like this again and just have a ton of extra code. However, I see the value here as effectively/exclusively a "version number capture/hackery". I wonder if we could do the name there obviously needs work, but the idea is: Only allow names (perhaps even 'reserved' named?) plus an integer literal? IF we decide to do more than 2-3 of these, I'd desperately want us to have some sort of 'list generated' version of these on the name when we see a |
|
@AaronBallman to be clear, I am not saying we should expose all defines that way (we really should not) just that we know we have at least 5-10 of these |
AFAICT, the only issue we have is with |
Its a bit of a Hyrem's law for me? I'd like to avoid having this be a special-case of special-case that we have to keep around forever because someone discovered this/shipped it on their platform (which could very reasonably happen as offload vendors might preprocess it to have as a "stable" version of the Standard Library" for device side?). So a more general SPELLING would be appreciated (even if we just have a "How DARE You spell anything but |
Is that something we actually support though? I think we don't (meaningfully) support separate compilation like that because we run the preprocessor always, including on code that came originally from
I won't block a more generalized spelling, but I would probably put my foot down for generalizing it enough that it looks like a user-facing feature. e.g., naming it |
I mean... we already DO? A At least SYCL shipped some preprocessed code for a while (as well as all of the offload models have some level of 'preprocess'). A few of the offload models have a "preprocess code with one compiler, then feed it to host and device compiler". Forcing their preprocessor to be aware of WHICH version of the hack-pragma we're using seems unnecessarily combative. I also don't care about the spelling as much as the fact that it is something that stays general and will be unchanged long-term. Name it `#pragma clang __I_better_not_catch_you_using_this_so_help_me_I_will_turn_this_compiler_around_and_we_can_just_eat_broccolli' for all I care. |
|
@AaronBallman there is a PR that needs something similar for msvc https://github.com/llvm/llvm-project/pull/194201/changes - and as i said earlier there are a few other places where we do things conditionally on macros being defined |
|
Right, Sirraide did not inject |
@cor3ntin To clarify, are you suggesting that the pragma should define |
|
It shouldn’t - exactly like you did, e.g just calling setStdLibCxxVersion is a good solution |
|
Ok, in that case should we generalise the spelling to something like |
That would be acceptable to me. |
I think that's reasonable. Will that be: or ? (Slight preference for the parenthesized form, but I don't think it's a requirement for anything) |
I was going to go w/ the latter since I don’t think we need the parentheses as delimiters here: we expect an identifier followed by a number, not arbitrary expressions or token sequences (and even if we did, it would just be everything until the end of the line after the identifier) |
I'd be fine with that. Actually, do we want something like: so that we could then support: instead of needing three sets of pragmas? I'm probably being too clever... :-D Any of these seem fine. |
I feel like we can just emit 3 pragmas instead; no-one should be writing these by hand anyway, so adding this feature would just make things harder to parse (and harder to generate if we actually wanted to use this for |
Agreed here, Aarons' suggestion is way too much work here. |
Heh, okay, sold. :-) |
|
Alright, I’ll implement this then |
erichkeane
left a comment
There was a problem hiding this comment.
This seems completely reasonable to me. Get 1 more +1 from the others, else LGTM.
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
| // Cached identifiers used to implement __set_pp_state. | ||
| IdentifierInfo *Ident__set_pp_state; | ||
| IdentifierInfo *Ident__GLIBCXX__; |
There was a problem hiding this comment.
This is neither needed nor used
There was a problem hiding this comment.
I see it being used; we need an IdentifierInfo * for the macro names we care about and we don't want to re-create the IdentifierInfo * each time so we cache it.
I'd rather these were grouped with the rest of them towards the top of the class though.
There was a problem hiding this comment.
See my comment below. we are gonna parse that once per TU
There was a problem hiding this comment.
True, we're not going to parse it often, but I don't see what the problem is with following the usual pattern of comparing IdentifierInfo *, either. I don't insist one way or the other.
There was a problem hiding this comment.
See my comment below. we are gonna parse that once per TU
That is true, but we do the comparison against __GLIBCXX__ every time we print a #pragma in -E mode; whether we care about that, I don’t know; we can probably get rid of Ident__set_pp_state though
There was a problem hiding this comment.
I'd rather these were grouped with the rest of them towards the top of the class though.
All of those are specifically predefined/builtin macros; we also have groups of cached IdentifierInfo*s elsewhere in the preprocessor, so I felt putting these here made more sense
There was a problem hiding this comment.
we do the comparison against
__GLIBCXX__every time we print a#pragmain-Emode
(via isPragmaSetPPStateMacro())
AaronBallman
left a comment
There was a problem hiding this comment.
One question: does this scheme work for modules and do we need any additional testing to ensure it works correctly there?
| // Cached identifiers used to implement __set_pp_state. | ||
| IdentifierInfo *Ident__set_pp_state; | ||
| IdentifierInfo *Ident__GLIBCXX__; |
There was a problem hiding this comment.
I see it being used; we need an IdentifierInfo * for the macro names we care about and we don't want to re-create the IdentifierInfo * each time so we cache it.
I'd rather these were grouped with the rest of them towards the top of the class though.
E.g. if you have the definition of C++ modules don’t do that though, so I don’t know what would happen if the standard library provided e.g. a |
We have a libstdc++ workaround in place to address a hack in libstdc++15 that is used in the definition of `std::format_kind`. GCC accepts the hack, while Clang does not. libstdc++ was eventually updated to remove the hack, but we still need the workaround for some versions of libstdc++15 (see llvm#139560 for more information). Whether this workaround (and others that address libstdc++ hacks) is active depends on the value of `__GLIBCXX__`. This stops working if someone first preprocesses the input (via `-E`) and then attempts to compile the preprocessed code with Clang (see llvm#160314): since preprocessing has already happened, `__GLIBCXX__` will be undefined and the workaround will not be applied. In case of this particular workaround, this results in `#include <format>` failing to compile on some versions of libstdc++ if you’re using separate preprocessing. To resolve this problem, this patch introduces a new pragma (`#pragma clang __set_pp_state MACRO_NAME INTEGER`). This is intended as a general solution to preserving preprocessor state across runs of the preprocessor. Currently, the only supported value for `MACRO_NAME`is `__GLIBCXX__`. When we encounter this form of the pragma, we update `Preprocessor::CXXStandardLibraryVersion` (we don’t actually define `__GLIBCXX__` or do anything w/ that macro here). In `-E` mode, the pragma is retained in the output, and a pragma is emitted for `#define __GLIBCXX__` if printing macro definitions is disabled. Things like `#undef __GLIBCXX__` and redefinitions of `__GLIBCXX__` are deliberately left unhandled (i.e. you get what you get), since users shouldn’t ever be undefining or redefining this macro anyway (and since it starts w/ `__`, it would also be UB to do so). Fixes llvm#160314.
| #pragma clang __set_pp_state void // expected-error {{invalid argument 'void' in '#pragma clang __set_pp_state'}} | ||
|
|
||
| #pragma clang __set_pp_state __GLIBCXX__ foo // expected-error {{expected integer after '#pragma clang __set_pp_state __GLIBCXX__'}} | ||
| #pragma clang __set_pp_state __GLIBCXX__ 100000000000000000000000000000 // expected-error {{expected integer after '#pragma clang __set_pp_state __GLIBCXX__'}} |
There was a problem hiding this comment.
Do we expect boolean, binary interer or hex integer literals to work?
Maybe check char and string literals.
nullptr?
There was a problem hiding this comment.
Well, currently, we always generate a decimal literal so that’s the only thing we need to support I think since this isn’t supposed to be user-facing anyway. Anything that’s not a numeric literal would be rejected here since that’s what we check for; I can add some tests for that if you’d like
There was a problem hiding this comment.
More tests here seems like a good thing.
We have a libstdc++ workaround in place to address a hack in libstdc++15 that is used in the definition of
std::format_kind. GCC accepts the hack, while Clang does not. libstdc++ was eventually updated to remove the hack, but we still need the workaround for some versions of libstdc++15 (see #139560 for more information).Whether this workaround (and others that address libstdc++ hacks) is active depends on the value of
__GLIBCXX__. This stops working if someone first preprocesses the input (via-E) and then attempts to compile the preprocessed code with Clang (see #160314): since preprocessing has already happened,__GLIBCXX__will be undefined and the workaround will not be applied.In case of this particular workaround, this results in
#include <format>failing to compile on some versions of libstdc++ if you’re using separate preprocessing.To resolve this problem, this patch introduces a new pragma (
#pragma clang __set_pp_state MACRO_NAME INTEGER). This is intended as a general solution to preserving preprocessor state across runs of the preprocessor.Currently, the only supported value for
MACRO_NAMEis__GLIBCXX__. When we encounter this form of the pragma, we updatePreprocessor::CXXStandardLibraryVersion(we don’t actually define__GLIBCXX__or do anything w/ that macro here). In-Emode, the pragma is retained in the output, and a pragma is emitted for#define __GLIBCXX__if printing macro definitions is disabled.Things like
#undef __GLIBCXX__and redefinitions of__GLIBCXX__are deliberately left unhandled (i.e. you get what you get), since users shouldn’t ever be undefining or redefining this macro anyway (and since it starts w/__, it would also be UB to do so).Fixes #160314.