Fix Issue 22536 - CTFE: Missing destruction of array literal argument for scope slice parameter by RazvanN7 · Pull Request #13496 · dlang/dmd

RazvanN7 · 2022-01-06T15:26:14Z

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

dlang-bot · 2022-01-06T15:26:18Z

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 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
✓	22536	blocker	CTFE: Missing destruction of array literal argument for scope slice parameter

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

Testing this PR locally

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

dub run digger -- build "master + dmd#13496"

RazvanN7 · 2022-01-06T15:26:28Z

cc @kinke as this is blocking a fix of yours.

MoonlightSentinel · 2022-01-06T16:11:58Z

Related issue with -preview=in that ~~might be~~ is fixed by this patch:

int previewIn()
{
    int numDtor;

    struct S
    {
        int x;
        ~this() { ++numDtor; }
    }

    static accept(in S s) {}
    accept(S(1));
    return numDtor;
}

static assert(previewIn() == 1);

src/dmd/dinterpret.d

RazvanN7 · 2022-01-07T10:59:02Z

test/fail_compilation/fail22536.d

+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))`


This could be improved, but I'll leave it for a different PR.

RazvanN7 · 2022-01-10T10:12:59Z

@MoonlightSentinel is this addition acceptable?

MoonlightSentinel

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.

src/dmd/dinterpret.d

RazvanN7 · 2022-01-11T10:02:50Z

@MoonlightSentinel Done.

MoonlightSentinel

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.

RazvanN7 · 2022-02-01T13:04:38Z

@MoonlightSentinel I have addressed your review.

RazvanN7 · 2022-02-01T13:05:46Z

src/dmd/dinterpret.d

+
+        // destroy temporaries
+        // https://issues.dlang.org/show_bug.cgi?id=22536
+        auto dtors = ctfeGlobals.dtorsForTemporaries.copy();


Making a copy because the dtor also contains ExpStatements so when it is interpreted it will mess up the shared dtor array.

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( [ ... ] ))

RazvanN7 · 2022-02-01T13:08:18Z

src/dmd/dinterpret.d

+        // destroy temporaries
+        // https://issues.dlang.org/show_bug.cgi?id=22536
+        auto dtors = ctfeGlobals.dtorsForTemporaries.copy();
+        ctfeGlobals.dtorsForTemporaries.setDim(0);


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.

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.

Suggested change

ctfeGlobals.dtorsForTemporaries.setDim(0);

ctfeGlobals.dtorsForTemporaries.setDim(0);

ctfeGlobals.zero();

MoonlightSentinel

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);

MoonlightSentinel · 2022-02-04T12:08:48Z

src/dmd/dinterpret.d

+        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);
+                }
+            }
+        }


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.

MoonlightSentinel · 2022-02-04T12:24:14Z

test/runnable/test22536.d

@@ -0,0 +1,69 @@
+// https://issues.dlang.org/show_bug.cgi?id=22536
+


Suggested change

// PERMUTE_ARGS:

This test doesn't need to be run with backend permutations

MoonlightSentinel · 2022-02-04T12:55:49Z

src/dmd/dinterpret.d

+        // destroy temporaries
+        // https://issues.dlang.org/show_bug.cgi?id=22536
+        auto dtors = ctfeGlobals.dtorsForTemporaries.copy();
+        ctfeGlobals.dtorsForTemporaries.setDim(0);


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.

Suggested change

ctfeGlobals.dtorsForTemporaries.setDim(0);

ctfeGlobals.dtorsForTemporaries.setDim(0);

ctfeGlobals.zero();

MoonlightSentinel · 2022-02-04T13:01:11Z

src/dmd/dinterpret.d

+
+        // destroy temporaries
+        // https://issues.dlang.org/show_bug.cgi?id=22536
+        auto dtors = ctfeGlobals.dtorsForTemporaries.copy();


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

dlang-bot added the Severity:Bug Fix label Jan 6, 2022

MoonlightSentinel self-requested a review January 6, 2022 15:41

MoonlightSentinel reviewed Jan 6, 2022

View reviewed changes

src/dmd/dinterpret.d Outdated Show resolved Hide resolved

RazvanN7 force-pushed the Issue_22536 branch from 2fbe929 to 3c4617b Compare January 7, 2022 10:58

RazvanN7 commented Jan 7, 2022

View reviewed changes

thewilsonator approved these changes Jan 10, 2022

View reviewed changes

MoonlightSentinel requested changes Jan 10, 2022

View reviewed changes

src/dmd/dinterpret.d Outdated Show resolved Hide resolved

MoonlightSentinel requested changes Jan 11, 2022

View reviewed changes

RazvanN7 commented Feb 1, 2022

View reviewed changes

RazvanN7 requested a review from MoonlightSentinel February 1, 2022 13:09

RazvanN7 force-pushed the Issue_22536 branch from edd79db to c68f38c Compare February 1, 2022 14:01

MoonlightSentinel requested changes Feb 4, 2022

View reviewed changes

RazvanN7 added 3 commits February 7, 2022 12:16

Fix Issue 22536 - CTFE: Missing destruction of array literal argument…

073ca86

… for scope slice parameter

Propagate CantExp + add preview=in test

7cbacc9

Fix Issue 22536 - CTFE: Missing destruction of array literal argument…

deb357d

… for scope slice parameter

RazvanN7 force-pushed the Issue_22536 branch from c68f38c to deb357d Compare February 14, 2022 13:09

kinke mentioned this pull request Mar 25, 2022

Lower all non-memcmp-able array equality comparisons to __equals template #13331

Draft

dlang-bot added the Review:stalled label May 5, 2022

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jul 11, 2022

dlang-bot added Merge:needs rebase stalled labels Dec 2, 2024

	ctfeGlobals.dtorsForTemporaries.setDim(0);
	ctfeGlobals.dtorsForTemporaries.setDim(0);
	ctfeGlobals.zero();

		@@ -0,0 +1,69 @@
		// https://issues.dlang.org/show_bug.cgi?id=22536

Uh oh!

Conversation

RazvanN7 commented Jan 6, 2022

Uh oh!

dlang-bot commented Jan 6, 2022

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

Uh oh!

RazvanN7 commented Jan 6, 2022

Uh oh!

MoonlightSentinel commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 10, 2022

Uh oh!

MoonlightSentinel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RazvanN7 commented Jan 11, 2022

Uh oh!

MoonlightSentinel left a comment

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MoonlightSentinel commented Jan 6, 2022 •

edited

Loading