[5/9] Rearrange symfile_bfd_open
Commit Message
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(-)
Comments
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
@@ -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))
{