Re: [PATCH,V4 13/14] gas: testsuite: add a x86_64 testsuite for SCFI

[Jan Beulich <jbeulich@suse.com>]

On 03.01.2024 08:15, Indu Bhagat wrote:
> [Changes from V3 to V4]
>   - Forward and backward pass failure is an error (was warning in V2).
>     Fix the tests.
>   - Add new test ginsn-add-1.
>   - Add new test scfi-unsupported-insn-1 which includes the new APX
>     instructions (pop2, push2).
> [End of changes from V3 to V4]
> 
> [Changes from V2 to V3]
>   - Fix inconsistent indentation across tests.
>   - Fixed some issues in scfi-x86.exp: add scfi-callee-saved-1 to the
>     list (was missing earlier), removed redundant test scfi-ignore-1
>   - Adjsuted tests for some rewording in the warning messages.
>   - Use register names instead of numbers in some CFI directives.
>   - Run each test with and without --scfi.  This will keep the CFI
>     annotations tested.

With this, ...

>   - Run scfi-unsupported-1 with --x32 as well.
>   - Added more tests:
>     + ginsn-dw2-regnum-1
>     + ginsn-pop-1
>     + ginsn-push-1
>     + scfi-enter-1
>     + scfi-cfi-sections-1, etc.
> [End of changes from V2 to V3]
> 
> The testsuite for SCFI contains target-specific tests.
> 
> As all the tests are executed with --scfi command line option, the CFI
> annotations in the test .s files are skipped altogether by the GAS for
> processing.  The CFI directives in the assembly files are added with the
> intention to aid maintainence only: CFI annotations in .s files help
> convey the expected EH Frame / SFrame data in a format-oblivious way.

... wouldn't this better have been re-worded?

> Some testcases are used to highlight those asm constructs that the SCFI
> machinery in GAS currently does not support:
> 
>   - Only AMD64 ABI is supported for now. Using --m32 with --scfi results
>     in hard error.
>     See scfi-unsupported-1.s.

DYM --32 here? And did you also want to mention --x32?

>   - Untraceable stack-pointer manipulation in function epilougue and prologue.
>     See scfi-unsupported-2.s.
> 
>   - Using Dynamically Realigned Arguement Pointer (DRAP) register to
>     realign the stack.  For SCFI, the CFA must be only REG_SP or REG_FP
>     based.  See scfi-unsupported-drap-1.s
> 
> Some testcases are used to highlight some diagnostics that the SCFI
> machinery in GAS currently issues, with an intent to help user correct
> inadvertent errors in their hand-written asm.  An error is issued in a
> situation where GAS is not sure it will be able to synthesize valid CFI.
> 
>   - (#1) "Warning: SCFI: Asymetrical register restore"
>   - (#2) "Error: SCFI: usage of REG_FP as scratch not supported"
>   - (#3) "Error: SCFI: unsupported stack manipulation pattern"

When "not sure", I think it should be warnings. The bulleted list looks
to match that, but the earlier text doesn't.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/README
> @@ -0,0 +1,17 @@
> +Notes on the SCFI testsuite in GAS:
> +
> +* At this time, SCFI machinery is only supported for x86_64.
> +
> +* When adding more tests, please keep CFI annotations updated in the .s files.
> +  Recall that user-specified, synthesizable CFI annotations are ignored by the
> +  GAS when --scfi (=all) is in effect.  Adding CFI annocations, irrespectively,
> +  makes the testcases clearer in terms of understanding the expected unwind
> +  data.

As per above, this may also want re-wording.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/ginsn-add-1.s
> @@ -0,0 +1,26 @@
> +## Testcase with a variety of add.
> +## Some add insns valid in 64-bit mode may not be processed for SCFI.
> +	.text
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	push	%rsp
> +	movq	%rsp, %rbp
> +
> +	addq	%rax, symbol
> +        add     symbol, %eax
> +
> +        add	(%eax), %esp
> +        add	%esp, (,%eax)
> +
> +        addq	%rax, %rbx
> +	add	%eax, %ebx
> +
> +        addq    $1, -16(%rbp)
> +
> +        add	(,%eax), %esp
> +        add	%esp, (,%eax)
> +
> +	ret
> +.LFE0:
> +	.size	foo, .-foo

In the revision log you say you sorted inconsistent indentation, but
here you clearly didn't.

> --- /dev/null
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
> @@ -0,0 +1,113 @@
> +# 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_list_test "ginsn-dw2-regnum-1" "--scfi -ali"
> +    run_list_test "ginsn-add-1" "--scfi -ali"
> +    run_list_test "ginsn-pop-1" "--scfi -ali"
> +    run_list_test "ginsn-push-1" "--scfi -ali"
> +
> +    run_dump_test "scfi-cfi-label-1"
> +    run_list_test "scfi-cfi-label-1" "--scfi --warn"

Starting here, where is it that you also check CFI generated from
the directives? Also, why does each source need assembling twice?
Can't you check for the diagnostics right in the "dump" tests?

Jan

> +    run_dump_test "scfi-cfi-sections-1"
> +    run_list_test "scfi-cfi-sections-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"
> +    run_list_test "scfi-unsupported-1" "--x32 --scfi"
> +    run_list_test "scfi-unsupported-insn-1" "--scfi"
> +    run_list_test "scfi-unsupported-2" "--scfi"
> +    run_list_test "scfi-unsupported-3" "--scfi"
> +    run_list_test "scfi-unsupported-4" "--scfi"
> +    run_list_test "scfi-unsupported-drap-1" "--scfi"
> +    run_list_test "scfi-unsupported-cfg-1" "--scfi"
> +    run_list_test "scfi-unsupported-cfg-2" "--scfi"
> +
> +    run_dump_test "scfi-add-1"
> +    run_list_test "scfi-add-1" "--scfi --warn"
> +    run_dump_test "scfi-add-2"
> +    run_list_test "scfi-add-2" "--scfi --warn"
> +    run_dump_test "scfi-cfg-1"
> +    run_list_test "scfi-cfg-1" "--scfi --warn"
> +    run_dump_test "scfi-cfg-2"
> +    run_list_test "scfi-cfg-2" "--scfi --warn"
> +    run_dump_test "scfi-asm-marker-1"
> +    run_list_test "scfi-asm-marker-1" "--scfi --warn"
> +    run_dump_test "scfi-asm-marker-2"
> +    run_list_test "scfi-asm-marker-2" "--scfi --warn"
> +    run_dump_test "scfi-asm-marker-3"
> +    run_list_test "scfi-asm-marker-3" "--scfi --warn"
> +    run_dump_test "scfi-pushsection-1"
> +    run_list_test "scfi-pushsection-1" "--scfi --warn"
> +    run_dump_test "scfi-pushsection-2"
> +    run_list_test "scfi-pushsection-2" "--scfi --warn"
> +
> +    run_dump_test "scfi-cofi-1"
> +    run_list_test "scfi-cofi-1" "--scfi --warn"
> +    run_dump_test "scfi-sub-1"
> +    run_list_test "scfi-sub-1" "--scfi --warn"
> +    run_dump_test "scfi-sub-2"
> +    run_list_test "scfi-sub-2" "--scfi --warn"
> +    run_dump_test "scfi-simple-1"
> +    run_list_test "scfi-simple-1" "--scfi --warn"
> +    run_dump_test "scfi-simple-2"
> +    run_list_test "scfi-simple-2" "--scfi --warn"
> +    run_dump_test "scfi-pushq-1"
> +    run_list_test "scfi-pushq-1" "--scfi --warn"
> +    run_dump_test "scfi-lea-1"
> +    run_list_test "scfi-lea-1" "--scfi --warn"
> +    run_dump_test "scfi-enter-1"
> +    run_list_test "scfi-enter-1" "--scfi --warn"
> +    run_dump_test "scfi-leave-1"
> +    run_list_test "scfi-leave-1" "--scfi --warn"
> +    run_dump_test "scfi-bp-sp-1"
> +    run_list_test "scfi-bp-sp-1" "--scfi --warn"
> +    run_dump_test "scfi-bp-sp-2"
> +    run_list_test "scfi-bp-sp-2" "--scfi --warn"
> +    run_dump_test "scfi-callee-saved-1"
> +    run_list_test "scfi-callee-saved-1" "--scfi --warn"
> +    run_dump_test "scfi-callee-saved-2"
> +    run_list_test "scfi-callee-saved-2" "--scfi --warn"
> +    run_dump_test "scfi-callee-saved-3"
> +    run_list_test "scfi-callee-saved-3" "--scfi --warn"
> +    run_dump_test "scfi-callee-saved-4"
> +    run_list_test "scfi-callee-saved-4" "--scfi --warn"
> +    run_dump_test "scfi-dyn-stack-1"
> +    run_list_test "scfi-dyn-stack-1" "--scfi --warn"
> +    run_dump_test "scfi-indirect-mov-1"
> +    run_list_test "scfi-indirect-mov-1" "--scfi --warn"
> +    run_dump_test "scfi-indirect-mov-2"
> +    run_list_test "scfi-indirect-mov-2" "--scfi --warn"
> +    run_dump_test "scfi-indirect-mov-3"
> +    run_list_test "scfi-indirect-mov-3" "--scfi --warn"
> +    run_dump_test "scfi-indirect-mov-4"
> +    run_list_test "scfi-indirect-mov-4" "--scfi --warn"
> +    run_dump_test "scfi-selfalign-func-1"
> +    run_list_test "scfi-selfalign-func-1" "--scfi --warn"
> +}
> +