Skip to content
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

Aggregate parens-init checks for dangling too aggressively #61567

Closed
timsong-cpp opened this issue Mar 20, 2023 · 9 comments · Fixed by llvm/llvm-project-release-prs#446
Closed
Assignees
Labels
c++20 clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer release:backport release:merged

Comments

@timsong-cpp
Copy link

timsong-cpp commented Mar 20, 2023

Clang rejects the well-formed but dangling cases from http://eel.is/c++draft/dcl.init.general#example-3:

struct A {
  int a;
  int&& r;
};

int f();
int n = 10;

A a2(1, f());                   // well-formed, but dangling reference
A a4(1.0, 1);                   // well-formed, but dangling reference
<source>:9:9: error: reference member 'r' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object
A a2(1, f());                   // well-formed, but dangling reference
        ^~~
<source>:3:9: note: reference member declared here
  int&& r;
        ^
<source>:10:11: error: reference member 'r' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object
A a4(1.0, 1);                   // well-formed, but dangling reference
          ^
<source>:3:9: note: reference member declared here
  int&& r;

Now, these particular examples may well have justified a warning, but Clang also rejects

void f(A);

void g() {
    f(A(1, 1));
}

in which both the temporary and the aggregate live to the end of the full-expression. It also warns on

void h(int i) {
   f(A(1, static_cast<int&&>(i)));
}
<source>:16:31: warning: binding reference member 'r' to stack allocated parameter 'i' [-Wdangling-field]
    f(A(1, static_cast<int&&>(i)));
                              ^
<source>:3:9: note: reference member declared here
  int&& r;
        ^

even though i outlives the aggregate.

This will affect uses of C++23 std::ranges::elements_of, which is an aggregate with a reference member that is intended to be used exactly like this.

@EugeneZelenko EugeneZelenko added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed new issue labels Mar 20, 2023
@shafik
Copy link
Collaborator

shafik commented Mar 21, 2023

@alanzhao1 @zygoloid I think this is more fallout from #61145 please confirm

@alanzhao1
Copy link
Contributor

alanzhao1 commented Mar 21, 2023

I think this has a different root cause than #61145

The root cause of #61145 is that certain parts of the AST weren't being carried over during a TreeTransform, but there's no TreeTransform going on in this bug.

@zygoloid
Copy link
Collaborator

Yeah, I suspect the problem in this case is that we're incorrectly reusing the rules that we use in a constructor's mem-initializer-list (in which context this kind of initialization is ill-formed) here. We probably need a new kind of InitializedElement so we can distinguish these cases.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 12, 2023

@llvm/issue-subscribers-c-20

@alanzhao1
Copy link
Contributor

Candidate patch: https://reviews.llvm.org/D148274

@alanzhao1
Copy link
Contributor

@shafik any thoughts on backporting this into 16.0.x?

@alanzhao1
Copy link
Contributor

Reopening for backporting per discussion in https://reviews.llvm.org/D150122

I'll wait for #62266 to be backported before working on this fix as otherwise there'll be a merge issue.

@alanzhao1 alanzhao1 reopened this May 9, 2023
@alanzhao1 alanzhao1 added this to the LLVM 16.0.X Release milestone May 9, 2023
alanzhao1 added a commit to alanzhao1/llvm-project that referenced this issue May 15, 2023
…gate initialization

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 llvm#61567

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

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D148274
@alanzhao1
Copy link
Contributor

/branch alanzhao1/llvm-project/backport-61567

@llvmbot
Copy link
Collaborator

llvmbot commented May 15, 2023

/pull-request llvm/llvm-project-release-prs#446

tstellar pushed a commit that referenced this issue May 31, 2023
…gate initialization

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

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D148274
karouzakisp pushed a commit to karouzakisp/llvm-project that referenced this issue Jul 20, 2024
…gate initialization

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 llvm#61567

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

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D148274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++20 clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer release:backport release:merged
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants