Skip to content
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

[REG2.064] Issue 12144 - Unresolved xopEquals when referenced by dynamic array constructor #3256

Merged
merged 3 commits into from
Feb 16, 2014

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Feb 14, 2014

https://d.puremagic.com/issues/show_bug.cgi?id=12144

Run semantic3 for TypeInfo-related struct member functions.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 14, 2014

OK, ready to review. @MartinNowak how about this?

@MartinNowak
Copy link
Member

I think I like my solution better, but I'll have a closer look later.
#3255
If we continue to generate TypeInfo during obj generation, it's too late to run semantic3 on a struct, so generating struct TypeInfo eagerly seems like a more stable fix.

@rainers
Copy link
Member

rainers commented Feb 14, 2014

I agree we must not generate type info from the glue code, especially if we want to support RTInfo. I doubt you captured all the places where this happens. See how many places I had to patch to get complete(?) RTInfo:

#2480

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 15, 2014

I agree with that eagerly generating TypeInfo for all types is the most easiest way to fix the issue. But I'm also afraid that doing it will make compilation speed more slower. So I'd like to keep "on-demand" semantic for TypeInfo generation.

This is necessary and sufficient change to fix the regression issue.

@MartinNowak
Copy link
Member

Let's at least add an assertion to TypeInfoStructDeclaration::toObjFile which checks that either semantic pass 3 was run for the StructDeclaration or the struct was forward declared.

But I'm also afraid that doing it will make compilation speed more slower.

Why would it? You're outputting a few extra bytes that are most likely needed anyhow. If you generate that on-demand you might need to do it multiple times, have to rerun semantic3 and the linker has to deduplicate it. If someone is using a complex RTInfo it only gets worse.
So I think we should try to get away from vague linkage here, but currently the compiler relies on TypeInfo even when not linking against that struct #3255 (comment).

@MartinNowak
Copy link
Member

Auto-merge toggled on

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 15, 2014

Auto-merge toggled off

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 15, 2014

Sorry, please wait a while to merge this. Maybe I completely misunderstand the issue. I'd reconsider the issue.

@MartinNowak
Copy link
Member

The issue is fairly simple, sometimes we call TypeInfoStructDeclaration::toObjFile for StructDeclarations which only had semantic2 run. The emitted typeinfo refers to __xOpEquals and __xOpCmp, but semantic3 would have shown that those should be xerreq and xerrcmp.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 15, 2014

The issue is fairly simple, sometimes we call TypeInfoStructDeclaration::toObjFile for StructDeclarations which only had semantic2 run.

That's right! I didn't understand the issue until now!
And if I understand correctly, we already have a solution. The TypeInfo "codegen" should be done as same as template instances. If a struct is non-root symbol (≒ a template is instantiated only in non-root module), its TypeInfo toObjFile should be elided.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 15, 2014

Sign... I tried to implement my idea, but currently it would need more work.
9rnsr/dmd@master...fix12144x

So I reverted this PR state.

@MartinNowak
Copy link
Member

The TypeInfo "codegen" should be done as same as template instances.

That's roughly my conclusion too, but as Rainer mentioned there are a few problem with structs declared in Deimos, because they don't require linkage.
We could resort to some fallback TypeInfo for those, but it gets kind of complicated.
#3255 (comment)

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 16, 2014

I'll merge this PR so it is necessary for the next release.

9rnsr added a commit that referenced this pull request Feb 16, 2014
[REG2.064] Issue 12144 - Unresolved xopEquals when referenced by dynamic array constructor
@9rnsr 9rnsr merged commit 4f8919f into dlang:master Feb 16, 2014
9rnsr added a commit to 9rnsr/dmd that referenced this pull request Feb 16, 2014
[REG2.064] Issue 12144 - Unresolved xopEquals when referenced by dynamic array constructor
Conflicts:
	src/cast.c
	src/typinf.c
Additional fix:
	src/expression.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants