From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Kingdon To: gdb-patches@sourceware.cygnus.com Subject: Re: `long double' support for ix86 targets Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: References: <200003031531.e23FV8T00285@delius.kettenis.local> X-SW-Source: 2000-q1/msg00477.html > Kevin's changes to findvar.c:extract_floating() and store_floating() > together with some further analysis have convinced me that for all but > one ix86 targets 96-bit long doubles of type &floatformat_i387_ext are > the right thing. That analysis looks sound to me. Unless you get some feedback from others, I'd go with that patch. >From ac131313@cygnus.com Sat Apr 01 00:00:00 2000 From: Andrew Cagney To: Jimmy Guo Cc: gdb-patches@sourceware.cygnus.com Subject: Re: Initialization of hpux_threads Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38D849A1.F5F706A@cygnus.com> References: X-SW-Source: 2000-q1/msg00894.html Content-length: 1125 Jimmy Guo wrote: > > I was looking at an old source tree and didn't see its 'revival'. > Yes it is some setup to solve the kinds of problem you mentioned. > But any other usage of it will only introduce multiple calls to > a _initialize_* routine. > > The initializer in hpux-thread.c is just one of these cases. They're > mostly fixed as of today's tree except for a remaining one - > remote-nrom.c. Thanks, I've applied the attatched. Andrew Wed Mar 22 15:09:34 2000 Andrew Cagney * configure.in (CONFIG_INITS): Do not append remote-nrom.c Index: configure.in =================================================================== RCS file: /cvs/src/src/gdb/configure.in,v retrieving revision 1.12 diff -p -r1.12 configure.in *** configure.in 2000/03/20 06:41:24 1.12 --- configure.in 2000/03/22 04:15:36 *************** esac]) *** 447,453 **** if test "${enable_netrom}" = "yes"; then CONFIG_OBS="${CONFIG_OBS} remote-nrom.o" CONFIG_SRCS="${CONFIG_SRCS} remote-nrom.c" - CONFIG_INITS="${CONFIG_INITS} remote-nrom.c" fi AC_ARG_ENABLE(build-warnings, --- 447,452 ---- >From msnyder@cygnus.com Sat Apr 01 00:00:00 2000 From: Michael Snyder To: Dmitry Sivachenko Cc: gdb-patches@sourceware.cygnus.com Subject: Re: problem with gdb.texinfo Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38DA67A1.3094@cygnus.com> References: <200003231748.UAA57854@netserv1.chg.ru> X-SW-Source: 2000-q1/msg00942.html Content-length: 347 Dmitry Sivachenko wrote: > > Hello! > It seems that last commit to gdb.texinfo (which lead to rev. 1.4) > broke building of info-file. > Now makeinfo reports: Too many columns in multitable item (max 3). > > Probable it is necessary to back out this commit. I don't get that error. What version of makeinfo are you using? Michael Snyder >From ac131313@cygnus.com Sat Apr 01 00:00:00 2000 From: Andrew Cagney To: Eli Zaretskii Cc: gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH] Some compiler warnings removed Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38B47B3A.24D778B2@cygnus.com> References: <200002232235.RAA18817@indy.delorie.com> X-SW-Source: 2000-q1/msg00348.html Content-length: 1753 Eli Zaretskii wrote: > > Here's my share of shutting up "gcc -Wall -W". Is "foo = foo;" an > okay solution for unused argument foo? > > These diffs are against Feb 17 snapshot. I hope they are still > relevant and apply cleanly. > > --- gdb/blockframe.c~0 Wed Dec 22 21:45:02 1999 > +++ gdb/blockframe.c Wed Feb 23 16:51:44 2000 > @@ -70,6 +70,7 @@ nonnull_frame_chain_valid (chain, thisfr > CORE_ADDR chain; > struct frame_info *thisframe; > { > + thisframe = thisframe; > return ((chain) != 0); > } > FWIW, The way GCC handles this is by appending ATTRIBUTE_UNUSED to the parameter declaratons. I see no reason for doing it differently. Also, I'd suggest configuring with: CFLAGS=-g .../configure --enable-build-warnings=-Wunused so that you don't have to chase down too many warning in a single hit. That in turn constrains the patch to a specific type of change and makes it very easy for people to approve. > { > - /* On AIX and i386 GNU/Linux, floating point values are returned in > - floating point registers. */ > -#if defined(I386_AIX_TARGET) || defined(I386_GNULINUX_TARGET) > + /* On AIX, i386 GNU/Linux and DJGPP, floating point values are > + returned in floating point registers. */ > +#if defined(I386_AIX_TARGET) || defined(I386_GNULINUX_TARGET) || defined(I386_DJGPP_TARGET) > if (TYPE_CODE_FLT == TYPE_CODE (type)) > { > double d; > @@ -720,7 +720,7 @@ i386_extract_return_value (type, regbuf, > store_floating (valbuf, TYPE_LENGTH (type), d); > } > else > -#endif /* I386_AIX_TARGET || I386_GNULINUX_TARGET*/ > +#endif /* I386_AIX_TARGET || I386_GNULINUX_TARGET || I386_DJGPP_TARGET */ This one looks more like an actual code change? enjoy, Andrew >From ac131313@cygnus.com Sat Apr 01 00:00:00 2000 From: Andrew Cagney To: Eli Zaretskii Cc: gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH] COPYING and copying.c Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38C2232A.D07D7D4F@cygnus.com> References: <200002270857.DAA25056@indy.delorie.com> <38BCD91E.8F9DF1A0@cygnus.com> <200003040707.CAA08174@indy.delorie.com> X-SW-Source: 2000-q1/msg00523.html Content-length: 1143 Eli Zaretskii wrote: > > > > +# When DJGPP Make runs on MS-DOS, it downcases all file names, so > > > +# it doesn't find COPYING, and wants to make it... > > > +copying.txt: > > > + test -f copying.txt || ln COPYING copying.txt > > > > This bit won't work when building in a separate directory. > > I'm also not sure about the ``ln'' command. configure.in contains stuff > > to set ``LN_S''. > > Then how about the following changes? They seem to work for me. > > 2000-03-03 Eli Zaretskii > > * Makefile.in (copying.c): Depend on copying.txt, not COPYING. > (copying.txt): New target, a link to COPYING. > (LN_S): Define. (covers eyes, looks other way ... :-) I don't have any problems with this. > +LN_S = @LN_S@ Not needed, recently added. > + { test "$$LN_S" = "ln -s" && \ > + ln -s $(srcdir)/COPYING copying.txt; } || \ > + cp -p $(srcdir)/COPYING copying.txt 2>/dev/null || \ > + ln $(srcdir)/COPYING copying.txt 2>/dev/null || \ > + cp $(srcdir)/COPYING copying.txt People tend to use ``( ... )'' instead of ``{ ... }''. Andrew >From ac131313@cygnus.com Sat Apr 01 00:00:00 2000 From: Andrew Cagney To: Jim Kingdon Cc: gdb-patches@sourceware.cygnus.com Subject: Re: GDB-5 2000-03-03 Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38C07857.36B3FB41@cygnus.com> References: <38BFBD74.79263683@cygnus.com> <200003031714.e23HEC900400@delius.kettenis.local> X-SW-Source: 2000-q1/msg00493.html Content-length: 614 Jim Kingdon wrote: > > > Almost nothing appears to have happened for Linux/i386 since you > > announced your plans for 5.0 :-(. I haven't heard from JimB yet ... > > Heard from him about what? In case it isn't clear, we made you > maintainer so that you wouldn't have to hear from him about every > patch (unless you think you need to in a particular case). Jim, in this case (where a patch is submitted before someone becomes maintainer) some general confusion is allowed. If I was in Mark's boots I wouldn't be sure what the state of many patches (especially the non Linux specific ones) were in. Andrew >From eliz@delorie.com Sat Apr 01 00:00:00 2000 From: Eli Zaretskii To: ac131313@cygnus.com Cc: gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH] Support for building with DJGPP Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <200003201005.FAA09379@indy.delorie.com> References: <200003191224.HAA08367@indy.delorie.com> <38D58623.3FBFFE30@cygnus.com> X-SW-Source: 2000-q1/msg00810.html Content-length: 410 > > * config/djgpp/djconfig.sh: New file. > > * config/djgpp/config.sed: New file. > > * config/djgpp/README: New file. > > * config/djgpp/fnchange.lst: New file. > > * config/djgpp/djcheck.sh: New file. > > > > I'd also add an entry to gdb/NEWS. (Yaa) It's on my TODO list. But since I didn't yet see the call for NEWS entries, I'm busy with more urgent items ;-). >From ac131313@cygnus.com Sat Apr 01 00:00:00 2000 From: Andrew Cagney To: Jim Blandy Cc: gdb-patches@sourceware.cygnus.com Subject: Re: RFA: gdbarch_free Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38BB1A9A.61A680AA@cygnus.com> References: <200002282302.SAA13215@zwingli.cygnus.com> X-SW-Source: 2000-q1/msg00409.html Content-length: 635 Jim Blandy wrote: > > There are no uses for this yet, except in some code that's not ready > for release yet. > > 2000-02-28 Jim Blandy > > * gdbarch.sh: Emit a definition and declaration for gdbarch_free, > a companion to gdbarch_alloc, which allows a gdbarch init function > to free partially-built gdbarch structures. > * gdbarch.c, gdbarch.h: Regenerated. Jim, Can you expand a little. >From memory, I figured that if an _initialize* function failed to create a gdbarch the process was somewhat hosed and calling internal_error() was probably the best thing to do. Andrew >From jimb@zwingli.cygnus.com Sat Apr 01 00:00:00 2000 From: Jim Blandy To: gdb-patches@sourceware.cygnus.com Subject: RFA: minor watchpoint code cleanup Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <200003180006.TAA26919@zwingli.cygnus.com> X-SW-Source: 2000-q1/msg00761.html Content-length: 1675 This is just a cosmetic change. Let me know. 2000-03-17 Jim Blandy * i386v-nat.c (i386_insert_nonaligned_watchpoint): Use a two-dimensional array, instead of faking it with explicit index arithmetic. Index: i386v-nat.c =================================================================== RCS file: /cvs/src/src/gdb/i386v-nat.c,v retrieving revision 1.1.1.3 diff -c -c -b -F'^(' -r1.1.1.3 i386v-nat.c *** i386v-nat.c 1999/12/08 02:50:38 1.1.1.3 --- i386v-nat.c 2000/03/17 23:57:07 *************** *** 229,240 **** int size; int rv; ! static int size_try_array[16] = { ! 1, 1, 1, 1, /* trying size one */ ! 2, 1, 2, 1, /* trying size two */ ! 2, 1, 2, 1, /* trying size three */ ! 4, 1, 2, 1 /* trying size four */ }; rv = 0; --- 229,240 ---- int size; int rv; ! static int size_try_array[4][4] = { ! { 1, 1, 1, 1 }, /* trying size one */ ! { 2, 1, 2, 1 }, /* trying size two */ ! { 2, 1, 2, 1 }, /* trying size three */ ! { 4, 1, 2, 1 } /* trying size four */ }; rv = 0; *************** *** 242,249 **** { align = addr % 4; /* Four is the maximum length for 386. */ ! size = (len > 4) ? 3 : len - 1; ! size = size_try_array[size * 4 + align]; rv = i386_insert_aligned_watchpoint (pid, waddr, addr, size, rw); if (rv) --- 242,249 ---- { align = addr % 4; /* Four is the maximum length for 386. */ ! size = (len > 4) ? 4 : len; ! size = size_try_array[size - 1][align]; rv = i386_insert_aligned_watchpoint (pid, waddr, addr, size, rw); if (rv) >From blizzard@redhat.com Sat Apr 01 00:00:00 2000 From: Chris Blizzard To: Jim Kingdon Cc: gdb-patches@sourceware.cygnus.com, blizzard@mozilla.org Subject: Re: dlclose() Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <38AB1394.B604B719@redhat.com> References: <200002161953.OAA07411@devserv.devel.redhat.com> X-SW-Source: 2000-q1/msg00203.html Content-length: 6783 Jim Kingdon wrote: > > Here's another patch for the dlclose() problem we've been talking > about (5130 in bugzilla.redhat.com and all). I'm not sure how well I > like this patch but it should avoid the problem with flushing and > reloading shared library symbols all the time as in the HJ/Sam patch. > > Chris, want to weigh in on this (e.g. try out the patches)? Mozilla > is going to be one of the most affected if we do something dumb (or > something great) with shared library support. > Well, it doesn't seem any worse. :) --Chris > 2000-02-16 Jim Kingdon > > * solib.c (find_solib): New argument recheck. > * solib.c (solib_add): Pass it as 1, and add logic to delete > shared libraries which aren't still in the inferior. > (struct so_list): New field found_me, for solib_add. > * solib.c (other find_solib callers): Pass recheck as 0. > > Index: solib.c > =================================================================== > RCS file: /cvs/src/src/gdb/solib.c,v > retrieving revision 1.1.1.10 > diff -u -r1.1.1.10 solib.c > --- solib.c 1999/11/17 02:30:28 1.1.1.10 > +++ solib.c 2000/02/16 19:43:12 > @@ -31,6 +31,7 @@ > #include "gdb_string.h" > #include > #include > +#include > > #ifndef SVR4_SHARED_LIBS > /* SunOS shared libs need the nlist structure. */ > @@ -143,6 +144,11 @@ > char so_name[MAX_PATH_SIZE]; /* shared object lib name (FIXME) */ > char symbols_loaded; /* flag: symbols read in yet? */ > char from_tty; /* flag: print msgs? */ > + > + /* Flag for use within solib_add: have we seen this library actually > + still mapped in the inferior this pass? */ > + char found_me; > + > struct objfile *objfile; /* objfile for loaded lib */ > struct section_table *sections; > struct section_table *sections_end; > @@ -181,8 +187,7 @@ > > static int symbol_add_stub PARAMS ((PTR)); > > -static struct so_list * > - find_solib PARAMS ((struct so_list *)); > +static struct so_list *find_solib (struct so_list *, int); > > static struct link_map * > first_link_map_member PARAMS ((void)); > @@ -975,8 +980,10 @@ > */ > > static struct so_list * > -find_solib (so_list_ptr) > - struct so_list *so_list_ptr; /* Last lm or NULL for first one */ > +find_solib (struct so_list *so_list_ptr, > + /* Nonzero if we should read all the entries from the inferior, > + not just the ones at the end of the list. */ > + int recheck) > { > struct so_list *so_list_next = NULL; > struct link_map *lm = NULL; > @@ -985,7 +992,9 @@ > if (so_list_ptr == NULL) > { > /* We are setting up for a new scan through the loaded images. */ > - if ((so_list_next = so_list_head) == NULL) > + so_list_next = so_list_head; > + if (so_list_next == NULL > + || recheck) > { > /* We have not already read in the dynamic linking structures > from the inferior, lookup the address of the base structure. */ > @@ -1002,7 +1011,8 @@ > { > /* We have been called before, and are in the process of walking > the shared library list. Advance to the next shared object. */ > - if ((lm = LM_NEXT (so_list_ptr)) == NULL) > + lm = LM_NEXT (so_list_ptr); > + if (recheck || lm == NULL) > { > /* We have hit the end of the list, so check to see if any were > added, but be quiet if we can't read from the target any more. */ > @@ -1020,7 +1030,7 @@ > } > so_list_next = so_list_ptr->next; > } > - if ((so_list_next == NULL) && (lm != NULL)) > + if ((so_list_next == NULL || recheck) && (lm != NULL)) > { > /* Get next link map structure from inferior image and build a local > abbreviated load_map structure */ > @@ -1188,7 +1198,7 @@ > /* Count how many new section_table entries there are. */ > so = NULL; > count = 0; > - while ((so = find_solib (so)) != NULL) > + while ((so = find_solib (so, 0)) != NULL) > { > if (so->so_name[0] && !match_main (so->so_name)) > { > @@ -1201,7 +1211,7 @@ > > /* Add these section table entries to the target's table. */ > old = target_resize_to_sections (target, count); > - while ((so = find_solib (so)) != NULL) > + while ((so = find_solib (so, 0)) != NULL) > { > if (so->so_name[0]) > { > @@ -1215,9 +1225,13 @@ > } > } > > + for (so = so_list_head; so != NULL; so = so->next) > + so->found_me = 0; > + > /* Now add the symbol files. */ > - while ((so = find_solib (so)) != NULL) > + while ((so = find_solib (so, 1)) != NULL) > { > + so->found_me = 1; > if (so->so_name[0] && re_exec (so->so_name) && > !match_main (so->so_name)) > { > @@ -1240,6 +1254,37 @@ > } > } > > + { > + struct so_list *prev; > + prev = NULL; > + for (so = so_list_head; so != NULL; so = so->next) > + { > + if (!so->found_me) > + { > + free_objfile (so->objfile); > + > + /* FIXME: also need to fix up section tables that > + we put in the target vector, right? */ > + if (so_list_head->sections) > + { > + free ((PTR) so_list_head->sections); > + } > + > + assert (so->abfd != NULL); > + if (!bfd_close (so->abfd)) > + warning ("cannot close \"%s\": %s", > + so->so_name, bfd_errmsg (bfd_get_error ())); > + if (prev == NULL) > + so_list_head = so->next; > + else > + prev->next = so->next; > + > + free (so); > + } > + prev = so; > + } > + } > + > /* Getting new symbols may change our opinion about what is > frameless. */ > if (so_last) > @@ -1289,7 +1334,7 @@ > addr_fmt = "016l"; > #endif > > - while ((so = find_solib (so)) != NULL) > + while ((so = find_solib (so, 0)) != NULL) > { > if (so->so_name[0]) > { > @@ -1347,7 +1392,7 @@ > { > register struct so_list *so = 0; /* link map state variable */ > > - while ((so = find_solib (so)) != NULL) > + while ((so = find_solib (so, 0)) != NULL) > { > if (so->so_name[0]) > { -- ------------ Christopher Blizzard http://people.redhat.com/blizzard/ A few years back, I saw a young child stuck in a tree. Nowadays, when I find myself in a troubling situation, I look back and wonder if that kid saw me take that chocolate bar from his backpack on the ground. ------------ >From kettenis@wins.uva.nl Sat Apr 01 00:00:00 2000 From: Mark Kettenis To: kingdon@redhat.com, jimb@cygnus.com Cc: gdb-patches@sourceware.cygnus.com Subject: Re: glibc 2.1.3: elf_gregset_t Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <200002282324.e1SNO8l10978@delius.kettenis.local> References: <200002141733.MAA23437@devserv.devel.redhat.com> <200002141833.TAA29671@landau.wins.uva.nl> <200002280255.VAA19901@devserv.devel.redhat.com> X-SW-Source: 2000-q1/msg00407.html Content-length: 1349 Date: Sun, 27 Feb 2000 21:55:42 -0500 From: Jim Kingdon > Here's my list of Linux/i386 issues: Well, if I correctly read Andrew's message in the other thread, you should now be able to offer your thumbs up on these (if the patches are ready) and have Andrew check in the patches. Well, I'd love to see my patches checked in, but JimB still is the maintainer of Linux/x86 native. A few weeks ago he wrote that my patches were not lost and that he would be looking into them (JimB: in case you lost track I can compile a list of Linux/x86 that are still up for review). I have no problems if you guys want me to take over maintainership of Linux/x86 native. I think I have a bit more time to spend on it than Jimb, but I probaby lack some experience. As for the x86 target: I do not have any experience with embedded targets. I think it is for Andrew and/or Stan to decide if that's aproblem or not. P.S. You've convinced me on elf_gregset_t - using your (MarkK's) patch is fine with me. Great :-) P.P.S. If there is anything else I can do about your backlog of submitted patches (or anything else GDB), MarkK, please let me know. Seems to me that things are getting worked out. They're sitting happily in my checked out tree, waiting for checkin once JimB tells me to go ahead. Mark >From taylor@cygnus.com Sat Apr 01 00:00:00 2000 From: David Taylor To: Jim Blandy Cc: Andrew Cagney , gdb-patches@sourceware.cygnus.com Subject: Re: RFA: put COERCE_FLOAT_TO_DOUBLE under gdbarch's control Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <200002211641.LAA06687@texas.cygnus.com> X-SW-Source: 2000-q1/msg00280.html Content-length: 797 Date: Fri, 18 Feb 2000 17:40:40 -0500 (EST) From: Jim Blandy For this, I need approval from: - David Taylor, for the changes to valops.c and value.h Approved. I do however request that you put a comment before the default definition of COERCE_FLOAT_TO_DOUBLE to remind people how to use it -- maybe just a simplified version of the gdbint.texinfo stuff. - Andrew Cagney, for the changes to gdbarch.{sh,c,h} (which you've seen before), and to mips-tdep.c (you haven't seen those before). The changes to the eight target-specific files are trivial, so I don't think we need approval from those port maintainers. It still compiles and runs for MIPS and D10V (which are multi-arched targets) and for Solaris UltraSPARC (which isn't). >From kevinb@cygnus.com Sat Apr 01 00:00:00 2000 From: Kevin Buettner To: David Taylor , gdb-patches@sourceware.cygnus.com Subject: RFA: utils.c changes for converting to/from DOUBLEST Date: Sat, 01 Apr 2000 00:00:00 -0000 Message-id: <1000118013340.ZM30925@ocotillo.lan> X-SW-Source: 2000-q1/msg00023.html Content-length: 15245 David, et.al, I've been working on a port of gdb to a 64 bit architecture. The floating point registers for this architecture are 82 bits long. I've defined an appropriate floatformat struct for this architecture, but found several bugs in the functions responsible for converting these arbitrary sized floating point numbers to DOUBLEST (and back again). Here is a summary of the problems: 1) Buffer overruns and underruns. 2) Little endian formats whose length in bits is not evenly divisible by eight ends up accessing the wrong set of bits. 3) When extracting a field which is entirely contained in one byte, not all of the bits are zero'd that should be. My patch appears in its entirety at the end of this message. It is somewhat complicated, so I will attempt to explain the various problems and why my patch fixes these problems. The fixes are in get_field() and put_field(). These functions are called by floatformat_to_doublest() and floatformat_from_doublest() (respectively) to extract (or insert) a bitfield of at most 32 bits from/to a floating point number. get_field() works by first computing a starting byte index (cur_byte) and an amount to shift (cur_bitshift). The index refers to a byte in the floating point number to convert and will start off (always) referring to a *partial* byte. By this I mean that some (perhaps all) of the bits will always need to be shifted out, leaving only the bits forming the low part of the bitfield. Even when the the starting byte *could* refer to the actual full first byte of the field to be extracted, it won't. In this case, the algorithm will arrange for cur_bitshift to be -8 which will cause the byte in question to be discarded. (If you're getting an uneasy feeling at this point, I assure you that your unease is justified.) Let us first examine the code that the original algorithm uses to set cur_byte and cur_bitshift. It looked like this: cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1; cur_bitshift = ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; Here is the equivalent code in the revised algorithm (comments elided): if (order == floatformat_little || order == floatformat_littlebyte_bigword) { int excess = FLOATFORMAT_CHAR_BIT - (total_len % FLOATFORMAT_CHAR_BIT); cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - ((start + len + excess) / FLOATFORMAT_CHAR_BIT); cur_bitshift = ((start + len + excess) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; } else { cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; cur_bitshift = ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; } The first thing to note is that the manner in which cur_byte and cur_bitshift is computed for big endian float formats remains the same. Now let's consider the computation of cur_byte for little endian (and littlebyte_bigword) formats when total_len is evenly divisible by 8. (All existing floatformat structs have a total_len which is evenly divisible by 8.) When total_len is evenly divisible by 8, excess will be set to 8. (Note: FLOATFORMAT_CHAR_BIT is defined to be 8.) The fact that excess is 8 in this circumstance means that cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - ((start + len + 8) / FLOATFORMAT_CHAR_BIT => cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - (((start + len) / FLOATFORMAT_CHAR_BIT + 1) => cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - ((start + len) / FLOATFORMAT_CHAR_BIT - 1 If we choose, we could rewrite this as two assignment statements: cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1; In other words, cur_byte will be computed the same way in both the original and revised algorithms when total_len is evenly divisible by 8. Now let's do a similar analysis of cur_bitshift when total_len is evenly divisible by 8... Again excess will be 8. This means that cur_bitshift = ((start + len + 8) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; But since FLOATFORMAT_CHAR_BIT is defined to be 8, we can rewrite the above as cur_bitshift = ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; Note that this is the same statement which was used to compute cur_bitshift in the original algorithm. Okay... we've verified that both the original and revised algorithms will function the same when working on big endian floats. We've also verified that they will operate the same way for little endian floats whose length in bits is evenly divisible by 8. This means that this portion of the revised algorithm will continue to function as it always has for all existing floatformats. So what happens for little endian floatformats when total_len is not evenly divisible by eight? Let us consider a simple example... Example 1: Let order == floatformat_little, total_len == 14, start == 1 and len == 2. This means that we wish to extract two bits as shown in the following picture (Note that the bit numbering used by the algorithm has bit zero referring to the most significant bit; big endian formats have a very regular structure with bit 0 being on the far left; OTOH, little endian is somewhat messy): Byte 0 Byte 1 | 1 1 1 1 | | | 3 2 1 0 9 8 7 6 | 5 4 3 2 1 0 _ _ | -+- | +-- bits to extract The original algorithm would cause cur_byte to be set to 0 and cur_bitshift to be set to -5. This means that byte 0 is fetched and the contents will be shifted right by 5. This is plainly wrong because a) The wrong byte is being extracted from. b) Even if it were the correct byte, the shift value is incorrect. c) Assuming that both a and b were okay, the high bits after the shift are being retained. The revised algorithm causes cur_byte to be set to 1 and cur_bitshift to be set to -3. This means that byte 1 is fetched and the contents are shifted right by 3 (which is correct). Zeroing of unwanted bits is taken care of later on in the revised algorithm. Let us now turn our attention to the fetching of the first byte. The original algorithm did it like this: result = *(data + cur_byte) >> (-cur_bitshift); Remember that uneasy feeling you had (or should have had) before? I'll now give two examples to help perhaps explain that unease. Example 2: Let order=floatformat_big, total_len=16, start=0, len=16. We can illustrate the situation with the following picture: Byte 0 Byte 1 | | 1 1 1 1 1 1 | | 0 1 2 3 4 5 6 7 | 8 9 0 1 2 3 4 5 | -------------------------+------- | +-- bits to extract cur_byte will be 2 for this example and cur_bitshift will be -8. But byte 2 doesn't appear in the picture! Well, suppose we extract it anyway. When we do, we'll just shift it right by 8 (--8 == 8) which means that it'll get entirely shifted away! Example 3: Let order=floatformat_little, total_len=16, start=0, len=16. (Same as example 2, but with little endian byte order.) This has a similar looking picture. Byte 0 Byte 1 | 1 1 1 1 1 1 | | | 5 4 3 2 1 0 9 8 | 7 6 5 4 3 2 1 0 | -------------------------+------- | +-- bits to extract In this case (regardless of which algorithm is used), cur_byte will be -1. Sort of. cur_byte is unsigned, so it'll actually be the largest unsigned integer possible. On machines which have the property that sizeof(unsigned int) == sizeof (unsigned char *), this code will probably work. The byte immediately before byte 0 will be read, even though this is technically a buffer underrun. (Just as example 2 illustrates an overrun.) On a machine where sizeof(unsigned int) == 4 and sizeof(unsigned char *) == 8, the above code is disasterous. The machine will attempt to fetch data + 4294967296 which will likely result in a segmentation violation. (It did on the platform that I'm porting to.) Okay, so we have overruns and underruns. How do we fix it? When this case occurs, cur_bitshift will always be -8 which means that *if* the data in question could be fetched safely, it would be shifted into nothingness anyway. Here is how we do it safely in the revised algorithm: if (cur_bitshift > -FLOATFORMAT_CHAR_BIT) result = *(data + cur_byte) >> (-cur_bitshift); else result = 0; I.e, we simply avoid fetching the over/underrun byte and zero out our result, since the byte would be shifted into nothingness anyway. The original algorithm contains the following lines which appear in the while loop for fetching the bytes after the first byte: if (len - cur_bitshift < FLOATFORMAT_CHAR_BIT) /* This is the last byte; zero out the bits which are not part of this field. */ result |= (*(data + cur_byte) & ((1 << (len - cur_bitshift)) - 1)) << cur_bitshift; else As the comment says, these lines are supposed to handle the zeroing of the unwanted bits. And in fact it does (if it ever gets into the loop). But as we saw in example 1, this code does not account for what happens when the bit field to extract doesn't straddle two (or more) bytes. In order to account for both cases, I simply moved the zeroing of unwanted bits after the while loop like so: if (len < sizeof(result) * FLOATFORMAT_CHAR_BIT) /* Mask out bits which are not part of the field */ result &= ((1UL << len) - 1); The changes to put_field() are very similar to those made to get_field(), with the exception that the problem of zeroing unwanted bits doesn't exist. Therefore I won't give a blow-by-blow account. (Unless you really want me to.) I have tested this patch on Linux/x86 and Solaris/sparc and see no new regressions. (I chose these two because they're little endian and big endian respectively.) I request approval for committing these changes. * utils.c (get_field, put_field): Fix buffer underruns and overruns. Also, handle case where total_len is not evenly divisible by 8. (getfield): Make sure zeroing of unwanted bits occurs even when bit field to extract does not straddle two or more bytes. Index: utils.c =================================================================== RCS file: /cvs/cvsfiles/devo/gdb/utils.c,v retrieving revision 1.229 diff -u -p -r1.229 utils.c --- utils.c 1999/12/14 12:40:46 1.229 +++ utils.c 2000/01/17 23:53:39 @@ -3203,12 +3203,31 @@ get_field (data, order, total_len, start int cur_bitshift; /* Start at the least significant part of the field. */ - cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) - cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1; - cur_bitshift = - ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; - result = *(data + cur_byte) >> (-cur_bitshift); + { + /* We start counting from the other end (i.e, from the high bytes + rather than the low bytes). As such, we need to be concerned + with what happens if bit 0 doesn't start on a byte boundary. + I.e, we need to properly handle the case where total_len is + not evenly divisible by 8. So we compute ``excess'' which + represents the number of bits from the end of our starting + byte needed to get to bit 0. */ + int excess = FLOATFORMAT_CHAR_BIT - (total_len % FLOATFORMAT_CHAR_BIT); + cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) + - ((start + len + excess) / FLOATFORMAT_CHAR_BIT); + cur_bitshift = ((start + len + excess) % FLOATFORMAT_CHAR_BIT) + - FLOATFORMAT_CHAR_BIT; + } + else + { + cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; + cur_bitshift = + ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; + } + if (cur_bitshift > -FLOATFORMAT_CHAR_BIT) + result = *(data + cur_byte) >> (-cur_bitshift); + else + result = 0; cur_bitshift += FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) ++cur_byte; @@ -3218,20 +3237,16 @@ get_field (data, order, total_len, start /* Move towards the most significant part of the field. */ while (cur_bitshift < len) { - if (len - cur_bitshift < FLOATFORMAT_CHAR_BIT) - /* This is the last byte; zero out the bits which are not part of - this field. */ - result |= - (*(data + cur_byte) & ((1 << (len - cur_bitshift)) - 1)) - << cur_bitshift; - else - result |= *(data + cur_byte) << cur_bitshift; + result |= (unsigned long)*(data + cur_byte) << cur_bitshift; cur_bitshift += FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) ++cur_byte; else --cur_byte; } + if (len < sizeof(result) * FLOATFORMAT_CHAR_BIT) + /* Mask out bits which are not part of the field */ + result &= ((1UL << len) - 1); return result; } @@ -3368,15 +3383,28 @@ put_field (data, order, total_len, start int cur_bitshift; /* Start at the least significant part of the field. */ - cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) - cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) - cur_byte - 1; - cur_bitshift = - ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; - *(data + cur_byte) &= - ~(((1 << ((start + len) % FLOATFORMAT_CHAR_BIT)) - 1) << (-cur_bitshift)); - *(data + cur_byte) |= - (stuff_to_put & ((1 << FLOATFORMAT_CHAR_BIT) - 1)) << (-cur_bitshift); + { + int excess = FLOATFORMAT_CHAR_BIT - (total_len % FLOATFORMAT_CHAR_BIT); + cur_byte = (total_len / FLOATFORMAT_CHAR_BIT) + - ((start + len + excess) / FLOATFORMAT_CHAR_BIT); + cur_bitshift = ((start + len + excess) % FLOATFORMAT_CHAR_BIT) + - FLOATFORMAT_CHAR_BIT; + } + else + { + cur_byte = (start + len) / FLOATFORMAT_CHAR_BIT; + cur_bitshift = + ((start + len) % FLOATFORMAT_CHAR_BIT) - FLOATFORMAT_CHAR_BIT; + } + if (cur_bitshift > -FLOATFORMAT_CHAR_BIT) + { + *(data + cur_byte) &= + ~(((1 << ((start + len) % FLOATFORMAT_CHAR_BIT)) - 1) + << (-cur_bitshift)); + *(data + cur_byte) |= + (stuff_to_put & ((1 << FLOATFORMAT_CHAR_BIT) - 1)) << (-cur_bitshift); + } cur_bitshift += FLOATFORMAT_CHAR_BIT; if (order == floatformat_little || order == floatformat_littlebyte_bigword) ++cur_byte;