[clang] Disallow VLA type compound literals

[J-MR-T]

C99-C23 6.5.2.5 says: The type name shall specify an object type or an array of unknown size, but not a variable length array type.

Closes issue #89835 .

I git clang-format'ed my changes and ran the clang tests, which passed after minor adjustments to the new behavior.

I mostly implemented everything as discussed in #89835, with the exception of clang/test/C/C2x/n2900_n3011_2.c: in that file I deleted the part about VLA compound literals, because the file relies on LLVM-IR being emitted by clang, and checking for diagnostics here wouldn't have fit with the rest of the file. Adding another RUN line and handling this case independently in that file would be an option, but it felt out of place. Of course, I'm still open to doing it that way, or preserving the test another way, if that is preferred.

As I point out here, the new behavior leads to a confusing reality brought about by the C23 standard:

int a[size] = {};

is valid code, while

int a[size] = (int[size]){};

is not. As this seems to be what the standard requests, I implemented it that way for now.

Tagging @Sirraide and @AaronBallman as requested :).

[github-actions[bot]]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[J-MR-T]

Please be sure to add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Oh, sorry, I didn't find anything know how release notes were handled - will do!

A question on that, would you classify this as a breaking change? Technically, it could break the compilation of programs previously compiled with Clang 17 that compile VLA type compound literals, but never execute the code that they're used in; is that enough for it to be listed as a breaking change? If so, would this go under the C/C++ Language Potentially Breaking Changes header? Or should I create a new C Language Potentially Breaking Changes header, as this does not affect Clang's C++ behavior itself in any way?

PS: I hope multiple commits in this PR are fine, they are squashed in the end anyway, right?

[J-MR-T]

Btw, you can probably move this PR out of Draft status, it seems awfully close to finished

I'll finish implementing your suggestions, run the tests again locally and then move it out of draft, if that's alright with you :).

[Sirraide]

A question on that, would you classify this as a breaking change?

This is just a bug fix, so no. Rejecting code that we would erroneously accept (and miscompile in the process) is not really a breaking change.

[Sirraide]

Btw, you can probably move this PR out of Draft status, it seems awfully close to finished

I'll finish implementing your suggestions, run the tests again locally and then move it out of draft, if that's alright with you :).

Either way is fine I’d say, but the thing is that I personally at least would use draft prs mainly for something where I’m nowhere close to done and it’s not really ready for review because things are probably going to change, but I just want to signal that I’m working on it. If it’s something that’s mostly done or which is at least mostly review-ready, then I’d just use a regular pr, but that’s just how I do it.

[AaronBallman]

Please be sure to add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Oh, sorry, I didn't find anything know how release notes were handled - will do!

No worries!

A question on that, would you classify this as a breaking change? Technically, it could break the compilation of programs previously compiled with Clang 17 that compile VLA type compound literals, but never execute the code that they're used in; is that enough for it to be listed as a breaking change? If so, would this go under the C/C++ Language Potentially Breaking Changes header? Or should I create a new C Language Potentially Breaking Changes header, as this does not affect Clang's C++ behavior itself in any way?

Nope -- potentially breaking changes are ones that are likely to be significantly disruptive, but I suspect very few folks will have code suddenly rejected as a result of your fix.

PS: I hope multiple commits in this PR are fine, they are squashed in the end anyway, right?

Totally fine!

[AaronBallman]

Btw, you can probably move this PR out of Draft status, it seems awfully close to finished

I'll finish implementing your suggestions, run the tests again locally and then move it out of draft, if that's alright with you :).

Either way is fine I’d say, but the thing is that I personally at least would use draft prs mainly for something where I’m nowhere close to done and it’s not really ready for review because things are probably going to change, but I just want to signal that I’m working on it. If it’s something that’s mostly done or which is at least mostly review-ready, then I’d just use a regular pr, but that’s just how I do it.

FWIW, that's what I'm used to as well. I usually ignore anything marked "Draft" on the assumption it's not ready for review, but I happened to remember the discussion on the issue and peeked at this one anyway. :-)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jim M. R. Teichgräber (J-MR-T)

Changes

C99-C23 6.5.2.5 says: The type name shall specify an object type or an array of unknown size, but not a variable length array type. Closes issue #89835 . I `git clang-format`'ed my changes and ran the clang tests, which passed after minor adjustments to the new behavior. I mostly implemented everything as discussed in #89835, with the exception of [clang/test/C/C2x/n2900_n3011_2.c](https://github.com/llvm/llvm-project/compare/main...J-MR-T:llvm-project:main#diff-aa2b77ff42fcd2f2c02cc456f5023e923d44e5018dd2af7c046ebdcc8cb00a4a): in that file I deleted the part about VLA compound literals, because the file relies on LLVM-IR being emitted by clang, and checking for diagnostics here wouldn't have fit with the rest of the file. Adding another RUN line and handling this case independently in that file would be an option, but it felt out of place. Of course, I'm still open to doing it that way, or preserving the test another way, if that is preferred. As I point out [here](https://github.com/llvm/llvm-project/compare/main...J-MR-T:llvm-project:main#diff-3c28567b5e0c77d68f174541a0b77f5a85d093f58b89cd3675ee04a550a44880R7278-R7283), the new behavior leads to a confusing reality brought about by the C23 standard: ```c int a[size] = {}; ``` is valid code, while ```c int a[size] = (int[size]){}; ``` is not. As this seems to be what the standard requests, I implemented it that way for now. Tagging @Sirraide and @AaronBallman as requested :). --- Full diff: https://github.com/llvm/llvm-project/pull/91891.diff 6 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+3) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2) - (modified) clang/lib/Sema/SemaExpr.cpp (+13-6) - (modified) clang/test/C/C2x/n2900_n3011.c (+7-1) - (modified) clang/test/C/C2x/n2900_n3011_2.c (-16) - (modified) clang/test/Sema/compound-literal.c (+12-1) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7c5dcc59c7016..1ffd29da0cba9 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -563,6 +563,9 @@ Bug Fixes in This Version - Clang will no longer emit a duplicate -Wunused-value warning for an expression `(A, B)` which evaluates to glvalue `B` that can be converted to non ODR-use. (#GH45783) +- Clang now correctly disallows VLA type compound literals, e.g. ``(int[size]){}``, + as the C standard mandates. (#GH89835) + Bug Fixes to Compiler Builtins ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index d6863f90edb6e..25d925da7b748 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3371,6 +3371,8 @@ def err_field_with_address_space : Error< "field may not be qualified with an address space">; def err_compound_literal_with_address_space : Error< "compound literal in function scope may not be qualified with an address space">; +def err_compound_literal_with_vla_type : Error< + "compound literal cannot be of variable-length array type">; def err_address_space_mismatch_templ_inst : Error< "conflicting address space qualifiers are provided between types %0 and %1">; def err_attr_objc_ownership_redundant : Error< diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index c688cb21f2364..e62e3f3285e5d 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -7274,12 +7274,19 @@ Sema::BuildCompoundLiteralExpr(SourceLocation LParenLoc, TypeSourceInfo *TInfo, // init a VLA in C++ in all cases (such as with non-trivial constructors). // FIXME: should we allow this construct in C++ when it makes sense to do // so? - std::optional NumInits; - if (const auto *ILE = dyn_cast(LiteralExpr)) - NumInits = ILE->getNumInits(); - if ((LangOpts.CPlusPlus || NumInits.value_or(0)) && - !tryToFixVariablyModifiedVarType(TInfo, literalType, LParenLoc, - diag::err_variable_object_no_init)) + // + // But: C99-C23 6.5.2.5 Compound literals constraint 1: The type name + // shall specify an object type or an array of unknown size, but not a + // variable length array type. This seems odd, as it allows int a[size] = + // {}; but forbids int a[size] = (int[size]){}; As this is what the + // standard says, this is what's implemented here for C (except for the + // extension that permits constant foldable size arrays) + + auto diagID = LangOpts.CPlusPlus + ? diag::err_variable_object_no_init + : diag::err_compound_literal_with_vla_type; + if (!tryToFixVariablyModifiedVarType(TInfo, literalType, LParenLoc, + diagID)) return ExprError(); } } else if (!literalType->isDependentType() && diff --git a/clang/test/C/C2x/n2900_n3011.c b/clang/test/C/C2x/n2900_n3011.c index 4350aa140691b..82a3b16c8acda 100644 --- a/clang/test/C/C2x/n2900_n3011.c +++ b/clang/test/C/C2x/n2900_n3011.c @@ -27,8 +27,14 @@ void test(void) { compat-warning {{use of an empty initializer is incompatible with C standards before C23}} int vla[i] = {}; // compat-warning {{use of an empty initializer is incompatible with C standards before C23}} \ pedantic-warning {{use of an empty initializer is a C23 extension}} + // C99 6.5.2.5 Compound literals constraint 1: The type name shall specify an + // object type or an array of unknown size, but not a variable length array + // type. int *compound_literal_vla = (int[i]){}; // compat-warning {{use of an empty initializer is incompatible with C standards before C23}} \ - pedantic-warning {{use of an empty initializer is a C23 extension}} + pedantic-warning {{use of an empty initializer is a C23 extension}}\ + compat-error {{compound literal cannot be of variable-length array type}} \ + pedantic-error {{compound literal cannot be of variable-length array type}}\ + struct T { int i; diff --git a/clang/test/C/C2x/n2900_n3011_2.c b/clang/test/C/C2x/n2900_n3011_2.c index eb15fbf905c86..ab659d636d155 100644 --- a/clang/test/C/C2x/n2900_n3011_2.c +++ b/clang/test/C/C2x/n2900_n3011_2.c @@ -76,22 +76,6 @@ void test_zero_size_vla() { // CHECK-NEXT: call void @llvm.memset.p0.i64(ptr {{.*}} %[[VLA]], i8 0, i64 %[[BYTES_TO_COPY]], i1 false) } -void test_compound_literal_vla() { - int num_elts = 12; - int *compound_literal_vla = (int[num_elts]){}; - // CHECK: define {{.*}} void @test_compound_literal_vla - // CHECK-NEXT: entry: - // CHECK-NEXT: %[[NUM_ELTS_PTR:.+]] = alloca i32 - // CHECK-NEXT: %[[COMP_LIT_VLA:.+]] = alloca ptr - // CHECK-NEXT: %[[COMP_LIT:.+]] = alloca i32 - // CHECK-NEXT: store i32 12, ptr %[[NUM_ELTS_PTR]] - // CHECK-NEXT: %[[NUM_ELTS:.+]] = load i32, ptr %[[NUM_ELTS_PTR]] - // CHECK-NEXT: %[[NUM_ELTS_EXT:.+]] = zext i32 %[[NUM_ELTS]] to i64 - // CHECK-NEXT: %[[BYTES_TO_COPY:.+]] = mul nuw i64 %[[NUM_ELTS_EXT]], 4 - // CHECK-NEXT: call void @llvm.memset.p0.i64(ptr {{.*}} %[[COMP_LIT]], i8 0, i64 %[[BYTES_TO_COPY]], i1 false) - // CHECK-NEXT: store ptr %[[COMP_LIT]], ptr %[[COMP_LIT_VLA]] -} - void test_nested_structs() { struct T t1 = { 1, {} }; struct T t2 = { 1, { 2, {} } }; diff --git a/clang/test/Sema/compound-literal.c b/clang/test/Sema/compound-literal.c index a64b6f9e5dfa4..3ed53d670d38f 100644 --- a/clang/test/Sema/compound-literal.c +++ b/clang/test/Sema/compound-literal.c @@ -29,7 +29,7 @@ int main(int argc, char **argv) { struct Incomplete; // expected-note{{forward declaration of 'struct Incomplete'}} struct Incomplete* I1 = &(struct Incomplete){1, 2, 3}; // expected-error {{variable has incomplete type}} void IncompleteFunc(unsigned x) { - struct Incomplete* I2 = (struct foo[x]){1, 2, 3}; // expected-error {{variable-sized object may not be initialized}} + struct Incomplete* I2 = (struct foo[x]){1, 2, 3}; // expected-error {{compound literal cannot be of variable-length array type}} (void){1,2,3}; // expected-error {{variable has incomplete type}} (void(void)) { 0 }; // expected-error{{illegal initializer type 'void (void)'}} } @@ -42,3 +42,14 @@ int (^block)(int) = ^(int i) { int *array = (int[]) {i, i + 2, i + 4}; return array[i]; }; + +// C99 6.5.2.5 Compound literals constraint 1: The type name shall specify an object type or an array of unknown size, but not a variable length array type. +// So check that VLA type compound literals are rejected (see https://github.com/llvm/llvm-project/issues/89835). +void vla(int n) { + int size = 5; + (void)(int[size]){}; // expected-warning {{use of an empty initializer is a C23 extension}} + // expected-error@-1 {{compound literal cannot be of variable-length array type}} + (void)(int[size]){1}; // expected-error {{compound literal cannot be of variable-length array type}} + (void)(int[size]){1,2,3}; // expected-error {{compound literal cannot be of variable-length array type}} + (void)(int[size]){1,2,3,4,5}; // expected-error {{compound literal cannot be of variable-length array type}} +} ``````````

[Sirraide]

Merging this for you since you don’t appear to have commit perms.

[github-actions[bot]]

@J-MR-T Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!