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

[Indu Bhagat <indu.bhagat@oracle.com>]

Hi Jan,

Thanks for reviewing.

On 10/31/23 09:13, Jan Beulich wrote:
> 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 ...
> 

There are no .cfi_* annotations for the %rsp updates here because the 
CFA tracking has already been switched to REG_FP based tracking.  From 
the perspective of DWARF CFI, this usages of the stack do not need to 
tracked.  If unwinding happens from any instruction in the above 
sequence, we already have the correct and complete unwind information.

>> +# Popping a callee-saved register.
>> +# RSP must be traceable.
>> +	pop     %r12
>> +	.cfi_restore 12
> 
> ... what the comment says about these.
> 

The comment here means that hand-written programs may use stack for 
their local usage etc. but must ensure that before or in the epilogue, 
rsp is restored to the desired value (before register restores via pop 
instructions).

This is not a special handling in the SCFI machinery but it is what is 
needed to ensure correctness of the function.  For asm not adhering to 
the ABI/calling conventions, SCFI cannot be used.

>> +	leave
>> +	.cfi_def_cfa_register 7
>> +	.cfi_restore 6
> 
> Using numbers here isn't very helpful, I'm afraid.
> 

I am not sure I understand.  The DWARF register numbers and the offset 
values are required by the semantics of those CFI directives.

>> --- /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.)
> 

The .size directives are not necessary for SCFI.  They are recommended.
In this test, removing the code after .LFE11 should still work, I will 
check this.

>> --- /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?
> 

Only in the RFC series, the implementation of SCFI required that the 
blocks be closed by using a .size directive.  In the V1 onwards 
implementation, this restriction has been lifted.  This was based on the 
feedback that users may have hand-written asm without the .size directives.

I have tried to correct the code comments and git commit log to reflect 
that .size directive is recommended but not necessary, but perhaps some 
stale text/statement has slipped through. I will fix if I see it.

>> --- /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.
> 

re: is repeating this useful? Strictly speaking no. _But_, I think as 
the code evolves, we may add more diagnostics to the SCFI machinery. 
Explicitly checking for the set of warnings helps us ensure no new 
warnings show up where they are not expected. In a way, it is a stricter 
check and ensures no unintented slippage.

>> --- /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?
> 

Not sure I understand. This is not a constraint for SCFI.  A pattern 
where the user does:
     push %rbp
     .. more stack usage..
     movq %rsp, %rbp
is not using frame pointer for stack tracing, but simply as a scratch 
register.

So the SCFI machinery simply acknowledges this and does not switch to 
REG_FP based CFA tracking.


>> +        xorl    %eax, %eax
>> +        addq    $32, %rsp
>> +        .cfi_def_cfa_offset 48
>> +        popq    %rbx
>> +	.cfi_restore 3
> 
> Nit: inconsistent indentation (also elsewhere).
> 

I found lots of instances of this issue. Will fix it.

>> --- /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?
> 

They are reminiscent of some previous state I had the tests in. I will 
add them back in here and other instances.

>> --- /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.)
> 

Correct, %rax is not calee-saved. I will fix the comment.

>> --- /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?
> 

Correct, we cannot know in GAS that it's the ultimate restore before 
return.  I ask later as well, but the usage of "Yet another constraint" 
expression is not clear to me.

>> --- /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?
> 

I thought adding #... is clearer as it indicates that the test knowingly 
skips lines thereafter.

>> --- /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.
> 

Yes, its inspired by compiler generated output. I ran into this when 
running some tests. Its still a good basic cfg test for SCFI - a 
function with two return paths.

>> --- /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).
> 

Only that the x86 -> ginsn does not choke on these instructions.  You 
are right that it doesnt test much for SCFI per se.

>> --- /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?
> 

Hmm. I need some clarification on the statement "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.
> 

The warning of "Warning: SCFI: asymetrical register restore" was added 
to alert user if they do something like
     pushq %rbx
     pushq %r12
     ...
     popq  %rbx
     popq  %r12
i.e., asymmetric save and restore.  Now the user may do a restore to any 
register and use what was saved on stack as scratch regiser; the only 
way to differentiate between a "true reg restore in epilogue" vs "reg 
restored for scratch purposes" is to know whether its the epilogue.  GAS 
will not be able to determine that easily.

We could remove the warning altogether.  I just thought it provides 
users with some protection / validation of hand-written asm.


>> --- /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.
> 

Yes, its on my TODO list. I plan to add this: if the stack is not 
balanced at the return from function, gas can warn.

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

Yes, one can removed. The two tests achieve the same end result.

>> --- /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.
> 

I will update this.

>> --- /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?
> 

At the moment, the plan was to next work on --scfi=inline and add 
support for aarch64/aapcs64 ABIs.  No plans to add support for x32 
unless a need arises.

Thanks
Indu