-
-
Notifications
You must be signed in to change notification settings - Fork 667
fix issues #10442: no or incomplete RTInfo #2480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
It seems this pull request has stalled the Win32 tester. Can someone kill the process? Is it terminated automatically after some timeout? |
|
Now this passes the tests, anyone care to comment if this going in the right direction? |
src/typinf.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of copy/pasta common code here. Shouldn't inheritance be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more or less the same duplication as in the toDt methods as it extracts their referenced TypeInfo. Both semantic3 and toDt might use common implementation for groups of classes with the referenced type in some overridable method but it could also complicate the codee and add boilerplate.
|
To make this work with recent changes I had to add avoiding to generate code for template instances that failed to instantiate during __traits(compiles). I know the method name "isInErrorTree" is horrible, but I could not find a better one. |
|
Fixed, now contains a fix for https://d.puremagic.com/issues/show_bug.cgi?id=11427, but probably #2757 is a more complete fix for it. |
|
The failure is caused by abuse of https://d.puremagic.com/issues/show_bug.cgi?id=11645 |
|
fixed conflicts and rebased. I had to disable semantic analysis of unittests on non-root modules to not expect type infos generated in a library which is not compiled with unittests enabled. |
|
Needs rebase |
|
now rebased. |
src/class.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sc->module->isRoot()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original change predates the introduction of isRoot, will change.
|
@WalterBright Thanks for review. Unfortunately the SSD with my D stuff on it is broken and I'm still hoping for repair without data loss, so I cannot do a lot right now. Do you agree with the general design of eagerly creating the necessary type infos? As described in the summary, it might generate more type info records than absolutely necessary. |
|
Updated according to comments. |
test/fail_compilation/ice10016.d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how to fix this message. S produced an error, so it doesn't display as "S". Should it be removed?
|
@rainers thanks for the quick responses, but we shouldn't pull this until after 2.065 is out. |
|
@WalterBright: So what is the point of the release branch then? Holding back on all merges for an indefinite amount of time can be an incredible demotivator for people working on the compiler. |
|
Redone this PR. It now uses semanticTypeInfo from #3256 most of the time, though it must be called quite often. |
|
Thanks for your pull request, @rainers! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#2480" |
Thanks for the info, I was already wondering. But it didn't bother me too much ;-) |
|
Thanks for your pull request, @rainers! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#2480" |
|
Thanks for your pull request, @rainers! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#2480" |
|
@dnadlinger, @WalterBright, @kinke, @Geod24 , @RazvanN7 Gentlemen, this request is pending your review/approval |
|
While this passes now (but for some styling and C++ header updates), let me point out a couple of thoughts:
I'm not a fan of the current design, though: eager generation of TypeInfo trying to predict what the glue layer might do is error-prone and inefficient.
|
|
@dnadlinger, @WalterBright, @kinke, @Geod24 , @RazvanN7 Gentlemen, this request needs your attention, review and approval |
|
Omg, is this from 2013? 😱 |
MoonlightSentinel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general notes/questions:
- why does this PR add more eager
TypeInfogeneration (e.g.semanticTypeInfoindcast.d) as well as more lazy generation (e.g.verifyTypeInfoinexpressionsem.d). - refactorings should be done as separate PR's
- this needs a lot more tests coverage (see CodeCov)
| * Returns: | ||
| * true if struct is all zeroes | ||
| */ | ||
| final bool isZeroInit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this function alongside TypeStruct.isZeroInit?
| }; | ||
|
|
||
| FuncDeclaration *search_toString(StructDeclaration *sd); | ||
| enum ZeroInit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum class can now be used
| } | ||
| } | ||
|
|
||
| final void generateRTInfo(AggregateDeclaration ad, Scope* sc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DDoc comment
|
@rainers What needs fixing before we can merge? ☀️ |
|
A decade has passed |
|
@rainers is this still a valid issue? I noticed on the bug report, you said you cannot reproduce any more. |
|
@schveiguy I tried the test case from this PR a couple of days ago, and it still failed. I haven't investigated further but I suspect most of the changes here are obsolete as less typeinfo is required now, so semanticTypeInfo() calls could even be removed. Even if I can figure this out, a new PR will probably simpler... |
|
Alright, let's close it to get it off the todo list. |
build TypeInfo in semantic, not during code-gen. This fixes RTInfo generation used by the precise GC implementation.
http://d.puremagic.com/issues/show_bug.cgi?id=10786
happens if the TypeInfo is referred to during the semantic3 pass, but is added to a module that has already completed the semantic3 step. This is fixable using the new deferred-semantic3 list. But the fixed version often triggers the next bug:
http://d.puremagic.com/issues/show_bug.cgi?id=10442
The problem is that the TypeInfo records are generated only from the glue or backend layer (e2ir.c or toobj.c) by calling Type::getTypeInfo(NULL), which outputs the TypeInfo without further analysis to the object file.
This ignores the RTInfo definition and just writes 0/1 depending on whether there are pointer fields in the struct/class or not.
Assuming that it is a bug to call Type::getTypeInfo(NULL) after semantic3 is finished, this patch forces the creation of the TypeInfo records during semantic for those expressions that later need them in the glue layer. A
problem with that approach is that it adds the TypeInfo of imported code into the object file (as COMDAT) if it is used in CTFE or type inference (similar to template functions). This makes http://d.puremagic.com/issues/show_bug.cgi?id=10831 show up more often.
Another problem is that the type of expressions is often changed later by implicite/explicite
conversions. This makes it hard to actually find those changes and generate TypeInfo
records for the new types aswell, leaving the original records unused.
I've made calls to getTypeInfo(NULL) that would trigger the creation of new TypeInfo records an ICE, but we might want to change the ICE to a warning to avoid breaking code that does not rely on proper TypeInfo.