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

Logical cores iteration #1362

Merged
merged 3 commits into from Oct 15, 2020
Merged

Conversation

Jimmy-653
Copy link
Contributor

Basically copied from you comment.....
Seems kinda hacky to get a PR that way.....

…. This works out to Long.MIN_VALUE which is not > 0.
@coveralls
Copy link

coveralls commented Oct 15, 2020

Coverage Status

Coverage remained the same at 87.975% when pulling d39bbfb on J-Jimmy:upstream/1360_bugfix_64_cores into f131fa1 on oshi:master.

@dbwiddis
Copy link
Member

dbwiddis commented Oct 15, 2020

It's not hacky to do an important bug fix! But you need a changelog entry and probably a better title for this PR.

Also I'm going to hold off on merging this while resolving #1361 which is probably in that same iteration. I suspect using i from the bitmask should be replaced by an incrementing (from 0) counter for each core instead, and it'd be nice to fix both problems in one PR.

@Jimmy-653 Jimmy-653 changed the title This fails on core 63, which is a long with a 1 followed by 63 zeroes. This works out to Long.MIN_VALUE which is not > 0. Logical cores iteration Oct 15, 2020
@Jimmy-653
Copy link
Contributor Author

I'm really not following all this code about CPU cores....
And I have no environment to test/reproduce and analyze the behaviour in order to fix it.

@dbwiddis
Copy link
Member

I don't have a test environment either but I understand the problem well enough to describe the fix, and the user can test it.

I'll go ahead and merge this.

@dbwiddis
Copy link
Member

Or not. Sorry, as I dig into this I keep finding more. The fix you did so far applies for the pre-Win7 branch of the code.

Same fix needs to be applied in getMatchingPackage(), getMatchingNumaNode(), and getMatchingCore(). Everywhere you see ... mask ... & (1L << lp)) > 0 should be ... mask ... & (1L << lp)) != 0

@dbwiddis
Copy link
Member

And also in getLogicalProcessorInformationEx()

@dbwiddis dbwiddis added the hacktoberfest-accepted Accepted during Hacktoberfest label Oct 15, 2020
@dbwiddis dbwiddis merged commit 4b4c269 into oshi:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants