[PATCH v2 #1/2] enable adjustment of return_pc debug attrs

[PATCH v2 #1/2] enable adjustment of return_pc debug attrs

[Alexandre Oliva, Alexandre Oliva]

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.  This will be used on rs6000, that sometimes issues another
instruction after the call proper as part of a call insn.

Regstrapped on x86_64-linux-gnu and ppc64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* 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.
---
 gcc/doc/tm.texi    |    7 +++++++
 gcc/doc/tm.texi.in |    2 ++
 gcc/dwarf2out.cc   |   26 +++++++++++++++++---------
 gcc/target.def     |    9 +++++++++
 4 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index c4a92a5ebee90..44e4c18ad5c0a 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -5426,6 +5426,13 @@ except the last are treated as named.
 You need not define this hook if it always returns @code{false}.
 @end deftypefn
 
+@deftypefn {Target Hook} int TARGET_CALL_OFFSET_RETURN_LABEL (rtx_insn *@var{call_insn})
+While generating call-site debug info for a CALL insn, or a SEQUENCE
+insn starting with a CALL, this target hook is invoked to compute the
+offset to be added to the debug label emitted after the call to obtain
+the return address that should be recorded as the return PC.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_CALL_ARGS (rtx, @var{tree})
 While generating RTL for a function call, this target hook is invoked once
 for each argument passed to the function, either a register returned by
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 4075e71624c04..68856978fe364 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -3785,6 +3785,8 @@ These machine description macros help implement varargs:
 
 @hook TARGET_STRICT_ARGUMENT_NAMING
 
+@hook TARGET_CALL_OFFSET_RETURN_LABEL
+
 @hook TARGET_CALL_ARGS
 
 @hook TARGET_END_CALL_ARGS
diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 1711ad2c2da31..d1db918c43900 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -3584,7 +3584,7 @@ typedef struct var_loc_list_def var_loc_list;
 
 /* Call argument location list.  */
 struct GTY ((chain_next ("%h.next"))) call_arg_loc_node {
-  rtx GTY (()) call_arg_loc_note;
+  rtx_insn * GTY (()) call_insn;
   const char * GTY (()) label;
   tree GTY (()) block;
   bool tail_call_p;
@@ -3768,7 +3768,8 @@ static void remove_addr_table_entry (addr_table_entry *);
 static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool);
 static inline rtx AT_addr (dw_attr_node *);
 static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *);
-static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *);
+static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *,
+			   int = 0);
 static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *);
 static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *);
 static void add_AT_range_list (dw_die_ref, enum dwarf_attribute,
@@ -5327,14 +5328,17 @@ add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind,
 
 static inline void
 add_AT_lbl_id (dw_die_ref die, enum dwarf_attribute attr_kind,
-               const char *lbl_id)
+	       const char *lbl_id, int offset)
 {
   dw_attr_node attr;
 
   attr.dw_attr = attr_kind;
   attr.dw_attr_val.val_class = dw_val_class_lbl_id;
   attr.dw_attr_val.val_entry = NULL;
-  attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
+  if (!offset)
+    attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
+  else
+    attr.dw_attr_val.v.val_lbl_id = xasprintf ("%s%+i", lbl_id, offset);
   if (dwarf_split_debug_info)
     attr.dw_attr_val.val_entry
         = add_addr_table_entry (attr.dw_attr_val.v.val_lbl_id,
@@ -23405,7 +23409,9 @@ gen_call_site_die (tree decl, dw_die_ref subr_die,
   if (stmt_die == NULL)
     stmt_die = subr_die;
   die = new_die (dwarf_TAG (DW_TAG_call_site), stmt_die, NULL_TREE);
-  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc), ca_loc->label);
+  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc),
+		 ca_loc->label,
+		 targetm.calls.call_offset_return_label (ca_loc->call_insn));
   if (ca_loc->tail_call_p)
     add_AT_flag (die, dwarf_AT (DW_AT_call_tail_call), 1);
   if (ca_loc->symbol_ref)
@@ -24092,11 +24098,14 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 	    {
 	      dw_die_ref die = NULL;
 	      rtx tloc = NULL_RTX, tlocc = NULL_RTX;
+	      rtx call_arg_loc_note
+		= find_reg_note (ca_loc->call_insn,
+				 REG_CALL_ARG_LOCATION, NULL_RTX);
 	      rtx arg, next_arg;
 	      tree arg_decl = NULL_TREE;
 
-	      for (arg = (ca_loc->call_arg_loc_note != NULL_RTX
-			  ? XEXP (ca_loc->call_arg_loc_note, 0)
+	      for (arg = (call_arg_loc_note != NULL_RTX
+			  ? XEXP (call_arg_loc_note, 0)
 			  : NULL_RTX);
 		   arg; arg = next_arg)
 		{
@@ -28125,8 +28134,7 @@ create_label:
 	= ggc_cleared_alloc<call_arg_loc_node> ();
       rtx_insn *prev = call_insn;
 
-      ca_loc->call_arg_loc_note
-	= find_reg_note (call_insn, REG_CALL_ARG_LOCATION, NULL_RTX);
+      ca_loc->call_insn = call_insn;
       ca_loc->next = NULL;
       ca_loc->label = last_label;
       gcc_assert (prev
diff --git a/gcc/target.def b/gcc/target.def
index f401fe148ee2d..94c0e22819687 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -4731,6 +4731,15 @@ not generate any instructions in this case.",
 	int *pretend_args_size, int second_time),
  default_setup_incoming_varargs)
 
+DEFHOOK
+(call_offset_return_label,
+ "While generating call-site debug info for a CALL insn, or a SEQUENCE\n\
+insn starting with a CALL, this target hook is invoked to compute the\n\
+offset to be added to the debug label emitted after the call to obtain\n\
+the return address that should be recorded as the return PC.",
+ int, (rtx_insn *call_insn),
+ hook_int_rtx_insn_0)
+
 DEFHOOK
 (call_args,
  "While generating RTL for a function call, this target hook is invoked once\n\

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

[PATCH v2 #2/2] [rs6000] adjust return_pc debug attrs

[Alexandre Oliva]

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.

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

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

Regstrapped on x86_64-linux-gnu and ppc64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* config/rs6000/rs6000.cc (TARGET_CALL_OFFSET_RETURN_LABEL):
	Override.
	(rs6000_call_offset_return_label): New.
---
 gcc/config/rs6000/rs6000.cc |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 8e0b0d022db2f..e55117159b270 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -1760,6 +1760,9 @@ static const struct attribute_spec rs6000_attribute_table[] =
 
 #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
 #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
+
+#undef TARGET_CALL_OFFSET_RETURN_LABEL
+#define TARGET_CALL_OFFSET_RETURN_LABEL rs6000_call_offset_return_label
 \f
 
 /* Processor table.  */
@@ -14593,6 +14596,22 @@ rs6000_assemble_integer (rtx x, unsigned int size, int aligned_p)
   return default_assemble_integer (x, size, aligned_p);
 }
 
+/* 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.  */
+
+static int
+rs6000_call_offset_return_label (rtx_insn *call_insn)
+{
+  /* All rs6000 CALL_INSN output patterns start with a b or bl, always
+     a 4-byte instruction, but some output patterns issue other
+     opcodes afterwards.  The return label is issued after the entire
+     call insn, including any such post-call opcodes.  Instead of
+     figuring out which cases need adjustments, we compute the offset
+     back to the address of the call opcode proper, then add the
+     constant 4 bytes, to get the address after that opcode.  */
+  return 4 - get_attr_length (call_insn);
+}
+
 /* Return a template string for assembly to emit when making an
    external call.  FUNOP is the call mem argument operand number.  */
 

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

Re: [PATCH v2 #1/2] enable adjustment of return_pc debug attrs

[Alexandre Oliva]

On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> 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.  This will be used on rs6000, that sometimes issues another
> instruction after the call proper as part of a call insn.

Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614452.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614453.html

This is a new feature, but it's needed to avoid regressions with the
recently-released GDB on ppc64.

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

Re: [PATCH v2 #1/2] enable adjustment of return_pc debug attrs

[Alexandre Oliva]

On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>> 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.  This will be used on rs6000, that sometimes issues another
>> instruction after the call proper as part of a call insn.

> Ping?
> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614452.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614453.html

Ping?

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

[PATCH v3 #1/2] enable adjustment of return_pc debug attrs

[Alexandre Oliva]

On Apr 27, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>> 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.  This will be used on rs6000, that sometimes issues another
>>> instruction after the call proper as part of a call insn.

>> Ping?
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614452.html

Ping?
Refreshed, retested on ppc64le-linux-gnu.  Ok to install?


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.  This will be used on rs6000, that sometimes issues another
instruction after the call proper as part of a call insn.


for  gcc/ChangeLog

	* 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.
---
 gcc/doc/tm.texi    |    7 +++++++
 gcc/doc/tm.texi.in |    2 ++
 gcc/dwarf2out.cc   |   26 +++++++++++++++++---------
 gcc/target.def     |    9 +++++++++
 4 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index cd50078227d98..8a7aa70d605ba 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -5557,6 +5557,13 @@ except the last are treated as named.
 You need not define this hook if it always returns @code{false}.
 @end deftypefn
 
+@deftypefn {Target Hook} int TARGET_CALL_OFFSET_RETURN_LABEL (rtx_insn *@var{call_insn})
+While generating call-site debug info for a CALL insn, or a SEQUENCE
+insn starting with a CALL, this target hook is invoked to compute the
+offset to be added to the debug label emitted after the call to obtain
+the return address that should be recorded as the return PC.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_START_CALL_ARGS (cumulative_args_t @var{complete_args})
 This target hook is invoked while generating RTL for a function call,
 after the argument values have been computed, and after stack arguments
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 058bd56487a9a..9e0830758aeea 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -3887,6 +3887,8 @@ These machine description macros help implement varargs:
 
 @hook TARGET_STRICT_ARGUMENT_NAMING
 
+@hook TARGET_CALL_OFFSET_RETURN_LABEL
+
 @hook TARGET_START_CALL_ARGS
 
 @hook TARGET_CALL_ARGS
diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 5b064ffd78ad1..1092880738df4 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -3593,7 +3593,7 @@ typedef struct var_loc_list_def var_loc_list;
 
 /* Call argument location list.  */
 struct GTY ((chain_next ("%h.next"))) call_arg_loc_node {
-  rtx GTY (()) call_arg_loc_note;
+  rtx_insn * GTY (()) call_insn;
   const char * GTY (()) label;
   tree GTY (()) block;
   bool tail_call_p;
@@ -3777,7 +3777,8 @@ static void remove_addr_table_entry (addr_table_entry *);
 static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool);
 static inline rtx AT_addr (dw_attr_node *);
 static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *);
-static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *);
+static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *,
+			   int = 0);
 static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *);
 static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *);
 static void add_AT_range_list (dw_die_ref, enum dwarf_attribute,
@@ -5353,14 +5354,17 @@ add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind,
 
 static inline void
 add_AT_lbl_id (dw_die_ref die, enum dwarf_attribute attr_kind,
-               const char *lbl_id)
+	       const char *lbl_id, int offset)
 {
   dw_attr_node attr;
 
   attr.dw_attr = attr_kind;
   attr.dw_attr_val.val_class = dw_val_class_lbl_id;
   attr.dw_attr_val.val_entry = NULL;
-  attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
+  if (!offset)
+    attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
+  else
+    attr.dw_attr_val.v.val_lbl_id = xasprintf ("%s%+i", lbl_id, offset);
   if (dwarf_split_debug_info)
     attr.dw_attr_val.val_entry
         = add_addr_table_entry (attr.dw_attr_val.v.val_lbl_id,
@@ -23515,7 +23519,9 @@ gen_call_site_die (tree decl, dw_die_ref subr_die,
   if (stmt_die == NULL)
     stmt_die = subr_die;
   die = new_die (dwarf_TAG (DW_TAG_call_site), stmt_die, NULL_TREE);
-  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc), ca_loc->label);
+  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc),
+		 ca_loc->label,
+		 targetm.calls.call_offset_return_label (ca_loc->call_insn));
   if (ca_loc->tail_call_p)
     add_AT_flag (die, dwarf_AT (DW_AT_call_tail_call), 1);
   if (ca_loc->symbol_ref)
@@ -24202,11 +24208,14 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 	    {
 	      dw_die_ref die = NULL;
 	      rtx tloc = NULL_RTX, tlocc = NULL_RTX;
+	      rtx call_arg_loc_note
+		= find_reg_note (ca_loc->call_insn,
+				 REG_CALL_ARG_LOCATION, NULL_RTX);
 	      rtx arg, next_arg;
 	      tree arg_decl = NULL_TREE;
 
-	      for (arg = (ca_loc->call_arg_loc_note != NULL_RTX
-			  ? XEXP (ca_loc->call_arg_loc_note, 0)
+	      for (arg = (call_arg_loc_note != NULL_RTX
+			  ? XEXP (call_arg_loc_note, 0)
 			  : NULL_RTX);
 		   arg; arg = next_arg)
 		{
@@ -28251,8 +28260,7 @@ create_label:
 	= ggc_cleared_alloc<call_arg_loc_node> ();
       rtx_insn *prev = call_insn;
 
-      ca_loc->call_arg_loc_note
-	= find_reg_note (call_insn, REG_CALL_ARG_LOCATION, NULL_RTX);
+      ca_loc->call_insn = call_insn;
       ca_loc->next = NULL;
       ca_loc->label = last_label;
       gcc_assert (prev
diff --git a/gcc/target.def b/gcc/target.def
index c27df8095be15..70070caebc768 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -4890,6 +4890,15 @@ Most ports do not need to implement anything for this hook.",
  void, (cumulative_args_t complete_args),
  hook_void_CUMULATIVE_ARGS)
 
+DEFHOOK
+(call_offset_return_label,
+ "While generating call-site debug info for a CALL insn, or a SEQUENCE\n\
+insn starting with a CALL, this target hook is invoked to compute the\n\
+offset to be added to the debug label emitted after the call to obtain\n\
+the return address that should be recorded as the return PC.",
+ int, (rtx_insn *call_insn),
+ hook_int_rtx_insn_0)
+
 DEFHOOK
 (push_argument,
  "This target hook returns @code{true} if push instructions will be\n\


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

[PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Alexandre Oliva]

On Apr 27, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>> 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.  This will be used on rs6000, that sometimes issues another
>>> instruction after the call proper as part of a call insn.

>> Ping?
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614453.html

> Ping?

Ping?
Refreshed, retested on ppc64le-linux-gnu.  Ok to install?


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.

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

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


for  gcc/ChangeLog

	* config/rs6000/rs6000.cc (TARGET_CALL_OFFSET_RETURN_LABEL):
	Override.
	(rs6000_call_offset_return_label): New.
---
 gcc/config/rs6000/rs6000.cc |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index e4dc629ddcc9a..77e6b94a539da 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -1779,6 +1779,8 @@ static const scoped_attribute_specs *const rs6000_attribute_table[] =
 #undef TARGET_OVERLAP_OP_BY_PIECES_P
 #define TARGET_OVERLAP_OP_BY_PIECES_P hook_bool_void_true
 
+#undef TARGET_CALL_OFFSET_RETURN_LABEL
+#define TARGET_CALL_OFFSET_RETURN_LABEL rs6000_call_offset_return_label
 \f
 
 /* Processor table.  */
@@ -14822,6 +14824,22 @@ rs6000_assemble_integer (rtx x, unsigned int size, int aligned_p)
   return default_assemble_integer (x, size, aligned_p);
 }
 
+/* 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.  */
+
+static int
+rs6000_call_offset_return_label (rtx_insn *call_insn)
+{
+  /* All rs6000 CALL_INSN output patterns start with a b or bl, always
+     a 4-byte instruction, but some output patterns issue other
+     opcodes afterwards.  The return label is issued after the entire
+     call insn, including any such post-call opcodes.  Instead of
+     figuring out which cases need adjustments, we compute the offset
+     back to the address of the call opcode proper, then add the
+     constant 4 bytes, to get the address after that opcode.  */
+  return 4 - get_attr_length (call_insn);
+}
+
 /* Return a template string for assembly to emit when making an
    external call.  FUNOP is the call mem argument operand number.  */
 


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Kewen.Lin]

Hi,

on 2024/5/25 20:13, Alexandre Oliva wrote:
> On Apr 27, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
> 
>> On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>>> 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.  This will be used on rs6000, that sometimes issues another
>>>> instruction after the call proper as part of a call insn.
> 
>>> Ping?
>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614453.html
> 
>> Ping?
> 
> Ping?
> Refreshed, retested on ppc64le-linux-gnu.  Ok to install?
> 
> 
> 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.
> 
> 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.  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.
> 
> This patch uses the infrastructure for targets to add an offset to the
> label issued after the call_insn to set the call_return_pc attribute,
> on rs6000, to account for opcodes issued after actual call opcode as
> part of call insns output patterns.

I wonder if it's possible to have a test case for this?

BR,
Kewen

> 
> 
> for  gcc/ChangeLog
> 
> 	* config/rs6000/rs6000.cc (TARGET_CALL_OFFSET_RETURN_LABEL):
> 	Override.
> 	(rs6000_call_offset_return_label): New.
> ---
>  gcc/config/rs6000/rs6000.cc |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index e4dc629ddcc9a..77e6b94a539da 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -1779,6 +1779,8 @@ static const scoped_attribute_specs *const rs6000_attribute_table[] =
>  #undef TARGET_OVERLAP_OP_BY_PIECES_P
>  #define TARGET_OVERLAP_OP_BY_PIECES_P hook_bool_void_true
>  
> +#undef TARGET_CALL_OFFSET_RETURN_LABEL
> +#define TARGET_CALL_OFFSET_RETURN_LABEL rs6000_call_offset_return_label
>  \f
>  
>  /* Processor table.  */
> @@ -14822,6 +14824,22 @@ rs6000_assemble_integer (rtx x, unsigned int size, int aligned_p)
>    return default_assemble_integer (x, size, aligned_p);
>  }
>  
> +/* 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.  */
> +
> +static int
> +rs6000_call_offset_return_label (rtx_insn *call_insn)
> +{
> +  /* All rs6000 CALL_INSN output patterns start with a b or bl, always
> +     a 4-byte instruction, but some output patterns issue other
> +     opcodes afterwards.  The return label is issued after the entire
> +     call insn, including any such post-call opcodes.  Instead of
> +     figuring out which cases need adjustments, we compute the offset
> +     back to the address of the call opcode proper, then add the
> +     constant 4 bytes, to get the address after that opcode.  */
> +  return 4 - get_attr_length (call_insn);
> +}
> +
>  /* Return a template string for assembly to emit when making an
>     external call.  FUNOP is the call mem argument operand number.  */
>  
> 
> 

Re: [PATCH v3 #1/2] enable adjustment of return_pc debug attrs

[Jason Merrill]

On 5/25/24 08:12, Alexandre Oliva wrote:
> On Apr 27, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>> On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>>> 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.  This will be used on rs6000, that sometimes issues another
>>>> instruction after the call proper as part of a call insn.
> 
>>> Ping?
>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614452.html
> 
> Ping?
> Refreshed, retested on ppc64le-linux-gnu.  Ok to install?

I wonder about adding this information to REG_CALL_ARG_LOCATION, but 
doing it this way also seems reasonable.  I'm interested in Jakub's 
input, but the patch is OK in a week if he doesn't get to it.
> 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.  This will be used on rs6000, that sometimes issues another
> instruction after the call proper as part of a call insn.
> 
> 
> for  gcc/ChangeLog
> 
> 	* 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.
> ---
>   gcc/doc/tm.texi    |    7 +++++++
>   gcc/doc/tm.texi.in |    2 ++
>   gcc/dwarf2out.cc   |   26 +++++++++++++++++---------
>   gcc/target.def     |    9 +++++++++
>   4 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index cd50078227d98..8a7aa70d605ba 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -5557,6 +5557,13 @@ except the last are treated as named.
>   You need not define this hook if it always returns @code{false}.
>   @end deftypefn
>   
> +@deftypefn {Target Hook} int TARGET_CALL_OFFSET_RETURN_LABEL (rtx_insn *@var{call_insn})
> +While generating call-site debug info for a CALL insn, or a SEQUENCE
> +insn starting with a CALL, this target hook is invoked to compute the
> +offset to be added to the debug label emitted after the call to obtain
> +the return address that should be recorded as the return PC.
> +@end deftypefn
> +
>   @deftypefn {Target Hook} void TARGET_START_CALL_ARGS (cumulative_args_t @var{complete_args})
>   This target hook is invoked while generating RTL for a function call,
>   after the argument values have been computed, and after stack arguments
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 058bd56487a9a..9e0830758aeea 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3887,6 +3887,8 @@ These machine description macros help implement varargs:
>   
>   @hook TARGET_STRICT_ARGUMENT_NAMING
>   
> +@hook TARGET_CALL_OFFSET_RETURN_LABEL
> +
>   @hook TARGET_START_CALL_ARGS
>   
>   @hook TARGET_CALL_ARGS
> diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
> index 5b064ffd78ad1..1092880738df4 100644
> --- a/gcc/dwarf2out.cc
> +++ b/gcc/dwarf2out.cc
> @@ -3593,7 +3593,7 @@ typedef struct var_loc_list_def var_loc_list;
>   
>   /* Call argument location list.  */
>   struct GTY ((chain_next ("%h.next"))) call_arg_loc_node {
> -  rtx GTY (()) call_arg_loc_note;
> +  rtx_insn * GTY (()) call_insn;
>     const char * GTY (()) label;
>     tree GTY (()) block;
>     bool tail_call_p;
> @@ -3777,7 +3777,8 @@ static void remove_addr_table_entry (addr_table_entry *);
>   static void add_AT_addr (dw_die_ref, enum dwarf_attribute, rtx, bool);
>   static inline rtx AT_addr (dw_attr_node *);
>   static void add_AT_symview (dw_die_ref, enum dwarf_attribute, const char *);
> -static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *);
> +static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *,
> +			   int = 0);
>   static void add_AT_lineptr (dw_die_ref, enum dwarf_attribute, const char *);
>   static void add_AT_macptr (dw_die_ref, enum dwarf_attribute, const char *);
>   static void add_AT_range_list (dw_die_ref, enum dwarf_attribute,
> @@ -5353,14 +5354,17 @@ add_AT_symview (dw_die_ref die, enum dwarf_attribute attr_kind,
>   
>   static inline void
>   add_AT_lbl_id (dw_die_ref die, enum dwarf_attribute attr_kind,
> -               const char *lbl_id)
> +	       const char *lbl_id, int offset)
>   {
>     dw_attr_node attr;
>   
>     attr.dw_attr = attr_kind;
>     attr.dw_attr_val.val_class = dw_val_class_lbl_id;
>     attr.dw_attr_val.val_entry = NULL;
> -  attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
> +  if (!offset)
> +    attr.dw_attr_val.v.val_lbl_id = xstrdup (lbl_id);
> +  else
> +    attr.dw_attr_val.v.val_lbl_id = xasprintf ("%s%+i", lbl_id, offset);
>     if (dwarf_split_debug_info)
>       attr.dw_attr_val.val_entry
>           = add_addr_table_entry (attr.dw_attr_val.v.val_lbl_id,
> @@ -23515,7 +23519,9 @@ gen_call_site_die (tree decl, dw_die_ref subr_die,
>     if (stmt_die == NULL)
>       stmt_die = subr_die;
>     die = new_die (dwarf_TAG (DW_TAG_call_site), stmt_die, NULL_TREE);
> -  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc), ca_loc->label);
> +  add_AT_lbl_id (die, dwarf_AT (DW_AT_call_return_pc),
> +		 ca_loc->label,
> +		 targetm.calls.call_offset_return_label (ca_loc->call_insn));
>     if (ca_loc->tail_call_p)
>       add_AT_flag (die, dwarf_AT (DW_AT_call_tail_call), 1);
>     if (ca_loc->symbol_ref)
> @@ -24202,11 +24208,14 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
>   	    {
>   	      dw_die_ref die = NULL;
>   	      rtx tloc = NULL_RTX, tlocc = NULL_RTX;
> +	      rtx call_arg_loc_note
> +		= find_reg_note (ca_loc->call_insn,
> +				 REG_CALL_ARG_LOCATION, NULL_RTX);
>   	      rtx arg, next_arg;
>   	      tree arg_decl = NULL_TREE;
>   
> -	      for (arg = (ca_loc->call_arg_loc_note != NULL_RTX
> -			  ? XEXP (ca_loc->call_arg_loc_note, 0)
> +	      for (arg = (call_arg_loc_note != NULL_RTX
> +			  ? XEXP (call_arg_loc_note, 0)
>   			  : NULL_RTX);
>   		   arg; arg = next_arg)
>   		{
> @@ -28251,8 +28260,7 @@ create_label:
>   	= ggc_cleared_alloc<call_arg_loc_node> ();
>         rtx_insn *prev = call_insn;
>   
> -      ca_loc->call_arg_loc_note
> -	= find_reg_note (call_insn, REG_CALL_ARG_LOCATION, NULL_RTX);
> +      ca_loc->call_insn = call_insn;
>         ca_loc->next = NULL;
>         ca_loc->label = last_label;
>         gcc_assert (prev
> diff --git a/gcc/target.def b/gcc/target.def
> index c27df8095be15..70070caebc768 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -4890,6 +4890,15 @@ Most ports do not need to implement anything for this hook.",
>    void, (cumulative_args_t complete_args),
>    hook_void_CUMULATIVE_ARGS)
>   
> +DEFHOOK
> +(call_offset_return_label,
> + "While generating call-site debug info for a CALL insn, or a SEQUENCE\n\
> +insn starting with a CALL, this target hook is invoked to compute the\n\
> +offset to be added to the debug label emitted after the call to obtain\n\
> +the return address that should be recorded as the return PC.",
> + int, (rtx_insn *call_insn),
> + hook_int_rtx_insn_0)
> +
>   DEFHOOK
>   (push_argument,
>    "This target hook returns @code{true} if push instructions will be\n\
> 
> 

Re: [PATCH v3 #1/2] enable adjustment of return_pc debug attrs

[Segher Boessenkool]

On Sat, May 25, 2024 at 09:12:05AM -0300, Alexandre Oliva wrote:
<snip>

You sent multiple patch series in one thread, and multiple versions of
the same series even.

This is very hard to even *read*, let alone work with.  Please don't.


Segher

Re: [PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Segher Boessenkool]

On Sat, May 25, 2024 at 09:13:12AM -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.

> 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.  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.
> 
> This patch uses the infrastructure for targets to add an offset to the
> label issued after the call_insn to set the call_return_pc attribute,
> on rs6000, to account for opcodes issued after actual call opcode as
> part of call insns output patterns.

> for  gcc/ChangeLog
> 
> 	* config/rs6000/rs6000.cc (TARGET_CALL_OFFSET_RETURN_LABEL):
> 	Override.

Please don't (incorrectly!) line-wrap changelogs.  Lines are 80
characters wide, not 60 or 72 or whatever.  80.  Indents are tabs that
take 8 columns.

> +/* 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.  */
> +
> +static int
> +rs6000_call_offset_return_label (rtx_insn *call_insn)
> +{
> +  /* All rs6000 CALL_INSN output patterns start with a b or bl, always

This isn't true.  There is a crlogical insn before the bcl for sysv for
example.

> +     a 4-byte instruction, but some output patterns issue other
> +     opcodes afterwards.  The return label is issued after the entire
> +     call insn, including any such post-call opcodes.  Instead of
> +     figuring out which cases need adjustments, we compute the offset
> +     back to the address of the call opcode proper, then add the
> +     constant 4 bytes, to get the address after that opcode.  */
> +  return 4 - get_attr_length (call_insn);

Please explain this magic, too -- in code preferably (so with a ? :
maybe, but don't try to "optimise" that expression, let the compiler
do that, it is much better at it anyway :-) )

> +}

Is that correct for all ABIs we support?  Even if so, it needs a lot
more documentation than this.


Segher

Re: [PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Alexandre Oliva]

On May 27, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> I wonder if it's possible to have a test case for this?

gcc.dg/guality/pr54519-[34].c at -O[1g] are fixed by this patch on
ppc64le-linux-gnu.  Are these the sort of test case you're interested
in, or are you looking for something that tests the offsets in debug
info, rather than the end-to-end debugging feature?

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH v3 #2/2] [rs6000] adjust return_pc debug attrs

[Alexandre Oliva]

Sorry, I misnumbered this patch as #1/2 when first posting v3.

On May 28, 2024, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Please don't (incorrectly!) line-wrap changelogs.  Lines are 80
> characters wide, not 60 or 72 or whatever.  80.  Indents are tabs that
> take 8 columns.

ACK.  When was it bumped up to 80, BTW?  It wasn't always like that, was
it?  I've noticed that something seems to change my line width settings
in Emacs, but I must have missed that memo.

>> +/* 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.  */
>> +
>> +static int
>> +rs6000_call_offset_return_label (rtx_insn *call_insn)
>> +{
>> +  /* All rs6000 CALL_INSN output patterns start with a b or bl, always

> This isn't true.  There is a crlogical insn before the bcl for sysv for
> example.

Hmm, if that's so, this function will have to look at the insn and
recognize those cases and use a different way to compute the offset.

However, I don't see any relevant uses of bcl in output patterns for
insns containing a call rtx.  There are other uses in profiling and pic
loading and whatnot, but those don't get mentioned in the call graph in
debug info, and so they don't get this target hook called on them.

>> +                                                we compute the offset
>> +     back to the address of the call opcode proper, then add the
>> +     constant 4 bytes, to get the address after that opcode.  */
>> +  return 4 - get_attr_length (call_insn);

> Please explain this magic, too -- in code preferably (so with a ? :
> maybe, but don't try to "optimise" that expression, let the compiler
> do that, it is much better at it anyway :-) )

There's neither optimization nor magic, it's literally what's written in
the comment quoted above: starting from the label at the end of the call
insn (that's what the caller offsets from, as in the documentation in
the actual #1/2), subtract the length (to get to the address of the call
opcode), and add 4 (to get past the call opcode).  Ok, I've reordered
the two addends for an IMHO more readable expression, but that was all.

> Is that correct for all ABIs we support?  

Back when I wrote this patchset, I went through all call opcodes I could
find in the md and .c files within rs6000/, and I was satisfied that it
covered what we had then, but I won't pretend to know all about all of
the ppc ABIs.  I may have missed disguised call insns, too.  I was
hoping some ppc maintainer, more familiar with the port than I am, would
catch any trouble on review and let me know about pitfalls and surprises
to watch out for.

> Even if so, it needs a lot more documentation than this.

I can write more documentation, but I'm at a loss as to what you're
hoping for.  If you set clearer expectations, I'll be glad to oblige.

Thanks,

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Segher Boessenkool]

On Wed, May 29, 2024 at 03:52:15AM -0300, Alexandre Oliva wrote:
> On May 27, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
> > I wonder if it's possible to have a test case for this?
> 
> gcc.dg/guality/pr54519-[34].c at -O[1g] are fixed by this patch on
> ppc64le-linux-gnu.  Are these the sort of test case you're interested
> in, or are you looking for something that tests the offsets in debug
> info, rather than the end-to-end debugging feature?

A testcase specifically for this would be good, yes.  Where you can say
at the top of the file "This tests that ... [X and Y]" :-)


Segher

Re: [PATCH v3 #2/2] [rs6000] adjust return_pc debug attrs

[Segher Boessenkool]

Hi Alex,

On Thu, May 30, 2024 at 01:40:27PM -0300, Alexandre Oliva wrote:
> Sorry, I misnumbered this patch as #1/2 when first posting v3.

I see at least five completely different patches in this email thread,
with different subjects and all.

> On May 28, 2024, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> > Please don't (incorrectly!) line-wrap changelogs.  Lines are 80
> > characters wide, not 60 or 72 or whatever.  80.  Indents are tabs that
> > take 8 columns.
> 
> ACK.  When was it bumped up to 80, BTW?  It wasn't always like that, was
> it?

It always was like that.  Some people say 79, that is fine too.

It mostly irks me because lines that end in : and then a lot of empty
space look like peoople used one of those awful "write the changelog for
me" helper things, that *at best* *slow you down*, and always (always!)
cause worse results.

> I've noticed that something seems to change my line width settings
> in Emacs, but I must have missed that memo.

Line length in source code is 79 or 80.  In email it is 72 or so.  This
has not changed since the dawn of time :-)

> >> +/* 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.  */
> >> +
> >> +static int
> >> +rs6000_call_offset_return_label (rtx_insn *call_insn)
> >> +{
> >> +  /* All rs6000 CALL_INSN output patterns start with a b or bl, always
> 
> > This isn't true.  There is a crlogical insn before the bcl for sysv for
> > example.
> 
> Hmm, if that's so, this function will have to look at the insn and
> recognize those cases and use a different way to compute the offset.
> 
> However, I don't see any relevant uses of bcl in output patterns for
> insns containing a call rtx.

bl, bcl, what's the difference (bit 4 is, heh, 16 vs. 18).  Read bl if
you want -- the point is that there are crlogical insns before the
branch-and-link.

> >> +                                                we compute the offset
> >> +     back to the address of the call opcode proper, then add the
> >> +     constant 4 bytes, to get the address after that opcode.  */
> >> +  return 4 - get_attr_length (call_insn);
> 
> > Please explain this magic, too -- in code preferably (so with a ? :
> > maybe, but don't try to "optimise" that expression, let the compiler
> > do that, it is much better at it anyway :-) )
> 
> There's neither optimization nor magic, it's literally what's written in
> the comment quoted above: starting from the label at the end of the call
> insn (that's what the caller offsets from, as in the documentation in
> the actual #1/2), subtract the length (to get to the address of the call
> opcode), and add 4 (to get past the call opcode).  Ok, I've reordered
> the two addends for an IMHO more readable expression, but that was all.

4 - length does not make any sense /an sich/, it *is* magic.

It is not clear it is correct at all, either.

> > Is that correct for all ABIs we support?  

(Context missing!  What did I ask?)

> Back when I wrote this patchset, I went through all call opcodes I could
> find in the md and .c files within rs6000/, and I was satisfied that it
> covered what we had then, but I won't pretend to know all about all of
> the ppc ABIs.  I may have missed disguised call insns, too.  I was
> hoping some ppc maintainer, more familiar with the port than I am, would
> catch any trouble on review and let me know about pitfalls and surprises
> to watch out for.

Yeah, things don't work that way.  If you need help, *ask* for that.
Don't pretend a patch is good if you have doubts yourself!

> > Even if so, it needs a lot more documentation than this.
> 
> I can write more documentation, but I'm at a loss as to what you're
> hoping for.  If you set clearer expectations, I'll be glad to oblige.

I want a patch submission that is a) understandable and b) a good thing
to have.  If a patch leaves me wondering what is going on at all, that
is not ideal ;-)


Segher

Re: [PATCH v3 #1/2] [rs6000] adjust return_pc debug attrs

[Kewen.Lin]

on 2024/5/29 14:52, Alexandre Oliva wrote:
> On May 27, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>> I wonder if it's possible to have a test case for this?
> 
> gcc.dg/guality/pr54519-[34].c at -O[1g] are fixed by this patch on

Nice!

> ppc64le-linux-gnu.  Are these the sort of test case you're interested

Yes, I was curious if we can have some testing coverage on this.  As
Segher pointed out, it would be good to have this information in commit
log.

BR,
Kewen

> in, or are you looking for something that tests the offsets in debug
> info, rather than the end-to-end debugging feature?
> 

Re: [PATCH v3 #1/2] enable adjustment of return_pc debug attrs

[Alexandre Oliva]

On May 28, 2024, Jason Merrill <jason@redhat.com> wrote:

> On 5/25/24 08:12, Alexandre Oliva wrote:
>> On Apr 27, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>> On Apr 14, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>>> On Mar 23, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>>>>> 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.  This will be used on rs6000, that sometimes issues another
>>>>> instruction after the call proper as part of a call insn.
>> 
>>>> Ping?
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614452.html
>> Ping?
>> Refreshed, retested on ppc64le-linux-gnu.  Ok to install?

> I wonder about adding this information to REG_CALL_ARG_LOCATION, but
> doing it this way also seems reasonable.  I'm interested in Jakub's 
> input, but the patch is OK in a week if he doesn't get to it.

Thanks, I'm putting it in, but I also look forward to Jakub's feedback.

As for REG_CALL_ARG_LOCATION, I suppose that would be a decent place to
hold it for the new hook to get at it, but since it can usually be
computed directly, possibly with help of an attribute, adding extra rtl
to call insns is probably unnecessary and undesirable.

>> for  gcc/ChangeLog
>> * 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.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive