make check in itcl

make check in itcl

[Andreas Jaeger]

Running make check with the whole sourceware tree checked out results
in:

make[1]: Leaving directory `/usr/src/build-gdb/intl'
make[1]: Entering directory `/usr/src/build-gdb/itcl'
make[1]: *** No rule to make target `check'.

We should ignore check in itcl.  Please apply the appended patch.

Andreas

2000-04-29  Andreas Jaeger  <aj@suse.de>

	* Makefile.in (check): Add rule to ignore make check.

Index: itcl/Makefile.in
===================================================================
RCS file: /cvs/src/src/itcl/Makefile.in,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 Makefile.in
--- Makefile.in	2000/02/07 00:19:46	1.1.1.1
+++ Makefile.in	2000/04/29 14:20:10
@@ -70,7 +70,7 @@
 		then true; else exit 1; fi \
 	done;
 
-install-info info install-check:
+install-info info install-check check:
 
 clean:
 	for dir in $(subdirs) ; do \

-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.rhein-neckar.de

Re: make check in itcl

[Syd Polk]

Actually, "make check" is supposed to run tests. "make test" should be a valid
target for itcl. Shouldn't this map "make check" to "make test"?

Andreas Jaeger wrote:
> 
> Running make check with the whole sourceware tree checked out results
> in:
> 
> make[1]: Leaving directory `/usr/src/build-gdb/intl'
> make[1]: Entering directory `/usr/src/build-gdb/itcl'
> make[1]: *** No rule to make target `check'.
> 
> We should ignore check in itcl.  Please apply the appended patch.
> 
> Andreas
> 
> 2000-04-29  Andreas Jaeger  <aj@suse.de>
> 
>         * Makefile.in (check): Add rule to ignore make check.
> 
> Index: itcl/Makefile.in
> ===================================================================
> RCS file: /cvs/src/src/itcl/Makefile.in,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 Makefile.in
> --- Makefile.in 2000/02/07 00:19:46     1.1.1.1
> +++ Makefile.in 2000/04/29 14:20:10
> @@ -70,7 +70,7 @@
>                 then true; else exit 1; fi \
>         done;
> 
> -install-info info install-check:
> +install-info info install-check check:
> 
>  clean:
>         for dir in $(subdirs) ; do \
> 
> --
>  Andreas Jaeger
>   SuSE Labs aj@suse.de
>    private aj@arthur.rhein-neckar.de

Re: make check in itcl

[Andreas Jaeger]

>>>>> Syd Polk writes:

Syd> Actually, "make check" is supposed to run tests. "make test" should be a valid
Syd> target for itcl. Shouldn't this map "make check" to "make test"?

Yes, it should.  I didn't notice that make test was valid :-(.  Shall
I send a new patch to add "check" to the test target?

Andreas

Syd> Andreas Jaeger wrote:
>> 
>> Running make check with the whole sourceware tree checked out results
>> in:
>> 
>> make[1]: Leaving directory `/usr/src/build-gdb/intl'
>> make[1]: Entering directory `/usr/src/build-gdb/itcl'
>> make[1]: *** No rule to make target `check'.
>> 
>> We should ignore check in itcl.  Please apply the appended patch.
>> 
>> Andreas
>> 
>> 2000-04-29  Andreas Jaeger  <aj@suse.de>
>> 
>> * Makefile.in (check): Add rule to ignore make check.
>> 
>> Index: itcl/Makefile.in
>> ===================================================================
>> RCS file: /cvs/src/src/itcl/Makefile.in,v
>> retrieving revision 1.1.1.1
>> diff -u -r1.1.1.1 Makefile.in
>> --- Makefile.in 2000/02/07 00:19:46     1.1.1.1
>> +++ Makefile.in 2000/04/29 14:20:10
>> @@ -70,7 +70,7 @@
>> then true; else exit 1; fi \
>> done;
>> 
>> -install-info info install-check:
>> +install-info info install-check check:
>> 
>> clean:
>> for dir in $(subdirs) ; do \
>> 
>> --
>> Andreas Jaeger
>> SuSE Labs aj@suse.de
>> private aj@arthur.rhein-neckar.de


-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.rhein-neckar.de

Re: make check in itcl

[Syd Polk]

That would be better.

At 06:12 PM 5/1/00 +0200, Andreas Jaeger wrote:
> >>>>> Syd Polk writes:
>
>Syd> Actually, "make check" is supposed to run tests. "make test" should 
>be a valid
>Syd> target for itcl. Shouldn't this map "make check" to "make test"?
>
>Yes, it should.  I didn't notice that make test was valid :-(.  Shall
>I send a new patch to add "check" to the test target?
>
>Andreas
>
>Syd> Andreas Jaeger wrote:
> >>
> >> Running make check with the whole sourceware tree checked out results
> >> in:
> >>
> >> make[1]: Leaving directory `/usr/src/build-gdb/intl'
> >> make[1]: Entering directory `/usr/src/build-gdb/itcl'
> >> make[1]: *** No rule to make target `check'.
> >>
> >> We should ignore check in itcl.  Please apply the appended patch.
> >>
> >> Andreas
> >>
> >> 2000-04-29  Andreas Jaeger  <aj@suse.de>
> >>
> >> * Makefile.in (check): Add rule to ignore make check.
> >>
> >> Index: itcl/Makefile.in
> >> ===================================================================
> >> RCS file: /cvs/src/src/itcl/Makefile.in,v
> >> retrieving revision 1.1.1.1
> >> diff -u -r1.1.1.1 Makefile.in
> >> --- Makefile.in 2000/02/07 00:19:46     1.1.1.1
> >> +++ Makefile.in 2000/04/29 14:20:10
> >> @@ -70,7 +70,7 @@
> >> then true; else exit 1; fi \
> >> done;
> >>
> >> -install-info info install-check:
> >> +install-info info install-check check:
> >>
> >> clean:
> >> for dir in $(subdirs) ; do \
> >>
> >> --
> >> Andreas Jaeger
> >> SuSE Labs aj@suse.de
> >> private aj@arthur.rhein-neckar.de
>
>
>--
>  Andreas Jaeger
>   SuSE Labs aj@suse.de
>    private aj@arthur.rhein-neckar.de

Syd Polk
Engineering Manager
Cygnus Solutions,a Red Hat company

Re: make check in itcl

[Andreas Jaeger]

Hi Syd,

as discussed here's a patch to handle make check like make test.

Andreas

2000-04-29  Andreas Jaeger  <aj@suse.de>

	* Makefile.in (check): Handle make check like make test.

Index: itcl/Makefile.in
===================================================================
RCS file: /cvs/src/src/itcl/Makefile.in,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 Makefile.in
--- Makefile.in	2000/02/07 00:19:46	1.1.1.1
+++ Makefile.in	2000/05/01 17:48:19
@@ -28,7 +28,7 @@
 		then true; else exit 1; fi \
 	done;
 
-test:
+test check:
 	for dir in $(subdirs) ; do \
 		if (echo "Making in $$dir"; cd $$dir && $(MAKE) $@); \
 		then true; else exit 1; fi \

-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.rhein-neckar.de

Re: make check in itcl

[Syd Polk]

This looks good. Thank you.

At 07:48 PM 5/1/00 +0200, Andreas Jaeger wrote:

>Hi Syd,
>
>as discussed here's a patch to handle make check like make test.
>
>Andreas
>
>2000-04-29  Andreas Jaeger  <aj@suse.de>
>
>         * Makefile.in (check): Handle make check like make test.
>
>Index: itcl/Makefile.in
>===================================================================
>RCS file: /cvs/src/src/itcl/Makefile.in,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 Makefile.in
>--- Makefile.in 2000/02/07 00:19:46     1.1.1.1
>+++ Makefile.in 2000/05/01 17:48:19
>@@ -28,7 +28,7 @@
>                 then true; else exit 1; fi \
>         done;
>
>-test:
>+test check:
>         for dir in $(subdirs) ; do \
>                 if (echo "Making in $$dir"; cd $$dir && $(MAKE) $@); \
>                 then true; else exit 1; fi \
>
>--
>  Andreas Jaeger
>   SuSE Labs aj@suse.de
>    private aj@arthur.rhein-neckar.de

Syd Polk
Engineering Manager
Cygnus Solutions,a Red Hat company

Re: make check in itcl

[Syd Polk]

I checked it into sourceware; however, on second look, I don't think it 
will work. When you run "make check" from the toplevel, you need to run 
"make test" in the itcl subdirectories.

+test check:
>         for dir in $(subdirs) ; do \
>                 if (echo "Making in $$dir"; cd $$dir && $(MAKE) $@); \
>                 then true; else exit 1; fi \

This just runs "make check" in the sub-directories, which will fail, right?

Should be

                 if (echo "Making in $$dir"; cd $$dir && $(MAKE) test); \




Syd Polk
Engineering Manager
Cygnus Solutions,a Red Hat company

Re: make check in itcl

[Andreas Jaeger]

>>>>> Syd Polk writes:

Syd> I checked it into sourceware; however, on second look, I don't think
Syd> it will work. When you run "make check" from the toplevel, you need to
Syd> run "make test" in the itcl subdirectories.

I've tested my first patch - but forgot to test that version :-(
Sorry about that.


Syd> +test check:
>> for dir in $(subdirs) ; do \
>> if (echo "Making in $$dir"; cd $$dir && $(MAKE) $@); \
>> then true; else exit 1; fi \

Syd> This just runs "make check" in the sub-directories, which will fail, right?
I checked the subdirectories - and it really should fail (and indeed does)

Syd> Should be

Syd>                  if (echo "Making in $$dir"; cd $$dir && $(MAKE) test); \

Yes, that works.  I've just run make;make check and it's fine with
this change.  Please commit this fix.

Thanks,
Andreas
-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.rhein-neckar.de

Re: make check in itcl

[Syd Polk]

>
>Syd> This just runs "make check" in the sub-directories, which will fail, 
>right?
>I checked the subdirectories - and it really should fail (and indeed does)
>
>Syd> Should be
>
>Syd>                  if (echo "Making in $$dir"; cd $$dir && $(MAKE) test); \
>
>Yes, that works.  I've just run make;make check and it's fine with
>this change.  Please commit this fix.

Done.

Syd Polk
Engineering Manager
Cygnus Solutions,a Red Hat company

Re: make check in itcl

[Andrew Cagney]

Syd Polk wrote:
> 
> >
> >Syd> This just runs "make check" in the sub-directories, which will fail,
> >right?
> >I checked the subdirectories - and it really should fail (and indeed does)
> >
> >Syd> Should be
> >
> >Syd>                  if (echo "Making in $$dir"; cd $$dir && $(MAKE) test); \
> >
> >Yes, that works.  I've just run make;make check and it's fine with
> >this change.  Please commit this fix.

FYI,

I've added this to the Branch.

	Andrew

rename to insight

[Tom Tromey]

Here's a different patch to have an `insight' executable.
This patch takes the shell script approach.

I like this approach because it doesn't cost much.  A distributor
(Debian, Red Hat, whoever) can build a single gdb.  By default this
gdb will just be gdb, but if the user invokes `insight' they get the
GUI.

I dislike the other approach (relinking for insight) because it makes
it harder for packagers.

For instance, how is documentation to be handled?  Only one package
can have a given file, so the packagers must either rename the insight
documentation (but why have two identical copies of the gdb docs?) or
leave the docs out of one package (eww), or make a new package holding
only the docs (also eww).

2000-11-21  Tom Tromey  <tromey@cygnus.com>

	* insight: New file.
	* Makefile.in (install-gdbtk): Install `insight'.
	* top.c (use_windows): Default to 0.

Tom

Index: insight
===================================================================
RCS file: insight
diff -N insight
--- /dev/null	Tue May  5 13:32:27 1998
+++ insight	Tue Nov 21 15:58:01 2000
@@ -0,0 +1,23 @@
+#! /bin/sh
+
+# GUI wrapper for gdb.
+# Copyright 1986-2000 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330,
+# Boston, MA 02111-1307, USA.
+
+exec gdb -w ${1+"$@"}
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.47
diff -u -r1.47 Makefile.in
--- Makefile.in	2000/11/10 23:02:56	1.47
+++ Makefile.in	2000/11/21 23:58:04
@@ -1334,11 +1340,20 @@
 	$(SHELL) $(srcdir)/../mkinstalldirs $(datadir)/gdbtcl/help \
 		$(datadir)/gdbtcl/help/images \
 		$(datadir)/gdbtcl/help/trace ; \
+	$(SHELL) $(srcdir)/../mkinstalldirs $(bindir)
 	cd $(srcdir)/gdbtk/library ; \
 	for i in *.tcl *.ith *.itb images/*.gif images2/*.gif images/icons.txt images2/icons.txt tclIndex help/*.html  help/trace/*.html help/trace/index.toc help/images/*.gif; \
 	  do \
 		$(INSTALL_DATA) $$i $(datadir)/gdbtcl/$$i ; \
-	  done ;
+	  done
+	transformed_name=`t='$(program_transform_name)'; \
+			  echo insight | sed -e $$t` ; \
+		if test "x$$transformed_name" = x; then \
+		  transformed_name=insight ; \
+		else \
+		  true ; \
+		fi ; \
+	$(INSTALL_PROGRAM) $(srcdir)/insight $(bindir)/$$transformed_name
 
 gdbres.o: $(srcdir)/gdbtk/gdb.rc $(srcdir)/gdbtk/gdbtool.ico
 	$(WINDRES) --include $(srcdir)/gdbtk $(srcdir)/gdbtk/gdb.rc gdbres.o
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.21
diff -u -r1.21 top.c
--- top.c	2000/11/16 14:51:50	1.21
+++ top.c	2000/11/21 23:58:07
@@ -173,7 +173,7 @@
 /* If nonzero, and GDB has been configured to be able to use windows,
    attempt to open them upon startup.  */
 
-int use_windows = 1;
+int use_windows = 0;
 
 extern char lang_frame_mismatch_warn[];		/* language.c */
 

Re: rename to insight

[Chris G. Demetriou]

tromey@cygnus.com (Tom Tromey) writes:
> +exec gdb -w ${1+"$@"}

I believe there's a slight problem with this:

This may get you a different 'gdb' than the one corresponding to the
'insight' being invoked.  $PATH is used to find gdb, but not
necessarily to find the 'insight' shell script in this case.

The good news is, i'm pretty sure that pretty much all UNIX-ish
systems invoke shell scripts with a path of the script as $0 (relative
if the script was invoked via a relative path, absolute if via an
absolute path or $PATH) so you could do something like:

	gdbpath="`dirname $0`"
	exec $gdbpath/gdb ...

(At least, all of the ones i could find right now do, and in general
that's been my experience... and i rely in it in scripts.  I dunno
what standards say, if anything.)

of course, 'dirname' isn't as portable as you might like...


chris

Re: rename to insight

[Tom Tromey]

>>>>> "Chris" == Chris G Demetriou <cgd@sibyte.com> writes:

>> +exec gdb -w ${1+"$@"}

Chris> This may get you a different 'gdb' than the one corresponding
Chris> to the 'insight' being invoked.

Yeah, and in fact I got to experience this yesterday.

Chris> The good news is, i'm pretty sure that pretty much all UNIX-ish
Chris> systems invoke shell scripts with a path of the script as $0

Yes.  See appended script.

Tom

#! /bin/sh

# GUI wrapper for gdb.
# Copyright 1986-2000 Free Software Foundation, Inc.

# This file is part of GDB.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.

# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.

# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330,
# Boston, MA 02111-1307, USA.

path="`echo $0 | sed -e 's,/[^/]*$,,'`"
if test -n "$path"; then
   gdb="$path/gdb"
else
   gdb=gdb
fi
exec $gdb -w ${1+"$@"}

Re: rename to insight

[Chris G. Demetriou]

Tom Tromey <tromey@cygnus.com> writes:
> Chris> This may get you a different 'gdb' than the one corresponding
> Chris> to the 'insight' being invoked.
> 
> Yeah, and in fact I got to experience this yesterday.

I just noticed another issue (will wonders never cease?!) with this
patch:

if you build a cross-gdb, e.g. --target=mips-elf, you end up with
mips-elf-gdb and mips-elf-insight, but the later invokes gdb.


(I'll come up with a better patch if i get the chance, but given the
nature of my todo list these days, i wouldn't hold my breath.  8-S)


cgd

Re: rename to insight

[Tom Tromey]

>>>>> "Chris" == Chris G Demetriou <cgd@sibyte.com> writes:

Chris> I just noticed another issue (will wonders never cease?!) with
Chris> this patch:

Sigh.  Try the appended.

BTW who would approve or reject this patch?

2000-11-28  Tom Tromey  <tromey@cygnus.com>

	* insight.in: New file.
	* Makefile.in (install-gdbtk): Install `insight'.
	(all-gdbtk): Depend on `insight'.
	(insight): New target.

Tom

Index: insight.in
===================================================================
RCS file: insight.in
diff -N insight.in
--- /dev/null	Tue May  5 13:32:27 1998
+++ insight.in	Tue Nov 28 09:32:54 2000
@@ -0,0 +1,35 @@
+#! /bin/sh
+
+# GUI wrapper for gdb.
+# Copyright 1986-2000 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330,
+# Boston, MA 02111-1307, USA.
+
+# Name of gdb, properly transformed.
+gdb="@gdb@"
+# Default location of gdb.
+bindir="@bindir@"
+
+# Try to pick up gdb from the same directory as insight.
+path="`echo $0 | sed -e 's,/[^/]*$,,'`"
+if test -n "$path"; then
+   exe="$path/$gdb"
+else
+   exe="$bindir/$gdb"
+fi
+exec $exe -w ${1+"$@"}
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.47
diff -u -r1.47 Makefile.in
--- Makefile.in	2000/11/10 23:02:56	1.47
+++ Makefile.in	2000/11/28 17:32:57
@@ -1308,7 +1314,7 @@
 # FIXME: cagney/2000-03-27: Post 5.0 this link code can be removed.
 # It should now be possible to run GDBtk from the build directory
 # without the link in place.
-all-gdbtk:
+all-gdbtk: insight
 	@if test ! -d gdbtcl/images ; then \
 	  if test "$(LN_S)" = "ln -s" ; then \
 	    echo linking ${srcdir}/gdbtk/library to gdbtcl ; \
@@ -1334,11 +1340,32 @@
 	$(SHELL) $(srcdir)/../mkinstalldirs $(datadir)/gdbtcl/help \
 		$(datadir)/gdbtcl/help/images \
 		$(datadir)/gdbtcl/help/trace ; \
+	$(SHELL) $(srcdir)/../mkinstalldirs $(bindir)
 	cd $(srcdir)/gdbtk/library ; \
 	for i in *.tcl *.ith *.itb images/*.gif images2/*.gif images/icons.txt images2/icons.txt tclIndex help/*.html  help/trace/*.html help/trace/index.toc help/images/*.gif; \
 	  do \
 		$(INSTALL_DATA) $$i $(datadir)/gdbtcl/$$i ; \
-	  done ;
+	  done
+	transformed_name=`t='$(program_transform_name)'; \
+			  echo insight | sed -e $$t` ; \
+		if test "x$$transformed_name" = x; then \
+		  transformed_name=insight ; \
+		else \
+		  true ; \
+		fi ; \
+	$(INSTALL_PROGRAM) $(srcdir)/insight $(bindir)/$$transformed_name
+
+insight: $(srcdir)/insight.in
+	transformed_name=`t='$(program_transform_name)'; \
+			  echo gdb | sed -e $$t` ; \
+	if test "x$$transformed_name" = x; then \
+	  transformed_name=gdb ; \
+	else \
+	  true ; \
+	fi ; \
+	sed -e "s,\@gdb\@,$$transformed_name," -e 's,\@bindir\@,$(bindir),' \
+	  < $(srcdir)/insight.in > insight \
+	  && chmod +x insight
 
 gdbres.o: $(srcdir)/gdbtk/gdb.rc $(srcdir)/gdbtk/gdbtool.ico
 	$(WINDRES) --include $(srcdir)/gdbtk $(srcdir)/gdbtk/gdb.rc gdbres.o
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.21
diff -u -r1.21 top.c
--- top.c	2000/11/16 14:51:50	1.21
+++ top.c	2000/11/28 17:33:00
@@ -173,7 +173,7 @@
 /* If nonzero, and GDB has been configured to be able to use windows,
    attempt to open them upon startup.  */
 
-int use_windows = 1;
+int use_windows = 0;
 
 extern char lang_frame_mismatch_warn[];		/* language.c */
 

Patch: fixlet in gdbtk-cmds.c

[Tom Tromey]

I found a buglet in gdbtk-cmds.c.  This patch fixes it.  Ok to commit?

2000-11-28  Tom Tromey  <tromey@cygnus.com>

	* gdbtk-cmds.c (gdb_clear_file): Return error if wrong number of
	arguments given.

Tom

Index: generic/gdbtk-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-cmds.c,v
retrieving revision 1.12
diff -u -r1.12 gdbtk-cmds.c
--- gdbtk-cmds.c	2000/07/25 20:41:07	1.12
+++ gdbtk-cmds.c	2000/11/28 18:33:19
@@ -580,8 +606,11 @@
      Tcl_Obj *CONST objv[];
 {
   if (objc != 1)
-    Tcl_SetStringObj (result_ptr->obj_ptr,
-		      "Wrong number of args, none are allowed.", -1);
+    {
+      Tcl_SetStringObj (result_ptr->obj_ptr,
+			"Wrong number of args, none are allowed.", -1);
+      return TCL_ERROR;
+    }
 
   if (inferior_pid != 0 && target_has_execution)
     {

Re: rename to insight

[Syd Polk]

The head maintainer is me, but I am but a placeholder. Fernando Nasser and 
Jim Ingham are also maintainers.

At 10:43 AM 11/28/00 -0700, Tom Tromey wrote:
> >>>>> "Chris" == Chris G Demetriou <cgd@sibyte.com> writes:
>
>Chris> I just noticed another issue (will wonders never cease?!) with
>Chris> this patch:
>
>Sigh.  Try the appended.
>
>BTW who would approve or reject this patch?
>
>2000-11-28  Tom Tromey  <tromey@cygnus.com>
>
>         * insight.in: New file.
>         * Makefile.in (install-gdbtk): Install `insight'.
>         (all-gdbtk): Depend on `insight'.
>         (insight): New target.
>
>Tom
>
>Index: insight.in
>===================================================================
>RCS file: insight.in
>diff -N insight.in
>--- /dev/null   Tue May  5 13:32:27 1998
>+++ insight.in  Tue Nov 28 09:32:54 2000
>@@ -0,0 +1,35 @@
>+#! /bin/sh
>+
>+# GUI wrapper for gdb.
>+# Copyright 1986-2000 Free Software Foundation, Inc.
>+
>+# This file is part of GDB.
>+
>+# This program is free software; you can redistribute it and/or modify
>+# it under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 2 of the License, or
>+# (at your option) any later version.
>+
>+# This program is distributed in the hope that it will be useful,
>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+# GNU General Public License for more details.
>+
>+# You should have received a copy of the GNU General Public License
>+# along with this program; if not, write to the Free Software
>+# Foundation, Inc., 59 Temple Place - Suite 330,
>+# Boston, MA 02111-1307, USA.
>+
>+# Name of gdb, properly transformed.
>+gdb="@gdb@"
>+# Default location of gdb.
>+bindir="@bindir@"
>+
>+# Try to pick up gdb from the same directory as insight.
>+path="`echo $0 | sed -e 's,/[^/]*$,,'`"
>+if test -n "$path"; then
>+   exe="$path/$gdb"
>+else
>+   exe="$bindir/$gdb"
>+fi
>+exec $exe -w ${1+"$@"}
>Index: Makefile.in
>===================================================================
>RCS file: /cvs/src/src/gdb/Makefile.in,v
>retrieving revision 1.47
>diff -u -r1.47 Makefile.in
>--- Makefile.in 2000/11/10 23:02:56     1.47
>+++ Makefile.in 2000/11/28 17:32:57
>@@ -1308,7 +1314,7 @@
>  # FIXME: cagney/2000-03-27: Post 5.0 this link code can be removed.
>  # It should now be possible to run GDBtk from the build directory
>  # without the link in place.
>-all-gdbtk:
>+all-gdbtk: insight
>         @if test ! -d gdbtcl/images ; then \
>           if test "$(LN_S)" = "ln -s" ; then \
>             echo linking ${srcdir}/gdbtk/library to gdbtcl ; \
>@@ -1334,11 +1340,32 @@
>         $(SHELL) $(srcdir)/../mkinstalldirs $(datadir)/gdbtcl/help \
>                 $(datadir)/gdbtcl/help/images \
>                 $(datadir)/gdbtcl/help/trace ; \
>+       $(SHELL) $(srcdir)/../mkinstalldirs $(bindir)
>         cd $(srcdir)/gdbtk/library ; \
>         for i in *.tcl *.ith *.itb images/*.gif images2/*.gif 
> images/icons.txt images2/icons.txt tclIndex 
> help/*.html  help/trace/*.html help/trace/index.toc help/images/*.gif; \
>           do \
>                 $(INSTALL_DATA) $$i $(datadir)/gdbtcl/$$i ; \
>-         done ;
>+         done
>+       transformed_name=`t='$(program_transform_name)'; \
>+                         echo insight | sed -e $$t` ; \
>+               if test "x$$transformed_name" = x; then \
>+                 transformed_name=insight ; \
>+               else \
>+                 true ; \
>+               fi ; \
>+       $(INSTALL_PROGRAM) $(srcdir)/insight $(bindir)/$$transformed_name
>+
>+insight: $(srcdir)/insight.in
>+       transformed_name=`t='$(program_transform_name)'; \
>+                         echo gdb | sed -e $$t` ; \
>+       if test "x$$transformed_name" = x; then \
>+         transformed_name=gdb ; \
>+       else \
>+         true ; \
>+       fi ; \
>+       sed -e "s,\@gdb\@,$$transformed_name," -e 's,\@bindir\@,$(bindir),' \
>+         < $(srcdir)/insight.in > insight \
>+         && chmod +x insight
>
>  gdbres.o: $(srcdir)/gdbtk/gdb.rc $(srcdir)/gdbtk/gdbtool.ico
>         $(WINDRES) --include $(srcdir)/gdbtk $(srcdir)/gdbtk/gdb.rc gdbres.o
>Index: top.c
>===================================================================
>RCS file: /cvs/src/src/gdb/top.c,v
>retrieving revision 1.21
>diff -u -r1.21 top.c
>--- top.c       2000/11/16 14:51:50     1.21
>+++ top.c       2000/11/28 17:33:00
>@@ -173,7 +173,7 @@
>  /* If nonzero, and GDB has been configured to be able to use windows,
>     attempt to open them upon startup.  */
>
>-int use_windows = 1;
>+int use_windows = 0;
>
>  extern char lang_frame_mismatch_warn[];                /* language.c */
>

Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: Patch: fixlet in gdbtk-cmds.c

[Syd Polk]

Approved, although I would have preferred using Tcl_WrongNumArgs. That 
would give a slightly different error message, however, and that might 
break some tests.

At 11:43 AM 11/28/00 -0700, Tom Tromey wrote:
>I found a buglet in gdbtk-cmds.c.  This patch fixes it.  Ok to commit?
>
>2000-11-28  Tom Tromey  <tromey@cygnus.com>
>
>         * gdbtk-cmds.c (gdb_clear_file): Return error if wrong number of
>         arguments given.
>
>Tom
>
>Index: generic/gdbtk-cmds.c
>===================================================================
>RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-cmds.c,v
>retrieving revision 1.12
>diff -u -r1.12 gdbtk-cmds.c
>--- gdbtk-cmds.c        2000/07/25 20:41:07     1.12
>+++ gdbtk-cmds.c        2000/11/28 18:33:19
>@@ -580,8 +606,11 @@
>       Tcl_Obj *CONST objv[];
>  {
>    if (objc != 1)
>-    Tcl_SetStringObj (result_ptr->obj_ptr,
>-                     "Wrong number of args, none are allowed.", -1);
>+    {
>+      Tcl_SetStringObj (result_ptr->obj_ptr,
>+                       "Wrong number of args, none are allowed.", -1);
>+      return TCL_ERROR;
>+    }
>
>    if (inferior_pid != 0 && target_has_execution)
>      {

Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: Patch: fixlet in gdbtk-cmds.c

[Tom Tromey]

>>>>> "Syd" == Syd Polk <spolk@redhat.com> writes:

Syd> Approved, although I would have preferred using Tcl_WrongNumArgs.

I wasn't even aware of that function.  It must have been added since I
last used Tcl.

gdbtk-cmds.c uses the "broken" method everywhere.  Bummer.  Also,
gdb_get_breakpoint_list() uses error(), which seems wrong.


I looked in gdb/testsuite/gdb.gdbtk and I didn't see anything that
mentions `gdb_clear_file'.  So I could check in the patch you prefer,
if you want.

Is there somewhere else I should look for tests?

Tom

Re: Patch: fixlet in gdbtk-cmds.c

[Syd Polk]

At 02:56 PM 11/28/00 -0700, Tom Tromey wrote:
> >>>>> "Syd" == Syd Polk <spolk@redhat.com> writes:
>
>Syd> Approved, although I would have preferred using Tcl_WrongNumArgs.
>
>I wasn't even aware of that function.  It must have been added since I
>last used Tcl.
>
>gdbtk-cmds.c uses the "broken" method everywhere.  Bummer.  Also,
>gdb_get_breakpoint_list() uses error(), which seems wrong.
>
>
>I looked in gdb/testsuite/gdb.gdbtk and I didn't see anything that
>mentions `gdb_clear_file'.  So I could check in the patch you prefer,
>if you want.
>
>Is there somewhere else I should look for tests?
>
>Tom

No, that is where they would be.

Basically, there are templates for how to parse command arguments in the 
Tcl code itself. The best bet is probably something like Tcl_FileCmd in 
tclCmdAH.c. IRIC, it shows a good example of the canonical way to parse 
arguments and return errors.

I know it is in 8.1 and later; I don't remember if it is in 8.0.


Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: Patch: fixlet in gdbtk-cmds.c

[Tom Tromey]

Syd> Approved, although I would have preferred using
Syd> Tcl_WrongNumArgs. That would give a slightly different error
Syd> message, however, and that might break some tests.

I looked at this some more.

No gdbtk tests test any command for the wrong number of arguments.  So
we're free to change this however we want.

Some of the code in gdbtk-cmds.c is quite bad.  For instance there are
functions which do no checking, or incorrect checking, of the number
of arguments they are given.

I discovered that if I just use Tcl_WrongNumArgs and return TCL_ERROR,
I don't get the right result, because you also have to do this:

      result_ptr->flags |= GDBTK_IN_TCL_RESULT;


Note that I changed all the functions in gdbtk-cmds.c that did any
argument number checking to use Tcl_WrongNumArgs.  However this issue
has to be resolved first :-(.  I didn't bother fixing all the
commands, since my time on Insight is limited and I doubt this sort of
cleanup is very high priority.

If we really do have to set result_ptr->flags, that's fine --
but I'd like to add a macro like this:

#define RETURN_TCL_ERROR \
    result_ptr->flags |= GDBTK_IN_TCL_RESULT; \
    return TCL_ERROR

Then we can just use `RETURN_TCL_ERROR;' all over.  This is ugly, but
imho more maintainable than remembering to put an assignment
everywhere we return TCL_ERROR.

OTOH I don't understand this comment in call_wrapper:

  /*
   * Now copy the result over to the true Tcl result.  If GDBTK_TO_RESULT flag
   * bit is set , this just copies a null object over to the Tcl result, 
   * which is fine because we should reset the result in this case anyway.
   */

First, the comment seems to lie.  It mentions GDBTK_TO_RESULT, which
makes sense, but the code actually checks for GDBTK_IN_TCL_RESULT.

Ok, I just read the comments in gdbtk.h and it makes a bit more
sense.  But wouldn't it be easier to adopt the heuristic that if a
wrapped command returns TCL_ERROR then we should assume that the
interpreter's result is already correctly set?  Are there cases where
we explicitly (as opposed to via error()) return TCL_ERROR but rely on
the call wrapper to set the result?  That seems strange.

Tom

Re: Patch: fixlet in gdbtk-cmds.c

[Syd Polk]

This all seems quite wacky. I don't know enough about the code to
comment, but it smells really bad.

I would rather have a global variable for whether a gdbtk command
generated the error as opposed to Tcl itself rather than messing with
interp result flags. This is just asking to break when the interp data
structure is changed.

I will examine this more in a couple of weeks.

Tom Tromey wrote:
> 
> Syd> Approved, although I would have preferred using
> Syd> Tcl_WrongNumArgs. That would give a slightly different error
> Syd> message, however, and that might break some tests.
> 
> I looked at this some more.
> 
> No gdbtk tests test any command for the wrong number of arguments.  So
> we're free to change this however we want.
> 
> Some of the code in gdbtk-cmds.c is quite bad.  For instance there are
> functions which do no checking, or incorrect checking, of the number
> of arguments they are given.
> 
> I discovered that if I just use Tcl_WrongNumArgs and return TCL_ERROR,
> I don't get the right result, because you also have to do this:
> 
>       result_ptr->flags |= GDBTK_IN_TCL_RESULT;
> 
> Note that I changed all the functions in gdbtk-cmds.c that did any
> argument number checking to use Tcl_WrongNumArgs.  However this issue
> has to be resolved first :-(.  I didn't bother fixing all the
> commands, since my time on Insight is limited and I doubt this sort of
> cleanup is very high priority.
> 
> If we really do have to set result_ptr->flags, that's fine --
> but I'd like to add a macro like this:
> 
> #define RETURN_TCL_ERROR \
>     result_ptr->flags |= GDBTK_IN_TCL_RESULT; \
>     return TCL_ERROR
> 
> Then we can just use `RETURN_TCL_ERROR;' all over.  This is ugly, but
> imho more maintainable than remembering to put an assignment
> everywhere we return TCL_ERROR.
> 
> OTOH I don't understand this comment in call_wrapper:
> 
>   /*
>    * Now copy the result over to the true Tcl result.  If GDBTK_TO_RESULT flag
>    * bit is set , this just copies a null object over to the Tcl result,
>    * which is fine because we should reset the result in this case anyway.
>    */
> 
> First, the comment seems to lie.  It mentions GDBTK_TO_RESULT, which
> makes sense, but the code actually checks for GDBTK_IN_TCL_RESULT.
> 
> Ok, I just read the comments in gdbtk.h and it makes a bit more
> sense.  But wouldn't it be easier to adopt the heuristic that if a
> wrapped command returns TCL_ERROR then we should assume that the
> interpreter's result is already correctly set?  Are there cases where
> we explicitly (as opposed to via error()) return TCL_ERROR but rely on
> the call wrapper to set the result?  That seems strange.
> 
> Tom

Re: Patch: fixlet in gdbtk-cmds.c

[Tom Tromey]

Syd> I would rather have a global variable for whether a gdbtk command
Syd> generated the error as opposed to Tcl itself rather than messing
Syd> with interp result flags. This is just asking to break when the
Syd> interp data structure is changed.

I looked at this a bit more.

In the code I quoted `result_ptr' is a global variable like that.
This isn't the intepreter or the result.

I think this patch does what I want:

2000-11-29  Tom Tromey  <tromey@cygnus.com>

	* gdbtk-cmds.c (call_wrapper): Don't reset result if wrapped
	command returned error.

Unfortunately, it is unclear what negative ramifications, if any, this
patch might have.  I'd have to consider it "dangerous".

Syd> I will examine this more in a couple of weeks.

I won't be working on Insight at that point.

Tom

Index: gdbtk-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-cmds.c,v
retrieving revision 1.12
diff -u -r1.12 gdbtk-cmds.c
--- gdbtk-cmds.c	2000/07/25 20:41:07	1.12
+++ gdbtk-cmds.c	2000/11/29 21:46:36
@@ -459,6 +485,7 @@
 {
   struct wrapped_call_args wrapped_args;
   gdbtk_result new_result, *old_result_ptr;
+  int wrapped_returned_error = 0;
 
   old_result_ptr = result_ptr;
   result_ptr = &new_result;
@@ -497,16 +524,24 @@
       Tcl_Eval (interp, "gdbtk_tcl_idle");
 
     }
+  else
+    {
+      /* If the wrapped call returned an error directly, then we don't
+	 want to reset the result.  */
+      wrapped_returned_error = wrapped_args.val == TCL_ERROR;
+    }
 
   /* do not suppress any errors -- a remote target could have errored */
   load_in_progress = 0;
 
   /*
-   * Now copy the result over to the true Tcl result.  If GDBTK_TO_RESULT flag
-   * bit is set , this just copies a null object over to the Tcl result, 
-   * which is fine because we should reset the result in this case anyway.
+   * Now copy the result over to the true Tcl result.  If
+   * GDBTK_TO_RESULT flag bit is set, this just copies a null object
+   * over to the Tcl result, which is fine because we should reset the
+   * result in this case anyway.  If the wrapped command returned an
+   * error, then we assume that the result is already set correctly.
    */
-  if (result_ptr->flags & GDBTK_IN_TCL_RESULT)
+  if ((result_ptr->flags & GDBTK_IN_TCL_RESULT) || wrapped_returned_error)
     {
       Tcl_DecrRefCount (result_ptr->obj_ptr);
     }
@@ -580,8 +615,10 @@
      Tcl_Obj *CONST objv[];
 {
   if (objc != 1)
-    Tcl_SetStringObj (result_ptr->obj_ptr,
-		      "Wrong number of args, none are allowed.", -1);
+    {
+      Tcl_WrongNumArgs (interp, 1, objv, NULL);
+      return TCL_ERROR;
+    }
 
   if (inferior_pid != 0 && target_has_execution)
     {

Re: Patch: fixlet in gdbtk-cmds.c

[Syd Polk]

This looks good then.

At 02:57 PM 11/29/00 -0700, Tom Tromey wrote:
>Syd> I would rather have a global variable for whether a gdbtk command
>Syd> generated the error as opposed to Tcl itself rather than messing
>Syd> with interp result flags. This is just asking to break when the
>Syd> interp data structure is changed.
>
>I looked at this a bit more.
>
>In the code I quoted `result_ptr' is a global variable like that.
>This isn't the intepreter or the result.
>
>I think this patch does what I want:
>
>2000-11-29  Tom Tromey  <tromey@cygnus.com>
>
>         * gdbtk-cmds.c (call_wrapper): Don't reset result if wrapped
>         command returned error.
>
>Unfortunately, it is unclear what negative ramifications, if any, this
>patch might have.  I'd have to consider it "dangerous".

I guess we will have to see what happens then.

>Syd> I will examine this more in a couple of weeks.

Well, I am out of town and unable to hack until late next week. Oh, well.

Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: Patch: fixlet in gdbtk-cmds.c

[Fernando Nasser]

Syd Polk wrote:
> 
> This looks good then.
> 
> At 02:57 PM 11/29/00 -0700, Tom Tromey wrote:
> >Syd> I would rather have a global variable for whether a gdbtk command
> >Syd> generated the error as opposed to Tcl itself rather than messing
> >Syd> with interp result flags. This is just asking to break when the
> >Syd> interp data structure is changed.
> >
> >I looked at this a bit more.
> >
> >In the code I quoted `result_ptr' is a global variable like that.
> >This isn't the intepreter or the result.
> >
> >I think this patch does what I want:
> >
> >2000-11-29  Tom Tromey  <tromey@cygnus.com>
> >
> >         * gdbtk-cmds.c (call_wrapper): Don't reset result if wrapped
> >         command returned error.
> >
> >Unfortunately, it is unclear what negative ramifications, if any, this
> >patch might have.  I'd have to consider it "dangerous".
> 
> I guess we will have to see what happens then.
> 
> >Syd> I will examine this more in a couple of weeks.
> 
> Well, I am out of town and unable to hack until late next week. Oh, well.
> 

Tom,

May I suggest that you register this possible cleanups, with your suggestions
on how to do it in the Gnats database.  As soon as one of us can take some
time to look deeply into this and have time to monitor any possible side
effects.

Thanks.

Fernando





-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: Patch: fixlet in gdbtk-cmds.c

[Tom Tromey]

>>>>> "Fernando" == Fernando Nasser <fnasser@cygnus.com> writes:

Fernando> May I suggest that you register this possible cleanups, with
Fernando> your suggestions on how to do it in the Gnats database.  As
Fernando> soon as one of us can take some time to look deeply into
Fernando> this and have time to monitor any possible side effects.

I already checked it in.
Would you like me to revert it and then add it to Gnats?

Tom

Re: Patch: fixlet in gdbtk-cmds.c

[Fernando Nasser]

Tom Tromey wrote:
> 
> >>>>> "Fernando" == Fernando Nasser <fnasser@cygnus.com> writes:
> 
> Fernando> May I suggest that you register this possible cleanups, with
> Fernando> your suggestions on how to do it in the Gnats database.  As
> Fernando> soon as one of us can take some time to look deeply into
> Fernando> this and have time to monitor any possible side effects.
> 
> I already checked it in.
> Would you like me to revert it and then add it to Gnats?
> 

Nope.  Lets see what happens.  And you still be around for one
more week ;-)

(BTW, it looks fine to me...)



-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9