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

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

On 1/5/24 06:22, Jan Beulich wrote:
> 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?
> 

Right. I have reworded this now for V5.

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

Yes, I meant --32. Corrected this here and other instances.  Also added 
--x32 in the text here.

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

Right. The commit log needs rewording. I have reworded this for V5.

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

Done.

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

Clearly. Fixed for V5.

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

Its the run_dump_test which runs each test with and without --scfi. 
E.g., in scfi-selfalign-func-1.d, we see:

#as: --scfi -W
#as:
#objdump: --sframe

Checking for warning in dump tests does not look possible as there will 
be warning (Warning: --scfi=all ignores most user-specified CFI 
directives) in only one case (#as: --scfi) and not the other (#as: ).

I thought checking for warnings explicitly may help in catching problems 
as the implementation evolves.

Indu