[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: BTW: I did not push for this feature to be exposed in earlier language modes because GCC will ship with GNU C23 by default with GCC 15. GCC 15 already saw distribution-wide testing in Redhat and Debian and from this experience I do not expect any problems related to this feature (so far, I am not aware of any issue caused by this feature, which can not be said about some other C23 changes). Unrelated testing exercising the new features uncovered a couple of bugs in my implementation (as well as other unrelated bugs in GCC), and I would expect that this will be true for Clang as well. But I would not expect anything critical. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: Another comment about attributes: Attributes should not generally affect type compatibility (in general, but also relevant for this feature) for two reasons: 1. Standard attributes are ignorable, and this would then break. 2. Compatibility affects aliasing, one one certainly does not want to affect aliasing decisions by adding [[deprecated]] somwhere. So only for vendor attributes that affect ABI, this should (and must) be considered. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: > Curiously, GCC doesn't merge the standard attributes either, it seems to do > last-one-wins: https://godbolt.org/z/j3W7ej5Kq I filed a bug for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119526 https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: I did some initial testing by compiling one of my projects and so far this worked fine. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: So one usability issue I noticed that we also had in GCC is that the warning about declaring structs in function prototypes (-Wvisibility) does not make sense anymore and becomes annoying. We turned it off in C23 for complete (but not for incomplete) structs) without tag. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Diagnose potential size confusion with VLA params (PR #129772)
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 %s -std=c23 -verify=expected,c -fsyntax-only
+// RUN: %clang_cc1 %s -std=c23 -verify=good -fsyntax-only -Wno-vla
+// RUN: %clang_cc1 -x c++ %s -verify -fsyntax-only
+// RUN: %clang_cc1 -DCARET -fsyntax-only -std=c23
-fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=1 %s 2>&1 |
FileCheck %s -strict-whitespace
+
+// good-no-diagnostics
+
+int n, m; // #decl
+int size(int);
+
+void foo(int vla[n], int n); // expected-warning {{variable length array size
expression refers to declaration from an outer scope}} \
+expected-note {{does not refer to this
declaration}} \
+expected-note@#decl {{refers to this
declaration instead}}
+
+void bar(int (*vla)[n], int n); // expected-warning {{variable length array
size expression refers to declaration from an outer scope}} \
+ expected-note {{does not refer to this
declaration}} \
+ expected-note@#decl {{refers to this
declaration instead}}
+
+void baz(int n, int vla[n]); // no diagnostic expected
+
+void quux(int vla[n + 12], int n); // expected-warning {{variable length array
size expression refers to declaration from an outer scope}} \
+ expected-note {{does not refer to this
declaration}} \
+ expected-note@#decl {{refers to this
declaration instead}}
+
+void quibble(int vla[size(n)], int n); // expected-warning {{variable length
array size expression refers to declaration from an outer scope}} \
+ expected-note {{does not refer to
this declaration}} \
+ expected-note@#decl {{refers to
this declaration instead}}
+
+void quobble(int vla[n + m], int n, int m); // expected-warning 2 {{variable
length array size expression refers to declaration from an outer scope}} \
+expected-note 2 {{does not
refer to this declaration}} \
+expected-note@#decl 2 {{refers
to this declaration instead}}
+
+// For const int, we still treat the function as having a variably-modified
+// type, but only in C.
uecker wrote:
Note that there is proposal in flight for C that might change this.
https://github.com/llvm/llvm-project/pull/129772
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Diagnose potential size confusion with VLA params (PR #129772)
uecker wrote: And a final comment. While the main motivating use case for you may be VLAs in parameter declarations (although as heavy user of those, I never felt the need for such a warning), it would seem the warning would be relevant in cases where the arrays are not actually VLAs. In general, it seems a warning about shadowing a visible identifier somewhere and just having it used with the old definition. So I wonder whether it should be renamed? https://github.com/llvm/llvm-project/pull/129772 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote:
> I now see there's two different parts of the problem to worry about:
>
> First case: A redeclaration in a _different_ scope. This always defines a new
> distinct type. This was valid before C23, and is still valid regardless of
> whether the type is compatible. (In contrast with function decls, which must
> be globally compatible across the entire TU in all scopes). This is the case
> I was thinking about earlier. This part really feels pretty-much
> straight-forward.
>
> E.g. The following example defines two different `struct Foo` types. The
> second does not inherit any semantics from the former, and the code is valid
> before and after C23.
>
> ```
> struct Foo { [[deprecated("hello")]] int x; };
> int test2() {
> struct Foo { int x; };
> struct Foo f = {0};
> return f.x; // no deprecation
> }
> ```
>
> Newly in C23, the inner `struct Foo` is compatible with the outer `Foo`. As
> such, you can now implicitly cast between pointers to the two types, use them
> to read/write the same memory, etc. The expected semantics seems to me to be
> very clear here: we should determine whether it's a compatible type according
> to the standard's rules, and we shouldn't inherit any attributes from the
> other scope.
>
> For determining compatibility of nonstandard attributes, we can do similarly
> to how we handle functions: explicitly check a handful of properties, such as
> calling convention, and assume compatibility if not otherwise specified. For
> aggregates, we can also validate that the field offsets are the same, which
> will handle most of the questionable cases.
>
> But then there's the other case...
>
> Where you have a redefinition of a struct in the _same scope_. This was
> formerly invalid, and is now valid only when compatible. In this case, both
> definitions define "the same type", so it seems reasonable to expect some
> sort of decl merging semantics. But the standard doesn't appear to say
> anything at all about what to do.
>
> However, we do have some prior art -- we're not inventing the whole idea of
> decl-merging from scratch right now. So maybe we can follow that prior art?
> For example, top-level attributes on a struct's definition can be inherited
> from a declaration in the same scope. It seems plausible that the same would
> be true for redefinition, and then maybe we'd also extend that same
> declaration-attribute-merging to fields within the struct.
>
> Except, while investigating, I discovered that the pre-C23 behavior is
> inconsistent between Clang and GCC, potentially resulting in ABI differences.
> E.g.
>
> ```
> struct __attribute__((packed)) Foo;
> struct Foo {
> char a;
> int b;
> };
>
> _Static_assert(sizeof(struct Foo) == 8); // with GCC, because packed on the
> decl was ignored
> _Static_assert(sizeof(struct Foo) == 5); // with Clang, because packed on
> decl was inherited by the definition
> ```
>
> Both GCC and Clang do inherit deprecation from the decl. E.g.:
>
> ```
> struct [[deprecated("hello")]] Foo;
> struct Foo* test1; // Both GCC and Clang say: deprecated: hello
>
> struct Foo { int x; };
> struct Foo* test2; // Both GCC and Clang say: deprecated: hello
> ```
>
> Given that, I'd definitely expect that under the new C23 semantics,
> _redefining_ the struct should obviously also preserve the deprecated
> attribute. Except...apparently it doesn't in GCC's implementation. Gah.
> Adding on to the previous test,
>
> ```
> struct Foo { int x; };
> struct Foo* test3; // GCC: no deprecation warning.
> ```
>
> So...yeah...
I think this is a bug in my implementation for GCC. Here, the wording in the
standard for the attributes makes this clear that an entity can be redeclared
with and without the attribute, but it is considered "marked" after the first
declaration that has it.
https://github.com/llvm/llvm-project/pull/132939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: > It seems to me that we could simplify the implementation by being a bit more > literal in the interpretation of the requirements: look only at the type, > alignment, name, and bitfield-width of members. Just don't bother checking > other stuff > > I'd hope it can look more like a simple loop over the fields of the two > record types (similar to mergeFunctionTypes's checks of the functions' param > types), rather than starting with the very-complex > ASTStructuralEquivalence.cpp, and adding even more complexity to validate > attributes. What I do in GCC is mostly look at those things required in the standard and I make sure that all field offsets are the same (e.g. if some attribute would change how fields are layed out, this would catch it). This is a simple loop. GCC already had a generic mechanism to compare attributes on types which decides whether an attribute makes a type incompatible (.e.g. affects ABI such as "packed"), or requires a warning, or whether it is just fine. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
@@ -450,6 +453,116 @@ class StmtComparer {
};
} // namespace
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ const Attr *Attr1, const Attr *Attr2) {
+ // Two attributes are structurally equivalent if they are the same kind
+ // of attribute, spelled with the same spelling kind, and have the same
+ // arguments. This means that [[noreturn]] and __attribute__((noreturn)) are
+ // not structurally equivalent, nor are [[nodiscard("foo")]] and
+ // [[nodiscard("bar")]].
+ if (Attr1->getKind() != Attr2->getKind())
+return false;
+
+ if (Attr1->getSyntax() != Attr2->getSyntax())
+return false;
+
+ if (Attr1->getSpellingListIndex() != Attr2->getSpellingListIndex())
+return false;
+
+ auto GetAttrName = [](const Attr *A) {
+if (const IdentifierInfo *II = A->getAttrName())
+ return II->getName();
+return StringRef{};
+ };
+
+ if (GetAttrName(Attr1) != GetAttrName(Attr2))
+return false;
+
+ // FIXME: check the attribute arguments. Attr does not track the arguments on
+ // the base class, which makes this awkward. We may want to tablegen a
+ // comparison function for attributes? In the meantime, we're doing this the
+ // cheap way by pretty printing the attributes and checking they produce
uecker wrote:
> > The standard defines when things are compatible and it explicitly does not
> > require attributes to match for types to be compatible.
>
> Anything the standard doesn't talk about is UB by omission, and the standard
> makes no mention of attributes. There is nothing explicit about this
> situation.
I don't think this applies here. We say that types are compatible under
conditions A B and C. That we do not mention D does not mean there is UB. We
also do not mention E, F and G or a billion other things. For me, an omission
is when something is not precisely specified and you can reasonable interpret
it either way. This is not the case here. But this only applies to standard
attributes, vendor attributes can do whatever they want.
>
> I want to dispense with this topic because I feel very strongly we move
> forward with the current approach. To make this more explicit as to why:
>
> 0. The goal of this feature is that two types which were compatible
> across TU boundaries are now compatible if defined within the same TU. So
> this is about naming and object layout, in general. Attributes impact these
> kinds of things (packed, transparent_union, type_visibility, preferred_name,
> randomize_layout, etc), and attribute arguments do as well (btf_decl_tag,
> objc_bridge_related, uuid, etc)
>
> 1. The ideal situation is we have individual attributes specify whether
> they do or do not make two types [in]compatible, then the equivalence checker
> can generically check this rather than requiring special logic that's trivial
> for us to forget when adding new attributes.
>
> 2. I'm not implementing the ideal solution because that's a significant
> undertaking, attributes are an edge case, and I only have so much bandwidth.
> I have a FIXME in the patch that makes this clear.
>
> 3. Since we're not getting the ideal solution today, we can either be
> safe by default (require attributes to be the same) or we can be unsafe by
> default (ignore attributes). I'm adamant we go with safe by default until the
> ideal solution can be implemented.
Sounds reasonable, but I would exempt standard attributes, because I believe
this is what the standard requires.
https://github.com/llvm/llvm-project/pull/132939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
@@ -450,6 +453,116 @@ class StmtComparer {
};
} // namespace
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ const Attr *Attr1, const Attr *Attr2) {
+ // Two attributes are structurally equivalent if they are the same kind
+ // of attribute, spelled with the same spelling kind, and have the same
+ // arguments. This means that [[noreturn]] and __attribute__((noreturn)) are
+ // not structurally equivalent, nor are [[nodiscard("foo")]] and
+ // [[nodiscard("bar")]].
+ if (Attr1->getKind() != Attr2->getKind())
+return false;
+
+ if (Attr1->getSyntax() != Attr2->getSyntax())
+return false;
+
+ if (Attr1->getSpellingListIndex() != Attr2->getSpellingListIndex())
+return false;
+
+ auto GetAttrName = [](const Attr *A) {
+if (const IdentifierInfo *II = A->getAttrName())
+ return II->getName();
+return StringRef{};
+ };
+
+ if (GetAttrName(Attr1) != GetAttrName(Attr2))
+return false;
+
+ // FIXME: check the attribute arguments. Attr does not track the arguments on
+ // the base class, which makes this awkward. We may want to tablegen a
+ // comparison function for attributes? In the meantime, we're doing this the
+ // cheap way by pretty printing the attributes and checking they produce
uecker wrote:
I guess then everything is UB because we never mention whether the time of day
plays a role in type compatibility.
https://github.com/llvm/llvm-project/pull/132939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
@@ -450,6 +453,116 @@ class StmtComparer {
};
} // namespace
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ const Attr *Attr1, const Attr *Attr2) {
+ // Two attributes are structurally equivalent if they are the same kind
+ // of attribute, spelled with the same spelling kind, and have the same
+ // arguments. This means that [[noreturn]] and __attribute__((noreturn)) are
+ // not structurally equivalent, nor are [[nodiscard("foo")]] and
+ // [[nodiscard("bar")]].
+ if (Attr1->getKind() != Attr2->getKind())
+return false;
+
+ if (Attr1->getSyntax() != Attr2->getSyntax())
+return false;
+
+ if (Attr1->getSpellingListIndex() != Attr2->getSpellingListIndex())
+return false;
+
+ auto GetAttrName = [](const Attr *A) {
+if (const IdentifierInfo *II = A->getAttrName())
+ return II->getName();
+return StringRef{};
+ };
+
+ if (GetAttrName(Attr1) != GetAttrName(Attr2))
+return false;
+
+ // FIXME: check the attribute arguments. Attr does not track the arguments on
+ // the base class, which makes this awkward. We may want to tablegen a
+ // comparison function for attributes? In the meantime, we're doing this the
+ // cheap way by pretty printing the attributes and checking they produce
uecker wrote:
The standard defines when things are compatible and it explicitly does not
require attributes to match for types to be compatible (in general, this has
not much to do with this proposal except that this general rule now also
applies here). In general, this proposal does not raise any new questions
here (but extends the scope of such questions) and the answer to the question
should be the same as elsewhere (e.g. as in typedef redefinitions,
compatibility of function types). I would recommend to warn on redefinitions
that add or remove standard attributes. For compatibility those should *not*
matter. For vendor attributes this up to you, of course and some attributes
will certainly make types incompatible (e.g. "packed", here one can take a look
at what you do for LTO because these things should already matter there). For
vendors attributes, the safe choice is to reject redefinitions. For
compatibility, one has make a choice for each case, but it should be mostly
obvious (does it change the representation?)
https://github.com/llvm/llvm-project/pull/132939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C23] Implement WG14 N3037 (PR #132939)
uecker wrote: > > Once that is settled, then go back to work on redeclaration/definition > support. Possibly it'd be good to pause and ask the standards body to clarify > how it's supposed to work first. While I think my proposed semantics above > make sense, the standard doesn't actually _specify_ that, one way or the > other. And it should. For standard attributes, the standard explicitly makes it clear that redeclarations w/ and w/o attributes are ok and can be mixed. This is also what GCC implements. I do not think there is anything unclear in the standard. Each attribute has specific wording that clarifies this. https://github.com/llvm/llvm-project/pull/132939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
