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 A828F385141F for ; Wed, 9 Sep 2020 20:19:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A828F385141F Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 089K1F6c091257 for ; Wed, 9 Sep 2020 16:19:54 -0400 Received: from ppma01dal.us.ibm.com (83.d6.3fa9.ip4.static.sl-reverse.com [169.63.214.131]) by mx0b-001b2d01.pphosted.com with ESMTP id 33f5h2grex-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 09 Sep 2020 16:19:54 -0400 Received: from pps.filterd (ppma01dal.us.ibm.com [127.0.0.1]) by ppma01dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 089KHt1H022573 for ; Wed, 9 Sep 2020 20:19:53 GMT Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by ppma01dal.us.ibm.com with ESMTP id 33d46mwe50-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 09 Sep 2020 20:19:53 +0000 Received: from b01ledav006.gho.pok.ibm.com (b01ledav006.gho.pok.ibm.com [9.57.199.111]) by b01cxnp22033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 089KJp2455968114 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 9 Sep 2020 20:19:51 GMT Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AF49AAC05B; Wed, 9 Sep 2020 20:19:51 +0000 (GMT) Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A2873AC059; Wed, 9 Sep 2020 20:19:50 +0000 (GMT) Received: from localhost.localdomain (unknown [9.163.23.179]) by b01ledav006.gho.pok.ibm.com (Postfix) with ESMTP; Wed, 9 Sep 2020 20:19:50 +0000 (GMT) Subject: Re: [PATCH v2] Use flexible target descriptors for Linux PowerPC To: Ulrich Weigand Cc: gdb-patches@sourceware.org References: <20200703182238.153386-1-rcardoso@linux.ibm.com> <20200703190321.156086-1-rcardoso@linux.ibm.com> <20200709154802.GA2538@oc3748833570.ibm.com> From: Rogerio Alves Message-ID: Date: Wed, 9 Sep 2020 17:19:49 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200709154802.GA2538@oc3748833570.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235, 18.0.687 definitions=2020-09-09_16:2020-09-09, 2020-09-09 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 bulkscore=0 mlxlogscore=999 priorityscore=1501 impostorscore=0 spamscore=0 suspectscore=0 phishscore=0 clxscore=1015 adultscore=0 mlxscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009090173 X-Spam-Status: No, score=-15.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, 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: Wed, 09 Sep 2020 20:19:56 -0000 Sorry for take me that long to reply this message. I was busy with other GDB tasks and I missed that reply On 7/9/20 12:48 PM, Ulrich Weigand wrote: > On Fri, Jul 03, 2020 at 04:03:21PM -0300, Rogerio Alves wrote: >> Use flexible target descriptors for Linux PowerPC. This allows a fine-tune >> selection of avalible target CPU features por POWER. > > Thanks for working on this! > >> diff --git a/gdb/arch/ppc-linux-common.c b/gdb/arch/ppc-linux-common.c >> index 183fc03395..36e2319a7f 100644 >> --- a/gdb/arch/ppc-linux-common.c >> +++ b/gdb/arch/ppc-linux-common.c >> -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. > [ 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. >> { >> - struct target_desc *tdesc = NULL; >> + target_desc *tdesc = allocate_target_description (); >> + long regnum = 0; >> >> if (features.wordsize == 8) >> { >> - if (features.vsx) >> - tdesc = (features.htm? tdesc_powerpc_isa207_htm_vsx64l >> - : features.isa207? tdesc_powerpc_isa207_vsx64l >> - : features.ppr_dscr? tdesc_powerpc_isa205_ppr_dscr_vsx64l >> - : features.isa205? tdesc_powerpc_isa205_vsx64l >> - : tdesc_powerpc_vsx64l); >> - else if (features.altivec) >> - tdesc = (features.isa205? tdesc_powerpc_isa205_altivec64l >> - : tdesc_powerpc_altivec64l); >> - else >> - tdesc = (features.isa205? tdesc_powerpc_isa205_64l >> - : tdesc_powerpc_64l); >> + #ifndef IN_PROCESS_AGENT >> + set_tdesc_architecture (tdesc, "power:common64"); > > 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] >> 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? > 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. >> 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. >> 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. >> --- 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 >> @@ -174,58 +173,12 @@ alloc_jump_pad_buffer (size_t size) >> const struct target_desc * >> get_ipa_tdesc (int idx) >> { >> - switch (idx) >> - { >> -#ifdef __powerpc64__ >> - case PPC_TDESC_BASE: >> - return tdesc_powerpc_64l; >> - case PPC_TDESC_ALTIVEC: >> - return tdesc_powerpc_altivec64l; >> - case PPC_TDESC_VSX: >> - return tdesc_powerpc_vsx64l; >> - case PPC_TDESC_ISA205: >> - return tdesc_powerpc_isa205_64l; >> - case PPC_TDESC_ISA205_ALTIVEC: >> - return tdesc_powerpc_isa205_altivec64l; >> - case PPC_TDESC_ISA205_VSX: >> - return tdesc_powerpc_isa205_vsx64l; >> - case PPC_TDESC_ISA205_PPR_DSCR_VSX: >> - return tdesc_powerpc_isa205_ppr_dscr_vsx64l; >> - case PPC_TDESC_ISA207_VSX: >> - return tdesc_powerpc_isa207_vsx64l; >> - case PPC_TDESC_ISA207_HTM_VSX: >> - return tdesc_powerpc_isa207_htm_vsx64l; >> -#else >> - case PPC_TDESC_BASE: >> - return tdesc_powerpc_32l; >> - case PPC_TDESC_ALTIVEC: >> - return tdesc_powerpc_altivec32l; >> - case PPC_TDESC_VSX: >> - return tdesc_powerpc_vsx32l; >> - case PPC_TDESC_ISA205: >> - return tdesc_powerpc_isa205_32l; >> - case PPC_TDESC_ISA205_ALTIVEC: >> - return tdesc_powerpc_isa205_altivec32l; >> - case PPC_TDESC_ISA205_VSX: >> - return tdesc_powerpc_isa205_vsx32l; >> - case PPC_TDESC_ISA205_PPR_DSCR_VSX: >> - return tdesc_powerpc_isa205_ppr_dscr_vsx32l; >> - case PPC_TDESC_ISA207_VSX: >> - return tdesc_powerpc_isa207_vsx32l; >> - case PPC_TDESC_ISA207_HTM_VSX: >> - return tdesc_powerpc_isa207_htm_vsx32l; >> - case PPC_TDESC_E500: >> - return tdesc_powerpc_e500l; >> -#endif >> - default: >> - internal_error (__FILE__, __LINE__, >> - "unknown ipa tdesc index: %d", idx); >> -#ifdef __powerpc64__ >> - return tdesc_powerpc_64l; >> -#else >> - return tdesc_powerpc_32l; >> -#endif >> - } >> + bool is_ppc64 = false; >> + #ifdef __powerpc64__ >> + is_ppc64 = true; >> + #endif >> + >> + return ppc_linux_read_description (is_ppc64); > > 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. >> @@ -234,26 +187,11 @@ get_ipa_tdesc (int idx) >> void >> initialize_low_tracepoint (void) >> { >> -#ifdef __powerpc64__ >> - init_registers_powerpc_64l (); >> - init_registers_powerpc_altivec64l (); >> - init_registers_powerpc_vsx64l (); >> - init_registers_powerpc_isa205_64l (); >> - init_registers_powerpc_isa205_altivec64l (); >> - init_registers_powerpc_isa205_vsx64l (); >> - init_registers_powerpc_isa205_ppr_dscr_vsx64l (); >> - init_registers_powerpc_isa207_vsx64l (); >> - init_registers_powerpc_isa207_htm_vsx64l (); >> -#else >> - init_registers_powerpc_32l (); >> - init_registers_powerpc_altivec32l (); >> - init_registers_powerpc_vsx32l (); >> - init_registers_powerpc_isa205_32l (); >> - init_registers_powerpc_isa205_altivec32l (); >> - init_registers_powerpc_isa205_vsx32l (); >> - init_registers_powerpc_isa205_ppr_dscr_vsx32l (); >> - init_registers_powerpc_isa207_vsx32l (); >> - init_registers_powerpc_isa207_htm_vsx32l (); >> - init_registers_powerpc_e500l (); >> -#endif >> + >> + 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. >> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc >> index 337d555aee..b982be3e1b 100644 >> --- a/gdbserver/linux-ppc-low.cc >> +++ b/gdbserver/linux-ppc-low.cc >> @@ -29,7 +29,6 @@ >> #include "arch/ppc-linux-tdesc.h" > > Same question as above: wasn't that file deleted? > Yes. >> diff --git a/gdbserver/linux-ppc-tdesc.cc b/gdbserver/linux-ppc-tdesc.cc >> new file mode 100644 >> index 0000000000..3bf97d20fc > >> +#include > > What is this needed for? > I will remove this include. I put that based on other linux-*-tdesc.cc files. >> +/* All possible ppc target descriptors. */ >> +struct target_desc *tdesc_ppc_list[2]; > > See above, I don't believe it is correct to only > support two variants here. > Ok. >> +/* Create the Power target description. */ >> +const target_desc * >> +ppc_linux_read_description (bool is_ppc64) >> +{ >> + struct target_desc *tdesc = tdesc_ppc_list[is_ppc64]; >> + >> + if (tdesc == NULL) >> + { >> + struct ppc_linux_features features; >> + features.wordsize = (is_ppc64) ? 8 : 4; >> + *tdesc = ppc_create_target_description (features); >> + >> + static const char* expedite_regs[] = { "r1", "pc" }; >> + init_target_desc (tdesc, expedite_regs); >> + >> + tdesc_ppc_list[is_pp64] = tdesc; >> + } >> + >> + return tdesc; >> +} > > 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. > Bye, > Ulrich > Regards Rogerio