Re: [PATCH, V2 09/10] gas: testsuite: add a x86_64 testsuite for SCFI

[Jan Beulich <jbeulich@suse.com>]

On 30.10.2023 17:51, Indu Bhagat wrote:
> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-add-2.s
> @@ -0,0 +1,48 @@
> +        .section        .rodata
> +	.type	simd_cmp_op, @object
> +	.size	simd_cmp_op, 8
> +simd_cmp_op:
> +	.long	2
> +	.zero	4
> +
> +# Testcase for add instruction.
> +# add reg, reg instruction
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	.cfi_startproc
> +	pushq	%rbp
> +	.cfi_def_cfa_offset 16
> +	.cfi_offset 6, -16
> +	movq    %rsp, %rbp
> +	.cfi_def_cfa_register 6
> +	pushq	%r12
> +	.cfi_offset 12, -24
> +	mov	%rsp, %r12

You copy %rsp to %r12 here, and ...

> +# Stack manipulation is permitted if the base register for
> +# tracking CFA has been changed to FP.
> +	addq    %rdx, %rsp
> +	addq	%rsp, %rax
> +# Some add instructions may access the stack indirectly.  Such
> +# accesses do not make REG_FP untraceable.
> +	addl    %eax, -84(%rbp)
> +# Other kind of add instructions should not error out in the
> +# x86_64 -> ginsn translator
> +	addq    $simd_cmp_op+8, %rdx
> +	addl    %edx, -32(%rsp)
> +	addl    $1, fb_low_counter(,%rbx,4)
> +	mov	%r12, %rsp

... you restore it here, but both without any .cfi_* annotation.
It is therefore unclear whether this is in any way related to ...

> +# Popping a callee-saved register.
> +# RSP must be traceable.
> +	pop     %r12
> +	.cfi_restore 12

... what the comment says about these.

> +	leave
> +	.cfi_def_cfa_register 7
> +	.cfi_restore 6

Using numbers here isn't very helpful, I'm afraid.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-asm-marker-1.s
> @@ -0,0 +1,27 @@
> +# Testcase where a user may define hot and cold areas of function
> +# Note how the .type, and .size directives may be placed differently
> +# than a regular function.
> +
> +	.globl  foo
> +	.type   foo, @function
> +foo:
> +        .cfi_startproc
> +        testl   %edi, %edi
> +        je      .L3
> +        movl    b(%rip), %eax
> +        ret
> +        .cfi_endproc
> +        .section        .text.unlikely
> +        .cfi_startproc
> +        .type   foo.cold, @function
> +foo.cold:
> +.L3:
> +        pushq   %rax
> +        .cfi_def_cfa_offset 16
> +        call    abort
> +       .cfi_endproc
> +.LFE11:
> +        .text
> +        .size   foo, .-foo
> +        .section        .text.unlikely
> +        .size   foo.cold, .-foo.cold

Related to the comment: Is it "may" or is it rather "need to"?
(In the latter case it would be yet another constraint on when this
new machinery is actually usable.)

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-asm-marker-2.s
> @@ -0,0 +1,11 @@
> +# A programmer may not bother to set the size of the 
> +# function symbols via an explicit .size directive.
> +	.globl  foo
> +	.type   foo, @function
> +foo:
> +        .cfi_startproc
> +        testl   %edi, %edi
> +        je      .L3
> +        movl    b(%rip), %eax
> +        ret
> +       .cfi_endproc

Wasn't it said elsewhere that .size needs using after the function?

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-asm-marker-3.l
> @@ -0,0 +1,2 @@
> +.*Assembler messages:
> +.*9: Warning: --scfi=all ignores some user-specified CFI directives

Is repeating this for (about?) every test really necessary / useful?
If multiple passes for every test were to make sense to me, I'd expect
one pass with SCFI and one pass without, where the directives then
take effect. And then the same set of expectations should match.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-bp-sp-2.s
> @@ -0,0 +1,49 @@
> +# Testcase for switching between sp/fp based CFA.
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +        .cfi_startproc
> +        pushq   %r14
> +        .cfi_def_cfa_offset 16
> +        .cfi_offset 14, -16
> +        pushq   %r13
> +        .cfi_def_cfa_offset 24
> +        .cfi_offset 13, -24
> +        pushq   %r12
> +        .cfi_def_cfa_offset 32
> +        .cfi_offset 12, -32
> +        pushq   %rbp
> +        .cfi_def_cfa_offset 40
> +        .cfi_offset 6, -40
> +        pushq   %rbx
> +        .cfi_def_cfa_offset 48
> +        .cfi_offset 3, -48
> +        movq    %rdi, %rbx
> +        subq    $32, %rsp
> +        .cfi_def_cfa_offset 80
> +# This mov does not switch CFA tracking to REG_FP, because there has already
> +# been stack usage between here and the push %rbp
> +        movq    %rsp, %rbp

Yet another constraint?

> +        xorl    %eax, %eax
> +        addq    $32, %rsp
> +        .cfi_def_cfa_offset 48
> +        popq    %rbx
> +	.cfi_restore 3

Nit: inconsistent indentation (also elsewhere).

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-callee-saved-1.s
> @@ -0,0 +1,26 @@
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +#	.cfi_startproc
> +	pushq	%rax
> +	.cfi_def_cfa_offset 16
> +	push	%rbx
> +	.cfi_def_cfa_offset 24
> +	.cfi_offset 3, -24
> +	pushq	%rbp
> +        .cfi_def_cfa_offset 32
> +        .cfi_offset 6, -32
> +	popq	%rbp
> +	.cfi_def_cfa_offset 24
> +	.cfi_restore 6
> +	popq	%rbx
> +	.cfi_def_cfa_offset 16
> +	.cfi_restore 3
> +	popq	%rax
> +        .cfi_def_cfa_offset 8
> +	ret
> +#	.cfi_endproc

Why are startproc/endproc commented out here?

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-callee-saved-2.s
> @@ -0,0 +1,42 @@
> +# Testcase for save reg ops for callee-saved registers
> +# These latter two pushq's of callee-saved regs must NOT generate
> +# .cfi_offset.
> +
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	.cfi_startproc
> +	pushq	%r12
> +	.cfi_def_cfa_offset 16
> +	.cfi_offset 12, -16
> +	pushq	%r13
> +	.cfi_def_cfa_offset 24
> +	.cfi_offset 13, -24
> +# The function may use callee-saved registers for its use, and may even
> +# chose to spill them to stack if necessary.
> +	addq    %rax, %r13
> +	subq 	$8, %r13
> +# These two pushq's of callee-saved regs must NOT generate
> +# .cfi_offset.
> +	pushq	%r13
> +	.cfi_def_cfa_offset 32
> +	pushq	%rax
> +	.cfi_def_cfa_offset 40

Why "two" in the comment? %rax isn't callee-saved, is it? (Same in
variant 3 of this kind of test then.)

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-callee-saved-4.s
> @@ -0,0 +1,55 @@
> +	.type	byte_insert_op1, @function
> +byte_insert_op1:
> +.LFB10:
> +	.cfi_startproc
> +	endbr64
> +	pushq	%rbp
> +	.cfi_def_cfa_offset 16
> +	.cfi_offset 6, -16
> +	movq	%rsp, %rbp
> +	.cfi_def_cfa_register 6
> +	pushq	%r12
> +	.cfi_offset 12, -24
> +	pushq	%rbx
> +	.cfi_offset 3, -32
> +	subq	$24, %rsp
> +	movl	%edi, -20(%rbp)
> +	movq	%rsi, -32(%rbp)
> +	movl	%edx, -24(%rbp)
> +	movq	%rcx, -40(%rbp)
> +# The program may use callee-saved registers for its use, and may even
> +# chose to read them from stack if necessary.  The following use should
> +# not be treated as reg restore for SCFI purposes (because rbx has been
> +# saved to -16(%rbp).

But even if the value was read back from -16(%rbp) you wouldn't know for
sure that that's the ultimate restore. Yet another constraint?

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-1.d
> @@ -0,0 +1,36 @@
> +#as: --scfi -W
> +#objdump: -Wf
> +#name: Synthesize CFI in presence of control flow 1
> +#...
> +Contents of the .eh_frame section:
> +
> +00000000 0+0014 0+0000 CIE
> +  Version:               1
> +  Augmentation:          "zR"
> +  Code alignment factor: 1
> +  Data alignment factor: -8
> +  Return address column: 16
> +  Augmentation data:     [01][abc]
> +  DW_CFA_def_cfa: r7 \(rsp\) ofs 8
> +  DW_CFA_offset: r16 \(rip\) at cfa-8
> +  DW_CFA_nop
> +  DW_CFA_nop
> +
> +0+0018 0+0024 0000001c FDE cie=00000000 pc=0+0000..0+003a
> +  DW_CFA_advance_loc: 1 to 0+0001
> +  DW_CFA_def_cfa_offset: 16
> +  DW_CFA_offset: r3 \(rbx\) at cfa-16
> +  DW_CFA_advance_loc: 37 to 0+0026
> +  DW_CFA_remember_state
> +  DW_CFA_advance_loc: 1 to 0+0027
> +  DW_CFA_restore: r3 \(rbx\)
> +  DW_CFA_def_cfa_offset: 8
> +  DW_CFA_advance_loc: 1 to 0+0028
> +  DW_CFA_restore_state
> +  DW_CFA_advance_loc: 9 to 0+0031
> +  DW_CFA_restore: r3 \(rbx\)
> +  DW_CFA_def_cfa_offset: 8
> +  DW_CFA_nop
> +#...
> +
> +#pass

Seeing this recurring pattern (the last three lines): Why not just #pass?

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-1.s
> @@ -0,0 +1,47 @@
> +# Testcase with one dominator bb and two exit bbs
> +# Something like for: return ferror (f) || fclose (f) != 0;
> +	.text
> +	.section	.rodata.str1.1,"aMS",@progbits,1
> +.LC0:
> +	.string	"w"
> +.LC1:
> +	.string	"conftest.out"
> +	.section	.text.startup,"ax",@progbits
> +	.p2align 4
> +	.globl	main
> +	.type	main, @function
> +main:
> +.LFB11:

Coming back to "hand-written assembly": The above looks very much like it
was compiler output (earlier tests did, too, but it's perhaps more
prominent here). That's not necessarily what a human might write, and
hence I wonder about the overall coverage that can be gained that way.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cofi-1.d
> @@ -0,0 +1,5 @@
> +#as: --scfi -W
> +#objdump: -Wf
> +#name: Synthesize CFI for add insn
> +
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cofi-1.l
> @@ -0,0 +1,3 @@
> +.*Assembler messages:
> +.*12: Warning: --scfi=all ignores some user-specified CFI directives
> +.*22: Warning: Untraceable control flow for func 'foo'; Skipping SCFI
> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cofi-1.s
> @@ -0,0 +1,23 @@
> +# Testcase with a variety of "change of flow instructions"
> +#
> +# Must be run with -W so it remains warning free.
> +#
> +# This test does not have much going on wrt synthesis of CFI;
> +# it just aims to ensure x8_64 -> ginsn decoding must behave
> +# gracefully for these "change of flow instructions"
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	.cfi_startproc
> +	addq    %rdx, %rax
> +	notrack jmp     *%rax
> +	call   *%r8
> +	jmp     *48(%rdi)
> +	jo      .L179
> +.L179:
> +	ret
> +	.cfi_endproc
> +.LFE0:
> +	.size	foo, .-foo

What exactly is being tested here? The .d file is effectively empty,
and the .l file expects a warning on the .size directive (which tells
about nothing regarding the reason for the failure to produce SCFI).

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-diag-1.s
> @@ -0,0 +1,23 @@
> +# Testcase for REG_FP based CFA
> +# and using REG_FP as scratch.
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	.cfi_startproc
> +	pushq	%rbp
> +	.cfi_def_cfa_offset 16
> +	.cfi_offset 6, -16
> +	movq    %rsp, %rbp
> +	.cfi_def_cfa_register 6
> +# The following add causes REG_FP to become untraceable
> +	addq	%rax, %rbp

But this isn't a problem as long as FP isn't further used. Indeed ...

> +	.cfi_def_cfa_register 7
> +	pop	%rbp

... its original value is restored right afterwards. Yet another constraint?

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-fp-diag-2.s
> @@ -0,0 +1,55 @@
> +# Testcase for a diagnostic around assymetrical restore
> +# Testcase inspired by byte_insert_op1 in libiberty
> +# False positive for the diagnostic
> +.type	foo, @function
> +foo:
> +.LFB10:
> +	.cfi_startproc
> +	endbr64
> +	pushq	%rbp
> +	.cfi_def_cfa_offset 16
> +	.cfi_offset 6, -16
> +	movq	%rsp, %rbp
> +	.cfi_def_cfa_register 6
> +	pushq	%r12
> +	pushq	%rbx
> +	subq	$24, %rsp
> +	.cfi_offset 12, -24
> +	.cfi_offset 3, -32
> +	movl	%edi, -20(%rbp)
> +	movq	%rsi, -32(%rbp)
> +	movl	%edx, -24(%rbp)
> +	movq	%rcx, -40(%rbp)
> +# The assembler cannot differentiate that the following
> +# mov to %rbx is not a true restore operation, but simply
> +# %rbx register usage as a scratch reg of some sort.
> +# The assembler merely warns of a possible assymetric restore operation
> +# In this case, its noise for the user unfortunately.
> +	movq	-40(%rbp), %rbx
> +	movq	-40(%rbp), %rax
> +	leaq	3(%rax), %r12
> +	jmp	.L563
> +.L564:
> +	subq	$1, %rbx
> +	subq	$1, %r12
> +	movzbl	(%rbx), %eax
> +	movb	%al, (%r12)
> +.L563:
> +	cmpq	-32(%rbp), %rbx
> +	jne	.L564

But this is pretty common usage. Imo this (the false positive warning)
cannot remain like this.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-ignore-1.s
> @@ -0,0 +1,13 @@
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	.cfi_startproc
> +	pushq	%rbp
> +        .cfi_def_cfa_offset 16
> +        .cfi_offset 6, -16
> +	ret
> +	.cfi_endproc
> +.LFE0:
> +	.size	foo, .-foo

This, otoh, imo wants diagnosing. The framework ought to recognize that
the RET doesn't use the correct stack slot.

Additionally, isn't scfi-simple-1.s effectively testing the same? The
code at least looks extremely similar.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-1.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +Fatal error: Synthesizing CFI is not supported for this ABI
> diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-1.s b/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-1.s
> new file mode 100644
> index 00000000000..87d2a4971a1
> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-1.s
> @@ -0,0 +1,10 @@
> +# Testcase run with --m32 (Not supported).
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	pushq	%rbp

This code, being built with --32, is bogus anyway. I wonder why this file
has any insns in the first place.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
> @@ -0,0 +1,103 @@
> +# Copyright (C) 2022-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
> +
> +if { ![is_elf_format] } then {
> +    return
> +}
> +
> +# common tests
> +if  { ([istarget "x86_64-*-*"]) } then {
> +
> +    global ASFLAGS
> +    set old_ASFLAGS "$ASFLAGS"
> +
> +    run_dump_test "scfi-cfi-label-1"
> +    run_list_test "scfi-cfi-label-1" "--scfi --warn"
> +
> +    run_list_test "scfi-diag-1" "--scfi"
> +    run_list_test "scfi-fp-diag-2" "--scfi"
> +    run_list_test "scfi-diag-2" "--scfi"
> +
> +    run_list_test "scfi-unsupported-1" "--32 --scfi"

Perhaps a 2nd run with --x32, unless (see above) you choose to support
that ABI as well?

Jan