[PING][PATCH] gas: gcfg: fix handling of non-local direct jmps in gcfg

[PING][PATCH] gas: gcfg: fix handling of non-local direct jmps in gcfg

[Indu Bhagat]

The ginsn infrastructure in GAS includes the ability to create a GCFG
(ginsn CFG).  A GCFG is currently used for SCFI passes.

This patch fixes the following invalid assumptions / code blocks:
 - The function ginsn_direct_local_jump_p () was erroneously _not_
   checking whether the symbol is locally defined (i.e., within the
   scope of the code block for which GCFG is desired).  Fix the code
   to do so.
 - Similarly, the GCFG creation code, in gcfg_build () itself had an
   assumption that a GINSN_TYPE_JUMP to a non-local symbol will not be
   seen.  The latter can indeed be seen, and in fact, needs to be treated
   the same way as an exit from the function in terms of control-flow.

gas/
        * ginsn.c (ginsn_direct_local_jump_p): Check if the symbol
	is local to the code block or function being assembled.
        (add_bb_at_ginsn): Remove buggy assumption.
        (frch_ginsn_data_append): Direct jmps do not disqualify a stream
	of ginsns from GCFG creation.

gas/testsuite/
	* gas/scfi/x86_64/scfi-cfg-3.d: New test.
	* gas/scfi/x86_64/scfi-cfg-3.l: New test.
	* gas/scfi/x86_64/scfi-cfg-3.s: New test.
	* gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
---
 gas/ginsn.c                                   | 43 +++++++++++--------
 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d    | 24 +++++++++++
 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l    |  2 +
 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s    | 21 +++++++++
 gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  2 +
 5 files changed, 73 insertions(+), 19 deletions(-)
 create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
 create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
 create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s

diff --git a/gas/ginsn.c b/gas/ginsn.c
index 661f51d23c5..9f8e2979ab2 100644
--- a/gas/ginsn.c
+++ b/gas/ginsn.c
@@ -447,13 +447,19 @@ ginsn_indirect_jump_p (ginsnS *ginsn)
 static bool
 ginsn_direct_local_jump_p (ginsnS *ginsn)
 {
-  bool ret_p = false;
+  bool local_p = false;
+  const symbolS *taken_label;
+
   if (!ginsn)
-    return ret_p;
+    return local_p;
 
-  ret_p |= (ginsn->type == GINSN_TYPE_JUMP
-	    && ginsn->src[0].type == GINSN_SRC_SYMBOL);
-  return ret_p;
+  if (ginsn->type == GINSN_TYPE_JUMP
+      && ginsn->src[0].type == GINSN_SRC_SYMBOL)
+    {
+      taken_label = ginsn->src[0].sym;
+      local_p |= (label_ginsn_map_find (taken_label) != NULL);
+    }
+  return local_p;
 }
 
 static char *
@@ -785,16 +791,13 @@ add_bb_at_ginsn (const symbolS *func, gcfgS *gcfg, ginsnS *ginsn, gbbS *prev_bb,
 	  || ginsn->type == GINSN_TYPE_JUMP_COND
 	  || ginsn->type == GINSN_TYPE_RETURN)
 	{
-	  /* Indirect Jumps or direct jumps to symbols non-local to the
-	     function must not be seen here.  The caller must have already
-	     checked for that.  */
+	  /* Indirect jumps must not be seen here.  The caller must have
+	     already checked for that.  */
 	  gas_assert (!ginsn_indirect_jump_p (ginsn));
-	  if (ginsn->type == GINSN_TYPE_JUMP)
-	    gas_assert (ginsn_direct_local_jump_p (ginsn));
 
-	  /* Direct Jumps.  May include conditional or unconditional change of
-	     flow.  What is important for CFG creation is that the target be
-	     local to function.  */
+	  /* Handle direct jumps.  For unconditional direct jumps, where the
+	     target is not local to the function, treat them later as similar
+	     to an exit from function (in the else block).  */
 	  if (ginsn->type == GINSN_TYPE_JUMP_COND
 	      || ginsn_direct_local_jump_p (ginsn))
 	    {
@@ -822,10 +825,14 @@ add_bb_at_ginsn (const symbolS *func, gcfgS *gcfg, ginsnS *ginsn, gbbS *prev_bb,
 	      /* Add the bb for the fall through path.  */
 	      find_or_make_bb (func, gcfg, ginsn->next, prev_bb, errp);
 	    }
-	 else if (ginsn->type == GINSN_TYPE_RETURN)
+	 else
 	   {
-	     /* We'll come back to the ginsns following GINSN_TYPE_RETURN
-		from another path if they are indeed reachable code.  */
+	     gas_assert (ginsn->type == GINSN_TYPE_RETURN
+			 || (ginsn->type == GINSN_TYPE_JUMP
+			     && !ginsn_direct_local_jump_p (ginsn)));
+	     /* We'll come back to the ginsns following GINSN_TYPE_RETURN or
+		other (non-local) unconditional jmps from another path if they
+		are indeed reachable code.  */
 	     break;
 	   }
 
@@ -1083,9 +1090,7 @@ frch_ginsn_data_append (ginsnS *ginsn)
     {
       temp->id = ++id;
 
-      if (ginsn_indirect_jump_p (temp)
-	  || (ginsn->type == GINSN_TYPE_JUMP
-	      && !ginsn_direct_local_jump_p (temp)))
+      if (ginsn_indirect_jump_p (temp))
 	frchain_now->frch_ginsn_data->gcfg_apt_p = false;
 
       if (listing & LISTING_GINSN_SCFI)
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
new file mode 100644
index 00000000000..b373653c748
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
@@ -0,0 +1,24 @@
+#as: --scfi=experimental -W
+#as:
+#objdump: -Wf
+#name: Synthesize CFI in presence of control flow 3
+#...
+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:     1b
+  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+0014 0+001c FDE cie=00000000 pc=0000000000000000..000000000000001c
+  DW_CFA_nop
+#...
+
+#pass
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
new file mode 100644
index 00000000000..5ff6048bef0
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
@@ -0,0 +1,2 @@
+.*Assembler messages:
+.*8: Warning: SCFI ignores most user-specified CFI directives
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
new file mode 100644
index 00000000000..ac63af892cb
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
@@ -0,0 +1,21 @@
+# Testcase with jmp to function instead of a call.
+# The CFG creation process is not expected to warn about
+# missing foo_handler_v1 or xstrdup.
+	.text
+	.globl	foo_handler
+	.type	foo_handler, @function
+foo_handler:
+	.cfi_startproc
+	movl	current_style(%rip), %eax
+	cmpl	$-1, %eax
+	je	.L5
+	testb	$4, %al
+	je	.L3
+	jmp	foo_handler_v1
+.L3:
+	xorl	%eax, %eax
+	ret
+.L5:
+	jmp	xstrdup
+	.cfi_endproc
+	.size	foo_handler, .-foo_handler
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
index 5324af386f8..c004d99b4ac 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
@@ -78,6 +78,8 @@ if  { ([istarget "x86_64-*-*"] && ![istarget "x86_64-*-linux*-gnux32"]) } then {
     run_list_test "scfi-cfg-1" "--scfi=experimental --warn"
     run_dump_test "scfi-cfg-2"
     run_list_test "scfi-cfg-2" "--scfi=experimental --warn"
+    run_dump_test "scfi-cfg-3"
+    run_list_test "scfi-cfg-3" "--scfi=experimental --warn"
     run_dump_test "scfi-asm-marker-1"
     run_list_test "scfi-asm-marker-1" "--scfi=experimental --warn"
     run_dump_test "scfi-asm-marker-2"
-- 
2.43.0

Re: [PING][PATCH] gas: gcfg: fix handling of non-local direct jmps in gcfg

[Jan Beulich]

On 27.03.2024 07:53, Indu Bhagat wrote:
> The ginsn infrastructure in GAS includes the ability to create a GCFG
> (ginsn CFG).  A GCFG is currently used for SCFI passes.
> 
> This patch fixes the following invalid assumptions / code blocks:
>  - The function ginsn_direct_local_jump_p () was erroneously _not_
>    checking whether the symbol is locally defined (i.e., within the
>    scope of the code block for which GCFG is desired).  Fix the code
>    to do so.
>  - Similarly, the GCFG creation code, in gcfg_build () itself had an
>    assumption that a GINSN_TYPE_JUMP to a non-local symbol will not be
>    seen.  The latter can indeed be seen, and in fact, needs to be treated
>    the same way as an exit from the function in terms of control-flow.
> 
> gas/
>         * ginsn.c (ginsn_direct_local_jump_p): Check if the symbol
> 	is local to the code block or function being assembled.
>         (add_bb_at_ginsn): Remove buggy assumption.
>         (frch_ginsn_data_append): Direct jmps do not disqualify a stream
> 	of ginsns from GCFG creation.
> 
> gas/testsuite/
> 	* gas/scfi/x86_64/scfi-cfg-3.d: New test.
> 	* gas/scfi/x86_64/scfi-cfg-3.l: New test.
> 	* gas/scfi/x86_64/scfi-cfg-3.s: New test.
> 	* gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
> ---
>  gas/ginsn.c                                   | 43 +++++++++++--------
>  gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d    | 24 +++++++++++
>  gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l    |  2 +
>  gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s    | 21 +++++++++
>  gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  2 +
>  5 files changed, 73 insertions(+), 19 deletions(-)
>  create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
>  create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
>  create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
> 
> diff --git a/gas/ginsn.c b/gas/ginsn.c
> index 661f51d23c5..9f8e2979ab2 100644
> --- a/gas/ginsn.c
> +++ b/gas/ginsn.c
> @@ -447,13 +447,19 @@ ginsn_indirect_jump_p (ginsnS *ginsn)
>  static bool
>  ginsn_direct_local_jump_p (ginsnS *ginsn)
>  {
> -  bool ret_p = false;
> +  bool local_p = false;
> +  const symbolS *taken_label;
> +
>    if (!ginsn)
> -    return ret_p;
> +    return local_p;
>  
> -  ret_p |= (ginsn->type == GINSN_TYPE_JUMP
> -	    && ginsn->src[0].type == GINSN_SRC_SYMBOL);
> -  return ret_p;
> +  if (ginsn->type == GINSN_TYPE_JUMP
> +      && ginsn->src[0].type == GINSN_SRC_SYMBOL)
> +    {
> +      taken_label = ginsn->src[0].sym;
> +      local_p |= (label_ginsn_map_find (taken_label) != NULL);

The new testcase covers just one of two possible cases, if I'm not mistaken.
Besides JMPing to an external label, wouldn't you want to also cover the
case of JMPing to a label in the same file (i.e. defined at least by the
end of parsing all of the source file)? From its name alone it isn't clear,
after all, whether label_ginsn_map_find() would treat such one way or the
other. And even looking at the function doesn't clarify that, without
further trying to understand what scope the consulted hash table covers.

Also, nit: Any reason to use |= here, when (afaict) = would do fine?

Jan

Re: [PING][PATCH] gas: gcfg: fix handling of non-local direct jmps in gcfg

[Indu Bhagat]

On 3/27/24 00:05, Jan Beulich wrote:
> On 27.03.2024 07:53, Indu Bhagat wrote:
>> The ginsn infrastructure in GAS includes the ability to create a GCFG
>> (ginsn CFG).  A GCFG is currently used for SCFI passes.
>>
>> This patch fixes the following invalid assumptions / code blocks:
>>   - The function ginsn_direct_local_jump_p () was erroneously _not_
>>     checking whether the symbol is locally defined (i.e., within the
>>     scope of the code block for which GCFG is desired).  Fix the code
>>     to do so.
>>   - Similarly, the GCFG creation code, in gcfg_build () itself had an
>>     assumption that a GINSN_TYPE_JUMP to a non-local symbol will not be
>>     seen.  The latter can indeed be seen, and in fact, needs to be treated
>>     the same way as an exit from the function in terms of control-flow.
>>
>> gas/
>>          * ginsn.c (ginsn_direct_local_jump_p): Check if the symbol
>> 	is local to the code block or function being assembled.
>>          (add_bb_at_ginsn): Remove buggy assumption.
>>          (frch_ginsn_data_append): Direct jmps do not disqualify a stream
>> 	of ginsns from GCFG creation.
>>
>> gas/testsuite/
>> 	* gas/scfi/x86_64/scfi-cfg-3.d: New test.
>> 	* gas/scfi/x86_64/scfi-cfg-3.l: New test.
>> 	* gas/scfi/x86_64/scfi-cfg-3.s: New test.
>> 	* gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
>> ---
>>   gas/ginsn.c                                   | 43 +++++++++++--------
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d    | 24 +++++++++++
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l    |  2 +
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s    | 21 +++++++++
>>   gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  2 +
>>   5 files changed, 73 insertions(+), 19 deletions(-)
>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
>>
>> diff --git a/gas/ginsn.c b/gas/ginsn.c
>> index 661f51d23c5..9f8e2979ab2 100644
>> --- a/gas/ginsn.c
>> +++ b/gas/ginsn.c
>> @@ -447,13 +447,19 @@ ginsn_indirect_jump_p (ginsnS *ginsn)
>>   static bool
>>   ginsn_direct_local_jump_p (ginsnS *ginsn)
>>   {
>> -  bool ret_p = false;
>> +  bool local_p = false;
>> +  const symbolS *taken_label;
>> +
>>     if (!ginsn)
>> -    return ret_p;
>> +    return local_p;
>>   
>> -  ret_p |= (ginsn->type == GINSN_TYPE_JUMP
>> -	    && ginsn->src[0].type == GINSN_SRC_SYMBOL);
>> -  return ret_p;
>> +  if (ginsn->type == GINSN_TYPE_JUMP
>> +      && ginsn->src[0].type == GINSN_SRC_SYMBOL)
>> +    {
>> +      taken_label = ginsn->src[0].sym;
>> +      local_p |= (label_ginsn_map_find (taken_label) != NULL);
> 
> The new testcase covers just one of two possible cases, if I'm not mistaken.
> Besides JMPing to an external label, wouldn't you want to also cover the
> case of JMPing to a label in the same file (i.e. defined at least by the
> end of parsing all of the source file)? From its name alone it isn't clear,
> after all, whether label_ginsn_map_find() would treat such one way or the
> other. And even looking at the function doesn't clarify that, without
> further trying to understand what scope the consulted hash table covers.
> 

Sure, adding such a jmp is good to make the testcase complete.

I have now added the following to the testcase:

      jmp   .L2
.L2:
      ...

The above should cover for the case when there is a unconditional jump 
to a label defined in the block of insns currently being process for 
GCFG creation. I have checked that the GCFG contains the edge to the 
successor bb.

I added a function level comment for ginsn_direct_local_jump_p function:

/* Return whether the GINSN is an unconditional jump to a label which is 
defined locally in the scope of the block of insns, which are currently 
being processed for GCFG creation.  */

static bool ginsn_direct_local_jump_p (ginsnS *ginsn)

> Also, nit: Any reason to use |= here, when (afaict) = would do fine?
> 

Thanks for reviewing. I switched to = here.

Re: [PING][PATCH] gas: gcfg: fix handling of non-local direct jmps in gcfg

[Jan Beulich]

On 27.03.2024 22:58, Indu Bhagat wrote:
> On 3/27/24 00:05, Jan Beulich wrote:
>> On 27.03.2024 07:53, Indu Bhagat wrote:
>>> The ginsn infrastructure in GAS includes the ability to create a GCFG
>>> (ginsn CFG).  A GCFG is currently used for SCFI passes.
>>>
>>> This patch fixes the following invalid assumptions / code blocks:
>>>   - The function ginsn_direct_local_jump_p () was erroneously _not_
>>>     checking whether the symbol is locally defined (i.e., within the
>>>     scope of the code block for which GCFG is desired).  Fix the code
>>>     to do so.
>>>   - Similarly, the GCFG creation code, in gcfg_build () itself had an
>>>     assumption that a GINSN_TYPE_JUMP to a non-local symbol will not be
>>>     seen.  The latter can indeed be seen, and in fact, needs to be treated
>>>     the same way as an exit from the function in terms of control-flow.
>>>
>>> gas/
>>>          * ginsn.c (ginsn_direct_local_jump_p): Check if the symbol
>>>     is local to the code block or function being assembled.
>>>          (add_bb_at_ginsn): Remove buggy assumption.
>>>          (frch_ginsn_data_append): Direct jmps do not disqualify a stream
>>>     of ginsns from GCFG creation.
>>>
>>> gas/testsuite/
>>>     * gas/scfi/x86_64/scfi-cfg-3.d: New test.
>>>     * gas/scfi/x86_64/scfi-cfg-3.l: New test.
>>>     * gas/scfi/x86_64/scfi-cfg-3.s: New test.
>>>     * gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
>>> ---
>>>   gas/ginsn.c                                   | 43 +++++++++++--------
>>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d    | 24 +++++++++++
>>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l    |  2 +
>>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s    | 21 +++++++++
>>>   gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  2 +
>>>   5 files changed, 73 insertions(+), 19 deletions(-)
>>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
>>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
>>>   create mode 100644 gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
>>>
>>> diff --git a/gas/ginsn.c b/gas/ginsn.c
>>> index 661f51d23c5..9f8e2979ab2 100644
>>> --- a/gas/ginsn.c
>>> +++ b/gas/ginsn.c
>>> @@ -447,13 +447,19 @@ ginsn_indirect_jump_p (ginsnS *ginsn)
>>>   static bool
>>>   ginsn_direct_local_jump_p (ginsnS *ginsn)
>>>   {
>>> -  bool ret_p = false;
>>> +  bool local_p = false;
>>> +  const symbolS *taken_label;
>>> +
>>>     if (!ginsn)
>>> -    return ret_p;
>>> +    return local_p;
>>>   -  ret_p |= (ginsn->type == GINSN_TYPE_JUMP
>>> -        && ginsn->src[0].type == GINSN_SRC_SYMBOL);
>>> -  return ret_p;
>>> +  if (ginsn->type == GINSN_TYPE_JUMP
>>> +      && ginsn->src[0].type == GINSN_SRC_SYMBOL)
>>> +    {
>>> +      taken_label = ginsn->src[0].sym;
>>> +      local_p |= (label_ginsn_map_find (taken_label) != NULL);
>>
>> The new testcase covers just one of two possible cases, if I'm not mistaken.
>> Besides JMPing to an external label, wouldn't you want to also cover the
>> case of JMPing to a label in the same file (i.e. defined at least by the
>> end of parsing all of the source file)? From its name alone it isn't clear,
>> after all, whether label_ginsn_map_find() would treat such one way or the
>> other. And even looking at the function doesn't clarify that, without
>> further trying to understand what scope the consulted hash table covers.
>>
> 
> Sure, adding such a jmp is good to make the testcase complete.
> 
> I have now added the following to the testcase:
> 
>      jmp   .L2
> .L2:
>      ...
> 
> The above should cover for the case when there is a unconditional jump to a label defined in the block of insns currently being process for GCFG creation. I have checked that the GCFG contains the edge to the successor bb.

That's useful too, but not what I asked for. What I meant was a JMP to
a label defined in the same source file, but outside the block of insns
currently being processed (e.g. into another, separate function).

Jan