[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #27 from Steven Schveighoffer  ---
(In reply to Sobirari Muhomori from comment #26)
> So all this is because encoding is a part of an unknown module cycle?

It's a known module cycle (cycle is printed when it's found).

However, I think the cycle only happens when unit testing phobos. I created
another related issue to solve that problem:
https://issues.dlang.org/show_bug.cgi?id=16265

But I keep finding other bugs when trying to fix this. Which is, in a way, a
good thing.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #26 from Sobirari Muhomori  ---
So all this is because encoding is a part of an unknown module cycle?

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #25 from Ketmar Dark  ---
> I'm like 90% convinced that nobody uses std.encoding.EncodingScheme.
i am using it. i have my own iv.encoding (and some others), where alot of
schemes are registered. and alot of my code depends on std.encoding. as badly
designed as it is, it is still better than nothing, and this time i -- for some
unknown reason -- decided to go with phobos implementation insted of rolling my
own. ;-)

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #23 from Steven Schveighoffer  ---
New PR: https://github.com/dlang/phobos/pull/4743

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #24 from Steven Schveighoffer  ---
Where's that edit button...

Real new PR: https://github.com/dlang/phobos/pull/4744

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #22 from Steven Schveighoffer  ---
I created a PR, but it still has cycles, only on Windows
(https://github.com/dlang/phobos/pull/4743)

Looks like it doesn't actually fix it :) I will work on a more suitable PR.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #21 from Steven Schveighoffer  ---
I have a branch to fix this. I'm waiting for my cycle detection fix to be
pulled, because I don't want to re-introduce cycles to make that fail (in case
it's specific to an OS I don't test locally). Once that is pulled, I will
create a PR to revert the changes I made.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Johannes Pfau  changed:

   What|Removed |Added

 CC||johannesp...@gmail.com

--- Comment #20 from Johannes Pfau  ---
> as nothing directly referring to module ctors, linker just doesn't know that 
> it > should not optimize 'em away. maybe we can fix that by emiting some flag 
> for .o > files, but i don't really know -- i never even read about dwarf 
> object file
> format, so i'm not sure if such flag exists.

The constructors are referenced by the ModuleInfo. ModuleInfo is theoretically
unreferenced which will cause issues with --gc-sections, but as long as you
don't use --gc-sections that doesn't matter. (LDC has a workaround, IIRC)


The problem in this case is that the linker does not even attempt to link in
the object file though. The linker only looks at object files in a static
library if it still has some unresolved symbols. A solution is using the whole
archive linker option:
http://stackoverflow.com/questions/80/ld-linker-question-the-whole-archive-option

As you said this will result in bigger executables though.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #19 from Ketmar Dark  ---
>But maybe your idea of registering on demand is worth considering even with 
>that
>update.
if you'll decide that it worth keeping, then just drop a note in 16298, i can
rewrite the patch to use CT reflection instead. if you don't want to create
your own patch from scratch, of course. ;-)

>Sorry about the strife!
no problems. ;-)

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #18 from Steven Schveighoffer  ---
(In reply to Ketmar Dark from comment #17)
> p.s. if it works, please, close my ER in issue 16298 then.

Sure. I personally hate the way std.encoding registers all the encodings, so
I'd like to fix that anyway (but by using compile-time reflection, not strings
and object.factory). But maybe your idea of registering on demand is worth
considering even with that update.

Sorry about the strife! Cycles in D suck. Hopefully we can make the whole thing
more pleasant.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #17 from Ketmar Dark  ---
p.s. if it works, please, close my ER in issue 16298 then.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #16 from Ketmar Dark  ---
how did you dare to make my great patch unnecessary?! ;-))

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #15 from Steven Schveighoffer  ---
I'm going to have to take back what I said. I added back in the static ctors,
and there is no cycle, even with my patch that fixes cycle detection. It's
probably all the work on trimming out unnecessary imports that has helped this.

I'll "revert" what I did by moving the static ctors back into std.encoding.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Jack Stouffer  changed:

   What|Removed |Added

   Hardware|x86_64  |All
 OS|Linux   |All

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Jack Stouffer  changed:

   What|Removed |Added

 CC||j...@jackstouffer.com
   Severity|enhancement |regression

--- Comment #14 from Jack Stouffer  ---
Because this broke code I am raising this to a regression.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #13 from Ketmar Dark  ---
did it: https://issues.dlang.org/show_bug.cgi?id=16298

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #12 from Steven Schveighoffer  ---
Ketmar, thanks for the patch. I think you should open a new bug report with it
so it doesn't get lost. I have to fix the phobosinit code anyway, which will
close this.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #11 from Ketmar Dark  ---
p.s. we probably can enumerate all std.encoding module symbols with CTFE
reflection instead, but i don't think that it makes huge difference here.
paying small fee at program startup looks better for me than paying this fee
each time we compiling the code that imports std.encoding. while CTFE time is
small in this case, i prefer it this way.

but if you think that we should use compile-time reflection instead, feel free
to say that, and i'll rework the patch. ;-)

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #10 from Ketmar Dark  ---
Created attachment 1603
  --> https://issues.dlang.org/attachment.cgi?id=1603=edit
lazy standard codec registration

the attached patch seems to work ok with my system. it registers standard
codecs when user is first trying to use `EncodingScheme.create()`, and it also
exposes `EncodingScheme.registerStandardCodecs()` function, which can be called
to manually register standard codecs if it is necessary in some other module
ctor.

it is safe to call `EncodingScheme.registerStandardCodecs()` multiple times,
and from multiple threads.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #9 from Steven Schveighoffer  ---
(In reply to Ketmar Dark from comment #8)
> yes, i know. but std.encoding seems to not import anything that may
> indirectly import it back, and it *seems* to not break anything for my
> system.

That's because you don't have proper cycle detection on your system :) I
manually verified the cycle did exist. I don't remember the exact cycle
modules.

Note that there really aren't any cycles in terms of std.encoding requiring
some other module ctor to work correctly, but the runtime doesn't do a proper
job of sorting the order of the ctors, and when that is fixed, it can't solve
the ordering.

My fix that I pushed was not right -- the dependency is lost, and now may
result in incorrect ordering still.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #8 from Ketmar Dark  ---
>I should tell you that cycle detection is currently broken
yes, i know. but std.encoding seems to not import anything that may indirectly
import it back, and it *seems* to not break anything for my system.

i'm not insisting on that solution, though. maybe we can use some kind of "lazy
initialization" scheme instead, by writing a weird hack: on the first call to
`EncodingScheme.create()` we can find std.encoding in module list, and register
every encoder it has. i think this *may* work (if linker will not throw away
"unused classes"). i probably will try to write such code to see if it will
work.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #7 from Steven Schveighoffer  ---
(In reply to Ketmar Dark from comment #4)
> for now, easy fix is to move that thing back to "std.encoding", as it seems
> to not conflict with anything now. and probably create a new issue that
> documents this behavior.

This would bring back the cycle that it fixed. I should tell you that cycle
detection is currently broken, see issue 16211. I was getting cycle errors with
the fix for that issue until this fix was in place.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Steven Schveighoffer  changed:

   What|Removed |Added

   Assignee|nob...@puremagic.com|schvei...@yahoo.com

--- Comment #6 from Steven Schveighoffer  ---
Hm... I also realized just now that what I did is not correct. There's a chance
that some other static ctor depends on std.encoding registry having been set
up. But the link between encoding and phobosinit isn't there, so the ordering
will be wrong.

I'll fix this in a PR.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #5 from Ketmar Dark  ---
p.s. i think that vibe.d works 'cause it passes all it's files to dmd instead
of creating .a library. then dmd generating single object file with everything
necessary inside, including module ctors. but for .a libraries, dmd is writing
alot of object files (one per function, i believe) to allow smartlinking.

so passing all phobos .d files to dmd should work as expected, but using .a
isn't. and i believe that vibe.d will have the same issue if it will be
complied as .a library.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #4 from Ketmar Dark  ---
as nothing directly referring to module ctors, linker just doesn't know that it
should not optimize 'em away. maybe we can fix that by emiting some flag for .o
files, but i don't really know -- i never even read about dwarf object file
format, so i'm not sure if such flag exists.

that is, linker really does it's work here: if nothing directly using the
symbol from object file, that object file is not linked to the final binary.
tbh, i don't know how can we solve this: if we will mark all module ctors as
"include always" (if it's possible), we will have alot of unnecessary (and
probably unused) code linked to the final binary. it doesn't hurt much, but
still...

for now, easy fix is to move that thing back to "std.encoding", as it seems to
not conflict with anything now. and probably create a new issue that documents
this behavior.

maybe we can introduce some UDA to mark modules that should not be optimized
away by smartlinking, or something like that, 'cause i guess that similar
issues are very possible in the future, as phobos becomes more and more
complex, with complex interdependencies.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

--- Comment #3 from Steven Schveighoffer  ---
(In reply to Ketmar Dark from comment #2)
> aha. i see.
> 
> the thing is that std.internal.phobosinit is not imported from anywhere, so
> linker simply not including it in resulting executable!
> 

Well... That's weird.

I assumed module ctors would still be included, even if not imported. How the
hell does vibe.d work, where everything is set up in a module ctor?

> 
> i.e. we have to do "import std.internal.phobosinit;" in "std.encoding". but
> that defeats the purpose of moving the whole thing to separate module, i
> guess.

Yes, it does. I purposely removed all imports of phobosinit (was called
processinit, and std.process imported it) to avoid cycles.

> what was the idea behind moving that initialization to separate module? it
> seems that leaving it in std.encoding is not conflicting with anything: not
> one of phobos modules that imports "std.encoding" has module ctors.

There was a cycle between std.encoding and something else. The easiest thing to
do was to remove the constructors.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Ketmar Dark  changed:

   What|Removed |Added

 CC||ket...@ketmar.no-ip.org

--- Comment #1 from Ketmar Dark  ---
it seems that at least for x86 GNU/Linux ctor is called if i'm using
libphobos2.so instead of libphobos2.a. i inserted printf there, and yes: with
static phobos ctor is skipped, but with .so phobos it is called.

i found this not with test, but in my own app, which mysteriously stopped
working when i linked it with static phobos.

--

[Issue 16291] phobosinit fails to register encodings on individual tests

[via Digitalmars-d-bugs]

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

Steven Schveighoffer  changed:

   What|Removed |Added

 CC||schvei...@yahoo.com

--