Fix Issue 19185 - [ICE] Nested struct segfaults when using variable from outer scope #8597

RazvanN7 · 2018-08-22T14:31:56Z

The context pointer was not initialized when when the nested struct was constructed implicitly.

dlang-bot · 2018-08-22T14:32:01Z

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 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
✓	19185	normal	[ICE] Nested struct segfaults when using variable from outer scope

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 "master + dmd#8597"

JinShil · 2018-08-22T23:36:38Z

src/dmd/expressionsem.d

+     * Returns:
+     *  The expression initializer
+     */
+    private Expression getInitExp(StructDeclaration sd, Loc loc, Scope* sc, Type t)


I wonder if functions like this should actually be member functions of the class.

It could be made a nested function inside visit(AssignExp)

JinShil · 2018-08-22T23:37:53Z

src/dmd/expressionsem.d

+     *
+     * Returns:
+     *  The expression initializer
+     */


If we want to be strict about the style (https://dlang.org/dstyle.html#phobos_documentation) we should not prepend * characters on each line. But I don't think it should hold up this PR.

The remark about not using leading * in documentation was added to the style guide in January of this year and reflects Phobos but doesn't reflect the style of the existing DMD codebase. IMO the most important thing is maintaining consistency within a document.

jacob-carlborg · 2018-08-23T05:47:10Z

Perhaps because I’m reading this on my phone but I don’t see what actually fixed the issue.

RazvanN7 · 2018-08-23T09:05:43Z

src/dmd/expressionsem.d

+            return new IntegerExp(loc, 0, Type.tint32);
+        }
+
+        if (sd.isNested())


Perhaps because I’m reading this on my phone but I don’t see what actually fixed the issue.

@jacob-carlborg This block of code is executed. Before this patch, it would have not.

n8sh · 2018-08-24T22:06:10Z

src/dmd/expressionsem.d

+     *  sd = the struct for which the expression initializer is needed
+     *  loc = the location of the initializer
+     *  sc = the scope where the expression is located
+     *  t1 = the type of the expression


n8sh · 2018-08-24T22:31:31Z

src/dmd/expressionsem.d

-                        Expression einit;
-                        einit = new BlitExp(exp.loc, e1x, e1x.type.defaultInit(exp.loc));
+                        Expression einit = getInitExp(sd, exp.loc, sc, t1);
+                        einit = new BlitExp(exp.loc, e1x, einit);


Note to other reviewers: it's fine that this PR replaces e1x.type with t1 because here e1x is exp.e1 and t1 is exp.e1.type.toBasetype().

Note that einit is assigned twice. A better method is to assign it only once.

JinShil · 2018-08-27T12:07:01Z

src/dmd/expressionsem.d

+     *  t = the type of the expression
+     *
+     * Returns:
+     *  The expression initializer


I think this should mention that it might return null and why.

Or, better yet, shouldn't this return an ErrorExp and then check for TOK.error in the caller?

JinShil · 2018-08-27T12:24:45Z

src/dmd/expressionsem.d

@@ -6833,8 +6851,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                            }
                        }

-                        Expression einit;
-                        einit = new BlitExp(exp.loc, e1x, e1x.type.defaultInit(exp.loc));
+                        Expression einit = new BlitExp(exp.loc, e1x, getInitExp(sd, exp.loc, sc, t1));


getInitExp might return null, but there's no check for null in the BlitExp class hierarchy. It seems like a segfault waiting to happen. Should there be an if (!einit) return setError(); here also?

Better yet, getInitExp should just return an ErrorExp and thus turn any use of such expressions into an error as well.

MartinNowak

I agree with @JinShil here, getInitExp should return an ErrorExp instead of null.

RazvanN7 · 2018-09-03T10:24:06Z

@MartinNowak is this better?

JinShil · 2018-09-04T01:13:35Z

src/dmd/expressionsem.d

-                                return setError();
+                        Expression einit = getInitExp(sd, exp.loc, sc, t1);
+                        if (einit.op == TOK.error)
+                            return setError();


This is redundant. getInitExp returned an ErrorExp and then setError immediately creates a new ErrorExp and assigns it to result. I see few options:

create an overload of setError to take an ErrorExp

instead of return setError() do result = einit; return;

Maybe there is another solution, but those are the two that come to mind for me.

Required changes were implemented

…rom outer scope

jacob-carlborg · 2018-09-06T09:20:18Z

160 commits!! This does't look correct 😃.

RazvanN7 · 2018-09-06T09:21:27Z

I wanted to fix the semaphore issue by rebasing master, but I forgot I was targeting stable :(. I'm just gonna target master for now.

dlang-bot added the Severity:Bug Fix label Aug 22, 2018

RazvanN7 closed this Aug 22, 2018

RazvanN7 reopened this Aug 22, 2018

JinShil reviewed Aug 22, 2018

View reviewed changes

RazvanN7 commented Aug 23, 2018

View reviewed changes

n8sh reviewed Aug 24, 2018

View reviewed changes

n8sh approved these changes Aug 24, 2018

View reviewed changes

WalterBright approved these changes Aug 25, 2018

View reviewed changes

RazvanN7 force-pushed the Issue_19185 branch from 93e6551 to da54cd0 Compare August 27, 2018 11:28

JinShil reviewed Aug 27, 2018

View reviewed changes

MartinNowak previously requested changes Aug 29, 2018

View reviewed changes

RazvanN7 force-pushed the Issue_19185 branch from da54cd0 to 512c2b1 Compare September 3, 2018 10:23

JinShil reviewed Sep 4, 2018

View reviewed changes

RazvanN7 force-pushed the Issue_19185 branch from 512c2b1 to 609db94 Compare September 5, 2018 08:34

JinShil approved these changes Sep 5, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Sep 5, 2018

PetarKirov closed this Sep 6, 2018

PetarKirov reopened this Sep 6, 2018

Fix Issue 19185 - [ICE] Nested struct segfaults when using variable f…

9b5a295

…rom outer scope

RazvanN7 force-pushed the Issue_19185 branch from 609db94 to 9b5a295 Compare September 6, 2018 09:17

RazvanN7 requested review from andralex, Geod24 and ibuclaw as code owners September 6, 2018 09:17

RazvanN7 requested review from jacob-carlborg, dnadlinger, rainers and wilzbach as code owners September 6, 2018 09:17

dlang-bot removed the Merge:auto-merge label Sep 6, 2018

RazvanN7 changed the base branch from stable to master September 6, 2018 09:19

RazvanN7 added the Merge:auto-merge label Sep 6, 2018

dlang-bot merged commit 88de313 into dlang:master Sep 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 19185 - [ICE] Nested struct segfaults when using variable from outer scope #8597

Fix Issue 19185 - [ICE] Nested struct segfaults when using variable from outer scope #8597

RazvanN7 commented Aug 22, 2018

dlang-bot commented Aug 22, 2018 •

edited

Loading

JinShil Aug 22, 2018

RazvanN7 Aug 23, 2018

JinShil Aug 22, 2018

n8sh Aug 24, 2018

jacob-carlborg commented Aug 23, 2018

RazvanN7 Aug 23, 2018 •

edited

Loading

n8sh Aug 24, 2018

n8sh Aug 24, 2018

WalterBright Aug 25, 2018

RazvanN7 Aug 27, 2018

JinShil Aug 27, 2018

JinShil Aug 27, 2018

JinShil Aug 27, 2018

MartinNowak Aug 29, 2018

MartinNowak left a comment

RazvanN7 commented Sep 3, 2018

JinShil Sep 4, 2018 •

edited

Loading

RazvanN7 Sep 5, 2018

jacob-carlborg commented Sep 6, 2018

RazvanN7 commented Sep 6, 2018

Fix Issue 19185 - [ICE] Nested struct segfaults when using variable from outer scope #8597

Fix Issue 19185 - [ICE] Nested struct segfaults when using variable from outer scope #8597

Conversation

RazvanN7 commented Aug 22, 2018

dlang-bot commented Aug 22, 2018 • edited Loading

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg commented Aug 23, 2018

RazvanN7 Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak left a comment

Choose a reason for hiding this comment

RazvanN7 commented Sep 3, 2018

JinShil Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg commented Sep 6, 2018

RazvanN7 commented Sep 6, 2018

dlang-bot commented Aug 22, 2018 •

edited

Loading

RazvanN7 Aug 23, 2018 •

edited

Loading

JinShil Sep 4, 2018 •

edited

Loading