diff mbox series

[V2,2/2] gas: scfi: untraceable control flow should be a hard error

Message ID 20240124072629.1193542-3-indu.bhagat@oracle.com
State Unresolved
Headers
Series Testcase refactoring and fix PR gas/31284 |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Indu Bhagat Jan. 24, 2024, 7:26 a.m. UTC 
  PR gas/31284

Currently, if an indirect jump is seen, GCFG (a CFG of ginsns) cannot be
created, and the SCFI machinery bails out with a warning:
  "Untraceable control flow for func 'foo'; Skipping SCFI"

It is, however, better suited if this is a hard error.  Change it to a
hard error.  Also change the text of the error to align better with
other existing SCFI warnings and errors (prepend with 'SCFI:'):
  "SCFI: untraceable control flow for func 'foo'"

gas/
PR gas/31284
	* ginsn.c (ginsn_data_end): Use as_bad instead of as_warn.

gas/testsuite/
PR gas/31284
	* gas/scfi/x86_64/ginsn-cofi-1.l: Adjust to the expected output
	in case of errors.
	* gas/scfi/x86_64/scfi-unsupported-cfg-1.l: Error not Warning.
---
 gas/ginsn.c                                    |  4 ++--
 gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l   | 18 ++++++++++--------
 .../gas/scfi/x86_64/scfi-unsupported-cfg-1.l   |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

Comments

Jan Beulich Jan. 25, 2024, 1:59 p.m. UTC | #1 
On 24.01.2024 08:26, Indu Bhagat wrote:
> --- a/gas/ginsn.c
> +++ b/gas/ginsn.c
> @@ -1161,8 +1161,8 @@ ginsn_data_end (const symbolS *label)
>    /* Build the cfg of ginsn(s) of the function.  */
>    if (!frchain_now->frch_ginsn_data->gcfg_apt_p)
>      {
> -      as_warn (_("Untraceable control flow for func '%s'; Skipping SCFI"),
> -	       S_GET_NAME (func));
> +      as_bad (_("SCFI: untraceable control flow for func '%s'"),
> +	      S_GET_NAME (func));
>        goto end;
>      }

This switch is probably fine. My question here is: How come ginsn.c issues
an SCFI-specific diagnostic? Really most if not all of ginsn_data_end() looks
to be concerned of only SCFI, when e.g. ginsn_pass_warn_unreachable_code()
might have value on its own.

Jan
Indu Bhagat Jan. 25, 2024, 7:43 p.m. UTC | #2 
On 1/25/24 05:59, Jan Beulich wrote:
> On 24.01.2024 08:26, Indu Bhagat wrote:
>> --- a/gas/ginsn.c
>> +++ b/gas/ginsn.c
>> @@ -1161,8 +1161,8 @@ ginsn_data_end (const symbolS *label)
>>     /* Build the cfg of ginsn(s) of the function.  */
>>     if (!frchain_now->frch_ginsn_data->gcfg_apt_p)
>>       {
>> -      as_warn (_("Untraceable control flow for func '%s'; Skipping SCFI"),
>> -	       S_GET_NAME (func));
>> +      as_bad (_("SCFI: untraceable control flow for func '%s'"),
>> +	      S_GET_NAME (func));
>>         goto end;
>>       }
> 
> This switch is probably fine. My question here is: How come ginsn.c issues
> an SCFI-specific diagnostic? Really most if not all of ginsn_data_end() looks
> to be concerned of only SCFI, when e.g. ginsn_pass_warn_unreachable_code()
> might have value on its own.
> 

Thank you for reminding me that - I do remember being of two minds on 
keeping the string "Skipping SCFI" originally. On the one hand, the 
argument was that "ginsn_pass_warn_unreachable_code() has value on its 
own" (like you mention).  On the other hand I wondered if users may find 
it confusing to see this warning about GAS trying to decipher control 
flow for a function and whether this affects the synthesized CFI. So, I 
ended up adding "Skipping SCFI"...

I will remove the "SCFI:" string from the message.  I think switch to 
error relieves me of some of those concerns regarding 'confusing warning 
when SCFI is enabled'

Thanks
diff mbox series

Patch

diff --git a/gas/ginsn.c b/gas/ginsn.c
index 5f6a67ce4f2..0625bcd67cf 100644
--- a/gas/ginsn.c
+++ b/gas/ginsn.c
@@ -1161,8 +1161,8 @@  ginsn_data_end (const symbolS *label)
   /* Build the cfg of ginsn(s) of the function.  */
   if (!frchain_now->frch_ginsn_data->gcfg_apt_p)
     {
-      as_warn (_("Untraceable control flow for func '%s'; Skipping SCFI"),
-	       S_GET_NAME (func));
+      as_bad (_("SCFI: untraceable control flow for func '%s'"),
+	      S_GET_NAME (func));
       goto end;
     }
 
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l b/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l
index fee76f9cc9b..d1809118e27 100644
--- a/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l
@@ -1,3 +1,5 @@ 
+.*: Assembler messages:
+.*:20: Error: SCFI: untraceable control flow for func 'foo'
 GAS LISTING .*
 
 
@@ -12,23 +14,23 @@  GAS LISTING .*
    8              	ginsn: SYM FUNC_BEGIN
    9              	foo:
    9              	ginsn: SYM foo
-  10 0000 4801D0   		addq    %rdx, %rax
+  10 \?\?\?\? 4801D0   		addq    %rdx, %rax
   10              	ginsn: ADD %r1, %r0, %r0
-  11 0003 E200     		loop    foo
+  11 \?\?\?\? E200     		loop    foo
   11              	ginsn: JCC 
-  12 0005 3EFFE0   		notrack jmp     \*%rax
+  12 \?\?\?\? 3EFFE0   		notrack jmp     \*%rax
   12              	ginsn: JMP %r0, 
-  13 0008 41FFD0   		call    \*%r8
+  13 \?\?\?\? 41FFD0   		call    \*%r8
   13              	ginsn: CALL
-  14 000b 67E305   		jecxz   .L179
+  14 \?\?\?\? 67E305   		jecxz   .L179
   14              	ginsn: JCC 
-  15 000e FF6730   		jmp     \*48\(%rdi\)
+  15 \?\?\?\? FF6730   		jmp     \*48\(%rdi\)
   15              	ginsn: JMP %r5, 
-  16 0011 7000     		jo      .L179
+  16 \?\?\?\? 7000     		jo      .L179
   16              	ginsn: JCC 
   17              	.L179:
   17              	ginsn: SYM .L179
-  18 0013 C3       		ret
+  18 \?\?\?\? C3       		ret
   18              	ginsn: RET
   19              	.LFE0:
   19              	ginsn: SYM .LFE0
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-cfg-1.l b/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-cfg-1.l
index 1e138a102fe..09446f51f1b 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-cfg-1.l
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-unsupported-cfg-1.l
@@ -1,3 +1,3 @@ 
 .*Assembler messages:
 .*50: Warning: SCFI ignores most user-specified CFI directives
-.*52: Warning: Untraceable control flow for func 'foo'; Skipping SCFI
+.*52: Error: SCFI: untraceable control flow for func 'foo'