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

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

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

> This does not seem to be a problem for exception handling tables, but
> the return_pc attribute in the call graph output in dwarf2+ debug
> information, that takes the address of a label output right after the
> call, does not match the value of the link register even for non-tail
> calls.

And why would it?  I think you mean something in the existing code
expects that, but please be explicit?

Subtracting 4 from what we currently return is very fragile.  The actual
return address is *always* the address of the branch insn plus 4, can't
you use that?  That is true for all architectures with a link register,
not just rs6000, fwiw (for some calue of "4" of course).

> E.g., with ABI_AIX or ABI_ELFv2, such code as:
> 
>   foo ();
> 
> outputs:
> 
>   bl foo
>   nop
>  LVL#:
> [...]
>   .8byte .LVL#  # DW_AT_call_return_pc
> 
> but debug info consumers may rely on the return_pc address, and draw
> incorrect conclusions from its off-by-4 value.

(Does the GCC code handle delay slots here, btw?  That is unrelated of
course.)

> 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"?  This is confusing on so many levels; worst it
suggests LK=1 opcodes (like bl: any branch that sets the link register),
which is not what this is about ;-)

> 	* target.def (call_offset_return_label): New hook.
> 	* gcc/doc/tm.texi.in (TARGET_CALL_OFFSET_RETURN_LABEL): Add
> 	placeholder.
> 	* gcc/doc/tm.texi: Rebuild.
> 	* dwarf2out.cc (struct call_arg_loc_node): Record call_insn
> 	instad of call_arg_loc_note.
> 	(add_AT_lbl_id): Add optional offset argument.
> 	(gen_call_site_die): Compute and pass on a return pc offset.
> 	(gen_subprogram_die): Move call_arg_loc_note computation...
> 	(dwarf2out_var_location): ... from here.  Set call_insn.
> 	* config/rs6000/rs6000.cc (TARGET_CALL_OFFSET_RETURN_LABEL):
> 	Override.
> 	(rs6000_call_offset_return_label): New.
> 	* config/rs6000/rs6000.md (call_needs_return_offset): New
> 	attribute.  Set it on call patterns that may require
> 	offsetting.

It is much nicer to review if you split the generic and target parts (it
makes it a better patch series in general, too, not accidentally).

> +/* 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.  Just delete the comment?  But,
is the assert useful at all anyway, won't the callers have checked for
this already?

> +  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 :-)

> +  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?

> +  else if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)

(No else after returns please).

> +    /* rs6000_call_template_1 outputs a nop after non-sibcall insns;
> +       we mark sibcall insns with NONE rather than DIRECT, so we
> +       should have returned zero above.

Please comment this *there*!

> +       rs6000_indirect_call_template_1 outputs an l insn after
> +       indirect calls in these ABIs.  */
> +    return -4;

The general point is that there is *some* insn, so that the linker (or
dynamic linker) can fix it up when needed.  That we already output an
ld or lwz (not "l", which is an ancient POWER insn, btw) there is not so
important.

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

> +;; Calls that output insns after bl need DW_AT_call_return_pc to be
> +;; adjusted.  rs6000_call_offset_return_label uses this attribute to
> +;; conservatively recognize the relevant patterns.
> +(define_attr "call_needs_return_offset" "none,direct,indirect"
> +  (const_string "none"))

Like I said above, this is all just because of a misdesign here: we
should calculate the return address from first principles, not try to
undo adding all the stuff we wrongly did.

This attribute should not exist.


Segher