fix issue 18456 - crt_constructor/crt_destructor segfaults if -lib #9132
Conversation
|
Thanks for your pull request, @rainers! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + dmd#9132" |
|
I just noticed that even if no longer crashing, it still doesn't work as expected: the initializing reference is placed into a different object file inside the library, so never linked and the function is actually not called. I'll remove auto-merge and see if a fix can be added. AFAICT auto-merge doesn't work on stable anyway ATM? |
|
Yeah auto-merge requires all CI to pass, but I don't know why Appveyor is unable to build this PR. Looks like it didn't even try. |
rainers
force-pushed
the
issue18456
branch
4 times, most recently
from
3b6f58a to
69fa274
Compare
delay objmod.setModuleCtorDtor until the function is actually generated don't make them extra object files in a library, they will never be linked
|
Works now, but implementation and tests are very different. Some refactorings might be desirable:
I'd rather make these changes on master, though. |
| else if (auto f = s.isFuncDeclaration()) | ||
| { | ||
| objmod.setModuleCtorDtor(s.csym, isCtor); | ||
| f.isCrtCtorDtor |= isCtor ? 1 : 2; |
There was a problem hiding this comment.
Why the |= ? Is there ever a case where 1 and 2 can both be set? Or any other bits?
There was a problem hiding this comment.
I was a bit surprised, too, but the existing implementation allows both: https://github.com/dlang/dmd/blob/master/test/runnable/test17868b.d#L7
There was a problem hiding this comment.
The pragma just places a function pointer in the init/fini segment, so as a low-level tool it can be used as people want.
WalterBright
left a comment
WalterBright
left a comment
There was a problem hiding this comment.
Fix the |= then it's good to go.
thewilsonator
dismissed
WalterBright’s stale review
Apparently a function can be both a constructor and a destructor.
|
Adding 72h -> merge |
thewilsonator
added
Merge:72h no objection -> merge
| pd.error("can only apply to a single declaration"); | ||
| } | ||
|
|
||
| visit(cast(AttribDeclaration)pd); |
There was a problem hiding this comment.
Why did you move the call to the base class?
There was a problem hiding this comment.
The flag has to be set before visiting the function generation, because the pointers in init/fini are now written when visiting the function, which might be delayed with multiobj.
The problem was that they ended up in a different object file.
|
This PR had quite a bad timing during the beta for a low-level compiler change. |
It's been lingering here quite some time, but I doubt a lot of people have used the feature as it is still undocumented. |
8 participants
ensure s.csym initialized