Fix and enforce reinterpret/unsafe_wrap alignment #21831
Conversation
yuyichao
added
system:arm
|
@jlbuild !filter=arm,aarch64,ppc64 |
|
@nanosoldier |
base/bitarray.jl
Outdated
| %p = getelementptr i64, i64* %0, i64 %1 | ||
| %v = load i64, i64* %p, align 1 | ||
| ret i64 %v | ||
| """, UInt64, Tuple{Ptr{UInt64},Int64}, p64, Int64(idx - 1)) |
There was a problem hiding this comment.
return Core.Intrinsics.pointerref(p64, #=1 based=# idx, #=align (constant)=# 1)
There was a problem hiding this comment.
and then can call this unsafe_load_unaligned, and just define it the same as unsafe_load
There was a problem hiding this comment.
Sure. I was actually going to just call it inline....
There was a problem hiding this comment.
Actually I didn't realize that unsafe_load uses align 1 so I'll just use that......................
There was a problem hiding this comment.
Ah right – I forgot that I originally didn't trust users / C libraries to declare types exactly correct :P (and then the align attribute was added because the GPU folks complained that it was being too pessimistic)
yuyichao
force-pushed
the
yyc/align
branch
2 times, most recently
from
a78c5fc
to
5819ae1
Compare
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
src/array.c
Outdated
| } | ||
| if (((uintptr_t)data) & (align - 1)) | ||
| jl_exceptionf(jl_argumenterror_type, | ||
| "unsafe_wrap: pointer %p is not properly aligned to %u", data, align); |
There was a problem hiding this comment.
would be clearer imo (and in the other instance below)
| @@ -4848,12 +4856,15 @@ end | |||
| let ni128 = sizeof(FP128test) ÷ sizeof(Int), | |||
| ns128 = sizeof(FP128align) ÷ sizeof(Int), | |||
| nbit = sizeof(Int) * 8, | |||
| arr = reinterpret(FP128align, collect(Int, 1:(2 * ns128))), | |||
| arr = Vector{FP128align}(2), | |||
There was a problem hiding this comment.
should this be larger than 2 on 32 bit?
There was a problem hiding this comment.
No. The original size is 2 * ns128 * sizeof(Int) ÷ sizeof(FP128align) == 2 * sizeof(FP128align) ÷ sizeof(Int) * sizeof(Int) ÷ sizeof(FP128align) == 2
There was a problem hiding this comment.
makes sense – we want to allocate this with the larger alignment (FP128align), but fill it with the smaller alignment (Int)
Welp, at least we get an exciting new error now... |
|
Any comment? (This got significantly fewer comments than I expected). Also what's the most up to date method of running PkgEval? |
|
It's kind of hacky and manual at the moment. I don't have a branch for nightly = 0.7 yet, but https://github.com/JuliaCI/PackageEvaluator.jl/tree/backport-0.6.0-rc2 is what I used for rc2 - change the parts of setup.sh that set the binary to download and the metadata subset to run on, trigger a linux 64 packaging run and use that binary for "LZ" (the vm names are lies when running this kind of comparison) |
|
I assume the binary from the buil 🐶 can also be used for this? @jlbuild !filter=arm,aarch64,ppc64,linux64 |
|
Updated the error message without updating the test.... Try again.................. @jlbuild !filter=arm,aarch64,ppc64,linux64 |
|
The fault on ARM doesn't even make sense GDB shows a dmesg log when the segv handler is removed. |
|
@jlbuild !filter=arm !nuke |
|
PkgEval results below. I've only check The new ARM failure on buidog is #21926 --- master 2017-05-17 11:42:40.483749791 -0400
+++ pr 2017-05-17 11:42:48.443905204 -0400
@@ -60,7 +60,7 @@
BaseTestNext.json "tests_fail"
BasisMatrices.json "tests_pass"
BayesianDataFusion.json "tests_fail"
-BayesianNonparametrics.json "tests_pass"
+BayesianNonparametrics.json "tests_fail"
BayesNets.json "tests_fail"
BDF.json "tests_pass"
BeaData.json "tests_pass"
@@ -87,7 +87,7 @@
Blocks.json "tests_fail"
BloomFilters.json "tests_pass"
Blosc.json "tests_pass"
-BlossomV.json "tests_pass"
+BlossomV.json "tests_fail"
BlsData.json "tests_fail"
Bokeh.json "tests_fail"
Boltzmann.json "tests_fail"
@@ -142,7 +142,7 @@
ClusterUtils.json "tests_fail"
CMakeWrapper.json "tests_pass"
COBRA.json "tests_fail"
-Codecs.json "tests_pass"
+Codecs.json "tests_fail"
CodeTools.json "tests_pass"
COESA.json "tests_pass"
COFF.json "tests_fail"
@@ -198,7 +198,7 @@
CPUTime.json "tests_pass"
CQL.json "tests_fail"
Crayons.json "tests_pass"
-CRC.json "tests_pass"
+CRC.json "tests_fail"
CreateMacrosFrom.json "tests_pass"
CRF.json "tests_fail"
CRlibm.json "tests_pass"
@@ -227,7 +227,7 @@
CUTEst.json "tests_fail"
CutPruners.json "tests_pass"
CVXOPT.json "tests_pass"
-Cxx.json "tests_pass"
+Cxx.json "tests_fail"
CxxWrap.json "tests_pass"
Dagger.json "tests_pass"
DASKR.json "tests_pass"
@@ -344,7 +344,7 @@
EnhancedGJK.json "tests_fail"
Ensemble.json "tests_fail"
Equations.json "tests_fail"
-Erdos.json "tests_pass"
+Erdos.json "tests_fail"
ERFA.json "tests_pass"
ErrorfreeArithmetic.json "tests_pass"
Escher.json "tests_fail"
@@ -521,10 +521,10 @@
IJulia.json "not_possible"
IJuliaPortrayals.json "tests_fail"
ImageAxes.json "tests_pass"
-ImageCore.json "tests_pass"
+ImageCore.json "tests_fail"
ImageFiltering.json "tests_pass"
ImageInTerminal.json "tests_fail"
-ImageMagick.json "tests_pass"
+ImageMagick.json "tests_fail"
ImageMetadata.json "tests_pass"
ImageProjectiveGeometry.json "tests_pass"
ImageQuilting.json "tests_pass"
@@ -556,7 +556,7 @@
Interact.json "tests_pass"
InterestRates.json "tests_pass"
Interfaces.json "tests_fail"
-Interpolations.json "tests_fail"
+Interpolations.json "tests_pass"
IntervalArithmetic.json "tests_pass"
IntervalConstraintProgramming.json "tests_pass"
IntervalContractors.json "tests_pass"
@@ -588,7 +588,7 @@
JLDArchives.json "tests_fail"
JLD.json "tests_fail"
JointMoments.json "tests_fail"
-JPLEphemeris.json "tests_pass"
+JPLEphemeris.json "tests_fail"
JSON.json "tests_pass"
JuliaFEM.json "tests_fail"
JuliaParser.json "tests_fail"
@@ -631,7 +631,7 @@
LearnBase.json "tests_pass"
LearningStrategies.json "tests_pass"
LeastSquaresOptim.json "tests_fail"
-LegacyStrings.json "tests_pass"
+LegacyStrings.json "tests_fail"
Lens.json "tests_fail"
LevelDB.json "tests_fail"
Levenshtein.json "tests_fail"
@@ -710,7 +710,7 @@
MatrixNetworks.json "tests_pass"
Maxima.json "tests_fail"
Mayday.json "tests_pass"
-MbedTLS.json "tests_pass"
+MbedTLS.json "tests_fail"
MCMC.json "no_tests"
MDCT.json "tests_pass"
MDPs.json "tests_fail"
@@ -810,7 +810,7 @@
NonNegLeastSquares.json "tests_pass"
NormalizeQuantiles.json "tests_pass"
NoveltyColors.json "tests_pass"
-NPZ.json "tests_pass"
+NPZ.json "tests_fail"
NRRD.json "tests_fail"
NullableArrays.json "tests_pass"
NumberedLines.json "tests_pass"
@@ -837,7 +837,7 @@
OnlineStats.json "tests_pass"
OpenCL.json "not_possible"
OpenDSSDirect.json "tests_pass"
-OpenEphysLoader.json "tests_pass"
+OpenEphysLoader.json "tests_fail"
OpenFOAM.json "tests_fail"
OpenGene.json "tests_fail"
OpenStreetMap.json "tests_fail"
@@ -871,7 +871,7 @@
PathDistribution.json "tests_pass"
PATHSolver.json "tests_pass"
PatternDispatch.json "tests_fail"
-Pcap.json "tests_pass"
+Pcap.json "tests_fail"
PdbTool.json "tests_fail"
PDMats.json "tests_pass"
Pedigrees.json "tests_fail"
@@ -1075,7 +1075,7 @@
SFML.json "tests_fail"
SGDOptim.json "tests_fail"
SGP4.json "tests_pass"
-SHA.json "tests_pass"
+SHA.json "tests_fail"
Shannon.json "tests_fail"
Shapefile.json "tests_pass"
ShapeModels.json "tests_fail"
@@ -1146,7 +1146,7 @@
StokesDiffEq.json "tests_pass"
StreamStats.json "tests_fail"
StringDistances.json "tests_pass"
-StringEncodings.json "tests_pass"
+StringEncodings.json "tests_fail"
StringInterpolation.json "tests_pass"
StrPack.json "tests_fail"
StructIO.json "tests_fail"
@@ -1260,7 +1260,7 @@
VisualRegressionTests.json "tests_pass"
VLFeat.json "tests_fail"
VML.json "not_possible"
-VoronoiCells.json "tests_pass"
+VoronoiCells.json "tests_fail"
VoronoiDelaunay.json "tests_pass"
Voronoi.json "tests_pass"
Voting.json "tests_fail"
@@ -1292,7 +1292,7 @@
WordNet.json "tests_pass"
WorldBankData.json "tests_fail"
WORLD.json "tests_pass"
-WriteVTK.json "tests_pass"
+WriteVTK.json "tests_fail"
XGBoost.json "tests_fail"
XKPasswd.json "tests_pass"
XMLconvert.json "tests_pass"
@@ -1306,7 +1306,7 @@
YT.json "not_possible"
ZChop.json "tests_pass"
Zeros.json "tests_pass"
-ZipFile.json "tests_pass"
+ZipFile.json "tests_fail"
Zlib.json "tests_fail"
ZMQ.json "tests_pass"
ZVSimulator.json "no_tests" |
Unrelated failures
Cxx.jl has a load failure
Real bugs
Failure due to dependency issue
This amount of breackage is not more than what I expected. I'll try to fix (i.e. PR) at least |
| work = zeros(UInt64, JN32 >> 1) | ||
| dsfmt = Vector{UInt64}(nval >> 1) | ||
| ccall(:memcpy, Ptr{Void}, (Ptr{UInt64}, Ptr{Int32}, Csize_t), | ||
| dsfmt, val, (nval - 1) * sizeof(Int32)) |
There was a problem hiding this comment.
As the last location of dsfmt::Vector{UInt64} below is overwritten, isn't it enough to copy (nval-2)*sizeof(Int32) in the memcpy here? Of course nothing wrong either.
|
Hmm, an interesting consequence here is that we now get errors when we try to To preserve the alignment, we'd have to make sure that the NRRD header's length is a multiple of |
This is exactly the type of error that this change is meant to catch #14885 (comment) |
|
The implementation of an array that provides unaligned access is exactly what I was suggesting in JuliaImages/ImageCore.jl#38 (comment) (though maybe not for the usage case in ImageCore). From the fixes I submitted, it seems that this could be useful as a package for memory mapped IO. |
|
Since it doesn't matter on x86, presumably the C library just didn't care? Should we have a very_unsafe_wrap that skips the check? |
|
x86 will start to care when you reinterpret a vector Vector (.... ah I hate this overloaded meaning of vector)...... Unless there isn't any better solution (I think a unaligned array could be good) I'd like to not provide interfaces that lies to LLVM.... |
|
Because I thought a different approach would be better, I'd forgotten about your andrewcooke/CRC.jl#14. Sure, I'll grab that and polish it up and submit it as a JuliaArrays package. |
|
JLD2.jl is breaking because of this. Unfortunately, I have a lot of research data tied up in JLD2-formatted files that I can no longer access. |
|
For the moment you should switch to 0.6-rc2, because that doesn't include this change. I do worry that this change is pretty invasive to have at such a late moment in 0.6. Perhaps we shouldn't backport? |
It's difficult for me to do that because I'm also in the middle of ensuring that LightGraphs and GraphIO play nicely with -nightly (until 0.6 is released, we're sort of in this strange situation where we have to make sure 0.6-rc* and 0.7 work). |
|
@timholy This will not be backported to 0.6. The part that should be is the first commit which is mentioned in #21831 (comment) |
|
I can confirm that JLD2 works without issues on -rc2. Thank you, @timholy. |
|
Am I correct in understanding that this PR disallows doing the following: julia> t = zeros(UInt8, 2)
2-element Array{UInt8,1}:
0x00
0x00
julia> reinterpret(UInt16, t)
ERROR: ArgumentError: reinterpret from alignment 1 bytes to alignment 2 bytes not allowed
Stacktrace:
[1] reinterpret(::Type{UInt16}, ::Array{UInt8,1}, ::Tuple{Int64}) at ./array.jl:160
[2] reinterpret(::Type{UInt16}, ::Array{UInt8,1}) at ./array.jl:139Do we really have to be this strict? And are there work-arounds? FWIW, this change broke FlatBuffers.jl, though I'm not sure why it didn't make it into all the discussions and PkgEval runs mentioned in this PR. But now I'm trying to figure it out; the use-case in FlatBuffers is that we essentially get binary blobs as inputs and decode structs and sometimes arrays of structs out of the binary blob; it seems a bit crazy to me if a |
Yes.
This is actually exactly the wrong thing to do. Though more presicely, anything with alignment larger than a byte.
Use |
|
And for an array we have to do something like |
|
That's one way to do it. You can also use the unaligned vector mentioned above. Or allocate it with the correct type and reinterpret back for reading etc. |
The first commit is a bug fix that fixes a LLVM undefined behavior which actually cause segfault on ARM and is the one that should be backported. The second commit is to reflect this semantic requirement in the julia API so that we won't have this kind of bug in the future especially since we've actually had this kind of bug in the test before...
It is in principle possible to loosen the alignment check in
reinterpreton the pointer though that make this kind of bug much harder to catch/reason about. We should ideally also run PkgEval on it to identify buggy packages.