From b9f8e7cf24273f2283a39f703b2367c9cb09a0dc Mon Sep 17 00:00:00 2001 From: Teodor Dutu Date: Mon, 13 Nov 2023 13:06:02 +0200 Subject: [PATCH] Fix Issue 24159: Store lowering of `CatAssignExp` in a separate field (#15791) This preserves the `CatAssignExp` in the AST until the glue layer where an error is printed in case this expression is used with `-betterC`. This is required to happen in the glue layer as the semantic analysis doesn't correctly distinguish when code needs to be generated. Signed-off-by: Teodor Dutu --- compiler/src/dmd/dsymbolsem.d | 2 +- compiler/src/dmd/e2ir.d | 65 ++++++++------ compiler/src/dmd/expression.d | 4 +- compiler/src/dmd/expression.h | 2 + compiler/src/dmd/expressionsem.d | 22 ++--- compiler/src/dmd/frontend.h | 2 +- compiler/src/dmd/id.d | 1 - compiler/src/dmd/inline.d | 23 +++++ compiler/src/dmd/nogc.d | 16 +--- compiler/src/dmd/optimize.d | 12 ++- compiler/test/fail_compilation/test24159.d | 14 +++ druntime/src/core/internal/array/appending.d | 89 ++++++++++---------- druntime/src/object.d | 3 +- 13 files changed, 151 insertions(+), 104 deletions(-) create mode 100644 compiler/test/fail_compilation/test24159.d diff --git a/compiler/src/dmd/dsymbolsem.d b/compiler/src/dmd/dsymbolsem.d index 6e9678b572..64a0fe7aea 100644 --- a/compiler/src/dmd/dsymbolsem.d +++ b/compiler/src/dmd/dsymbolsem.d @@ -5901,7 +5901,7 @@ private bool isDRuntimeHook(Identifier id) id == Id._d_arraysetlengthTImpl || id == Id._d_arraysetlengthT || id == Id._d_arraysetlengthTTrace || id == Id._d_arrayappendT || id == Id._d_arrayappendTTrace || - id == Id._d_arrayappendcTXImpl; + id == Id._d_arrayappendcTX; } void templateInstanceSemantic(TemplateInstance tempinst, Scope* sc, ArgumentList argumentList) diff --git a/compiler/src/dmd/e2ir.d b/compiler/src/dmd/e2ir.d index 5f97d71ac8..7daf061d79 100644 --- a/compiler/src/dmd/e2ir.d +++ b/compiler/src/dmd/e2ir.d @@ -2748,24 +2748,25 @@ elem* toElem(Expression e, ref IRState irs) { //printf("CatAssignExp.toElem('%s')\n", ce.toChars()); elem *e; - Type tb1 = ce.e1.type.toBasetype(); - Type tb2 = ce.e2.type.toBasetype(); - assert(tb1.ty == Tarray); - Type tb1n = tb1.nextOf().toBasetype(); - - elem *e1 = toElem(ce.e1, irs); - elem *e2 = toElem(ce.e2, irs); - - /* Because e1 is an lvalue, refer to it via a pointer to it in the form - * of ev. Put any side effects into re1 - */ - elem* re1 = addressElem(e1, ce.e1.type.pointerTo(), false); - elem* ev = el_same(&re1); switch (ce.op) { case EXP.concatenateDcharAssign: { + Type tb1 = ce.e1.type.toBasetype(); + Type tb2 = ce.e2.type.toBasetype(); + assert(tb1.ty == Tarray); + Type tb1n = tb1.nextOf().toBasetype(); + + elem *e1 = toElem(ce.e1, irs); + elem *e2 = toElem(ce.e2, irs); + + /* Because e1 is an lvalue, refer to it via a pointer to it in the form + * of ev. Put any side effects into re1 + */ + elem* re1 = addressElem(e1, ce.e1.type.pointerTo(), false); + elem* ev = el_same(&re1); + // Append dchar to char[] or wchar[] assert(tb2.ty == Tdchar && (tb1n.ty == Tchar || tb1n.ty == Twchar)); @@ -2776,30 +2777,44 @@ elem* toElem(Expression e, ref IRState irs) : RTLSYM.ARRAYAPPENDWD; e = el_bin(OPcall, TYdarray, el_var(getRtlsym(rtl)), ep); toTraceGC(irs, e, ce.loc); - elem_setLoc(e, ce.loc); + + /* Generate: (re1, e, *ev) + */ + e = el_combine(re1, e); + ev = el_una(OPind, e1.Ety, ev); + e = el_combine(e, ev); + break; } case EXP.concatenateAssign: - { - assert(0, "This case should have been rewritten to `_d_arrayappendT` in the semantic phase"); - } - case EXP.concatenateElemAssign: { - assert(0, "This case should have been rewritten to `_d_arrayappendcTX` in the semantic phase"); + /* Do this check during code gen rather than semantic because appending is + * allowed during CTFE, and we cannot distinguish that in semantic. + */ + if (!irs.params.useGC) + { + irs.eSink.error(ce.loc, + "appending to array in `%s` requires the GC which is not available with -betterC", + ce.toChars()); + return el_long(TYint, 0); + } + + if (auto lowering = ce.lowering) + e = toElem(lowering, irs); + else if (ce.op == EXP.concatenateAssign) + assert(0, "This case should have been rewritten to `_d_arrayappendT` in the semantic phase"); + else + assert(0, "This case should have been rewritten to `_d_arrayappendcTX` in the semantic phase"); + + break; } default: assert(0); } - /* Generate: (re1, e, *ev) - */ - e = el_combine(re1, e); - ev = el_una(OPind, e1.Ety, ev); - e = el_combine(e, ev); - elem_setLoc(e, ce.loc); return e; } diff --git a/compiler/src/dmd/expression.d b/compiler/src/dmd/expression.d index 87611f4690..4ccee004a0 100644 --- a/compiler/src/dmd/expression.d +++ b/compiler/src/dmd/expression.d @@ -5633,7 +5633,9 @@ extern (C++) final class UshrAssignExp : BinAssignExp */ extern (C++) class CatAssignExp : BinAssignExp { - extern (D) this(const ref Loc loc, Expression e1, Expression e2) @safe + Expression lowering; // lowered druntime hook `_d_arrayappend{cTX,T}` + + extern (D) this(const ref Loc loc, Expression e1, Expression e2) { super(loc, EXP.concatenateAssign, e1, e2); } diff --git a/compiler/src/dmd/expression.h b/compiler/src/dmd/expression.h index f7f6b0b63f..b699d145d7 100644 --- a/compiler/src/dmd/expression.h +++ b/compiler/src/dmd/expression.h @@ -1148,6 +1148,8 @@ public: class CatAssignExp : public BinAssignExp { public: + Expression *lowering; // lowered druntime hook `_d_arrayappend{cTX,T}` + void accept(Visitor *v) override { v->visit(this); } }; diff --git a/compiler/src/dmd/expressionsem.d b/compiler/src/dmd/expressionsem.d index 1ddb2b1ea0..d7ccf7f2b3 100644 --- a/compiler/src/dmd/expressionsem.d +++ b/compiler/src/dmd/expressionsem.d @@ -11199,8 +11199,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor result = res; - if ((exp.op == EXP.concatenateAssign || exp.op == EXP.concatenateElemAssign) && - sc.needsCodegen()) + if ((exp.op == EXP.concatenateAssign || exp.op == EXP.concatenateElemAssign) && sc.needsCodegen()) { // if aa ordering is triggered, `res` will be a CommaExp // and `.e2` will be the rewritten original expression. @@ -11244,7 +11243,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor arguments.push(exp.e1); arguments.push(exp.e2); Expression ce = new CallExp(exp.loc, id, arguments); - *output = ce.expressionSemantic(sc); + + exp.lowering = ce.expressionSemantic(sc); + *output = exp; } else if (exp.op == EXP.concatenateElemAssign) { @@ -11264,15 +11265,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor } Identifier hook = global.params.tracegc ? Id._d_arrayappendcTXTrace : Id._d_arrayappendcTX; - if (!verifyHookExist(exp.loc, *sc, Id._d_arrayappendcTXImpl, "appending element to arrays", Id.object)) + if (!verifyHookExist(exp.loc, *sc, hook, "appending element to arrays", Id.object)) return setError(); - // Lower to object._d_arrayappendcTXImpl!(typeof(e1))._d_arrayappendcTX{,Trace}(e1, 1), e1[$-1]=e2 + // Lower to object._d_arrayappendcTX{,Trace}(e1, 1), e1[$-1]=e2 Expression id = new IdentifierExp(exp.loc, Id.empty); id = new DotIdExp(exp.loc, id, Id.object); - auto tiargs = new Objects(); - tiargs.push(exp.e1.type); - id = new DotTemplateInstanceExp(exp.loc, id, Id._d_arrayappendcTXImpl, tiargs); id = new DotIdExp(exp.loc, id, hook); auto arguments = new Expressions(); @@ -11299,11 +11297,10 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor { /* Before the template hook, this check was performed in e2ir.d * for expressions like `a ~= a[$-1]`. Here, $ will be modified - * by calling `_d_arrayappendcT`, so we need to save `a[$-1]` in + * by calling `_d_arrayappendcTX`, so we need to save `a[$-1]` in * a temporary variable. */ value2 = extractSideEffect(sc, "__appendtmp", eValue2, value2, true); - exp.e2 = value2; // `__appendtmp*` will be destroyed together with the array `exp.e1`. auto vd = eValue2.isDeclarationExp().declaration.isVarDeclaration(); @@ -11319,13 +11316,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor auto e0 = Expression.combine(ce, ae).expressionSemantic(sc); e0 = Expression.combine(e0, value1); e0 = Expression.combine(eValue1, e0); - e0 = Expression.combine(eValue2, e0); - *output = e0.expressionSemantic(sc); + exp.lowering = e0.expressionSemantic(sc); + *output = exp; } } - } override void visit(AddExp exp) diff --git a/compiler/src/dmd/frontend.h b/compiler/src/dmd/frontend.h index 75cdad207c..d4090e7d36 100644 --- a/compiler/src/dmd/frontend.h +++ b/compiler/src/dmd/frontend.h @@ -7831,6 +7831,7 @@ public: class CatAssignExp : public BinAssignExp { public: + Expression* lowering; void accept(Visitor* v) override; }; @@ -8714,7 +8715,6 @@ struct Id final static Identifier* _d_arraysetlengthTTrace; static Identifier* _d_arrayappendT; static Identifier* _d_arrayappendTTrace; - static Identifier* _d_arrayappendcTXImpl; static Identifier* _d_arrayappendcTX; static Identifier* _d_arrayappendcTXTrace; static Identifier* _d_arraycatnTX; diff --git a/compiler/src/dmd/id.d b/compiler/src/dmd/id.d index 5fcda91b43..4f51a25797 100644 --- a/compiler/src/dmd/id.d +++ b/compiler/src/dmd/id.d @@ -366,7 +366,6 @@ immutable Msgtable[] msgtable = { "_d_arraysetlengthTTrace"}, { "_d_arrayappendT" }, { "_d_arrayappendTTrace" }, - { "_d_arrayappendcTXImpl" }, { "_d_arrayappendcTX" }, { "_d_arrayappendcTXTrace" }, { "_d_arraycatnTX" }, diff --git a/compiler/src/dmd/inline.d b/compiler/src/dmd/inline.d index ab74bff78d..f7209d6206 100644 --- a/compiler/src/dmd/inline.d +++ b/compiler/src/dmd/inline.d @@ -773,6 +773,21 @@ public: result = ce; } + override void visit(CatAssignExp e) + { + auto cae = cast(CatAssignExp) e.copy(); + + if (auto lowering = cae.lowering) + cae.lowering = doInlineAs!Expression(cae.lowering, ids); + else + { + cae.e1 = doInlineAs!Expression(e.e1, ids); + cae.e2 = doInlineAs!Expression(e.e2, ids); + } + + result = cae; + } + override void visit(BinExp e) { auto be = cast(BinExp)e.copy(); @@ -1279,6 +1294,14 @@ public: inlineScan(e.e2); } + override void visit(CatAssignExp e) + { + if (auto lowering = e.lowering) + inlineScan(lowering); + else + visit(cast(BinExp) e); + } + override void visit(BinExp e) { inlineScan(e.e1); diff --git a/compiler/src/dmd/nogc.d b/compiler/src/dmd/nogc.d index 59bf1d5d1d..e59b01019f 100644 --- a/compiler/src/dmd/nogc.d +++ b/compiler/src/dmd/nogc.d @@ -108,12 +108,6 @@ public: return; f.printGCUsage(e.loc, "setting `length` may cause a GC allocation"); } - else if (fd.ident == Id._d_arrayappendT || fd.ident == Id._d_arrayappendcTX) - { - if (setGC(e, "cannot use operator `~=` in `@nogc` %s `%s`")) - return; - f.printGCUsage(e.loc, "operator `~=` may cause a GC allocation"); - } } override void visit(ArrayLiteralExp e) @@ -187,20 +181,14 @@ public: override void visit(CatAssignExp e) { - /* CatAssignExp will exist in `__traits(compiles, ...)` and in the `.e1` branch of a `__ctfe ? :` CondExp. - * The other branch will be `_d_arrayappendcTX(e1, 1), e1[$-1]=e2` which will generate the warning about - * GC usage. See visit(CallExp). - */ if (checkOnly) { err = true; return; } - if (f.setGC(e.loc, null)) - { - err = true; + if (setGC(e, "cannot use operator `~=` in `@nogc` %s `%s`")) return; - } + f.printGCUsage(e.loc, "operator `~=` may cause a GC allocation"); } override void visit(CatExp e) diff --git a/compiler/src/dmd/optimize.d b/compiler/src/dmd/optimize.d index 0065b016f8..4a9fbe360b 100644 --- a/compiler/src/dmd/optimize.d +++ b/compiler/src/dmd/optimize.d @@ -928,6 +928,14 @@ Expression Expression_optimize(Expression e, int result, bool keepLvalue) } } + void visitCatAssign(CatAssignExp e) + { + if (auto lowering = e.lowering) + Expression_optimize(lowering, result, keepLvalue); + else + visitBinAssign(e); + } + void visitBin(BinExp e) { //printf("BinExp::optimize(result = %d) %s\n", result, e.toChars()); @@ -1392,9 +1400,9 @@ Expression Expression_optimize(Expression e, int result, bool keepLvalue) case EXP.leftShiftAssign: case EXP.rightShiftAssign: case EXP.unsignedRightShiftAssign: + case EXP.concatenateDcharAssign: visitBinAssign(ex.isBinAssignExp()); break; case EXP.concatenateElemAssign: - case EXP.concatenateDcharAssign: - case EXP.concatenateAssign: visitBinAssign(ex.isBinAssignExp()); break; + case EXP.concatenateAssign: visitCatAssign(cast(CatAssignExp) ex); break; case EXP.minusMinus: case EXP.plusPlus: diff --git a/compiler/test/fail_compilation/test24159.d b/compiler/test/fail_compilation/test24159.d new file mode 100644 index 0000000000..35c7f36416 --- /dev/null +++ b/compiler/test/fail_compilation/test24159.d @@ -0,0 +1,14 @@ +// https://issues.dlang.org/show_bug.cgi?id=24159 +// REQUIRED_ARGS: -betterC +/* +TEST_OUTPUT: +--- +fail_compilation/test24159.d(13): Error: appending to array in `x ~= 3` requires the GC which is not available with -betterC +--- +*/ + +extern(C) void main() +{ + int[] x = null; + x ~= 3; +} diff --git a/druntime/src/core/internal/array/appending.d b/druntime/src/core/internal/array/appending.d index bb24813ae9..ba34727a30 100644 --- a/druntime/src/core/internal/array/appending.d +++ b/druntime/src/core/internal/array/appending.d @@ -14,56 +14,55 @@ private extern (C) byte[] _d_arrayappendcTX(const TypeInfo ti, ref return scope private enum isCopyingNothrow(T) = __traits(compiles, (ref T rhs) nothrow { T lhs = rhs; }); -/// Implementation of `_d_arrayappendcTX` and `_d_arrayappendcTXTrace` -template _d_arrayappendcTXImpl(Tarr : T[], T) +/** + * Extend an array `px` by `n` elements. + * Caller must initialize those elements. + * Params: + * px = the array that will be extended, taken as a reference + * n = how many new elements to extend it with + * Returns: + * The new value of `px` + * Bugs: + * This function template was ported from a much older runtime hook that bypassed safety, + * purity, and throwabilty checks. To prevent breaking existing code, this function template + * is temporarily declared `@trusted pure` until the implementation can be brought up to modern D expectations. + */ +ref Tarr _d_arrayappendcTX(Tarr : T[], T)(return ref scope Tarr px, size_t n) @trusted { - private enum errorMessage = "Cannot append to array if compiling without support for runtime type information!"; - - /** - * Extend an array `px` by `n` elements. - * Caller must initialize those elements. - * Params: - * px = the array that will be extended, taken as a reference - * n = how many new elements to extend it with - * Returns: - * The new value of `px` - * Bugs: - * This function template was ported from a much older runtime hook that bypassed safety, - * purity, and throwabilty checks. To prevent breaking existing code, this function template - * is temporarily declared `@trusted pure` until the implementation can be brought up to modern D expectations. - */ - ref Tarr _d_arrayappendcTX(return ref scope Tarr px, size_t n) @trusted pure nothrow + // needed for CTFE: https://github.com/dlang/druntime/pull/3870#issuecomment-1178800718 + version (DigitalMars) pragma(inline, false); + version (D_TypeInfo) + { + auto ti = typeid(Tarr); + + // _d_arrayappendcTX takes the `px` as a ref byte[], but its length + // should still be the original length + auto pxx = (cast(byte*)px.ptr)[0 .. px.length]; + ._d_arrayappendcTX(ti, pxx, n); + px = (cast(T*)pxx.ptr)[0 .. pxx.length]; + + return px; + } + else + assert(0, "Cannot append to array if compiling without support for runtime type information!"); +} + +version (D_ProfileGC) +{ + /** + * TraceGC wrapper around $(REF _d_arrayappendT, core,internal,array,appending). + */ + ref Tarr _d_arrayappendcTXTrace(Tarr : T[], T)(string file, int line, string funcname, return ref scope Tarr px, size_t n) @trusted { - // needed for CTFE: https://github.com/dlang/druntime/pull/3870#issuecomment-1178800718 - version (DigitalMars) pragma(inline, false); version (D_TypeInfo) { - auto ti = typeid(Tarr); + import core.internal.array.utils: TraceHook, gcStatsPure, accumulatePure; + mixin(TraceHook!(Tarr.stringof, "_d_arrayappendcTX")); - // _d_arrayappendcTX takes the `px` as a ref byte[], but its length - // should still be the original length - auto pxx = (cast(byte*)px.ptr)[0 .. px.length]; - ._d_arrayappendcTX(ti, pxx, n); - px = (cast(T*)pxx.ptr)[0 .. pxx.length]; - - return px; + return _d_arrayappendcTX(px, n); } else - assert(0, errorMessage); - } - - version (D_ProfileGC) - { - import core.internal.array.utils : _d_HookTraceImpl; - - /** - * TraceGC wrapper around $(REF _d_arrayappendcTX, rt,array,appending,_d_arrayappendcTXImpl). - * Bugs: - * This function template was ported from a much older runtime hook that bypassed safety, - * purity, and throwabilty checks. To prevent breaking existing code, this function template - * is temporarily declared `@trusted pure` until the implementation can be brought up to modern D expectations. - */ - alias _d_arrayappendcTXTrace = _d_HookTraceImpl!(Tarr, _d_arrayappendcTX, errorMessage); + static assert(0, "Cannot append to array if compiling without support for runtime type information!"); } } @@ -78,7 +77,7 @@ ref Tarr _d_arrayappendT(Tarr : T[], T)(return ref scope Tarr x, scope Tarr y) @ enum hasPostblit = __traits(hasPostblit, T); auto length = x.length; - _d_arrayappendcTXImpl!Tarr._d_arrayappendcTX(x, y.length); + _d_arrayappendcTX(x, y.length); // Only call `copyEmplace` if `T` has a copy ctor and no postblit. static if (hasElaborateCopyConstructor!T && !hasPostblit) @@ -126,7 +125,7 @@ version (D_ProfileGC) return _d_arrayappendT(x, y); } else - assert(0, "Cannot append to array if compiling without support for runtime type information!"); + static assert(0, "Cannot append to array if compiling without support for runtime type information!"); } } diff --git a/druntime/src/object.d b/druntime/src/object.d index 768b358d10..70cafd01ca 100644 --- a/druntime/src/object.d +++ b/druntime/src/object.d @@ -4667,11 +4667,12 @@ public import core.internal.array.appending : _d_arrayappendT; version (D_ProfileGC) { public import core.internal.array.appending : _d_arrayappendTTrace; + public import core.internal.array.appending : _d_arrayappendcTXTrace; public import core.internal.array.concatenation : _d_arraycatnTXTrace; public import core.lifetime : _d_newitemTTrace; public import core.internal.array.construction : _d_newarrayTTrace; } -public import core.internal.array.appending : _d_arrayappendcTXImpl; +public import core.internal.array.appending : _d_arrayappendcTX; public import core.internal.array.comparison : __cmp; public import core.internal.array.equality : __equals; public import core.internal.array.casting: __ArrayCast;