From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTPS id 3163C385843D; Thu, 23 Mar 2023 03:06:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3163C385843D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 15FBC116407; Wed, 22 Mar 2023 23:06:52 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id G4VwcCbdmB6q; Wed, 22 Mar 2023 23:06:52 -0400 (EDT) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id A8011116403; Wed, 22 Mar 2023 23:06:50 -0400 (EDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 32N36d9Q1266203 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 23 Mar 2023 00:06:40 -0300 From: Alexandre Oliva To: Segher Boessenkool Cc: gcc-patches@gcc.gnu.org, Tom Tromey , David Edelsohn , Kewen Lin , Jason Merrill , Cary Coutant , Jakub Jelinek Subject: Re: [PATCH] [rs6000] adjust return_pc debug attrs Organization: Free thinker, does not speak for AdaCore References: <20230313143043.GA25951@gate.crashing.org> Errors-To: aoliva@lxoliva.fsfla.org Date: Thu, 23 Mar 2023 00:06:39 -0300 In-Reply-To: <20230313143043.GA25951@gate.crashing.org> (Segher Boessenkool's message of "Mon, 13 Mar 2023 09:30:43 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mar 13, 2023, Segher Boessenkool wrote: > 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). 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, or that no 64-bit call variant existed or would come into existence. > Subtracting 4 from what we currently return is very fragile. Agreed. > 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. I'll post a new version of the patch, now broken up into rs6000-specific and machine-independent, momentarily. > (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. >> +/* 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. > But, is the assert useful at all anyway, won't the callers have > checked for this already? No, the callers would let a SEQUENCE through, if there was one. But rs6000 won't ever see one, because there are no delay slots. 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. >> + 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 :-) I happen to not share your subjective preference, but I don't mind following that style in code you maintain. >> + 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? It was not an assumption, I took the conditions from the code, both from output functions in rs6000.cc and from call patterns in rs6000.md. Both rs6000_call_template_1 and rs6000_indirect_call_template_1 test for rs6000_pcrel_p() and then output b or bl opcodes without any subsequence instruction. This test mirrors those. >> + 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. Of course. But mirroring the structure of the corresponding code makes it easier to understand, and to check that the correspondence is there. But now that style aspect is irrelevant, it's obviated by your suggested alternate implementation. Thanks, -- 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