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

std.socket: replace static this with crt_constructor #8670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WalterBright
Copy link
Member

This is the only static this left in Phobos. Removing it makes for reducing need for ModuleInfo.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8670"

@Geod24
Copy link
Member

Geod24 commented Jan 26, 2023

What's the purpose here ?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 26, 2023

Seriously? No.

@WalterBright
Copy link
Member Author

As in core.cpuid, the reason is having a static this means that a ModuleInfo will need to be generated for every object file that imports it. Since the static constructor has no dependencies on any other module, by initializing it with the C library constructor will not produce such dependencies.

@WalterBright
Copy link
Member Author

It's just good engineering to eliminate unnecessary dependencies.

@WalterBright
Copy link
Member Author

ModuleInfo also gets in the way of using betterC, which does not support ModuleInfo.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 26, 2023

None of these are actually eliminating dependencies. Rather making it harder for people to use them without C.

@WalterBright
Copy link
Member Author

None of these are actually eliminating dependencies.

It eliminates the dependency on ModuleInfo and the dependency on druntime.

Rather making it harder for people to use them without C.

All pragma(crt_constructor) actually does is put a pointer to the function in a named section. Besides, std.socket relies on the existence of an operating system anyway, and that operating system will have the C standard library on board.

Copy link
Member

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do this:

  • It will possibly be using uninitialized GC, which can be catastrophic
  • It uses exceptions, which depends on runtime initialization anyway

As in core.cpuid, the reason is having a static this means that a ModuleInfo will need to be generated for every object file that imports it. Since the static constructor has no dependencies on any other module, by initializing it with the C library constructor will not produce such dependencies.

This is a clear reason to fix the ModuleInfo ABI. I remember @deadalnix telling me how broken ModuleInfo is on DMD at DConf. Why don't we implement order independent construction and add a priority tag to it? People already need to do shenanigans to make independent construction working and it possibly doesn't work on separate compilation module, due to how ModuleInfo is designed.

It's also an annoying real issue I have at production code with huge inter-module dependencies. Every time I include a module with a static this that also includes itself indirectly gets broken at runtime, on runtime initialization, and this is the biggest flaw of ModuleInfo.

We can surely implement something that doesn't need the requirement above mentioned by you, just like what C does. Also, ABI breakage isn't a reason to not change this, because the ABI is already broken.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 26, 2023

All pragma(crt_constructor) actually does is put a pointer to the function in a named section.

And so does shared static this.

Besides, std.socket relies on the existence of an operating system anyway, and that operating system will have the C standard library on board.

It also still relies on the existence of a D runtime, what with its use of new, exceptions, rtti, dynamic arrays, etc...

Anyway, this is not the way to do it.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #5470 for an example of eliminating static this.

@CyberShadow
Copy link
Member

Another approach to this:

@WalterBright
Copy link
Member Author

#5813

108 lines of changes, abandoned as too much work

#5470

Also by Andrei, also a lot of work. Yeah, one could do lazy initialization, though one would lose the advantages of immutable variables.

And so does shared static this.

Yes, but shared static this comes with a requirement for ModuleInfo

It also still relies on the existence of a D runtime, what with its use of new, exceptions, rtti, dynamic arrays, etc...

Yes, but does not require ModuleInfo

It will possibly be using uninitialized GC, which can be catastrophic. It uses exceptions, which depends on runtime initialization anyway

The constructor does indeed throw, good catch. This can be fixed, as the error is fatal anyway.

I remember @deadalnix telling me how broken ModuleInfo is on DMD

I wish people would include bugzilla references when making statements like this.

It's also an annoying real issue I have at production code with huge inter-module dependencies.

Ironically, that's a reason for this PR.

@WalterBright
Copy link
Member Author

Every time I include a module with a static this that also includes itself indirectly gets broken at runtime, on runtime initialization, and this is the biggest flaw of ModuleInfo.

Both modules have to have a static this to trigger an error. The error is:

object.Error@src/rt/minfo.d(364): Cyclic dependency between module constructors/destructors of test4x and test4

which is deliberate. With these cycles, how is the compiler to know which one gets run first? (At least D detects the error, with C one will arbitrarily get run first. If that isn't the right order, too bad.)

The solution is the user will have to specify which one comes first. The way to do that is to create a third module with just the static constructor in it that needs to run first, and have both modules import that module. That eliminates the dependency loop.

I don't know of any other way to do it that would be robust.

@WalterBright
Copy link
Member Author

BTW, this static this is the last static constructor left in Phobos. Wouldn't it be nice to have it gone, too?

@ljmf00
Copy link
Member

ljmf00 commented Jan 27, 2023

Every time I include a module with a static this that also includes itself indirectly gets broken at runtime, on runtime initialization, and this is the biggest flaw of ModuleInfo.

Both modules have to have a static this to trigger an error. The error is:

object.Error@src/rt/minfo.d(364): Cyclic dependency between module constructors/destructors of test4x and test4

which is deliberate. With these cycles, how is the compiler to know which one gets run first? (At least D detects the error, with C one will arbitrarily get run first. If that isn't the right order, too bad.)

Not both modules imported directly, but if they indirectly import one that has a static this, this happens. I understand the rationale of having this on the current implementation of ModuleInfo, but the thing is: we can provide alternatives, as already discussed in the forum for this problematic.

The solution is the user will have to specify which one comes first. The way to do that is to create a third module with just the static constructor in it that needs to run first, and have both modules import that module. That eliminates the dependency loop.

I don't know of any other way to do it that would be robust.

The creation of another module is the shenanigans I was talking about. And due to how ModuleInfo is designed, it potentially doesn't run. That works on one compiler invocation, but what if I compile that isolated module that doesn't get imported by anyone? Well, the linker eliminates those unused symbols.

@WalterBright
Copy link
Member Author

The creation of another module is the shenanigans I was talking about.

How can the compiler determine which one should go first? Isn't it a failure of proper encapsulation for modules that each import each other? (I think some languages don't even allow that.)

@FeepingCreature
Copy link
Contributor

FeepingCreature commented Jan 30, 2023

The creation of another module is the shenanigans I was talking about. And due to how ModuleInfo is designed, it potentially doesn't run. That works on one compiler invocation, but what if I compile that isolated module that doesn't get imported by anyone? Well, the linker eliminates those unused symbols.

If you compile an isolated module, ~all symbols are unused. If a public section is removed, that's a compiler bug.

(I think some languages don't even allow that.)

(Neat doesn't! (Because it can't compile it. :P))

Anyway, this is the sort of code that should prompt soul-searching about the language. I understand the motivation, but as we'd say at work: "merge it, but backlog a TODO task to refactor that design."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants