[PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

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

This last part modifies the build system (in order to implement the
GDB's datadir), adds the documentation and the testcase for the new
feature.

Regards,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

gdb/ChangeLog:

2008-11-04  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>

	* Makefile.in: Support for relocatable GDB datadir and XML
	syscall.
	* configure.ac: Support for relocatable GDB datadir.

gdb/doc/ChangeLog:

2008-11-04  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>

	* gdb.texinfo (catch syscall): Documentation about the new
	feature.

gdb/testsuite/ChangeLog:

2008-11-04  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>

	* Makefile.in: Inclusion of catch-syscall object.
	* gdb.base/catch-syscall.c: New file.
	* gdb.base/catch-syscall.exp: New file.

[-- Attachment #2: catch-syscall-build-doc-testcase.patch --]
[-- Type: text/x-patch, Size: 14954 bytes --]

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5432c88..1712f19 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -172,6 +172,9 @@ LIBICONV = @LIBICONV@
 TARGET_SYSTEM_ROOT = @TARGET_SYSTEM_ROOT@
 TARGET_SYSTEM_ROOT_DEFINE = @TARGET_SYSTEM_ROOT_DEFINE@
 
+# Did the user give us a --with-gdb-datadir option?
+GDB_DATADIR_PATH = @GDB_DATADIR_PATH@
+
 # Helper code from gnulib.
 LIBGNU = gnulib/libgnu.a
 INCGNU = -I$(srcdir)/gnulib -Ignulib
@@ -657,6 +660,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	valarith.c valops.c valprint.c value.c varobj.c vec.c \
 	wrapper.c \
 	xml-tdesc.c xml-support.c \
+	xml-syscall.c \
 	inferior.c
 
 LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -727,7 +731,7 @@ config/sparc/nm-sol2.h config/nm-linux.h config/mips/nm-irix5.h	\
 config/rs6000/nm-rs6000.h top.h bsd-kvm.h gdb-stabs.h reggroups.h \
 annotate.h sim-regno.h dictionary.h dfp.h main.h frame-unwind.h	\
 remote-fileio.h i386-linux-tdep.h vax-tdep.h objc-lang.h \
-sentinel-frame.h bcache.h symfile.h
+sentinel-frame.h bcache.h symfile.h xml-syscall.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -806,10 +810,16 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	trad-frame.o \
 	tramp-frame.o \
 	solib.o solib-null.o \
-	prologue-value.o memory-map.o xml-support.o \
+	prologue-value.o memory-map.o xml-support.o xml-syscall.o \
 	target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
 	inferior.o
 
+# Definitions for the syscall's XML files and dir
+XML_SYSCALLS_DIR = syscalls/
+XML_SYSCALLS_FILES = gdb-syscalls.dtd \
+		     ppc-syscalls.xml ppc64-syscalls.xml \
+		     i386-syscalls.xml
+
 TSOBS = inflow.o
 
 SUBDIRS = @subdirs@
@@ -843,11 +853,35 @@ generated_files = config.h observer.h observer.inc ada-lex.c \
 	$(COMPILE) $<
 	$(POSTCOMPILE)
 
-all: gdb$(EXEEXT) $(CONFIG_ALL)
+all: gdb$(EXEEXT) $(CONFIG_ALL) xml-syscall-copy
 	@$(MAKE) $(FLAGS_TO_PASS) DO=all "DODIRS=`echo $(SUBDIRS) | sed 's/testsuite//'`" subdir_do
 .PHONY: all-tui
 all-tui: $(TUI)$(EXEEXT)
 
+xml-syscall-copy:
+	list='$(XML_SYSCALLS_FILES)' ; \
+	for file in $$list ; do \
+	  f=$(srcdir)/$(XML_SYSCALLS_DIR)/$$file ; \
+	  if test -f $$f ; then \
+	    $(INSTALL_DATA) $$f \
+	    	./$(XML_SYSCALLS_DIR) ; \
+	  fi ; \
+	done ;
+
+# This target is responsible for properly installing the syscalls'
+# XML files in the system.
+xml-syscall-install:
+	$(SHELL) $(srcdir)/../mkinstalldirs \
+		$(DESTDIR)$(GDB_DATADIR_PATH)/$(XML_SYSCALLS_DIR) ; \
+	list='$(XML_SYSCALLS_FILES)' ; \
+	for file in $$list ; do \
+	  f=$(srcdir)/$(XML_SYSCALLS_DIR)/$$file ; \
+	  if test -f $$f ; then \
+	    $(INSTALL_DATA) $$f \
+	    	$(DESTDIR)$(GDB_DATADIR_PATH)/$(XML_SYSCALLS_DIR) ; \
+	  fi ; \
+	done ;
+
 installcheck:
 
 # The check target can not use subdir_do, because subdir_do does not
@@ -901,7 +935,10 @@ gdb.z:gdb.1
 # source file and doesn't care about rebuilding or just wants to save the
 # time it takes for make to check that all is up to date.
 # install-only is intended to address that need.
-install: all install-only
+#
+# Also, the "install" target installs the syscalls' XML files in the system.
+install: all install-only xml-syscall-install
+
 install-only: $(CONFIG_INSTALL)
 	transformed_name=`t='$(program_transform_name)'; \
 			  echo gdb | sed -e "$$t"` ; \
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 054d5c2..cc5dc9a 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -118,7 +118,37 @@ case ${debugdir} in
   ;;
 esac
 
-AC_CONFIG_SUBDIRS(doc testsuite)
+# GDB's datadir relocation
+
+gdbdatadir=${datadir}/gdb
+
+AC_ARG_WITH(gdb-datadir,
+[  --with-gdb-datadir=path   Look for global separate data files in this path [DATADIR/gdb]],
+[gdbdatadir="${withval}"])
+
+AC_DEFINE_DIR(GDB_DATADIR, gdbdatadir,
+              [Global directory for GDB data files. ])
+
+if test "x$exec_prefix" = xNONE || test "x$exec_prefix" = 'x${prefix}'; then
+  if test "x$prefix" = xNONE; then
+    test_prefix=/usr/local
+  else
+    test_prefix=$prefix
+  fi
+else
+  test_prefix=$exec_prefix
+fi
+
+case ${gdbdatadir} in
+  "${test_prefix}"|"${test_prefix}/"*|\
+  '${exec_prefix}'|'${exec_prefix}/'*)
+    AC_DEFINE(GDB_DATADIR_RELOCATABLE, 1, [Define if GDB datadir should be relocated when GDB is moved.])
+  ;;
+esac
+GDB_DATADIR_PATH=${gdbdatadir}
+AC_SUBST(GDB_DATADIR_PATH)
+
+AC_CONFIG_SUBDIRS(doc testsuite syscalls)
 
 # Check whether to support alternative target configurations
 AC_ARG_ENABLE(targets,
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 261d1c7..3641fff 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3635,6 +3635,12 @@ A failed Ada assertion.
 A call to @code{exec}.  This is currently only available for HP-UX
 and @sc{gnu}/Linux.
 
+@item syscall
+@itemx syscall @var{syscall name}
+@cindex break on call to/return from a system call
+A call to/return from a @code{syscall} -- similar to the @code{strace}
+utility.
+
 @item fork
 A call to @code{fork}.  This is currently only available for HP-UX
 and @sc{gnu}/Linux.
diff --git a/gdb/testsuite/gdb.base/Makefile.in b/gdb/testsuite/gdb.base/Makefile.in
index 9f382db..12db521 100644
--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -12,7 +12,7 @@ EXECUTABLES = all-types annota1 bitfields break \
 	scope section_command setshow setvar shmain sigall signals \
 	solib solib_sl so-impl-ld so-indr-cl \
 	step-line step-test structs structs2 \
-	twice-tmp varargs vforked-prog watchpoint whatis
+	twice-tmp varargs vforked-prog watchpoint whatis catch-syscall
 
 MISCELLANEOUS = coremmap.data ../foobar.baz \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl
diff --git a/gdb/testsuite/gdb.base/catch-syscall.c b/gdb/testsuite/gdb.base/catch-syscall.c
new file mode 100644
index 0000000..64850de
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-syscall.c
@@ -0,0 +1,25 @@
+/* This file is used to test the 'catch syscall' feature on GDB.
+ 
+   Please, if you are going to edit this file DO NOT change the syscalls
+   being called (nor the order of them).  If you really must do this, then
+   take a look at catch-syscall.exp and modify there too.
+
+   Written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
+   September, 2008 */
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+
+int
+main (void)
+{
+	/* A close() with a wrong argument.  We are only
+	   interested in the syscall.  */
+	close (-1);
+
+	chroot (".");
+
+	/* The last syscall.  Do not change this.  */
+	_exit (0);
+}
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
new file mode 100644
index 0000000..1358828
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -0,0 +1,251 @@
+#   Copyright 1997, 1999, 2007, 2008 Free Software Foundation, Inc.
+
+# 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 3 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, see <http://www.gnu.org/licenses/>.
+
+
+# This program tests the 'catch syscall' functionality.
+#
+# It was written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
+# on September/2008.
+
+if { [is_remote target] || ![isnative] } then {
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+global srcfile
+set testfile "catch-syscall"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested catch-syscall.exp
+     return -1
+}
+
+# Until "catch syscall" is implemented on other targets...
+if {![istarget "hppa*-hp-hpux*"] && ![istarget "*-linux*"]} then {
+    continue
+}
+
+# Internal procedure used to check if, before any syscall is caught,
+# the command 'info breakpoints' correctly lists the catchpoints AND
+# says that nothing was caught yet.
+proc check_init_info_breakpoints {} {
+    global gdb_prompt
+
+    # Verifying that the catchpoint appears in the 'info breakpoints'
+    # command, but with "<unknown syscall>".
+    set thistest "catch syscall appears in 'info breakpoints'"
+    gdb_test "info breakpoints" ".*catchpoint.*keep y.*syscall \"<unknown syscall>\".*" $thistest
+}
+
+# This procedure checks if, after a syscall catchpoint is hit, the
+# command 'info breakpoints' correctly lists the syscall name.
+proc check_info_breakpoints { syscall_name } {
+    global gdb_prompt
+
+    set thistest "syscall '$syscall_name' appears in 'info breakpoints'"
+    gdb_test "info breakpoints" ".*catchpoint.*keep y.*syscall \"$syscall_name\".*" $thistest
+}
+
+# This procedure checks if there was a call to a syscall.
+proc check_call_to_syscall { syscall_name } {
+    global gdb_prompt
+
+    set thistest "program has called '$syscall_name'"
+    gdb_test "continue" "Catchpoint .*(calling syscall .$syscall_name ().).*" $thistest
+
+    # Checking if the syscall is reported to be caught in
+    # 'info breakpoints'.
+    check_info_breakpoints $syscall_name
+}
+
+# This procedure checks if the syscall returned.
+proc check_return_from_syscall { syscall_name } {
+    global gdb_prompt
+
+    set thistest "syscall '$syscall_name' has returned"
+    gdb_test "continue" "Catchpoint .*(returned from syscall .$syscall_name ().).*" $thistest
+
+    # Checking if the syscall is reported to be caught in
+    # 'info breakpoints'.
+    check_info_breakpoints $syscall_name
+}
+
+# Internal procedure that performs two 'continue' commands and checks if
+# a syscall call AND return occur.
+proc check_continue { syscall_name } {
+    global gdb_prompt
+
+    # Testing if the 'continue' stops at the
+    # specified syscall_name.  If it does, then it should
+    # first print that the infeior has called the syscall,
+    # and after print that the syscall has returned.
+
+    # Testing if the inferiorr has called the syscall.
+    check_call_to_syscall $syscall_name
+    # And now, that the syscall has returned.
+    check_return_from_syscall $syscall_name
+}
+
+# Inserts a syscall catchpoint with an argument.
+proc insert_catch_syscall_with_arg { syscall_name } {
+    global gdb_prompt
+
+    # Trying to set the syscall
+    set thistest "catch syscall with arguments ($syscall_name)"
+    gdb_test "catch syscall $syscall_name" "Catchpoint .*(syscall .$syscall_name ().).*" $thistest
+
+}
+
+proc check_for_program_end {} {
+    global gdb_prompt
+
+    # Deleting the catchpoints
+    delete_breakpoints
+
+    set thistest "successful program end"
+    gdb_test "continue" "Program exited normally.*" $thistest
+
+}
+
+proc test_catch_syscall_without_args {} {
+    global gdb_prompt
+    # All (but the last) syscalls from the example code
+    # They are ordered according to the file, so do not change this.
+    set all_syscalls { "close" "chroot" }
+    # The last syscall (exit()) does not return, so
+    # we cannot expect the catchpoint to be triggered
+    # twice.  It is a special case.
+    set last_syscall "exit_group"
+
+    # Trying to set the syscall
+    set thistest "setting catch syscall without arguments"
+    gdb_test "catch syscall" "Catchpoint .*(syscall).*" $thistest
+
+    check_init_info_breakpoints
+
+    # We have to check every syscall
+    foreach name $all_syscalls {
+        check_continue $name
+    }
+
+    # At last but not least, we check if the inferior
+    # has called the last (exit) syscall.
+    check_call_to_syscall $last_syscall
+
+    # Now let's see if the inferior correctly finishes.
+    check_for_program_end
+}
+
+proc test_catch_syscall_with_args {} {
+    global gdb_prompt
+    set syscall_name "close"
+
+    insert_catch_syscall_with_arg $syscall_name 
+    check_init_info_breakpoints
+
+    # Can we continue until we catch the syscall?
+    check_continue $syscall_name
+
+    # Now let's see if the inferior correctly finishes.
+    check_for_program_end
+}
+
+proc test_catch_syscall_with_wrong_args {} {
+    global gdb_prompt
+    # mlock is not called from the source
+    set syscall_name "mlock"
+
+    insert_catch_syscall_with_arg $syscall_name
+    check_init_info_breakpoints
+
+    # Now, we must verify if the program stops with a continue.
+    # If it doesn't, everything is right (since we don't have
+    # a syscall named "mlock" in it).  Otherwise, this is a failure.
+    set thistest "catch syscall with unused syscall ($syscall_name)"
+    gdb_test "continue" "Program exited normally.*" $thistest
+}
+
+proc test_catch_syscall_restarting_inferior {} {
+    global gdb_prompt
+    set syscall_name "chroot"
+
+    insert_catch_syscall_with_arg $syscall_name
+    check_init_info_breakpoints
+
+    # Let's first reach the call of the syscall.
+    check_call_to_syscall $syscall_name
+
+    # Now, restart the program
+    rerun_to_main
+
+    # And check for call/return
+    check_continue $syscall_name
+
+    # Can we finish?
+    check_for_program_end
+}
+
+proc do_syscall_tests {} {
+    global gdb_prompt srcdir
+
+    # First, we need to set GDB datadir.
+    send_gdb "maintenance set gdb_datadir $srcdir/..\n"
+    gdb_expect 10 {
+	-re "$gdb_prompt $" {
+	    verbose "Setting GDB datadir to $srcdir/..." 2
+	}
+	timeout {
+	    error "Couldn't set GDB datadir."
+	}
+    }
+
+    # Verify that the 'catch syscall' help is available
+    set thistest "help catch syscall"
+    gdb_test "help catch syscall" "Catch calls to syscalls.*" $thistest
+
+    # Try to set a catchpoint to a nonsense syscall
+    set thistest "catch syscall to a nonsense syscall is prohibited"
+    gdb_test "catch syscall nonsense_syscall" "Invalid syscall name .*" $thistest
+
+    # Testing the 'catch syscall' command without arguments.
+    # This test should catch any syscalls.
+    if [runto_main] then { test_catch_syscall_without_args }
+
+    # Testing the 'catch syscall' command with arguments.
+    # This test should only catch the specified syscall.
+    if [runto_main] then { test_catch_syscall_with_args }
+
+    # Testing the 'catch syscall' command with WRONG arguments.
+    # This test should not trigger any catchpoints.
+    if [runto_main] then { test_catch_syscall_with_wrong_args }
+
+    # Testing the 'catch' syscall command during a restart of
+    # the inferior.
+    if [runto_main] then { test_catch_syscall_restarting_inferior }
+}
+
+# Start with a fresh gdb
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Execute the tests
+do_syscall_tests

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Phil Muldoon]

Sérgio Durigan Júnior wrote:
> This last part modifies the build system (in order to implement the
> GDB's datadir), adds the documentation and the testcase for the new
> feature.
>   

Nice, looks great!  Just some comments on tests.

This is a forward of a comment from the archer list on the original 
patch-set.  On targets that do support syscall breakpoints, but there is 
no xml database (like on x8664) will these tests just fail? Are they 
gated to XFAIL (or however one would gracefully fail here).

My only other comment is how this would work on remote targets (i.e.  
remote target is x86, but your host is PPC).

Regards

Phil Muldoon

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

Hi Phil,

On Tue, 2008-11-04 at 08:18 +0000, Phil Muldoon wrote:
> Sérgio Durigan Júnior wrote:
> > This last part modifies the build system (in order to implement the
> > GDB's datadir), adds the documentation and the testcase for the new
> > feature.
> >   
> 
> Nice, looks great!  Just some comments on tests.
> 
> This is a forward of a comment from the archer list on the original 
> patch-set.  On targets that do support syscall breakpoints, but there is 
> no xml database (like on x8664) will these tests just fail? Are they 
> gated to XFAIL (or however one would gracefully fail here).

Oh, sorry about this. I completely forgot about our conversation. The
way it is now, the patch will fail if the arch doesn't provide the XML
file. You're right, I need to fix this and put an XFAIL.

> My only other comment is how this would work on remote targets (i.e.  
> remote target is x86, but your host is PPC).

To be completely honest, I still don't know the answer to this question.
I'd be glad if some good soul could try to answer this, so that I can
fix things if needed.

Thanks for your valid comments.

Regards,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Daniel Jacobowitz]

On Tue, Nov 04, 2008 at 01:01:59PM -0200, Sérgio Durigan Júnior wrote:
> > My only other comment is how this would work on remote targets (i.e.  
> > remote target is x86, but your host is PPC).
> 
> To be completely honest, I still don't know the answer to this question.
> I'd be glad if some good soul could try to answer this, so that I can
> fix things if needed.

Your patch relies on a change to the native file, right?  If so, it
will never trigger on remote.  In order for it to trigger, we'd need:

 - a new remote protocol extension in gdb.texinfo
 - remote.c implementation
 - gdbserver implementation

It shouldn't be too hard for someone to add it later.  So restricting
the tests to native targets would be enough.  Search for
[is_remote target] in the testsuite.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

Hi Daniel,

On Tue, 2008-11-04 at 10:14 -0500, Daniel Jacobowitz wrote:

> It shouldn't be too hard for someone to add it later.  So restricting
> the tests to native targets would be enough.  Search for
> [is_remote target] in the testsuite.

I don't know if I'm doing this correctly, but I already use [is_remote
target] in the testcase. If this test returns true, I simply do a 
"continue", which (I think) skips this test and goes to the next one. If
that's not correct, then I should fix it :-).

Regards,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Daniel Jacobowitz]

On Tue, Nov 04, 2008 at 01:35:24PM -0200, Sérgio Durigan Júnior wrote:
> Hi Daniel,
> 
> On Tue, 2008-11-04 at 10:14 -0500, Daniel Jacobowitz wrote:
> 
> > It shouldn't be too hard for someone to add it later.  So restricting
> > the tests to native targets would be enough.  Search for
> > [is_remote target] in the testsuite.
> 
> I don't know if I'm doing this correctly, but I already use [is_remote
> target] in the testcase. If this test returns true, I simply do a 
> "continue", which (I think) skips this test and goes to the next one. If
> that's not correct, then I should fix it :-).

In that case, you're covered.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Tom Tromey]

>>>>> "Sérgio" == Sérgio Durigan Júnior <sergiodj@linux.vnet.ibm.com> writes:

Sérgio> This last part modifies the build system (in order to
Sérgio> implement the GDB's datadir), adds the documentation and the
Sérgio> testcase for the new feature.

Nice.

Sérgio> +xml-syscall-copy:
Sérgio> +	list='$(XML_SYSCALLS_FILES)' ; \
Sérgio> +	for file in $$list ; do \

Two nits here.

First, if you build with builddir!=srcdir, you need to mkdir
XML_SYSCALLS_DIR here.

Second, I think it would be nice to detect the case where
builddir==srcdir and not copy in that case.

Sérgio> +xml-syscall-install:
Sérgio> +	$(SHELL) $(srcdir)/../mkinstalldirs \
Sérgio> +		$(DESTDIR)$(GDB_DATADIR_PATH)/$(XML_SYSCALLS_DIR) ; \

Using DESTDIR is nice attention to detail :-)

Sérgio> +# Also, the "install" target installs the syscalls' XML files in the system.
Sérgio> +install: all install-only xml-syscall-install

I think install-only should depend on xml-syscall-install.
I believe the intent here is that "make install-only" will install
everything without rebuilding.

Sérgio> +AC_ARG_WITH(gdb-datadir,
Sérgio> +[  --with-gdb-datadir=path   Look for global separate data files in this path [DATADIR/gdb]],

It is preferable, IMO, to use AS_HELP_STRING here.
There are some examples of this in gdb's configure.ac.

Sérgio> +AC_CONFIG_SUBDIRS(doc testsuite syscalls)
 
You don't want this.  There is no syscalls directory, and even if
there was, I would argue that it should not have its own configure
script.

Sérgio> +@item syscall
Sérgio> +@itemx syscall @var{syscall name}
Sérgio> +@cindex break on call to/return from a system call
Sérgio> +A call to/return from a @code{syscall} -- similar to the @code{strace}
Sérgio> +utility.

This needs a minor update now that "catch syscall" can take multiple
arguments.

I didn't read the implementation patch in detail... if it parses
numeric syscalls, then that should be mentioned here.

Finally, instead of "call to/return from", how about "call to or
return from"?  This is more grammatical and not much longer :-)

Tom

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Pedro Alves]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain;   charset="utf-8", Size: 428 bytes --]

Hi,

On Tuesday 04 November 2008 04:31:28, Sérgio Durigan Júnior wrote:

>         * Makefile.in: Support for relocatable GDB datadir and XML
>         syscall.
>         * configure.ac: Support for relocatable GDB datadir.

Minor nit here, please spell out entries for files are to
be regenerated.

  e.g.,
   * configure, config.in, ... : Regenerate.

-- 
Pedro Alves
\x16º&Öéj×!zÊÞ¶êççÞ¹æX¬µªÜ†\a[¥«\…ë

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

Olá Pedro :-)

On Tue, 2008-11-04 at 20:03 +0000, Pedro Alves wrote:
> Hi,
> 
> On Tuesday 04 November 2008 04:31:28, Sérgio Durigan Júnior wrote:
> 
> >         * Makefile.in: Support for relocatable GDB datadir and XML
> >         syscall.
> >         * configure.ac: Support for relocatable GDB datadir.
> 
> Minor nit here, please spell out entries for files are to
> be regenerated.

I just haven't spelled out because those files (configure, config.in)
are not included in the patch. Should I mention them anyway?

Regards,

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Pedro Alves]

On Tuesday 04 November 2008 20:32:35, Sérgio Durigan Júnior wrote:
> I just haven't spelled out because those files (configure, config.in)
> are not included in the patch. Should I mention them anyway?

Yep.  It helps us make sure the right pieces will be
regenerated; and help the ones that want to try the patch to know
which files they need to regenerate, and you'll have to spell them out
anyway at commit time.  :-)

(personally, if regenerated changes are small-ish, I don't mind if
they're included in the patch, since it makes it easier to
try the patches out (bandwidth is cheaper nowadays), and confirm the
submitter is using the correct autotools version.  But, that's not
established practice, and I'm a newbie here  :-).  In fact, not
including them is recommended in gdb/CONTRIBUTE.)

-- 
Pedro Alves

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Eli Zaretskii]

> From: =?ISO-8859-1?Q?S=E9rgio?= Durigan =?ISO-8859-1?Q?J=FAnior?= <sergiodj@linux.vnet.ibm.com>
> Date: Tue, 04 Nov 2008 02:31:28 -0200
> 
> This last part modifies the build system (in order to implement the
> GDB's datadir), adds the documentation and the testcase for the new
> feature.

Thanks.

However,

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 261d1c7..3641fff 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3635,6 +3635,12 @@ A failed Ada assertion.
>  A call to @code{exec}.  This is currently only available for HP-UX
>  and @sc{gnu}/Linux.
>  
> +@item syscall
> +@itemx syscall @var{syscall name}
> +@cindex break on call to/return from a system call
> +A call to/return from a @code{syscall} -- similar to the @code{strace}
> +utility.

...this is not really a user documentation.  Please describe the
feature instead of sending the reader to look for external
documentation of an unrelated utility, which, btw, is available only
on some versions of Unix, but not on others.

At the very least, we should explain what is @var{syscall name}.  An
example of usage and typical GDB output get bonus points.

Thanks.

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

On Tue, 2008-11-04 at 22:53 +0200, Eli Zaretskii wrote:
> > From: =?ISO-8859-1?Q?S=E9rgio?= Durigan =?ISO-8859-1?Q?J=FAnior?= <sergiodj@linux.vnet.ibm.com>
> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> > index 261d1c7..3641fff 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -3635,6 +3635,12 @@ A failed Ada assertion.
> >  A call to @code{exec}.  This is currently only available for HP-UX
> >  and @sc{gnu}/Linux.
> >  
> > +@item syscall
> > +@itemx syscall @var{syscall name}
> > +@cindex break on call to/return from a system call
> > +A call to/return from a @code{syscall} -- similar to the @code{strace}
> > +utility.
> 
> ...this is not really a user documentation.  Please describe the
> feature instead of sending the reader to look for external
> documentation of an unrelated utility, which, btw, is available only
> on some versions of Unix, but not on others.
> 
> At the very least, we should explain what is @var{syscall name}.  An
> example of usage and typical GDB output get bonus points.

I will fix this. Thanks.

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil

Re: [PATCH 4/4] 'catch syscall' feature -- Build system, documentation and testcase

[Sérgio Durigan Júnior]

Hi Tom,

On Tue, 2008-11-04 at 09:32 -0700, Tom Tromey wrote:
> >>>>> "Sérgio" == Sérgio Durigan Júnior <sergiodj@linux.vnet.ibm.com> writes:
> 
> Sérgio> This last part modifies the build system (in order to
> Sérgio> implement the GDB's datadir), adds the documentation and the
> Sérgio> testcase for the new feature.
> 
> Nice.
> 
> Sérgio> +xml-syscall-copy:
> Sérgio> +	list='$(XML_SYSCALLS_FILES)' ; \
> Sérgio> +	for file in $$list ; do \
> 
> Two nits here.
> 
> First, if you build with builddir!=srcdir, you need to mkdir
> XML_SYSCALLS_DIR here.
> 
> Second, I think it would be nice to detect the case where
> builddir==srcdir and not copy in that case.

You're right. Thanks, I'll fix it.

> Sérgio> +xml-syscall-install:
> Sérgio> +	$(SHELL) $(srcdir)/../mkinstalldirs \
> Sérgio> +		$(DESTDIR)$(GDB_DATADIR_PATH)/$(XML_SYSCALLS_DIR) ; \
> 
> Using DESTDIR is nice attention to detail :-)

Haha, I'm learning :-)

> Sérgio> +# Also, the "install" target installs the syscalls' XML files in the system.
> Sérgio> +install: all install-only xml-syscall-install
> 
> I think install-only should depend on xml-syscall-install.
> I believe the intent here is that "make install-only" will install
> everything without rebuilding.

Ok, consider it done.

> Sérgio> +AC_ARG_WITH(gdb-datadir,
> Sérgio> +[  --with-gdb-datadir=path   Look for global separate data files in this path [DATADIR/gdb]],
> 
> It is preferable, IMO, to use AS_HELP_STRING here.
> There are some examples of this in gdb's configure.ac.

OK, no problem by me. But I did a cut & paste from the other parts
(specifically the DEBUGDIR part), which does exactly this :-). Just to
let you know.

> Sérgio> +AC_CONFIG_SUBDIRS(doc testsuite syscalls)
>  
> You don't want this.  There is no syscalls directory, and even if
> there was, I would argue that it should not have its own configure
> script.

Sure, this was totally my fault. I'm sorry.

> Sérgio> +@item syscall
> Sérgio> +@itemx syscall @var{syscall name}
> Sérgio> +@cindex break on call to/return from a system call
> Sérgio> +A call to/return from a @code{syscall} -- similar to the @code{strace}
> Sérgio> +utility.
> 
> This needs a minor update now that "catch syscall" can take multiple
> arguments.
> 
> I didn't read the implementation patch in detail... if it parses
> numeric syscalls, then that should be mentioned here.

Yeah, thanks. Eli already pointed this error too. Thanks.

> 
> Finally, instead of "call to/return from", how about "call to or
> return from"?  This is more grammatical and not much longer :-)
> 
> Tom
-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil