From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 781 invoked by alias); 7 Jun 2013 14:43:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 768 invoked by uid 89); 7 Jun 2013 14:43:54 -0000 X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_EG autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 07 Jun 2013 14:43:53 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UkxtM-0002rB-O5 from Luis_Gustavo@mentor.com ; Fri, 07 Jun 2013 07:43:48 -0700 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 7 Jun 2013 07:43:48 -0700 Received: from [172.30.64.221] ([172.30.64.221]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 7 Jun 2013 07:43:47 -0700 Message-ID: <51B1F195.70008@codesourcery.com> Date: Fri, 07 Jun 2013 14:43:00 -0000 From: Luis Machado Reply-To: lgustavo@codesourcery.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Andreas Arnez CC: gdb-patches@sourceware.org, Ulrich Weigand Subject: Re: [PATCH 3/3] Dynamic core regset sections support References: <87fvwu5937.fsf@br87z6lw.de.ibm.com> <87li6mvxgv.fsf@br87z6lw.de.ibm.com> In-Reply-To: <87li6mvxgv.fsf@br87z6lw.de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00158.txt.bz2 H Andreas, Thanks for the patch. Just a general thought. If a core file section must not always be generated, would it make more sense to use dynamic section information for s390 instead of forcing all the other targets to use a mechanism that is not required for them? I'm not necessarily against this modification, but i was wondering about other options. See comments below... > Index: gdb/gdb/ppc-linux-tdep.c > =================================================================== > --- gdb.orig/gdb/ppc-linux-tdep.c > +++ gdb/gdb/ppc-linux-tdep.c > @@ -256,53 +256,26 @@ ppc_linux_return_value (struct gdbarch * > readbuf, writebuf); > } > > -static struct core_regset_section ppc_linux_vsx_regset_sections[] = > -{ > - { ".reg", 48 * 4, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { ".reg-ppc-vmx", 544, "ppc Altivec" }, > - { ".reg-ppc-vsx", 256, "POWER7 VSX" }, > - { NULL, 0} > -}; > - > -static struct core_regset_section ppc_linux_vmx_regset_sections[] = > -{ > - { ".reg", 48 * 4, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { ".reg-ppc-vmx", 544, "ppc Altivec" }, > - { NULL, 0} > -}; > - > -static struct core_regset_section ppc_linux_fp_regset_sections[] = > -{ > - { ".reg", 48 * 4, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { NULL, 0} > -}; > - > -static struct core_regset_section ppc64_linux_vsx_regset_sections[] = > -{ > - { ".reg", 48 * 8, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { ".reg-ppc-vmx", 544, "ppc Altivec" }, > - { ".reg-ppc-vsx", 256, "POWER7 VSX" }, > - { NULL, 0} > -}; > - > -static struct core_regset_section ppc64_linux_vmx_regset_sections[] = > +static void > +ppc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch, > + iterate_over_regset_sections_cb *cb, > + void *cb_data, > + const struct regcache *regcache) > { > - { ".reg", 48 * 8, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { ".reg-ppc-vmx", 544, "ppc Altivec" }, > - { NULL, 0} > -}; > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + int have_vsx = tdep->ppc_vr0_regnum != -1; > + int have_altivec = tdep->ppc_vsr0_upper_regnum != -1; > > -static struct core_regset_section ppc64_linux_fp_regset_sections[] = > -{ > - { ".reg", 48 * 8, "general-purpose" }, > - { ".reg2", 264, "floating-point" }, > - { NULL, 0} > -}; > + if ((*cb) (".reg", 48 * 4, "general-purpose", cb_data)) > + return; > + if ((*cb) (".reg2", 264, "floating-point", cb_data)) > + return; > + if (have_altivec) > + if ((*cb) (".reg-ppc-vmx", 544, "ppc Altivec", cb_data)) > + return; > + if (have_vsx) > + (*cb) (".reg-ppc-vsx", 256, "POWER7 VSX", cb_data); > +} > > /* PLT stub in executable. */ > static struct ppc_insn_pattern powerpc32_plt_stub[] = > @@ -1305,18 +1278,9 @@ ppc_linux_init_abi (struct gdbarch_info > else > set_gdbarch_gcore_bfd_target (gdbarch, "elf32-powerpc"); > > - /* Supported register sections. */ > - if (tdesc_find_feature (info.target_desc, > - "org.gnu.gdb.power.vsx")) > - set_gdbarch_core_regset_sections (gdbarch, > - ppc_linux_vsx_regset_sections); > - else if (tdesc_find_feature (info.target_desc, > - "org.gnu.gdb.power.altivec")) > - set_gdbarch_core_regset_sections (gdbarch, > - ppc_linux_vmx_regset_sections); > - else > - set_gdbarch_core_regset_sections (gdbarch, > - ppc_linux_fp_regset_sections); > + /* Iterator for supported register sections. */ > + set_gdbarch_iterate_over_regset_sections > + (gdbarch, ppc_linux_iterate_over_regset_sections); > > if (powerpc_so_ops.in_dynsym_resolve_code == NULL) > { > @@ -1360,18 +1324,9 @@ ppc_linux_init_abi (struct gdbarch_info > else > set_gdbarch_gcore_bfd_target (gdbarch, "elf64-powerpc"); > > - /* Supported register sections. */ > - if (tdesc_find_feature (info.target_desc, > - "org.gnu.gdb.power.vsx")) > - set_gdbarch_core_regset_sections (gdbarch, > - ppc64_linux_vsx_regset_sections); > - else if (tdesc_find_feature (info.target_desc, > - "org.gnu.gdb.power.altivec")) > - set_gdbarch_core_regset_sections (gdbarch, > - ppc64_linux_vmx_regset_sections); > - else > - set_gdbarch_core_regset_sections (gdbarch, > - ppc64_linux_fp_regset_sections); > + /* Iterator for supported register sections. */ > + set_gdbarch_iterate_over_regset_sections > + (gdbarch, ppc_linux_iterate_over_regset_sections); > } > > /* PPC32 uses a different prpsinfo32 compared to most other Linux I am commenting explicitly for the ppc bits, but this may apply to the other backends as well. Did you maybe forget to handle ppc64 core file sections in this change? I see a core file register set entry for ppc64 being removed, like so: - { ".reg", 48 * 8, "general-purpose" }, ... but it is not being checked by the new code. Though the name is the same, the size of the section is different compared to ppc32. - { ".reg", 48 * 4, "general-purpose" }, Another general comment/suggestion. Removing static definitions of core file register sets and replacing them with hardcoded arguments for a pointer-based call to a callback function seems to scramble/confuse things a little. It may be nicer to keep the static definitions around, but iterate through them in the new iterator function. What do you think? Regards, Luis