Page MenuHomePhabricator

[Sema] Fold VLAs to constant arrays in a few more contexts
ClosedPublic

Authored by erik.pilkington on Nov 5 2020, 11:54 AM.

Details

Summary

D89523 removed support for promoting VLAs to constant arrays when the bounds isn't an ICE, since this can result in miscompiling a conforming program that assumes that the array is a VLA. Promoting VLAs for fields is still supported, since clang doesn't support VLAs in fields, so no conforming program could have a field VLA.

This change is really disruptive for us (hundreds of projects are failing to compile with this change), so I think we should carve out two more cases where we promote VLAs which can't miscompile a conforming program:

  1. When the VLA appears in an ivar -- this seems like a corollary to the field thing
  2. When the VLA has an initializer -- VLAs can't have an initializer

Thanks for taking a look!

Diff Detail

Event Timeline

erik.pilkington requested review of this revision.Nov 5 2020, 11:54 AM

The direction here seems fine to me.

clang/include/clang/Sema/Sema.h
2478 ↗(On Diff #303204)

Can we set a HasInitializer flag on Declarator instead (much like we provide a FunctionDefinitionKind)? This flag seems a bit ad-hoc (and also doesn't cover C++ braced initializers as named).

Use a bit on Declarator instead of an ad-hoc bool parameter.

rsmith accepted this revision.Nov 29 2020, 11:51 PM

Thanks, that's very clean.

clang/lib/Sema/SemaDecl.cpp
6027

Typo "it we"

6899–6901

Is there a reason to make this C-specific? We support VLAs in C++ as an extension; it'd seem reasonable to do this folding as an extension too, if it's as simple as moving the new code out of the if (!C++).

clang/test/Sema/vla.c
104

Can you also add a positive test? Eg, a goto over one of the folded-to-constant cases, to show that we can jump over those. (As-is, this test ensures that we produce the warning, but not that we actually treat the array variable as not being a VLA.)

This revision is now accepted and ready to land.Nov 29 2020, 11:51 PM
This revision was automatically updated to reflect the committed changes.
erik.pilkington marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 7:04 AM
thakis added a subscriber: thakis.Dec 4 2020, 7:54 AM

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

Thanks for letting me know, should be fixed by: 4fa0dbd6885bc4a48ceecdbb0fca9638792762cf

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

Thanks for letting me know, should be fixed by: 4fa0dbd6885bc4a48ceecdbb0fca9638792762cf

Thanks for the fix! But just adding a triple means the existing test in that file is no longer tested with the win api, which is not ideal. (But probably also not a huge deal.)