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

[Jan Beulich <jbeulich@suse.com>]

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