Skip to content

Fix CountLeadingZeroes on MSVC#3028

Merged
kripken merged 1 commit into
masterfrom
winfix
Aug 6, 2020
Merged

Fix CountLeadingZeroes on MSVC#3028
kripken merged 1 commit into
masterfrom
winfix

Conversation

@kripken

@kripken kripken commented Aug 6, 2020

Copy link
Copy Markdown
Member

We just had the logic there wrong - MSVC's intrinsic returns the bit
index, not the number of leading zeros. That's identical when scanning
forward but not in reverse...

This fixes one set of windows (or rather MSVC) specific errors on CI,
which I verified on CI. (But other issues remain after this, so we can't
turn on windows CI yet.)

This hopefully will fix #2942 , cc @RReverser

@RReverser

Copy link
Copy Markdown
Member

Ohh fingers crossed, but from a glance seems relevant.

Comment thread src/support/bits.cpp
// BitScanReverse gives the bit position (0 for the LSB, then 1, etc.) of the
// first bit that is 1, when looking from the MSB. To count leading zeros, we
// need to adjust that.
return 31 - int(count);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, good catch!

@RReverser

Copy link
Copy Markdown
Member

Just tried out using the binary from CI artifacts, and it does fix the issue!
Is it worth adding a regression test or do you think something so small won't get repeated?

@kripken

kripken commented Aug 6, 2020

Copy link
Copy Markdown
Member Author

It's already noticed by testing, so no need to add a new test I think. The problem is we can't enable windows tests on CI as several other failures exist. One option might be to try to enable at least some tests as they pass.

@RReverser

RReverser commented Aug 6, 2020

Copy link
Copy Markdown
Member

Oh, I see, I thought it was something not covered by tests. Are those generic failures? How hard would it be to fix them?

Either way, yeah, at least enabling some tests could be better than nothing. (not in scope of this PR though)

@kripken

kripken commented Aug 6, 2020

Copy link
Copy Markdown
Member Author

There is at least #2781, an issue with translate-to-fuzz (the output is not as expected - perhaps order of operations issue in fuzzing.h, we've seen such issues before), and after that I saw there was at least one more issue that I didn't investigate.

Not sure how hard they are to fix. Complicating this is that I don't have a windows machine and can't seem to access the normal windows VM I've been using. I'll look into that, but meanwhile I debugged this particular issue using CI here. Would be good if someone with a windows machine could find some time for this.

@RReverser

RReverser commented Aug 6, 2020

Copy link
Copy Markdown
Member

Complicating this is that I don't have a windows machine and can't seem to access the normal windows VM I've been using.

I think a VM I mentioned earlier on the issue should be enough for such sort of debugging and it includes most of necessary tooling:

If you don't have access to a Windows machine, you can probably use free VMs from https://developer.microsoft.com/en-us/microsoft-edge/tools/vms https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/ for testing.

But, of course, it's up to you whether you want to try that or leave to someone else with "proper" Windows machine :)

@kripken kripken merged commit 449e7a4 into master Aug 6, 2020
@kripken kripken deleted the winfix branch August 6, 2020 23:15
@kripken

kripken commented Aug 6, 2020

Copy link
Copy Markdown
Member Author

Ah, I don't think I can run those windows VMs locally either... anyhow, I'm probably the worst person for windows-specific issues as I would need to figure out basics like getting cmake and such, it's been a very long time since I did windows development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Misoptimization causing a runtime panic in Rust code

3 participants