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

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

On 3/28/24 03:10, Jan Beulich wrote:
> On 28.03.2024 08:59, Indu Bhagat wrote:
>> [Changes in V2]
>>    - Add function level comments for ginsn_direct_local_jump_p.
>>    - Add a jmp to locally defined label in the testcase to make it more
>>      complete.  Also, add a jmp to label defined in another function.
>>      The testcase now generates an additional warning (as expected):
>>      "Warning: GINSN: found unreachable code in func 'bar'"
> 
> For this - can't you simply omit the unreachable pair of insns?
> 

We do omit those instructions from GCFG and SCFI.  GAS does, however, 
warn for now. Do you mean skip this warning too?

> Other than that, looks plausible, so feel free to put in.
> 
>>    - Use formatting style consistent with other testcases.
>> [End of changes in V2]
> 
> As a minor remark, this entire section would better go ...
> 

Yes, I usually get rid of this before committing.  But I agree, its 
better put below after the separator.

>> 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: gcfg: address review comments for local jump issue
>>
>> ChangeLog:
>> 	* gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d
>> 	* gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l
>> 	* gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s
>> ---
>>   gas/ginsn.c                                   | 47 +++++++++++--------
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.d    | 32 +++++++++++++
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.l    |  3 ++
>>   gas/testsuite/gas/scfi/x86_64/scfi-cfg-3.s    | 43 +++++++++++++++++
>>   gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  2 +
>>   5 files changed, 108 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
> 
> ... below here, perhaps with another --- separator. This way it's clear
> that the revlog isn't going to be part of the commit message.
> 
> Jan