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