This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add tests for statement expression in initializers
ClosedPublic

Authored by wanders on Jun 7 2022, 4:37 AM.

Details

Summary

The commit 683e83c5

[Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

fixed a code generation bug when using (C-extension) statement
expressions inside initializer expressions.

Before that commit a nested static initializer inside the statement
expression would not be emitted, causing it to be zero initialized.

It is a bit surprising (at least to me) that a commit implementing a new
C++ feature would fix this code generation bug. Zooming in it is the
change done in ExprConstant.cpp that helps. That changes so that
"ESR_Failed" is returned in more cases, causing the expression to not be
deemed constant. This fixes the code generation as instead the compiler
has to resort to generating a dynamic initializer.

That commit also meant that some statement expressions (in particular
the ones using static variables) that previously were accepted now are
errors due to not being constant (matching GCC behavior).

Given how a seemingly unrelated change caused this behavior to change,
it is probably a good thing to add at least some rudimentary tests for
these kind expressions.

Diff Detail

Event Timeline

wanders created this revision.Jun 7 2022, 4:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 4:37 AM
wanders requested review of this revision.Jun 7 2022, 4:37 AM

One week gentle ping.
+ added reviewers from the https://reviews.llvm.org/D111400 patch that fixed the bug these tests covers.

The command line on one of the tests is a little strange to me, and I don't see the need for the -std=gnu11 on either, but the test content itself looks fine enough to me.

clang/test/CodeGen/stmtexpr-init.c
2

why the -std=gnu11 here? I would assume our default would be enough? Also, why the opt-flag? That likely is unnecessary/will cause 'bad things' to happen. Clang tests typically don't use the opt flags.

Thanks for adding some more test coverage! Would it be worth mentioning the miscompile fix in the release notes more explicitly?

clang/test/Sema/stmtexpr-init.c
2

I'd remove the gnu11 from here as well (we default to being in GNU mode anyway).

Thanks for adding some more test coverage! Would it be worth mentioning the miscompile fix in the release notes more explicitly?

Yes, might be a good idea, in particualar as some cases which were accepted (but miscompiled) now gives compilation errors.

Will add something to release notes.

clang/test/CodeGen/stmtexpr-init.c
2

why the -std=gnu11 here? I would assume our default would be enough? Also, why the opt-flag? That likely is unnecessary/will cause 'bad things' to happen. Clang tests typically don't use the opt flags.

Right, gnu11 shouldn't be needed.

The -O flag was there as that made the optimizer remove some of the dynamic initialization things making it easier to see the pattern to match on. But I see now that the checks I ended up doing matches just fine without it.

So will drop these. Thanks.

wanders updated this revision to Diff 436561.Jun 13 2022, 2:11 PM
wanders added a subscriber: kimgr.

Updated diff with:

  • Release note
  • Cleaned up command line options in RUN commands
  • Fixed type mimatch pointed out by @kimgr (out of band)
aaron.ballman accepted this revision.Jun 14 2022, 5:39 AM

LGTM! (The precommit CI failures appear to be unrelated to the changes in this patch.) Thanks for the additional test coverage!

This revision is now accepted and ready to land.Jun 14 2022, 5:39 AM
erichkeane accepted this revision.Jun 14 2022, 5:50 AM
This revision was landed with ongoing or failed builds.Jun 14 2022, 2:18 PM
This revision was automatically updated to reflect the committed changes.