From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15500 invoked by alias); 7 Feb 2008 19:15:30 -0000 Received: (qmail 15489 invoked by uid 22791); 7 Feb 2008 19:15:29 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate2.de.ibm.com (HELO mtagate2.de.ibm.com) (195.212.29.151) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 07 Feb 2008 19:14:58 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate2.de.ibm.com (8.13.8/8.13.8) with ESMTP id m17JEtb2085774 for ; Thu, 7 Feb 2008 19:14:55 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id m17JEt5J2035862 for ; Thu, 7 Feb 2008 20:14:55 +0100 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m17JEsuN017566 for ; Thu, 7 Feb 2008 20:14:54 +0100 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id m17JEsGC017563; Thu, 7 Feb 2008 20:14:54 +0100 Message-Id: <200802071914.m17JEsGC017563@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 7 Feb 2008 20:14:54 +0100 Subject: Re: [patch] nto target update To: aristovski@qnx.com (Aleksandar Ristovski) Date: Thu, 07 Feb 2008 19:15:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, RMansfield@qnx.com (Ryan Mansfield), aristovski@qnx.com In-Reply-To: <479A5F45.6060101@qnx.com> from "Aleksandar Ristovski" at Jan 25, 2008 05:14:29 PM X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-02/txt/msg00136.txt.bz2 Aleksandar Ristovski wrote: > Sorry for the long delay. I have cleaned up the code and am resubmitting the patch. > > Please review. Thanks for addressing most of the issues I pointed out. The patch is still not quite ready for inclusion, unfortunately. Please see below for a list of further issues to be fixed ... ... in the ChangeLog: > (procfs_notice_signals): New function. This is not actually true; there was a procfs_notice_signals before ... Changes to procfs_create_inferior are not mentioned in the ChangeLog. Changes to _initialize_nto_tdep are not mentioned in the ChangeLog. ... and in the patch itself: > i386-nto-tdep.o: i386-nto-tdep.c $(defs_h) $(frame_h) $(osabi_h) \ > $(regcache_h) $(target_h) $(gdb_assert_h) $(gdb_string_h) \ > $(i386_tdep_h) $(i387_tdep_h) $(nto_tdep_h) $(solib_h) \ >- $(solib_svr4_h) >+ $(solib_svr4_h) $(gdb_stat_h) $(top_h) You have not actually added these includes to i386-nto-tdep.c, so you shouldn't change the dependencies either ... (Or maybe some part of the patch got lost?) > nto-tdep.o: nto-tdep.c $(gdb_stat_h) $(gdb_string_h) $(nto_tdep_h) $(top_h) \ > $(cli_decode_h) $(cli_cmds_h) $(inferior_h) $(gdbarch_h) $(bfd_h) \ >- $(elf_bfd_h) $(solib_svr4_h) $(gdbcore_h) $(objfiles_h) >+ $(elf_bfd_h) $(solib_svr4_h) $(gdbcore_h) $(objfiles_h) \ >+ $(filenames_h) $(gdbcmd_h) $(safe_ctype_h) $(gdb_assert_h) Please keep the sequence of the dependency list in sync with the sequence of include files in the source. You have added "gdb_assert_h" as the first include file in nto-tdep.c, so you should also add the dependency first in the Makefile. >+ /* Thread is alive or dead but not yet joined, >+ or dead and there is an alive (or dead unjoined) thread with >+ higher tid. We return tidinfo. >+ >+ Client should check if the tid is the same as >+ requested; if not, requested tid is dead. */ Comment indentation is a bit unusual. It should be aligned like this: /* Thread is alive or dead but not yet joined, or dead and there is an alive (or dead unjoined) thread with higher tid. We return tidinfo. Also, there should be two spaces after a '.'. >+ if (sigwaitres == -1) >+ perror_with_name (__FILE__); You should probably use some descriptive text as argument to perror_with_name, e.g. perror_with_name ("sigwaitinfo failed"). >+/* Workaround for aliasing rules violation. In our case, >+ violation is o.k. We use sigaddset on fltset_t which is >+ equivalent to sigset_t in nto. */ Again the comment indentation. >+typedef union >+ { >+ sigset_t *ps; >+ fltset_t *pflt; >+ } ufltset; > >+ /* Workaround for aliasing rules violation. */ >+ ufltset psigset; > >+ psigset.pflt = &run.fault; >+ sigemptyset (psigset.ps); This seems odd. The original code: >- sigemptyset ((sigset_t *) &run.fault); does not appear to violate any aliasing rules (and if it did, the use of union on the *pointer* types would certainly not cure the violation). >+ if ((status.why == _DEBUG_WHY_REQUESTED) || >+ (status.why & (_DEBUG_WHY_SIGNALLED | _DEBUG_WHY_FAULTED))) Please format like if (... || ...) instead of if (... || ...) > init_procfs_ops (void) > { >+ memset (&procfs_ops, 0, sizeof (procfs_ops)); > procfs_ops.to_shortname = "procfs"; This change should be unnecessary; the structure is default- initialized to zero ... > if (ret >= 0) > *temp_pathname = gdb_realpath (arch_path); > else >- **temp_pathname = '\0'; >+ { >+ if (*temp_pathname) >+ **temp_pathname = '\0'; >+ else >+ *temp_pathname = ""; >+ } This is still wrong. It should simply be *temp_pathname = NULL; (See e.g. the implementation of openp -- this routine should follow the same rules.) >+#if defined (__MINGW32__) >+#define PATH_SEP ";" >+#else >+#define PATH_SEP ":" >+#endif This should be just DIRNAME_SEPARATOR as defined in defs.h, right? >+ sprintf (buf, "set solib-search-path %s/%s" PATH_SEP "%s/%s", arch_path, "lib", arch_path, "usr/lib"); Line too long. >+nto_parse_redirection (char *pargv[], const char **pin, const char **pout, const char **perr) Likewise. >+ gdb_byte *buf = so->lm_info->lm + lmo->l_addr_offset; >+ if (NULL == buf) >+ return 0; This seems wrong -- do you really want to check whether the *sum* of lm plus l_addr_offset is NULL? > return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset, >- builtin_type_void_data_ptr); >+ builtin_type_void_data_ptr); Why this change? The original indentation is fine ... >+ sec->addr = nto_truncate_ptr (sec->addr + LM_ADDR_FROM_LINK_MAP (so) - vaddr); >+ sec->endaddr = nto_truncate_ptr (sec->endaddr + LM_ADDR_FROM_LINK_MAP (so) - vaddr); Lines too long. >- if (in_plt_section (pc, NULL)) >- return 1; >+ if (in_plt_section (pc, NULL)) >+ { >+ return 1; >+ } Please leave the original formatting, which corresponds to the GDB coding conventions ... >+char * >+nto_target_extra_thread_info (struct thread_info *ti) >+{ >+ if (ti && ti->private && ti->private->name[0]) >+ return ti->private->name; >+ return ""; >+} I cannot see any place where this is called. I assume you intended to use this as the to_extra_thread_info callback in the target operations structure? The code to set this up apparently got lost somehow ... >+ fprintf_filtered (file, _("QNX NTO debug level is %d.\n"), nto_internal_debugging); Line too long. >+ printf_filtered("%c%d\t%d\t%d\n", ptid_equal (tp->ptid, inferior_ptid) ? '*' : ' ', tp->private->tid, tp->private->state, tp->private->flags ); Likewise. Also you should have a space before '(', but no space before the ')' of the printf_filtered call. >+ printf_filtered("Threads for pid %d (%s)\nTid:\tState:\tFlags:\n", ptid_get_pid (inferior_ptid), get_exec_file (0)); Line too long; missing space before '('. >- NULL, /* FIXME: i18n: QNX NTO internal debugging is %s. */ >- &setdebuglist, &showdebuglist); >+ show_nto_debug, >+ &maintenance_set_cmdlist, >+ &maintenance_show_cmdlist); This change moves the command from set debug nto-debug to set nto-debug If this is deliberate, you'll need to update the documentation to reflect this change. >+ add_info ("tidinfo", nto_info_tidinfo_command, "List threads for current process." ); Line too long. Also, this adds a new command, so you'll also have to add it to the documentation. Maybe the command name should be changes to reflect the fact that it is NTO-specific? >+/* Used by gdbthread.h. Same as struct tidinfo in pdebug protocol */ '.' (followed by two spaces) missing at the end of the comment. >+struct private_thread_info { The '{' belongs on the start of the next line. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com