Skip to content

Commit

Permalink
Make lambda symbols stable post a3abf11
Browse files Browse the repository at this point in the history
Commit a3abf11 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.
  • Loading branch information
yanok committed Dec 2, 2024
1 parent d8c0e79 commit d5964d8
Showing 1 changed file with 13 additions and 4 deletions.
17 changes: 13 additions & 4 deletions compiler/src/dmd/identifier.d
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<prefix>_L<line>_C<col>`
OutBuffer idBuf;
Expand All @@ -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
{
Expand All @@ -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("_");
Expand Down

0 comments on commit d5964d8

Please sign in to comment.