[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/higher-performance closed https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/higher-performance updated
https://github.com/llvm/llvm-project/pull/141133
>From f384cb655ca7412f80427fafb47a26c74917b9ff Mon Sep 17 00:00:00 2001
From: higher-performance
Date: Thu, 22 May 2025 16:30:29 -0400
Subject: [PATCH 1/2] Include [[clang::require_explicit_initialization]]
warnings in system headers
Fixes #141103
---
.../clang/Basic/DiagnosticSemaKinds.td| 3 ++-
clang/test/SemaCXX/uninitialized.cpp | 27 ---
2 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0b2553d82153c..934f4453f02b9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2479,7 +2479,8 @@ def note_uninit_reference_member : Note<
"uninitialized reference member is here">;
def warn_field_requires_explicit_init : Warning<
"field %select{%1|in %1}0 requires explicit initialization but is not "
- "explicitly initialized">, InGroup;
+ "explicitly initialized">, InGroup,
+ ShowInSystemHeader;
def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
InGroup;
def warn_base_class_is_uninit : Warning<
diff --git a/clang/test/SemaCXX/uninitialized.cpp
b/clang/test/SemaCXX/uninitialized.cpp
index c7cef3e06fb2b..98e1e93ec53cc 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -2,6 +2,23 @@
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++1z -verify %s -fexperimental-new-constant-interpreter
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++20 -verify %s
+#if defined(BE_THE_HEADER)
+
+// Wuninitialized-explicit-init should warn in system headers
(std::construct_at...) too.
+
+#pragma clang system_header
+namespace std {
+template
+constexpr T* construct_at(T* ptr, U&&... args) {
+ return ::new (static_cast(ptr)) T(static_cast(args)...); //
#FIELD_EMBED2_CONSTRUCT_AT
+}
+}
+
+#else
+
+#define BE_THE_HEADER
+#include __FILE__
+
void* operator new(__SIZE_TYPE__, void*);
// definitions for std::move
@@ -1564,11 +1581,11 @@ void aggregate() {
explicit F(const char(&)[1]) : f1() {
// expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
::new(static_cast(&f1)) decltype(f1);
- // expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
- ::new(static_cast(&f1)) decltype(f1)();
+ // expected-warning@#FIELD_EMBED2_CONSTRUCT_AT {{field in 'Embed'
requires explicit initialization but is not explicitly initialized}}
expected-note@+1 {{in instantiation of function template specialization
'std::construct_at' requested here}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
+ std::construct_at(&f1);
#if __cplusplus >= 202002L
- // expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
- ::new(static_cast(&f1)) decltype(f1)(1);
+ // expected-warning@#FIELD_EMBED2_CONSTRUCT_AT {{field 'embed2' requires
explicit initialization but is not explicitly initialized}} expected-note@+1
{{in instantiation of function template specialization
'std::construct_at' requested here}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
+ std::construct_at(&f1, 1);
#endif
// expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
::new(static_cast(&f1)) decltype(f1){1};
@@ -1726,3 +1743,5 @@ void aggregate() {
InheritWithExplicit specialized_implicit; // no-warning
(void)specialized_implicit;
}
+
+#endif
>From c5d3605436197426f08724369c6e0acf534b6cab Mon Sep 17 00:00:00 2001
From: higher-performance
Date: Tue, 27 May 2025 16:37:01 -0400
Subject: [PATCH 2/2] Suppress [[clang::require_explicit_initialization]] in
unevaluated operands
---
clang/include/clang/Basic/AttrDocs.td | 6 +++---
clang/lib/Sema/SemaInit.cpp | 14 +-
clang/test/Sema/uninit-variables.c| 2 ++
clang/test/SemaCXX/uninitialized.cpp | 2 ++
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 43442f177ab7b..fefdaba7f8bf5 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -1750,9 +1750,9 @@ to guard against use of uninitialized memory.
Rather, it is inte
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Okay great, thanks so much! I'll go ahead and merge this then. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/AaronBallman approved this pull request. LGTM! https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
AaronBallman wrote: > Hi all! So given the discussion above and that @erichkeane also okayed this, > can we finally proceed with this PR? I'd like to note that after discussing > it for the past month and a half, it still seems like we have no idea what > the ideal solution to the more-general problem could even look like in > principle (if it's even possible), never mind how to get there in practice. > Whereas this particular solution would address the issue we're facing, > without closing the door to any improvements we may think of later. Yeah, I think we should move forward. This may not be ideal, but we also don't know what ideal will be and this is solving a problem. Thank you for the really good discussion on this! I know it's frustrating to have long-running discussions, but we try to have a high bar for quality and that sometimes leads to this kind of thing, so we appreciate all the consideration given here, https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote:
Thanks!
One thing I want to also highlight here is that the general problem of figuring
out "whom to blame" (i.e. where the diagnostic should be attributed to, and
thus when to suppress the diagnostic) for an incorrect template instantiation
seems difficult at best, and fundamentally impossible (without additional
out-of-band information) at worst.
For example, consider this:
```
template
struct S;
template
T foo() { return typename S::type(); }
```
If you see a call to `foo()`, and it turns out that instantiation
is wrong, _whom exactly_ should take the blame/responsibility here? Any of
these are possible:
- The definition of `foo` would be to blame _if_ its author has control over
all the specializations of `S`.
- The definition of `S` would be to blame _if_ its author is guaranteeing that
`::type` is instantiable.
- The `foo()` expression would be to blame _if_ `S` and `foo` are
entirely orthogonal, and thus the author of `foo()` is the one
imposing instantiability as a requirement.
There is simply no way to know which is the correct choice -- and thus no way
to know where the suppressions should be processed -- without knowing the
implicit assumptions in the code. *Any* choice made is going to be wrong some
of the time.
This is why I'm so weary of trying to solve this larger problem for the sake of
this PR here -- it's not even clear to me that it's solvable in the first
place, never mind the implementation challenges.
_
Now, responding to @erichkeane's thoughts above:
1- `-W[no-]system-headers=...` seems like a reasonable way to address this --
do you see issues with it?
2- (see highlight above)
3- Default-construct + memcpy still requires the default-construction to be
valid. It doesn't really matter whether it's occurring in the STL or not,
right? (Just like how `std::make_unique()` requires default-construction of
`T` to be valid, even if it's in the STL.) If the STL is instantiating `T` then
it has to know that `T` is valid to instantiate that way. For a user type, it
wouldn't possibly know that, so it wouldn't do that except on behalf of the
user. For its own types, then of course it has to respect its own attributes,
but then there's no problem. If you have an example that neither of these
addresses, please share them. The _only_ one I can think of is `std::bit_cast`,
but that seems fine to ignore.
4- From my _biased_ standpoint, I'm not inherently against error-by-default (we
are currently using it under `-Werror` anyway), **but** I think it is (a)
strictly worse than allowing it to be either a warning or an error? (Note that
I'm assuming `-Wno-system-headers=...` as a further extension here, if we end
up needing it.)
5- I very much agree. To me the worst case (if they come up at all) is that
some people would stop using the attribute, which is... where we were before
the attribute was introduced, and where other compilers are now. At least in
that case they can resort to alternate (more inconvenient) methods for
discovering all construction sites. Contrast that to the current situation,
where unaware users mistakenly _think_ the attribute is working when it is
actually suppressed in some cases -- and thus may assume they have updated all
call sites, when they actually haven't, which is _worse_ than not having it at
all.
(Important side note: _we_ aren't running into this problem, because we have
implemented a safety mechanism: we're hiding the attribute behind a macro that
_also_ adds an in-class initializer, so we at least get a linker error if the
attribute diagnosis fails for some reason. The error message is terrible, but
at least we get _some_ error, and thus the attribute is still very valuable to
us, even with its current flaw. But a user who is unaware of this failure mode
wouldn't do that, and our solution isn't possible in C either.)
https://github.com/llvm/llvm-project/pull/141133
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
erichkeane wrote: Here is where my mind is at: 1- We need, as a project to re-think/come up with a better way to figure out how to suppress warnings in header files. I don't think there is a silver-bullet here, and no one has come up with a reasonable way forward on this. 2- I think the idea that 'warnings in the STL aren't actionable by users' is correct in SOME cases, but not all. It is frustrating we don't have a way to figure out when an STL function is a 'proxy' for user action (like construct_at/invoke/emplace_back/ etc), where we should no longer suppress warnings in that instantiation-context. PERHAPS we could find a way to mark it that way? 3- This one somewhat scares me for things where the STL will need to 'invent' an object (eg, sees it is 'default constructible'/'trivially copyable', so does a default-construct + memcpy instead of a construct-in-place because reasons), which will result in false-positives, because as I said, the STL is 'assumed right'. 4- It is PROBABLY not a huge deal here, and is something we could perhaps use to fix 'later'. This is an opt-in and perhaps a bit of a 'you get what you asked for' (even if you didn't know what you were asking for) via the attribute. IMO, I wonder if we should INSTEAD start marking this diagnostic as an error? It plays awkwardly with SFINAE IIRC, but we can mark it as SFINAE error anyway. 5- Even if we choose not to make it an error, I think I'm about 55/45 that we can accept this and hold our nose/push off the issues with figuring out STL proxy-function vs normal-function. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Got it, thanks for the update! Okay, let's see what they think when they have a chance to reply. If there's still disagreement, I'll try to suggest some other ways to move forward. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Are folks still on the fence after the latest discussion? If anyone still has objections I can try to suggest some path forward, but I'm not sure what everyone is thinking right now (or what this is pending on in general), and it would be lovely to move forward somehow. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Hi all~ where do folks stand on the PR currently, given the last discussion? https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
zygoloid wrote: > Its more that we assume/like to assume that the Standard Library always 'does > the right thing', even if it does so in ways that look 'wrong'. So we > suppress warnings in the standard library headers, since they are assumed to > all be false-positives. > > This of course is an interesting 'that assumption is not perfectly true' > anymore type of situation. For me the concern here is minimal: the standard library should *never* be "inventing" its own way of initializing a user-defined type. If the standard library is performing an initialization, it should either have a reason to believe the initialization works (either because it owns the type being initialized -- never a problem here unless the standard library itself starts using this attribute -- or because the user of the standard library used it in a way that implies that the initialization works -- which is exactly the case in which we want a warning), or it should be testing whether the initialization works (eg, in a SFINAE context). I wonder if a different approach to this problem would be to make the diagnostic produced by this attribute be an error rather than a warning. That'd make it rather more clear that it should be produced even in system headers :) (And we then shouldn't disable it in unevaluated operands -- instead SFINAE could handle the relevant cases there.) https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: I see, thanks for explaining. That would be great if you can discuss it offline in the near future. In the meantime, let me just emphasize a some points from my side before I forget: - The question of delayed initialization is (so far as I can tell) entirely orthogonal to the question of what the standard library should be doing here. If we keep the current spec, then obviously the existing behavior is buggy, and this PR is required for the fix. If we _do_ relax the attribute to allow delayed initialization, then it would require dataflow analysis out of the standard library (since it would now be okay to do `std::construct_at(...)` followed by filling in the fields). Either way, I struggle to see how treating the standard library differently would solve anything. - Insofar as the current spec goes -- just as `std::construct_at` cannot possibly detect (let alone try to avoid) an in-class member initializer that goes awry when invoked, I think it also cannot (and should not) try to avoid the same issue w.r.t. this attribute. It simply does not have enough information, and this is true for every function out there that constructs a user-specified type (`emplace_back`, etc.), not just those in the standard library. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
erichkeane wrote:
> @erichkeane the question of whether `S s{}; s.f = ...;` should be deemed as a
> relaxation of `S s{.f = ...};` came up during the initial RFC. From memory,
> the conclusion was that it's something we could relax in a v2 of the RFC, but
> it is a _significant_ amount of work, and the initial design shouldn't be
> blocked on it. Furthermore, we haven't really seen significant demand for
> that on our side. By contrast, the current bug is actually a loophole in the
> original design that is causing bugs to slip through for current users who
> only needed the guarantees from the original attribute, so blocking this on
> that doesn't really feel justified at this stage.
Its more that we assume/like to assume that the Standard Library always 'does
the right thing', even if it does so in ways that look 'wrong'. So we suppress
warnings in the standard library headers, since they are assumed to all be
false-positives.
This of course is an interesting 'that assumption is not perfectly true'
anymore type of situation.
> Could I move forward with merging this? Or does anyone feel there are
> blockers here?
@AaronBallman , @cor3ntin , and need to discuss this still I think and make
sure we are sure. Unfortunately Aaron's next 'office hours' (when we'd do this
typically) aren't for a few weeks, but I'll see if we can discuss this offline
in the near future.
https://github.com/llvm/llvm-project/pull/141133
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Could I move forward with merging this? Or does anyone feel there are blockers here? https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
erichkeane wrote: I think the unevaluated contexts DOES help a bunch, though I'm still concerned about the ability of the library to create 'temproary' objects/etc via some sort of default construction/aggregate construction, to then fill in the rest. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
zygoloid wrote: I don't think this concern is specific to system headers. I don't think we have precedent for disabling a warning in a SFINAE context. We do suppress warnings produced while substituting into a function template specialization during deduction if we end up not selecting that overload, but I don't think we have something analogous for concepts, and it's not clear that that would help here, given that the warning would be produced in the case where the concept check *succeeds*. You can check if you're in an unevaluated context with `Sema::isUnevaluatedContext`. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Interesting point! A few thoughts: - Are system headers really any different from non-system headers in that case? It feels like it might be an orthogonal issue we should address in a separate PR. - Do you mean SFINAE contexts rather than merely unevaluated contexts (?) - How would I detect if a given `Expr` (say, an `InitListExpr`) is unevaluated? Is that easy or would it require a huge rework of the implementation? https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
zygoloid wrote:
There are
[techniques](https://github.com/carbon-language/carbon-lang/blob/69ab97d7167146ae1deea662126af66f8f5523a2/common/struct_reflection.h#L74)
for determining the number of fields in an aggregate type `T` by attempting to
initialize with `T{}` then `T{arg}` then `T{arg, arg}`, and so on (with an
`arg` that converts to anything), until the initialization fails. Such
techniques might in principle show up in a system header (eg, boost probably
has this somewhere).
However, any such code should be in an unevaluated operand, so as long as this
warning is suppressed in unevaluated operands I think it's fine for it to be
enabled in system headers. (For extra assurance, can we make this a
`DiagRuntimeBehavior` to also suppress it in unreachable code?)
https://github.com/llvm/llvm-project/pull/141133
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: Yeah. I'm not even sure I can think of a case with the irrelevant/unused temporaries you mention, to be honest. The way I would think about this is: this attribute is intended to be equivalent to adding an in-class member initializer that is impossible for anyone to actually invoke. The only reason we don't do that directly is that the error messages are terrible, but otherwise they are solving the same problem. And, similarly, there's no reason to allow that to be bypassed either. (But, just as you said, I don't see reasons to have sympathy for it even if that case was possible to bypass.) https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
erichkeane wrote: > > I don't think this is a good idea, warning on EVERY use of this is > > incorrect in system headers. IT is going to result in a ton of > > we-want-to-be-suppressed-positives > > Thanks @erichkeane. I think I fundamentally disagree with this. The entire > intention of this attribute is to flag such a thing -- it's no different from > calling `std::invoke(foo, x)` and expecting to get a diagnostic on _every_ > invocation when the function is `void foo(int x, int y)`, especially when you > consider that these are basically simulating named-parameters. Do you have a > plausible use case in mind that makes you believe people would actually want > to suppress the warning in system headers? Hmm... I just spent a while looking at the attribute more and changed how I was thinking about this problem, and I'm less confident in my above statement. SINCE this is competely opt-in we could presumably document this better to mean "we mean this even if it is in the nitty-gritty of the standard library". Typically these sorts of attributes don't really mean that/are intended for use in violations by the 'users' code. Of course, `construct-at` and `invoke`/etc all make this somewhat more complicated of a differentiation. My biggest 'plausible' concern is cases where the system header needs to make irrelevant/unused temporaries for the purposes of various operations along the way, which still DO the right thing but are wrong along the way. BUT I also wonder how much sympathy I have for those cases when the author of the type has opted into this. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: > I don't think this is a good idea, warning on EVERY use of this is incorrect > in system headers. IT is going to result in a ton of > we-want-to-be-suppressed-positives Thanks @erichkeane. I think I fundamentally disagree with this. The entire intention of this attribute is to flag such a thing -- it's no different from calling `std::invoke(foo, x)` and expecting to get a diagnostic on *every* invocation when the function is `void foo(int x, int y)`, especially when you consider that these are basically simulating named-parameters. Do you have a plausible use case in mind that makes you believe people would actually want to suppress the warning in system headers? https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/erichkeane commented: I don't think this is a good idea, warning on EVERY use of this is incorrect in system headers. IT is going to result in a ton of we-want-to-be-suppressed-positives I also don't think we can have a heuristic that wouldn't have an absurd amount of false positives. IMO, the only thing we COULD do here is to have some sort of `#pragma clang` that wraps the template and says "show this even in system headers", which is an opt-in at the library level. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: What do folks think? https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: @cor3ntin feel free to chat, but in this case we do actually want to warn (and error, under -Werror) if _anybody_ (mis)uses `std::construct_at` (i.e. neglects to explicitly initialize a field that declares itself as such). It's the exact analog of using std::invoke to call a function that has a mandatory positional parameter -- it doesn't make sense to allow anyone to bypass it. The entire goal is to make sure that field/parameter has an explicitly provided value in all cases. So I think this really is the right fix for this particular warning, regardless of the more general problem. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
cor3ntin wrote: Let me chat with @AaronBallman and @erichkeane on Monday with this. This feels like a very narrow fix for a widespread issue. - Presumably, we don't want to warn if library implementers (mis)use std::construct_at internally - There are _tons_ of other warnings that are silenced. So I think that either - We need a better heuristics to hide warnings than "comes from a system header" - We should add the ShowInSystemHeader flags to a lot of warnings, not just this one. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/higher-performance updated
https://github.com/llvm/llvm-project/pull/141133
>From 1006e27f02b1e6e94b4712d9ccbc5005a616d670 Mon Sep 17 00:00:00 2001
From: higher-performance
Date: Thu, 22 May 2025 16:30:29 -0400
Subject: [PATCH] Include [[clang::require_explicit_initialization]] warnings
in system headers
Fixes #141103
---
.../clang/Basic/DiagnosticSemaKinds.td| 3 ++-
clang/test/SemaCXX/uninitialized.cpp | 27 ---
2 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 78b36ceb88125..8a3aafeabdad3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2423,7 +2423,8 @@ def note_uninit_reference_member : Note<
"uninitialized reference member is here">;
def warn_field_requires_explicit_init : Warning<
"field %select{%1|in %1}0 requires explicit initialization but is not "
- "explicitly initialized">, InGroup;
+ "explicitly initialized">, InGroup,
+ ShowInSystemHeader;
def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
InGroup;
def warn_base_class_is_uninit : Warning<
diff --git a/clang/test/SemaCXX/uninitialized.cpp
b/clang/test/SemaCXX/uninitialized.cpp
index c7b987e2172e6..df606b0827b1a 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -2,6 +2,23 @@
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++1z -verify %s -fexperimental-new-constant-interpreter
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++20 -verify %s
+#if defined(BE_THE_HEADER)
+
+// Wuninitialized-explicit-init should warn in system headers
(std::construct_at...) too.
+
+#pragma clang system_header
+namespace std {
+template
+constexpr T* construct_at(T* ptr, U&&... args) {
+ return ::new (static_cast(ptr)) T(static_cast(args)...); //
#FIELD_EMBED2_CONSTRUCT_AT
+}
+}
+
+#else
+
+#define BE_THE_HEADER
+#include __FILE__
+
void* operator new(__SIZE_TYPE__, void*);
// definitions for std::move
@@ -1564,11 +1581,11 @@ void aggregate() {
explicit F(const char(&)[1]) : f1() {
// expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
::new(static_cast(&f1)) decltype(f1);
- // expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
- ::new(static_cast(&f1)) decltype(f1)();
+ // expected-warning@#FIELD_EMBED2_CONSTRUCT_AT {{field in 'Embed'
requires explicit initialization but is not explicitly initialized}}
expected-note@+1 {{in instantiation of function template specialization
'std::construct_at' requested here}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
+ std::construct_at(&f1);
#if __cplusplus >= 202002L
- // expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
- ::new(static_cast(&f1)) decltype(f1)(1);
+ // expected-warning@#FIELD_EMBED2_CONSTRUCT_AT {{field 'embed2' requires
explicit initialization but is not explicitly initialized}} expected-note@+1
{{in instantiation of function template specialization
'std::construct_at' requested here}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
+ std::construct_at(&f1, 1);
#endif
// expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
::new(static_cast(&f1)) decltype(f1){1};
@@ -1726,3 +1743,5 @@ void aggregate() {
InheritWithExplicit specialized_implicit; // no-warning
(void)specialized_implicit;
}
+
+#endif
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/efriedma-quic commented: Looks fine, but I'd like a second approval; I haven't really looked at this template instantiation stuff recently. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: @efriedma-quic Ah I figured out how to test this - just added a test. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/higher-performance updated
https://github.com/llvm/llvm-project/pull/141133
>From 00e0cf90f1d53ec8b2472bd740066016ee88f584 Mon Sep 17 00:00:00 2001
From: higher-performance
Date: Thu, 22 May 2025 16:30:29 -0400
Subject: [PATCH] Include [[clang::require_explicit_initialization]] warnings
in system headers
Fixes #141103
---
.../clang/Basic/DiagnosticSemaKinds.td| 3 ++-
clang/test/SemaCXX/uninitialized.cpp | 23 +--
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 78b36ceb88125..8a3aafeabdad3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2423,7 +2423,8 @@ def note_uninit_reference_member : Note<
"uninitialized reference member is here">;
def warn_field_requires_explicit_init : Warning<
"field %select{%1|in %1}0 requires explicit initialization but is not "
- "explicitly initialized">, InGroup;
+ "explicitly initialized">, InGroup,
+ ShowInSystemHeader;
def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
InGroup;
def warn_base_class_is_uninit : Warning<
diff --git a/clang/test/SemaCXX/uninitialized.cpp
b/clang/test/SemaCXX/uninitialized.cpp
index c7b987e2172e6..6986a6f0b77b0 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -2,6 +2,23 @@
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++1z -verify %s -fexperimental-new-constant-interpreter
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized
-Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference
-std=c++20 -verify %s
+#if defined(BE_THE_HEADER)
+
+// Wuninitialized-explicit-init should warn in system headers
(std::construct_at...) too.
+
+#pragma clang system_header
+namespace std {
+template
+constexpr T* construct_at(T* ptr, U&&... args) {
+ return ::new (static_cast(ptr)) T(std::forward(args)...);
+}
+}
+
+#else
+
+#define BE_THE_HEADER
+#include __FILE__
+
void* operator new(__SIZE_TYPE__, void*);
// definitions for std::move
@@ -1565,10 +1582,10 @@ void aggregate() {
// expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
::new(static_cast(&f1)) decltype(f1);
// expected-warning@+1 {{field in 'Embed' requires explicit
initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2
{{'embed2' declared here}}
- ::new(static_cast(&f1)) decltype(f1)();
+ std::construct_at(&f1);
#if __cplusplus >= 202002L
// expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
- ::new(static_cast(&f1)) decltype(f1)(1);
+ std::construct_at(&f1, 1);
#endif
// expected-warning@+1 {{field 'embed2' requires explicit initialization
but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2'
declared here}}
::new(static_cast(&f1)) decltype(f1){1};
@@ -1726,3 +1743,5 @@ void aggregate() {
InheritWithExplicit specialized_implicit; // no-warning
(void)specialized_implicit;
}
+
+#endif
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
cor3ntin wrote: I've been considering similar issues recently. e.g https://compiler-explorer.com/z/5xcKf8Kj1 A more general solution would be to emit diagnostics in system headers if one of the involved types/expressions names an entity declared outside of a system header. I haven't had a brilliant idea on how to do that yet. Note that looking at the instantiation stack doesn't help because warnings unrelated to user code can be produced in system headers, caused by an instantiation in user code. So we need to look at entities somehow. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
higher-performance wrote: @efriedma-quic: I'm not sure what the appropriate way to test this is. The file [clang/test/SemaCXX/uninitialized.cpp](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/uninitialized.cpp) doesn't include any system headers. Should I just add a system header there and include it? Re: libc++ cooperation: I thought about something along those lines too, but I couldn't think of a satisfactory solution. Especially because people can have their own templated wrappers too, and also because "system headers" could be anything third-party -- not just the standard library. I think the current approach might actually be best, as inelegant as it feels. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
efriedma-quic wrote: Testcase? This seems like the sort of thing where we might want to cooperate with the C++ library somehow to get better diagnostics, but getting some diagnostic seems like an improvement. https://github.com/llvm/llvm-project/pull/141133 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (higher-performance)
Changes
Fixes #141103
---
Full diff: https://github.com/llvm/llvm-project/pull/141133.diff
1 Files Affected:
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-1)
``diff
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 78b36ceb88125..8a3aafeabdad3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2423,7 +2423,8 @@ def note_uninit_reference_member : Note<
"uninitialized reference member is here">;
def warn_field_requires_explicit_init : Warning<
"field %select{%1|in %1}0 requires explicit initialization but is not "
- "explicitly initialized">, InGroup;
+ "explicitly initialized">, InGroup,
+ ShowInSystemHeader;
def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
InGroup;
def warn_base_class_is_uninit : Warning<
``
https://github.com/llvm/llvm-project/pull/141133
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Include [[clang::require_explicit_initialization]] warnings in system headers (PR #141133)
https://github.com/higher-performance created
https://github.com/llvm/llvm-project/pull/141133
Fixes #141103
>From 0bdda2b593167bb7f75bea33da1f3adf4d8b6276 Mon Sep 17 00:00:00 2001
From: higher-performance
Date: Thu, 22 May 2025 16:30:29 -0400
Subject: [PATCH] Include [[clang::require_explicit_initialization]] warnings
in system headers
Fixes #141103
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 78b36ceb88125..8a3aafeabdad3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2423,7 +2423,8 @@ def note_uninit_reference_member : Note<
"uninitialized reference member is here">;
def warn_field_requires_explicit_init : Warning<
"field %select{%1|in %1}0 requires explicit initialization but is not "
- "explicitly initialized">, InGroup;
+ "explicitly initialized">, InGroup,
+ ShowInSystemHeader;
def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
InGroup;
def warn_base_class_is_uninit : Warning<
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
