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
Fix Issue 22536 - CTFE: Missing destruction of array literal argument for scope slice parameter #13496
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @RazvanN7! 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
|
cc @kinke as this is blocking a fix of yours. |
Related issue with int previewIn()
{
int numDtor;
struct S
{
int x;
~this() { ++numDtor; }
}
static accept(in S s) {}
accept(S(1));
return numDtor;
}
static assert(previewIn() == 1); |
test/fail_compilation/fail22536.d
Outdated
TEST_OUTPUT: | ||
--- | ||
fail_compilation/fail22536.d(22): Error: uncaught CTFE exception `object.Exception("exception")` | ||
fail_compilation/fail22536.d(25): called from here: `foo(((S[2] __arrayliteral_on_stack3 = [S(1), S(2)];) , cast(S[])__arrayliteral_on_stack3))` |
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 could be improved, but I'll leave it for a different PR.
@MoonlightSentinel is this addition acceptable? |
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.
There's probably a more general solution for intermediate temporaries but this patch fixes the issue for now.
Please add #13496 (comment) to runnable/previewin.d
.
@MoonlightSentinel Done. |
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.
Destroying temporaries immediatly after the function actually violates the spec w.r.t. to the lifetime of temporaries.
That causes following test to pass at runtime but fail during CTFE:
struct Inc
{
int* ptr;
~this()
{
(*ptr)++;
}
}
int* foo(scope Inc[] arr)
{
return arr[0].ptr;
}
int main()
{
int i;
assert(*foo([ Inc(&i) ]) == 0);
return 0;
}
static assert(main() == 0);
Regarding my earlier comment, it might be better to maintain a shared dtors
array in the interpreter and destroy all entries at the end of a statement.
@MoonlightSentinel I have addressed your review. |
|
||
// destroy temporaries | ||
// https://issues.dlang.org/show_bug.cgi?id=22536 | ||
auto dtors = ctfeGlobals.dtorsForTemporaries.copy(); |
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.
Making a copy because the dtor also contains ExpStatements so when it is interpreted it will mess up the shared dtor array.
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.
Shouldn't this state be saved/restored for other statements too? ExpStatement
isn't the only statement that may contain expressions with temporaries, e.g. while (foo( [ ... ] ))
// destroy temporaries | ||
// https://issues.dlang.org/show_bug.cgi?id=22536 | ||
auto dtors = ctfeGlobals.dtorsForTemporaries.copy(); | ||
ctfeGlobals.dtorsForTemporaries.setDim(0); |
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.
I just noticed that setting the dim to 0 does not actually free any memory, it simply sets the length to 0. Since struct Array
uses a dynamic array underneath, it should also set the length of it. Currently, the memory is not freed until the actual Expression pointers (in this case) are overwritten by later element additions.
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.
Eagerly releasing the underlying memory might not be ideal if the array is reused immediately. But it should zero the "removed" elements to avoid false pointers with -lowmem
.
ctfeGlobals.dtorsForTemporaries.setDim(0); | |
ctfeGlobals.dtorsForTemporaries.setDim(0); | |
ctfeGlobals.zero(); |
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.
Looks better but doesn't respect the eager cleanup of temporaries of temporaries in ||
/ &&
:
struct S
{
const int num;
int* dtorCount;
this(int num, int* dtorCount)
{
this.num = num;
this.dtorCount = dtorCount;
if (num == 4)
assert(*dtorCount == 2); // Should've been destroyed already
}
~this()
{
const destr = ++(*dtorCount);
assert(num == destr);
}
}
int conditionals()
{
int dtorCount;
int* p = &dtorCount;
bool b = (S(6, p) == S(5, p) || S(2, p) != S(1, p)) && S(4, p) == S(3, p);
return 0;
}
static assert(conditionals() == 0);
if (auto commaExp = earg.isCommaExp()) | ||
{ | ||
if (auto declExp = commaExp.e1.isDeclarationExp()) | ||
{ | ||
if (auto vd = declExp.declaration.isVarDeclaration()) | ||
{ | ||
if (vd.edtor) | ||
ctfeGlobals.dtorsForTemporaries.push(vd.edtor); | ||
} | ||
} | ||
} |
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 doesn't belong here anymore with the more general solution. Dtors should probably be registered by visit(DeclarationExp)
s.t. it works without this special casing.
That should also fix the example from the spec:
bool b = (S(1) == S(2) || S(3) != S(4)) && S(5) == S(6);
Neither of those temporaries is currently destroyed.
@@ -0,0 +1,69 @@ | |||
// https://issues.dlang.org/show_bug.cgi?id=22536 | |||
|
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.
// PERMUTE_ARGS: |
This test doesn't need to be run with backend permutations
// destroy temporaries | ||
// https://issues.dlang.org/show_bug.cgi?id=22536 | ||
auto dtors = ctfeGlobals.dtorsForTemporaries.copy(); | ||
ctfeGlobals.dtorsForTemporaries.setDim(0); |
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.
Eagerly releasing the underlying memory might not be ideal if the array is reused immediately. But it should zero the "removed" elements to avoid false pointers with -lowmem
.
ctfeGlobals.dtorsForTemporaries.setDim(0); | |
ctfeGlobals.dtorsForTemporaries.setDim(0); | |
ctfeGlobals.zero(); |
|
||
// destroy temporaries | ||
// https://issues.dlang.org/show_bug.cgi?id=22536 | ||
auto dtors = ctfeGlobals.dtorsForTemporaries.copy(); |
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.
Shouldn't this state be saved/restored for other statements too? ExpStatement
isn't the only statement that may contain expressions with temporaries, e.g. while (foo( [ ... ] ))
… for scope slice parameter
… for scope slice parameter
c68f38c
to
deb357d
Compare
It appears that CTFE does not know how to destroy array literal temporaries that are put on stack. My solution first collects the temporary destructors that are created here [1] and interprets them after the function call was interpreted.
[1] https://github.com/dlang/dmd/blob/master/src/dmd/expressionsem.d#L2074