This is an archive of the discontinued LLVM Phabricator instance.

[clang] add -Wvla-stack-allocation
Needs ReviewPublic

Authored by Origami404 on Nov 3 2022, 10:05 AM.

Details

Reviewers
aaron.ballman
efriedma
rjmccall
Group Reviewers
Restricted Project
Summary

New behaviors:

-Wvla: produces portability warning for all VLAs like before
-Wvla-portability: not exists (equal to -Wvla)
-Wvla-stack-allocation -Wno-vla: only warns for VLAs that need stack allocation
-Wvla -Wvla-stack-allocation: warn for VLAs that need stack allocation, and give portablility warnings for other VLAs

Note that only one warning can be produced by one VLA because of
implementation complexity. Currently, VLAs are detected in
BuildArrayType, which will be used both by Parser and Tree Transformer.
And it passes the VLA-related warnings as an argument to other
functions. Producing multiple warnings for one VLA needs to change many
functions.

Fixes: https://github.com/llvm/llvm-project/issues/57098
Link: https://reviews.llvm.org/D132952

Co-authored-by: YingChi Long <me@inclyc.cn>

Diff Detail

Event Timeline

Origami404 created this revision.Nov 3 2022, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 10:05 AM
Origami404 edited the summary of this revision. (Show Details)Nov 3 2022, 10:08 AM
Origami404 edited the summary of this revision. (Show Details)
Origami404 added reviewers: aaron.ballman, efriedma.
Origami404 added a subscriber: inclyc.
Origami404 published this revision for review.Nov 3 2022, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 10:10 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

We may need a release note here so users know about the new changes to -Wvla. See clang/docs/ReleaseNotes.rst!

Update release notes.

aaron.ballman added reviewers: Restricted Project, rjmccall.Dec 7 2022, 10:58 AM

ping :)

Thank you for your patience until I could get to this! I've added a few more reviewers for additional opinions on the correct design. I think what you have here is kind of close to what I was expecting, but with differences in the warning flags we expose. I had the impression we were looking for:

-Wvla-stack-allocation -- warns on use of a VLA that involves a stack allocation
-Wvla-portability -- warns on any use of a VM type, except if -Wvla-stack-allocation is also enabled it won't warn on use of VLA that involves a stack allocation (because the other warning already alerts the user to the portability concern).
-Wvla -- groups together -Wvla-stack-allocation and -Wvla-portability so people can turn on all VLA-related diagnostics at once.

The end result is that the use of -Wvla is the same as it's always been, but users can decide to use one of the more fine-grained diagnostics if they want to reduce the scope.

That said, based on the discussion in https://github.com/llvm/llvm-project/issues/59010, there may actually be a third kind of diagnostic we want to consider as well: -Wvla-bounds-[not]-evaluated that triggers on code like:

int foo(void);

void bar(void) {
  sizeof(int[foo()]); // foo is called
  sizeof(sizeof(int[foo()])); // foo is not called
}

void baz(int what[foo()]); // foo is not called
void baz(int what[foo()]) { // foo is called
  extern void whee(int what[foo()]); // foo is not called
}

If this is worth doing, it's probably best done in a separate patch though...

clang/lib/Sema/SemaType.cpp
2543–2547

FWIW, I don't think this is a behavior we need to retain. I think we issued that warning because it was easier than suppressing it, but the error diagnostic suffices to let the user know what's gone wrong in this particular case.

2553–2559

I'm not convinced the logic here is correct yet. Consider cases like:

int foo(void);

void bar(int a, int b[*]); // variable length array used, correct
void bar(int a, int b[a]) { variable length array used, correct
  int x[foo()]; // variable length array that may require stack allocation used, correct

  (void)sizeof(int[foo()]); // variable length array that may require stack allocation used, incorrect and the diagnostic triggers twice

  int (*y)[foo()]; // variable length array that may require stack allocation used, incorrect, this is a pointer to a VLA
}

One thing that may help is to double-check your test cases against the LLVM IR generated by the compiler; if there's an actual stack allocation, then there will be an alloca call in the IR for it.

clang/test/Sema/warn-vla.c
34

For example, there's no stack allocation happening here; test4_num is being evaluated but not to form an allocation.

int foo(void);

void bar(int a, int b[*]); // variable length array used, correct
void bar(int a, int b[a]) { variable length array used, correct
  int x[foo()]; // variable length array that may require stack allocation used, correct

  (void)sizeof(int[foo()]); // variable length array that may require stack allocation used, incorrect and the diagnostic triggers twice

  int (*y)[foo()]; // variable length array that may require stack allocation used, incorrect, this is a pointer to a VLA
}

Thanks to ballman's example here, it remains me that when non-LCE appears as an array index in a variable declaration, it is still possible to be a part of a pointer/function prototype instead of a VLA. Classifying different VLA usages requires more information about the declaration.

The old VLA checking implements just insert checking for the location in array type construction when a non-ICE is used as the array's length. And it assumes that only one error or warning could be produced by one VLA usage. So the old method seems hard to be extended to support the new checking on VLAs.

I plan to throw away the old method here, trying to use "eval functions" on AST of declarations or LLVM IR to classify different kinds of VLAs.

Since I am preparing for several final examinations next week, I will try to give a new implementation in about a week. And in advance, sorry for my late response next week.

uecker added a subscriber: uecker.Dec 11 2022, 12:23 PM

I would still suggest to change the behavior of -Wvla to warn only about VLAs which are allocated on the stack as the most useful warning. Portability warnings should be turned on for -Wc++-compat.

[clang] add -Wvla-stack-allocation and divide different VLA warning

New behaviors:

-Wvla-stack-allocation

warns on use of a VLA that involves a stack allocation

-Wvla-portability

warns on any use of a VM type, even if -Wvla-stack-allocation is
given.

-Wvla

groups together -Wvla-stack-allocation and -Wvla-portability

Fixes: https://github.com/llvm/llvm-project/issues/57098
Link: https://reviews.llvm.org/D132952

Co-authored-by: YingChi Long <me@inclyc.cn>

Differential Revision: https://reviews.llvm.org/D137343

Hello, I am back now and will be available anytime next week, so if anyone has any idea on this topic, please at me!

I haven't found a way that "feels right" to achieve consensus yet. But I do have something to share currently.


For the semantics of the warning flag, I agree with @uecker that -Wvla-portability should be changed to -Wc++-compat. In my reading of C2x standard, using VM type is acceptable and the only reason we need to issue a warning is that the C++ standard doesn't allow non-constant array length at all. But I have no opinion on the semantics of -Wvla and -Wvla-stack-alloc.

And should -Wvla-stack-alloc fires on static VLAs? (like clang/test/drs/dr3xx.c:164)


For implement details, I add a diag at ``, which will be called after a variable declaration. Here we can get the full type of the variable to tell whether the variable has a VLA type or just a VM type.

In the GitHub issue, @mizvekov said:

A general check about any VLA usage at all should probably happen early when we finished parsing and are building VariableArray types, probably in Sema::BuildArrayType.

Otherwise sprinkling around checks for VM types seems messy, so I don't think considering VM for this problem is an efficient approach.

But AFAIK, a VLA type cannot be told just at Sema::BuildArrayType level because the constructing VLA type may just be a part of another VM type but not a VLA type for a VLA. At Sema::BuildArrayType, we seem only can detect VM type instead of VLA type.


In Ballman's summary, the -Wvla-stack-alloc should suppress the -Wvla-portability:

-Wvla-portability warns on any use of a VM type, except if -Wvla-stack-allocation is also enabled it won't warn on the use of VLA that involves a stack allocation

I failed to achieve the "except" here because, in the current implementation, VM type-related warnings are checked directly at Sema::BuildArrayType, by checkArraySize which used Sema::VerifyIntegerConstantExpression. But the Sema::VerifyIntegerConstantExpression requires at least one diag at each non-ICE occurrence. I haven't found a way to avoid the diag yet.


By the way, I seem to find a bug in the current implementation of VM type warning. For VLA at sizeof context, the warning will be issued twice. Github issue here. It cause the test clang/test/Sema/warn-vla.c:48 giving two warnings.


The reason why I set both -Wvla-stack-alloc and -Wvla-portability to DefaultIgnore is that:

  • if both of them are not DefaultIgnore, 200 tests will fail
  • if only -Wvla-stack-alloc is not DefaultIgnore, 169 tests will fail
  • if both of them are DefaultIgnore, only 4 tests will fail

Most of the failed tests seem cannot be avoided because they just use VLAs. If DefaultIgnore is strongly unrecommended, I can go through all the tests and add an expected warning diag for them. It's a bit time-consuming, but I am free all day now.

Origami404 marked an inline comment as done.Dec 17 2022, 6:41 PM
Origami404 marked an inline comment as done.

Aaron's suggested design makes the most sense to me, of all the designs I've seen here.