Skip to content
This repository has been archived by the owner on Jun 20, 2019. It is now read-only.

Fix issue 231 (mixin template emitted into multiple object files) #245

Merged
merged 2 commits into from
Oct 1, 2016

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented Sep 26, 2016

See https://bugzilla.gdcproject.org/show_bug.cgi?id=231

The main problem is that our FuncDeclaration::toThunkSymbol function calls toObjFile whereas DMD doesn't call toObjFile here. I tried modifying the thunk code to work without this toObjFile call, but I couldn't get this into a working state.

So because of this toObjFile call we should not really emit functions in some cases (external functions, thunk in current module). We have to handle these cases in d_finish_function. The old code uses inNonRoot which does neither handle templates nor template mixins. It simply returns false for any function in a template mixin.

isInstantiated on the other hand explicitly checks for templates and does return null for template mixins. So isInstantiated is true for functions in normal templates only which can always be emitted (functions are marked weak anyway). For all other cases - including template mixins - getModule will get the correct root module.

I don't know why the old code explicitly called toParent2, but the new code should be equivalent (getModule traverses the parent dsymbols in the same way)

@jpf91 jpf91 changed the title 231 Fix issue 231 (mixin template emitted into multiple object files) Sep 26, 2016
@ibuclaw
Copy link
Member

ibuclaw commented Sep 26, 2016

The main problem is that our FuncDeclaration::toThunkSymbol function calls toObjFile whereas DMD doesn't call toObjFile here. I tried modifying the thunk code to work without this toObjFile call, but I couldn't get this into a working state.

If I recall correctly, it's to do with thunks to externally visible symbols. Without the function, no thunk gets emitted, and we instead get linker errors.

Ideally, there should not be a distinction between toSymbol and toObjFile, other than perhaps one keeps track of the IDENTIFIER_NODE (only generates the DECL_NAME) and which scopes (module, class, function) it binds to, and the other generates the declaration in its entirety.

I am currently sitting on #243 and thinking about the best way forward in terms of data structure and how we interact with it. I would prefer to generate the function bodies of all symbols that we can, using the information given to us by the frontend to determine what static/extern/public flags we set on them. The more referenced external functions we generate, the more inlining opportunities we give to gcc backend.

As opposed to our current design of "we are calling it's toObjFile, so we must emit it and ignore what the front-end says about the symbol. Oh wait! We were meant to do that, revert the flags that we set".

@jpf91
Copy link
Contributor Author

jpf91 commented Sep 27, 2016

I tried modifying the thunk code to work without this toObjFile call, ...

If I recall correctly, it's to do with thunks to externally visible symbols. Without the function, no thunk gets emitted, and we instead get linker errors.

I didn't even get to the linking part, some phobos modules always produced crashes in the GCC backend ;-)

I would prefer to generate the function bodies of all symbols that we can, using the information given to us by the frontend to determine what static/extern/public flags we set on them. The more referenced external functions we generate, the more inlining opportunities we give to gcc backend.

Sounds good. getModule and isRoot should provide enough information to decide whether we want to output the symbol/function, afaics.

See https://bugzilla.gdcproject.org/show_bug.cgi?id=231

The main problem is that our FuncDeclaration::toThunkSymbol function calls toObjFile whereas
DMD doesn't call toObjFile here. I tried modifying the thunk code to work without this
toObjFile call, but I couldn't get this into a working state.

So because of this toObjFile call we should not really emit functions in some cases (external
functions, thunk in current module). We have to handle these cases in d_finish_function.
The old code uses inNonRoot which does neither handle templates nor template mixins.
It simply returns false for any function in a template mixin.

isInstantiated on the other hand explicitly checks for templates and does return null for
template mixins. So isInstantiated is true for functions in normal templates only which can
always be emitted (functions are marked weak anyway). For all other cases - including template
mixins - getModule will get the correct root module.
@jpf91 jpf91 merged commit ef3141d into D-Programming-GDC:master Oct 1, 2016
@jpf91 jpf91 deleted the 231 branch October 1, 2016 09:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants