Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: "Maciej W. Rozycki" <macro@imgtec.com>
To: Pedro Alves <palves@redhat.com>
Cc: Luis Machado <lgustavo@codesourcery.com>,
	<gdb-patches@sourceware.org>,
	Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: Re: [PATCH] Handle loading improper core files gracefully in the mips backend.
Date: Thu, 04 Feb 2016 21:01:00 -0000	[thread overview]
Message-ID: <alpine.DEB.2.00.1602041949090.15885@tp.orcam.me.uk> (raw)
In-Reply-To: <56B0BBB4.6050105@redhat.com>

On Tue, 2 Feb 2016, Pedro Alves wrote:

> > Did you try to trigger the assertion by loading a 32-bit MIPS binary
> > into gdb, and playing with "set mips abi n64/o64...", "set mipsfpu",
> > etc?
> > 
> > I think that adding a test to the testsuite that iterates through all
> > the possible combinations just to make sure gdb doesn't crash
> > would be great, and also show that the patch stands on its own
> > as well, irrespective of the bfd arch compatibility issues.
> 
> TBC, I meant, the original patch that checked unsuitable ABI/ISA
> combinations.

 NB I can only see the use for these knobs to deal with two situations:

1. There's no executable and we want to connect to a live target for 
   minimal binary-only/disasembly-level debugging.  We need to set the 
   endianness, ABI, ISA, etc. to match the target then (although arguably 
   at least the endianness should be supplied by the debug stub somehow; 
   we just don't have a way defined right now).

2. We have a buggy or outdated GDB executable which fails to determine the 
   right settings automatically.  As it looks for example as recently as 
   last week I came across a problem where GDB failed to detect the 
   presence of the FPU under Linux for some reason.  I had to forcefully
   request `set mipsfpu double' to override the wrongly chosen setting of 
   `none', at which point accesses to the FPU worked as expected, so it 
   wasn't like the unit was inaccessible.

Obviously I need to debug #2, but overall I agree we ought to bail out 
gracefully rather than crash if the manual settings lead to a nonsensical 
configuration.

 Therefore I went back to the original patch now.  It obscures things a 
bit I'm afraid from my point of view as it combines a syntactical change 
(the addition of the `arch_is_incompatible' auxiliary variable) and a 
semantical change (the addition of the `mips_isa == ISA_MIPS16' check), so 
I'd like to see the two changes separated.

 TBH I'm not convinced whether the auxiliary variable buys us anything 
here -- it doesn't serve as documentation as we have an explanatory 
comment here already, which BTW needs to be updated accordingly if the 
condition is extended to cover an ISA incompatibility.

 As to which, and more importantly -- there is no actual architectural 
incompatibility between the n64 (or n32) ABI and the MIPS16 instruction 
set; there are 64-bit MIPS processors in existence which implement the 
MIPS16 ISA as well, e.g. the NEC VR4111, and the ISA itself includes 
64-bit instructions on such a processor.  So the MIPS16 ISA is really 
agnostic to the ABI, just as is the regular MIPS ISA or the microMIPS ISA.  
Therefore any such fix needs to go elsewhere I'm afraid -- we probably do 
something outright silly for the ISA_MIPS16 setting.

  Maciej


  reply	other threads:[~2016-02-04 21:01 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-08 18:32 Luis Machado
2016-01-09  3:02 ` Maciej W. Rozycki
2016-01-11 15:47   ` Luis Machado
2016-01-12 12:46     ` Pedro Alves
2016-01-12 13:25       ` Luis Machado
2016-01-12 14:10         ` Pedro Alves
2016-01-12 15:43           ` Luis Machado
2016-01-12 16:00             ` Pedro Alves
2016-01-12 18:30             ` Maciej W. Rozycki
2016-01-12 19:08               ` Pedro Alves
2016-02-02 12:58               ` Luis Machado
2016-02-02 14:19                 ` Pedro Alves
2016-02-02 14:22                   ` Pedro Alves
2016-02-04 21:01                     ` Maciej W. Rozycki [this message]
2016-02-05 11:29                       ` Luis Machado
2016-02-05 14:10                         ` Maciej W. Rozycki
2017-01-09 19:57               ` Luis Machado
2017-01-19 16:56                 ` Pedro Alves
2017-01-19 17:05                   ` Luis Machado

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.00.1602041949090.15885@tp.orcam.me.uk \
    --to=macro@imgtec.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jan.kratochvil@redhat.com \
    --cc=lgustavo@codesourcery.com \
    --cc=palves@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox