Fix issue #17503 - Associative Arrays improperly register a GC-allocated TypeInfo for element cleanup #20863
Conversation
|
Thanks for your pull request, @rainers! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
|
Nice @rainers! I have been steeped in this for the last week, so I'll download and see if I can get the library side to work. It's just the compiler side I was unsure of. |
|
The type in After that, I got a compiler assert when building druntime unittests, and I'm not sure what to do there. |
|
That is one of these two asserts https://github.com/dlang/dmd/blob/master/compiler/src/dmd/typesem.d#L3323-L3326 |
|
I tested this on druntime, but still fails on phobos. The new entry doesn't seem to be generated for some reason. Digging deeper... |
rainers
force-pushed
the
rainer_aa_fak_entry_ti
branch
2 times, most recently
from
87b980f to
4c45158
Compare
The code failing is this: @safe unittest
{
struct S;
static assert( isAssociativeArray!(int[string]));
static assert( isAssociativeArray!(S[S]));
static assert(!isAssociativeArray!(string[]));
static assert(!isAssociativeArray!S);
static assert(!isAssociativeArray!(int[4]));
}Failing, because I think you may have to ask if the struct has a definition, and if not, just generate a null typeinfo. What does it generate for the key and value typeinfos? UPDATE: it's just putting garbage in there... Honestly, I think this unittest is bogus. |
|
Thanks for the hint. I guess it needs to better check for errors on the input types. I'm currently struggling with the druntime unittests which are missing a single symbol when linking. The trouble is that type infos are referenced from the backend, when no more semantic analysis must run, so they have to be generated up-front predicting what happens to the type later. That's similar to issues with RTInfo generation, reminding me of #2480, still open after almost 12 years ;) |
Wow, reading that makes me sad. We should have merged that by now... If I get a chance, I'll bring it up in the next DLF meeting. |
|
I don't know the way the compiler would do this, but is it possible to instantiate the struct on AA declaration, instead of when the typeinfo is needed? EDIT: or maybe that's already what you are doing? I can't tell. |
I haven't checked whether it's still valid since the last rebase 4 years ago, though. Obviously, it needs another rebase. |
In the pushed state, it is generated with the assoc array most of the time, but in my local version I have moved it to the type info generation (which usually happens immediately, too). My latest investigations show that the compiler is a bit confused what happens when adding const to the type and removing it later in the back end:
Unfortunately removing const is ambiguous, but that's what's happening in the backend and then requesting typeinfo for the latter. A workaround could be to genrerate that typeinfo early, too. I'll try that... |
thewilsonator
added
Druntime:AA
rainers
force-pushed
the
rainer_aa_fak_entry_ti
branch
from
1c9916b to
69c774b
Compare
|
I have opted to remove all modifiers from the key and value types, so we don't have to care how the glue layer modifies them. |
rainers
force-pushed
the
rainer_aa_fak_entry_ti
branch
3 times, most recently
from
7d1682a to
87a2b72
Compare
…llocated TypeInfo for element cleanup - let the compiler generate type info for the AA Entry and add it to TypeInfo_AssociativeArray - strip modifiers for index and next in TypeInfo_AssociativeArray to make it agnostic to changes by the glue layer
rainers
force-pushed
the
rainer_aa_fak_entry_ti
branch
from
87a2b72 to
7d6fff7
Compare
|
circleci says frontend.h out of date. Looks auto generated, not sure how it needs to be generated? |
I fixed that manually, but the failures on buildkite seem a bit more complicated. Let's hope they can be reproduced locally. |
|
For dstep, this is the line that is asserting: It's in the new code, so maybe there is an issue with where it is called (hinted by the comment "must not be called in the code generation phase")? There is an exception trace. |
|
Hurray, it passed all tests now! This might still be a bit brittle, as this is a bit similar to TypeInfo emission for structs which has a number of workarounds (e.g. semanticTypeInfoMembers() being called from the glue layer). It would be nice if someone with more knowledge of the current code emission/suppression strategies could have a look. BTW: I noticed that the Dlang-community/D-Scanner test on buildkite is cancelled after 20 minutes, but that happens with other PRs, too: |
There was a problem hiding this comment.
Hurray, it passed all tests now!
Yay!
This might still be a bit brittle
Honestly, if we have a pass, I think we should merge rather than look for the perfect solution. I would anticipate that any future issues will provide more bugs to fix, but this should be par for the course!
One thing remains -- please add the test as identified in the bug report. You can copy from here: https://github.com/dlang/dmd/pull/20853/files#diff-598818d2bef54acd1fba7714831fb453393c558a57c1257f2cd7ea34655fb614
Approved so this can be merged once done.
Added, but the test will get interesting only if someone adds back a runtime allocated type info. |
Oops, missed that. Now updated. |
I guess it works fine with LDC's emission strategy (for non-classes: lazily whenever accessed from codegen, into every referencing object file), where |
|
OK, I'm going to pull the trigger. I'd rather have this in than bitrot, and it's a huge improvement. Things can't be hackier than they are now... The true path forward is the template-based hooks. |
👍
Maybe this is a small step in that direction with the entry struct now templated. |
let the compiler generate type info for the AA entry defined by TypeInfo_AssociativeArray.Entry(K, V) and add it to TypeInfo_AssociativeArray.
@schveiguy Here's my attempt for getting rid of the fake TypeInfo. I guess more logic regarding the dtor can be removed as it should all be there in the Entry TypeInfo.