This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 1/3] Refactor/simplify (+fix) svr4_current_sos
On Thu, 06 Oct 2011 20:30:15 +0200, Pedro Alves wrote:
> On Monday 03 October 2011 22:54:24, Jan Kratochvil wrote:
> > gdb/
> > 2011-10-03 Jan Kratochvil <jan.kratochvil@redhat.com>
> > Paul Pluzhnikov <ppluzhnikov@google.com>
> >
> > * defs.h (struct so_list): New forward declaration.
> > (make_cleanup_free_so): New declaration.
> > * solib-svr4.c (ignore_first_link_map_entry): Remove.
> > (svr4_free_so): Move here. Handle NULL so->lm_info.
>
> Move what here? Looks like a stale comment.
svr4_free_so is moved upwards in this code, as it is used earlier.
>From what I know about the coding style for GDB it is preferred to move
functions rather than to create new forward declarations for them.
Also ChangeLog enties should be present in the same order as chunks of the
patch.
Which all leads to this statement. Used now:
(svr4_free_so): Move the function here from downwards. Handle NULL
so->lm_info.
> > @@ -1029,58 +1028,93 @@ svr4_current_sos (void)
> > SVR4, it has no name. For others (Solaris 2.3 for example), it
> > does have a name, so we can no longer use a missing name to
> > decide when to ignore it. */
> > - else if (ignore_first_link_map_entry (new) && ldsomap == 0)
> > + if (ignore_first && lm_prev (new) == 0)
> > {
> > + struct svr4_info *info = get_svr4_info ();
> > +
> > info->main_lm_addr = new->lm_info->lm_addr;
>
> A shame that this is still hidden here IMO, and dependent on
> how you call the function, though not documented in the function
> header. An alternative would be to move this "ignore first" logic to
> the caller, making the caller itself delete the first entry in the list
> if it wanted to, and setting main_lm_addr. We already have to allocate/free
> this so_list, so nothing seems be to lost that way.
While the idea is good the implementation has some issues, it also adds LoCs:
gdb/solib-svr4.c | 81 ++++++++++++++++++++++++++++++------------------------
1 files changed, 45 insertions(+), 36 deletions(-)
"For some versions of SVR4, it has no name." - I am not sure if it means the
name is "" (empty) or that the name is unreadable and thus it would print
false warning messages (while trying to create the list entry going to be
freed by the caller).
The caller will need to reiterate the list and free entries without names,
which is more complicated than in the callee.
Anyway I do not have an opinion on it, if you like it better this way.
No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.
Thanks,
Jan
gdb/
2011-10-13 Jan Kratochvil <jan.kratochvil@redhat.com>
Paul Pluzhnikov <ppluzhnikov@google.com>
* defs.h (struct so_list): New forward declaration.
(make_cleanup_free_so): New declaration.
* solib-svr4.c (ignore_first_link_map_entry): Remove.
(svr4_free_so): Move the function here from downwards. Handle NULL
so->lm_info.
(svr4_free_library_list): New.
(svr4_read_so_list): New, moved here code from svr4_current_sos.
Use more cleanups.
(svr4_current_sos): New variable. New variable back_to, use it for
svr4_free_library_list protection. Move filtering out anonymous
entries into a postprocessing pass.
(svr4_free_so): Remove - move upwards.
* utils.c: Include solist.h.
(do_free_so, make_cleanup_free_so): New functions.
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -375,6 +375,9 @@ extern struct cleanup *
extern struct cleanup *make_cleanup_value_free_to_mark (struct value *);
extern struct cleanup *make_cleanup_value_free (struct value *);
+struct so_list;
+extern struct cleanup *make_cleanup_free_so (struct so_list *so);
+
extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
extern struct cleanup *make_my_cleanup (struct cleanup **,
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -302,17 +302,6 @@ lm_name (struct so_list *so)
ptr_type);
}
-static int
-ignore_first_link_map_entry (struct so_list *so)
-{
- /* Assume that everything is a library if the dynamic loader was loaded
- late by a static executable. */
- if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
- return 0;
-
- return lm_prev (so) == 0;
-}
-
/* Per pspace SVR4 specific data. */
struct svr4_info
@@ -942,6 +931,32 @@ open_symbol_file_object (void *from_ttyp)
return 1;
}
+/* Implementation for target_so_ops.free_so. */
+
+static void
+svr4_free_so (struct so_list *so)
+{
+ if (so->lm_info)
+ xfree (so->lm_info->lm);
+ xfree (so->lm_info);
+}
+
+/* Free so_list built so far (called via cleanup). */
+
+static void
+svr4_free_library_list (void *p_list)
+{
+ struct so_list *list = *(struct so_list **) p_list;
+
+ while (list != NULL)
+ {
+ struct so_list *next = list->next;
+
+ svr4_free_so (list);
+ list = next;
+ }
+}
+
/* If no shared library information is available from the dynamic
linker, build a fallback list from other sources. */
@@ -971,16 +986,74 @@ svr4_default_sos (void)
return new;
}
+/* Read the whole inferior libraries chain starting at address LM. Add the
+ entries to the tail referenced by LINK_PTR_PTR. */
+
+static void
+svr4_read_so_list (CORE_ADDR lm, struct so_list ***link_ptr_ptr)
+{
+ CORE_ADDR prev_lm = 0, next_lm;
+
+ for (; lm != 0; prev_lm = lm, lm = next_lm)
+ {
+ struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
+ struct so_list *new;
+ struct cleanup *old_chain;
+ int errcode;
+ char *buffer;
+
+ new = XZALLOC (struct so_list);
+ old_chain = make_cleanup_free_so (new);
+
+ new->lm_info = xmalloc (sizeof (struct lm_info));
+ new->lm_info->l_addr = (CORE_ADDR) -1;
+ new->lm_info->lm_addr = lm;
+ new->lm_info->lm = xzalloc (lmo->link_map_size);
+
+ read_memory (lm, new->lm_info->lm, lmo->link_map_size);
+
+ next_lm = lm_next (new);
+
+ if (lm_prev (new) != prev_lm)
+ {
+ warning (_("Corrupted shared library list"));
+ do_cleanups (old_chain);
+ break;
+ }
+
+ /* Extract this shared object's name. */
+ target_read_string (lm_name (new), &buffer,
+ SO_NAME_MAX_PATH_SIZE - 1, &errcode);
+ if (errcode != 0)
+ {
+ warning (_("Can't read pathname for load map: %s."),
+ safe_strerror (errcode));
+ do_cleanups (old_chain);
+ continue;
+ }
+
+ strncpy (new->so_name, buffer, SO_NAME_MAX_PATH_SIZE - 1);
+ new->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
+ strcpy (new->so_original_name, new->so_name);
+ xfree (buffer);
+
+ discard_cleanups (old_chain);
+ new->next = 0;
+ **link_ptr_ptr = new;
+ *link_ptr_ptr = &new->next;
+ }
+}
+
/* Implement the "current_sos" target_so_ops method. */
static struct so_list *
svr4_current_sos (void)
{
- CORE_ADDR lm, prev_lm;
- struct so_list *head = 0;
+ CORE_ADDR lm;
+ struct so_list *head = NULL;
struct so_list **link_ptr = &head;
- CORE_ADDR ldsomap = 0;
struct svr4_info *info;
+ struct cleanup *back_to;
info = get_svr4_info ();
@@ -993,95 +1066,65 @@ svr4_current_sos (void)
if (! info->debug_base)
return svr4_default_sos ();
+ back_to = make_cleanup (svr4_free_library_list, &head);
+
/* Walk the inferior's link map list, and build our list of
`struct so_list' nodes. */
- prev_lm = 0;
lm = solib_svr4_r_map (info);
+ if (lm)
+ svr4_read_so_list (lm, &link_ptr);
- while (lm)
+ /* Assume that everything is a library if the dynamic loader was loaded
+ late by a static executable. */
+ if (head && (exec_bfd == NULL
+ || bfd_get_section_by_name (exec_bfd, ".dynamic") != NULL))
{
- struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
- struct so_list *new = XZALLOC (struct so_list);
- struct cleanup *old_chain = make_cleanup (xfree, new);
- CORE_ADDR next_lm;
-
- new->lm_info = xmalloc (sizeof (struct lm_info));
- make_cleanup (xfree, new->lm_info);
-
- new->lm_info->l_addr = (CORE_ADDR)-1;
- new->lm_info->lm_addr = lm;
- new->lm_info->lm = xzalloc (lmo->link_map_size);
- make_cleanup (xfree, new->lm_info->lm);
-
- read_memory (lm, new->lm_info->lm, lmo->link_map_size);
-
- next_lm = lm_next (new);
-
- if (lm_prev (new) != prev_lm)
- {
- warning (_("Corrupted shared library list"));
- free_so (new);
- next_lm = 0;
- }
-
/* For SVR4 versions, the first entry in the link map is for the
inferior executable, so we must ignore it. For some versions of
SVR4, it has no name. For others (Solaris 2.3 for example), it
does have a name, so we can no longer use a missing name to
decide when to ignore it. */
- else if (ignore_first_link_map_entry (new) && ldsomap == 0)
- {
- info->main_lm_addr = new->lm_info->lm_addr;
- free_so (new);
- }
- else
- {
- int errcode;
- char *buffer;
-
- /* Extract this shared object's name. */
- target_read_string (lm_name (new), &buffer,
- SO_NAME_MAX_PATH_SIZE - 1, &errcode);
- if (errcode != 0)
- warning (_("Can't read pathname for load map: %s."),
- safe_strerror (errcode));
- else
- {
- strncpy (new->so_name, buffer, SO_NAME_MAX_PATH_SIZE - 1);
- new->so_name[SO_NAME_MAX_PATH_SIZE - 1] = '\0';
- strcpy (new->so_original_name, new->so_name);
- }
- xfree (buffer);
- /* If this entry has no name, or its name matches the name
- for the main executable, don't include it in the list. */
- if (! new->so_name[0]
- || match_main (new->so_name))
- free_so (new);
- else
- {
- new->next = 0;
- *link_ptr = new;
- link_ptr = &new->next;
- }
- }
+ struct svr4_info *info = get_svr4_info ();
+ struct so_list *first = head;
+
+ info->main_lm_addr = first->lm_info->lm_addr;
- prev_lm = lm;
- lm = next_lm;
+ head = first->next;
+ if (head == NULL)
+ link_ptr = &head;
+ svr4_free_so (first);
+ }
+
+ /* On Solaris, the dynamic linker is not in the normal list of
+ shared objects, so make sure we pick it up too. Having
+ symbol information for the dynamic linker is quite crucial
+ for skipping dynamic linker resolver code. */
+ lm = solib_svr4_r_ldsomap (info);
+ if (lm)
+ svr4_read_so_list (lm, &link_ptr);
+
+ /* If this entry has no name, or its name matches the name for the main
+ executable, don't include it in the list. Delay this filtering only after
+ the first entry has been recognized and possibly removed above. */
+ link_ptr = &head;
+ while (*link_ptr)
+ {
+ struct so_list *so = *link_ptr;
- /* On Solaris, the dynamic linker is not in the normal list of
- shared objects, so make sure we pick it up too. Having
- symbol information for the dynamic linker is quite crucial
- for skipping dynamic linker resolver code. */
- if (lm == 0 && ldsomap == 0)
+ if (so->so_name[0] != 0 && !match_main (so->so_name))
+ link_ptr = &so->next;
+ else
{
- lm = ldsomap = solib_svr4_r_ldsomap (info);
- prev_lm = 0;
- }
+ /* Free this SO entry. */
- discard_cleanups (old_chain);
+ *link_ptr = so->next;
+ svr4_free_so (so);
+ }
}
+ discard_cleanups (back_to);
+
if (head == NULL)
return svr4_default_sos ();
@@ -2048,14 +2091,6 @@ svr4_clear_solib (void)
info->debug_loader_name = NULL;
}
-static void
-svr4_free_so (struct so_list *so)
-{
- xfree (so->lm_info->lm);
- xfree (so->lm_info);
-}
-
-
/* Clear any bits of ADDR that wouldn't fit in a target-format
data pointer. "Data pointer" here refers to whatever sort of
address the dynamic linker uses to manage its sections. At the
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -60,6 +60,7 @@
#include "gdbcore.h"
#include "top.h"
#include "main.h"
+#include "solist.h"
#include "inferior.h" /* for signed_pointer_to_address */
@@ -495,6 +496,24 @@ make_cleanup_value_free (struct value *value)
return make_my_cleanup (&cleanup_chain, do_value_free, value);
}
+/* Helper for make_cleanup_free_so. */
+
+static void
+do_free_so (void *arg)
+{
+ struct so_list *so = arg;
+
+ free_so (so);
+}
+
+/* Make cleanup handler calling free_so for SO. */
+
+struct cleanup *
+make_cleanup_free_so (struct so_list *so)
+{
+ return make_my_cleanup (&cleanup_chain, do_free_so, so);
+}
+
struct cleanup *
make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
void *arg, void (*free_arg) (void *))