Re: `long double' support for ix86 targets

[Jim Kingdon <kingdon@redhat.com>]

> 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 <ac131313@cygnus.com>
To: Jimmy Guo <guo@cup.hp.com>
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: <Pine.LNX.4.10.10003211552360.31590-100000@hpcll168.cup.hp.com>
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  <cagney@b1.cygnus.com>

	* 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 <msnyder@cygnus.com>
To: Dmitry Sivachenko <dima@Chg.RU>
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 <ac131313@cygnus.com>
To: Eli Zaretskii <eliz@delorie.com>
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 <ac131313@cygnus.com>
To: Eli Zaretskii <eliz@delorie.com>
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  <eliz@is.elta.co.il>
> 
>         * 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 <ac131313@cygnus.com>
To: Jim Kingdon <kingdon@redhat.com>
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> <b66v3g6zb.fsf@rtl.cygnus.com>
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 <eliz@delorie.com>
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 <ac131313@cygnus.com>
To: Jim Blandy <jimb@cygnus.com>
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  <jimb@redhat.com>
> 
>         * 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 <jimb@zwingli.cygnus.com>
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  <jimb@redhat.com>

	* 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 <blizzard@redhat.com>
To: Jim Kingdon <kingdon@redhat.com>
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  <kingdon@redhat.com>
> 
>         * 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 <sys/param.h>
>  #include <fcntl.h>
> +#include <assert.h>
> 
>  #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 <kettenis@wins.uva.nl>
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 <kingdon@redhat.com>

   > 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 <taylor@cygnus.com>
To: Jim Blandy <jimb@cygnus.com>
Cc: Andrew Cagney <cagney@cygnus.com>, 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 <jimb@cygnus.com>

    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 <kevinb@cygnus.com>
To: David Taylor <taylor@cygnus.com>, 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;