[RFC] Default-on diagnostics for Implement P2843R3 - Preprocessing is never undefined

Clang Frontend
1

Hi,

During the implementation of C++26 P2843R3(https://wg21.link/P2843R3), we encountered several issues; specifically, a significant amount of existing code may rely on the undefined behaviors described in this paper. We are therefore seeking further input to ensure that, while adhering to the relevant standards, we can maintain the highest possible level of compatibility within Clang. Furthermore, we wish to use this discussion to alert Clang users to this situation, thereby providing them with sufficient time to smoothly address any potential issues in their code.

Background

P2843R3 (adopted for C++26) makes a number of previously-undefined preprocessor constructs IFNDR. WG21’s position, expressed on the reflectors, is essentially that because the constructs are IFNDR, implementations are free to do whatever they want – including nothing.

Clang has historically diagnosed most of these constructs under -pedantic. PR #192073 marked the feature implemented; #196989 re-landed it and, in addition, promoted most of those -pedantic warnings to default-on so that users in any language mode would catch real bugs (the constructs are also UB pre-C++26).

#196989 has now been reverted twice because of breakage in widely-used third-party code:

1. Windows SDK `WinBase.h`:
     #define M (_WIN32_WINNT >= 0x0502 || !defined(_WINBASE_))
     #if M ...
   -- macro expansion producing `defined`.

2. llvm-libc `__llvm-libc-common.h`, compiled in C++17:
     #undef _Noreturn
     #undef _Alignas
     #undef _Static_assert
     #undef _Alignof

3. libjpeg-turbo, compiled in C11:
     #undef inline

These are not bugs in Clang – the constructs really are UB / IFNDR – but they are pervasive in shipping headers and toolchains, and -Werror turns the warning into a hard build break.

We would like community feedback before re-landing.

Questions for the community

Q1. Default-on vs. -pedantic?

For each of the diagnostics promoted by P2843R3:

  • (a) defined produced by macro expansion in a conditional – already split into object-type (default-on, existing behavior) and function-type (-pedantic, existing behavior). #196989 promoted function-type to default-on. nico@ points out the function-type case is noisy and often unfixable in third-party code, which is why it was gated under -pedantic in the first place. Should we keep the function-type case under -pedantic?

  • (b) Preprocessing directive embedded in a function-like macro argument. Promote to default-on, or keep under -pedantic?

  • (c) #define / #undef of a name that is lexically identical to a keyword, special identifier, or standard attribute token. In particular, #undef keyword: this is dubious to diagnose because the preceding #define keyword ... is what created the UB, and the #undef only restores the keyword. Should the #undef case be default-off, or -pedantic, even when #define is default-on?

  • (d) #line 0, #line N for N > 2^31. P4136R2 (adopted as a DR against P2843R3) walks these back, so #196989 has already reverted the #line changes. We should land the P4136R2 status update separately.

  • (e) Reserved attribute identifiers. #196989 removes DefaultIgnore on warn_pp_macro_is_reserved_attribute_id. Should this be default-on or stay -pedantic?

Q2. Language-mode gating?

The IFNDR rule lives in C++26 wording, but the underlying UB has always existed (in C and earlier C++). Should the default-on policy be:

  • (a) on in all language modes (current PR direction), or
  • (b) default-on only in C++26 and later, -pedantic elsewhere,
  • (c) default-on in C++26, and downgraded to -pedantic only for constructs we have evidence are pervasive in real code (i.e., (a)-function-type and (c)-#undef)?

Q3. Warning group structure?

Today -Wkeyword-macro covers both #define keyword (which hides the keyword) and #undef keyword. Aaron and nico both suggested separating these so users can suppress one without losing the other. Concretely we propose:

   -Wkeyword-macro              (umbrella, default-on)
     -Wkeyword-macro-define     (#define hiding a keyword)
     -Wkeyword-macro-undef      (#undef of keyword/attr-token)

and analogously for the embedded-directive and defined-from-macro cases.

Q4. System headers.

The Windows SDK case is in a system header; Clang already suppresses these diagnostics there, so once libc / libjpeg-turbo are updated, the remaining default-on noise should be largely confined to user code. Is the SystemHeaders default sufficient, or do we want a -W...-in-system-headers re-enable for users who actually want to clean up vendor headers?

Proposed plan

Pending feedback…

Prior discussion

- PR  #192073 (initial implementation, reverted)
  https://github.com/llvm/llvm-project/pull/192073
- PR  #196989 (reapply, reverted)
  https://github.com/llvm/llvm-project/pull/196989
- WG21 P2843R3:  https://wg21.link/P2843
- WG21 P4136R2:  https://wg21.link/P4136R2 (DR against P2843R3)
- CWG 2575:      https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2575

Thanks!

1 Like
2

As a general principle, we want to warn when it’s helpful, and skip warning when it’s not. This is impossible to get right in general, but that’s the direction we want. I think it’s worth considering why these constructs are undefined in the first place.

For define/undef of keywords, the primary issue isn’t related to the preprocessor itself; it’s the interaction of the definitions with system headers. Users have no idea what system headers contain, so defining keywords can break headers (sometimes in non-obvious ways). But I’m not sure that warning by default is useful here; presumably nobody is doing this by accident.

For defined produced by a macro expansion, there isn’t really any reason to make it undefined; the original spec was just incomplete. The paper itself even suggests removing the undefined behavior.

1 Like
3

I would like to see the diagnostic be on by default for when the user uses #define of a reserved identifier because the failure mode there is really frustrating. e.g., #define const to remove const qualifiers, doing #define private public, etc are all games that I think should be discouraged by default. However, I don’t think #undef is the same severity and maybe that should be off by default. If the keyword was defined as a macro, the user would have gotten a diagnostic on the #define so giving a diagnostic on #undef seems chatty and if the keyword was never defined as a macro to begin with, the #undef has no effect anyway.

1 Like
4

Please feel free to offer wording to reconcile the implementation divergence for whether, during macro expansion, a defined that would result from macro expansion is the defined unary operator (and thus subject to the invocation form matching and also capable of suppressing macro expansion of its argument): Compiler Explorer, Compiler Explorer .

Clang, GCC, and the MSVC “conforming” preprocessor consider defined to not be the defined unary operator when expanding the argument to a function-like macro. EDG and the MSVC traditional preprocessor take the opposite interpretation.

5

My analysis of that case was mostly just based on reading the C++ standard paper, which seemed to indicate that the commonly used constructs are actually consistent across compilers. Is there some portable subset?

6

The object-macro cases would be a bit off-topic for this thread (IMO) as the diagnostic covering them were already default-on. I am not aware of a meaningful portable subset of those cases as I have not found a rationalization of the traditional MSVC behaviour for the case documented at llvm-project/clang/lib/Lex/PPExpressions.cpp at fadbc3fa86443ca8ce0359cd47c392317153f676 · llvm/llvm-project · GitHub.

For the function-like-macro cases, I do not have the boundary between “portable” and “non-portable” pinned down. Even the pattern in llvm-project/clang/lib/Lex/PPExpressions.cpp at fadbc3fa86443ca8ce0359cd47c392317153f676 · llvm/llvm-project · GitHub is not portable (despite what the code comment claims), even in the simplest usage (i.e., directly invoked from the controlling expression of #if): Compiler Explorer

7

Note that, for a few of this the wording was changed from UB to ill-formed no-diagnostic required. If any of these things are problematic the conservative approach is to do nothing. So to be conforming with the paper we only need to concerned about ill-formed case, which is mostly (c) in your example - I do believe this should be an error, at least in c++26 mode.

But there are multiple issues with your initial post.

Why are _Static_assert and all considered keywords in C++? they should not be. Why does the error applies to a C11 library? it probably should not be either.

If we make it a warning-defaulted to an error, i think it would not trigger in system headers… even if in this case it probably should for the sake of users.

8

I like Eli’s “is it useful?” framing.

The commit message of where I added the warning has some details: Add -Wexpansion-to-undefined: warn when using `defined` in a macro de… · llvm/llvm-project@b2348f4 · GitHub The object-type UB actually has differing behavior between MSVC and clang/gcc (or had, back then), and it’s possible to write it in a different way. For function-type UB, I’m not aware of it causing problems in practice, and I’m also not aware of a simple and mechanical way to rewrite macros expanding to function-style macros in a way to not trigger the diag.

In any case, it’d be nice if new warnings here were added in new warning subgroups, so that they can be toggled off without having to toggle off all warnings in the group they’re in.

9

… for code that has already been written. Having the compiler not warn increases the likelihood of new code being written where it would be a problem.

10

For precisely the defined macro in said commit message, Clang generates the result opposite to that of the traditional MSVC preprocessor ( Compiler Explorer ):

#define FOO(x) (defined __foo_##x && __foo_##x)
#define __foo_hello true

#define NOT(X) (!(X))

#if NOT(FOO(hello))
#error Clang says hello
#else
#error MSVC says hello
#endif
11

The Clang Area Team discussed this and we concluded this needs a bit more exploration.

We think a good principle is to diagnose as much as possible without being overly disruptive.

In particular:

  • #define-ing keyword is obviously something we should diagnose as an error but #undef-inig them is less harmful and a warning is probably sufficient.
  • We intentionally support some C keywords such as _Static_assert in C++. maybe we want to adopt a different, less restrictive behavior for these?

In short, there are more questions to answer and we are not yet ready to call the outcome of this RFC

12

Right, and that’s a function-style macro. That’s why we warn on those :slight_smile:

13

Not by default (unlike for object-style ones): https://godbolt.org/z/33vf6bhTh

14

Thanks for all your suggestions! Sorry for the very lay reply.

As a general principle, we want to warn when it’s helpful, and skip warning when it’s not.

We think a good principle is to diagnose as much as possible without being overly disruptive.

I‘m 100% agree with these principles.

Regarding the #define keyword:

#define-ing is obviously something we should diagnose as an error.

I’m a bit concern about the severity of diagnostics, because nobody is doing this by accident. I’ve ever debugging with #define const/#define private public before, and I’m not sure if others would do the same. Therefore, I wonder if we use a default warning would be better? This way, if the user clearly knows what they’re trying to do, clang won’t forcibly prevent them from doing so. What do you think?

Our main concern is that define-ing keyword might break definitions in system header files. If we do more detailed checks here, would it be more user-friendly? If the community encourages this, I’d be happy to try it.

We intentionally support some C keywords such as _Static_assert in C++. Perhaps we want to adopt a different, less restrictive behavior for them?

I agree with this.

Regarding the #undef keyword:

I agree to include this warning in the pendatic group.

Regarding the disagreement between Clang/GC vs. MSVC:

This is very interesting; I wasn’t aware of this issue. At least for clang-cl, should we allow clang to maintain the same behavior as MSVC?

15

I think that depends on the macro being defined. I don’t think people do #define const by accident (though I think it’s easy to accidentally forget to remove), but I do think people do accidentally define macros that become keywords because the C++ committee steals keywords out from under users with every release. e.g., I see evidence of #define module, #define requires, etc including in system headers: android-platform-headers/android-4.0.4_r1/frameworks/base/media/libstagefright/codecs/mp3dec/src/pvmp3_dec_defs.h at 14d8c3064d602a2ac2cf1646c18ab09e6d893dab · pfalcon/android-platform-headers · GitHub

I think a warning which defaults to an error would be reasonable, particularly because folks need some kind of escape hatch for when a committee steals an unreserved identifier as a keyword, and it would be nice for that not to be “avoid newer language modes”.

The paper even acknowledges that this might be required for practical reasons:

In some cases we make a program ill-formed, even where implementations do not emit a diagnostic today. As noted above, in order to preserve customer code and migrate at a pace that implementers are happy no negotiate with their clients, each of these cases can be turned into a conforming extension by issuing a warning while accepting the program. The meaning of such programs would naturally be for said implementations to define, but the Standard places no documentation requirements on implementations to document the behavior of their extensions, but merely to diagnose their use.

I think we only support those keywords when they’re a conforming extension, which means the keyword is a reserved identifier by virtue of its name (not just because it’s a keyword). Given that it’s invalid to define an identifier starting with an underscore and a capital letter (or double underscores) and it’s also invalid to define an identifier that is a keyword, I don’t think we want to have a less restrictive behavior here. Maybe we want to word the diagnostic slightly differently? e.g., '_Static_assert' is a reserved identifier *and* it's also being treated as a keyword in C++, so your code is wrong twice over But I don’t think that’s necessary either. That said, if the C keywords in C++ are sufficiently problematic, we certainly could treat them differently if we wanted.

I think we’d want to match MSVC’s behavior when the user asks for it; so I think this would be tied to -fms-compatibility or -fms-extensions (and clang-cl would pick that up automatically)

16

FreeBSD’s “standalone” environment, primarily used for its bootloader, builds with -Ddouble=jagged-little-pill -Dfloat=floaty-mcfloatface in order to poison uses of double and float (yes, it’s not perfect, floating-point literals won’t be caught), since the environment does not support them (and of course appropriate target-specific flags to disable hardware floating-point).

17

Thank you for providing these examples. A warning which defaults to an error sounds good to me. The users can downgrade it to a warning with -Wno-error=<diag-name> or suppress it entirely.

Agree, we can fix it in ms-compatibility mode.

Ah, does this means we emit an error for defining keyword would break FreeBSD’s “standalone” environment? I’m not sure whether we should provide an option to disable floating point features, rr we already have a similar option.

18

Based on my understanding of what’s being proposed, yes. I think we’d be more than happy to migrate to a more comprehensive and less hacky compiler flag to enforce this, though.

19

+1

1 Like
20

The clang Area Team discussed this this week.
We are not going to call the RFC yet because there are still some reservations, notably from @reinterpretcast.

However, we think the direction this is going is as follow:

  • Error-by-default warning for #define
  • Pedantic warning for #undef
  • #line is covered by P4136R2
  • For attributes, there has been no discussions - The conservative approach is to do nothing
  • For define in macros, the discussion doesn’t seem to have reached its course yet

We hope we can make a final decision at our next meeting in 2 weeks

1 Like