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

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

On 11/2/23 05:28, Jan Beulich wrote:
> On 01.11.2023 07:24, Indu Bhagat wrote:
>> 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).
> 
> And how exactly do you tell that the "mov %r12, %rsp" is a restore, not
> the setting of an arbitrary new %rsp value?
> 

It checks that there must have been a "mov %rsp, %r12" seen earlier. 
The SCFI machinery caches such a save of stack pointer state.

>> 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.
> 
> But the calling convention doesn't go as far as dictating local frame
> layout in a function, I don't think?
> 

No you are right on that one.

In this example, we only concern ourselves only with the callee-saved 
registers and their save/restores; the components that affect the CFI 
annotations.  The local stack usage is not of interest per se; the 
program is allowed to allocate and use as it pleases.

>>>> +	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.
> 
> Offsets are (largely) unavoidable to be numerics, yes. But .cfi_*
> directives support register names, and using them is (imo) far better
> than Dwarf register numbers - you may have memorized them by now, but
> others likely won't.
> 

Ah ok. Yeah I can use register numbers.

>>>> --- /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.
> 
> Well, okay, that's fair. You didn't respond to the other part of my
> comment, though.
> 

Sorry I missed that.  Yes, we could do two runs for the meaningful 
tests, one with --scfi and another without. That should be useful.  I 
recall there was only one case (dont recollect which one) where the 
generated FDEs were reversed in order with --scfi, but it was not a 
functional difference.  Other than that test, rest all 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?
>>>
>>
>> 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.
> 
> Why might it not be? We're talking about hand-written assembly, and
> assembly writers are free to e.g. first push all (necessary) callee-
> saved register and then copy %rsp into %rbp. For a function with many
> stack locals this has the benefit of allowing more of them to be
> accessed via <disp8>(%rbp).
> 

You are right on this one. It should be allowed. The mov %rsp, %rbp 
should switch to REG_FP based CFA in this example. I will fix it.

>>>> --- /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.
> 
> Well, #pass alone already says exactly this. If you didn't expect further
> output lines, you'd _omit_ #pass.
> 

OK.

>>>> --- /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.
> 
> You understand though that I used this only as (sutiable) example. My point
> being that hand-written assembly frequently doesn't resemble compiler-
> genertaed code. Otherwise, i.e. if the resulting code wasn't to be different,
> why would one write assembly code in nthe first place?
> 

True.
Open to suggestions on improving the testsuite.

>>>> --- /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".
> 
> By "constraint" I mean assumptions you make on the way assembly code is
> written, for your machinery to be usable. The more such assumptions, the
> smaller the set of code "eligible" to (future) use of your work. Hence
> also why I think all such "constraints" need to be spelled out in a
> single place, such that one can
> - verify that everything meeting these constraints actually also works,
> - check up front whether one's assembly code is actually suitable for
>    enabling --scfi.
> 

OK, Thanks. We are on the same page wrt "Constraints" then. In the above 
example, though, the "constraint" is due to the desire to be able 
unwind/stack trace asynchronously. Which is why it confused me in the 
first place, I think I wouldn't call this a "constraint" here. But this 
is a subjective line.

In the above example, the 'addq    %rax, %rbp' does cause a problem for 
unwinding because the CFA is REG_FP based. And the SCFI machinery needs 
to be able to tell what offset from REG_FP is the CFA. I think its 
correct for the SCFI machinery to complain with a ".*14: Error: SCFI: 
usage of REG_FP as scratch not supported" right away, because the unwind 
information at the next instruction is already affected.

That said, I do agree that the constraints need to be spelled out in a 
single place. I already have done some of it in scfi.c, but there is 
room for improvement, I see.

>>>> --- /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.
> 
> I'm in favor of such assisting warnings, but they're useful only if there
> are few false positives and few false negatives. Too many warnings makes
> people ignore them all (or shut them off), while too few makes people
> mistrust them being helpful.
> 

Noted. And I do agree. I just so happens that I dont have a solution to 
this problem of false positive warnings yet.

>>>> --- /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.
> 
> But that's precisely my point: As long as you don't support --x32, you
> want to check that its use is properly diagnosed (just like use of
> --32 is).
> 

Ah, apologies I construed this comment differently earlier. Yes, I agree 
we can add another run with --x32.