| Message ID | orv8329jjb.fsf_-_@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,#1/2,rs6000] adjust return_pc debug attrs | expand |
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 > > > /* 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. */ > > >
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
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?
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,
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
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
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? >
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 /* 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. */