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

[Luis Machado <lgustavo@codesourcery.com>]

On 10/25/2016 01:04 PM, Pedro Alves wrote:
> 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.

Before i go ahead and adjust this even more, what's your plan and ETA 
for the above? This is disturbing more code as we try to consolidade 
slightly different functions into a single one in order to make things a 
bit more clean. But i'm afraid this is besides the point of the original 
patch itself?

I just want to understand what's the end goal, because the scope seems 
to be changing slightly with each iteration. :-)

Thanks,
Luis