Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

fix Issue 18734 - bitnum parameter of core.bitop.bt should be signed #3141

Closed
wants to merge 1 commit into from

Conversation

WalterBright
Copy link
Member

The BT instructions take signed offsets, not unsigned ones.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
18734 normal bitnum parameter of core.bitop.bt should be signed

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 + druntime#3141"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jun 21, 2020
@WalterBright
Copy link
Member Author

Looks like dlang.druntime (Windows_OMF x86) is looking at an outdated import of core/bitop.d:

Error 42: Symbol Undefined __D4core5bitop2btFNaNbNiMxPkkZi

@rainers
Copy link
Member

rainers commented Jun 21, 2020

Looks like dlang.druntime (Windows_OMF x86) is looking at an outdated import of core/bitop.d:

The problem is that the new phobos.lib is copied into the test directory, and optlink uses this one if d_do_test is compiled by the host compiler.

@WalterBright
Copy link
Member Author

The problem is that the new phobos.lib is copied into the test directory, and optlink uses this one if d_do_test is compiled by the host compiler.

The import path and the library used should be the same. Otherwise, we can never update the library interface.

@rainers
Copy link
Member

rainers commented Jun 22, 2020

The import path and the library used should be the same.

Sure. This workaround https://github.com/dlang/dmd/blob/master/.azure-pipelines/windows.sh#L117-L119 probably was set up when the tools were still built with the new compiler.

Mixing builds with different compilers is not so easy with optlink because it picks up information from sc.ini in its containing folder. It probably needs to be copied into the generated folder and an appropriate sc.ini to be generated.

@kinke
Copy link
Contributor

kinke commented Jun 22, 2020

I fail to see a good reason why the druntime API should depend on the 'signature' of a particular x86 instruction, and why a bit index should all of a sudden be a signed ptrdiff_t.

@n8sh
Copy link
Member

n8sh commented Jun 25, 2020

I fail to see a good reason why the druntime API should depend on the 'signature' of a particular x86 instruction

Isn't the primary purpose of core.bitop to expose various instructions?

EDIT: That said, bt is used in three places in Phobos, all of which cease being correct if bitnum is interpreted as a signed number.

kinke added a commit to kinke/dmd that referenced this pull request Jun 26, 2020
Previously, the tools were compiled by the DMD host compiler, but linked
with freshly compiled druntime/Phobos, leading to inevitable issues
popping up in dlang/druntime#3141 and dlang/druntime#3142.
ghost pushed a commit to WalterBright/dmd that referenced this pull request Aug 12, 2020
Previously, the tools were compiled by the DMD host compiler, but linked
with freshly compiled druntime/Phobos, leading to inevitable issues
popping up in dlang/druntime#3141 and dlang/druntime#3142.
UplinkCoder pushed a commit to UplinkCoder/dmd that referenced this pull request Aug 12, 2020
Previously, the tools were compiled by the DMD host compiler, but linked
with freshly compiled druntime/Phobos, leading to inevitable issues
popping up in dlang/druntime#3141 and dlang/druntime#3142.
@RazvanN7
Copy link
Contributor

@WalterBright This change doesn't really seem to provide any benefit. Should we close this?

@dlang-bot dlang-bot removed the stalled label Aug 24, 2021
@RazvanN7 RazvanN7 closed this Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants