[REG2.064a] Issue 10726 - Bogus Circular Reference error if opEquals defined and has a loop #2421
Conversation
| * directly compiled, the target TypeInfo symbol would already | ||
| * exist in separately compiled object file. | ||
| * So don't generate it again. | ||
| */ |
There was a problem hiding this comment.
I'm afraid this assumption is not true. If the declaration is in a .di file (like in deimos), the module itself is never compiled.
I have tried to fix http://d.puremagic.com/issues/show_bug.cgi?id=10442 by adding semantic3 to the TypeinfoDeclaration types, but I'm not sure yet whether that causes too much bloat: rainers@b1a8e1b
|
Superseded by #2444. |
|
This fix does not add any new semantic phase. Instead |
| * the TypeInfo object would be speculatively stored in each object | ||
| * files. To set correct function pointer, run semantic3 for sd->xeq. | ||
| */ | ||
| if (FuncDeclaration *xeq = sd->buildXopEquals3(NULL)) |
There was a problem hiding this comment.
I don't think it is good to run semantic3 at this stage of compilation as object file generation has already begun but the AST might get changed by semantics.
I also consider all calls of getTypeInfo(NULL) at this stage as bugs, but they seem ubiquitous.
There was a problem hiding this comment.
See latest change. Now semantic3 is not invoked in glue layer.
|
I updated this PR. |
| @@ -114,6 +117,10 @@ void AggregateDeclaration::semantic3(Scope *sc) | |||
| //printf("AggregateDeclaration::semantic3(%s)\n", toChars()); | |||
| if (members) | |||
| { | |||
| StructDeclaration *sd = isStructDeclaration(); | |||
| if (isImportedSym(sd)) | |||
| goto Lxop; | |||
There was a problem hiding this comment.
I don't think you can skip RTInfo generation for imported aggregates here, they also go into the TyeInfo.
There was a problem hiding this comment.
I know there would be another issue, but it is not directly related to bug 10726. So, I won't fix RTInfo issue in this PR.
There was a problem hiding this comment.
Wouldn't it be a regression on symbols from imported modules that never generate RTInfo after the change? But maybe they have never done that...
There was a problem hiding this comment.
No. Currently, just imported non-template struct does not instantiate RTinfo and store the code to the importing module obj file, because semantic3 is never run for those structs.
By this change, semantic3 will be invoked for some non-template structs even if it is just imported. To keep the current behavior, this goto is necessary.
Thanks, much better now. I'm currently trying to get rid of getTypeInfo() calls (mostly from the backend) that add new AST nodes that are never analyzed, or write incomplete info directly to the object file. Part of that is to add the TypeInfoDeclaration to deferredSemantic3 and add a semantic3 pass to it. Maybe this could also trigger buildXopEquals and buildXopCmp. On the other hand, if your patch also includes the RTInfo generation for all structs, it might solve the troubles with getTypeInfo(NULL) (probably not completely for associative arrays), but it could also cause code bloat. |
| @@ -701,6 +732,16 @@ void StructDeclaration::semantic(Scope *sc) | |||
|
|
|||
| buildOpAssign(sc2); | |||
| buildOpEquals(sc2); | |||
|
|
|||
| xeq = buildXopEquals(sc); | |||
| xcmp = buildXopCmp(sc); | |||
There was a problem hiding this comment.
I see it is identical to what was previously done in semantic2, but should this use sc2 instead of sc?
[Sorry if I'm asking dumb questions, but without having a complete overview of the implications, I just want to make sure that if something looks suspicious to me, it is not an oversight.]
There was a problem hiding this comment.
Ouch, that's my copy-paste mistake. Fixed.
…and has a loop This is also the better fix for issue 10567.
[REG2.064a] Issue 10726 - Bogus Circular Reference error if opEquals defined and has a loop
|
This pull request slowed down DMD unittest compilation dramatically. Compilation now takes 15% - 20% longer on all platforms. Phobos test time barely changed (~2%), so it is possible that most of the time increase happens only on particular tests. |
|
I don't know whether it was this pull, but timings for building a simple test file have exploded: module test;
import std.stdio;
void main() { }2.063.2: Git-head (1a6da16): I've reported this in d-internals (but the post awaits moderation approval). |
| scx->scopesym->members && !scx->scopesym->isTemplateMixin() | ||
| if (scx && scx->scopesym && scx->scopesym->members && | ||
| !scx->scopesym->isTemplateMixin() && | ||
| !scx->scopesym->isModule() |
There was a problem hiding this comment.
It seems this change has caused the reported performance regression.
Actually, the line below could have hinted in that direction.
There was a problem hiding this comment.
@9rnsr I've run the tests successfully without the isModule check, do you think it can be removed to avoid the performance issues?
There was a problem hiding this comment.
OK. I confirmed the issue, and opened #2496.
http://d.puremagic.com/issues/show_bug.cgi?id=10726
This is also the better fix for issue 10567.