From 24d5387a3a55365fc66777654bb74244e6a196c1 Mon Sep 17 00:00:00 2001 From: Ilya Yanok Date: Fri, 22 Nov 2024 16:41:46 +0100 Subject: [PATCH 1/2] Make lambda symbols stable post c1ace6355ad35fbe53eec8a349598d1bc011deff Commit c1ace6355ad35fbe53eec8a349598d1bc011deff fixes some cases of lambdas having unstable symbol names between compilation units by using `generateIdWithLoc` to generate stable lambda names, however since LOC doesn't uniquely identify a lambda instance (because templates, mixins, static foreach and foreach unrolling), `generateIdWithLoc` adds a counter, so there is still some instability going on. `generateIdWithLoc` makes the name uniq per file+loc, by adding adding a numeric suffix. But the order of instantiations might be different across compilation units, so with this counting scheme we are back to unstable names, so one module might have `t!0.__lambda_LOC` and `t!1.__lambda_LOC_1` while another one has `t!1.__lambda_LOC` This is not a critical problem, but at very least the code gets duplicated for no reason. I also have an example where it leads to linking error, but since it's not a small one and fails to minimize further, I suspect it's a result of interaction with some other bug. The thing is we don't even need uniqueness for those lambdas inside templates/mixins: their final names will have the instantiation prefix anyway. But we can't also just disable this uniqueness check completely: `static foreach` as well as unrollings of the normal `foreach` with lambdas in the loop body will have several copies of a single lambda with the same file+loc. So here we do want to keep making them unique. Fortunately, I don't think a `foreach` could be iterated in different order in different compilation units, so hopefully if we limit the counting to this case only, it won't make symbols unstable. To implement this idea, I've added an extra `parent` argument to `generateIdWithLoc`: it works like using `parent ~ prefix` prefix, but without adding `parent` to the final output. Fixes since last review: 1. Changed `fromStringz` to `toDString` 2. Added a test to showcase the problem --- dmd/expressionsem.d | 2 +- dmd/identifier.d | 17 +++++++++++++---- tests/dmd/runnable/imports/test23722_2b.d | 13 +++++++++++++ tests/dmd/runnable/test23722_2.d | 10 ++++++++++ 4 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 tests/dmd/runnable/imports/test23722_2b.d create mode 100644 tests/dmd/runnable/test23722_2.d diff --git a/dmd/expressionsem.d b/dmd/expressionsem.d index a83e992cee..6ebb20a56a 100644 --- a/dmd/expressionsem.d +++ b/dmd/expressionsem.d @@ -5582,7 +5582,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor symtab = sds.symtab; } assert(symtab); - Identifier id = Identifier.generateIdWithLoc(s, exp.loc); + Identifier id = Identifier.generateIdWithLoc(s, exp.loc, cast(string) toDString(sc.parent.toPrettyChars())); exp.fd.ident = id; if (exp.td) exp.td.ident = id; diff --git a/dmd/identifier.d b/dmd/identifier.d index 6fd0d3ad5e..74be1beb29 100644 --- a/dmd/identifier.d +++ b/dmd/identifier.d @@ -211,11 +211,14 @@ nothrow: * Params: * prefix = first part of the identifier name. * loc = source location to use in the identifier name. + * parent = (optional) extra part to be used in uniqueness check, + * if (prefix1, loc1) == (prefix2, loc2), but + * parent1 != parent2, no new name will be generated. * Returns: * Identifier (inside Identifier.idPool) with deterministic name based * on the source location. */ - extern (D) static Identifier generateIdWithLoc(string prefix, const ref Loc loc) + extern (D) static Identifier generateIdWithLoc(string prefix, const ref Loc loc, string parent = "") { // generate `_L_C` OutBuffer idBuf; @@ -234,14 +237,20 @@ nothrow: * https://issues.dlang.org/show_bug.cgi?id=18880 * https://issues.dlang.org/show_bug.cgi?id=18868 * https://issues.dlang.org/show_bug.cgi?id=19058 + * + * It is a bit trickier for lambdas/dgliterals: we want them to be unique per + * module/mixin + function/template instantiation context. So we use extra parent + * argument for that when dealing with lambdas. We could have added it to prefix + * directly, but that would unnecessary lengthen symbols names. See issue: + * https://issues.dlang.org/show_bug.cgi?id=23722 */ - static struct Key { Loc loc; string prefix; } + static struct Key { Loc loc; string prefix; string parent; } __gshared uint[Key] counters; static if (__traits(compiles, counters.update(Key.init, () => 0u, (ref uint a) => 0u))) { // 2.082+ - counters.update(Key(loc, prefix), + counters.update(Key(loc, prefix, parent), () => 1u, // insertion (ref uint counter) // update { @@ -253,7 +262,7 @@ nothrow: } else { - const key = Key(loc, prefix); + const key = Key(loc, prefix, parent); if (auto pCounter = key in counters) { idBuf.writestring("_"); diff --git a/tests/dmd/runnable/imports/test23722_2b.d b/tests/dmd/runnable/imports/test23722_2b.d new file mode 100644 index 0000000000..b0e54a936f --- /dev/null +++ b/tests/dmd/runnable/imports/test23722_2b.d @@ -0,0 +1,13 @@ +module imports.test23722_2b; + +struct T(alias fun) { } + +struct S(int i) { + auto t = T!(x => i)(); +} + +string g() { + S!0 s0; + S!1 s1; + return s1.t.init.mangleof; +} diff --git a/tests/dmd/runnable/test23722_2.d b/tests/dmd/runnable/test23722_2.d new file mode 100644 index 0000000000..cce4629933 --- /dev/null +++ b/tests/dmd/runnable/test23722_2.d @@ -0,0 +1,10 @@ +// COMPILE_SEPARATELY: +// EXTRA_SOURCES: imports/test23722_2b.d +// https://issues.dlang.org/show_bug.cgi?id=23722 +// Lambdas are mangled incorrectly when using multiple compilation units, resulting in incorrect code +import imports.test23722_2b; + +void main() { + S!1 s1; + assert(s1.t.init.mangleof == g); +} From 5c3509e7d4b9cd533683cfb6de4811580f9fe5fc Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sat, 7 Dec 2024 23:59:30 +0100 Subject: [PATCH 2/2] Bump Phobos --- CHANGELOG.md | 8 ++++---- runtime/phobos | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dda9721aa..cd30cc6c68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,13 @@ # LDC master #### Big news -- Frontend, druntime and Phobos are at version [2.110.0](https://dlang.org/changelog/2.110.0.html). (#4707, #4737, #4749, #4768, #4784, #4792) -- Support for [LLVM 19](https://releases.llvm.org/19.1.0/docs/ReleaseNotes.html); LLVM for prebuilt packages bumped to v19.1.3 (incl. macOS arm64). (#4712, #4735, #4763, #4772) +- Frontend, druntime and Phobos are at version [2.110.0](https://dlang.org/changelog/2.110.0.html). (#4707, #4737, #4749, #4768, #4784, #4792, #4798) +- Support for [LLVM 19](https://releases.llvm.org/19.1.0/docs/ReleaseNotes.html). The prebuilt packages use v19.1.3 (incl. macOS arm64). (#4712, #4735, #4763, #4772) +- Objective-C: The compiler now properly supports Objective-C classes and protocols, as well as swift stub classes (via the `@swift` UDA). (#4777) - Android: NDK for prebuilt package bumped from r26d to r27c. (#4711, #4772) -- ldc2.conf: %%ldcconfigpath%% placeholder added - specifies the directory where current configuration file is located. (#4717) +- ldc2.conf: `%%ldcconfigpath%%` placeholder added - specifies the directory where current configuration file is located. (#4717) - Add support for building against a system copy of zlib through `-DPHOBOS_SYSTEM_ZLIB=ON`. (#4742) - Emscripten: The compiler now mimicks a musl Linux platform wrt. extra predefined versions (`linux`, `Posix`, `CRuntime_Musl`, `CppRuntime_LLVM`). (#4750) -- Objective-C: The compiler now properly supports Objective-C classes and protocols, as well as swift stub classes (via the `@swift` UDA). (#4777) #### Platform support - Supports LLVM 15 - 19. diff --git a/runtime/phobos b/runtime/phobos index e6d14f5151..e053773c6b 160000 --- a/runtime/phobos +++ b/runtime/phobos @@ -1 +1 @@ -Subproject commit e6d14f51518719988db1b83ab7af541dc4824e95 +Subproject commit e053773c6bc837b1136654e122963bfa878768f3