-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Bugfix: Use Base.aligned_sizeof instead of sizeof in Mmap.mmap #58998
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
Bugfix: Use Base.aligned_sizeof instead of sizeof in Mmap.mmap #58998
Conversation
Very nice, thank you for finding and fixing this. |
stdlib/Mmap/test/runtests.jl
Outdated
@@ -343,6 +343,18 @@ end | |||
GC.gc() | |||
rm(file) | |||
|
|||
# test for #58982 |
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.
Should this be put into a @testset
to make failures here more localized?
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.
We can wrap it into a testset.
I just tried to match the (oldish) style of the test file as closely as possible.
I think this broke Windows CI (#59053) |
The only difference from the other tests is that it is wrapped into an |
CI at least passed on the Windows worker that ran (https://buildkite.com/julialang/julia-master/builds/49176#01980ab7-c042-4ddb-a06e-6020753851d3) before the testset was introduced. |
The purpose of this PR is to fix #58982 .
The current (bugged) behaviour is:
This is because
Mmap
erroneously usessizeof
instead ofBase.aligned_sizeof
to compute the number of elements (32 ÷ 9 == 3
).This has gone undetected for a long time because
sizeof(T) == Base.aligned_sizeof(T)
is true for most types including structs. (Padding is to a multiple of 8 bytes is included insizeof(T)
)closes #58982
edit:
Previously,
sizeof
would throw an error exception when presented with a non-mappable type such asRef
.This occurred within the default argument generation.
Base.aligned_sizeof
does not error here, and this is instead caught in the dedicatedisbits
check.This seems desirable and I changed the test to require an
ArgumentError
.