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

Don't emit init symbol for zero-initialized structs #3131

Merged
merged 3 commits into from
Sep 11, 2019

Conversation

kinke
Copy link
Member

@kinke kinke commented Aug 11, 2019

No description provided.

gen/llvmhelpers.cpp Outdated Show resolved Hide resolved
@JohanEngelen
Copy link
Member

Last time I looked at this, I think we came to the conclusion that the symbol always needs to be emitted because of people referencing it, but becaue .init is not an lvalue I don't know how to create a reference to the init symbol (not whether that'd be invalid code or not)

@kinke
Copy link
Member Author

kinke commented Aug 12, 2019

Ah yeah, that rings a bell now, I completely forgot about it.

but becaue .init is not an lvalue I don't know how to create a reference to the init symbol

Me neither, it's not referenced by the TypeInfo either, so I can only think of referencing it manually by name (pragma mangle tricks etc.).

@kinke
Copy link
Member Author

kinke commented Sep 7, 2019

Do we want to risk this (replacing the assert by a fatal codegen error obviously) or should I take a step back, re-emitting the symbols and only avoiding their usage?

@JohanEngelen
Copy link
Member

I wish I could test this on Weka's codebase, but there is too much breakage with recent compilers to test this quickly.
I'll see if I can backport this and check linking. Will take me a week or so.

JohanEngelen added a commit to weka/ldc that referenced this pull request Sep 9, 2019
@kinke
Copy link
Member Author

kinke commented Sep 9, 2019

Okay, would be nice; I'll defer the beta then (but please let me know if you don't find any time).

@JohanEngelen
Copy link
Member

Done, testing showed no issues. Good to go from my side.

@kinke
Copy link
Member Author

kinke commented Sep 11, 2019

Thx mate.

@kinke kinke changed the title WIP: Don't emit init symbol for zero-initialized structs Don't emit init symbol for zero-initialized structs Sep 11, 2019
@kinke kinke merged commit 3840a03 into ldc-developers:master Sep 11, 2019
@kinke kinke deleted the zeroinit branch September 11, 2019 22:30
EyalIO pushed a commit to weka/ldc that referenced this pull request Jul 18, 2020
)

And optimize previous usages of it to direct memset-zero.
@EyalIO
Copy link

EyalIO commented Jul 18, 2020

Thanks, @kinke !

I just backported this fix to our compiler, to see its effect on executable size.
I've noticed we stil have very large init symbols for all-zero classes.
Is there a reason this isn't done for classes too?
The vtbl/etc inside classes isn't really copied from the "init" symbol but set by generated assignments, so all we use the "init" for is the fields (which is equivalent to memset).

Anyway, I'm trying my luck at changing classes to skip the init symbols too, and hoping I'm doing it correctly :)

@kinke
Copy link
Member Author

kinke commented Jul 18, 2020

Is there a reason this isn't done for classes too?
The vtbl/etc inside classes isn't really copied from the "init" symbol but set by generated assignments, so all we use the "init" for is the fields (which is equivalent to memset).

It'd be a violation or at least a big dangerous breaking change of the TypeInfo_Class.initializer protocol. After a quick glance, there's _d_newThrowable (-dip1008) which depends on it being complete (compiler doesn't re-init manually afterwards) and would currently simply segfault if the pointer was null, like presumably other places in druntime (and possibly Phobos too).
DtoInitClass() also just sets vptr and monitor manually; e.g., it does depend on interface vptrs being set in the init symbol, which can interleave with base class fields...

@EyalIO
Copy link

EyalIO commented Jul 18, 2020

Oh bummer :-(

Thanks for the input!

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