Bug 16193 – opApply() doesn't heap allocate closure

Status
REOPENED
Severity
normal
Priority
P3
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2016-06-22T13:46:23Z
Last change time
2024-12-13T18:48:31Z
Keywords
pull, safe, wrong-code
Assigned to
No Owner
Creator
Marc Schütz
Moved to GitHub: dmd#19146 →

Comments

Comment #0 by schuetzm — 2016-06-22T13:46:23Z
// yy.d module yy; struct S { int opApply(int delegate(int) dg) { foreach(i; 1 .. 10) { int result = dg(i); if(result) return result; } return 0; } } // xx.d import yy; int* px, py; int foo() { int x = 0, y = 0; foreach(i; S.init) { px = &x; py = &y; y++; } return y; } void main() { import std.stdio; writeln(foo()); int z = 0; writeln("&x = ", px, " &y = ", py, " &z = ", &z); } # dmd -c xx.d # dmd -c yy.d # dmd -ofxx xx.o yy.o # ./xx prints: 9 &x = 7FFEAD3C47C0 &y = 7FFEAD3C47C4 &z = 7FFEAD3C47E8 (or similar) As can be seen, all local variables are allocated next to each other, evidently on the stack. This happens even though S.opApply() doesn't take its delegate as scope. I'm deliberately using separate compilation here to make sure that the compiler has no way to infer that the closure doesn't escape.
Comment #1 by mathias.lang — 2016-06-22T13:50:20Z
Why do you want it to allocate ? That can either be changed (with a BIG RED warning), or the specs can be updated to mention the delegate is always `scope`. Either way, I'm just puzzled what's the use case here.
Comment #2 by ketmar — 2016-06-23T11:50:47Z
(In reply to Mathias Lang from comment #1) > Why do you want it to allocate ? for consistency. > Either way, I'm just puzzled what's the use case here. less special cases to remember.
Comment #3 by schveiguy — 2016-06-23T13:36:26Z
(In reply to Marc Schütz from comment #0) > I'm deliberately using separate compilation here to > make sure that the compiler has no way to infer that the closure doesn't > escape. Separate compilation doesn't imply anything. The compiler can see all the source for opApply, so it could potentially make the decision that it doesn't escape. To prove the compiler is making this optimizing assumption, actually *do* escape the delegate, and see what happens. When I do this, it actually still does not create a closure: // yy.d: module yy; struct S { int delegate(int) escaped; int opApply(int delegate(int) dg) { escaped = dg; foreach(i; 1 .. 10) { int result = dg(i); if(result) return result; } return 0; } } // xx.d: import yy; import std.stdio; int* px, py; S s; int foo() { int x = 0, y = 0; foreach(i; s) { px = &x; py = &y; y++; writeln("y = ", y); } return y; } void main() { writeln(foo()); int z = 0; int *b = new int; writeln("&x = ", px, " &y = ", py, " &z = ", &z, " b = ", b); s.escaped(0); } output (purposely not using separate compilation to prove this has nothing to do with it): # dmd xx.d yy.d # ./xx y = 1 y = 2 y = 3 y = 4 y = 5 y = 6 y = 7 y = 8 y = 9 9 &x = 7FFF5F29E8E8 &y = 7FFF5F29E8EC &z = 7FFF5F29E920 b = 100C70000 y = 32768 Note the difference in address between the stack variables and the obvious heap variable. And note the invalid value for y for the escaped call since foo has gone out of scope. BTW, tested on a Mac. This is just a straight-up wrong-code bug. I'm expecting this is also a regression, but I'm too lazy to find the version where this properly worked :) However, I do know that dmd at one point ALWAYS allocated a closure, so this must have worked at some point.
Comment #4 by schuetzm — 2016-06-23T14:59:07Z
(In reply to Steven Schveighoffer from comment #3) > (In reply to Marc Schütz from comment #0) > > I'm deliberately using separate compilation here to > > make sure that the compiler has no way to infer that the closure doesn't > > escape. > > Separate compilation doesn't imply anything. The compiler can see all the > source for opApply, so it could potentially make the decision that it > doesn't escape. > Because of separate compilation, the compiler IMO _must not_ rely on the implementation details of the called function, it may only use the information available in the function signature. That's my understanding, at least: attribute inference must only happen for template and auto functions. Inlining is probably an exception, but I didn't enable that in this example.
Comment #5 by schveiguy — 2016-06-23T15:08:06Z
(In reply to Marc Schütz from comment #4) > Because of separate compilation, the compiler IMO _must not_ rely on the > implementation details of the called function, it may only use the > information available in the function signature. That's my understanding, at > least: attribute inference must only happen for template and auto functions. > Inlining is probably an exception, but I didn't enable that in this example. This is somewhat OT, but separate compilation doesn't imply that. If the implementation is available, it's available. If you change the implementation between compilations, you're in for a world of hurt. Consider that templates can be used with separate compilation, how would that fare? The reason D does not infer attributes for non-template non-auto functions is because you *could* provide a .di file that just contains prototypes, and then the inferred attributes are no longer correct. For a template or auto-return, the function would be invalid, so that is why we can infer there. In this case, however, we are talking about an optimization, and something decided by the caller, not the callee. So whether to allocate a closure can easily be decided by looking at all the code that will use that particular delegate instance and seeing if it escapes. It need not modify or infer any attributes of functions to achieve this.
Comment #6 by bugzilla — 2016-09-15T02:43:39Z
Not a fix for this bug, but Phobos should be fixed: https://github.com/dlang/phobos/pull/4787
Comment #7 by bugzilla — 2016-09-15T09:53:20Z
https://github.com/dlang/dmd/pull/6135 Also, this was not a regression.
Comment #8 by schveiguy — 2016-09-15T12:29:02Z
(In reply to Walter Bright from comment #7) > Also, this was not a regression. I distinctly remember at some point in the last 10 years that you HAD to mark opApply scope or it would allocate a closure. How is this not a regression? It's not terribly important if it gets fixed now, but I'm just surprised if this has always been the behavior.
Comment #9 by bugzilla — 2016-09-15T22:50:19Z
Comment #10 by bugzilla — 2016-09-15T22:52:31Z
It's possible that was true briefly, but it would have been a regression in itself because of its negative affect on legacy code.
Comment #11 by github-bugzilla — 2016-09-16T03:03:06Z
Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/f7819c898a05d28297024b7355d4ace94f1e4465 fix Issue 16193 - opApply() doesn't heap allocate closure https://github.com/dlang/dmd/commit/946a8f4e0eb488313e53da41ad6f81e3a778b9a8 Merge pull request #6135 from WalterBright/fix16193 fix Issue 16193 - opApply() doesn't heap allocate closure
Comment #12 by github-bugzilla — 2016-10-01T11:48:45Z
Commits pushed to stable at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/f7819c898a05d28297024b7355d4ace94f1e4465 fix Issue 16193 - opApply() doesn't heap allocate closure https://github.com/dlang/dmd/commit/946a8f4e0eb488313e53da41ad6f81e3a778b9a8 Merge pull request #6135 from WalterBright/fix16193
Comment #13 by github-bugzilla — 2016-11-12T02:59:41Z
Commit pushed to stable at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/aac715ff2597d256b5ffe8ecb397316abf32fb2e Revert "fix Issue 16193 - opApply() doesn't heap allocate closure" This reverts commit f7819c898a05d28297024b7355d4ace94f1e4465. Fixes issue 16678
Comment #14 by github-bugzilla — 2016-12-18T22:53:05Z
Commit pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/aac715ff2597d256b5ffe8ecb397316abf32fb2e Revert "fix Issue 16193 - opApply() doesn't heap allocate closure"
Comment #15 by github-bugzilla — 2016-12-27T14:41:03Z
Commit pushed to scope at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/aac715ff2597d256b5ffe8ecb397316abf32fb2e Revert "fix Issue 16193 - opApply() doesn't heap allocate closure"
Comment #16 by github-bugzilla — 2017-01-16T23:24:53Z
Commit pushed to newCTFE at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/aac715ff2597d256b5ffe8ecb397316abf32fb2e Revert "fix Issue 16193 - opApply() doesn't heap allocate closure"
Comment #17 by dfj1esp02 — 2017-01-17T16:13:59Z
Is it fixed or is it github winning the edit war? :)
Comment #18 by public — 2017-01-17T16:18:19Z
Comment #19 by nick — 2023-02-03T11:04:16Z
Code from comment #3 does seem to work now and prints: > y = 10 When calling `s.escaped(0);`
Comment #20 by robert.schadek — 2024-12-13T18:48:31Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/19146 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB