Message ID | CAF3nGXd_3y70X7QRnK10_yB31fArv+VmSdQ1qq+ewBVAmcnKfQ@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 62521 invoked by alias); 29 Oct 2018 08:18:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 62499 invoked by uid 89); 29 Oct 2018 08:18:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=arches, H*r:sk:i15-v6s, H*c:alternative, sincerely X-HELO: mail-qt1-f179.google.com Received: from mail-qt1-f179.google.com (HELO mail-qt1-f179.google.com) (209.85.160.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 29 Oct 2018 08:18:46 +0000 Received: by mail-qt1-f179.google.com with SMTP id i15-v6so8229317qtr.0 for <gdb-patches@sourceware.org>; Mon, 29 Oct 2018 01:18:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=+fdlnLhzJUer2gk2/ue08RlmBchWiYYH7WR4n2AMA5Q=; b=DrNXVVFbqDw1AcPRMMAebGjGVTG/sbC46l0yvwa6TIk4WnssGbP0AKKdMygs2JBkK7 Ju0rxctOBbo5QQ3sxNfyo29o2K1+NmcU9ZJ572RvqBqok2RNYD1y2x2db0D6MrbT5Fx9 yrfC9GgNR9HpxbiSmDOMkydlNeErnPGjSDCwO1dOc88UaG7vJ2CzmvzEqFAmSw3xIon7 5aPSOrRbZiOFI2j7XsC8Vqvzs3oJmjoJZWPOBltOpjOXblG5Iyz37j1lZsxvDR48PD2Q M0x8JCL+xwLnSsPnRahzLFP2DGGKE8UOEMzhUt6iQHjRNWUw+2CzMv7e6hAmPaX+pwLf FTng== MIME-Version: 1.0 From: Denis Dmitriev <zealot351@gmail.com> Date: Mon, 29 Oct 2018 11:18:33 +0300 Message-ID: <CAF3nGXd_3y70X7QRnK10_yB31fArv+VmSdQ1qq+ewBVAmcnKfQ@mail.gmail.com> Subject: [PATCH] Abi selection based on set arch command in case of remote debugging for MIPS To: gdb-patches@sourceware.org Content-Type: text/plain; charset="UTF-8" |
Commit Message
Denis Dmitriev
Oct. 29, 2018, 8:18 a.m. UTC
Abi selection based on the execution of the "set arch" command in the case of remote debugging. Mips. No need to manually configure abi. if (gdbarch_debug)
Comments
Maciej, Just wondering, would this patch still fall under your maintainership, and do you plan on reviewing it? I think it's a followup this, which should give more context: https://sourceware.org/ml/gdb-patches/2018-10/msg00342.html Denis: In the mean time, I noted some comments about formatting. Also, you would need to provide a ChangeLog entry with the change. See this for more details: https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog For this change, it could look like: * mips-tdep.c (mips_gdbarch_init): Select ABI based on architecture if no binary is provided. On 2018-10-29 4:18 a.m., Denis Dmitriev wrote: > Abi selection based on the execution of the "set arch" command in the case > of remote debugging. Mips. No need to manually configure abi. > > diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c > index 5e0a606..dc6b6181 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -8085,7 +8085,20 @@ mips_gdbarch_init (struct gdbarch_info info, struct > gdbarch_list *arches) > if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour) > elf_flags = elf_elfheader (info.abfd)->e_flags; > else if (arches != NULL) > + { Applicable to all if/else, the braces should indented: if (...) { ... } else { ... } > elf_flags = gdbarch_tdep (arches->gdbarch)->elf_flags; > + if (elf_flags == 0) > + { > + if (info.bfd_arch_info && info.bfd_arch_info->bits_per_address == 64) When checking if a pointer is null, use the explicit if (info.bfd_arch_info != NULL && ...) or if (info.bfd_arch_info != nullptr && ...) I know there are violations of this rule in some places, but for new code we try to enforce the GNU style. > + { > + elf_flags = E_MIPS_ABI_EABI64; > + } > + else if (info.bfd_arch_info->bits_per_address == 32) > + { > + elf_flags = MIPS_ABI_EABI32; > + } When there is a single statement, drop the braces: if (...) elf_flags = E_MIPS_ABI_EABI64; else elf_flags = MIPS_ABI_EABI32; > + } > + } > else > elf_flags = 0; > if (gdbarch_debug) > > Simon
Simon, Apologies for the long RTT, I've come across your e-mail only now, while routinely reviewing general mailing list traffic received. I think my mips.com address does not work anymore (did you get a bounce?), and even if it does, I have no access to whatever system collects incoming messages there. > Just wondering, would this patch still fall under your maintainership, and do > you plan on reviewing it? I think it's a followup this, which should give more > context: > > https://sourceware.org/ml/gdb-patches/2018-10/msg00342.html Thanks for the pointer. I suppose I could look into it as a general issue, even though Octeon specifically used here is not a legacy MIPS implementation, but the change proposed makes no sense to me I am afraid. You can't infer the ABI just from the architecture and whatever you choose may work with one remote debug stub and then break with another (e.g. o32 Linux `gdbserver' will exchange register data in 32-bit quantities and use 32-bit addressing even with a 64-bit processor). Also AFAIK the embedded MIPS ABIs have been obsolete for years now; I haven't seen any use of them for well over a decade now, so I recommend against making any new use of them. The correct solution to get the address and register size right in GDB is to communicate it with an XML target description, and this is what I'd suggest implementing in the debug stub, as it is the stub that knows what it communicates to GDB. Then GDB can present that information according to the ABI selected, either manually or inferred from the ELF file selected for debugging, however in the absence of an ELF file GDB still won't be able to guess the ABI (we might consider having a different default selected somehow at build time, although o32 has the benefit of being compatible with all MIPS hardware). It would be good to know in detail though what the original problem was that the change proposed was intended to solve, as my proposal for the solution has been based on a guess and may not actually address what has been causing troubles. > > + { > > + elf_flags = E_MIPS_ABI_EABI64; > > + } > > + else if (info.bfd_arch_info->bits_per_address == 32) > > + { > > + elf_flags = MIPS_ABI_EABI32; > > + } > > When there is a single statement, drop the braces: > > if (...) > elf_flags = E_MIPS_ABI_EABI64; > else > elf_flags = MIPS_ABI_EABI32; This clearly hasn't been even barely tested :( as `E_MIPS_ABI_*' and `MIPS_ABI_*' groups of constants have distinct uses each. Maciej
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index 5e0a606..dc6b6181 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -8085,7 +8085,20 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour) elf_flags = elf_elfheader (info.abfd)->e_flags; else if (arches != NULL) + { elf_flags = gdbarch_tdep (arches->gdbarch)->elf_flags; + if (elf_flags == 0) + { + if (info.bfd_arch_info && info.bfd_arch_info->bits_per_address == 64) + { + elf_flags = E_MIPS_ABI_EABI64; + } + else if (info.bfd_arch_info->bits_per_address == 32) + { + elf_flags = MIPS_ABI_EABI32; + } + } + } else elf_flags = 0;