fix issue 19497 - the program crash using dmd with -O, it works fine … #9113

aG0aep6G · 2018-12-21T12:22:40Z

…without optimizations.

dlang-bot · 2018-12-21T12:22:42Z

Thanks for your pull request and interest in making D better, @aG0aep6G! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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
✓	19497	critical	the program crash using dmd with -O, it works fine without optimizations.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "stable + dmd#9113"

thewilsonator

Looks fine to me, but I'll wait for someone who understands what that code is actually doing. cc @WalterBright

rainers · 2018-12-22T08:54:47Z

src/dmd/backend/cod4.d

@@ -443,7 +443,7 @@ void cdeq(ref CodeBuilder cdb,elem *e,regm_t *pretregs)
                postinc = e11.EV.E2.EV.Vint;
                if (e11.Eoper == OPpostdec)
                    postinc = -postinc;
-                getlvalue(cdb,&cs,e11,RMstore);
+                getlvalue(cdb,&cs,e1,RMstore);


dmd/src/dmd/backend/cod4.d

Line 713 in 2ede66c

getlvalue(cdb,&cs,e11,RMstore | retregs);

and
https://github.com/dlang/dmd/blob/master/src/dmd/backend/cgxmm.d#L257
have more or less the same code, should they be changed, too?

The first one seems to have a workaround following it:

dmd/src/dmd/backend/cod4.d

Lines 715 to 716 in 5cb179f

if (I64 && sz < 8)

cs.Irex &= ~REX_W; // incorrectly set by getlvalue()

Blame leads to this commit: 521e68c. Unfortunately, there's hardly a hint as to what issue was fixed there. It looks like removing the workaround, and fixing the getlvalue call instead, could work. But maybe we should first find/add a test that breaks without the workaround.

Regarding the one in cgxmm.d, I got nothing.

The first one seems to have a workaround following it:

This code triggers the workaround (Win64):

// REQUIRES: -O int toStringz(string s) { return s.length > 0 ? s[0] : 0; } private void toAStringz(in string[] a, int*az) { foreach (string s; a) { *az++ = toStringz(s); } } void main() { string[1] sa = [ "abc" ]; int[2] tgt = 0x2a; toAStringz(sa[], tgt.ptr); assert(tgt[0] == 'a'); assert(tgt[1] == 0x2a); }

It also passes if e11 is replaced by e1 instead of the workaround.

Regarding the one in cgxmm.d, I got nothing.

It is hit by the same code when using float, double or __vector(T) instead of int, but doesn't seem to be affected by the issue.

I've added a commit that removes the workaround and adds your test.

gtoutoungis · 2018-12-24T20:53:48Z

Hi, I am George who submitted this bug.
Thanks for the fix, waiting for the new version of dmd :)
And Thanks to maintain and improve such a wonderful programming language.

looking at the test case, just I wonder how it works. maybe I miss something!!

void test19497() // https://issues.dlang.org/show_bug.cgi?id=19497
{
    alias Seq(stuff ...) = stuff;
    static foreach (T; Seq!(ubyte, ushort, uint, ulong, byte, short, int, long))
    {{
        T[2] data = 0x2A;
        T* q = &data[0];
        *q++ = cast(T) 0x1122334455667788;
        if (*q != 0x2A) assert(false);
    }}
}

as q is a pointer to data[0], and we initialize the data[2] with 0x2A.
we put a value in data[0] using the pointer q, and incrementing q using q++.
then q should point to data[1] and not data[2].
so how we test *q to content of data[2] which is 0x2A !!

Best Regards,
Georges Toutoungis

aG0aep6G · 2018-12-24T23:12:18Z

as q is a pointer to data[0], and we initialize the data[2] with 0x2A.

We initialize both data[0] and data[1] to 0x2A. There is no data[2].

we put a value in data[0] using the pointer q, and incrementing q using q++.
then q should point to data[1] and not data[2].

That's right. After the increment, q points to data[1].

so how we test *q to content of data[2] which is 0x2A !!

There is no data[2]. I'm asserting that data[1] still contains 0x2A. With the bug, the assignment to data[0]also wrote over data[1] when T = int.

gtoutoungis · 2018-12-26T15:24:10Z

Thanks, I didn’t pay attention to this line. T[2] data = 0x2A; it is initialization to the entire array :) in my mind, I thought it was data[2] = 0x2A this is classic with me, as I am most of the time absent minded :) Thanks a lot, Wish a Merry Christmas and a Happy new year to you and to the entire D community, Georges Toutoungis

…

On Dec 24, 2018, at 6:12 PM, aG0aep6G ***@***.***> wrote: as q is a pointer to data[0], and we initialize the data[2] with 0x2A. We initialize both data[0] and data[1] to 0x2A. There is no data[2]. we put a value in data[0] using the pointer q, and incrementing q using q++. then q should point to data[1] and not data[2]. That's right. After the increment, q points to data[1]. so how we test *q to content of data[2] which is 0x2A !! There is no data[2]. I'm asserting that data[1] still contains 0x2A. With the bug, the assignment to data[0]also wrote over data[1] when T = int. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9113 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE0pt4DwgKGC1nuQSXB-m1JOsXITxdsJks5u8V9VgaJpZM4Zd5xs>.

thewilsonator · 2018-12-26T23:49:17Z

Is this good to go now?

WalterBright · 2018-12-27T04:08:47Z

The test suite for DMC++ has much more extensive code generator tests. Let me run that first.

Also, now that the backend is in D, why aren't we getting the test coverage information for the back end?

WalterBright · 2018-12-27T05:59:52Z

This change is crashing DMC++ when running its test suite.

rainers · 2018-12-27T07:36:43Z

This change is crashing DMC++ when running its test suite.

Too bad. Maybe the workaround from line 714 should be copied to the original location at line 446.

WalterBright · 2018-12-27T07:39:10Z

Here's the test case that fails when this PR is added:
#9146

rainers · 2018-12-27T09:46:45Z

Maybe the workaround from line 714 should be copied to the original location at line 446.

That seems to work, too. But let's see #9146 fail the testers with the current version...

rainers · 2018-12-27T16:24:57Z

But let's see #9146 fail the testers with the current version...

ccompile.d doesn't fail on the auto-tester. @WalterBright Did it do that for you with the translated version or only for the C version?

WalterBright · 2018-12-28T00:22:11Z

It did it for the translated version. At the moment, I cannot explain why it is failing on my system and not the autotester.

WalterBright · 2018-12-29T08:23:43Z

The problems I was seeing with this were my own construction.

fix issue 19497 - the program crash using dmd with -O, it works fine …

2ede66c

…without optimizations.

dlang-bot added the Bug Fix label Dec 21, 2018

thewilsonator reviewed Dec 21, 2018

View reviewed changes

rainers reviewed Dec 22, 2018

View reviewed changes

fix another getlvalue call, remove workaround

9d31e0d

WalterBright mentioned this pull request Dec 27, 2018

add optimizer test from DMC++ #9146

Merged

WalterBright merged commit 973c000 into dlang:stable Dec 29, 2018

aG0aep6G deleted the 19497 branch December 31, 2018 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue 19497 - the program crash using dmd with -O, it works fine … #9113

fix issue 19497 - the program crash using dmd with -O, it works fine … #9113

aG0aep6G commented Dec 21, 2018

dlang-bot commented Dec 21, 2018

thewilsonator left a comment

rainers Dec 22, 2018

aG0aep6G Dec 22, 2018

rainers Dec 25, 2018

rainers Dec 25, 2018

aG0aep6G Dec 25, 2018

gtoutoungis commented Dec 24, 2018

aG0aep6G commented Dec 24, 2018

gtoutoungis commented Dec 26, 2018 via email

thewilsonator commented Dec 26, 2018

WalterBright commented Dec 27, 2018

WalterBright commented Dec 27, 2018

rainers commented Dec 27, 2018

WalterBright commented Dec 27, 2018

rainers commented Dec 27, 2018

rainers commented Dec 27, 2018

WalterBright commented Dec 28, 2018

WalterBright commented Dec 29, 2018

	if (I64 && sz < 8)
	cs.Irex &= ~REX_W; // incorrectly set by getlvalue()

fix issue 19497 - the program crash using dmd with -O, it works fine … #9113

fix issue 19497 - the program crash using dmd with -O, it works fine … #9113

Conversation

aG0aep6G commented Dec 21, 2018

dlang-bot commented Dec 21, 2018

Bugzilla references

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

rainers Dec 22, 2018

Choose a reason for hiding this comment

aG0aep6G Dec 22, 2018

Choose a reason for hiding this comment

rainers Dec 25, 2018

Choose a reason for hiding this comment

rainers Dec 25, 2018

Choose a reason for hiding this comment

aG0aep6G Dec 25, 2018

Choose a reason for hiding this comment

gtoutoungis commented Dec 24, 2018

aG0aep6G commented Dec 24, 2018

gtoutoungis commented Dec 26, 2018 via email

thewilsonator commented Dec 26, 2018

WalterBright commented Dec 27, 2018

WalterBright commented Dec 27, 2018

rainers commented Dec 27, 2018

WalterBright commented Dec 27, 2018

rainers commented Dec 27, 2018

rainers commented Dec 27, 2018

WalterBright commented Dec 28, 2018

WalterBright commented Dec 29, 2018