D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 10966 - Optimizer generates wrong code with try-catch
Summary: Optimizer generates wrong code with try-catch
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 blocker
Assignee: Walter Bright
URL:
Keywords: pull, wrong-code
: 15302 (view as issue list)
Depends on:
Blocks:
 
Reported: 2013-09-05 00:26 UTC by monarchdodra
Modified: 2020-08-10 09:06 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description monarchdodra 2013-09-05 00:26:54 UTC
This is a study of what happens in a static array, if a postblit throws. Tested with 2.063 and 2.063.2:

The "caught" issue is that if we are doing "1-element to N-element" postblit, then the "so-far contructed" objects will not be destroyed:

//----
import std.stdio;

int k;

struct S
{
    int a;
    this(this)
    {
        writef("postblitting %s... ", a);
        if (k++ == 2)
        {
            writefln("FAILED!");
            throw new Exception("BOOM!");
        }
        a += 10;
        writefln("OK => %s", a);
    }
    ~this()
    {
        writefln("destroying %s", a);
    }
}

void main()
{
    S s0     = S(0);
    S[4] ss1 = [S(1), S(2), S(3), S(4)];
    S[4] ss2  =
        ss1; //Case N to N
        //s0; //Case 1 to N
}
//----

Version N to N:
//----
ostblitting 1... OK => 11
postblitting 2... OK => 12
postblitting 3... FAILED!
destroying 12
destroying 11
destroying 4
destroying 3
destroying 2
destroying 1
destroying 0
//----
Comment: This behavior is correct: Two items have been constructed, they are both destructed.

Version 1 to N:
//----
ostblitting 1... OK => 11
postblitting 2... OK => 12
postblitting 3... FAILED!
destroying 4
destroying 3
destroying 2
destroying 1
destroying 0
//----
Comment: This behavior is IN-correct: Two items have been constructed (11 and 12), yet neither gets destroyed.

########################

If I may, I think the "root" issue is that the way static array postblit is implemented is wrong ("eg, the call to typeid(S[4]).postblit"). Currently, all it does is "call postblit 1 by 1, until it succeeds or fails". If it fails though, it then relies on the compiler to know which single items in the array have been constructed, and then destroy them individually.

It should be the postblit itself that deconstructs the "so-far-built" items in the array.

For the compiler, there should be 1 and only 1 item: The array. It's state should be binary: Built or not built. Period. A related issue (I'm about to file) is how static array assignment works.
Comment 1 Kenji Hara 2013-09-05 02:07:08 UTC
This is -O switch issue with dmd.
Test following reduced case.

--------
extern(C) int printf(const char*, ...);

int k;

struct S
{
    int a;
    this(this)
    {
        printf("postblitting %d... ", a);
        if (k++ == 2)
        {
            printf("FAILED!\n");
            throw new Exception("BOOM!");
        }
        a += 10;
        printf("OK => %d\n", a);
    }
    ~this()
    {
        printf("destroying %d\n", a);
    }
}

void main()
{
    S s0     = S(0);
    S[4] ss1 = [S(1), S(2), S(3), S(4)];
    S[4] ss2 = s0;  //Case 1 to N
    // call _d_arraysetctor defined in this module
    // instead of the one stored in druntime
}

// copy from druntime/src/rt/arrayassign.d
extern (C) void* _d_arraysetctor(void* p, void* value, int count, TypeInfo ti)
{
    import core.stdc.string : memcpy;

    void* pstart = p;   // with -O, variable pstart is wrongly made an alias of p
    auto element_size = ti.tsize;

    try
    {
        foreach (i; 0 .. count)
        {
            // Copy construction is defined as bit copy followed by postblit.
            memcpy(p, value, element_size);
            ti.postblit(p);
            p += element_size;
        }
    }
    catch (Throwable o)
    {
        // Destroy, in reverse order, what we've constructed so far
        while (p > pstart)
        {
            p -= element_size;
            ti.destroy(p);
        }
        printf("--end\n");

        throw o;
    }
    return pstart;
}
--------

Without -O switch, _d_arraysetctor works as expected.
With -O switch, it doesn't work correctly.

Today druntime is normally compiled with -O switch, therefore this problem occurs always.
Comment 2 Kenji Hara 2013-09-05 20:28:34 UTC
More reduced test case:

extern(C) int printf(const char*, ...);

void main()
{
    int s;
    int[4] ss;
    bug10966(&ss[0], &s, ss.length, int.sizeof);
}

void* bug10966(void* p, void* value, int count, size_t element_size)
{
    // with -O, variable pstart is wrongly made an alias of p
    void* pstart = p;

    try
    {
        printf("+pstart = %p, p = %p\n", pstart, p);
        foreach (i; 0 .. count)
        {
            if (i == 2) throw new Exception("dummy");
            printf("postblit p = %p\n", p);
            p += element_size;
        }
    }
    catch (Throwable o)
    {
        printf("-pstart = %p, p = %p\n", pstart, p);
        assert(p != pstart);    // asserts with -O
        while (p > pstart)
        {
            p -= element_size;
            printf("destroy p = %p\n", p);
        }
    }
    return pstart;
}
Comment 3 yebblies 2014-07-29 15:43:17 UTC
Reduces to:

void bug10966(void* p)
{
    void* pstart = p;

    try
    {
        p = null;
        throw new Exception("dummy");
    }
    catch (Throwable o)
    {
        assert(p != pstart);
    }
}

void test10966()
{
    int s;
    int[4] ss;
    bug10966(ss.ptr);
}

The glue layer turns this into basic blocks, but with not connection between the block containing the throw and the catch block.  The backend assumes that it is not possible for the assert to be run after p has been reassigned, so it propagates the old value of p.  This is very similar to issue 12503

I can easily fix this exact case by adding a relationship between the basic blocks, but replacing the throw with a function that throws will cause the same problem.  Maybe simply adding the dependency for all basic blocks inside the try will do the trick, but it seems like something that should be better supported in the backend.

dmc does the same thing with the equivalent code - it all looks horribly broken.

Walter, is there a mechanism for this in the backend or is it completely unsupported?
Comment 4 yebblies 2016-03-25 07:27:41 UTC
*** Issue 15302 has been marked as a duplicate of this issue. ***
Comment 5 Walter Bright 2020-08-09 09:28:14 UTC
Yes, it looks like a bug with the data flow analysis and catch blocks.
Comment 6 Dlang Bot 2020-08-10 06:31:53 UTC
@WalterBright created dlang/dmd pull request #11541 "fix Issue 10966 - Optimizer generates wrong code with try-catch" fixing this issue:

- fix Issue 10966 - Optimizer generates wrong code with try-catch

https://github.com/dlang/dmd/pull/11541
Comment 7 Dlang Bot 2020-08-10 09:06:18 UTC
dlang/dmd pull request #11541 "fix Issue 10966 - Optimizer generates wrong code with try-catch" was merged into master:

- 921fe0d1065e9b656e7472b9cbb9c6748fbd5ba4 by Walter Bright:
  fix Issue 10966 - Optimizer generates wrong code with try-catch

https://github.com/dlang/dmd/pull/11541