From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id CA4AF3857C57 for ; Mon, 14 Sep 2020 12:16:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CA4AF3857C57 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 08EC2RGw073100 for ; Mon, 14 Sep 2020 08:16:47 -0400 Received: from ppma05fra.de.ibm.com (6c.4a.5195.ip4.static.sl-reverse.com [149.81.74.108]) by mx0b-001b2d01.pphosted.com with ESMTP id 33j7v5108j-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 14 Sep 2020 08:16:46 -0400 Received: from pps.filterd (ppma05fra.de.ibm.com [127.0.0.1]) by ppma05fra.de.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 08ECDN9o026586 for ; Mon, 14 Sep 2020 12:16:44 GMT Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by ppma05fra.de.ibm.com with ESMTP id 33gny815c3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 14 Sep 2020 12:16:44 +0000 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 08ECGfge19398966 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 14 Sep 2020 12:16:41 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 6DEB14203F; Mon, 14 Sep 2020 12:16:41 +0000 (GMT) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 52A9142045; Mon, 14 Sep 2020 12:16:41 +0000 (GMT) Received: from oc3748833570.ibm.com (unknown [9.145.47.193]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Mon, 14 Sep 2020 12:16:41 +0000 (GMT) Received: by oc3748833570.ibm.com (Postfix, from userid 1000) id D11D1D80327; Mon, 14 Sep 2020 14:16:40 +0200 (CEST) Date: Mon, 14 Sep 2020 14:16:40 +0200 From: Ulrich Weigand To: Rogerio Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] Use flexible target descriptors for Linux PowerPC Message-ID: <20200914121640.GA10546@oc3748833570.ibm.com> References: <20200703182238.153386-1-rcardoso@linux.ibm.com> <20200703190321.156086-1-rcardoso@linux.ibm.com> <20200709154802.GA2538@oc3748833570.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235, 18.0.687 definitions=2020-09-14_02:2020-09-10, 2020-09-14 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 impostorscore=0 suspectscore=0 adultscore=0 mlxlogscore=999 mlxscore=0 phishscore=0 clxscore=1015 malwarescore=0 lowpriorityscore=0 spamscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009140095 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 14 Sep 2020 12:16:49 -0000 On Wed, Sep 09, 2020 at 05:19:49PM -0300, Rogerio Alves wrote: > On 7/9/20 12:48 PM, Ulrich Weigand wrote: > > On Fri, Jul 03, 2020 at 04:03:21PM -0300, Rogerio Alves wrote: > >> -const struct target_desc * > >> -ppc_linux_match_description (struct ppc_linux_features features) > >> +const target_desc * > >> +ppc_create_target_description (struct ppc_linux_features features) > > > > So you've renamed it from a name including "linux" to one without, > > while the function is still actually Linux-specific. Is this a > > good idea? > > > I've renamed this only because the others architectures uses something > like _create_target_description. But in fact this is linux specfic > so I will change it to ppc_linux_create_target_description. OK, thanks. > > [ I guess a separate question is whether we shouldn't also switch > > all other powerpc targets to the new method ... ] > > Not sure about the other power targets for now I only targeted linux. > If that's the case maybe I should sent a separated patch if you agree > with that. It's certainly fine to do Linux first. Just something to think about. > > Do we need to call set_tdesc_osabi here like is done for other targets? > > Yes. Since it's linux I should call set_tdesc_osbi here. [Not sure why > aarch64 did not call it] OK. > >> index cc65baa6ed..3140eeb00c 100644 > >> --- a/gdb/features/Makefile > >> +++ b/gdb/features/Makefile > >> @@ -51,17 +51,6 @@ WHICH = arm/arm-with-iwmmxt arm/arm-with-vfpv2 arm/arm-with-vfpv3 \ > >> mips64-linux mips64-dsp-linux \ > >> nios2-linux \ > >> rs6000/powerpc-32 \ > > > > It seems to me that this file is actually unused as well, so we can really > > remove all the rs6000 regformats files then. (Including the special Makefile > > rule associated with them.) > > > > In that case we have a rs6000-tdep files on top folder that use this > regformats that is why I did not remove. I've only remove the rules for > rs6000/powerpc-*l* (linux) since my patch only target Linux for now. > In fact it's is kinda confusing to me why we have those rs6000? Yes, we still need the XML files, but this rule is about the *.dat files in regformats, and I believe the powerpc-32.dat file is actually unused. These files are only used by gdbserver, and there is no gdbserver target that uses powerpc-32.o as far as I can see. I believe I just forgot to remove that rule years ago. > > Talking about removed regformats files: did you verify that the new code still > > uses the same layout of the register packets, e.g. by running a new gdb against > > an old gdbserver and vice versa? > > > > I am checking it here. Since I am new on GDB I am not sure how to check > this layout. That's actually good question, I don't think there's a simple way to perform this check. One option to check, as I mentioned, would be to run the GDB test suite running a GDB built with the patch against a gdbserver built without the patch, and vice versa, and making sure there are no additional failures. > >> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv > >> index 9a027e44af..0ff094d32c 100644 > >> --- a/gdbserver/configure.srv > >> +++ b/gdbserver/configure.srv > > > >> - powerpc*-*-linux*) srv_regobj="powerpc-32l.o" > > > > Should we now use > > srv_regobj="" > > like the other targets do? > > > > With the new interface we should not use srv_regobj. Only the old > interfaces use it. Targets like aarch64 and x86_64 do not uses. I was just confused why some targets explicitly set srv_regobj to the empty string, while others do not set it at all. But I guess given that other Linux targets also do not set it at all, that should be fine. > >> ipa_obj="${ipa_ppc_linux_regobj} linux-ppc-ipa.o" > >> + ipa_obj="${ipa_obj} linux-ppc-tdesc-ipa.o" > >> + ipa_obj="${ipa_obj} arch/ppc-linux-common-ipa.o" > > > > I believe we now no longer need ${ipa_ppc_linux_regobj}, right? > > > > In fact we don't need. Not sure why I did not removed this. OK. > >> --- a/gdbserver/linux-ppc-ipa.cc > >> +++ b/gdbserver/linux-ppc-ipa.cc > >> @@ -22,7 +22,6 @@ > >> #include > >> #include "tracepoint.h" > >> #include "arch/ppc-linux-tdesc.h" > > > > Wasn't that file deleted? How does this still build? Or am I > > missing something here ... > > > > Yes. In fact I did forgot to remove those but when I've tested it got a > build error I removed those and works but I forgot to commit this small > change before generate the patch. It will be fixed on v3 OK. > > Something seems wrong here. If I'm reading this correctly, > > that description will only have the bare minimum register set, > > with all optional features switched off. The original code > > used the full register set available on the current machine. > > > > I was talking to Pedro about it by the time I was working on this patch > he explains that IPA only colects a few registers and we may not get > those registers in IPA that's why I did that. Now I am testing using a > fast tracepoint to see what registers it collects and I may include the > ones that is missing here. Well, I believe even with fast tracepoints the user can specify which registers he wants to collect, so I do think the full set should be available. That's what other target do as well, anyway. > >> + bool is_ppc64 = false; > >> + #ifdef __powerpc64__ > >> + is_ppc64 = true; > >> + #endif > >> + > >> + ppc_linux_read_description (is_ppc64) > > > > Why do you call that routine and ignore it's result? > > Isn't that a no-op here? > > > > Yes I am ignoring the result in that case because I don't need the tdesc > returned. read_description calls create description and save that on a > table. The original intend was to call that function on linux-low too > (to get the expedite regs): > > ppc-linux-low.cc: > > current_process ()->tdesc = tdesc; > > should be > > current_process ()->tdesc = ppc_linux_read_description (features); > > But I made a mistake and change the parameter to is_ppc and decide to > call create_description directly on ppc-linux-low. In fact I have to > replace = tdesc to ppc_linux_read_description (features) and change the > parameter from is_ppc64 to features. OK. > > You're using this only in the -ipa code. I don't think > > it makes sense to have a separate file just for that. > > > > On some other platforms that do have this file, they're > > using it both from the linux-low code and the -ipa code, > > but you're directly calling ppc_create_target_description > > there. > > > > However, it does look like you're missing the call to > > init_target_desc in your linux-low code, so we don't > > actually get expedite_regs set, which is a regression. > > > > I kinda explain that above when I call ppc_linux_read_description and > ignore the result. My solution will be change the parameter and call > ppc_linux_read_description on linux-low instead call create_description > directly. This should fix this mistake. OK. Thanks, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com