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.

Sign up for GitHub

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

Jump to bottom

Fix issue 20401 - The glue shouldn't copy side effects by value where the lvalue is wanted #10577

Merged
merged 1 commit into from Nov 23, 2019
Merged

Fix issue 20401 - The glue shouldn't copy side effects by value where the lvalue is wanted #10577

merged 1 commit into from Nov 23, 2019

Conversation

SSoulaimane
Copy link
Member

@SSoulaimane SSoulaimane commented Nov 17, 2019 •
edited

Issue 20401.

@SSoulaimane SSoulaimane marked this pull request as ready for review November 17, 2019 05:15
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @SSoulaimane! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 20401 major ref variable copied before return

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10577"

@thewilsonator
Copy link
Contributor

Please use a while loop instead of a for, otherwise looks good.

kinke added a commit to kinke/ldc that referenced this pull request Nov 17, 2019
@kinke
Fix DMD issue 20401
e02050a 
See https://issues.dlang.org/show_bug.cgi?id=20401; a testcase will be
added with dlang/dmd#10577.

It's a pretty serious issue (the previously returned reference in the
testcase was a dangling reference to a local memcopy of the original
value in memory), hence this early fix.
@kinke kinke mentioned this pull request Nov 17, 2019
Merged
@SSoulaimane
Copy link
Member Author

Done.
This should probably target stable. What do you think?

n8sh
n8sh reviewed Nov 17, 2019
View reviewed changes
src/dmd/e2ir.d
{
/* Expensive to copy, to take a pointer to it instead
Copy link
Member

Choose a reason for hiding this comment

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

Not retaining this behavior for expensive-to-copy things?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is retained actually, structs bigger than two registers match the given definition of lvalue, i.e they are always lvalues in the backend regardless of the frontend's judgement. As far as the backend is concerned what lives in registers is an rvalue and what lives in memory is an lvalue.

@SSoulaimane
Fix issue 20401 - The glue shouldn't copy side effects by value where…
4cfff1c 
… the lvalue is wanted

By ref semantics should be preserved when extracting side effects.
@RazvanN7
Copy link
Contributor

This should probably target stable. What do you think?

I think we're fine with master for now.

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 19, 2019
@thewilsonator thewilsonator added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Nov 23, 2019
@dlang-bot dlang-bot merged commit b0cdc91 into dlang:master Nov 23, 2019
kinke added a commit to ldc-developers/ldc that referenced this pull request Nov 23, 2019
@kinke
Fix DMD issue 20401 (#3233)
9a87d45 
See https://issues.dlang.org/show_bug.cgi?id=20401; a testcase will be
added with dlang/dmd#10577.

It's a pretty serious issue (the previously returned reference in the
testcase was a dangling reference to a local memcopy of the original
value in memory), hence this early fix.
@MoonlightSentinel
Copy link
Contributor

This introduced a regression: https://issues.dlang.org/show_bug.cgi?id=20809

@welkam welkam mentioned this pull request Jun 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n8sh n8sh n8sh left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

Assignees
No one assigned
Labels
auto-merge Bug Fix
Projects
None yet
Milestone
No milestone
6 participants
@SSoulaimane @dlang-bot @thewilsonator @RazvanN7 @MoonlightSentinel @n8sh