go32-nat, always a thread

go32-nat, always a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

This patch makes the go32 (djgpp) target always register the main thread
in GDB's thread table.

Anyone able to test this?  Eli?

I think this is fairly low risk, and very easy to fix
if I missed something.

OK?

-- 
Pedro Alves

[-- Attachment #2: go32-always_a_thread.diff --]
[-- Type: text/x-diff, Size: 2193 bytes --]

2008-08-08  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c: Include "gdbthread.h".
	(go32_stop, go32_kill_inferior): Delete the main thread.
	(go32_create_inferior): Add it.
	(go32_thread_alive, go32_pid_to_str): New.
	(init_go32_ops): Register go32_thread_alive and go32_pid_to_str.

---
 gdb/go32-nat.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c	2008-08-07 19:38:07.000000000 +0100
+++ src/gdb/go32-nat.c	2008-08-08 04:34:44.000000000 +0100
@@ -22,6 +22,7 @@
 
 #include "defs.h"
 #include "inferior.h"
+#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "gdbcore.h"
 #include "command.h"
@@ -562,9 +563,12 @@ go32_files_info (struct target_ops *targ
 static void
 go32_stop (ptid_t ptid)
 {
+  ptid_t ptid;
   normal_stop ();
   cleanup_client ();
+  ptid = inferior_ptid;
   inferior_ptid = null_ptid;
+  delete_thread_silent (ptid);
   prog_has_started = 0;
 }
 
@@ -574,6 +578,8 @@ go32_kill_inferior (void)
   redir_cmdline_delete (&child_cmd);
   resume_signal = -1;
   resume_is_step = 0;
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    delete_thread_silent (inferior_ptid);
   unpush_target (&go32_ops);
 }
 
@@ -656,6 +662,9 @@ go32_create_inferior (char *exec_file, c
 
   inferior_ptid = pid_to_ptid (SOME_PID);
   push_target (&go32_ops);
+
+  add_thread_silent (inferior_ptid);
+
   clear_proceed_status ();
   insert_breakpoints ();
   prog_has_started = 1;
@@ -846,6 +855,20 @@ go32_terminal_ours (void)
   }
 }
 
+static int
+go32_thread_alive (ptid_t ptid)
+{
+  return 1;
+}
+
+static char *
+go32_pid_to_str (ptid_t ptid)
+{
+  static char buf[64];
+  xsnprintf (buf, sizeof buf, "Thread <main>");
+  return buf;
+}
+
 static void
 init_go32_ops (void)
 {
@@ -876,6 +899,8 @@ init_go32_ops (void)
   go32_ops.to_mourn_inferior = go32_mourn_inferior;
   go32_ops.to_can_run = go32_can_run;
   go32_ops.to_stop = go32_stop;
+  go32_ops.to_stop = go32_thread_alive;
+  go32_ops.to_stop = go32_pid_to_str;
   go32_ops.to_stratum = process_stratum;
   go32_ops.to_has_all_memory = 1;
   go32_ops.to_has_memory = 1;

Re: go32-nat, always a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

On Friday 08 August 2008 04:39:17, Pedro Alves wrote:
> +  go32_ops.to_stop = go32_thread_alive;
> +  go32_ops.to_stop = go32_pid_to_str;

Well, this is probably a hint I should get to sleep.  :-)

Updated patch attached.

OK?

-- 
Pedro Alves

[-- Attachment #2: go32-always_a_thread.diff --]
[-- Type: text/x-diff, Size: 2207 bytes --]

2008-08-08  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c: Include "gdbthread.h".
	(go32_stop, go32_kill_inferior): Delete the main thread.
	(go32_create_inferior): Add it.
	(go32_thread_alive, go32_pid_to_str): New.
	(init_go32_ops): Register go32_thread_alive and go32_pid_to_str.

---
 gdb/go32-nat.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c	2008-08-07 19:38:07.000000000 +0100
+++ src/gdb/go32-nat.c	2008-08-08 04:40:23.000000000 +0100
@@ -22,6 +22,7 @@
 
 #include "defs.h"
 #include "inferior.h"
+#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "gdbcore.h"
 #include "command.h"
@@ -562,9 +563,12 @@ go32_files_info (struct target_ops *targ
 static void
 go32_stop (ptid_t ptid)
 {
+  ptid_t ptid;
   normal_stop ();
   cleanup_client ();
+  ptid = inferior_ptid;
   inferior_ptid = null_ptid;
+  delete_thread_silent (ptid);
   prog_has_started = 0;
 }
 
@@ -574,6 +578,8 @@ go32_kill_inferior (void)
   redir_cmdline_delete (&child_cmd);
   resume_signal = -1;
   resume_is_step = 0;
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    delete_thread_silent (inferior_ptid);
   unpush_target (&go32_ops);
 }
 
@@ -656,6 +662,9 @@ go32_create_inferior (char *exec_file, c
 
   inferior_ptid = pid_to_ptid (SOME_PID);
   push_target (&go32_ops);
+
+  add_thread_silent (inferior_ptid);
+
   clear_proceed_status ();
   insert_breakpoints ();
   prog_has_started = 1;
@@ -846,6 +855,20 @@ go32_terminal_ours (void)
   }
 }
 
+static int
+go32_thread_alive (ptid_t ptid)
+{
+  return 1;
+}
+
+static char *
+go32_pid_to_str (ptid_t ptid)
+{
+  static char buf[64];
+  xsnprintf (buf, sizeof buf, "Thread <main>");
+  return buf;
+}
+
 static void
 init_go32_ops (void)
 {
@@ -876,6 +899,8 @@ init_go32_ops (void)
   go32_ops.to_mourn_inferior = go32_mourn_inferior;
   go32_ops.to_can_run = go32_can_run;
   go32_ops.to_stop = go32_stop;
+  go32_ops.to_thread_alive = go32_thread_alive;
+  go32_ops.to_pid_to_str = go32_pid_to_str;
   go32_ops.to_stratum = process_stratum;
   go32_ops.to_has_all_memory = 1;
   go32_ops.to_has_memory = 1;

Re: go32-nat, always a thread

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 8 Aug 2008 04:39:17 +0100
> 
> This patch makes the go32 (djgpp) target always register the main thread
> in GDB's thread table.

What do we need this for?  DJGPP doesn't support threading; the ptid_t
variables in go32-nat.c are simply dummies devised to make the
compiler happy because go32-nat.c uses interfaces that pass thread
IDs.  But there's no infrastructure behind this farce.

The code you added will now cause all kinds of notifications, like
"thread started", be displayed, which is just confusing when the
programmer knows there're no threads.  Is this really justified?

> Anyone able to test this?  Eli?

It's been a while since I built the DJGPP port, so I will probably
need a good vacation to do that again.  But let's first decide if this
change is really necessary.

Re: go32-nat, always a thread

[Daniel Jacobowitz]

On Fri, Aug 08, 2008 at 11:00:47AM +0300, Eli Zaretskii wrote:
> What do we need this for?  DJGPP doesn't support threading; the ptid_t
> variables in go32-nat.c are simply dummies devised to make the
> compiler happy because go32-nat.c uses interfaces that pass thread
> IDs.  But there's no infrastructure behind this farce.

The GDB core has a list of threads.  Until recently, a single-threaded
program would usually have no threads in the list (though in some
cases, there would be a single thread).  This makes it difficult to
keep track of per-thread state in a uniform way; state that a
single-threaded program should have one copy of but a multi-threaded
program should have one copy per thread.

> The code you added will now cause all kinds of notifications, like
> "thread started", be displayed, which is just confusing when the
> programmer knows there're no threads.  Is this really justified?

No notifications should be displayed; that's because Pedro used
add_thread_silent and delete_thread_silent.

-- 
Daniel Jacobowitz
CodeSourcery

Re: go32-nat, always a thread

[Eli Zaretskii]

> Date: Fri, 8 Aug 2008 08:42:29 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org
> 
> > The code you added will now cause all kinds of notifications, like
> > "thread started", be displayed, which is just confusing when the
> > programmer knows there're no threads.  Is this really justified?
> 
> No notifications should be displayed; that's because Pedro used
> add_thread_silent and delete_thread_silent.

Then what do the calls to observer_notify_new_thread and its ilk do?

Re: go32-nat, always a thread

[Daniel Jacobowitz]

On Fri, Aug 08, 2008 at 03:46:26PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 8 Aug 2008 08:42:29 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org
> > 
> > > The code you added will now cause all kinds of notifications, like
> > > "thread started", be displayed, which is just confusing when the
> > > programmer knows there're no threads.  Is this really justified?
> > 
> > No notifications should be displayed; that's because Pedro used
> > add_thread_silent and delete_thread_silent.
> 
> Then what do the calls to observer_notify_new_thread and its ilk do?

They allow parts of GDB to respond to changes in the thread list.  The
user-visible notification isn't handled from there, but the MI front
end notification is; we decided (after a lot of discussion...) that
this is the desirable behavior, for the same reason it's useful in GDB
to always have at least one thread while the program is running.

-- 
Daniel Jacobowitz
CodeSourcery

Re: go32-nat, always a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 7291 bytes --]

On Fri, 8 Aug 2008 04:39:17 +0100, Pedro Alves wrote:
> Anyone able to test this?  Eli?

So, I took a few hours today to build a GDB on DJGPP.

After some hacks and patches, I succeeded.  

This is what is looks like with the attached patches installed:

 bash-2.04$ ./gdb ./gdb
 GNU gdb (GDB) 6.8.50.20080808-cvs
 Copyright (C) 2008 Free Software Foundation, Inc.
 License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
 This is free software: you are free to change and redistribute it.
 There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
 and "show warranty" for details.
 This GDB was configured as "--host=i586-pc-msdosdjgpp --target=djgpp".
 For bug reporting instructions, please see:
 <http://www.gnu.org/software/gdb/bugs/>...
 (gdb) start
 Temporary breakpoint 1 at 0x1f8c: file ../../gdb/gdb.c, line 28.
 Starting program: c:/djgdb/src/djgpp/gdb/./gdb.exe

 Temporary breakpoint 1, main (argc=1, argv=0x36da0c) at ../../gdb/gdb.c:28
 28        memset (&args, 0, sizeof args);
 (gdb) info threads
 * 1 Thread <main>  main (argc=1, argv=0x36da0c) at ../../gdb/gdb.c:28
 (gdb) kill
 Kill the program being debugged? (y or n) y
 (gdb) info threads
 (gdb) n
 The program is not being run.
 (gdb)

Here is a description of what I've done to build gdb on DJGPP:

I used a Windows XP machine.

Downloaded DJGPP 2.03, bash 2.04, and all the needed dependencies to
build GDB from djgpp's ftp archive.  These are all the zips I downloaded
and installed.  Most likelly a couple aren't needed, though.

 bnu217b.zip    djlsr203.zip  fil41d.zip    gwk316b.zip   rhid15ab.zip
 bnu217s.zip    djtst203.zip  find41b.zip   gwk316d.zip   rhid15as.zip
 bsh204br2.zip  em2005b.zip   find41d.zip   m4-149b.zip   sed415b.zip
 bsh204d.zip    em2005li.zip  flx254b.zip   m4-149d.zip   sed415d.zip
 bsn23b.zip     em2005s1.zip  gcc423b.zip   mak3791b.zip  shl2011b.zip
 bsn23s.zip     em2005s2.zip  gdb611b.zip   objc423b.zip  shl2011d.zip
 dif287b.zip    faq230b.zip   gpp423b.zip   pakk023b.zip  txi412b.zip
 dif287d.zip    faq230s.zip   grep253b.zip  pakk023s.zip  txt20b.zip
 djdev203.zip   fil41b.zip    grep253d.zip  pdcur33b.zip  txt20d.zip

I didn't need to use the special djunpack to rename gdb's sources to
8.3 format, since I was under bash + winxp, which sees long filenames.

 bash-2.04$ gcc -v
 Using built-in specs.
 Target: djgpp
 Configured with: /v203/gcc-4.23/configure djgpp --prefix=/dev/env/DJDIR --disable-nls --disable-werror --enable-languages=c,c++,fortran,objc,obj-c++,ada
 Thread model: single
 gcc version 4.2.3

I configured gdb out of tree with:

 ../src/configure \
       --target=i586-pc-msdosdjgpp \
       --host=i586-pc-msdosdjgpp \
        --build=i586-pc-msdosdjgpp

I to pass target,host to force a native build.  The default of not passing
any argument to configure, ended up building with target=i586-pc-msdosdjgpp
host=i386-pc-msdosdjgpp, which, being different, triggeres a cross build,
which excludes go32-nat.c.

About the first patch:

- The readline bits, the patch explains what's needed.

Here's the error log
gcc -DHAVE_CONFIG_H    -I. -I../../readline -DRL_LIBRARY_VERSION='"5.1"' -O0 -g3
 -c ../../readline/support/wcwidth.c
In file included from ../../readline/support/wcwidth.c:9:
c:/djgpp/include/wchar.h:24: error: expected '=', ',', ';', 'asm' or '__attribut
e__' before 'typedef'
../../readline/support/wcwidth.c: In function 'wcwidth':
../../readline/support/wcwidth.c:130: warning: comparison is always false due to
 limited range of data type
../../readline/support/wcwidth.c:130: warning: comparison is always true due to
limited range of data type
make.exe: *** [wcwidth.o] Error 1

- libbfd 

 cc1.exe: warnings being treated as errors
 ../../bfd/archive.c: In function '_bfd_archive_bsd_update_armap_timestamp':
 ../../bfd/archive.c:2314: warning: comparison between signed and unsigned

 time_t in djgpp is unsigned int, armap_timestamp is long.
 There's a comment at the definition of armap_timestamp, claiming that it
 isn't time_t until more compilers support it.
 
- libiberty:

 I get this error when building libiberty:

 rm -f needed-list; touch needed-list; \
 for f in atexit calloc memchr memcmp memcpy memmove memset rename strchr strerro
 r strncmp strrchr strstr strtol strtoul tmpnam vfprintf vprintf vfork waitpid bc
 mp bcopy bzero; do \
   for g in  ; do \
     case "$g" in \
       *$f*) echo $g >> needed-list ;; \
     esac; \
   done; \
 done
 c:\djgpp\tmp/dj750000: line 1: syntax error near unexpected token `;'
 c:\djgpp\tmp/dj750000: line 1: `rm -f needed-list; touch needed-list;  for f in
 atexit calloc memchr memcmp memcpy memmove memset rename strchr strerror strncmp
  strrchr strstr strtol strtoul tmpnam vfprintf vprintf vfork waitpid bcmp bcopy
 bzero; do  for g in  ; do  case "$g" in  *$f*) echo $g >> needed-list ;;  esac;
  done;  done'
 make.exe[2]: *** [needed-list] Error 2

 I can't tell what's going on here.  Maybe it's related to using make
 3.79.1.

 I just hacked the Makefile to just touch the needed-list file and do
 nothing else with it.  There's a comment in the makefile claining
 this is needed for libstdc++.

 For some reason, maybe related to the above, a few libiberty objects
 are not being built and archived:

 I manually added snprintf.o, vasprintf.o, asprintf.o and vsnprintf.o
 to REQUIRED_OFILES to libiberty's generated Makefile.  I don't plan to
 persue to proper fix for these issues.

GDB changes:

- DJGPP 2.03 doesn't come bundled with stdint.h, but I
  downloaded it djgpp's cvs (from the web frontend to CVS actually),
  and install it manually.  Seems to work fine.

  The build complains about not finding stdint.h.  DJGPP 2.03 doesn't
  come bundled with stdint.h, but we should be picking up
  stdint.h from gdb/gnulib.  There seems to be a build order
  dependency going wrong.  We configure gdb/gnulib before starting
  to build gdb/, but we build gdb/ before gdb/gnulib.

  I had to cd to build/gdb/gnulib, and "make" it.  This
  creates a gdb/gnulib/stdint.h, and also gdb/gnulib/string.h
  with memmem declared in it, which is needed to compile target.c, due

  Then I can come back to build/gdb, and continue building.

  This is using GNU Make 3.79.1, which is what prebuilt at djgpp's
  download site.  It seems to be that even if we don't
  support auto-dependencies with this make version, it should
  still build gnulib before gdb.  Actually, I just tried with
  GNU Make 3.81 on linux, and I see the same issue.

- "missing sentinel in function calls" warnings:

  NULL is not a pointer in djgpp.  GCC was barking on a few concat calls.

- cp-name-parse.y

  Needed to include config.h, so HAVE_DECL_SNPRINTF was defined,
  so libiberty.h declares snprintf.

- go32-nat.c:

  There was a bit of bit rot around.
  GCC complains about attribute packed like so:

 cc1.exe: warnings being treated as errors
 ../../gdb/go32-nat.c:1292: warning: 'packed' attribute ignored for field of type 'unsigned char'
 ../../gdb/go32-nat.c:1301: warning: 'packed' attribute ignored for field of type 'unsigned char'
 make.exe: *** [go32-nat.o] Error 1

  Just removed the attribute from the offending fields.

Are the GDB parts of patches OK?  I checked that
x86_64-unknown-linux-gnu still builds OK.

-- 
Pedro Alves

[-- Attachment #2: go32_fixes.diff --]
[-- Type: text/x-diff, Size: 7829 bytes --]

bfd/
2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* archive.c (_bfd_archive_bsd_update_armap_timestamp): Cast stat
	st_mtime to long before comparison.

gdb/
2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* buildsym.c (start_subfile): Cast sentinel NULL to void*.
	* cp-name-parser.y: Include "config.h".
	* posix-hdep.c [__GO32__]: Include time.h.
	* xml-tdesc.c (fetch_xml_from_file): Cast sentinel NULL to void*.
	* gdb_select.h: Include sys/types.h if available.
	* go32-nat.c (fetch_register, store_register): Pass the regcache
	gdbarch to i386_fp_regnum_p and i386_fpc_regnum_p.
	(go32_xfer_memory): Change type of myaddr parameter to gdb_byte.
	(struct seg_descr, struct seg_descr): Don't pack byte sized
	members.

readline/
2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* signals.c (rl_set_sighandler): Guard access to SIGWINCH.
	* wcwidth.c [__GO32__]: Include wctype.h before wchar.h.

---
 bfd/archive.c              |    2 +-
 gdb/buildsym.c             |    2 +-
 gdb/cp-name-parser.y       |    1 +
 gdb/gdb_select.h           |    4 ++++
 gdb/go32-nat.c             |   18 ++++++++++--------
 gdb/posix-hdep.c           |    6 ++++++
 gdb/xml-tdesc.c            |    2 +-
 readline/signals.c         |    4 ++++
 readline/support/wcwidth.c |    5 +++++
 9 files changed, 33 insertions(+), 11 deletions(-)

Index: src/gdb/buildsym.c
===================================================================
--- src.orig/gdb/buildsym.c
+++ src/gdb/buildsym.c
@@ -547,7 +547,7 @@ start_subfile (char *name, char *dirname
 	  && !IS_ABSOLUTE_PATH (subfile->name)
 	  && subfile->dirname != NULL)
 	subfile_name = concat (subfile->dirname, SLASH_STRING,
-			       subfile->name, NULL);
+			       subfile->name, (void*) NULL);
       else
 	subfile_name = subfile->name;
 
Index: src/gdb/cp-name-parser.y
===================================================================
--- src.orig/gdb/cp-name-parser.y
+++ src/gdb/cp-name-parser.y
@@ -36,6 +36,7 @@ Boston, MA 02110-1301, USA.  */
 #include <unistd.h>
 #include <string.h>
 
+#include "config.h"
 #include "safe-ctype.h"
 #include "libiberty.h"
 #include "demangle.h"
Index: src/gdb/posix-hdep.c
===================================================================
--- src.orig/gdb/posix-hdep.c
+++ src/gdb/posix-hdep.c
@@ -24,6 +24,12 @@
 
 #include "gdb_select.h"
 
+#ifdef __GO32__
+/* DJGPP defines the fd_set type in sys/types.h, but `select' goes
+   here. */
+# include <time.h>
+#endif
+
 /* The strerror() function can return NULL for errno values that are
    out of range.  Provide a "safe" version that always returns a
    printable string. */
Index: src/gdb/xml-tdesc.c
===================================================================
--- src.orig/gdb/xml-tdesc.c
+++ src/gdb/xml-tdesc.c
@@ -443,7 +443,7 @@ fetch_xml_from_file (const char *filenam
 
   if (dirname && *dirname)
     {
-      char *fullname = concat (dirname, "/", filename, NULL);
+      char *fullname = concat (dirname, "/", filename, (void*) NULL);
       if (fullname == NULL)
 	nomem (0);
       file = fopen (fullname, FOPEN_RT);
Index: src/readline/signals.c
===================================================================
--- src.orig/readline/signals.c
+++ src/readline/signals.c
@@ -251,7 +251,11 @@ rl_set_sighandler (sig, handler, ohandle
   struct sigaction act;
 
   act.sa_handler = handler;
+#if defined (SIGWINCH)
   act.sa_flags = (sig == SIGWINCH) ? SA_RESTART : 0;
+#else
+  act.sa_flags = 0;
+#endif
   sigemptyset (&act.sa_mask);
   sigemptyset (&ohandler->sa_mask);
   sigaction (sig, &act, &old_handler);
Index: src/readline/support/wcwidth.c
===================================================================
--- src.orig/readline/support/wcwidth.c
+++ src/readline/support/wcwidth.c
@@ -6,6 +6,11 @@
  * Markus Kuhn -- 2001-09-08 -- public domain
  */
 
+#ifdef __GO32__
+/* DJGPP needs to include this before including wchar.h.  */
+# include <wctype.h>
+#endif
+
 #include <wchar.h>
 
 struct interval {
Index: src/bfd/archive.c
===================================================================
--- src.orig/bfd/archive.c
+++ src/bfd/archive.c
@@ -2311,7 +2311,7 @@ _bfd_archive_bsd_update_armap_timestamp 
       /* Can't read mod time for some reason.  */
       return TRUE;
     }
-  if (archstat.st_mtime <= bfd_ardata (arch)->armap_timestamp)
+  if (((long) archstat.st_mtime) <= bfd_ardata (arch)->armap_timestamp)
     /* OK by the linker's rules.  */
     return TRUE;
 
Index: src/gdb/gdb_select.h
===================================================================
--- src.orig/gdb/gdb_select.h
+++ src/gdb/gdb_select.h
@@ -24,6 +24,10 @@
 #include <sys/select.h>
 #endif
 
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+
 #ifdef USE_WIN32API
 #include <winsock2.h>
 #endif
Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c
+++ src/gdb/go32-nat.c
@@ -176,7 +176,7 @@ static void go32_fetch_registers (struct
 static void store_register (const struct regcache *, int regno);
 static void go32_store_registers (struct regcache *, int regno);
 static void go32_prepare_to_store (struct regcache *);
-static int go32_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len,
+static int go32_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len,
 			     int write,
 			     struct mem_attrib *attrib,
 			     struct target_ops *target);
@@ -465,10 +465,11 @@ go32_wait (ptid_t ptid, struct target_wa
 static void
 fetch_register (struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  if (regno < gdbarch_fp0_regnum (gdbarch))
     regcache_raw_supply (regcache, regno,
 			 (char *) &a_tss + regno_mapping[regno].tss_ofs);
-  else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
+  else if (i386_fp_regnum_p (gdbarch, regno) || i386_fpc_regnum_p (gdbarch, regno))
     i387_supply_fsave (regcache, regno, &npx);
   else
     internal_error (__FILE__, __LINE__,
@@ -493,10 +494,11 @@ go32_fetch_registers (struct regcache *r
 static void
 store_register (const struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  if (regno < gdbarch_fp0_regnum (gdbarch))
     regcache_raw_collect (regcache, regno,
 			  (char *) &a_tss + regno_mapping[regno].tss_ofs);
-  else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
+  else if (i386_fp_regnum_p (gdbarch, regno) || i386_fpc_regnum_p (gdbarch, regno))
     i387_collect_fsave (regcache, regno, &npx);
   else
     internal_error (__FILE__, __LINE__,
@@ -524,7 +526,7 @@ go32_prepare_to_store (struct regcache *
 }
 
 static int
-go32_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
+go32_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len, int write,
 		  struct mem_attrib *attrib, struct target_ops *target)
 {
   if (write)
@@ -1263,7 +1265,7 @@ go32_sysinfo (char *arg, int from_tty)
 struct seg_descr {
   unsigned short limit0          __attribute__((packed));
   unsigned short base0           __attribute__((packed));
-  unsigned char  base1           __attribute__((packed));
+  unsigned char  base1;
   unsigned       stype:5         __attribute__((packed));
   unsigned       dpl:2           __attribute__((packed));
   unsigned       present:1       __attribute__((packed));
@@ -1272,7 +1274,7 @@ struct seg_descr {
   unsigned       dummy:1         __attribute__((packed));
   unsigned       bit32:1         __attribute__((packed));
   unsigned       page_granular:1 __attribute__((packed));
-  unsigned char  base2           __attribute__((packed));
+  unsigned char  base2;
 };
 
 struct gate_descr {

[-- Attachment #3: go32-always_a_thread.diff --]
[-- Type: text/x-diff, Size: 2062 bytes --]

2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c: Include "gdbthread.h".
	(go32_stop, go32_kill_inferior): Delete the main thread.
	(go32_create_inferior): Add it.
	(go32_thread_alive, go32_pid_to_str): New.
	(init_go32_ops): Register go32_thread_alive and go32_pid_to_str.

---
 gdb/go32-nat.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c
+++ src/gdb/go32-nat.c
@@ -22,6 +22,7 @@
 
 #include "defs.h"
 #include "inferior.h"
+#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "gdbcore.h"
 #include "command.h"
@@ -566,7 +567,9 @@ go32_stop (ptid_t ptid)
 {
   normal_stop ();
   cleanup_client ();
+  ptid = inferior_ptid;
   inferior_ptid = null_ptid;
+  delete_thread_silent (ptid);
   prog_has_started = 0;
 }
 
@@ -576,6 +579,8 @@ go32_kill_inferior (void)
   redir_cmdline_delete (&child_cmd);
   resume_signal = -1;
   resume_is_step = 0;
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    delete_thread_silent (inferior_ptid);
   unpush_target (&go32_ops);
 }
 
@@ -658,6 +663,9 @@ go32_create_inferior (char *exec_file, c
 
   inferior_ptid = pid_to_ptid (SOME_PID);
   push_target (&go32_ops);
+
+  add_thread_silent (inferior_ptid);
+
   clear_proceed_status ();
   insert_breakpoints ();
   prog_has_started = 1;
@@ -848,6 +856,20 @@ go32_terminal_ours (void)
   }
 }
 
+static int
+go32_thread_alive (ptid_t ptid)
+{
+  return 1;
+}
+
+static char *
+go32_pid_to_str (ptid_t ptid)
+{
+  static char buf[64];
+  xsnprintf (buf, sizeof buf, "Thread <main>");
+  return buf;
+}
+
 static void
 init_go32_ops (void)
 {
@@ -878,6 +900,8 @@ init_go32_ops (void)
   go32_ops.to_mourn_inferior = go32_mourn_inferior;
   go32_ops.to_can_run = go32_can_run;
   go32_ops.to_stop = go32_stop;
+  go32_ops.to_thread_alive = go32_thread_alive;
+  go32_ops.to_pid_to_str = go32_pid_to_str;
   go32_ops.to_stratum = process_stratum;
   go32_ops.to_has_all_memory = 1;
   go32_ops.to_has_memory = 1;

Re: go32-nat, always a thread

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro>   The build complains about not finding stdint.h.  DJGPP 2.03 doesn't
Pedro>   come bundled with stdint.h, but we should be picking up
Pedro>   stdint.h from gdb/gnulib.  There seems to be a build order
Pedro>   dependency going wrong.  We configure gdb/gnulib before starting
Pedro>   to build gdb/, but we build gdb/ before gdb/gnulib.

Please try this patch.

Tom

ChangeLog:
2008-08-09  Tom Tromey  <tromey@redhat.com>

	* Makefile.in (generated_files): Add GNULIB_H.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.1045
diff -u -r1.1045 Makefile.in
--- Makefile.in	8 Aug 2008 17:38:35 -0000	1.1045
+++ Makefile.in	9 Aug 2008 18:00:45 -0000
@@ -831,7 +831,7 @@
 
 
 # All generated files which can be included by another file.
-generated_files = config.h observer.h observer.inc ada-lex.c
+generated_files = config.h observer.h observer.inc ada-lex.c $(GNULIB_H)
 
 .c.o:
 	$(COMPILE) $<

Re: go32-nat, always a thread

[Pedro Alves]

On Saturday 09 August 2008 19:03:09, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
>
> Pedro>   The build complains about not finding stdint.h.  DJGPP 2.03
> doesn't Pedro>   come bundled with stdint.h, but we should be picking up
> Pedro>   stdint.h from gdb/gnulib.  There seems to be a build order
> Pedro>   dependency going wrong.  We configure gdb/gnulib before starting
> Pedro>   to build gdb/, but we build gdb/ before gdb/gnulib.
>
> Please try this patch.
>
> Tom
>
> ChangeLog:
> 2008-08-09  Tom Tromey  <tromey@redhat.com>
>
>         * Makefile.in (generated_files): Add GNULIB_H.

It fixes the issue, thanks.

-- 
Pedro Alves

Re: go32-nat, always a thread

[Daniel Jacobowitz]

On Sat, Aug 09, 2008 at 12:03:09PM -0600, Tom Tromey wrote:
> ChangeLog:
> 2008-08-09  Tom Tromey  <tromey@redhat.com>
> 
> 	* Makefile.in (generated_files): Add GNULIB_H.

This is OK (obvious, even)

-- 
Daniel Jacobowitz
CodeSourcery

Re: go32-nat, always a thread

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sat, 9 Aug 2008 18:53:44 +0100
> 
>   GCC complains about attribute packed like so:
> 
>  cc1.exe: warnings being treated as errors
>  ../../gdb/go32-nat.c:1292: warning: 'packed' attribute ignored for field of type 'unsigned char'
>  ../../gdb/go32-nat.c:1301: warning: 'packed' attribute ignored for field of type 'unsigned char'

This one is the only one that bother me a little, since these
structures need to match the memory layouts expected by the system
calls to which we pass them.  Does the warning mean that GCC will
never add any padding between the previous field and the `unsigned
char' field?  If so, the change is safe; if not, we need to find some
way of enforcing the no-padding layouts.

> Are the GDB parts of patches OK?  I checked that
> x86_64-unknown-linux-gnu still builds OK.

Thanks for all your footwork, and sorry I couldn't do this fast enough
myself (my weekend was taken by a different project).

The go32-nat.c changes are fine with me.

Re: go32-nat, always a thread

[Mark Kettenis]

> Date: Sat, 09 Aug 2008 21:54:20 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Sat, 9 Aug 2008 18:53:44 +0100
> > 
> >   GCC complains about attribute packed like so:
> > 
> >  cc1.exe: warnings being treated as errors
> >  ../../gdb/go32-nat.c:1292: warning: 'packed' attribute ignored for field of type 'unsigned char'
> >  ../../gdb/go32-nat.c:1301: warning: 'packed' attribute ignored for field of type 'unsigned char'
> 
> This one is the only one that bother me a little, since these
> structures need to match the memory layouts expected by the system
> calls to which we pass them.  Does the warning mean that GCC will
> never add any padding between the previous field and the `unsigned
> char' field?  If so, the change is safe; if not, we need to find some
> way of enforcing the no-padding layouts.

Instead of

struct foo {
  char c __attribute__((packed));
  int d __attribute__((packed));
};

you can (should?) write

struct foo {
  char c;
  int d;
} __attribute__((packed));

I think that'll get rid of the warning.

Re: go32-nat, always a thread

[Daniel Jacobowitz]

On Sat, Aug 09, 2008 at 09:16:09PM +0200, Mark Kettenis wrote:
> Instead of
> 
> struct foo {
>   char c __attribute__((packed));
>   int d __attribute__((packed));
> };
> 
> you can (should?) write
> 
> struct foo {
>   char c;
>   int d;
> } __attribute__((packed));

Indeed, should (even must).  I believe old versions of GCC silently
ignored the packed attribute on fields, but I'm not 100% certain of
that - I must be mistaken if the construct in go32-nat.c previously
worked.

-- 
Daniel Jacobowitz
CodeSourcery

Re: go32-nat, always a thread

[Eli Zaretskii]

> Date: Sat, 9 Aug 2008 15:30:25 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sat, Aug 09, 2008 at 09:16:09PM +0200, Mark Kettenis wrote:
> > Instead of
> > 
> > struct foo {
> >   char c __attribute__((packed));
> >   int d __attribute__((packed));
> > };
> > 
> > you can (should?) write
> > 
> > struct foo {
> >   char c;
> >   int d;
> > } __attribute__((packed));
> 
> Indeed, should (even must).  I believe old versions of GCC silently
> ignored the packed attribute on fields, but I'm not 100% certain of
> that - I must be mistaken if the construct in go32-nat.c previously
> worked.

It's actually the other way around: some old versions of GCC (and I'm
talking _real_ old, like 2.8x) had a bug whereby the packed attribute
on the whole struct would not work, while doing that on each field
would.

If that problem is long gone now, I'm fine with marking the whole
struct with the attribute.

Thanks.

Re: go32-nat, always a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

On Saturday 09 August 2008 19:54:20, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Sat, 9 Aug 2008 18:53:44 +0100
> >
> >   GCC complains about attribute packed like so:
> >
> >  cc1.exe: warnings being treated as errors
> >  ../../gdb/go32-nat.c:1292: warning: 'packed' attribute ignored for field
> > of type 'unsigned char' ../../gdb/go32-nat.c:1301: warning: 'packed'
> > attribute ignored for field of type 'unsigned char'
>
> This one is the only one that bother me a little, since these
> structures need to match the memory layouts expected by the system
> calls to which we pass them.  Does the warning mean that GCC will
> never add any padding between the previous field and the `unsigned
> char' field?  If so, the change is safe; if not, we need to find some
> way of enforcing the no-padding layouts.
>

On Saturday 09 August 2008 20:16:09, Mark Kettenis wrote:
> Instead of
>
> struct foo {
>   char c __attribute__((packed));
>   int d __attribute__((packed));
> };
>
> you can (should?) write
>
> struct foo {
>   char c;
>   int d;
> } __attribute__((packed));
>
> I think that'll get rid of the warning.

I considered changing it that way, but resisted because I 
assumed there was some obscure reason this wasn't done this way,
that I was just failing to see.  I've always used the latter form.

Indeed, from GDB's manual:

"Specifying this attribute for struct and union types is equivalent
to specifying the packed attribute on each of the structure or
union members."

> The go32-nat.c changes are fine with me.

Thanks, I've checked in the go32-nat.c changes, as attached.

I'll post the rest of the changes in another thread.

-- 
Pedro Alves

[-- Attachment #2: go32_fixes_only.diff --]
[-- Type: text/x-diff, Size: 4828 bytes --]

2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c (fetch_register, store_register): Pass the regcache
	gdbarch to i386_fp_regnum_p and i386_fpc_regnum_p.
	(go32_xfer_memory): Change type of myaddr parameter to gdb_byte.
	(struct seg_descr, struct seg_descr): pack the whole struct
	instead of each member individually.

---
 gdb/go32-nat.c |   58 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c	2008-08-09 20:55:08.000000000 +0100
+++ src/gdb/go32-nat.c	2008-08-09 20:55:13.000000000 +0100
@@ -176,7 +176,7 @@ static void go32_fetch_registers (struct
 static void store_register (const struct regcache *, int regno);
 static void go32_store_registers (struct regcache *, int regno);
 static void go32_prepare_to_store (struct regcache *);
-static int go32_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len,
+static int go32_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len,
 			     int write,
 			     struct mem_attrib *attrib,
 			     struct target_ops *target);
@@ -465,10 +465,11 @@ go32_wait (ptid_t ptid, struct target_wa
 static void
 fetch_register (struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  if (regno < gdbarch_fp0_regnum (gdbarch))
     regcache_raw_supply (regcache, regno,
 			 (char *) &a_tss + regno_mapping[regno].tss_ofs);
-  else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
+  else if (i386_fp_regnum_p (gdbarch, regno) || i386_fpc_regnum_p (gdbarch, regno))
     i387_supply_fsave (regcache, regno, &npx);
   else
     internal_error (__FILE__, __LINE__,
@@ -493,10 +494,11 @@ go32_fetch_registers (struct regcache *r
 static void
 store_register (const struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  if (regno < gdbarch_fp0_regnum (gdbarch))
     regcache_raw_collect (regcache, regno,
 			  (char *) &a_tss + regno_mapping[regno].tss_ofs);
-  else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
+  else if (i386_fp_regnum_p (gdbarch, regno) || i386_fpc_regnum_p (gdbarch, regno))
     i387_collect_fsave (regcache, regno, &npx);
   else
     internal_error (__FILE__, __LINE__,
@@ -524,7 +526,7 @@ go32_prepare_to_store (struct regcache *
 }
 
 static int
-go32_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
+go32_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len, int write,
 		  struct mem_attrib *attrib, struct target_ops *target)
 {
   if (write)
@@ -1261,30 +1263,30 @@ go32_sysinfo (char *arg, int from_tty)
 }
 
 struct seg_descr {
-  unsigned short limit0          __attribute__((packed));
-  unsigned short base0           __attribute__((packed));
-  unsigned char  base1           __attribute__((packed));
-  unsigned       stype:5         __attribute__((packed));
-  unsigned       dpl:2           __attribute__((packed));
-  unsigned       present:1       __attribute__((packed));
-  unsigned       limit1:4        __attribute__((packed));
-  unsigned       available:1     __attribute__((packed));
-  unsigned       dummy:1         __attribute__((packed));
-  unsigned       bit32:1         __attribute__((packed));
-  unsigned       page_granular:1 __attribute__((packed));
-  unsigned char  base2           __attribute__((packed));
-};
+  unsigned short limit0;
+  unsigned short base0;
+  unsigned char  base1;
+  unsigned       stype:5;
+  unsigned       dpl:2;
+  unsigned       present:1;
+  unsigned       limit1:4;
+  unsigned       available:1;
+  unsigned       dummy:1;
+  unsigned       bit32:1;
+  unsigned       page_granular:1;
+  unsigned char  base2;
+} __attribute__ ((packed));
 
 struct gate_descr {
-  unsigned short offset0         __attribute__((packed));
-  unsigned short selector        __attribute__((packed));
-  unsigned       param_count:5   __attribute__((packed));
-  unsigned       dummy:3         __attribute__((packed));
-  unsigned       stype:5         __attribute__((packed));
-  unsigned       dpl:2           __attribute__((packed));
-  unsigned       present:1       __attribute__((packed));
-  unsigned short offset1         __attribute__((packed));
-};
+  unsigned short offset0;
+  unsigned short selector;
+  unsigned       param_count:5;
+  unsigned       dummy:3;
+  unsigned       stype:5;
+  unsigned       dpl:2;
+  unsigned       present:1;
+  unsigned short offset1;
+} __attribute__ ((packed));
 
 /* Read LEN bytes starting at logical address ADDR, and put the result
    into DEST.  Return 1 if success, zero if not.  */

[-- Attachment #3: go32-always_a_thread.diff --]
[-- Type: text/x-diff, Size: 2062 bytes --]

2008-08-09  Pedro Alves  <pedro@codesourcery.com>

	* go32-nat.c: Include "gdbthread.h".
	(go32_stop, go32_kill_inferior): Delete the main thread.
	(go32_create_inferior): Add it.
	(go32_thread_alive, go32_pid_to_str): New.
	(init_go32_ops): Register go32_thread_alive and go32_pid_to_str.

---
 gdb/go32-nat.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Index: src/gdb/go32-nat.c
===================================================================
--- src.orig/gdb/go32-nat.c
+++ src/gdb/go32-nat.c
@@ -22,6 +22,7 @@
 
 #include "defs.h"
 #include "inferior.h"
+#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "gdbcore.h"
 #include "command.h"
@@ -566,7 +567,9 @@ go32_stop (ptid_t ptid)
 {
   normal_stop ();
   cleanup_client ();
+  ptid = inferior_ptid;
   inferior_ptid = null_ptid;
+  delete_thread_silent (ptid);
   prog_has_started = 0;
 }
 
@@ -576,6 +579,8 @@ go32_kill_inferior (void)
   redir_cmdline_delete (&child_cmd);
   resume_signal = -1;
   resume_is_step = 0;
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    delete_thread_silent (inferior_ptid);
   unpush_target (&go32_ops);
 }
 
@@ -658,6 +663,9 @@ go32_create_inferior (char *exec_file, c
 
   inferior_ptid = pid_to_ptid (SOME_PID);
   push_target (&go32_ops);
+
+  add_thread_silent (inferior_ptid);
+
   clear_proceed_status ();
   insert_breakpoints ();
   prog_has_started = 1;
@@ -848,6 +856,20 @@ go32_terminal_ours (void)
   }
 }
 
+static int
+go32_thread_alive (ptid_t ptid)
+{
+  return 1;
+}
+
+static char *
+go32_pid_to_str (ptid_t ptid)
+{
+  static char buf[64];
+  xsnprintf (buf, sizeof buf, "Thread <main>");
+  return buf;
+}
+
 static void
 init_go32_ops (void)
 {
@@ -878,6 +900,8 @@ init_go32_ops (void)
   go32_ops.to_mourn_inferior = go32_mourn_inferior;
   go32_ops.to_can_run = go32_can_run;
   go32_ops.to_stop = go32_stop;
+  go32_ops.to_thread_alive = go32_thread_alive;
+  go32_ops.to_pid_to_str = go32_pid_to_str;
   go32_ops.to_stratum = process_stratum;
   go32_ops.to_has_all_memory = 1;
   go32_ops.to_has_memory = 1;