Summary: | [11 Regression] unaligned access generated with memset or {} and -O2 -mstrict-align | ||
---|---|---|---|
Product: | gcc | Reporter: | felix |
Component: | target | Assignee: | Wilco <wilco> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | amonakov, hp, keven.kloeckner, pinskia, sjames, wilco |
Priority: | P2 | Keywords: | wrong-code |
Version: | 11.2.0 | ||
Target Milestone: | 12.5 | ||
See Also: |
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101934 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111121 |
||
Host: | Target: | aarch64 | |
Build: | Known to work: | 14.0 | |
Known to fail: | 13.3.1 | Last reconfirmed: | 2021-11-05 00:00:00 |
Bug Depends on: | |||
Bug Blocks: | 105886 | ||
Attachments: |
source code that generates the faulty assembly
New simplier patch which does the right thing too |
Description
felix
2021-11-05 12:52:19 UTC
Dup of bug 101934, fixed for gcc 11.3.0. *** This bug has been marked as a duplicate of bug 101934 *** Actually it is not a dup. Confirmed, another simplier testcase: void use(unsigned char*); void g() { unsigned char t2[216]; __builtin_memset(t2, 0, sizeof(t2)); use(t2); } Mine. This should fix it (untested): diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 699c105a42a..5d0872be4ef 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -23584,7 +23584,9 @@ aarch64_expand_setmem (rtx *operands) over writing. */ opt_scalar_int_mode mode_iter; FOR_EACH_MODE_IN_CLASS (mode_iter, MODE_INT) - if (GET_MODE_BITSIZE (mode_iter.require ()) <= MIN (n, copy_limit)) + if (GET_MODE_BITSIZE (mode_iter.require ()) <= MIN (n, copy_limit) + && (!STRICT_ALIGNMENT + || MEM_ALIGN (dst) >= GET_MODE_ALIGNMENT (mode_iter.require ()))) cur_mode = mode_iter.require (); gcc_assert (cur_mode != BLKmode); (In reply to Andrew Pinski from comment #4) > Mine. > This should fix it (untested): And yes it works. Patch submitted: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583554.html Created attachment 51830 [details]
New simplier patch which does the right thing too
New patch submitted: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584927.html Updated patch submitted: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589254.html GCC 11.3 is being released, retargeting bugs to GCC 11.4. *** Bug 105886 has been marked as a duplicate of this bug. *** (In reply to Andrew Pinski from comment #10) > Updated patch submitted: > https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589254.html I think you need to ping your patches more aggressively ... (In reply to Richard Biener from comment #13) > (In reply to Andrew Pinski from comment #10) > > Updated patch submitted: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589254.html > > I think you need to ping your patches more aggressively ... Richard Sandiford reviewed it here:| https://gcc.gnu.org/pipermail/gcc-patches/2022-February/589581.html So the problem is that the review wasn't followed up by the submitter. (In reply to Richard Earnshaw from comment #14) > (In reply to Richard Biener from comment #13) > > (In reply to Andrew Pinski from comment #10) > > > Updated patch submitted: > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589254.html > > > > I think you need to ping your patches more aggressively ... > > Richard Sandiford reviewed it here:| > https://gcc.gnu.org/pipermail/gcc-patches/2022-February/589581.html > So the problem is that the review wasn't followed up by the submitter. I did not know that I have any further obligation on this past submitting the bug, I never submitted a bug before. Anyway, the since the patch works (at least for my use case), do I have to mark this as resolved, fixed? (In reply to felix from comment #15) He means apinski who submitted a patch, not you. Another testcase which is now affecting us at Marvell in our early firmware: ``` void f(const char*); void g(void) { char t[32] = "0l2345678"; f(t); } ``` So I am planning on getting back on this patch starting today. (In reply to Andrew Pinski from comment #17) > Another testcase which is now affecting us at Marvell in our early firmware: > ``` > void f(const char*); > > void g(void) > { > char t[32] = "0l2345678"; > f(t); > } > > ``` > So I am planning on getting back on this patch starting today. I should say that testcase happens at `-Os -mstrict-align`, at `-O2 -mstrict-align` it works. (In reply to Andrew Pinski from comment #18) > I should say that testcase happens at `-Os -mstrict-align`, at `-O2 > -mstrict-align` it works. Because for -Os we don't forcibly align arrays - see AARCH64_EXPAND_ALIGNMENT and the macros that use it. Submitted a version new patch: https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611684.html *** Bug 109273 has been marked as a duplicate of this bug. *** I am no longer working on this. GCC 11.4 is being released, retargeting bugs to GCC 11.5. Patch to avoid emitting unaligned LDP/STP with -mstrict-align: https://gcc.gnu.org/pipermail/gcc-patches/2023-September/631022.html The master branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>: https://gcc.gnu.org/g:318f5232cfb3e0c9694889565e1f5424d0354463 commit r14-6012-g318f5232cfb3e0c9694889565e1f5424d0354463 Author: Wilco Dijkstra <wilco.dijkstra@arm.com> Date: Wed Oct 25 16:28:04 2023 +0100 AArch64: Fix strict-align cpymem/setmem [PR103100] The cpymemdi/setmemdi implementation doesn't fully support strict alignment. Block the expansion if the alignment is less than 16 with STRICT_ALIGNMENT. Clean up the condition when to use MOPS. gcc/ChangeLog/ PR target/103100 * config/aarch64/aarch64.md (cpymemdi): Remove pattern condition. (setmemdi): Likewise. * config/aarch64/aarch64.cc (aarch64_expand_cpymem): Support strict-align. Cleanup condition for using MOPS. (aarch64_expand_setmem): Likewise. (In reply to GCC Commits from comment #25) > The master branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>: Are you considering back-porting this to other branches (like gcc-13) or is there another reason this PR is still open? The commit applies cleanly there, but is untested. Asking for a friend. :) The releases/gcc-13 branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>: https://gcc.gnu.org/g:5aa9ed0f353f835005c3df8932c7bc6e26f53904 commit r13-8874-g5aa9ed0f353f835005c3df8932c7bc6e26f53904 Author: Wilco Dijkstra <wilco.dijkstra@arm.com> Date: Wed Oct 25 16:28:04 2023 +0100 AArch64: Fix strict-align cpymem/setmem [PR103100] The cpymemdi/setmemdi implementation doesn't fully support strict alignment. Block the expansion if the alignment is less than 16 with STRICT_ALIGNMENT. Clean up the condition when to use MOPS. gcc/ChangeLog/ PR target/103100 * config/aarch64/aarch64.md (cpymemdi): Remove pattern condition. (setmemdi): Likewise. * config/aarch64/aarch64.cc (aarch64_expand_cpymem): Support strict-align. Cleanup condition for using MOPS. (aarch64_expand_setmem): Likewise. (cherry picked from commit 318f5232cfb3e0c9694889565e1f5424d0354463) (In reply to GCC Commits from comment #27) > The releases/gcc-13 branch has been updated by Wilco Dijkstra > <wilco@gcc.gnu.org>: Thanks! The releases/gcc-12 branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>: https://gcc.gnu.org/g:b9d16d8361a9e3a82a2f21e759e760d235d43322 commit r12-10603-gb9d16d8361a9e3a82a2f21e759e760d235d43322 Author: Wilco Dijkstra <wilco.dijkstra@arm.com> Date: Wed Oct 25 16:28:04 2023 +0100 AArch64: Fix strict-align cpymem/setmem [PR103100] The cpymemdi/setmemdi implementation doesn't fully support strict alignment. Block the expansion if the alignment is less than 16 with STRICT_ALIGNMENT. Clean up the condition when to use MOPS. gcc/ChangeLog/ PR target/103100 * config/aarch64/aarch64.md (cpymemdi): Remove pattern condition. (setmemdi): Likewise. * config/aarch64/aarch64.cc (aarch64_expand_cpymem): Support strict-align. Cleanup condition for using MOPS. (aarch64_expand_setmem): Likewise. (cherry picked from commit 318f5232cfb3e0c9694889565e1f5424d0354463) Fixed on GCC12 branch too. It doesn't apply to GCC11, so it's unlikely to be worth fixing since GCC11 branch will be closed soon. |