TypeInfo_Struct: Switch to stored mangled name & demangle lazily (with per-thread cache) #3527

kinke · 2021-07-26T20:16:25Z

Making the TypeInfo_Struct names truly unique and more compact at the same time. Requires compiler support: dlang/dmd#12928

Caching the demangled name in the instance itself isn't possible because TypeInfo.toString() is const. I had to remove its pure in order to cache the demangled name in a per-thread AA (and using const void* as key, as const TypeInfo_Struct leads to an infinite recursion...).

dlang-bot · 2021-07-26T20:16:27Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 + druntime#3527"

Making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527.

kinke · 2021-07-26T21:17:26Z

I've run the tests locally on Linux x64 with both dmd and druntime branches; all green (dmd/druntime/phobos) except for (apparently hanging) druntime integration subtest printf of test/gc/.

kinke · 2021-07-26T21:20:05Z

src/rt/util/typeinfo.d

-    }
+unittest
+{
+    assert(typeid(void).toString == "void");


TypeInfo.toString() isn't pure anymore.

rainers · 2021-07-27T06:04:59Z

except for (apparently hanging) druntime integration subtest printf of test/gc/

The test is trying to print the types of allocations while the GC lock is held: this deadlocks when accessing name, see https://github.com/dlang/druntime/blob/master/src/core/internal/gc/impl/conservative/gc.d#L4061.

That also shows another subtle change: using TypeInfo_Struct.name no longer is @nogc. Could you use HashTab from core.internal.container.hashtab instead of the AA?

PetarKirov · 2021-07-27T06:50:14Z

TypeInfo.toString() isn't pure

This is a breaking change (although I was expecting quite more to be broken than just atilaneves/unit-threaded), it needs a changelog entry and stronger justification.

Making the TypeInfo_Struct names truly unique

While this certainly is a good thing, can you explain why is it necessary? Are you trying to solve some bigger problem with this?

kinke · 2021-07-27T10:56:55Z

The test is trying to print the types of allocations while the GC lock is held: this deadlocks when accessing name

Cheers; worked around it for now by using mangledName there.

That also shows another subtle change: using TypeInfo_Struct.name no longer is @nogc. Could you use HashTab from core.internal.container.hashtab instead of the AA?

The AA shouldn't really matter, as core.demangle GC-allocates anyway, unless using some huge buffer that is guaranteed to suffice for all names and then malloc'ing an appropriate copy afterwards...

kinke · 2021-07-27T11:27:48Z

@PetarKirov: I haven't found an existing issue yet, so filed a new one: https://issues.dlang.org/show_bug.cgi?id=22149

This is especially bad for classes (tackled later), where it can e.g. break EH with LDC on Windows (see ldc-developers/ldc#3517), and dynamic casting in general AFAIK.

…ched equality semantics By storing the mangled name, making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527.

kinke · 2021-07-27T11:35:22Z

Testcase added to dlang/dmd#12928.

PetarKirov · 2021-07-27T11:36:37Z

Indeed, that's a quite severe. Thanks for the explanations and linking the LDC PR!

…ched equality semantics By storing the mangled name, making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527.

rainers · 2021-07-27T16:45:13Z

Cheers; worked around it for now by using mangledName there.

Fine with me, a log can be run through ddemangle if needed.

The AA shouldn't really matter, as core.demangle GC-allocates anyway

Noticed that too after commenting. An alternative could be to abort demangling if the passed buffer is too small and let the caller allocate a larger one. If only the demangler would not use exceptions (which it only needs for backward compatibilty with older ambiguous manglings IIRC). Changing the demangler would be another PR, though.

kinke · 2021-07-27T18:39:55Z

Fine with me, a log can be run through ddemangle if needed.

ddemangle unfortunately doesn't seem to support such type names (S3foo3bar).

An alternative could be to abort demangling if the passed buffer is too small and let the caller allocate a larger one.

Yeah, or passing some sink which is responsible for increasing the internal capacity as needed.

If only the demangler would not use exceptions (which it only needs for backward compatibilty with older ambiguous manglings IIRC).

Yeah, I find it also very suboptimal as demangling is used for exception backtraces, and throws at that time aren't really to be expected.

Changing the demangler would be another PR, though.

Definitely. I've just seen that there are other 'beautiful' related hacks like

druntime/src/object.d

Lines 1209 to 1210 in 3a9ede0

    
           alias SafeDemangleFunctionType = char[] function (const(char)[] buf, char[] dst = null) @safe nothrow pure; 
        
           SafeDemangleFunctionType demangle = cast(SafeDemangleFunctionType) &demangleType;

CI doesn't seem to really work with feature branches (same-named branches directly on the official dlang repos) to tackle compiler and druntime at the same time; both PRs currently include a temporary commit to make Cirrus CI use the correct branches at least. Those commits obviously need to be removed before merging.

Restoring pure to TypeInfo_Class.toString() etc. made unit-threaded pass again for Buildkite. A changelog entry still needs to be added to emphasize that breaking change; ~~not sure if here or in the DMD PR~~ [edit: added here]. The latter also includes the (trivial) fix for TypeInfo_Class now.

kinke · 2021-07-27T18:52:33Z

src/object.d

+    {
+        return hashOf(this.mangledName);
+    }
+


TypeInfo.opCmp uses toString() too; I'm not convinced overriding it here to check for a TypeInfo_Struct rhs and comparing the mangledName instead is worth it.

…h per-thread cache) Making the TypeInfo_Struct names truly unique and more compact at the same time. Requires compiler support. Caching the demangled name in the instance itself isn't possible because `TypeInfo.toString()` is const. I had to remove its `pure` in order to cache the demangled name in a per-thread AA (and using `const void*` as key, as `const TypeInfo_Struct` leads to an infinite recursion...).

This is especially useful to make `typeid(<class ref>).toString()` pure again (e.g., used by unit-threaded's `shouldThrow`).

… from the official dlang repo

kinke · 2021-07-28T00:20:24Z

CI doesn't seem to really work with feature branches (same-named branches directly on the official dlang repos) to tackle compiler and druntime at the same time

I've made it work for Azure Pipelines now (one according commit in each PR; all Windows jobs are green); the druntime patch here is a bit of an ugly copy-pasta but does the job. So PRs originating from branches on the official dlang repos will try to use same-named branches for the other repos, and fall back to master if the branch doesn't exist. To make that work for Phobos PRs too, it would need a commit analogous to the one here, but Phobos isn't as tightly tied to the compiler as druntime, so I don't think it's really needed. So

useful when a change involves both compiler and druntime at the same time like this one here,
only for people with write access to both dlang repos,
preventing PRs originating from the official repo from targeting stable (due to the master fallback) - submit those from your fork instead.

Edit: Now (properly) working with Cirrus CI too and thus covering Windows/Linux/macOS/FreeBSD; the previous hack doesn't need to be removed anymore.

…ing from the official dlang repo

PetarKirov · 2021-07-28T06:52:15Z

CI doesn't seem to really work with feature branches

We really should stop wasting everyone's time with the artificial split between dmd and druntime (and possibly phobos) and merge them in a mono repo.

kinke · 2021-07-29T18:29:18Z

[Would be nice to have this land in 2.098; Walter has approved of the DMD PR.]

RazvanN7 · 2021-07-30T09:18:58Z

@kinke It seems that the autotester is failing a couple of tests in dmd. Is this expected?

kinke · 2021-07-30T13:35:53Z

@RazvanN7: Yes, these 2 DMD testsuite failures are expected if DMD and druntime aren't in sync.

RazvanN7 · 2021-07-30T14:00:07Z

I'm gonna pull the trigger on these PRs. Let's hope everything will run smoothly.

RazvanN7 · 2021-07-30T14:02:39Z

Hmm, unfortunately I don't have the right to merge this unless it passes all required tests. However, I have enough permissions to merge the dmd PR.

Maybe I can do that and wait for this PR to be green? That will stall the entire pipeline. But I don't see any other way out of it.

kinke · 2021-07-30T19:14:09Z

Can't you merge manually via git cmdline? Should I try to?

RazvanN7 · 2021-08-01T11:06:33Z

Feel free to merge both PRs. (I apparently can't merge this one)

kinke · 2021-08-01T11:56:08Z

@RazvanN7: I can't merge any one of these PRs either; looks like the GitHub branch protection rules even apply to manual pushes. So if you can indeed merge the DMD one, please go ahead.

…otched equality semantics (#12928) * Fix Issue 22149 - TypeInfo_Struct names aren't unique, leading to botched equality semantics By storing the mangled name, making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527. * Fix Issue 22150 - TypeInfo_Class names aren't unique, leading to botched equality semantics By fully qualifying template arguments. * [temp] Cirrus CI: Use same-named druntime branch * Azure Pipelines: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo * Cirrus CI: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo

RazvanN7 · 2021-08-01T12:05:14Z

@kinke Merged it. ~~Maybe re-push to restart failing tests.~~ Autotester has restarted and buildkite can be manually restarted

…otched equality semantics (#12928) * Fix Issue 22149 - TypeInfo_Struct names aren't unique, leading to botched equality semantics By storing the mangled name, making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527. * Fix Issue 22150 - TypeInfo_Class names aren't unique, leading to botched equality semantics By fully qualifying template arguments. * [temp] Cirrus CI: Use same-named druntime branch * Azure Pipelines: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo * Cirrus CI: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo

…otched equality semantics (dlang#12928) * Fix Issue 22149 - TypeInfo_Struct names aren't unique, leading to botched equality semantics By storing the mangled name, making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527. * Fix Issue 22150 - TypeInfo_Class names aren't unique, leading to botched equality semantics By fully qualifying template arguments. * [temp] Cirrus CI: Use same-named druntime branch * Azure Pipelines: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo * Cirrus CI: Try to use same-named druntime/Phobos branches for PRs originating from the official dlang repo

kinke added a commit to dlang/dmd that referenced this pull request Jul 26, 2021

TypeInfo_Struct: Switch to stored mangled name

742483e

Making the TypeInfo_Struct names truly unique and more compact at the same time. Requires dlang/druntime#3527.

kinke mentioned this pull request Jul 26, 2021

Fix Issues 22149 & 22150 - TypeInfo names aren't unique, leading to botched equality semantics dlang/dmd#12928

Merged

kinke force-pushed the typeinfostruct_mangledName branch 3 times, most recently from 8201c3c to 1ce87d4 Compare July 26, 2021 21:13

kinke requested a review from rainers July 26, 2021 21:18

kinke commented Jul 26, 2021

View reviewed changes

src/rt/util/typeinfo.d

}

unittest

{

assert(typeid(void).toString == "void");

Copy link

Contributor Author

kinke Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeInfo.toString() isn't pure anymore.

kinke force-pushed the typeinfostruct_mangledName branch from c7782cd to 24ab248 Compare July 27, 2021 14:32

kinke marked this pull request as ready for review July 27, 2021 18:39

kinke requested review from andralex, MartinNowak, schveiguy, WalterBright and wilzbach as code owners July 27, 2021 18:40

kinke commented Jul 27, 2021

View reviewed changes

kinke added 3 commits July 27, 2021 21:14

[temp] Cirrus CI: Use same-named dmd branch

92e5c1b

Avoid deadlock with PRINTF GC debugging

7e760cd

kinke and others added 2 commits July 27, 2021 21:14

Re-add pure to toString() of TypeInfo subclasses wherever possible

68c5f99

This is especially useful to make `typeid(<class ref>).toString()` pure again (e.g., used by unit-threaded's `shouldThrow`).

Add changelog entry

9a30e97

kinke force-pushed the typeinfostruct_mangledName branch from 24ab248 to 9a30e97 Compare July 27, 2021 19:16

Azure Pipelines: Try to use same-named DMD branch for PRs originating…

04e0258

… from the official dlang repo

Cirrus CI: Try to use same-named DMD/Phobos branches for PRs originat…

cd591f4

…ing from the official dlang repo

RazvanN7 approved these changes Jul 30, 2021

View reviewed changes

RazvanN7 added the auto-merge label Jul 30, 2021

dlang-bot merged commit af04c94 into master Aug 1, 2021

kinke deleted the typeinfostruct_mangledName branch August 1, 2021 12:36

This was referenced Aug 1, 2021

Fix TypeInfo.opEquals for some templates #3138

Closed

Fix exceptions and dynamic casts across dll boundaries #2828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeInfo_Struct: Switch to stored mangled name & demangle lazily (with per-thread cache) #3527

TypeInfo_Struct: Switch to stored mangled name & demangle lazily (with per-thread cache) #3527

kinke commented Jul 26, 2021 •

edited

Loading

dlang-bot commented Jul 26, 2021

kinke commented Jul 26, 2021

kinke Jul 26, 2021

rainers commented Jul 27, 2021

PetarKirov commented Jul 27, 2021 •

edited

Loading

kinke commented Jul 27, 2021 •

edited

Loading

kinke commented Jul 27, 2021

kinke commented Jul 27, 2021

PetarKirov commented Jul 27, 2021

rainers commented Jul 27, 2021

kinke commented Jul 27, 2021 •

edited

Loading

kinke Jul 27, 2021

kinke commented Jul 28, 2021 •

edited

Loading

PetarKirov commented Jul 28, 2021

kinke commented Jul 29, 2021

RazvanN7 commented Jul 30, 2021

kinke commented Jul 30, 2021

RazvanN7 commented Jul 30, 2021

RazvanN7 commented Jul 30, 2021

kinke commented Jul 30, 2021

RazvanN7 commented Aug 1, 2021

kinke commented Aug 1, 2021

RazvanN7 commented Aug 1, 2021 •

edited

Loading

TypeInfo_Struct: Switch to stored mangled name & demangle lazily (with per-thread cache) #3527

TypeInfo_Struct: Switch to stored mangled name & demangle lazily (with per-thread cache) #3527

Conversation

kinke commented Jul 26, 2021 • edited Loading

dlang-bot commented Jul 26, 2021

Bugzilla references

Testing this PR locally

kinke commented Jul 26, 2021

kinke Jul 26, 2021

Choose a reason for hiding this comment

rainers commented Jul 27, 2021

PetarKirov commented Jul 27, 2021 • edited Loading

kinke commented Jul 27, 2021 • edited Loading

kinke commented Jul 27, 2021

kinke commented Jul 27, 2021

PetarKirov commented Jul 27, 2021

rainers commented Jul 27, 2021

kinke commented Jul 27, 2021 • edited Loading

kinke Jul 27, 2021

Choose a reason for hiding this comment

kinke commented Jul 28, 2021 • edited Loading

PetarKirov commented Jul 28, 2021

kinke commented Jul 29, 2021

RazvanN7 commented Jul 30, 2021

kinke commented Jul 30, 2021

RazvanN7 commented Jul 30, 2021

RazvanN7 commented Jul 30, 2021

kinke commented Jul 30, 2021

RazvanN7 commented Aug 1, 2021

kinke commented Aug 1, 2021

RazvanN7 commented Aug 1, 2021 • edited Loading

kinke commented Jul 26, 2021 •

edited

Loading

PetarKirov commented Jul 27, 2021 •

edited

Loading

kinke commented Jul 27, 2021 •

edited

Loading

kinke commented Jul 27, 2021 •

edited

Loading

kinke commented Jul 28, 2021 •

edited

Loading

RazvanN7 commented Aug 1, 2021 •

edited

Loading