[5/9] Rearrange symfile_bfd_open

  symfile_bfd_open handles what were remote files as a special case.
Now that remote files are replaced with target the reverse is true
and the BFD opening, format checking and error printing code is
essentially duplicated.  This commit rearranges symfile_bfd_open
to treat local files as a special case, removing the duplication.

gdb/ChangeLog:

	* symfile.c (symfile_bfd_open): Reorder to remove duplicated
	checks and error messages.
---
 gdb/ChangeLog |    5 ++++
 gdb/symfile.c |   69 ++++++++++++++++++++++++--------------------------------
 2 files changed, 35 insertions(+), 39 deletions(-)
  

On 03/20/2015 04:48 PM, Gary Benson wrote:
> symfile_bfd_open handles what were remote files as a special case.
> Now that remote files are replaced with target the reverse is true
> and the BFD opening, format checking and error printing code is
> essentially duplicated.  This commit rearranges symfile_bfd_open
> to treat local files as a special case, removing the duplication.

Where were they already done?  Can you add a comment?

> 
> gdb/ChangeLog:
> 
> 	* symfile.c (symfile_bfd_open): Reorder to remove duplicated
> 	checks and error messages.
> ---
>  gdb/ChangeLog |    5 ++++
>  gdb/symfile.c |   69 ++++++++++++++++++++++++--------------------------------
>  2 files changed, 35 insertions(+), 39 deletions(-)
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 0d8dae7..0c35ffa 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1718,60 +1718,51 @@ set_initial_language (void)
>     absolute).  In case of trouble, error() is called.  */
>  
>  bfd *
> -symfile_bfd_open (const char *cname)
> +symfile_bfd_open (const char *name)
>  {
>    bfd *sym_bfd;
> -  int desc;
> -  char *name, *absolute_name;
> -  struct cleanup *back_to;
> +  int desc = -1;
> +  struct cleanup *back_to = make_cleanup (null_cleanup, 0);
>  
> -  if (is_target_filename (cname))
> +  if (!is_target_filename (name))
>      {
> -      sym_bfd = gdb_bfd_open (cname, gnutarget, -1);
> -      if (!sym_bfd)
> -	error (_("`%s': can't open to read symbols: %s."), cname,
> -	       bfd_errmsg (bfd_get_error ()));
> -
> -      if (!bfd_check_format (sym_bfd, bfd_object))
> -	{
> -	  make_cleanup_bfd_unref (sym_bfd);
> -	  error (_("`%s': can't read symbols: %s."), cname,
> -		 bfd_errmsg (bfd_get_error ()));
> -	}
> +      char *expanded_name, *absolute_name;
>  
> -      return sym_bfd;
> -    }
> -
> -  name = tilde_expand (cname);	/* Returns 1st new malloc'd copy.  */
> +      expanded_name = tilde_expand (name); /* Returns 1st new malloc'd copy.  */
>  
> -  /* Look down path for it, allocate 2nd new malloc'd copy.  */
> -  desc = openp (getenv ("PATH"), OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, name,
> -		O_RDONLY | O_BINARY, &absolute_name);
> +      /* Look down path for it, allocate 2nd new malloc'd copy.  */
> +      desc = openp (getenv ("PATH"),
> +		    OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH,
> +		    expanded_name, O_RDONLY | O_BINARY, &absolute_name);
>  #if defined(__GO32__) || defined(_WIN32) || defined (__CYGWIN__)
> -  if (desc < 0)
> -    {
> -      char *exename = alloca (strlen (name) + 5);
> +      if (desc < 0)
> +	{
> +	  char *exename = alloca (strlen (expanded_name) + 5);
>  
> -      strcat (strcpy (exename, name), ".exe");
> -      desc = openp (getenv ("PATH"), OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH,
> -		    exename, O_RDONLY | O_BINARY, &absolute_name);
> -    }
> +	  strcat (strcpy (exename, expanded_name), ".exe");
> +	  desc = openp (getenv ("PATH"),
> +			OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH,
> +			exename, O_RDONLY | O_BINARY, &absolute_name);
> +	}
>  #endif
> -  if (desc < 0)
> -    {
> -      make_cleanup (xfree, name);
> -      perror_with_name (name);
> -    }
> +      if (desc < 0)
> +	{
> +	  make_cleanup (xfree, expanded_name);
> +	  perror_with_name (expanded_name);
> +	}
>  
> -  xfree (name);
> -  name = absolute_name;
> -  back_to = make_cleanup (xfree, name);
> +      xfree (expanded_name);
> +      make_cleanup (xfree, absolute_name);
> +      name = absolute_name;
> +    }
>  
>    sym_bfd = gdb_bfd_open (name, gnutarget, desc);
>    if (!sym_bfd)
>      error (_("`%s': can't open to read symbols: %s."), name,
>  	   bfd_errmsg (bfd_get_error ()));
> -  bfd_set_cacheable (sym_bfd, 1);
> +
> +  if (!gdb_bfd_has_target_filename (sym_bfd))
> +    bfd_set_cacheable (sym_bfd, 1);
>  
>    if (!bfd_check_format (sym_bfd, bfd_object))
>      {
> 


Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 03/20/2015 04:48 PM, Gary Benson wrote:
> > symfile_bfd_open handles what were remote files as a special case.
> > Now that remote files are replaced with target the reverse is true
> > and the BFD opening, format checking and error printing code is
> > essentially duplicated.  This commit rearranges symfile_bfd_open
> > to treat local files as a special case, removing the duplication.
> 
> Where were they already done?  Can you add a comment?

They were done twice in the same function:

  if remote:
    open bfd, check format, etc
    return
  local-specific stuff
  open bfd, check format, etc
  return

I rearranged it to be like this:

  if local:
      local-specific stuff
  open bfd, check format, etc
  return

How about I change the commit message to this:

  symfile_bfd_open handled what were remote files as a special case.
  Converting from "remote:" files to "target:" means the two cases
  are now very similar and may be merged.  This commit rearranges
  symfile_bfd_open to remove the duplicated code.

?

Cheers,
Gary
  

On 04/01/2015 04:50 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 03/20/2015 04:48 PM, Gary Benson wrote:
>>> symfile_bfd_open handles what were remote files as a special case.
>>> Now that remote files are replaced with target the reverse is true
>>> and the BFD opening, format checking and error printing code is
>>> essentially duplicated.  This commit rearranges symfile_bfd_open
>>> to treat local files as a special case, removing the duplication.
>>
>> Where were they already done?  Can you add a comment?
> 
> They were done twice in the same function:
> 
>   if remote:
>     open bfd, check format, etc
>     return
>   local-specific stuff
>   open bfd, check format, etc
>   return
> 
> I rearranged it to be like this:
> 
>   if local:
>       local-specific stuff
>   open bfd, check format, etc
>   return
> 
> How about I change the commit message to this:
> 
>   symfile_bfd_open handled what were remote files as a special case.
>   Converting from "remote:" files to "target:" means the two cases
>   are now very similar and may be merged.  This commit rearranges
>   symfile_bfd_open to remove the duplicated code.

Ah, I see now.  The pseudo code above helped me a lot too.  I'd
suggest putting in the commit log too.

Patch is OK.

Thanks,
Pedro Alves