Skip to content

Reapply "[Clang][CWG1815] Support lifetime extension of temporary created by aggregate initialization using a default member initializer" #92527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -10041,12 +10041,6 @@ def warn_new_dangling_initializer_list : Warning<
"the allocated initializer list}0 "
"will be destroyed at the end of the full-expression">,
InGroup<DanglingInitializerList>;
def warn_unsupported_lifetime_extension : Warning<
"lifetime extension of "
"%select{temporary|backing array of initializer list}0 created "
"by aggregate initialization using a default member initializer "
"is not yet supported; lifetime of %select{temporary|backing array}0 "
"will end at the end of the full-expression">, InGroup<Dangling>;

// For non-floating point, expressions of the form x == x or x != x
// should result in a warning, since these always evaluate to a constant.
Expand Down
31 changes: 23 additions & 8 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5621,10 +5621,9 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
Res = Immediate.TransformInitializer(Param->getInit(),
/*NotCopy=*/false);
});
if (Res.isInvalid())
return ExprError();
Res = ConvertParamDefaultArgument(Param, Res.get(),
Res.get()->getBeginLoc());
if (Res.isUsable())
Res = ConvertParamDefaultArgument(Param, Res.get(),
Res.get()->getBeginLoc());
if (Res.isInvalid())
return ExprError();
Init = Res.get();
Expand Down Expand Up @@ -5660,7 +5659,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
Expr *Init = nullptr;

bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();

bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
EnterExpressionEvaluationContext EvalContext(
*this, ExpressionEvaluationContext::PotentiallyEvaluated, Field);

Expand Down Expand Up @@ -5695,19 +5694,35 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
ImmediateCallVisitor V(getASTContext());
if (!NestedDefaultChecking)
V.TraverseDecl(Field);
if (V.HasImmediateCalls) {

// CWG1815
// Support lifetime extension of temporary created by aggregate
// initialization using a default member initializer. We should always rebuild
// the initializer if it contains any temporaries (if the initializer
// expression is an ExprWithCleanups). Then make sure the normal lifetime
// extension code recurses into the default initializer and does lifetime
// extension when warranted.
bool ContainsAnyTemporaries =
isa_and_present<ExprWithCleanups>(Field->getInClassInitializer());
if (V.HasImmediateCalls || InLifetimeExtendingContext ||
ContainsAnyTemporaries) {
ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
CurContext};
ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
NestedDefaultChecking;

// Pass down lifetime extending flag, and collect temporaries in
// CreateMaterializeTemporaryExpr when we rewrite the call argument.
keepInLifetimeExtendingContext();
EnsureImmediateInvocationInDefaultArgs Immediate(*this);
ExprResult Res;

// Rebuild CXXDefaultInitExpr might cause diagnostics.
SFINAETrap Trap(*this);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we suppress diagnostic messages? Perhaps the initializer with errors should not be rebuilt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to detect if there is an error in the initializer? For example, access errors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get the question? The SFINAETrap IS suppressing at least some diagnostics.

We can see if there are already errors pre-transformation via containsError (or if it is nullptr, depending on RecoveryExpr). So if there was previously detected error, one of those should catch it.

Copy link
Contributor Author

@yronglin yronglin May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review! Here we add SFINAETrap Trap(*this). It was used to resolve the issue which looks like https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/dcl.decl/dcl.init/p14-0x.cpp .
A shorter reproducer:

class Private {
  Private(int); // expected-note {{here}}
public:
  Private();
};

class S {
  Private p = 42; // expected-error {{private constructor}}

  S() {} // expected-error {{call to deleted constructor of 'NoDefault'}} \
            expected-error {{must explicitly initialize the member 'e1' which does not have a default constructor}}
  S(int) {}
};

// FIXME: test the other forms which use copy-initialization

Field->getInClassInitializer() contains an bad access error, then we try to rebuild this initializer, the diagnostic will emit 3 times. (The result of Field->getInClassInitializer()->containsErrors() 3 times are false).
1st time: parser member initializer.
2nd time: After rebuild, Line 5726 Res = ConvertMemberDefaultInitExpression(Field, Res.get(), Loc);
3rd time: After rebuild, Line 5726 Res = ConvertMemberDefaultInitExpression(Field, Res.get(), Loc);

./error.cpp:8:15: error: field of type 'Private' has private constructor
    8 |   Private p = 42; // expected-error {{private constructor}}
      |               ^
./error.cpp:2:3: note: implicitly declared private here
    2 |   Private(int); // expected-note {{here}}
      |   ^
./error.cpp:8:15: error: field of type 'Private' has private constructor
    8 |   Private p = 42; // expected-error {{private constructor}}
      |               ^
./error.cpp:2:3: note: implicitly declared private here
    2 |   Private(int); // expected-note {{here}}
      |   ^
./error.cpp:8:15: error: field of type 'Private' has private constructor
    8 |   Private p = 42; // expected-error {{private constructor}}
      |               ^
./error.cpp:2:3: note: implicitly declared private here
    2 |   Private(int); // expected-note {{here}}
      |   ^
3 errors generated.

runWithSufficientStackSpace(Loc, [&] {
Res = Immediate.TransformInitializer(Field->getInClassInitializer(),
/*CXXDirectInit=*/false);
});
if (!Res.isInvalid())
if (Res.isUsable())
Res = ConvertMemberDefaultInitExpression(Field, Res.get(), Loc);
if (Res.isInvalid()) {
Field->setInvalidDecl();
Expand Down
3 changes: 0 additions & 3 deletions clang/lib/Sema/SemaExprCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1554,9 +1554,6 @@ Sema::BuildCXXTypeConstructExpr(TypeSourceInfo *TInfo,
bool ListInitialization) {
QualType Ty = TInfo->getType();
SourceLocation TyBeginLoc = TInfo->getTypeLoc().getBeginLoc();

assert((!ListInitialization || Exprs.size() == 1) &&
"List initialization must have exactly one expression.");
SourceRange FullRange = SourceRange(TyBeginLoc, RParenOrBraceLoc);

InitializedEntity Entity =
Expand Down
19 changes: 1 addition & 18 deletions clang/lib/Sema/SemaInit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8066,11 +8066,6 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
enum PathLifetimeKind {
/// Lifetime-extend along this path.
Extend,
/// We should lifetime-extend, but we don't because (due to technical
/// limitations) we can't. This happens for default member initializers,
/// which we don't clone for every use, so we don't have a unique
/// MaterializeTemporaryExpr to update.
ShouldExtend,
/// Do not lifetime extend along this path.
NoExtend
};
Expand All @@ -8082,7 +8077,7 @@ shouldLifetimeExtendThroughPath(const IndirectLocalPath &Path) {
PathLifetimeKind Kind = PathLifetimeKind::Extend;
for (auto Elem : Path) {
if (Elem.Kind == IndirectLocalPathEntry::DefaultInit)
Kind = PathLifetimeKind::ShouldExtend;
Kind = PathLifetimeKind::Extend;
else if (Elem.Kind != IndirectLocalPathEntry::LambdaCaptureInit)
return PathLifetimeKind::NoExtend;
}
Expand Down Expand Up @@ -8202,18 +8197,6 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
ExtendingEntity->allocateManglingNumber());
// Also visit the temporaries lifetime-extended by this initializer.
return true;

case PathLifetimeKind::ShouldExtend:
// We're supposed to lifetime-extend the temporary along this path (per
// the resolution of DR1815), but we don't support that yet.
//
// FIXME: Properly handle this situation. Perhaps the easiest approach
// would be to clone the initializer expression on each use that would
// lifetime extend its temporaries.
Diag(DiagLoc, diag::warn_unsupported_lifetime_extension)
<< RK << DiagRange;
break;

case PathLifetimeKind::NoExtend:
// If the path goes through the initialization of a variable or field,
// it can't possibly reach a temporary created in this full-expression.
Expand Down
12 changes: 8 additions & 4 deletions clang/lib/Sema/TreeTransform.h
Original file line number Diff line number Diff line change
Expand Up @@ -14102,6 +14102,13 @@ TreeTransform<Derived>::TransformCXXTemporaryObjectExpr(
if (TransformExprs(E->getArgs(), E->getNumArgs(), true, Args,
&ArgumentChanged))
return ExprError();

if (E->isListInitialization() && !E->isStdInitListInitialization()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is list initialization, but the constructor is called, and it is not the case of std::initializer_list, we should recreate an InitListExpr. https://godbolt.org/z/K4rzxosx3

ExprResult Res = RebuildInitList(E->getBeginLoc(), Args, E->getEndLoc());
if (Res.isInvalid())
return ExprError();
Args = {Res.get()};
}
}

if (!getDerived().AlwaysRebuild() &&
Expand All @@ -14113,12 +14120,9 @@ TreeTransform<Derived>::TransformCXXTemporaryObjectExpr(
return SemaRef.MaybeBindToTemporary(E);
}

// FIXME: We should just pass E->isListInitialization(), but we're not
// prepared to handle list-initialization without a child InitListExpr.
SourceLocation LParenLoc = T->getTypeLoc().getEndLoc();
return getDerived().RebuildCXXTemporaryObjectExpr(
T, LParenLoc, Args, E->getEndLoc(),
/*ListInitialization=*/LParenLoc.isInvalid());
T, LParenLoc, Args, E->getEndLoc(), E->isListInitialization());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue in #87933 caused by /*ListInitialization=*/LParenLoc.isInvalid().

}

template<typename Derived>
Expand Down
6 changes: 3 additions & 3 deletions clang/test/AST/ast-dump-default-init-json.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -789,10 +789,10 @@ void test() {
// CHECK-NEXT: "valueCategory": "lvalue",
// CHECK-NEXT: "extendingDecl": {
// CHECK-NEXT: "id": "0x{{.*}}",
// CHECK-NEXT: "kind": "FieldDecl",
// CHECK-NEXT: "name": "a",
// CHECK-NEXT: "kind": "VarDecl",
// CHECK-NEXT: "name": "b",
// CHECK-NEXT: "type": {
// CHECK-NEXT: "qualType": "const A &"
// CHECK-NEXT: "qualType": "B"
// CHECK-NEXT: }
// CHECK-NEXT: },
// CHECK-NEXT: "storageDuration": "automatic",
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ast-dump-default-init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ void test() {
}
// CHECK: -CXXDefaultInitExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue has rewritten init
// CHECK-NEXT: `-ExprWithCleanups 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue
// CHECK-NEXT: `-MaterializeTemporaryExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue extended by Field 0x{{[^ ]*}} 'a' 'const A &'
// CHECK-NEXT: `-MaterializeTemporaryExpr 0x{{[^ ]*}} <{{.*}}> 'const A' lvalue extended by Var 0x{{[^ ]*}} 'b' 'B'
// CHECK-NEXT: `-ImplicitCastExpr 0x{{[^ ]*}} <{{.*}}> 'const A' <NoOp>
// CHECK-NEXT: `-CXXFunctionalCastExpr 0x{{[^ ]*}} <{{.*}}> 'A' functional cast to A <NoOp>
// CHECK-NEXT: `-InitListExpr 0x{{[^ ]*}} <{{.*}}> 'A'
Expand Down
9 changes: 5 additions & 4 deletions clang/test/Analysis/lifetime-extended-regions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,11 @@ void aggregateWithReferences() {
clang_analyzer_dump(viaReference); // expected-warning-re {{&lifetime_extended_object{RefAggregate, viaReference, S{{[0-9]+}}} }}
clang_analyzer_dump(viaReference.rx); // expected-warning-re {{&lifetime_extended_object{int, viaReference, S{{[0-9]+}}} }}
clang_analyzer_dump(viaReference.ry); // expected-warning-re {{&lifetime_extended_object{Composite, viaReference, S{{[0-9]+}}} }}

// clang does not currently implement extending lifetime of object bound to reference members of aggregates,
// that are created from default member initializer (see `warn_unsupported_lifetime_extension` from `-Wdangling`)
RefAggregate defaultInitExtended{i}; // clang-bug does not extend `Composite`

// FIXME: clang currently support extending lifetime of object bound to reference members of aggregates,
// that are created from default member initializer. But CFG and ExprEngine need to be updated to address this change.
// The following expect warning: {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
RefAggregate defaultInitExtended{i};
clang_analyzer_dump(defaultInitExtended.ry); // expected-warning {{Unknown }}
}

Expand Down
2 changes: 0 additions & 2 deletions clang/test/CXX/drs/cwg16xx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,6 @@ namespace cwg1696 { // cwg1696: 7
const A &a = A(); // #cwg1696-D1-a
};
D1 d1 = {}; // #cwg1696-d1
// since-cxx14-warning@-1 {{lifetime extension of temporary created by aggregate initialization using a default member initializer is not yet supported; lifetime of temporary will end at the end of the full-expression}}
// since-cxx14-note@#cwg1696-D1-a {{initializing field 'a' with default member initializer}}

struct D2 {
const A &a = A(); // #cwg1696-D2-a
Expand Down
19 changes: 14 additions & 5 deletions clang/test/CXX/drs/cwg18xx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -206,19 +206,28 @@ namespace cwg1814 { // cwg1814: yes
#endif
}

namespace cwg1815 { // cwg1815: no
namespace cwg1815 { // cwg1815: 19
#if __cplusplus >= 201402L
// FIXME: needs codegen test
struct A { int &&r = 0; }; // #cwg1815-A
struct A { int &&r = 0; };
A a = {};
// since-cxx14-warning@-1 {{lifetime extension of temporary created by aggregate initialization using a default member initializer is not yet supported; lifetime of temporary will end at the end of the full-expression}} FIXME
// since-cxx14-note@#cwg1815-A {{initializing field 'r' with default member initializer}}

struct B { int &&r = 0; }; // #cwg1815-B
// since-cxx14-error@-1 {{reference member 'r' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object}}
// since-cxx14-note@#cwg1815-B {{initializing field 'r' with default member initializer}}
// since-cxx14-note@#cwg1815-b {{in implicit default constructor for 'cwg1815::B' first required here}}
B b; // #cwg1815-b

#if __cplusplus >= 201703L
struct C { const int &r = 0; };
constexpr C c = {}; // OK, since cwg1815
static_assert(c.r == 0);

constexpr int f() {
A a = {}; // OK, since cwg1815
return a.r;
}
static_assert(f() == 0);
#endif
#endif
}

Expand Down
34 changes: 34 additions & 0 deletions clang/test/CXX/special/class.temporary/p6.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,40 @@ void init_capture_init_list() {
// CHECK: }
}

void check_dr1815() { // dr1815: yes
#if __cplusplus >= 201402L

struct A {
int &&r = 0;
~A() {}
};

struct B {
A &&a = A{};
~B() {}
};
B a = {};

// CHECK: call {{.*}}block_scope_begin_function
extern void block_scope_begin_function();
extern void block_scope_end_function();
block_scope_begin_function();
{
// CHECK: call void @_ZZ12check_dr1815vEN1BD1Ev
// CHECK: call void @_ZZ12check_dr1815vEN1AD1Ev
B b = {};
}
// CHECK: call {{.*}}block_scope_end_function
block_scope_end_function();

// CHECK: call {{.*}}some_other_function
extern void some_other_function();
some_other_function();
// CHECK: call void @_ZZ12check_dr1815vEN1BD1Ev
// CHECK: call void @_ZZ12check_dr1815vEN1AD1Ev
#endif
}

namespace P2718R0 {
namespace basic {
template <typename E> using T2 = std::list<E>;
Expand Down
4 changes: 2 additions & 2 deletions clang/test/SemaCXX/constexpr-default-arg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ void test_default_arg2() {
}

// Check that multiple CXXDefaultInitExprs don't cause an assertion failure.
struct A { int &&r = 0; }; // expected-note 2{{default member initializer}}
struct A { int &&r = 0; };
struct B { A x, y; };
B b = {}; // expected-warning 2{{lifetime extension of temporary created by aggregate initialization using a default member initializer is not yet supported}}
B b = {}; // expected-no-diagnostics

}
74 changes: 74 additions & 0 deletions clang/test/SemaCXX/cxx11-default-member-initializers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,80 @@ class MemInit {
C m = s;
};

namespace std {
typedef decltype(sizeof(int)) size_t;

// libc++'s implementation
template <class _E> class initializer_list {
const _E *__begin_;
size_t __size_;

initializer_list(const _E *__b, size_t __s) : __begin_(__b), __size_(__s) {}

public:
typedef _E value_type;
typedef const _E &reference;
typedef const _E &const_reference;
typedef size_t size_type;

typedef const _E *iterator;
typedef const _E *const_iterator;

initializer_list() : __begin_(nullptr), __size_(0) {}

size_t size() const { return __size_; }
const _E *begin() const { return __begin_; }
const _E *end() const { return __begin_ + __size_; }
};
} // namespace std

#if __cplusplus >= 201703L
namespace test_rebuild {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more test that from #87933 's comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

template <typename T, int> class C {
public:
C(std::initializer_list<T>);
};

template <typename T> using Ptr = __remove_pointer(T) *;
template <typename T> C(T) -> C<Ptr<T>, sizeof(T)>;

class A {
public:
template <typename T1, typename T2> T1 *some_func(T2 &&);
};

struct B : A {
// Test CXXDefaultInitExpr rebuild issue in
// https://github.com/llvm/llvm-project/pull/87933
int *ar = some_func<int>(C{some_func<int>(0)});
B() {}
};

int TestBody_got;
template <int> class Vector {
public:
Vector(std::initializer_list<int>);
};
template <typename... Ts> Vector(Ts...) -> Vector<sizeof...(Ts)>;
class ProgramBuilder {
public:
template <typename T, typename ARGS> int *create(ARGS);
};

struct TypeTest : ProgramBuilder {
int *str_f16 = create<int>(Vector{0});
TypeTest() {}
};
class TypeTest_Element_Test : TypeTest {
void TestBody();
};
void TypeTest_Element_Test::TestBody() {
int *expect = str_f16;
&TestBody_got != expect; // expected-warning {{inequality comparison result unused}}
}
} // namespace test_rebuild
#endif // __cplusplus >= 201703L

#if __cplusplus >= 202002L
// This test ensures cleanup expressions are correctly produced
// in the presence of default member initializers.
Expand Down
6 changes: 2 additions & 4 deletions clang/test/SemaCXX/eval-crashes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ namespace pr33140_0b {
}

namespace pr33140_2 {
// FIXME: The declaration of 'b' below should lifetime-extend two int
// temporaries.
struct A { int &&r = 0; }; // expected-note 2{{initializing field 'r' with default member initializer}}
struct A { int &&r = 0; };
struct B { A x, y; };
B b = {}; // expected-warning 2{{lifetime extension of temporary created by aggregate initialization using a default member initializer is not yet supported}}
B b = {};
}

namespace pr33140_3 {
Expand Down
2 changes: 1 addition & 1 deletion clang/www/cxx_dr_status.html
Original file line number Diff line number Diff line change
Expand Up @@ -10698,7 +10698,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
<td><a href="https://cplusplus.github.io/CWG/issues/1815.html">1815</a></td>
<td>CD4</td>
<td>Lifetime extension in aggregate initialization</td>
<td class="none" align="center">No</td>
<td class="unreleased" align="center">Clang 19</td>
</tr>
<tr id="1816">
<td><a href="https://cplusplus.github.io/CWG/issues/1816.html">1816</a></td>
Expand Down
Loading