Uh oh!
There was an error while loading. Please reload this page.
Emit correct alignment information for loads/store of small aggregates - #24472
Conversation
rust-highfive
commented
Apr 15, 2015
(rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
commented
Apr 15, 2015
Nice find! Could a test case be added as well for this? (perhaps a |
alexcrichton
commented
Apr 16, 2015
Er looks like I mistook this issue for another, never mind! |
nikomatsakis
commented
Apr 16, 2015
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. |
dotdash
commented
Apr 16, 2015
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 |
dotdash
commented
Apr 16, 2015
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. |
alexcrichton
commented
Apr 16, 2015
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. Fixesrust-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. Fixesrust-lang#23431
bors
commented
Apr 17, 2015
⌛ Testing commit e153dea with merge 1963a3d... |
bors
commented
Apr 17, 2015
💔 Test failed - auto-linux-64-nopt-t |
alexcrichton
commented
Apr 17, 2015
@bors: retry On Fri, Apr 17, 2015 at 9:26 AM, bors notifications@github.com wrote:
|
bors
commented
Apr 18, 2015
⌛ Testing commit e153dea with merge 0773d10... |
bors
commented
Apr 18, 2015
💔 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. Fixesrust-lang#23431
dotdash
commented
Apr 18, 2015
bors
commented
Apr 18, 2015
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. Fixesrust-lang#23431
bors
commented
Apr 18, 2015
⛄ The build was interrupted to prioritize another pull request. |
bors
commented
Apr 18, 2015
⌛ Testing commit 78745a4 with merge 3e08c96... |
bors
commented
Apr 18, 2015
⛄ 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