dmd
dmd copied to clipboard
dlang
fix issues #10442: no or incomplete RTInfo
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.
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? It is vital for these issues to be fixed to continue with the precise GC.
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. Maybe this is an optimization that makes sense in general.
@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.
@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.
Rebased again. Everybody considering embedding data into RTInfo, please review and comment.
A number of places where wrong TypeInfo was generated have already been identified and fixed by Kenji. This fixes some more.
I want to pull this shortly after 2.066 releases, that way it'll get a long beta cycle. Ping me if I forget.
I want to pull this shortly after 2.066 releases, that way it'll get a long beta cycle. Ping me if I forget.
Cool, will do.
2.066 is branched off. It's safe to pull this (assuming it's still good to go).
@WalterBright:
I want to pull this shortly after 2.066 releases, that way it'll get a long beta cycle. Ping me if I forget.
@9rnsr: I wonder if Kenji has some comment regarding https://github.com/D-Programming-Language/dmd/pull/2480/files#diff-ffa5582af7d723c487d4a11ac6743b85L93 as it removes a carefully crafted condition.
I'm really worried about negative effect against separate compilation.
Today, even if a type T is defined in non-root module (not directly compiled from command line), its TypeInfo object and RTInfo!T code are generated. I think it would be a not good behavior, because it looks contrary to the separate compilation model.
This PR will reinforce the (incorrect) compilation behavior. So currently I cannot accept this PR.
Instead of that, if a type T is in non-root module, compiler can just skip the RTInfo!T codegen. It is consistent with current template instantiation model.
@WalterBright I'm continuously maintained the new template instantiation model that you had implemented in #2550 from 2.064. But, current semantic analysis model for TypeInfo and RTInfo is not consistent with it. I'm planning to apply the #2550 model to TypeInfo and RTInfo, but this PR would prevent it. What do you think about the semantic model inconsistency?
Auto-merge toggled on
Thanks. At least, it started a discussion ;-)
@9rnsr I'm not sure how the desired semantic analysis model is supposed to be. Can I read about it somewhere?
The comment at the top of typinf.c tries to clarify where TypeInfo is currently generated to (maybe a comment about templates could be added). This PR doesn't change that but tries to ensure that correct TypeInfo is generated. If the backend calls getTypeInfo(NULL) and RTInfo is missing, semantic analysis cannot be run anymore. I guess you noticed this issues aswell when introducing semanticTypeInfo.
I agree that there are changes where calling semanticTypeInfo(sc) might be overly pessimistic, but it is hard to predict what code will be removed or converted later. It is just the same for almost any of the existing semanticTypeInfo calls.
IMO the correct approach would be to move all the code transformations done in the glue layer into the semantic pass, but that would be a huge change. I wonder what the GDC/LDC compiler maintainers think about such a change?
Instead of that, if a type T is in non-root module, compiler can just skip the RTInfo!T codegen. It is consistent with current template instantiation model.
TypeInfo is only generated if needed in some situations (e.g. zero-init struct - this allows di files with struct declarations never to be compiled).
For template classes/structs, there is no root-module.
The comment in typinf.c already clarify the inconsistency.
- type info of classes and interfaces are generated into the object file for the module that contains the class declaration
- type info for structs,enums and typedefs are treated as classes if the initializer is non-zero, otherwise they are COMDATs in every referring object file
Why type info generation for structs and enums are different from class and interfaces? I think all user-defined types TypeInfo generations should follow class/inferrfaces.
Why type info generation for structs and enums are different from class and interfaces? I think all user-defined types TypeInfo generations should follow class/inferrfaces.
Even though this breaks some uses of structs in non-compiled di files, I'd be fine with that change. It only makes sense to a chosen few why converting a byte-member to a char-member can break linkage now.
Would you then generate TypeInfo for all instantiated template types, too, even if never used?
Would you then generate TypeInfo for all instantiated template types, too, even if never used?
Today, if a template is explicitly instantiated in non-root module, compiler will skip its codegen. It's a new behavior from 2.064, introduced in #2550.
Today, if a template is explicitly instantiated in non-root module, compiler will skip its codegen. It's a new behavior from 2.064, introduced in #2550.
I guess there is a solution to cyclic imports with the same template instantiation being used in both modules. The existence of "-allinst" shows that there are some unresolved cases.
Will the suggested change make all the semanticTypeInfo calls obsolete? That would be great.
I think just removing the tests for "zeroInit" would make this PR fit in (though containing some code unnecessary in the future). Please note that it also contains fixes for
- calculating zeroInit lazily instead of assuming "false" before analysis
- avoiding code generation for template instantiations that failed, but the error was gagged by __trait(compiles) or similar
I guess there is a solution to cyclic imports with the same template instantiation being used in both modules. The existence of "-allinst" shows that there are some unresolved cases.
It's already handled. See TemplateInstance::needsCodegen().
avoiding code generation for template instantiations that failed, but the error was gagged by __trait(compiles) or similar
A template instance which contains any errors won't be emit in obj file. I think it's already handled in TemplateInstance::toObjFile (do nothing if isError(this)).
And in git-head, situation has been improved. If a template is successfully instantiated, but it appears only in speculative contexts (IsExpression, template constraint, and static assert condition), its codegen will be elided. I'm opened #3948 to finalize brushing up template instantiation and codegen algorithm.
Will the suggested change make all the semanticTypeInfo calls obsolete? That would be great.
I'd do it, but for that we need to remove inferred attributes from mangled names of instantiated functions.
Kenji, I think you're on the right track.
Typeinfo for all non-template types should only be generated when the module defining them is compiled (not merely imported).
For template types, Typeinfo should always be generated, unless that specific template instantiation is performed by an imported (not compiled) module.
This is the same model that code generation for templates follows. It enormously reduces bloat in the object files. The -allinst case is needed when a compiled module has different versions in effect than when it is imported, meaning different paths are taken through the imported module declarations. I suggest having -allinst apply to Typeinfo generation, too.