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

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, Mar 23, 2023 at 12:06:39AM -0300, Alexandre Oliva wrote:
> On Mar 13, 2023, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > 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,

That would give headaches with maximal offsets, but more importantly
perhaps, many tools depend on knowing exactly where the branches are,
and some actual (assembler) code as well.  It's best not to toy with
branches :-)

> or that no 64-bit call variant existed or would come into existence.

64-bit offsets will not fit in the opcode of course, not even with
prefixes.  And a normal indirect branch *is* to a 64 bit address too.

Making prefixed branch instructions would get us in much the same hot
water (but everywhere instead of just in select cases), and our (I-form)
branches reach 32MB in either direction already (for relative branches;
and you can reach to the low or high 32MB of the address space too, but
not much code uses that at all anyway.  Terrible shame, not in the least
because the extended mnemonics for that are "ba" and "bla").

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

Very glad we agree on that!

> > 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.

It is the same in any other arch I have seen that has an LR or RA reg.

> I'll post a new version of the patch, now broken up
> into rs6000-specific and machine-independent, momentarily.

Looking forward to it.

> > (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.

Ah, "l%s", I see.  On the old POWER ("RIOS") architecture we *did* have
an actual "l" instruction.  There also is the "@l" relocation syntax.
I wasn't quite sure what you were talking about :-)

> >> +/* 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.

Ah, in that sense.  But that is more confusing than just not saying
anything imo.

> 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.

Yeah.  Blindly copying code from annother port is a recipe for disaster
always.  It is much nicer to not say anything / to not have to say
anything, just let the code speak for itself?


Segher