Fix issue 16291 -- phobosinit fails to register encodings on individual tests by schveiguy · Pull Request #4744 · dlang/phobos

schveiguy · 2016-08-23T20:09:21Z

std.encoding and std.process depend on an extern(C) binding to call their standalone static ctors. Doing it this way avoids cycles in the modules for unit tests.

However, my previous scheme of having std.internal.phobosinit import std.encoding allows the linker to simply discard that entire module since nothing depends on or calls it. It must be imported the other way, and must not import anything (to avoid cycles). I also had removed the import from std.process thinking it was unnecessary (incorrectly).

dlang-bot · 2016-08-23T20:09:28Z

Fix	Bugzilla	Description
✓	16291	phobosinit never gets called (EncodingScheme.create fails)

dlang-bot · 2016-08-23T20:09:36Z

@schveiguy, thanks for your PR! By analyzing the annotation information on this pull request, we identified @JackStouffer, @kyllingstad and @andralex to be potential reviewers. @JackStouffer: The PR was automatically assigned to you, please reassign it if you were identified mistakenly.

(The DLang Bot is under development. If you experience any issues, please open an issue at its repo.)

andralex · 2016-08-23T20:22:09Z

std/encoding.d

+// cycles.
+extern(C) void std_encoding_shared_static_this()
+{
+    foreach(scheme; [


Does this literal allocate?

I think it does. ~~Is it important for it not to?~~ I'll fix it back to a slew of calls. Would be nice to have a way to make an array at compile-time mid-expression for things like this.

schveiguy · 2016-08-23T20:30:15Z

Awesome. I've uncovered some sort of dmd heisenbug that works just fine on my system. @WalterBright how do I debug that signal 11 from here?

std.encoding and std.process.

codecov-io · 2016-08-23T20:35:27Z

Current coverage is 88.78% (diff: 100%)

Merging #4744 into master will decrease coverage by <.01%

@@             master      #4744   diff @@
==========================================
  Files           121        121          
  Lines         74159      74152     -7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          65845      65838     -7   
  Misses         8314       8314          
  Partials          0          0

Powered by Codecov. Last update 0538d0c...78d7c3b

schveiguy · 2016-08-23T20:47:06Z

I misread the call, I thought it was the build, but it's actually the checkwhitespace.d program. I can reproduce the crash locally.

andralex · 2016-08-23T20:51:29Z

Auto-merge toggled on

andralex · 2016-08-23T20:51:48Z

Whatever it is, I trust you'll get to the bottom of it.

schveiguy · 2016-08-23T20:58:37Z

HAHA I think somehow the linker/compiler is trimming out all the data in this file. If I insert this line, then it works:

auto ascii = new EncodingSchemeASCII;

That has to be a bug... I'm going to just put in the line above, to get this done. This whole file needs a rewrite anyway.

dlang-bot · 2016-08-23T21:09:30Z

@schveiguy, thanks for your PR! By analyzing the annotation information on this pull request, we identified @JackStouffer, @complexmath and @kyllingstad to be potential reviewers. @JackStouffer: The PR was automatically assigned to you, please reassign it if you were identified mistakenly.

(The DLang Bot is under development. If you experience any issues, please open an issue at its repo.)

schveiguy · 2016-08-23T21:25:08Z

Auto-merge toggled off

schveiguy · 2016-08-23T21:25:37Z

Something is really really wrong on OSX for this fix...

schveiguy · 2016-08-23T21:29:39Z

OK, I guess the failure is from the exe size growing too much. WTF...

schveiguy · 2016-08-23T21:33:43Z

I'm like 90% convinced that nobody uses std.encoding.EncodingScheme. I think none of the schemes were registered before this PR, and clearly the EXE size grows considerably when they are included.

JackStouffer · 2016-08-23T21:42:21Z

Dear christ, this is such a dirty hack. I would really rather not have code like this in Phobos; if I wasn't following the development of this, maintaining this would royally suck.

Maybe I'm overreacting, but if we can't get normal shared static this to work the way it's supposed to in the standard library then something is definitely wrong outside of the design of std.encoding.

schveiguy · 2016-08-23T21:52:51Z

@JackStouffer this is an old old module. At this point, changing it to be better designed is kind of a lost cause.

The hack for cycles is fine for now. That's not the issue with this PR. Until we can somehow mark a static ctor as being manually-verified as standalone, it's impossible to deal with the issue without doing something like this -- the runtime has to run static ctors in the right order, and the only way to do this is sort them at startup. The compiler doesn't give enough info to do a complete perfect dependency sort, so we are stuck with module-level sorting. When there are module cycles, you have to refactor like this.

The issue with this PR is that on OSX (at least), the module info isn't properly being stored and/or read. I'm setting up a bug report right now.

schveiguy · 2016-08-23T22:14:02Z

Bug report: https://issues.dlang.org/show_bug.cgi?id=16423

MartinNowak · 2016-10-01T19:51:29Z

This won't fully solve the problem (and all the trouble for some stupid false cycle detection).
Without a static ctor there no module info ends up in a binary using std.encoding.
Without a module info none of the classes are referenced (and thus no encoding scheme present, except for the ASCII one you referenced).
Because the stupid EncodingScheme.register just takes a string instead of a class alias, it'll fail (now silently), to register most of the encodings, b/c literally noone uses EncodingSchemeUTF8 in his code.

With the current situation std.net.curl becomes unusable (b/c it instantiates whatever Transfer-Encoding it sees). Please add a new EncodingScheme.register(alias klass) overload and properly register all the existing schemes to fix the regression, fix should address stable branch now.

MartinNowak · 2016-10-01T19:54:08Z

I'm like 90% convinced that nobody uses std.encoding.EncodingScheme. I think none of the schemes were registered before this PR, and clearly the EXE size grows considerably when they are included.

Wrong, anyone importing std.encoding (e.g. indirectly through std.net.curl), got all the encodings (and their binary sizes), and they are necessary for a working curl transfer decoding.

MartinNowak · 2016-10-01T19:55:41Z

std/process.d

 {
    version (OSX)
    {
+        import std.internal.phobosinit; // needed to make sure the function gets called


Please don't reference the encoding scheme init from std.process, merging all constructors in phobosinit is a really bad idea.

Yes, I think that's why default hello world goes up in space so much. Ugly as it is, we need an individual internal module for each module that has cycles.

I think this relates to https://issues.dlang.org/show_bug.cgi?id=16580 as well.

schveiguy · 2016-10-02T15:15:26Z

Without a static ctor there no module info ends up in a binary using std.encoding.

Why is that? I think moduleInfo should be present if classinfo is used (at the very least). Otherwise, runtime reflection initialization doesn't work (Object.factory).

I agree that std.encoding should use compile-time reflection for initialization.

MartinNowak · 2016-10-02T21:52:38Z

Why is that? I think moduleInfo should be present if classinfo is used (at the very least). Otherwise, runtime reflection initialization doesn't work (Object.factory).

We're running in circles dlang/dmd#6076 (comment), Object.factory does not work w/ static libraries and unreferenced classes. It's just by chance that static ctors drag in ModuleInfo which drag in all class references.
I don't see any reason why EncodingScheme.create/register uses Object.factory.
In fact given how slow Object.factory is, it's a really bad idea to rely on that for EncodingScheme.create.
So unless I'm overlooking a good reason, let's please deprecate this nonsense, fix the existing EncodingSchemes, and move on.

schveiguy · 2016-10-05T16:51:46Z

Fixed in #4840

schveiguy force-pushed the fixcycles5 branch from dea7e7c to 564befb Compare August 23, 2016 20:13

andralex reviewed Aug 23, 2016
View reviewed changes

Fix issue 16291. Make sure internal shared ctor is called from

369cd90

std.encoding and std.process.

schveiguy force-pushed the fixcycles5 branch from 564befb to 369cd90 Compare August 23, 2016 20:35

Make sure the classinfo of all the encoding schemes gets included!

78d7c3b

schveiguy added the Merge:Blocked label Aug 23, 2016

schveiguy mentioned this pull request Sep 8, 2016

Fix issue 16423 - ModuleInfo missing when linking to static lib with classes dlang/dmd#6076

Closed

MartinNowak added the Review:Needs Work label Oct 1, 2016

MartinNowak reviewed Oct 1, 2016

View reviewed changes

schveiguy mentioned this pull request Oct 2, 2016

Fix cycles in Phobos #4493

Merged

schveiguy closed this Oct 5, 2016

Uh oh!

Comments

Conversation

schveiguy commented Aug 23, 2016

Uh oh!

dlang-bot commented Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Aug 23, 2016

Uh oh!

andralex Aug 23, 2016

Choose a reason for hiding this comment

Uh oh!

schveiguy Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

codecov-io commented Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 88.78% (diff: 100%)

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

andralex commented Aug 23, 2016

Uh oh!

andralex commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

dlang-bot commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

JackStouffer commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

schveiguy commented Aug 23, 2016

Uh oh!

MartinNowak commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinNowak commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinNowak Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy Oct 2, 2016

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

schveiguy commented Oct 2, 2016

Uh oh!

MartinNowak commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schveiguy commented Oct 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dlang-bot commented Aug 23, 2016 •

edited

Loading

schveiguy Aug 23, 2016 •

edited

Loading

codecov-io commented Aug 23, 2016 •

edited

Loading

MartinNowak commented Oct 1, 2016 •

edited

Loading

MartinNowak commented Oct 1, 2016 •

edited

Loading

MartinNowak Oct 1, 2016 •

edited

Loading

MartinNowak commented Oct 2, 2016 •

edited

Loading