From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 32A273858D37 for ; Wed, 2 Mar 2022 10:14:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 32A273858D37 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-582-nVMwQTIxONCxM365E13qGA-1; Wed, 02 Mar 2022 05:14:51 -0500 X-MC-Unique: nVMwQTIxONCxM365E13qGA-1 Received: by mail-wr1-f71.google.com with SMTP id a16-20020adff7d0000000b001f0473a6b25so160039wrq.1 for ; Wed, 02 Mar 2022 02:14:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version:content-transfer-encoding; bh=NkhLESRSb6SqJSgFxjQki5yNo8hRn5TaFRqNoOObc6I=; b=Jt/Z0Koi6n1W1keDM4wmLn5sTYClh3HsG2fIFgiF4TffKn0Wmwv+sBsz0w/znfEx3O GUhZx4i+9rxreTT5/op6TX6QX/u/vu7FMrUNlr+y9ftmVFhz2jPl65Xr27wj1HNPiCPz lUkenzllKBuYxvdumFc/QRkmn2DEaFgkTdruzytbhpUpnU00TeqvBMbR0w+/ZglUcFQz KfOllvsSmM6d2qemop/p2xr6h7QwVMlfgR/Z1TNz+UiaGtT3qrXtB+UniMZhbH/+D/jB GeMygFz0+Yu7Xhdel68bHVTs1VbI2WGGbwo0C6Al37LrmwtjLabmJ+1lYpPPKcWYc/qu 23gg== X-Gm-Message-State: AOAM533sdw0uVRxFJnO/5eH1T44GvRxNAfEg8Tus1q3+o2VGeYKBf0pT BExk7N7bhsfbOazIRU2J8kETfEbW5rr6x5irIdImI4UT9RtOcyX93KQunvF3Cck6VgKB8z6w9ui wVb035sCbwuh0duKdNLF++Q== X-Received: by 2002:a05:6000:ce:b0:1ef:189b:e31e with SMTP id q14-20020a05600000ce00b001ef189be31emr20596858wrx.538.1646216089424; Wed, 02 Mar 2022 02:14:49 -0800 (PST) X-Google-Smtp-Source: ABdhPJzVvkJ4iyKd+kvzoajND7lGDaiVHuVB28vqeD2ti1fFn70SFYtKREgy9wAXxzjmHdoaJPYFSw== X-Received: by 2002:a05:6000:ce:b0:1ef:189b:e31e with SMTP id q14-20020a05600000ce00b001ef189be31emr20596849wrx.538.1646216089052; Wed, 02 Mar 2022 02:14:49 -0800 (PST) Received: from localhost (host86-169-131-29.range86-169.btcentralplus.com. [86.169.131.29]) by smtp.gmail.com with ESMTPSA id i5-20020a1c3b05000000b00382871cf734sm4096046wma.25.2022.03.02.02.14.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 02:14:48 -0800 (PST) From: Andrew Burgess To: lifang_xia@linux.alibaba.com, 'Lifang Xia' , gdb-patches@sourceware.org Subject: Re: =?utf-8?B?562U5aSNOg==?= [PATCH] [RISC-V]: Handling optimized prologue In-Reply-To: <002b01d82dd9$7bfc38f0$73f4aad0$@linux.alibaba.com> References: <20220301073231.1927-1-lifang_xia@c-sky.com> <87h78iynam.fsf@redhat.com> <002b01d82dd9$7bfc38f0$73f4aad0$@linux.alibaba.com> Date: Wed, 02 Mar 2022 10:14:47 +0000 Message-ID: <877d9czmnc.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Mar 2022 10:14:53 -0000 lifang_xia--- via Gdb-patches writes: > Hi Andrew, > Thanks for reviewing. > See below. > > >> -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- >> =E5=8F=91=E4=BB=B6=E4=BA=BA: Andrew Burgess >> =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2022=E5=B9=B43=E6=9C=881=E6=97=A5 = 18:34 >> =E6=94=B6=E4=BB=B6=E4=BA=BA: Lifang Xia ; gdb-patc= hes@sourceware.org >> =E6=8A=84=E9=80=81: Lifang Xia >> =E4=B8=BB=E9=A2=98: Re: [PATCH] [RISC-V]: Handling optimized prologue >>=20 >> Lifang Xia writes: >>=20 >> Hello Lifang, >>=20 >> Thanks for working on this. >>=20 >> > From: Lifang Xia >> > >> > The optimizer might shove anything into the prologue, if we build up >> > cache (cache !=3D NULL) from scanning prologue, we just skip what we >> > don't recognize and scan further to make cache as complete as >> > possible. >>=20 >> My concern here would be, what if the thing you are skipping over modifi= es >> the state in such a way, that when we think we are building the cache, > we're >> really doing the wrong thing in some way... >>=20 >>=20 >> > However, if we skip prologue, >> > we'll stop immediately on unrecognized instruction. >> > >> > The case is: >> > >> > ---------------------------- >> > Disassembly of section .text: >> > >> > 0000000000010078 : >> > 10078: 00100513 li a0,1 >> > 1007c: 008000ef jal ra,10084 >> > 10080: 00008067 ret >> > >> > 0000000000010084 : >> > 10084: 00051263 bnez a0,10088 >> > 10088: ff010113 addi sp,sp,-16 >> > 1008c: 00813023 sd s0,0(sp) >> > 10090: 00050413 mv s0,a0 >> > 10094: 00113423 sd ra,8(sp) >> > 10098: 014000ef jal ra,100ac >> > 1009c: 00813083 ld ra,8(sp) >> > 100a0: 00013403 ld s0,0(sp) >> > 100a4: 01010113 addi sp,sp,16 >> > 100a8: 00008067 ret >> > >> > 00000000000100ac : >> > 100ac: 00000013 nop >> >>> 100b0: 00008067 ret >> > ---------------------- >> > (gdb) bt >> > #0 0x00000000000100b0 in aaa () >> > #1 0x000000000001009c in bar () >> > Backtrace stopped: frame did not save the PC >>=20 >> I assume it's the bnez that's causing the problem for the prologue scan > here? >>=20 >> Not that I really understand what that instruction is about, it appears = to > be >> branching to the next instruction. I wonder if this objdump output is f= or > the >> object file, rather than the executable? I'm guessing the bnez has a > reloc >> which points at, maybe, the ret at 0x100a8? >>=20 >> I wonder if a better solution in this case would be to allow the prologu= e > scan >> to skip over forward branches within the prologue, maybe with the > limitation >> that the destination has to be within the function bounds? >>=20 > > Yes, You are right. bnez cause the prologue scan failed. > It's one of the cases I have met. As mentioned before, The optimizer of > compiler might shove anything to the prologue. > It could be multiple instructions, shift instructions, condition branches > ... > We can't handle all of instructions from the ISA specs or vendor extensi= ons > here. > > Actually, if cache is not nullptr, that means we need to build the frame > information from prologue. Before the loop, we get the base address and = the > prologue_end, > traversing all the prologue is not a bad choice to build the frame(if ca= che > is not nullptr). Except right now, if we declare that we know something based on the prologue then we can be reasonably sure that what we claim to know is true. Under your proposal we would claim to know things for which we actually have no knowledge of whether it is true or not, e.g. if some previously unknown instruction has adjusted the contents of a register in some way, but we ignore the adjustment and just claim to know the previous register contents anyway. This then raises the question, would we prefer GDB to say "I don't know", if it's not sure, or say "The answer is ....", when that might not be true? Are there other targets that do this aggressive prologue scanning? An alternative might be to offer a new setting, which is off by default, but which allows for the aggressive prologue scanning you suggest. However, I think such an option would have to include a suitable warning to indicate that some prologue instructions might be ignored. I still think you should consider just adding support for forward branches to the prologue scanner. Though any instruction _could_ be moved into the prologue, I suspect (guess) the number that actually _do_ get moved up is pretty small. A different strategy might be to handle unknown instructions based on their instruction class (R, I, S, etc). If the instruction is only touching registers that we don't need to preserve over the unwind, or is touching registers for which the previous value has already been saved, then maybe we don't need to care about the actual meaning of the instruction, we can just mark the destination register as unknown and move on. I think this sounds much closer to what you want. Thanks, Andrew > >> > >> > gdb/ >> > =09* riscv-tdep.c (riscv_scan_prologue): Keep scaning if cache is not >> NULL. >> > --- >> > gdb/riscv-tdep.c | 10 +++++++++- >> > 1 file changed, 9 insertions(+), 1 deletion(-) >> > >> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index >> > 886996c..e46d441 100644 >> > --- a/gdb/riscv-tdep.c >> > +++ b/gdb/riscv-tdep.c >> > @@ -1987,7 +1987,15 @@ riscv_scan_prologue (struct gdbarch *gdbarch, >> > else >> > =09{ >> > =09 end_prologue_addr =3D cur_pc; >> > -=09 break; >> > + >> > +=09 /* The optimizer might shove anything into the prologue, if >> > +=09 we build up cache (cache !=3D NULL) from scanning prologue, >> > +=09 we just skip what we don't recognize and scan further to >> > +=09 make cache as complete as possible. However, if we skip >> > +=09 prologue, we'll stop immediately on unrecognized >> > +=09 instruction. */ >> > +=09 if (cache =3D=3D NULL) >>=20 >> Use nullptr not NULL please. >>=20 >> Thanks, >> Andrew >>=20 >>=20 >>=20 >> > +=09 break; >> > =09} >> > } >> > >> > -- >> > 2.7.4