From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31082 invoked by alias); 25 Apr 2012 17:02:42 -0000 Received: (qmail 31074 invoked by uid 22791); 25 Apr 2012 17:02:41 -0000 X-SWARE-Spam-Status: No, hits=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_BJ X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 25 Apr 2012 17:02:26 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SN5bl-0002uk-Lo from Maciej_Rozycki@mentor.com ; Wed, 25 Apr 2012 10:02:25 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 25 Apr 2012 10:02:24 -0700 Received: from [172.30.0.84] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Wed, 25 Apr 2012 18:02:23 +0100 Date: Wed, 25 Apr 2012 17:18:00 -0000 From: "Maciej W. Rozycki" To: Joel Brobecker CC: Subject: Re: [PATCH] microMIPS support In-Reply-To: <20120425152847.GG10958@adacore.com> Message-ID: References: <20120425152847.GG10958@adacore.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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 X-SW-Source: 2012-04/txt/msg00873.txt.bz2 On Wed, 25 Apr 2012, Joel Brobecker wrote: > I could only scan the patch briefly for today, but noticed something > that caught my attention: > > > +/* For backwards compatibility we default to MIPS16. This flag is > > + overridden as soon as unambiguous ELF file flags tell us the > > + compressed ISA encoding used. */ > > +static const char mips_compression_mips16[] = "mips16"; > > +static const char mips_compression_micromips[] = "micromips"; > > +static const char *mips_compression_strings[] = { > > + mips_compression_mips16, > > + mips_compression_micromips, > > + NULL > > +}; > > We usually provide this sort of feature a little differently: Instead > of 2 values that are adjusted automatically by the debugger, we provide > 3 values: auto, mips16, and micromips. If auto, then the debugger has > to guess, possibly defaulting to mips16 if guessing did not work. > But if the user sets the setting to either of the non-auto values, > then the setting should be honored, even if the user is wrong. > > This is usually implemented using two variables: One representing > the setting, and one representing the actual value. I have been aware of that and decided the model does not fit. Please note that this setting is a fall-back that's never used when you have an executable available. If you do have an executable, then the ISA mode of compressed code is always known, because it's recorded in two places: individual symbol's ELF st_other flags, and globally, in the ELF file header. The latter is actually optional, in the sense that originally the compressed ISA was not recorded at all. However all microMIPS objects must have the microMIPS flag set and therefore any compressed code in an executable that does not have any compressed flag set must be MIPS16 code in a legacy object. It's the st_other flags that's normally used by GDB to determine the compressed ISA kind, the ELF header flags are only checked for pieces of code that have no symbol information available for some reason. And there is no point in overriding the ISA mode mandated by the executable, it must be clearly wrong, and there is really nothing to "guess" for GDB here -- all the required information is always available. Therefore the "auto" setting is meaningless -- you cannot infer any mode in the scope this setting has any use for, that is debugging with no executable (e.g. connecting to a live remote target), which is the very point of providing this setting at all. Also note that this setting is "sticky", that is it's set from any executable selected. If the executable is later on discarded, then the last setting is preserved assuming that if the user worked with one compressed ISA before it's more likely that they still use that ISA and not the other for non-debug code. Note this is a corner case too, but I decided it has to be handled somehow for robustness of GDB -- e.g. you may want to step-through power-on firmware you have no matching executable for via a JTAG probe for some reason. > This brings me to the next question: Could this be an objfile-specific > setting? In other words, is it possible to that the same executable > might have one objfile using micromips while another might still be > using mips16? (this might be the stupidest question of the week...). > If not, I still believe that this is at least an inferior-specific > property. With multi-inferior debugging being possible, I can see > how one debugs two programs using a different setting. In that case, > you need to store that information in the inferior private data > (I do that in ada-tasks, IIRC, for storing the layout of some data > structures). The question is of course fine and actually asked before in the process of adding microMIPS support to binutils. There is no way for MIPS16 and microMIPS code to be present in a single executable. We mandated this in binutils deciding that it would be a corner case not worth handling. Background information: the architecture does not permit the MIPS16 and the microMIPS ASE to be implemented in a single processor, one precludes the other (note that the architecture does permit an implementation that only supports the microMIPS mode and not the "traditional" standard MIPS mode, known since ~1985). Therefore the only case where an executable could contain both MIPS16 and microMIPS code would be a piece of software that dynamically determines which of the two ASEs is present on a processor and switch to the right set of procedures on the fly (this is similar to what some power-on firmware does to handle both endiannesses with a single image on systems where you can switch the endianness on the fly, either with a physical switch or via some board logic). We decided this is too much complication for little possibility of this case to ever matter for anyone. And personally I think the case of mixing MIPS16- and microMIPS-capable processors in a multi-processor systems (for multi-inferior debugging) is very unlikely too. I don't think anyone has plans to support such a system, that would sort of be missing the point (the microMIPS ASE is intended to replace the MIPS16 ASE in deployments where code density matters), not even mentioning this would most likely require different base processor types too as I don't expect any single MIPS processor to support the choice between the MIPS16 ASE and the microMIPS ASE as RTL options (currently only the MIPS M14K and M14Kc processors support the microMIPS ASE and neither base architecture supports the MIPS16 ASE as an option). So until (or really unless) somebody comes with the requirement to support both the microMIPS and the MIPS16 ASE at a time in a single debug session let's keep this simple. > (oops, style issue as well, the opening curly bracket should be > at the start of the next line - we've seen a lot of your style too, > but I think it should be fixed) Yes, I've changed that, thanks. Actually the very preceding array uses the very style I did. ;) Actually I keep getting confused about the style expected for aggregate types, especially in the context of initialisers. So for example is this correct: static int foo (void) { struct { int i; void *p; } s[] = { { 1, &foo }, { 2, NULL } }; } or should that be written yet differently? What if that's defined at the file scope: struct bar { int i; void *p; } s[] = { { 1, &foo }, { 2, NULL } }; or: struct bar { int i; void *p; } s[] = { { 1, &foo }, { 2, NULL } }; ? I recall seeing both styles (plus some variations), but my inclination is not to give indentation to curly brackets at the file level in any cases -- compare global functions vs nested functions (a GCC extension). Then this gets even weirder with C99's compound literals, e.g. this piece gets tricky: void baz(struct bar b[]) { } void bax(void) { baz ((struct bar[]) { { .i = 3, .p = &bax }, { .i = 4, .p = NULL } }); } when you need to wrap the line (e.g. add third element), sigh... Maciej