Emit correct alignment information for loads/store of small aggregates #24472
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Nice find! Could a test case be added as well for this? (perhaps a |
|
Er looks like I mistook this issue for another, never mind! |
|
This does indeed look great, but I would very much like to see some tests. It'd probably be good to setup some kind of infrastructure that can verify properties of generated LLVM IR (perhaps this is what @alexcrichton was suggesting?). (But I'm not quite sure how that ought to look -- it'd obviously be hard to isolate the relevant load.) Anyway, r+ from me, but I'd like to know whether @dotdash has any thoughts on a way to write some sort of easily reproducible test -- one though I have is to create a big byte buffer and load an aggregate from there, which would allow us to observe if the alignment is wrong. |
|
I can't think of anything that would trigger on x86 hardware, because AFAIK the relevant instructions don't care about alignment. The problem was discovered on an ARMv5 that has rather "interesting" behaviour on unaligned loads. The only thing I can think of would indeed be to verify the LLVM IR, but we have no infrastructure for that yet. What do you think of repurposing the codegen test directory to perform checks on the LLVM IR using LLVM's |
To clarify, the alignment of the data is correct, but the alignment communicated to LLVM is wrong, so on some architectures, it lowers it to machine instructions that only work with the alignment that was communicated to LLVM, not the actual alignment of the data. So I can't think of a way to observe the problem easily from rust code in a portable way. |
Loading from and storing to small aggregates happens by casting the aggregate pointer to an appropriately sized integer pointer to avoid the usage of first class aggregates which would lead to less optimized code. But this means that, for example, a tuple of type (i16, i16) will be loading through an i32 pointer and because we currently don't provide alignment information LLVM assumes that the load should use the ABI alignment for i32 which would usually be 4 byte alignment. But the alignment requirement for the (i16, i16) tuple will usually be just 2 bytes, so we're overestimating alignment, which invokes undefined behaviour. Therefore we must emit appropriate alignment information for stores/loads through such casted pointers. Fixes rust-lang#23431
Loading from and storing to small aggregates happens by casting the aggregate pointer to an appropriately sized integer pointer to avoid the usage of first class aggregates which would lead to less optimized code. But this means that, for example, a tuple of type (i16, i16) will be loading through an i32 pointer and because we currently don't provide alignment information LLVM assumes that the load should use the ABI alignment for i32 which would usually be 4 byte alignment. But the alignment requirement for the (i16, i16) tuple will usually be just 2 bytes, so we're overestimating alignment, which invokes undefined behaviour. Therefore we must emit appropriate alignment information for stores/loads through such casted pointers. Fixes rust-lang#23431
|
⌛ Testing commit e153dea with merge 1963a3d... |
|
💔 Test failed - auto-linux-64-nopt-t |
|
@bors: retry On Fri, Apr 17, 2015 at 9:26 AM, bors notifications@github.com wrote:
|
|
⌛ Testing commit e153dea with merge 0773d10... |
|
💔 Test failed - auto-mac-64-opt |
Loading from and storing to small aggregates happens by casting the aggregate pointer to an appropriately sized integer pointer to avoid the usage of first class aggregates which would lead to less optimized code. But this means that, for example, a tuple of type (i16, i16) will be loading through an i32 pointer and because we currently don't provide alignment information LLVM assumes that the load should use the ABI alignment for i32 which would usually be 4 byte alignment. But the alignment requirement for the (i16, i16) tuple will usually be just 2 bytes, so we're overestimating alignment, which invokes undefined behaviour. Therefore we must emit appropriate alignment information for stores/loads through such casted pointers. Fixes rust-lang#23431
|
⌛ Testing commit 78745a4 with merge 1b2251b... |
Loading from and storing to small aggregates happens by casting the aggregate pointer to an appropriately sized integer pointer to avoid the usage of first class aggregates which would lead to less optimized code. But this means that, for example, a tuple of type (i16, i16) will be loading through an i32 pointer and because we currently don't provide alignment information LLVM assumes that the load should use the ABI alignment for i32 which would usually be 4 byte alignment. But the alignment requirement for the (i16, i16) tuple will usually be just 2 bytes, so we're overestimating alignment, which invokes undefined behaviour. Therefore we must emit appropriate alignment information for stores/loads through such casted pointers. Fixes rust-lang#23431
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit 78745a4 with merge 3e08c96... |
|
⛄ The build was interrupted to prioritize another pull request. |
Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.
But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.
Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.
Fixes #23431