Fix cycles in Phobos by schveiguy · Pull Request #4493 · dlang/phobos

schveiguy · 2016-06-28T19:03:02Z

Found while debugging #4467, the cycle detection code reverted to old broken behavior. See https://issues.dlang.org/show_bug.cgi?id=16211.

This is needed to pull the cycle detection code fix (dlang/druntime#1602).

The std.experimental.allocator offending static ctors can just be removed, because the referenced OSX issue has been fixed for a long time.

The std.encoding cycle is a little more convoluted -- I overtook std.internal.processinit and renamed it to phobosinit. I figured we can put general hacks like this in there. It would be nice to have a better way to fix this...

DmitryOlshansky · 2016-06-28T20:01:19Z

std/internal/phobosinit.d

+
+shared static this()
+{
+    import std.encoding;


Lots of folks would like to avoid pulling in std.encoding at all

I have to say, the way this is done in std.encoding is horrific. -- You register the encoding class name by string, and then it uses the runtime to look up the class info via search of that string. THEN it constructs it, and checks all the encodings that are supported (and then lets the GC collect it).

There MUST be a better way to do this...

Oh, let's not forget that register isn't thread safe, because the AA holding the encoding schemes is __gshared

Well, technically it doesn't matter for shared static this, since there is only one thread. But the fact that it's not protected at later times is bad

register is designed to be called by user code as well as Phobos, there's no guarantee that the user code will be in a shared static this

mathias-lang-sociomantic · 2016-06-28T20:20:22Z

We might as well put stdiobase in it (or at least move it outside of sight).
Though that looks like a clear hole in the module system.
Is there something that would prevent detecting such cyclic dependency at CT ?

schveiguy · 2016-06-28T20:25:39Z

Is there something that would prevent detecting such cyclic dependency at CT ?

Yes, the modules being assembled are not known until the linker is done :)

schveiguy · 2016-06-28T20:26:05Z

We might as well put stdiobase in it (or at least move it outside of sight).

Probably a good idea, but should be done separately (after this).

JackStouffer · 2016-06-29T14:17:59Z

std/encoding.d

 {
+    /* // moved to std.internal.phobosinit
    shared static this()
    {


Why not just delete these

I could if that's the consensus. The issue is that these really should go here. The example in the base class says to do it this way (and it will be fine for external libs to do that)

JackStouffer · 2016-06-30T14:28:40Z

I honestly can't think of a better way to fix this, so I guess LGTM, as this is better than what we have now.

schveiguy · 2016-07-05T14:09:12Z

Anyone else? I need this pulled in order to fix the cycle detection algorithm.

Hackerpilot · 2016-07-05T21:03:22Z

✅

dnadlinger · 2016-07-05T22:28:11Z

Auto-merge toggled on

dnadlinger · 2016-07-05T22:28:37Z

Ugly, but necessary, it seems.

schveiguy · 2016-07-05T22:49:09Z

Thanks! Yeah, it's ugly. The best fix would be to be able to mark somehow that certain static ctors are trusted to be independent, but that requires dmd fixes (and a language change).

JackStouffer · 2016-07-12T18:15:38Z

@schveiguy This PR is causing test failures on my local machine, a 2015 Macbook Pro i5

$ make -j16 -f posix.mak BUILD=debug std/encoding.test

[snip]

std.encoding.EncodingException@std/encoding.d(2308): Unrecognized Encoding: utf-16le
----------------
4   dmd_run2WRwPr                       0x000000010bc67199 std.encoding.EncodingScheme std.encoding.EncodingScheme.create(immutable(char)[]) + 169
5   dmd_run2WRwPr                       0x000000010bc69db5 void std.encoding.__unittestL3150_17() + 37
6   dmd_run2WRwPr                       0x000000010bc5fde7 void std.encoding.__modtest() + 83
7   dmd_run2WRwPr                       0x000000010bcaeda4 int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) + 44
8   dmd_run2WRwPr                       0x000000010bca66ac int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*)) + 40
9   dmd_run2WRwPr                       0x000000010bccbd20 int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_osx_x86_64.SectionGroup) + 112
10  dmd_run2WRwPr                       0x000000010bccc645 int rt.sections_osx_x86_64.SectionGroup.opApply(scope int delegate(ref rt.sections_osx_x86_64.SectionGroup)) + 37
11  dmd_run2WRwPr                       0x000000010bccbc99 int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) + 33
12  dmd_run2WRwPr                       0x000000010bca667d int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) + 33
13  dmd_run2WRwPr                       0x000000010bcaec96 runModuleUnitTests + 126
14  dmd_run2WRwPr                       0x000000010bcc6011 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() + 29
15  dmd_run2WRwPr                       0x000000010bcc5f97 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) + 51
16  dmd_run2WRwPr                       0x000000010bcc5f03 _d_run_main + 831
17  dmd_run2WRwPr                       0x000000010bc6022d main + 33
18  dmd_run2WRwPr                       0x000000010bc5fd33 start + 51
19  ???                                 0x0000000000000000 0x0 + 0

Going to the commit before it

$ git checkout e41b83f0a37fa88402e92df3ceee73fecb57a1c4
$ make -j16 -f posix.mak BUILD=debug std/encoding.test
$

schveiguy · 2016-07-12T18:22:13Z

@JackStouffer I would try a make clean. You may have stale objects. It definitely works on Mac as autotester (and my system as well) shows.

JackStouffer · 2016-07-12T18:39:59Z

I did

$ make -j16 -f posix.mak clean

I still got

$ make -j16 -f posix.mak BUILD=debug std/encoding.test
std.encoding.EncodingException@std/encoding.d(2308): Unrecognized Encoding: utf-16le
----------------
4   dmd_run865fcv                       0x000000010dffbd29 std.encoding.EncodingScheme std.encoding.EncodingScheme.create(immutable(char)[]) + 169
5   dmd_run865fcv                       0x000000010dffe945 void std.encoding.__unittestL3150_17() + 37
6   dmd_run865fcv                       0x000000010dff4977 void std.encoding.__modtest() + 83
7   dmd_run865fcv                       0x000000010e03b1a4 int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) + 44
8   dmd_run865fcv                       0x000000010e032aac int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*)) + 40
9   dmd_run865fcv                       0x000000010e058120 int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_osx_x86_64.SectionGroup) + 112
10  dmd_run865fcv                       0x000000010e058a45 int rt.sections_osx_x86_64.SectionGroup.opApply(scope int delegate(ref rt.sections_osx_x86_64.SectionGroup)) + 37
11  dmd_run865fcv                       0x000000010e058099 int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) + 33
12  dmd_run865fcv                       0x000000010e032a7d int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) + 33
13  dmd_run865fcv                       0x000000010e03b096 runModuleUnitTests + 126
14  dmd_run865fcv                       0x000000010e052411 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() + 29
15  dmd_run865fcv                       0x000000010e052397 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) + 51
16  dmd_run865fcv                       0x000000010e052303 _d_run_main + 831
17  dmd_run865fcv                       0x000000010dff4dbd main + 33
18  dmd_run865fcv                       0x000000010dff48c3 start + 51
19  ???                                 0x0000000000000000 0x0 + 0

mathias-lang-sociomantic · 2016-07-12T18:47:03Z

Can you try a fresh clone ?

schveiguy · 2016-07-12T19:01:09Z

@JackStouffer Can you try just making the entire thing instead of that one test? e.g. make -f posix.mak unittest

wilzbach · 2016-07-12T19:01:25Z

I would try a make clean. You may have stale objects. It definitely works on Mac as autotester (and my system as well) shows.

I observed the same (and more) errors on Travis at #4587 :/

schveiguy · 2016-07-12T19:05:54Z

It seems as if the static ctor in phobosinit isn't running. I feel like it may be attributable to the way you are building it (you are building one test, but IIRC, the tests are normally all built into one exe, and then run individually, or maybe the test runner does the individual runs). In any case, I've never built the tests that way.

This may be a bug in the way the compiler does module info when you build individual modules.

JackStouffer · 2016-07-12T19:12:47Z

Can you try just making the entire thing instead of that one test? e.g. make -f posix.mak unittest

That passed.

wilzbach · 2016-07-18T17:53:14Z

This may be a bug in the way the compiler does module info when you build individual modules.

I had to workaround it for #4587 and thus reported it as a bug.

https://issues.dlang.org/show_bug.cgi?id=16291

As said it can be easily reproduced on any Linux machine by either testing std.encoding or std.net.curl in an individual test:

make -f posix.mak std/encoding.test

or

make -f posix.mak std/net/curl.test

MartinNowak · 2016-10-01T19:33:52Z

How is this supposed to work? Nobody is referencing (importing) phobosinit at all, so the constructors will simply never run.

schveiguy · 2016-10-02T02:40:56Z

@MartinNowak yes, this was my mistake. I'm trying to fix it, but running into other issues in ~~#4743~~ er... #4744

MartinNowak · 2016-10-03T15:23:32Z

And another regression caused by the fact that phobosinit is never run.
Issue 16580 – [REG 2.072.0-b1] spawnShell segfaults on macOS.

dnadlinger · 2016-10-03T19:22:04Z

Sorry for the mess, I should have noticed that the module is never used before merging the PR.

- partially Revert "Merge pull request dlang#4493 from schveiguy/fixcycles2" - recreate processinit (and import it from std.process) to call std.process shared ctor w/o creating a cycle - keep it separate from phobosinit to not drag std.encoding into any binary using std.process

MartinNowak · 2016-10-04T22:58:29Z

See #4839 and #4840.

Fix cycles in Phobos

df16848

schveiguy mentioned this pull request Jun 28, 2016

Fixed std.algorithm imports in std.format #4467

Merged

DmitryOlshansky reviewed Jun 28, 2016
View reviewed changes

schveiguy mentioned this pull request Jun 28, 2016

Fix issue 16211 - Cyclic dependencies broken again dlang/druntime#1602

Merged

JackStouffer reviewed Jun 29, 2016
View reviewed changes

dnadlinger merged commit c33f386 into dlang:master Jul 5, 2016

aG0aep6G mentioned this pull request Jul 6, 2016

Fix issue 4509 - XML parser in std.xml throws TagException if the attr value is put in apostrophes. #4567

Merged

schveiguy deleted the fixcycles2 branch July 6, 2016 11:43

wilzbach mentioned this pull request Jul 18, 2016

show code coverage with Codecov.io #4587

Merged

wilzbach mentioned this pull request Aug 7, 2016

Switch to CircleCi #4719

Merged

schveiguy mentioned this pull request Aug 23, 2016

Fix issue 16291 -- phobosinit fails to register encodings on individual tests #4743

Closed

MartinNowak mentioned this pull request Oct 4, 2016

fix Issue 16580 - spawnShell segfaults on macOS #4839

Merged

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

DmitryOlshansky Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

schveiguy Jun 28, 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!

JackStouffer Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

schveiguy Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

JackStouffer Jun 29, 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!

mathias-lang-sociomantic commented Jun 28, 2016

Uh oh!

schveiguy commented Jun 28, 2016

Uh oh!

schveiguy commented Jun 28, 2016

Uh oh!

JackStouffer Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

schveiguy Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jun 30, 2016

Uh oh!

schveiguy commented Jul 5, 2016

Uh oh!

Hackerpilot commented Jul 5, 2016

Uh oh!

dnadlinger commented Jul 5, 2016

Uh oh!

dnadlinger commented Jul 5, 2016

Uh oh!

schveiguy commented Jul 5, 2016

Uh oh!

JackStouffer commented Jul 12, 2016

Uh oh!

schveiguy commented Jul 12, 2016

Uh oh!

JackStouffer commented Jul 12, 2016

Uh oh!

mathias-lang-sociomantic commented Jul 12, 2016

Uh oh!

schveiguy commented Jul 12, 2016

Uh oh!

wilzbach commented Jul 12, 2016

Uh oh!

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

Uh oh!

Uh oh!

JackStouffer commented Jul 12, 2016

Uh oh!

wilzbach commented Jul 18, 2016

Uh oh!

MartinNowak commented Oct 1, 2016

Uh oh!

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

Uh oh!

Uh oh!

MartinNowak commented Oct 3, 2016

Uh oh!

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

Uh oh!

Uh oh!

MartinNowak commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

schveiguy commented Jun 28, 2016 •

edited

Loading

schveiguy Jun 28, 2016 •

edited

Loading

JackStouffer Jun 29, 2016 •

edited

Loading

schveiguy commented Jul 12, 2016 •

edited

Loading

schveiguy commented Oct 2, 2016 •

edited

Loading

dnadlinger commented Oct 3, 2016 •

edited

Loading