Re: [patch] nto target update

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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