* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
@ 2024-02-14 18:00 ` jakub at gcc dot gnu.org
2024-02-14 18:13 ` torvalds@linux-foundation.org
` (13 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-14 18:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jakub at gcc dot gnu.org
--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bisection points to r12-5301-g045206450386bcd774db3bde0c696828402361c6 making
the problem go away,
3abc: b8 10 28 00 00 mov $0x2810,%eax
3ac1: 0f 78 c0 vmread %rax,%rax
3ac4: 0f 86 1e 01 00 00 jbe 3be8
<sync_vmcs02_to_vmcs12+0x858>
3aca: 45 31 e4 xor %r12d,%r12d
3acd: 4c 89 a3 f0 00 00 00 mov %r12,0xf0(%rbx)
to
3a4f: b8 10 28 00 00 mov $0x2810,%eax
3a54: 0f 78 c0 vmread %rax,%rax
3a57: 0f 86 1b 01 00 00 jbe 3b78
<sync_vmcs02_to_vmcs12+0x808>
3a5d: 48 89 83 f0 00 00 00 mov %rax,0xf0(%rbx)
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
2024-02-14 18:00 ` [Bug middle-end/113921] " jakub at gcc dot gnu.org
@ 2024-02-14 18:13 ` torvalds@linux-foundation.org
2024-02-14 18:16 ` torvalds@linux-foundation.org
` (12 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: torvalds@linux-foundation.org @ 2024-02-14 18:13 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #2 from Linus Torvalds <torvalds@linux-foundation.org> ---
(In reply to Jakub Jelinek from comment #1)
> Bisection points to r12-5301-g045206450386bcd774db3bde0c696828402361c6
> making the problem go away,
Well, that certainly explains why I can't see the problem with my gcc 13.2.1.
It looks like that commit is in gcc-12.1.o and later:
git log --oneline --all --grep="tree-optimization/102880 - improve CD-DCE"
only returns that one commit, and
git name-rev --refs '*releases*' 045206450386b
says "gcc-12.1.0~3038".
So we could make our workaround option be something like
config GCC_ASM_GOTO_WORKAROUND
def_bool y
depends on CC_IS_GCC && GCC_VERSION < 120100
but maybe there is some backporting policy with gcc that my quick git check
missed?
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
2024-02-14 18:00 ` [Bug middle-end/113921] " jakub at gcc dot gnu.org
2024-02-14 18:13 ` torvalds@linux-foundation.org
@ 2024-02-14 18:16 ` torvalds@linux-foundation.org
2024-02-14 18:21 ` jakub at gcc dot gnu.org
` (11 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: torvalds@linux-foundation.org @ 2024-02-14 18:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #3 from Linus Torvalds <torvalds@linux-foundation.org> ---
(In reply to Linus Torvalds from comment #2)
>
> So we could make our workaround option be something like
>
> config GCC_ASM_GOTO_WORKAROUND
> def_bool y
> depends on CC_IS_GCC && GCC_VERSION < 120100
Actually, rather than add a new kernel config option, I'll just make the
workaround conditional in our <linux/compiler-gcc.h> header file.
But I'll wait for confirmation from gcc people that Jakub's bisection makes
sense, and is the real fix, rather than just a change that just happens to hide
the issue.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (2 preceding siblings ...)
2024-02-14 18:16 ` torvalds@linux-foundation.org
@ 2024-02-14 18:21 ` jakub at gcc dot gnu.org
2024-02-14 18:25 ` torvalds@linux-foundation.org
` (10 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-14 18:21 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bisection in the other direction doesn't make much sense, since asm goto with
output operands is only supported in GCC 11 and later.
Anyway, with gcc 11, I can see something fishy already during expansion:
(jump_insn 927 926 1285 191 (parallel [
(set (reg:DI 385 [ value ])
(asm_operands/v:DI ("1: vmread %1, %0
jna %l2
.pushsection "__ex_table","a"
.balign 4
.long (1b) - .
.long (%l3) - .
.long 1
.popsection
") ("=r") 0 [
(reg:DI 386)
]
[
(asm_input:DI ("r")
/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h:97)
]
[
(label_ref:DI 959)
(label_ref:DI 965)
]
/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h:97))
(clobber (reg:CC 17 flags))
]) "/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h":97:2
-1
(insn_list:REG_LABEL_TARGET 1253 (insn_list:REG_LABEL_TARGET 959 (nil)))
-> 965)
;; succ: 197 count:99052688 (estimated locally)
;; 198 count:99052688 (estimated locally)
;; 192 count:99052688 (estimated locally) (FALLTHRU)
;; basic block 192, loop depth 0, count 99052688 (estimated locally), maybe hot
;; prev block 191, next block 193, flags: (NEW, REACHABLE, RTL, MODIFIED)
;; pred: 191 count:99052688 (estimated locally) (FALLTHRU)
(note 1285 927 931 192 [bb 192] NOTE_INSN_BASIC_BLOCK)
(jump_insn 931 1285 932 192 (set (pc)
(label_ref:DI 1253))
"/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h":97:2 807 {jump}
(nil)
-> 1253)
;; succ: 199 [always] count:99052688 (estimated locally)
...
(code_label 1253 1251 1252 199 1127 (nil) [1 uses])
(note 1252 1253 49 199 [bb 199] NOTE_INSN_BASIC_BLOCK)
(insn 49 1252 930 199 (set (reg:DI 152 [ _241 ])
(reg/v:DI 151 [ value ]))
"/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h":107:9 -1
(nil))
(insn 930 49 968 199 (set (reg/v:DI 151 [ value ])
(reg:DI 385 [ value ]))
"/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/vmx_ops.h":97:2 -1
(nil))
;; succ: 200 [always] count:16508781 (estimated locally) (FALLTHRU)
(code_label 968 930 969 200 1083 (nil) [5 uses])
(note 969 968 970 200 [bb 200] NOTE_INSN_BASIC_BLOCK)
(insn 970 969 971 200 (set (mem:DI (plus:DI (reg/v/f:DI 283 [ vmcs12 ])
(const_int 240 [0xf0])) [19 vmcs12_30(D)->guest_pdptr3+0 S8
A8])
(reg:DI 152 [ _241 ]))
"/home/sean/go/src/kernel.org/linux/arch/x86/kvm/vmx/nested.c":4422:25 -1
(nil))
;; succ: 201 [always] count:55029271 (estimated locally) (FALLTHRU)
So, the asm goto sets pseudo 385 and in case it doesn't jump anywhere, it then
goes
into the 2 pseudo moves but they'd need to be reversed in order to store the
asm goto
output into gues_pdptr3.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (3 preceding siblings ...)
2024-02-14 18:21 ` jakub at gcc dot gnu.org
@ 2024-02-14 18:25 ` torvalds@linux-foundation.org
2024-02-14 18:40 ` jakub at gcc dot gnu.org
` (9 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: torvalds@linux-foundation.org @ 2024-02-14 18:25 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #5 from Linus Torvalds <torvalds@linux-foundation.org> ---
(In reply to Linus Torvalds from comment #2)
>
> So we could make our workaround option be something like
>
> config GCC_ASM_GOTO_WORKAROUND
> def_bool y
> depends on CC_IS_GCC && GCC_VERSION < 120100
Replying to myself some more, since that kernel side workaround is actually in
two parts: it does the extra empty inline asm as an extra barrier, but it
*also* adds the 'volatile' to the asm with outputs to work around the other gcc
bug.
And that other fix ("Mark asm goto with outputs as volatile") is *not* in
gcc-12.1.0. It has only made it into gcc-13.2.0 (and it's pending in the gcc-12
release branch if I read things right).
I suspect that other bug doesn't affect Sean's kvm case, because the outputs
are always used, but it could affect other kernel code.
So we'd want to have at least gcc-13.2 to be safe.
Hmm.
We could make the "add volatile manually" be the default workaround, though,
since it shouldn't matter.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (4 preceding siblings ...)
2024-02-14 18:25 ` torvalds@linux-foundation.org
@ 2024-02-14 18:40 ` jakub at gcc dot gnu.org
2024-02-14 19:07 ` jakub at gcc dot gnu.org
` (8 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-14 18:40 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |vmakarov at gcc dot gnu.org
--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
insn 49 above is inserted in
#1 0x0000000000d419db in emit_insn (x=0x7fffe6e921f8) at
../../gcc/emit-rtl.c:5106
#2 0x0000000001895ab7 in ix86_expand_move (mode=E_DImode,
operands=0x7fffffffd600) at ../../gcc/config/i386/i386-expand.c:368
#3 0x0000000001de4226 in gen_movdi (operand0=0x7fffe6ec8108,
operand1=0x7fffe6ec80f0) at ../../gcc/config/i386/i386.md:1935
#4 0x0000000000db804f in insn_gen_fn::operator()<rtx_def*, rtx_def*>
(this=0x2c6cd48 <insn_data+242536>) at ../../gcc/recog.h:407
#5 0x0000000000d8f394 in emit_move_insn_1 (x=0x7fffe6ec8108, y=0x7fffe6ec80f0)
at ../../gcc/expr.c:3766
#6 0x0000000000d8fe7a in emit_move_insn (x=0x7fffe6ec8108, y=0x7fffe6ec80f0)
at ../../gcc/expr.c:3936
#7 0x00000000013b8a75 in emit_partition_copy (dest=0x7fffe6ec8108,
src=0x7fffe6ec80f0, unsignedsrcp=1, sizeexp=<ssa_name 0x7fffe9cfc6c0 240>) at
../../gcc/tree-outof-ssa.c:259
#8 0x00000000013b8c5b in insert_partition_copy_on_edge (e=<edge 0x7fffe7314000
(138 -> 145)>, dest=72, src=71, locus=2147592841) at
../../gcc/tree-outof-ssa.c:293
#9 0x00000000013b9f0b in elim_create (g=0x7fffffffd8e0, T=72) at
../../gcc/tree-outof-ssa.c:729
#10 0x00000000013ba0ab in eliminate_phi (e=<edge 0x7fffe7314000 (138 -> 145)>,
g=0x7fffffffd8e0) at ../../gcc/tree-outof-ssa.c:771
#11 0x00000000013baaa5 in expand_phi_nodes (sa=0x3169360 <SA>) at
../../gcc/tree-outof-ssa.c:1024
later on insert_partition_copy_on_edge will insert_insn_on_edge on the 138 ->
145 edge, but the insertion remains uncommitted.
Later on the asm goto expansion copies after_rtl_seq which contains (set
(reg:DI 151) (reg:DI 385)) and calls insert_insn_on_edge on it too.
But insert_insn_on_edge appends to the e->insns.r rather than prepends it which
presumably is what we'd want for the asm goto case.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (5 preceding siblings ...)
2024-02-14 18:40 ` jakub at gcc dot gnu.org
@ 2024-02-14 19:07 ` jakub at gcc dot gnu.org
2024-02-14 20:31 ` jakub at gcc dot gnu.org
` (7 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-14 19:07 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Ever confirmed|0 |1
Status|UNCONFIRMED |NEW
Last reconfirmed| |2024-02-14
--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, GCC 11 version:
--- gcc/cfgexpand.c.jj 2023-05-09 12:59:04.381738365 +0200
+++ gcc/cfgexpand.c 2024-02-14 19:56:08.733150432 +0100
@@ -3639,7 +3639,16 @@ expand_asm_stmt (gasm *stmt)
emit_insn (copy_insn (PATTERN (curr)));
rtx_insn *copy = get_insns ();
end_sequence ();
- insert_insn_on_edge (copy, e);
+ if (rtx_insn *prev = e->insns.r)
+ {
+ /* Prepend copy before any other previously
+ inserted insns on the edge rather than append. */
+ e->insns.r = NULL;
+ insert_insn_on_edge (copy, e);
+ insert_insn_on_edge (prev, e);
+ }
+ else
+ insert_insn_on_edge (copy, e);
}
}
}
changes the emitted assembler:
@@ -7328,7 +7328,7 @@ sync_vmcs02_to_vmcs12:
# 0 "" 2
#NO_APP
.L1127:
- xorl %r12d, %r12d
+ movq %rax, %r12
.L1083:
movq %r12, 240(%rbx)
jmp .L1047
@@ -29897,7 +29897,7 @@ nested_vmx_vmexit:
# 0 "" 2
#NO_APP
.L5187:
- xorl %r12d, %r12d
+ movq %rax, %r12
.L5113:
movq %r12, %rdx
movl $7, %esi
which is I believe exactly what we want.
For GCC trunk the patch would be
--- gcc/cfgexpand.cc.jj 2024-02-10 11:25:09.995474027 +0100
+++ gcc/cfgexpand.cc 2024-02-14 19:54:30.811505882 +0100
@@ -3687,7 +3687,16 @@ expand_asm_stmt (gasm *stmt)
copy = get_insns ();
end_sequence ();
}
- insert_insn_on_edge (copy, e);
+ if (rtx_insn *prev = e->insns.r)
+ {
+ /* Prepend copy before any other previously
+ inserted insns on the edge rather than append. */
+ e->insns.r = NULL;
+ insert_insn_on_edge (copy, e);
+ insert_insn_on_edge (prev, e);
+ }
+ else
+ insert_insn_on_edge (copy, e);
}
}
}
and with trunk it triggers (I mean the prev != NULL case) only on the
nested_vmx_vmexit
function and not the other one.
Guess one could try to build whole kernel with instrumented gcc (just add
FILE *f = fopen ("/tmp/asmgoto", "a");
fprintf (f, "%s %s\n", main_input_filename ? main_input_filename : "-",
current_function_name ());
fclose (f);
next to the e->insns.r = NULL; in the patch or so) to find out what else it
affects.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (6 preceding siblings ...)
2024-02-14 19:07 ` jakub at gcc dot gnu.org
@ 2024-02-14 20:31 ` jakub at gcc dot gnu.org
2024-02-15 8:00 ` rguenth at gcc dot gnu.org
` (6 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-14 20:31 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org
Status|NEW |ASSIGNED
--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 57430
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57430&action=edit
gcc14-pr113921.patch
Full untested trunk patch.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (7 preceding siblings ...)
2024-02-14 20:31 ` jakub at gcc dot gnu.org
@ 2024-02-15 8:00 ` rguenth at gcc dot gnu.org
2024-02-15 8:21 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: rguenth at gcc dot gnu.org @ 2024-02-15 8:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
So just to say all versions seem to be affected and the issue is just latent
because of the rev we bisected to.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (8 preceding siblings ...)
2024-02-15 8:00 ` rguenth at gcc dot gnu.org
@ 2024-02-15 8:21 ` jakub at gcc dot gnu.org
2024-02-15 14:56 ` cvs-commit at gcc dot gnu.org
` (4 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-15 8:21 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
*** Bug 107385 has been marked as a duplicate of this bug. ***
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (9 preceding siblings ...)
2024-02-15 8:21 ` jakub at gcc dot gnu.org
@ 2024-02-15 14:56 ` cvs-commit at gcc dot gnu.org
2024-02-15 15:14 ` cvs-commit at gcc dot gnu.org
` (3 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-02-15 14:56 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #11 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:2b4efc5db2aedb59196987300e14951d08cd7106
commit r14-9012-g2b4efc5db2aedb59196987300e14951d08cd7106
Author: Jakub Jelinek <jakub@redhat.com>
Date: Thu Feb 15 15:53:01 2024 +0100
expand: Fix handling of asm goto outputs vs. PHI argument adjustments
[PR113921]
The Linux kernel and the following testcase distilled from it is
miscompiled, because tree-outof-ssa.cc (eliminate_phi) emits some
fixups on some of the edges (but doesn't commit edge insertions).
Later expand_asm_stmt emits further instructions on the same edge.
Now the problem is that expand_asm_stmt uses insert_insn_on_edge
to add its own fixups, but that function appends to the existing
sequence on the edge if any. And the bug triggers when the
fixup sequence emitted by eliminate_phi uses a pseudo which the
fixup sequence emitted by expand_asm_stmt later on sets.
So, we end up with
(set (reg A) (asm_operands ...))
and on one of the edges queued sequence
(set (reg C) (reg B)) // added by eliminate_phi
(set (reg B) (reg A)) // added by expand_asm_stmt
That is wrong, what we emit by expand_asm_stmt needs to be as close
to the asm_operands as possible (they aren't known until expand_asm_stmt
is called, the PHI fixup code assumes it is reg B which holds the right
value) and the PHI adjustments need to be done after it.
So, the following patch introduces a prepend_insn_to_edge function and
uses it from expand_asm_stmt, so that we queue
(set (reg B) (reg A)) // added by expand_asm_stmt
(set (reg C) (reg B)) // added by eliminate_phi
instead and so the value from the asm_operands output propagates correctly
to the PHI result.
2024-02-15 Jakub Jelinek <jakub@redhat.com>
PR middle-end/113921
* cfgrtl.h (prepend_insn_to_edge): New declaration.
* cfgrtl.cc (insert_insn_on_edge): Clarify behavior in function
comment.
(prepend_insn_to_edge): New function.
* cfgexpand.cc (expand_asm_stmt): Use prepend_insn_to_edge instead
of
insert_insn_on_edge.
* gcc.target/i386/pr113921.c: New test.
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (10 preceding siblings ...)
2024-02-15 14:56 ` cvs-commit at gcc dot gnu.org
@ 2024-02-15 15:14 ` cvs-commit at gcc dot gnu.org
2024-02-15 15:17 ` cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-02-15 15:14 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #12 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:4040d472825f203660371331c9e86cd75e30f8d2
commit r13-8328-g4040d472825f203660371331c9e86cd75e30f8d2
Author: Jakub Jelinek <jakub@redhat.com>
Date: Thu Feb 15 15:53:01 2024 +0100
expand: Fix handling of asm goto outputs vs. PHI argument adjustments
[PR113921]
The Linux kernel and the following testcase distilled from it is
miscompiled, because tree-outof-ssa.cc (eliminate_phi) emits some
fixups on some of the edges (but doesn't commit edge insertions).
Later expand_asm_stmt emits further instructions on the same edge.
Now the problem is that expand_asm_stmt uses insert_insn_on_edge
to add its own fixups, but that function appends to the existing
sequence on the edge if any. And the bug triggers when the
fixup sequence emitted by eliminate_phi uses a pseudo which the
fixup sequence emitted by expand_asm_stmt later on sets.
So, we end up with
(set (reg A) (asm_operands ...))
and on one of the edges queued sequence
(set (reg C) (reg B)) // added by eliminate_phi
(set (reg B) (reg A)) // added by expand_asm_stmt
That is wrong, what we emit by expand_asm_stmt needs to be as close
to the asm_operands as possible (they aren't known until expand_asm_stmt
is called, the PHI fixup code assumes it is reg B which holds the right
value) and the PHI adjustments need to be done after it.
So, the following patch introduces a prepend_insn_to_edge function and
uses it from expand_asm_stmt, so that we queue
(set (reg B) (reg A)) // added by expand_asm_stmt
(set (reg C) (reg B)) // added by eliminate_phi
instead and so the value from the asm_operands output propagates correctly
to the PHI result.
2024-02-15 Jakub Jelinek <jakub@redhat.com>
PR middle-end/113921
* cfgrtl.h (prepend_insn_to_edge): New declaration.
* cfgrtl.cc (insert_insn_on_edge): Clarify behavior in function
comment.
(prepend_insn_to_edge): New function.
* cfgexpand.cc (expand_asm_stmt): Use prepend_insn_to_edge instead
of
insert_insn_on_edge.
* gcc.target/i386/pr113921.c: New test.
(cherry picked from commit 2b4efc5db2aedb59196987300e14951d08cd7106)
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (11 preceding siblings ...)
2024-02-15 15:14 ` cvs-commit at gcc dot gnu.org
@ 2024-02-15 15:17 ` cvs-commit at gcc dot gnu.org
2024-02-15 15:20 ` cvs-commit at gcc dot gnu.org
2024-02-15 15:22 ` jakub at gcc dot gnu.org
14 siblings, 0 replies; 16+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-02-15 15:17 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #13 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:ba09da9787e8db8903b2b0f7c647c0d7af68bb74
commit r12-10158-gba09da9787e8db8903b2b0f7c647c0d7af68bb74
Author: Jakub Jelinek <jakub@redhat.com>
Date: Thu Feb 15 15:53:01 2024 +0100
expand: Fix handling of asm goto outputs vs. PHI argument adjustments
[PR113921]
The Linux kernel and the following testcase distilled from it is
miscompiled, because tree-outof-ssa.cc (eliminate_phi) emits some
fixups on some of the edges (but doesn't commit edge insertions).
Later expand_asm_stmt emits further instructions on the same edge.
Now the problem is that expand_asm_stmt uses insert_insn_on_edge
to add its own fixups, but that function appends to the existing
sequence on the edge if any. And the bug triggers when the
fixup sequence emitted by eliminate_phi uses a pseudo which the
fixup sequence emitted by expand_asm_stmt later on sets.
So, we end up with
(set (reg A) (asm_operands ...))
and on one of the edges queued sequence
(set (reg C) (reg B)) // added by eliminate_phi
(set (reg B) (reg A)) // added by expand_asm_stmt
That is wrong, what we emit by expand_asm_stmt needs to be as close
to the asm_operands as possible (they aren't known until expand_asm_stmt
is called, the PHI fixup code assumes it is reg B which holds the right
value) and the PHI adjustments need to be done after it.
So, the following patch introduces a prepend_insn_to_edge function and
uses it from expand_asm_stmt, so that we queue
(set (reg B) (reg A)) // added by expand_asm_stmt
(set (reg C) (reg B)) // added by eliminate_phi
instead and so the value from the asm_operands output propagates correctly
to the PHI result.
2024-02-15 Jakub Jelinek <jakub@redhat.com>
PR middle-end/113921
* cfgrtl.h (prepend_insn_to_edge): New declaration.
* cfgrtl.cc (insert_insn_on_edge): Clarify behavior in function
comment.
(prepend_insn_to_edge): New function.
* cfgexpand.cc (expand_asm_stmt): Use prepend_insn_to_edge instead
of
insert_insn_on_edge.
* gcc.target/i386/pr113921.c: New test.
(cherry picked from commit 2b4efc5db2aedb59196987300e14951d08cd7106)
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (12 preceding siblings ...)
2024-02-15 15:17 ` cvs-commit at gcc dot gnu.org
@ 2024-02-15 15:20 ` cvs-commit at gcc dot gnu.org
2024-02-15 15:22 ` jakub at gcc dot gnu.org
14 siblings, 0 replies; 16+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-02-15 15:20 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
--- Comment #14 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:7a6e9e70ea88061981c5565c043985d8cde9ecc8
commit r11-11239-g7a6e9e70ea88061981c5565c043985d8cde9ecc8
Author: Jakub Jelinek <jakub@redhat.com>
Date: Thu Feb 15 15:53:01 2024 +0100
expand: Fix handling of asm goto outputs vs. PHI argument adjustments
[PR113921]
The Linux kernel and the following testcase distilled from it is
miscompiled, because tree-outof-ssa.cc (eliminate_phi) emits some
fixups on some of the edges (but doesn't commit edge insertions).
Later expand_asm_stmt emits further instructions on the same edge.
Now the problem is that expand_asm_stmt uses insert_insn_on_edge
to add its own fixups, but that function appends to the existing
sequence on the edge if any. And the bug triggers when the
fixup sequence emitted by eliminate_phi uses a pseudo which the
fixup sequence emitted by expand_asm_stmt later on sets.
So, we end up with
(set (reg A) (asm_operands ...))
and on one of the edges queued sequence
(set (reg C) (reg B)) // added by eliminate_phi
(set (reg B) (reg A)) // added by expand_asm_stmt
That is wrong, what we emit by expand_asm_stmt needs to be as close
to the asm_operands as possible (they aren't known until expand_asm_stmt
is called, the PHI fixup code assumes it is reg B which holds the right
value) and the PHI adjustments need to be done after it.
So, the following patch introduces a prepend_insn_to_edge function and
uses it from expand_asm_stmt, so that we queue
(set (reg B) (reg A)) // added by expand_asm_stmt
(set (reg C) (reg B)) // added by eliminate_phi
instead and so the value from the asm_operands output propagates correctly
to the PHI result.
2024-02-15 Jakub Jelinek <jakub@redhat.com>
PR middle-end/113921
* cfgrtl.h (prepend_insn_to_edge): New declaration.
* cfgrtl.c (insert_insn_on_edge): Clarify behavior in function
comment.
(prepend_insn_to_edge): New function.
* cfgexpand.c (expand_asm_stmt): Use prepend_insn_to_edge instead
of
insert_insn_on_edge.
* gcc.target/i386/pr113921.c: New test.
(cherry picked from commit 2b4efc5db2aedb59196987300e14951d08cd7106)
^ permalink raw reply [flat|nested] 16+ messages in thread* [Bug middle-end/113921] Output register of an "asm volatile goto" is incorrectly clobbered/discarded
2024-02-14 17:19 [Bug c/113921] New: Output register of an "asm volatile goto" is incorrectly clobbered/discarded seanjc at google dot com
` (13 preceding siblings ...)
2024-02-15 15:20 ` cvs-commit at gcc dot gnu.org
@ 2024-02-15 15:22 ` jakub at gcc dot gnu.org
14 siblings, 0 replies; 16+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-15 15:22 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution|--- |FIXED
--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Should be fixed now for GCC 14.1+, 13.3+, 12.4+ and 11.5.
^ permalink raw reply [flat|nested] 16+ messages in thread