From ce7c5df44f4ac6aee1671ecbc54bb08c2a8f3e31 Mon Sep 17 00:00:00 2001 From: Iain Buclaw Date: Wed, 4 Jan 2023 19:03:17 +0100 Subject: [PATCH 1/5] =?UTF-8?q?fix=20Issue=C2=A018026=20-=20Stack=20overfl?= =?UTF-8?q?ow=20in=20dmd/dtemplate.d:6248,=20TemplateInstance::needsCodege?= =?UTF-8?q?n()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- compiler/src/dmd/dtemplate.d | 74 +++++++++++++++++----------- compiler/test/compilable/test18026.d | 12 +++++ 2 files changed, 58 insertions(+), 28 deletions(-) create mode 100644 compiler/test/compilable/test18026.d diff --git a/compiler/src/dmd/dtemplate.d b/compiler/src/dmd/dtemplate.d index 32799aa04e32..3cfe2013c166 100644 --- a/compiler/src/dmd/dtemplate.d +++ b/compiler/src/dmd/dtemplate.d @@ -6264,24 +6264,31 @@ extern (C++) class TemplateInstance : ScopeDsymbol if (global.params.allInst) { // Do codegen if there is an instantiation from a root module, to maximize link-ability. - - // Do codegen if `this` is instantiated from a root module. - if (minst && minst.isRoot()) - return true; - - // Do codegen if the ancestor needs it. - if (tinst && tinst.needsCodegen()) + static ThreeState needsCodegenAllInst(TemplateInstance tithis, TemplateInstance tinst) { - minst = tinst.minst; // cache result - assert(minst); - assert(minst.isRoot()); - return true; + // Do codegen if `this` is instantiated from a root module. + if (tithis.minst && tithis.minst.isRoot()) + return ThreeState.yes; + + // Do codegen if the ancestor needs it. + if (tinst && tinst.needsCodegen()) + { + tithis.minst = tinst.minst; // cache result + assert(tithis.minst); + assert(tithis.minst.isRoot()); + return ThreeState.yes; + } + return ThreeState.none; } + if (const needsCodegen = needsCodegenAllInst(this, tinst)) + return needsCodegen == ThreeState.yes ? true : false; + // Do codegen if a sibling needs it. - if (tnext) + for (; tnext; tnext = tnext.tnext) { - if (tnext.needsCodegen()) + const needsCodegen = needsCodegenAllInst(tnext, tnext.tinst); + if (needsCodegen == ThreeState.yes) { minst = tnext.minst; // cache result assert(minst); @@ -6293,6 +6300,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol minst = tnext.minst; // cache result from non-speculative sibling return false; } + else if (needsCodegen != ThreeState.none) + break; } // Elide codegen because there's no instantiation from any root modules. @@ -6317,31 +6326,38 @@ extern (C++) class TemplateInstance : ScopeDsymbol * => Elide codegen if there is at least one instantiation from a non-root module * which doesn't import any root modules. */ - - // If the ancestor isn't speculative, - // 1. do codegen if the ancestor needs it - // 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree) - if (tinst) + static ThreeState needsCodegenRootOnly(TemplateInstance tithis, TemplateInstance tinst) { - const needsCodegen = tinst.needsCodegen(); // sets tinst.minst - if (tinst.minst) // not speculative + // If the ancestor isn't speculative, + // 1. do codegen if the ancestor needs it + // 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree) + if (tinst) { - minst = tinst.minst; // cache result - return needsCodegen; + const needsCodegen = tinst.needsCodegen(); // sets tinst.minst + if (tinst.minst) // not speculative + { + tithis.minst = tinst.minst; // cache result + return needsCodegen ? ThreeState.yes : ThreeState.no; + } } + + // Elide codegen if `this` doesn't need it. + if (tithis.minst && !tithis.minst.isRoot() && !tithis.minst.rootImports()) + return ThreeState.no; + + return ThreeState.none; } - // Elide codegen if `this` doesn't need it. - if (minst && !minst.isRoot() && !minst.rootImports()) - return false; + if (const needsCodegen = needsCodegenRootOnly(this, tinst)) + return needsCodegen == ThreeState.yes ? true : false; // Elide codegen if a (non-speculative) sibling doesn't need it. - if (tnext) + for (; tnext; tnext = tnext.tnext) { - const needsCodegen = tnext.needsCodegen(); // sets tnext.minst + const needsCodegen = needsCodegenRootOnly(tnext, tnext.tinst); // sets tnext.minst if (tnext.minst) // not speculative { - if (!needsCodegen) + if (needsCodegen == ThreeState.no) { minst = tnext.minst; // cache result assert(!minst.isRoot() && !minst.rootImports()); @@ -6352,6 +6368,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol minst = tnext.minst; // cache result from non-speculative sibling return true; } + else if (needsCodegen != ThreeState.none) + break; } } diff --git a/compiler/test/compilable/test18026.d b/compiler/test/compilable/test18026.d new file mode 100644 index 000000000000..97d83b4b409d --- /dev/null +++ b/compiler/test/compilable/test18026.d @@ -0,0 +1,12 @@ +// https://issues.dlang.org/show_bug.cgi?id=18026 +bool f(T)(T x) +{ + return false; +} + +static foreach(i; 0..60000) +{ + static if(f(i)) + { + } +} From 5002893314bd3c9a9f5ad0d4d99d074fb0e6d063 Mon Sep 17 00:00:00 2001 From: Iain Buclaw Date: Mon, 9 Jan 2023 20:32:04 +0100 Subject: [PATCH 2/5] dmd.dtemplate: Add assert(this is inst) to needsCodegen --- compiler/src/dmd/dtemplate.d | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/compiler/src/dmd/dtemplate.d b/compiler/src/dmd/dtemplate.d index 3cfe2013c166..1b9aa4949d01 100644 --- a/compiler/src/dmd/dtemplate.d +++ b/compiler/src/dmd/dtemplate.d @@ -6261,6 +6261,9 @@ extern (C++) class TemplateInstance : ScopeDsymbol return false; } + // This should only be called on the primary instantiation. + assert(this is inst || inst is null); + if (global.params.allInst) { // Do codegen if there is an instantiation from a root module, to maximize link-ability. @@ -6271,6 +6274,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol return ThreeState.yes; // Do codegen if the ancestor needs it. + if (tinst && tinst.inst && tinst !is tinst.inst) + tinst = tinst.inst; if (tinst && tinst.needsCodegen()) { tithis.minst = tinst.minst; // cache result @@ -6333,6 +6338,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol // 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree) if (tinst) { + if (tinst.inst && tinst !is tinst.inst) + tinst = tinst.inst; const needsCodegen = tinst.needsCodegen(); // sets tinst.minst if (tinst.minst) // not speculative { From 1ad92d71ad9d88008dc84834ddd9d5e14ef22e00 Mon Sep 17 00:00:00 2001 From: Iain Buclaw Date: Tue, 10 Jan 2023 00:46:42 +0100 Subject: [PATCH 3/5] dmd.dtemplate: Don't end search after making inst non-speculative in needsCodegen --- compiler/src/dmd/dtemplate.d | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/src/dmd/dtemplate.d b/compiler/src/dmd/dtemplate.d index 1b9aa4949d01..94fd8e4a2655 100644 --- a/compiler/src/dmd/dtemplate.d +++ b/compiler/src/dmd/dtemplate.d @@ -6303,10 +6303,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol else if (!minst && tnext.minst) { minst = tnext.minst; // cache result from non-speculative sibling - return false; + // continue searching } - else if (needsCodegen != ThreeState.none) - break; } // Elide codegen because there's no instantiation from any root modules. @@ -6373,10 +6371,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol else if (!minst) { minst = tnext.minst; // cache result from non-speculative sibling - return true; + // continue searching } - else if (needsCodegen != ThreeState.none) - break; } } From 2128aad761140c6fce35996d86d87a8d0d544a7a Mon Sep 17 00:00:00 2001 From: Iain Buclaw Date: Tue, 10 Jan 2023 15:34:47 +0100 Subject: [PATCH 4/5] dmd.dtemplate: Break if needsCodegen is not ThreeState.none --- compiler/src/dmd/dtemplate.d | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/src/dmd/dtemplate.d b/compiler/src/dmd/dtemplate.d index 94fd8e4a2655..160a44b01e0b 100644 --- a/compiler/src/dmd/dtemplate.d +++ b/compiler/src/dmd/dtemplate.d @@ -6305,6 +6305,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol minst = tnext.minst; // cache result from non-speculative sibling // continue searching } + else if (needsCodegen != ThreeState.none) + break; } // Elide codegen because there's no instantiation from any root modules. @@ -6373,6 +6375,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol minst = tnext.minst; // cache result from non-speculative sibling // continue searching } + else if (needsCodegen != ThreeState.none) + break; } } From 2a993e3f30cb395068aec46b2034df86705d95e6 Mon Sep 17 00:00:00 2001 From: Iain Buclaw Date: Tue, 10 Jan 2023 17:37:17 +0100 Subject: [PATCH 5/5] dmd.dtemplate: Recurse through the primary instantiation, skip if 'inst' was set to null --- compiler/src/dmd/dtemplate.d | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/src/dmd/dtemplate.d b/compiler/src/dmd/dtemplate.d index 160a44b01e0b..26b2fa250964 100644 --- a/compiler/src/dmd/dtemplate.d +++ b/compiler/src/dmd/dtemplate.d @@ -6255,14 +6255,17 @@ extern (C++) class TemplateInstance : ScopeDsymbol this.tnext = null; this.tinst = null; - if (errors || (inst && inst.isDiscardable())) + // Don't do codegen if the instance has errors, + // is a dummy instance (see evaluateConstraint), + // or is determined to be discardable. + if (errors || inst is null || inst.isDiscardable()) { minst = null; // mark as speculative return false; } // This should only be called on the primary instantiation. - assert(this is inst || inst is null); + assert(this is inst); if (global.params.allInst) { @@ -6274,11 +6277,9 @@ extern (C++) class TemplateInstance : ScopeDsymbol return ThreeState.yes; // Do codegen if the ancestor needs it. - if (tinst && tinst.inst && tinst !is tinst.inst) - tinst = tinst.inst; - if (tinst && tinst.needsCodegen()) + if (tinst && tinst.inst && tinst.inst.needsCodegen()) { - tithis.minst = tinst.minst; // cache result + tithis.minst = tinst.inst.minst; // cache result assert(tithis.minst); assert(tithis.minst.isRoot()); return ThreeState.yes; @@ -6336,10 +6337,9 @@ extern (C++) class TemplateInstance : ScopeDsymbol // If the ancestor isn't speculative, // 1. do codegen if the ancestor needs it // 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree) - if (tinst) + if (tinst && tinst.inst) { - if (tinst.inst && tinst !is tinst.inst) - tinst = tinst.inst; + tinst = tinst.inst; const needsCodegen = tinst.needsCodegen(); // sets tinst.minst if (tinst.minst) // not speculative {