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

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

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.