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

[Pedro Alves <palves@redhat.com>]

On 10/25/2016 06:12 PM, Luis Machado wrote:
> Adjusted a few more bits.
> 
> I ended up using the ugly explicit cast for an enum that is being used as a
> bitfield. I'm guessing we'll want to convert this to the macro-fied
> template-fied version that is more appropriate to C++?

Yup.  I actually have a patch for that in one of my C++ branches
somewhere.

> 
> If so then i can come up with a follow up patch that does the conversion. I
> didn't want to disrupt v4 with more unrelated changes.

Use 'int' in this patch like the rest of the functions that pass
around this enum, and remove the casts.  Then when the enum
is made a proper enum-flags with DEF_ENUM_FLAGS_TYPE, you won't
have to remove the casts then, and all functions that need adjustment
will get the same adjustment at the same time.  No need for a partial
transition.

> 

>  void
> -exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
> +try_open_exec_file (const char *exec_file_host, struct inferior *inf,
> +		    enum symfile_add_flags flags, int from_tty)

Use int.

>  {
> -  char *exec_file, *full_exec_path = NULL;
>    struct cleanup *old_chain;
>    struct gdb_exception prev_err = exception_none;
>  
> -  /* Do nothing if we already have an executable filename.  */
> -  exec_file = (char *) get_exec_file (0);
> -  if (exec_file != NULL)
> -    return;
> -
> -  /* Try to determine a filename from the process itself.  */
> -  exec_file = target_pid_to_exec_file (pid);
> -  if (exec_file == NULL)
> -    {
> -      warning (_("No executable has been specified and target does not "
> -		 "support\n"
> -		 "determining executable automatically.  "
> -		 "Try using the \"file\" command."));
> -      return;
> -    }
> -
> -  /* If gdb_sysroot is not empty and the discovered filename
> -     is absolute then prefix the filename with gdb_sysroot.  */
> -  if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file))
> -    {
> -      full_exec_path = exec_file_find (exec_file, NULL);
> -      if (full_exec_path == NULL)
> -	return;
> -    }
> -  else
> -    {
> -      /* It's possible we don't have a full path, but rather just a
> -	 filename.  Some targets, such as HP-UX, don't provide the
> -	 full path, sigh.
> -
> -	 Attempt to qualify the filename against the source path.
> -	 (If that fails, we'll just fall back on the original
> -	 filename.  Not much more we can do...)  */
> -      if (!source_full_path_of (exec_file, &full_exec_path))
> -	full_exec_path = xstrdup (exec_file);
> -    }
> -
> -  old_chain = make_cleanup (xfree, full_exec_path);
> -  make_cleanup (free_current_contents, &prev_err.message);
> +  old_chain = make_cleanup (free_current_contents, &prev_err.message);
>  
>    /* exec_file_attach and symbol_file_add_main may throw an error if the file
>       cannot be opened either locally or remotely.
> @@ -217,7 +160,9 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
>       errors/exceptions in the following code.  */
>    TRY
>      {
> -      exec_file_attach (full_exec_path, from_tty);
> +      /* We must do this step even if exec_file_host is NULL, so that
> +	 exec_file_attach will clear state.  */
> +      exec_file_attach (exec_file_host, from_tty);
>      }
>    CATCH (err, RETURN_MASK_ERROR)
>      {
> @@ -232,20 +177,64 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
>      }
>    END_CATCH
>  
> -  TRY
> +  if (exec_file_host != NULL)
>      {
> -      if (defer_bp_reset)
> -	current_inferior ()->symfile_flags |= SYMFILE_DEFER_BP_RESET;
> -      symbol_file_add_main (full_exec_path, from_tty);
> +      /* 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.  */
> +      TRY

I don't understand how it makes sense to this comment _here_.
Move this to where the flag is first passed down.  Note how there's
no "solib_create_inferior_hook below" at all here.  This function
just passes down the "flags" argument unmodified.

> +	{
> +	  symbol_file_add_main_1 (exec_file_host, from_tty, flags);
> +
> +	  if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> +	    set_initial_language ();

This is already done by symbol_file_add_main_1..

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

Before the patch, inf->symfile_flags is enabled/disabled
around the body of the code.  But now you pass down
SYMFILE_DEFER_BP_RESET in 'flags'.  I think we should
be able to remove this?

>      }
> -  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;
> +  struct cleanup *old_chain;
> +  symfile_add_flags flags = SYMFILE_MAINLINE;

Use int for now.  Start with 0.  try_open_exec_file
always adds the SYMFILE_MAINLINE for you.

> +
> +  /* 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);
> +
> +  if (defer_bp_reset)
> +    flags = (enum symfile_add_flags) (flags | SYMFILE_DEFER_BP_RESET);

Now with int, you can write:

    flags |= SYMFILE_DEFER_BP_RESET;

>    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.  */
> +  try_open_exec_file (exec_file_host, inf, (enum symfile_add_flags) 0, 0);

Hmm, doesn't thi lose the SYMFILE_DEFER_BP_RESET described ...

>  
> -  /* 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.  */
> -
> -  symbol_file_add (execd_pathname,
> -		   (inf->symfile_flags
> -		    | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET),
> -		   NULL, 0);

... here, in the code that you're replacing?

Seems to be that you should leave the comment here, and do:

  /* 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.  */
  try_open_exec_file (exec_file_host, inf, SYMFILE_DEFER_BP_RESET, 0);

Here is where it makes sense to talk about the solib_create_inferior_hook
call below, because there's actually such a call below.

> -
> -  if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> -    set_initial_language ();
> +  do_cleanups (old_chain);
>  
>    /* If the target can specify a description, read it.  Must do this
>       after flipping to the new executable (because the target supplied

BTW, symbol_file_add_main_1 doesn't actually add "flags" to
its add_flags local, so it seems like we're actually losing 
the passed down SYMFILE_DEFER_BP_RESET?  Or is there some confusion
regarding symfile_add_flags vs objfile flags here?  Oh looks
like there is...

I think I'll go dig up my enum-flags patch, and that will I think
help clear this up a lot.

Thanks,
Pedro Alves