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

Compile Phobos with -dip1000 #5915

Closed
wants to merge 1 commit into from
Closed

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Dec 9, 2017

AFAICT we have finally reached the point.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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.

@wilzbach
Copy link
Member Author

wilzbach commented Dec 9, 2017

I should have written more description about this PR. This only enables building Phobos with -dip1000, the testsuite still throws errors (one step at a time).
However, it looks like building with -dip1000 changes the mangling (?) and thus leads to an error while building d_do_test during the dmd build stage:

generated/d_do_test.o: In function `_D3std3uni__T8CowArrayTSQwQu8GcPolicyZQz__T6__ctorTAkZQlMFNaNbNcNfQpZSQCqQCp__TQCoTQCiZQCw':
d_do_test.d:(.text._D3std3uni__T8CowArrayTSQwQu8GcPolicyZQz__T6__ctorTAkZQlMFNaNbNcNfQpZSQCqQCp__TQCoTQCiZQCw[_D3std3uni__T8CowArrayTSQwQu8GcPolicyZQz__T6__ctorTAkZQlMFNaNbNcNfQpZSQCqQCp__TQCoTQCiZQCw]+0x60): undefined reference to `_D3std9algorithm8mutation__T4copyTAkTQdZQmFNaNbNiNfQrQtZQw'
generated/d_do_test.o: In function `_D3std5regex8internal6parser__T6ParserTAyaTSQBqQBpQBmQBg7CodeGenZQBi11parseEscapeMFNeZv':
d_do_test.d:(.text._D3std5regex8internal6parser__T6ParserTAyaTSQBqQBpQBmQBg7CodeGenZQBi11parseEscapeMFNeZv[_D3std5regex8internal6parser__T6ParserTAyaTSQBqQBpQBmQBg7CodeGenZQBi11parseEscapeMFNeZv]+0x7d5): undefined reference to `_D3std3uni__T5StackTkZQj3topMFNaNbNcNdNiNfZk'

So I guess we can't go one step at a time?

@CyberShadow
Copy link
Member

Huh, isn't that bad? Doesn't this mean that if we start building Phobos with -dip1000, all D users would then be forced to start using -dip1000 too?

@nordlow
Copy link
Contributor

nordlow commented Dec 15, 2017

What about making -dip25 default aswell? Does -dip1000 include -dip25?

@schveiguy
Copy link
Member

The mangling has to be identical, or we can't do a dip1000 incremental change. Is it possible to change the mangling, but not enforce the semantics for dip1000?

@dnadlinger
Copy link
Member

dnadlinger commented Dec 15, 2017

Doesn't this mean that if we start building Phobos with -dip1000, all D users would then be forced to start using -dip1000 too?

It does mean exactly that; I've raised this a number of months ago. It is especially fun once attribute inference gets involved.

@schveiguy
Copy link
Member

It is especially fun once attribute inference gets involved

ugh. This is going to cause a lot of problems. We have 3 options I think at this point:

  1. Leave the name mangling changes out, and re-introduce them once -dip1000 becomes permanent
  2. Somehow detect the attributes for mangling purposes only, and only enforce the semantics of dip1000 when the switch is given.
  3. Ship multiple versions of libraries.

Option 3 seems the easiest, but also the worst user experience.

@PetarKirov
Copy link
Member

PetarKirov commented Dec 15, 2017

@klickverbot what problems do you anticipate? I don't see problem with e.g. compiling phobos with -dip1000 when running the tests, while shipping the final library built without -dip1000.

Ship multiple versions of libraries.

@schveiguy's suggestion is also a workable one.

@PetarKirov
Copy link
Member

@JackStouffer
Copy link
Member

Whatever approach we decide to take towards DIP1000, simply throwing the switch seems like the wrong option. Closing to free up the PR queue; please reopen if you want to continue the discussion.

@wilzbach
Copy link
Member Author

We could have at least kept the existing -dip1000 fixes. Rebased to that. Though I would like to wait until #6278 has been merged, s.t. we can ensure that we actually make progress.

@wilzbach wilzbach changed the title Compile Phobos with -dip1000 Fix std.utf for -dip1000 Mar 15, 2018

static if (!hasIndirections!R)
@property auto save() scope
/* `return scope` cannot be inferred because compiler does not
Copy link
Member

Choose a reason for hiding this comment

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

change comment to remove mention of return

@wilzbach
Copy link
Member Author

Closing this and submitting this as a new approach with -dip1000.mak

@wilzbach wilzbach closed this Mar 28, 2018
@wilzbach wilzbach changed the title Fix std.utf for -dip1000 Compile Phobos with -dip1000 Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants