Re: [PATCH] [rs6000] adjust return_pc debug attrs

[Alexandre Oliva <oliva@adacore.com>]

On Mar 13, 2023, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!
> This is stage 1 stuff (or does it fix some regression or such?)

> On Fri, Mar 03, 2023 at 03:00:02PM -0300, Alexandre Oliva wrote:
>> Some of the rs6000 call patterns, on some ABIs, issue multiple opcodes
>> out of a single call insn, but the call (bl) or jump (b) is not always
>> the last opcode in the sequence.

> Yes.  On most architectures you can get multiple machine instructions of
> course (for long calls for example), but on rs6000 (with some ABIs, in
> some circumstances) we generate a nop insn after calls, so that the
> linker has a spot to insert fixup code after calls (typically to restore
> the r2 contents, but it could be anything).

Thanks.  I wasn't entirely sure that the linker would never ever relax
the call sequence, moving the bl to the second instruction in the pair,
or that no 64-bit call variant existed or would come into existence.


> Subtracting 4 from what we currently return is very fragile.

Agreed.

> The actual return address is *always* the address of the branch insn
> plus 4, can't you use that?

Yup, given this piece of knowledge I didn't have, I agree that's a far
saner approach.  I'll post a new version of the patch, now broken up
into rs6000-specific and machine-independent, momentarily.

> (Does the GCC code handle delay slots here, btw?

It does, in that the label is output after the insn sequence.

>> This patch introduces infrastructure for targets to add an offset to
>> the label issued after the call_insn to set the call_return_pc
>> attribute, and uses that on rs6000 to account for nop and l opcodes
>> issued after actual call opcode as part of call insns output patterns.

> What is an "l opcode"?

I have a vague recollection of seeing call sequences ended by loads.
Ah, yes, rs6000_indirect_call_template_1 outputs ld or lwz, depending on
TARGET_64BIT, in both speculate and non-speculate cases after the branch
in ABI_AIX and ABI_ELFv2 calls.  I understand the l in ld and lwz stands
for 'load', and so I meant 'load' updates, but I guess in the context of
calls the 'l' can indeed be misleading.  Anyway, that complexity is gone
thanks to your suggestion.

>> +/* Return the offset to be added to the label output after CALL_INSN
>> +   to compute the address to be placed in DW_AT_call_return_pc.  Some
>> +   call insns output nop or l after bl, so the return address would be
>> +   wrong without this offset.  */
>> +
>> +static int
>> +rs6000_call_offset_return_label (rtx_insn *call_insn)
>> +{
>> +  /* We don't expect SEQUENCEs in this port.  */
>> +  gcc_checking_assert (GET_CODE (call_insn) == CALL_INSN);

> That is not doing what the comment says.

It is.  The documented interface, in the .def file, states that it must
be either a CALL_INSN or a SEQUENCE.

> But, is the assert useful at all anyway, won't the callers have
> checked for this already?

No, the callers would let a SEQUENCE through, if there was one.  But
rs6000 won't ever see one, because there are no delay slots.

My rationale to put it in was to (i) confirm that the case of SEQUENCEs
was considered and needs not be handled in this port, and (ii) should
someone take inspiration from this implementation of the hook for a port
that supported delay slots, it would have to be handled.


>> +  enum attr_call_needs_return_offset cnro
>> +    = get_attr_call_needs_return_offset (call_insn);
>> +
>> +  if (cnro == CALL_NEEDS_RETURN_OFFSET_NONE)
>> +    return 0;

>   if (get_attr_call_needs_return_offset (call_insn)
>       == CALL_NEEDS_RETURN_OFFSET_NONE)
>     return 0;

> Shorter, simpler, doesn't need a variable for which you have no good
> name :-)

I happen to not share your subjective preference, but I don't mind
following that style in code you maintain.


>> +  if (rs6000_pcrel_p ())
>> +    return 0;

> Why this?  Please look at what actual code is generated, don't assume
> you don't need anything here if we generate pcrel code?

It was not an assumption, I took the conditions from the code, both from
output functions in rs6000.cc and from call patterns in rs6000.md.

Both rs6000_call_template_1 and rs6000_indirect_call_template_1 test for
rs6000_pcrel_p() and then output b or bl opcodes without any subsequence
instruction.  This test mirrors those.



>> +  else if (DEFAULT_ABI == ABI_V4)
>> +    return 0;
>> +  else if (DEFAULT_ABI == ABI_DARWIN)
>> +    return 0;
>> +  else
>> +    return 0;
>> +}

> The first two of these are superfluous.

Of course.  But mirroring the structure of the corresponding code makes
it easier to understand, and to check that the correspondence is there.
But now that style aspect is irrelevant, it's obviated by your suggested
alternate implementation.

Thanks,

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>