-
-
Notifications
You must be signed in to change notification settings - Fork 699
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
[stable] Fix std.experimental.allocator.common.effectiveAlignment() #6622
Conversation
It was unable to handle alignments > 2^31 bytes, e.g., returning 2 for the 64-bit pointer 0x2FA_00000000. Return a size_t instead of a uint. Note that it's only called in one place, an assertion in a ctor of the BitmappedBlockImpl mixin, which handles size_t fine. This fixes sporadic std.experimental.allocator.building_blocks.bitmapped_block unittest failures for LDC CI on Win64 (something like 1 out of 100 runs failing).
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + phobos#6622" |
@@ -99,7 +99,7 @@ private mixin template BitmappedBlockImpl(bool isShared, bool multiBlock) | |||
auto leadingUlongs = blocks.divideRoundUp(64); | |||
import std.algorithm.comparison : min; | |||
immutable initialAlignment = min(parentAlignment, | |||
1U << trailingZeros(leadingUlongs * 8)); | |||
1U << min(31U, trailingZeros(leadingUlongs * 8))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only other occurrence of dangerous 1U <<
in the allocator package.
The CircleCi error can be safely ignored. #6623 is the cherry-pick from master. |
{ | ||
return 1U << trailingZeros(cast(size_t) ptr); | ||
return (cast(size_t) 1) << trailingZeros(cast(size_t) ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible for trailingZeros to return 64
?
assert(trailingZeros(0) == 64);
So maybe trailingZeros
needs to be fixed to accept size_t
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, good point, so for 32-bit size_t(0)
, trailingZeros() would indeed return 64 as well. Here, it doesn't matter, as effectiveAlignment(null)
returns size_t(1) << 64U => 1
for both 32/64-bit size_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: prefer size_t(1)
over cast syntax.
It was unable to handle alignments > 2^31 bytes, e.g., returning 2 for the 64-bit pointer 0x2FA_00000000. Return a
size_t
instead of auint
. Note that it's only called in one place, an assertion in a ctor of theBitmappedBlockImpl
mixin, which handlessize_t
fine.This fixes sporadic
std.experimental.allocator.building_blocks.bitmapped_block
unittest failures for LDC CI on Win64 (something like 1 out of 100 runs failing).