This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization
ClosedPublic

Authored by ayzhao on Apr 13 2023, 3:37 PM.

Details

Summary

Before this patch, initialized class members would have the LifetimeKind
LK_MemInitializer, which does not allow for binding a temporary to a
reference. Binding to a temporary however is allowed in parenthesized
aggregate initialization, even if it leads to a dangling reference. To
fix this, we create a new EntityKind, EK_ParenAggInitMember, which has
LifetimeKind LK_FullExpression.

This patch does *not* attempt to diagnose dangling references as a
result of using this feature.

This patch also refactors TryOrBuildParenListInitialization(...) to
accomodate creating different InitializedEntity objects.

Fixes #61567

[0]: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html

Diff Detail

Event Timeline

ayzhao created this revision.Apr 13 2023, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 3:37 PM
ayzhao requested review of this revision.Apr 13 2023, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 3:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ayzhao updated this revision to Diff 513377.Apr 13 2023, 3:43 PM

fix missing EOF newline

clang/include/clang/Basic/DiagnosticSemaKinds.td
2130

This was missing an entry for EK_BINDING, so I added one (otherwise, we hit an assertion failure).

clang/test/SemaCXX/paren-list-agg-init.cpp
138

This turned out to be a bug in the original implementation. Currently, diagnostics are only emitted for class members if we fail a parenthesized aggregate init. However, this used to not be the case with value initialized members, which emits the diagnostic for the class itself. Refactoring TryOrBuildParenListInit(...) fixes this and makes things consistent.

shafik added a subscriber: shafik.Apr 14 2023, 2:11 PM
shafik added inline comments.
clang/include/clang/Sema/Initialization.h
400
511

Does this really apply? It looks like this function is only used in canPerformArrayCopy

clang/lib/Sema/SemaAccess.cpp
1657

It looks like don't have a test that triggers this diagnostic, I was looking for field of type.*has.*

clang/lib/Sema/SemaInit.cpp
583

I don't see a test triggering this diagnostic for parens init.

595

I don't see a test triggering this diagnostic for parens init.

shafik added inline comments.Apr 14 2023, 2:39 PM
clang/lib/Sema/SemaInit.cpp
5338

Do we have tests that trigger this diagnostic and the one right below?

9273

I don't see any tests that checks this set of diagnostics for parens init.

ayzhao updated this revision to Diff 513805.Apr 14 2023, 4:42 PM
ayzhao marked 7 inline comments as done.

Address code review comments and (try) to fix Windows bot failure

clang/include/clang/Sema/Initialization.h
511

Reverted.

Also reverted isDefaultMemberInitializer since this is never true for EK_ParenAggInitMember

clang/lib/Sema/SemaInit.cpp
583

Removed since this is part of InitListChecker and InitializedEntity::EK_ParenAggInitMember does not get created when an InitListExpr is created.

595

Removed.

5338

Yes:

ayzhao updated this revision to Diff 513808.Apr 14 2023, 4:59 PM

fix comments

shafik added inline comments.Apr 17 2023, 12:01 PM
clang/lib/Sema/SemaInit.cpp
9743

This and the changes below looks like unrelated formatting changes.

ayzhao updated this revision to Diff 514389.Apr 17 2023, 1:39 PM
ayzhao marked an inline comment as done.

remove unrelated formatting changes

I am mostly done but others should also look at this.

clang/lib/Sema/SemaInit.cpp
5348

Why not just use a conditional expression here or alternatively just turn it into an IIILE?

5394
5421
ayzhao updated this revision to Diff 514425.Apr 17 2023, 2:51 PM
ayzhao marked 3 inline comments as done.

code review comments + rebase

rsmith added inline comments.Apr 24 2023, 10:33 AM
clang/lib/Sema/SemaInit.cpp
5366–5371

What happens if the array is of VariableArrayType or DependentSizedArrayType? I guess we shouldn't get here in the latter case, but the former case seems possible, and presumably shouldn't result in constructing a value of ConstantArrayType. Test:

constexpr int f(int n, int i) {
    int arr[n](1, 2, 3);
    return arr[i];
}

constexpr int a = f(1, 2);
constexpr int b = f(4, 3);

GCC appears to leave the type alone in this case, and treats the evaluation as UB if n is less than the number of initializers given. That matches what GCC does for a {...} initializer of a VLA. We should probably match what Clang does for {...} initialization of a VLA and reject.

5393–5395

It would be nice to use the original type here in the case where we didn't add an array bound, so we preserve type sugar (typedefs etc). Also, do we ever need to preserve type qualifiers from the original entity's type?

5403

Does this have the same wrong-lifetime-kind problem as members did?

5477

Does this entity kind do the right thing for lifetime warnings? (I'm not sure why this is a distinct kind of InitializedEntity; the thing that changes here is not the entity, it's how it's initialized.)

5487–5488

Is there any possibility of lifetime warnings here? I don't *think* value initialization can ever create problems, but it would seem more obviously right to use the parenthesized aggregate initialization entity kind here.

ayzhao updated this revision to Diff 517246.Apr 26 2023, 11:13 AM
ayzhao marked 5 inline comments as done.

code review comments + rebase

clang/lib/Sema/SemaInit.cpp
5366–5371

Clang simply does not allow braced-initialization for VariableArrayType, even if n is greater than or equal to the number of initializers given: https://godbolt.org/z/MaPjvn694. I updated the patch to make parenthesized aggregate initialization do the same thing.

5393–5395

It would be nice to use the original type here in the case where we didn't add an array bound, so we preserve type sugar (typedefs etc).

Done

Also, do we ever need to preserve type qualifiers from the original entity's type?

I don't think so. In the case of IncompleteArrayType, which is the only situation where we would need to deduce the ConstantArrayType based on the number of arguments, the corresponding QualType object is always created with Quals = 0 [0].

[0]: https://github.com/llvm/llvm-project/blob/c95533a7be2858893ec32b8abaa37a2d912ebe63/clang/lib/AST/ASTContext.cpp#L3888

5403

I don't think so:

  1. This will never initialize a reference because it's initializing a base class.
  2. In the case where the constructor of a base class accepts a reference parameter, the lifetime is not extended: https://godbolt.org/z/MaqhTr1rP
5477

This now uses InitializeMemberFromParenAggInit(..) (before, it would emit the same error it emits for braced init lists). I also added a test for this.

5487–5488

GCC rejects value initialization of references: https://godbolt.org/z/nhhehrf7r

This patch currently rejects value initialization of references, but I updated the diagnostics to match those Clang emits with braced initialization: https://godbolt.org/z/rbh17ecqe

ayzhao updated this revision to Diff 517249.Apr 26 2023, 11:19 AM

add another test and fix whitespace

ayzhao updated this revision to Diff 517250.Apr 26 2023, 11:20 AM

add another test

LGTM, I'll leave approval to @shafik :)

clang/lib/Sema/SemaInit.cpp
5370

Can we assert that we have an IncompleteArrayType here? (That is, that we don't have a DependentSizedArrayType.)

5507

Is there a reason to not use InitializeMemberFromParenAggInit here? That'd mean we use the same InitializedEntity for every case in this loop, and could pull it out of the individual if branches.

ayzhao updated this revision to Diff 517699.Apr 27 2023, 1:42 PM
ayzhao marked an inline comment as done.

code review comments + pull in D149301

ayzhao marked an inline comment as done.

I need to look at this more carefully but can we add a release note in the meantime

ayzhao updated this revision to Diff 518044.Apr 28 2023, 1:53 PM

add release note

shafik accepted this revision.Apr 28 2023, 6:11 PM

LGTM

This revision is now accepted and ready to land.Apr 28 2023, 6:11 PM
ayzhao updated this revision to Diff 518479.May 1 2023, 10:01 AM

rebase + fix merge conflicts

This revision was landed with ongoing or failed builds.May 1 2023, 10:02 AM
This revision was automatically updated to reflect the committed changes.