Translate `_d_newclass` to a template by teodutu · Pull Request #14837 · dlang/dmd

teodutu · 2023-01-18T18:22:22Z

This PR makes the following changes:

Add template _d_newclassT to druntime.src.core.lifetime.d
Replace lowering of new C() to _d_newclassT!C()
Add lowering member to NewExp. This field stores the above lowering to be used by e2ir.d
Keep the old _d_newclass hook because it's used by TypeInfo_Class.create()
Add dummy _d_newclassT hook to tests that redefine the object module
Remove new MinHeap!(TestType)() from fail308.d. Otherwise the errror was changed and printed the local path to druntime
Account for the GC.malloc() called by the template hook in the -profile=gc tests

dlang-bot · 2023-01-18T18:22:25Z

Thanks for your pull request, @teodutu!

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

compiler/src/dmd/expression.d

teodutu · 2023-01-18T18:34:33Z

fail_compilation/shared.d fails because of https://issues.dlang.org/show_bug.cgi?id=23639. The bug is currently being fixed by #14836.

maxhaton · 2023-01-18T20:45:47Z

compiler/src/dmd/expression.d

    bool onstack;               // allocate on stack
    bool thrownew;              // this NewExp is the expression of a ThrowStatement

+    Expression lowering;        // lowered druntime hook: `_d_newclass`


Is this necessary? If yes keep it but keep an eye on adding unnecessary fields as dmd's classes are absolutely enormous

I think it's necessary. I need an expression where to store the lowering, so I can then pass it to the backend in e2ir.d. This expression can either be a field in NewExp or the result set by visit(NewExp) (instead of the original expression) in expressionsemantic.d. The latter option is difficult because it requires me to initialise the context pointer in the semantic phase. I tried doing this in the past and it's ugly. I'd rather use the existing machinery that handles context pointers in e2ir.d instead. Furthermore, I received feedback from @ibuclaw in the past that eliding the original expressions might hinder some optimisations in GDC and LDC.

RazvanN7

Why aren't we deleting the old hook?

teodutu · 2023-01-20T13:09:02Z

Why aren't we deleting the old hook?

Because it's used by TypeInfo_Class.create(), which is publicly exported.

RazvanN7 · 2023-01-20T13:10:20Z

Can't we just call the template?

teodutu · 2023-01-20T13:15:43Z

Can't we just call the template?

Maybe, but for this, we would need the actual type to be available at compile time. We can obtain it from TypeInfo_Class.name, but only at run time. LE: one can create a TypeInfo_Class object manually, in which case the type embedded within it is totally inaccessible at compile time.

RazvanN7 · 2023-01-20T16:59:49Z

@ibuclaw @maxhaton any objections against this addition?

ibuclaw · 2023-01-21T09:24:14Z

compiler/src/dmd/e2ir.d

-                const rtl = RTLSYM.NEWCLASS;
-                ex = el_bin(OPcall,TYnptr,el_var(getRtlsym(rtl)),el_ptr(csym));
-                toTraceGC(irs, ex, ne.loc);
+                ex = toElem(ne.lowering, irs);


Are previous lowerings going to be retroactively transplanted into a lowering field?

Yes, that's the plan.

druntime/.gdb_history

druntime/src/rt/sections.d

Points were addressed

RazvanN7 · 2023-01-23T08:40:08Z

@thewilsonator why have you removed the 72h no objections->merge label. Is there anything that you are requesting?

thewilsonator · 2023-01-23T08:41:49Z

#14837 (review)

RazvanN7 · 2023-01-23T08:42:41Z

It has been addressed.

RazvanN7 · 2023-01-23T08:43:13Z

@ibuclaw is this ready to go?

Addressed

RazvanN7 · 2023-01-24T10:49:44Z

@thewilsonator @ibuclaw re-adding the 72h no objections -> merge label

druntime/src/core/lifetime.d

This makes the following changes: - Add template `_d_newclassT` to `druntime.src.core.lifetime.d` - Replace lowering of `new C()` to `_d_newclassT!C()` - Add `lowering` member to `NewExp`. This field stores the above lowering to be used by e2ir.d - Keep the old `_d_newclass` hook because it's used by `TypeInfo_Class.create()` - Add dummy `_d_newclassT` hook to tests that redefine the `object` module - Remove `new MinHeap!(TestType)()` from `fail308.d`. Otherwise the errror was changed and printed the local path to druntime - Move `err` to global scope in rt.sections.d to avoid the frontend lowering - Account for the `GC.malloc()` called by the template hook in the `-profile=gc` tests Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

jsatellite · 2023-02-01T22:55:52Z

This changes how COM classes are managed - the memory will now be allocated by the GC whereas currently _d_newclass uses malloc. Looks like the getLinkage trait replaces the test for ci.m_flags & TypeInfo_Class.ClassFlags.isCOMClass - but __traits(getLinkage, ...) doesn't return "Windows" even if the class is declared with extern(Windows) (is it supposed to?). COM classes should either use malloc, or GC memory and a root added.

rainers · 2023-02-02T18:51:44Z

This changes how COM classes are managed

Good catch. The linkage reported for a class is derived from its member classKind which is C++ for COM classes.

COM classes should either use malloc, or GC memory and a root added.

IMO GC memory is the right choice, but adding/removing roots should be done in AddRef/Release. See https://issues.dlang.org/show_bug.cgi?id=4092

RazvanN7 · 2023-02-03T09:03:04Z

doesn't return "Windows" even if the class is declared with extern(Windows) (is it supposed to?)

~~It returns Windows only if compiled for 32-bit.~~

Scratch that, it returns D on my machine for both 32 and 64 bit builds.

RazvanN7 · 2023-02-03T09:19:29Z

How are we supposed to test if a class is a COM class? See if it implements IUnknown? Is there an easier way?

jsatellite · 2023-02-03T18:08:58Z

Can't you use T.classinfo.m_flags & TypeInfo_Class.ClassFlags.isCOMClass like in the un-templated _d_newclass?

ibuclaw · 2023-02-12T14:15:40Z

This PR introduced a regression https://issues.dlang.org/show_bug.cgi?id=23688

RazvanN7 · 2023-03-14T10:25:34Z

compiler/src/dmd/expression.h

    bool onstack;               // allocate on stack
    bool thrownew;              // this NewExp is the expression of a ThrowStatement

+    Expression lowering;        // lowered druntime hook: `_d_newclass`


Shouldn't have this been a pointer?

Yes. Fixed by #14872.

ibuclaw · 2023-06-07T17:01:14Z

This PR caused a regression.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110113

I'm going to have to revert this PR if a proper fix doesn't come in a reasonable time-frame given the severity of it.

maxhaton · 2023-06-07T20:29:51Z

Does it only trigger with dip1021?

…

On Wed, 7 Jun 2023, 18:01 Iain Buclaw, ***@***.***> wrote: This PR caused a regression. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110113 I'm going to have to revert this PR if a proper fix doesn't come in a reasonable time-frame given the severity of it. — Reply to this email directly, view it on GitHub <#14837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLI75BIM27AEIKVTJWJIF3XKCXWLANCNFSM6AAAAAAT7M52QY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ibuclaw · 2023-06-07T22:23:57Z

Does it only trigger with dip1021?

All that dip1021 is doing is making the compiler do a few more GC allocations/freeing by executing this code.

https://github.com/dlang/dmd/blob/master/compiler/src/dmd/ob.d

I think more likely, the trigger is -lowmem. Turning on dip1021 just allows the mesns.

Upstream bug report. https://issues.dlang.org/show_bug.cgi?id=23978

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>

…NEWCLASS

* Remove RTLSYM for Translation PR #15819: Removed NEWARRAYMITX, NEWARRAYMITX, TRACENEWARRAYMTX and TRACENEWARRAYMITX * Remove RTLSYM for Translation PR #15299: Removed NEWARRAYT, NEWARRAYIT, TRACENEWARRAYT and TRACENEWARRAYIT * Remove RTLSYM for Translation PR #14837: Removed NEWCLASS, TRACENEWCLASS * Remove RTLSYM for Translation PR #14664: Removed NEWITEMT, NEWITEMIT, TRACENEWITEMT and TRACENEWITEMIT * Remove RTLSYM for Translation PR #14550: Removed ARRAYCATNTX, ARRAYCATT, TRACEARRAYCATNTX and TRACEARRAYCATT * Remove RTLSYM for Translation PR #14382: Removed ARRAYSETASSIGN * Remove RTLSYM for Translation PR #14310: Removed ARRAYASSIGN * Remove RTLSYM for Translation PR #13495: Removed ARRAYAPPENDT, ARRAYAPPENDCTX, TRACEARRAYAPPENDT and TRACEARRAYAPPENDCTX

teodutu requested a review from ibuclaw as a code owner January 18, 2023 18:22

teodutu force-pushed the template-_d_newclass branch from 4a44315 to b56560c Compare January 18, 2023 18:25

ibuclaw previously requested changes Jan 18, 2023

View reviewed changes

compiler/src/dmd/expression.d Show resolved Hide resolved

teodutu force-pushed the template-_d_newclass branch from b56560c to 60fbea0 Compare January 18, 2023 18:48

maxhaton reviewed Jan 18, 2023

View reviewed changes

RazvanN7 reviewed Jan 20, 2023

View reviewed changes

teodutu force-pushed the template-_d_newclass branch from 60fbea0 to 91cd038 Compare January 20, 2023 13:06

teodutu force-pushed the template-_d_newclass branch 2 times, most recently from 08a5136 to de58969 Compare January 20, 2023 13:31

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 20, 2023

ibuclaw reviewed Jan 21, 2023

View reviewed changes

dkorpel previously requested changes Jan 21, 2023

View reviewed changes

druntime/.gdb_history Outdated Show resolved Hide resolved

druntime/src/rt/sections.d Show resolved Hide resolved

teodutu force-pushed the template-_d_newclass branch 3 times, most recently from e1f1ebc to 0af6853 Compare January 21, 2023 14:43

teodutu requested review from dkorpel and ibuclaw and removed request for dkorpel January 21, 2023 19:43

thewilsonator removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 22, 2023

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 24, 2023

dkorpel approved these changes Jan 26, 2023

View reviewed changes

druntime/src/core/lifetime.d Outdated Show resolved Hide resolved

teodutu force-pushed the template-_d_newclass branch from 0af6853 to 48eacda Compare January 26, 2023 22:32

RazvanN7 merged commit 276ef21 into dlang:master Jan 27, 2023

ibuclaw mentioned this pull request Feb 12, 2023

fix Issue 23688 - FTBFS: error: cannot convert 'Expression' to 'Expression*' #14872

Merged

RazvanN7 reviewed Mar 14, 2023

View reviewed changes

andreirusanescu mentioned this pull request Nov 23, 2024

Moved _d_newThrowable in the semantic phase to lowering #17084

Merged

nybzmr added a commit to nybzmr/dmd that referenced this pull request Mar 19, 2025

Remove RTLSYM for Translation PR dlang#14837: Removed NEWCLASS, TRACE…

5b41516

…NEWCLASS

Uh oh!

Conversation

teodutu commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jan 18, 2023

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

teodutu commented Jan 18, 2023

Uh oh!

maxhaton Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

teodutu commented Jan 20, 2023

Uh oh!

RazvanN7 commented Jan 20, 2023

Uh oh!

teodutu commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Jan 20, 2023

Uh oh!

ibuclaw Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

teodutu Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RazvanN7 commented Jan 23, 2023

Uh oh!

thewilsonator commented Jan 23, 2023

Uh oh!

RazvanN7 commented Jan 23, 2023

Uh oh!

RazvanN7 commented Jan 23, 2023

Uh oh!

RazvanN7 commented Jan 24, 2023

Uh oh!

Uh oh!

jsatellite commented Feb 1, 2023

Uh oh!

rainers commented Feb 2, 2023

Uh oh!

RazvanN7 commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Feb 3, 2023

Uh oh!

jsatellite commented Feb 3, 2023

Uh oh!

ibuclaw commented Feb 12, 2023

Uh oh!

RazvanN7 Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

teodutu Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jun 7, 2023

Uh oh!

maxhaton commented Jun 7, 2023 via email

Uh oh!

ibuclaw commented Jun 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

teodutu commented Jan 18, 2023 •

edited

Loading

teodutu Jan 18, 2023 •

edited

Loading

teodutu commented Jan 20, 2023 •

edited

Loading

RazvanN7 commented Feb 3, 2023 •

edited

Loading