This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, v3] PR 20569, segv in follow_exec


On 10/24/2016 10:57 PM, Luis Machado wrote:
> Follows the updated patch with the comments addressed and a few more things
> adjusted. I can't say i like the name "maybe_open_exec_file", but that was
> the best that came to mind on a Monday. :-)

"maybe" is often used when a function first checks some condition
before doing the actual work.  This is not the case here.
The whole point of the function is actually trying to open the file,
with try/catch + warning handling.  So I'd suggest s/maybe_open_.../try_open_.../.

> 
> How does it look now?

Getting closer, but not yet, sorry.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 43175ff..0c6cc6d 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,27 @@
> +2016-10-24  Sandra Loosemore  <sandra@codesourcery.com>
> +	    Luis Machado  <lgustavo@codesourcery.com>
> +
> +	PR gdb/20569
> +	gdb/

Please don't add this "gdb/".

>  void
> -exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
> +maybe_open_exec_file (const char *exec_file_host, struct inferior *inf,
> +		      int main_symbol_file, int defer_bp_reset, int from_tty)

"main_symbol_file" and "defer_bp_reset" are both symfile_flags:
SYMFILE_MAINLINE, SYMFILE_DEFER_BP_RESET.  Seems to me that
the function could take a single symfile_add_flags parameter
instead of those two, helping avoid the "boolean trap".

>      {
> -      if (defer_bp_reset)
> -	current_inferior ()->symfile_flags |= SYMFILE_DEFER_BP_RESET;
> -      symbol_file_add_main (full_exec_path, from_tty);
> +      TRY
> +	{
> +	  if (main_symbol_file)
> +	    {
> +	      if (defer_bp_reset)
> +		inf->symfile_flags |= SYMFILE_DEFER_BP_RESET;
> +	      symbol_file_add_main (exec_file_host, from_tty);
> +	    }
> +	  else
> +	    {
> +	      symbol_file_add (exec_file_host,
> +			       (inf->symfile_flags
> +			       | SYMFILE_MAINLINE

Both then/else branches are adding a main file...  We should
be able to merge the branches.  E.g., export symbol_file_add_main_1
and call it here, passing down the new add_flags parameter that
I suggested above.

 | SYMFILE_DEFER_BP_RESET),

See, this here is not considering the defer_bp_reset argument.

> +			       NULL, 0);
> +
> +	      if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> +		set_initial_language ();
> +	    }
> +	}
> +      CATCH (err, RETURN_MASK_ERROR)
> +	{
> +	  if (!exception_print_same (prev_err, err))
> +	    warning ("%s", err.message);
> +
> +	  if (main_symbol_file)
> +	    inf->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;


And now this isn't restored on normal return.  It was, on the
current code, AFAICS.

> +	}
> +      END_CATCH
>      }
> -  CATCH (err, RETURN_MASK_ERROR)
> +
> +  do_cleanups (old_chain);
> +}
> +
> +/* See gdbcore.h.  */
> +
> +void
> +exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
> +{
> +  char *exec_file_target, *exec_file_host = NULL;

Don't need to initialize to NULL.  The existing code needs it,
because the path to initialize it may be skipped.  But that's not
the case any longer here.

> +  struct cleanup *old_chain;
> +
> +  /* Do nothing if we already have an executable filename.  */
> +  if (get_exec_file (0) != NULL)
> +    return;
> +
> +  /* Try to determine a filename from the process itself.  */
> +  exec_file_target = target_pid_to_exec_file (pid);
> +  if (exec_file_target == NULL)
>      {
> -      if (!exception_print_same (prev_err, err))
> -	warning ("%s", err.message);
> +      warning (_("No executable has been specified and target does not "
> +		 "support\n"
> +		 "determining executable automatically.  "
> +		 "Try using the \"file\" command."));
> +      return;
>      }
> -  END_CATCH
> -  current_inferior ()->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;
>  
> +  exec_file_host = exec_file_find (exec_file_target, NULL);
> +  old_chain = make_cleanup (xfree, exec_file_host);
> +
> +  /* Attempt to open the exec file.  */
> +  maybe_open_exec_file (exec_file_host, current_inferior (), 1,
> +			defer_bp_reset, from_tty);
>    do_cleanups (old_chain);
>  }
>  
> diff --git a/gdb/exec.h b/gdb/exec.h
> index 4044cb7..e893fd2 100644
> --- a/gdb/exec.h
> +++ b/gdb/exec.h
> @@ -113,4 +113,14 @@ extern void print_section_info (struct target_section_table *table,
>  
>  extern void exec_close (void);
>  
> +/* Helper function that attempts to open the symbol file at EXEC_FILE_HOST.
> +   If successful, it proceeds to add the symbol file as the main symbol file
> +   if MAIN_SYMBOL_FILE is 1.  Otherwise it attempts to add it as a regular
> +   non-main symbol file.

This is not correct.  The file is always added as a main symbol file.

> +
> +   DEFER_BP_RESET and FROM_TTY are passed on to functions called from within
> +   this helper function.  */
> +extern void maybe_open_exec_file (const char *exec_file_host,
> +				  struct inferior *inf, int main_symbol_file,
> +				  int defer_bp_reset, int from_tty);
>  #endif
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 00bba16..7d89e8e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1077,15 +1077,17 @@ show_follow_exec_mode_string (struct ui_file *file, int from_tty,
>    fprintf_filtered (file, _("Follow exec mode is \"%s\".\n"),  value);
>  }
>  
> -/* EXECD_PATHNAME is assumed to be non-NULL.  */
> +/* EXEC_FILE_TARGET is assumed to be non-NULL.  */
>  
>  static void
> -follow_exec (ptid_t ptid, char *execd_pathname)
> +follow_exec (ptid_t ptid, char *exec_file_target)
>  {
>    struct thread_info *th, *tmp;
>    struct inferior *inf = current_inferior ();
>    int pid = ptid_get_pid (ptid);
>    ptid_t process_ptid;
> +  char *exec_file_host = NULL;

No need for NULL initialization.

> +  struct cleanup *old_chain;
>  
>    /* This is an exec event that we actually wish to pay attention to.
>       Refresh our symbol table to the newly exec'd program, remove any
> @@ -1155,7 +1157,7 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>    process_ptid = pid_to_ptid (pid);
>    printf_unfiltered (_("%s is executing new program: %s\n"),
>  		     target_pid_to_str (process_ptid),
> -		     execd_pathname);
> +		     exec_file_target);
>  
>    /* We've followed the inferior through an exec.  Therefore, the
>       inferior has essentially been killed & reborn.  */
> @@ -1164,14 +1166,17 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>    breakpoint_init_inferior (inf_execd);
>  
> -  if (*gdb_sysroot != '\0')
> -    {
> -      char *name = exec_file_find (execd_pathname, NULL);
> +  exec_file_host = exec_file_find (exec_file_target, NULL);
> +  old_chain = make_cleanup (xfree, exec_file_host);
>  
> -      execd_pathname = (char *) alloca (strlen (name) + 1);
> -      strcpy (execd_pathname, name);
> -      xfree (name);
> -    }
> +  /* If we were unable to map the executable target pathname onto a host
> +     pathname, tell the user that.  Otherwise GDB's subsequent behavior
> +     is confusing.  Maybe it would even be better to stop at this point
> +     so that the user can specify a file manually before continuing.  */
> +  if (exec_file_host == NULL)
> +    warning (_("Could not load symbols for executable %s.\n"
> +	       "Do you need \"set sysroot\"?"),
> +	     exec_file_target);
>  
>    /* Reset the shared library package.  This ensures that we get a
>       shlib event when the child reaches "_start", at which point the
> @@ -1193,7 +1198,7 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>        inf = add_inferior_with_spaces ();
>        inf->pid = pid;
> -      target_follow_exec (inf, execd_pathname);
> +      target_follow_exec (inf, exec_file_target);
>  
>        set_current_inferior (inf);
>        set_current_program_space (inf->pspace);
> @@ -1212,22 +1217,16 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>    gdb_assert (current_program_space == inf->pspace);
>  
> -  /* That a.out is now the one to use.  */
> -  exec_file_attach (execd_pathname, 0);
> +  /* Attempt to open the exec file.  */
> +  maybe_open_exec_file (exec_file_host, inf, 0, 0, 0);
> +
> +  do_cleanups (old_chain);
>  
>    /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE
>       (Position Independent Executable) main symbol file will get applied by
>       solib_create_inferior_hook below.  breakpoint_re_set would fail to insert
>       the breakpoints with the zero displacement.  */

This comment needs to move somewhere.  Here it will feel very lonely
and out of place.  :-)


>  
> -  symbol_file_add (execd_pathname,
> -		   (inf->symfile_flags
> -		    | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET),
> -		   NULL, 0);
> -
> -  if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> -    set_initial_language ();
> -
>    /* If the target can specify a description, read it.  Must do this
>       after flipping to the new executable (because the target supplied
>       description must be compatible with the executable's
> diff --git a/gdb/solib.c b/gdb/solib.c
> index b8c2b42..db0ef02 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -380,21 +380,22 @@ solib_find_1 (char *in_pathname, int *fd, int is_solib)
>  /* Return the full pathname of the main executable, or NULL if not
>     found.  The returned pathname is malloc'ed and must be freed by
>     the caller.  If FD is non-NULL, *FD is set to either -1 or an open
> -   file handle for the main executable.
> -
> -   The search algorithm used is described in solib_find_1's comment
> -   above.  */
> +   file handle for the main executable.  */
>  
>  char *
>  exec_file_find (char *in_pathname, int *fd)
>  {
> -  char *result = solib_find_1 (in_pathname, fd, 0);
> +  char *result;
> +  const char *fskind = effective_target_file_system_kind ();
> +
> +  if (!in_pathname)

in_pathname == NULL

> +    return NULL;

> +
> +# This test exercises PR20569.  GDB crashes when attempting to follow
> +# an exec call when it cannot resolve the path to the symbol file.
> +# This is the case when an invalid sysroot is provided.

I think it'd be better to write this in the past tense.  GDB no longer
crashes after the fix.  would crash...could not...was... etc.

> +
> +standard_testfile foll-exec.c
> +
> +global binfile
> +set binfile [standard_output_file "foll-exec"]
> +set testfile2 "execd-prog"
> +set srcfile2 ${testfile2}.c
> +set binfile2 [standard_output_file ${testfile2}]
> +
> +set compile_options debug
> +
> +# build the first test case
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable $compile_options] != "" } {
> +    untested foll-exec.exp

Wrong .exp name.  But:
  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls

> +    return -1
> +}
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_options] != "" } {
> +    untested foll-exec.exp

Ditto.

> +    return -1
> +}
> +
> +proc do_exec_sysroot_test {} {
> +    global binfile srcfile srcfile2 testfile testfile2
> +    global gdb_prompt
> +
> +    # Start the program running, and stop at main.
> +    #
> +    if ![runto_main] then {
> +	fail "Couldn't run ${testfile}"
> +	return
> +    }
> +
> +    # Verify that the system supports "catch exec".
> +    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
> +    set test "continue to first exec catchpoint"

s/first// while at it.  There's only one.

> +    gdb_test_multiple "continue" $test {
> +	-re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
> +	    unsupported $test
> +	    return
> +	}
> +	-re ".*Could not load symbols for executable.*$gdb_prompt $" {
> +	    pass $test
> +	}
> +    }
> +}
> +
> +# Start with a fresh gdb
> +clean_restart $binfile
> +gdb_test_no_output "set sysroot /a/path/that/does/not/exist"

Odd to put this here instead of inside the sysroot_test function, IMO.

> +do_exec_sysroot_test

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]