Translate `_d_newThrowable` hook to template #13494

teodutu · 2022-01-05T18:37:02Z

This PR lowers expressions such as throw new E(args) to:

throw _d_newThrowable!E(args);

when compiling with -dip1008.

The DRuntime PRs which implement the template _d_newThrowable are:

dlang-bot · 2022-01-05T18:37:04Z

Thanks for your pull request and interest in making D better, @teodutu! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#13494"

thewilsonator · 2022-01-20T03:36:51Z

dependant PR merged, please rebase this.

thewilsonator

is this able to be tested in this PR?

teodutu · 2022-01-20T11:42:03Z

@thewilsonator Yes. This lowering is enabled by -dip1008. The following tests use this parameter:

test/compilable/test20318.d
test/fail_compilation/test20023.d
test/runnable/test19317.d
test/compilable/testdip1008.d

thewilsonator · 2022-01-20T11:48:06Z

There are a few build kite failures, could you take a look. cc @atilaneves this fails on your projects

atilaneves · 2022-01-24T14:04:47Z

There are a few build kite failures, could you take a look. cc @atilaneves this fails on your projects

Yes - but I don't know why it'd fall on me to go and see why. I downloaded the branch from the PR anyway and tried it on unit-threaded, and it seems this PR ends up changing the purity of std.format.spec.FormatSpec!char.FormatSpec.toString!(Appender!string).toString to impure.

The buildkite test projects are in CI exactly to try and catch this type of breakage.

I'm more than happy to help and answer questions because of familiarity (or lack thereof) with the codebase.

RazvanN7 · 2022-02-21T08:56:26Z

Rebase this and let's get it in.

teodutu · 2022-02-21T11:12:21Z

The buildkite test failed because the hook was impure. This was not verified in the tests, so I added a function to check qualifiers in test/compilable/testdip1008.d.

src/dmd/dinterpret.d

RazvanN7 · 2022-02-21T11:19:41Z

src/dmd/dinterpret.d

+                auto se = ctfeEmplaceExp!StructLiteralExp(e.loc, cast(StructDeclaration) cd, elems, e.type);
+                se.origin = se;
+                se.ownedByCtfe = OwnedBy.ctfe;
+                Expression eref = ctfeEmplaceExp!ClassReferenceExp(e.loc, se, e.type);


Do you mind explaining why is this necessary?

eref needs to be a reference to this of the constructed class, in order to call interpretFunction below. This is why it needs to be a ClassReferenceExp. The ctor of ClassReferenceExp receives a StructLiteralExp [1]. Hence, the need for the "hack".

[1]

dmd/src/dmd/ctfeexpr.d

Lines 43 to 53 in bc3a0f1

extern (C++) final class ClassReferenceExp : Expression

{

StructLiteralExp value;

extern (D) this(const ref Loc loc, StructLiteralExp lit, Type type)

{

super(loc, EXP.classReference, __traits(classInstanceSize, ClassReferenceExp));

assert(lit && lit.sd && lit.sd.isClassDeclaration());

this.value = lit;

this.type = type;

}

MoonlightSentinel · 2022-02-21T11:30:09Z

src/dmd/dinterpret.d

+            else if (fd.ident == Id._d_newThrowable)
+            {
+                // Treat `_d_newThrowable!E(args)` as `new E(args)` and interpret `E.ctor(args)`.
+                auto tc = e.type.toBasetype().isTypeClass();
+                auto cd = tc.sym;
+
+                if (!cd.ctor)
+                {
+                    e.error("`%s` cannot be constructed at compile time, because the constructor has no available source code", e.type.toChars());
+                    result = CTFEExp.cantexp;
+                    return;
+                }
+
+                size_t totalFieldCount = 0;
+                for (ClassDeclaration c = cd; c; c = c.baseClass)
+                    totalFieldCount += c.fields.dim;
+                auto elems = new Expressions(totalFieldCount);
+                size_t fieldsSoFar = totalFieldCount;
+                for (ClassDeclaration c = cd; c; c = c.baseClass)
+                {
+                    fieldsSoFar -= c.fields.dim;
+                    foreach (i, v; c.fields)
+                    {
+                        if (v.inuse)
+                        {
+                            e.error("circular reference to `%s`", v.toPrettyChars());
+                            result = CTFEExp.cantexp;
+                            return;
+                        }
+                        Expression m;
+                        if (v._init)
+                        {
+                            if (v._init.isVoidInitializer())
+                                m = voidInitLiteral(v.type, v).copy();
+                            else
+                                m = v.getConstInitializer(true);
+                        }
+                        else
+                            m = v.type.defaultInitLiteral(e.loc);
+                        if (exceptionOrCant(m))
+                            return;
+                        (*elems)[fieldsSoFar + i] = copyLiteral(m).copy();
+                    }
+                }
+
+                // Hack: store a ClassDeclaration instead of a StructDeclaration.
+                auto se = ctfeEmplaceExp!StructLiteralExp(e.loc, cast(StructDeclaration) cd, elems, e.type);
+                se.origin = se;
+                se.ownedByCtfe = OwnedBy.ctfe;
+                Expression eref = ctfeEmplaceExp!ClassReferenceExp(e.loc, se, e.type);
+
+                UnionExp ue = void;
+                auto err = interpretFunction(&ue, cast(FuncDeclaration) cd.ctor, istate, e.arguments, eref);
+                if (exceptionOrCant(err))
+                    return;
+
+                eref.loc = e.loc;
+                result = eref;
+                return;
+            }


This block seems to be copied from the class branch of NewExp - refactor to that code into a function instead of duplicating it here.

Wouldn't it be shorter and more expressive, albeit a bit less efficient, if I just created a NewExp and interpreted it directly?

MoonlightSentinel · 2022-02-21T11:31:23Z

src/dmd/e2ir.d

@@ -1096,8 +1096,11 @@ extern (C++) class ToElemVisitor : Visitor
            }
            else
            {
+                assert(!(global.params.ehnogc && ne.thrownew),
+                    "This should have been rewritten to `_d_newThrowable` in the semantic phase.");


This assert will trigger for COM / C++ classes which are not rewritten in expressionsem.d?

You're right. I changed this assert to the same condition as in expressionsem.d:

dmd/src/dmd/expressionsem.d

Lines 3727 to 3728 in bf6d206

if (global.params.ehnogc && exp.thrownew &&

!cd.isCOMclass() && !cd.isCPPclass())

It doesn't seem that the !isCOMclass() && !cd.isCPPclass() condition did not make it.

RazvanN7 · 2022-02-22T08:53:09Z

It seems that this is still failing a couple of projects on buildkite

kinke · 2022-02-26T16:30:32Z

Looks like the undefined symbol errors are caused by the same underlying issue as using -checkaction=context. E.g., failing unit-threaded uses -preview=dip1008 for its unittests: https://github.com/atilaneves/unit-threaded/blob/e9e612b5126b3c99c8239f2e09b65c9303e53d94/dub.json#L51

The compiler assumes druntime and Phobos are compiled with the same switch, and so most likely skips codegen for some templates encountered in druntime/Phobos imports. So once again a template culling problem when it comes to linking libraries compiled with different switches. Edit: And which wouldn't be an issue if -allinst fully worked.

RazvanN7 · 2022-02-28T10:12:14Z

@kinke : @teodutu compiled druntime locally with dip1008 enabled and then compiled unit-threaded with this version however the linker error persisted. One thing we could try is to compile both druntime and phobos with dip1008 and see if that works, however, phobos uses exceptions a lot and underlying dip1008 issues might surface (I remember looking at some dip1008 issue a few years ago and my thought back then was that it works for simple cases, but there are more complex uses where it's gonna fail; IIRC it was something regarding scope exceptions that could/should have escaped their scope). But if that doesn't work, what else can we do?

kinke · 2022-02-28T11:19:01Z

I don't know all the DIP1008 details, but to me it seemed to be just a way to use exceptions with -betterC, without GC and without druntime/Phobos. So compiling druntime/Phobos with DIP1008 doesn't seem to make much sense to me (not linked with -betterC anyway).

Besides fixing -allinst, another option might be to use pragma(inline, true) for _d_newThrowable. Not sure how that is implemented by DMD, but with LDC, that should work in the sense of truly inlining it for each all call, in each compiled object file.

teodutu · 2022-02-28T11:43:44Z

@kinke Using pragma(inline, true) didn't solve the linking error. I guess the symbol that causes it stays the same regardless of inlining.

thewilsonator · 2022-05-23T09:46:16Z

Please rebase.

RazvanN7

Don't forget to delete the old hook in druntime!

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

This simplifies the lowering of `throw new E(args)` to `throw _d_newThrowable!E(args)`. During ctfe, the hook is interpreted like `new E(args)`. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

The new `_d_newThrowable` hook no longer calls `Error`'s ctor. Thus, the compiler must do this instead. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

addressed

It was replaced by a template hook: - dlang#3661 - dlang/dmd#13494 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

nordlow · 2022-06-08T21:01:37Z

How does this PR improve a throw new E(args)?

teodutu · 2022-06-09T13:32:02Z

@nordlow It's pretty transparent from the perspective of the developer. The PR makes the code in the compiler easier to maintain and contributes to dlang/project-ideas#25. The issue only mentions arrays, but there are other cases where TypeInfo is used, like this one.

It was replaced by a template hook: - dlang/druntime#3661 - dlang/dmd#13494 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Pr dlang#14837 added templated lowering for _d_newclass and lowering field to NewExp. Pr dlang#13494 added templated lowering for _d_newThroable. This Pr merges those two. In expressionsem.d, instead of giving up the expression node and keeping just the id.expressionSemantic of it, the result is the whole node with all its previous fields and a new one, lowering, that is set to the previously returned value. In e2ir.d, for a newExpression, the expression is already created in the semantic phase with all the arguments needed, so the visitor will not recreate it, therefore the assert at line 1153 is no longer necessary. Signed-off-by: Andrei Rusanescu <andreirusanescu154@gmail.com>

teodutu requested a review from ibuclaw as a code owner January 5, 2022 18:37

teodutu changed the title ~~Template _d_newThrowable hook to template~~ Translate _d_newThrowable hook to template Jan 5, 2022

RazvanN7 added the Merge:Blocked label Jan 6, 2022

teodutu mentioned this pull request Jan 6, 2022

Implement templated version of _d_newThrowable dlang/druntime#3661

Merged

RazvanN7 removed the Merge:Blocked label Jan 13, 2022

teodutu mentioned this pull request Jan 19, 2022

Make _d_newThrowable return a fully constructed Throwable dlang/druntime#3692

Merged

teodutu force-pushed the template-_d_newThrowable-hook branch 2 times, most recently from 59ef55a to 9e470ab Compare January 20, 2022 11:01

thewilsonator reviewed Jan 20, 2022

View reviewed changes

dlang-bot added Review:Needs Rebase Review:Needs Work labels Feb 17, 2022

teodutu mentioned this pull request Feb 20, 2022

Allow _d_newThrowable to be pure dlang/druntime#3750

Merged

teodutu force-pushed the template-_d_newThrowable-hook branch from 9e470ab to 4d532f3 Compare February 21, 2022 11:10

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Feb 21, 2022

RazvanN7 reviewed Feb 21, 2022

View reviewed changes

MoonlightSentinel previously requested changes Feb 21, 2022

View reviewed changes

teodutu mentioned this pull request Feb 24, 2022

Pass args to ctor without forward in _d_newThrowable dlang/druntime#3761

Closed

dlang-bot added Review:Needs Rebase Review:Needs Work labels May 9, 2022

teodutu mentioned this pull request May 23, 2022

Do not call throwable's ctor from _d_newThrowable dlang/druntime#3823

Merged

teodutu force-pushed the template-_d_newThrowable-hook branch from 4d532f3 to 2770a0a Compare May 23, 2022 10:02

dlang-bot removed Review:Needs Work Review:Needs Rebase labels May 23, 2022

teodutu force-pushed the template-_d_newThrowable-hook branch from 2770a0a to dc3273c Compare June 3, 2022 16:40

teodutu requested review from thewilsonator, MoonlightSentinel and RazvanN7 June 3, 2022 17:07

RazvanN7 approved these changes Jun 5, 2022

View reviewed changes

teodutu added 6 commits June 7, 2022 22:55

Translate _d_newThrowable to template

69af92d

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

dinterpret.d: Forbid evaluating _d_newThrowable at CTFE

d07f1f2

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

e2ir.d: Only evaluate the false branch in __ctfe ? ...

0a196c1

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Lower throw new E() to improved hook

7917496

This simplifies the lowering of `throw new E(args)` to `throw _d_newThrowable!E(args)`. During ctfe, the hook is interpreted like `new E(args)`. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

testdip1008.d: Verify @safe pure @nogc qualifiers

2e0302d

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Lower throw new Error() to new runtime hook

4bac161

The new `_d_newThrowable` hook no longer calls `Error`'s ctor. Thus, the compiler must do this instead. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

teodutu force-pushed the template-_d_newThrowable-hook branch from dc3273c to 4bac161 Compare June 7, 2022 20:23

RazvanN7 merged commit 9767806 into dlang:master Jun 8, 2022

teodutu added a commit to teodutu/druntime that referenced this pull request Jun 8, 2022

rt.ehalloc.d: Remove old _d_newThrowable hook

7b7a2ec

It was replaced by a template hook: - dlang#3661 - dlang/dmd#13494 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

teodutu mentioned this pull request Jun 8, 2022

Remove old _d_newThrowable hook dlang/druntime#3838

Merged

teodutu deleted the template-_d_newThrowable-hook branch June 15, 2022 21:06

kinke pushed a commit to ldc-developers/ldc that referenced this pull request Nov 25, 2022

rt.ehalloc.d: Remove old _d_newThrowable hook

de1d5e9

It was replaced by a template hook: - dlang/druntime#3661 - dlang/dmd#13494 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

andreirusanescu mentioned this pull request Nov 23, 2024

Moved _d_newThrowable in the semantic phase to lowering #17084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate `_d_newThrowable` hook to template #13494

Translate `_d_newThrowable` hook to template #13494

teodutu commented Jan 5, 2022 •

edited

Loading

dlang-bot commented Jan 5, 2022

thewilsonator commented Jan 20, 2022

thewilsonator left a comment

teodutu commented Jan 20, 2022

thewilsonator commented Jan 20, 2022

atilaneves commented Jan 24, 2022

RazvanN7 commented Feb 21, 2022

teodutu commented Feb 21, 2022

RazvanN7 Feb 21, 2022

teodutu Feb 21, 2022 •

edited

Loading

MoonlightSentinel Feb 21, 2022

teodutu Feb 21, 2022 •

edited

Loading

MoonlightSentinel Feb 21, 2022

teodutu Feb 21, 2022 •

edited

Loading

RazvanN7 Jun 5, 2022

RazvanN7 commented Feb 22, 2022

kinke commented Feb 26, 2022 •

edited

Loading

RazvanN7 commented Feb 28, 2022 •

edited

Loading

kinke commented Feb 28, 2022

teodutu commented Feb 28, 2022

thewilsonator commented May 23, 2022

RazvanN7 left a comment

nordlow commented Jun 8, 2022 •

edited

Loading

teodutu commented Jun 9, 2022

	extern (C++) final class ClassReferenceExp : Expression
	{
	StructLiteralExp value;

	extern (D) this(const ref Loc loc, StructLiteralExp lit, Type type)
	{
	super(loc, EXP.classReference, __traits(classInstanceSize, ClassReferenceExp));
	assert(lit && lit.sd && lit.sd.isClassDeclaration());
	this.value = lit;
	this.type = type;
	}

	if (global.params.ehnogc && exp.thrownew &&
	!cd.isCOMclass() && !cd.isCPPclass())

Translate _d_newThrowable hook to template #13494

Translate _d_newThrowable hook to template #13494

Conversation

teodutu commented Jan 5, 2022 • edited Loading

dlang-bot commented Jan 5, 2022

Bugzilla references

Testing this PR locally

thewilsonator commented Jan 20, 2022

thewilsonator left a comment

Choose a reason for hiding this comment

teodutu commented Jan 20, 2022

thewilsonator commented Jan 20, 2022

atilaneves commented Jan 24, 2022

RazvanN7 commented Feb 21, 2022

teodutu commented Feb 21, 2022

RazvanN7 Feb 21, 2022

Choose a reason for hiding this comment

teodutu Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

MoonlightSentinel Feb 21, 2022

Choose a reason for hiding this comment

teodutu Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

MoonlightSentinel Feb 21, 2022

Choose a reason for hiding this comment

teodutu Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

RazvanN7 Jun 5, 2022

Choose a reason for hiding this comment

RazvanN7 commented Feb 22, 2022

kinke commented Feb 26, 2022 • edited Loading

RazvanN7 commented Feb 28, 2022 • edited Loading

kinke commented Feb 28, 2022

teodutu commented Feb 28, 2022

thewilsonator commented May 23, 2022

RazvanN7 left a comment

Choose a reason for hiding this comment

nordlow commented Jun 8, 2022 • edited Loading

teodutu commented Jun 9, 2022

Translate `_d_newThrowable` hook to template #13494

Translate `_d_newThrowable` hook to template #13494

teodutu commented Jan 5, 2022 •

edited

Loading

teodutu Feb 21, 2022 •

edited

Loading

teodutu Feb 21, 2022 •

edited

Loading

teodutu Feb 21, 2022 •

edited

Loading

kinke commented Feb 26, 2022 •

edited

Loading

RazvanN7 commented Feb 28, 2022 •

edited

Loading

nordlow commented Jun 8, 2022 •

edited

Loading