Fix issue 20809 - return statement might access memory from destructed temporary

[welkam]

This is an attempt at fixing this issue. I realized that I bit more than I can chew on this one.
What this patch provides:

1.An idea where the bug is and what kind of change is needed to fix it. (I copy pasted function and commented out code so that the peace of code that I want to run gets to run. To my surprise it worked and it passes all the tests. ¯_(ツ)_/¯)
2.A test case that is combination of issue 20401, issue 20809 and ldc-developers/ldc#3426

The problems:

1. Copy paste is not good engineering but I dont know exactly how to proceed forward. There is not enough information arriving at appendDtors() to make proper decision. One solution is to create new function specific to this case. Or pass additional information trough function parameters(kinda hard when its in else statement). And of course dont break more code.
2. -Attached test is run with -inline and it triggers Issue 20803 - [REG2.071.0] Objects are destroyed too early with -inline - No longer true

Solutions:
A. Some one uses this to kickstart a new pull request.
B. Some one helps with guidance because im not confident that I can make required changes and think of all possible cases to make sure it doesnt break something else.

[MoonlightSentinel]

2. Attached test is run with -inline and it triggers Issue 20803 - [REG2.071.0] Objects are destroyed too early with -inline.

run.d runs all permutations of "-inline -release -g -O (and -fPIC on non-windows targets) by default for runnable tests. But you can override this set with the PERMUTE_ARGS parameter, e.g. as -release -g -O.

[welkam]

Since no on said anything I decided to pass additional variable to toElemDtor(). With this patch if function returns ref then it will use a fix from #10577 aka current behavior. Otherwise it uses old behavior.
While this fixes initial reported case it does not fix a case reported by timon.gehr in the comments of this bugzilla issue.
P.s. working on this I formed a hate relationship with compound statement. I think it should be removed and replaced with lowering

[MoonlightSentinel]

Since no on said anything I decided to pass additional variable to toElemDtor(). With this patch if function returns ref then it will use a fix from #10577 aka current behavior. Otherwise it uses old behavior.

The current approach looks better. I would've preferred a default parameter as false isn't really descriptive though (but Walter might disagree on this).

While this fixes initial reported case it does not fix a case reported by timon.gehr in the comments of this bugzilla issue.

Any idea why?

[welkam]

a default parameter as false isn't really descriptive

Its not. I view this pull request as a first aid to stop the hemorrhaging(bleeding).

Any idea why?

There are 5 calls to appendDtors() in a visit(ReturnStatement s) function. The example in issue 20809 triggers call #5. The example in issue 20401 triggers call #4. And the example from timon.gehr triggers call #3. If you try to fix one issue then you break the others.
All these examples need different logic when adding destructor and there is not enough information in toElemDtor() to make the right decision. The proper fix would be to write more test cases. Then manually inline all calls of appendDtors() and toElemDtor() inside visit(ReturnStatement s) and then see where it leads. The whole ReturnStatement function needs refactoring.

If you would to asked my opinion then the whole compound statement needs to go. It caused bugs, is causing bugs and I believe it will cause more bugs. There is just too much complexity that is leaving semantic analysis.

[MoonlightSentinel]

Its not. I view this pull request as a first aid to stop the hemorrhaging(bleeding).

Fair enough.

All these examples need different logic when adding destructor and there is not enough information in toElemDtor() to make the right decision. The proper fix would be to write more test cases.

Thanks for the detailed explanation. Can't comment on the details because it don't know much about the backend but your approach sounds reasonable.

Had a short look at that function anyways and found some additional test cases that might help:

// Triggers #1
S getter() { return make(); }
assert(getter().a == 2);

// Triggers #2 (but no checks yet)
struct T
{
    double x, y;
    T opBinary(string op)(T) { return T(); }
}

T sqr(T x) { return x * x; }

[foerdi]

(I do not know, if this belongs here)

It look like this PR solve my issue #20809 in my project too. I have tested this PR with the latest dmd version (2.093) and all my unittest pass.

Thanks for your work.

[RazvanN7]

Hi @welkam ! Sorry for being late to the party. Could you maybe rebase this? I will try to get a review on this from @WalterBright

[RazvanN7]

@welkam could you maybe add a commit that contains "Fix Issue 20809" in the commit message so that dlang-bot picks it up and links it to bugzilla?

[WalterBright]

https://issues.dlang.org/show_bug.cgi?id=20809

[WalterBright]

return S(2).a;
generates:

S s; s.a = 2; p = &s.a; s.dtor(); return *p;

so we can see why it returns 0.

[WalterBright]

With this PR, this code is generated instead:

S s; s.a = 2; p = &s.a; i = *p; s.dtor(); return i;

which is correct. What disturbs me, however, is if a is something more complex than a basic type. Making a copy, for example, may be disallowed. Or it may contain more indirections that get destructed. What is really happening is a reference to an object is "live" beyond its destruction.

In order to fix this properly, the desired value should be copyable with a bit copy, and should not have a destructor, i.e. is POD (Plain Old Data).

Otherwise, the compiler should emit an error.

[WalterBright]

On further reflection, the rewrite should be in ReturnStatement's semantic routine:

return f().a;

should be, if f() returns a ref:

return tmp = f(); tmp;

and the rest of the semantic code will take care of copy construction, destructors, etc.