Fix Issue 21097 - prevent stack allocation when destroying large aggregates #3467
Conversation
Thanks for your pull request and interest in making D better, @JohanEngelen! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + druntime#3467" |
5e12dec
to
c8fd138
Compare
885cf5a
to
9f6dc63
Compare
9f6dc63
to
efd561e
Compare
Adding
The problematic line is |
alias Unshared = T; | ||
// Avoid stack allocation by hacking to get to the init symbol. | ||
pragma(mangle, "_D" ~ T.mangleof[1..$] ~ "6__initZ") | ||
__gshared extern immutable typeof(T.init) initializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used in the else
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
I guess your comment is to static-if this away, but I would have to replicate the full static-if chain, and it does not harm to have it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move it into the else
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be a global variable (and what i remember is that making it static did not work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a fwd decl, so no harm done if unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird but should not block this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a fwd decl, so no harm done if unused.
Not for LDC:
FAILED: runtime/objects-unittest-debug/std/experimental/allocator/building_blocks/affix_allocator.o
cd /home/runner/work/ldc/ldc/runtime/phobos && /home/runner/work/ldc/ldc/bin/ldc2 -c --output-o -conf= -w -de -dip1000 -preview=dtorfields -g -link-defaultlib-debug -d-debug -d-version=StdUnittest -unittest -linkonce-templates -relocation-model=pic -I/home/runner/work/ldc/ldc/runtime/druntime/src -I/home/runner/work/ldc/ldc/runtime/phobos -of=/home/runner/work/ldc/ldc/runtime/objects-unittest-debug/std/experimental/allocator/building_blocks/affix_allocator.o std/experimental/allocator/building_blocks/affix_allocator.d
/home/runner/work/ldc/ldc/runtime/druntime/src/core/internal/lifetime.d(99): Error: Global variable type does not match previous declaration with same mangled name: `_D6__initZ`
/home/runner/work/ldc/ldc/runtime/druntime/src/core/internal/lifetime.d(99): Previous IR type: i64, const, non-thread-local
/home/runner/work/ldc/ldc/runtime/druntime/src/core/internal/lifetime.d(99): New IR type: i32, const, non-thread-local
ping |
Your test is not executed by |
fa60672
to
32e0aaa
Compare
Thanks @MoonlightSentinel . Added it to the makefile (really, this test framework we have here is so horrible...), and squashed all fixup commits. |
32e0aaa
to
21d863c
Compare
|
21d863c
to
1320e41
Compare
OK to rebase onto stable and merge? |
What was the failure?
Yes as this fixes a regression. |
import core.stdc.string : memset; | ||
memset(cast(void*) &chunk, 0, T.sizeof); | ||
} | ||
else static if (T.sizeof <= 16 && !hasElaborateAssign!T && __traits(compiles, (){ T chunk; chunk = T.init; })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiousity, why 16
instead of a larger value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this value is indeed pretty arbitrary. It's to limit the stack space used.
…alizer for large aggregates by preventing stack allocation
1320e41
to
5981d49
Compare
Rebased onto stable, PR targets stable now. |
Build fails because CI checks out phobos master instead of phobos stable. I'm going to tentatively force merge this. |
This fixes
destroy
andcore.internal.lifetime.emplaceInitializer
for large aggregates by preventing stack allocation for LDC and DMD.For DMD, it also fixes a codegen issue for large static array of structs where
destroy
/emplaceInitializer
would generate a string ofmov
instructions proportional to the size of the array.Intentionally did not add unittests for this issue here, but instead added a runnable test case to the testsuite, to prevent a very large unittest binary. Test is added in separate DMD PR: dlang/dmd#12509.Tests have been added to this PR.