[PATCH 0/2] Improved variable object invalidation in GDB

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Improved variable object invalidation in GDB

Taimoor Mirza-2
Hi,
This patch series improves variable object invalidation in GDB.
This patch has been regression tested on ppc-gnu-linux
and i686 targets using both simulator and real boards.

Taimoor Mirza (2):
  Fix varobj updation after symbol removal
  Testsuite for varobj updation after symbol removal

 gdb/objfiles.c                             |   19 ++
 gdb/testsuite/gdb.mi/mi-var-invalidate.exp |   67 ++++++
 gdb/testsuite/gdb.mi/sym-file-lib.c        |   26 ++
 gdb/testsuite/gdb.mi/sym-file-loader.c     |  353 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/sym-file-loader.h     |   99 ++++++++
 gdb/testsuite/gdb.mi/sym-file-main.c       |   84 +++++++
 gdb/varobj.c                               |   37 +++
 gdb/varobj.h                               |    3 +
 8 files changed, 688 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-lib.c
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-loader.c
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-loader.h
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-main.c

--
1.7.9.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Fix varobj updation after symbol removal

Taimoor Mirza-2
This problem was observed while loading and unloading symbols using
add-symbol-file and remove-symbol-file. When remove-symbol-file
command is invoked, it calls clear_symtab_users that calls varobj_invalidate
to invalidate variable objects. This function invalidates the varobjs
that are tied to locals and re-create the ones that are defined on
globals. During this re-creation of globals, variable objects are
re-evaluated that can result in new value. But this change is not recorded
and because of this, -var-update for such modified variable objects
gives empty change list.

Proposed Fix:
=============
GDB has mechanism of marking varobj's updated if they are set via
varobj_set_value operation. This allows any next -var-update to report
this change. Same approach should be used during varobj invalidation.
If value of newly created varobj is different from previous one, mark it
updated so that -var-update can get this change.

Variable object invalidation code is cleaned up to avoid using pointers
whose target has been already freed.

Fix Testing:
===========
This fix has been regression tested on both simulator and real boards.

2015-04-15  Taimoor Mirza  <[hidden email]>
        Maciej W. Rozycki  <[hidden email]>

        gdb/
        * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
        prototypes.
        * varobj.c (varobj_is_valid_p, varobj_set_invalid): New functions.
        (varobj_invalidate_iter): Mark re-created global object updated
        if its value is different from previous value.
        * objfiles.c (invalidate_objfile_varobj_type_iter): New function.
        (free_objfile): Call it.

Signed-off-by: Taimoor Mirza <[hidden email]>
---
 gdb/objfiles.c |   19 +++++++++++++++++++
 gdb/varobj.c   |   37 +++++++++++++++++++++++++++++++++++++
 gdb/varobj.h   |    3 +++
 3 files changed, 59 insertions(+)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index ff20bc8..5905998 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -32,6 +32,7 @@
 #include "bcache.h"
 #include "expression.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -533,6 +534,21 @@ free_objfile_separate_debug (struct objfile *objfile)
     }
 }
 
+/* Mark the variable object VAR invalid if built upon a type coming from
+   the objfile requested, passed as DATA.  Also clear the type reference.  */
+
+static void
+invalidate_objfile_varobj_type_iter (struct varobj *var, void *data)
+{
+  struct objfile *objfile = data;
+
+  if (varobj_is_valid_p (var) && TYPE_OBJFILE (var->type) == objfile)
+    {
+      varobj_set_invalid (var);
+      var->type = NULL;
+    }
+}
+
 /* Destroy an objfile and all the symtabs and psymtabs under it.  */
 
 void
@@ -579,6 +595,9 @@ free_objfile (struct objfile *objfile)
      lists.  */
   preserve_values (objfile);
 
+  /* Varobj may refer to types stored in objfile's obstack.  */
+  all_root_varobjs (invalidate_objfile_varobj_type_iter, objfile);
+
   /* It still may reference data modules have associated with the objfile and
      the symbol file data.  */
   forget_cached_source_info_for_objfile (objfile);
diff --git a/gdb/varobj.c b/gdb/varobj.c
index b220fd8..4860a37 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2699,6 +2699,22 @@ varobj_floating_p (const struct varobj *var)
   return var->root->floating;
 }
 
+/* Get the valid flag of varobj VAR.  */
+
+int
+varobj_is_valid_p (struct varobj *var)
+{
+  return var->root->is_valid;
+}
+
+/* Clear the valid flag on varobj VAR.  */
+
+void
+varobj_set_invalid (struct varobj *var)
+{
+  var->root->is_valid = 0;
+}
+
 /* Implement the "value_is_changeable_p" varobj callback for most
    languages.  */
 
@@ -2760,6 +2776,7 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
   if (var->root->floating || var->root->valid_block == NULL)
     {
       struct varobj *tmp_var;
+      char *tmp_var_value, *var_value;
 
       /* Try to create a varobj with same expression.  If we succeed
  replace the old varobj, otherwise invalidate it.  */
@@ -2768,6 +2785,26 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
       if (tmp_var != NULL)
  {
   tmp_var->obj_name = xstrdup (var->obj_name);
+  tmp_var_value = varobj_get_value (tmp_var);
+  var_value = varobj_get_value (var);
+
+  /* As varobjs are re-evaluated during creation so there is a
+     chance that new value is different from old one.  Compare
+     value of old varobj and newly created varobj and mark
+     varobj updated If new value is different.  */
+  if (var_value == NULL && tmp_var_value == NULL)
+    ; /* Equal.  */
+  else if (var_value == NULL || tmp_var_value == NULL)
+    tmp_var->updated = 1;
+  else
+    {
+      /* Mark tmp_var updated if new value is different.  */
+      if (strcmp (tmp_var_value, var_value) != 0)
+ tmp_var->updated = 1;
+    }
+
+  xfree (tmp_var_value);
+  xfree (var_value);
   varobj_delete (var, NULL, 0);
   install_variable (tmp_var);
  }
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 8860526..4afb9e8 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -311,6 +311,9 @@ extern int varobj_editable_p (const struct varobj *var);
 
 extern int varobj_floating_p (const struct varobj *var);
 
+extern int varobj_is_valid_p (struct varobj *var);
+extern void varobj_set_invalid (struct varobj *var);
+
 extern void varobj_set_visualizer (struct varobj *var,
    const char *visualizer);
 
--
1.7.9.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Testsuite for varobj updation after symbol removal

Taimoor Mirza-2
In reply to this post by Taimoor Mirza-2
This patch provides testcases for variable object updation after removing
symbols. Test programs are same as used for testing remove-symbol-file
command in gdb.base. sym-file-main.c is modified to just add a global
variable for which varible object is created in testsuite.

Testing:
=======
This is tested for x86 target and also on ppc-linux-gnu and mips-gnu-linux
using both simulator and real boards.

2015-04-15  Taimoor Mirza  <[hidden email]>

        * gdb.mi/mi-var-invalidate.exp: Add tests to check global
        variable object change list is correct when its value is
        updated before removing symbols.
        * gdb.mi/sym-file-loader.c: New file.
        * gdb.mi/sym-file-loader.h: New file.
        * gdb.mi/sym-file-main.c: New file.
        * gdb.mi/sym-file-lib.c: New file.

Signed-off-by: Taimoor Mirza <[hidden email]>
---
 gdb/testsuite/gdb.mi/mi-var-invalidate.exp |   67 ++++++
 gdb/testsuite/gdb.mi/sym-file-lib.c        |   26 ++
 gdb/testsuite/gdb.mi/sym-file-loader.c     |  353 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/sym-file-loader.h     |   99 ++++++++
 gdb/testsuite/gdb.mi/sym-file-main.c       |   84 +++++++
 5 files changed, 629 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-lib.c
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-loader.c
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-loader.h
 create mode 100644 gdb/testsuite/gdb.mi/sym-file-main.c

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
index 0df1fd3..6492335 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
@@ -121,5 +121,72 @@ mi_gdb_test "-var-info-type global_simple" \
  "\\^done,type=\"\"" \
  "no type for invalid variable global_simple"
 
+# Test varobj updation after removing symbols.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set target_size TARGET_UNKNOWN
+if {[is_lp64_target]} {
+    set target_size TARGET_LP64
+} elseif {[is_ilp32_target]} {
+   set target_size TARGET_ILP32
+} else {
+    return 0
+}
+
+set main_basename sym-file-main
+set loader_basename sym-file-loader
+set lib_basename sym-file-lib
+
+standard_testfile $main_basename.c $loader_basename.c $lib_basename.c
+
+set libsrc "${srcdir}/${subdir}/${srcfile3}"
+set test_bin_name "sym-test-file"
+set test_bin [standard_output_file ${test_bin_name}]
+set shlib_name [standard_output_file ${lib_basename}.so]
+set exec_opts [list debug "additional_flags= -I$srcdir/../../include/ \
+-D$target_size -DSHLIB_NAME\\=\"$shlib_name\""]
+
+if [get_compiler_info] {
+    return -1
+}
+
+if {[gdb_compile_shlib $libsrc $shlib_name {debug}] != ""} {
+    untested ${testfile}
+    return -1
+}
+
+if {[build_executable $testfile  $test_bin "$srcfile $srcfile2" $exec_opts]} {
+    return -1
+}
+
+mi_delete_breakpoints
+mi_gdb_load ${test_bin}
+
+# Create varobj for count variable.
+mi_create_varobj var_count count "Create global varobj for count"
+
+# Run to GDB_ADD_SYMBOl_FILE in $srcfile for adding
+#    $shlib_name.
+mi_runto gdb_add_symbol_file
+
+# Add $shlib_name using 'add-symbol-file'.
+mi_gdb_test "-interpreter-exec console \"add-symbol-file ${shlib_name} addr\"" \
+    "~\"add symbol table from file .*so.*at.*= $hex.*\\^done" \
+    "add-symbol-file ${shlib_name}"
+
+# Continue to gdb_remove_symbol_file in $srcfile.
+mi_runto gdb_remove_symbol_file
+
+#  Remove $shlib_name using 'remove-symbol-file'.
+mi_gdb_test "-interpreter-exec console \"remove-symbol-file -a addr\"" \
+                ".*\\^done"\
+                "remove-symbol-file test"
+
+# Check var_count varobj changelist is not empty.
+mi_varobj_update var_count {var_count} "Update var_count"
+
 mi_gdb_exit
 return 0
diff --git a/gdb/testsuite/gdb.mi/sym-file-lib.c b/gdb/testsuite/gdb.mi/sym-file-lib.c
new file mode 100644
index 0000000..dae5188
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/sym-file-lib.c
@@ -0,0 +1,26 @@
+/* Copyright 2013-2014 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+extern int
+bar ()
+{
+  return 1; /* gdb break at bar.  */
+}
+
+extern int
+foo (int a)
+{
+  return a; /* gdb break at foo.  */
+}
diff --git a/gdb/testsuite/gdb.mi/sym-file-loader.c b/gdb/testsuite/gdb.mi/sym-file-loader.c
new file mode 100644
index 0000000..c72a1d1
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/sym-file-loader.c
@@ -0,0 +1,353 @@
+/* Copyright 2013-2014 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+
+#include "sym-file-loader.h"
+
+#ifdef TARGET_LP64
+
+uint8_t
+elf_st_type (uint8_t st_info)
+{
+  return ELF64_ST_TYPE (st_info);
+}
+
+#elif defined TARGET_ILP32
+
+uint8_t
+elf_st_type (uint8_t st_info)
+{
+  return ELF32_ST_TYPE (st_info);
+}
+
+#endif
+
+/* Load a program segment.  */
+
+static struct segment *
+load (uint8_t *addr, Elf_External_Phdr *phdr, struct segment *tail_seg)
+{
+  struct segment *seg = NULL;
+  uint8_t *mapped_addr = NULL;
+  void *from = NULL;
+  void *to = NULL;
+
+  /* For the sake of simplicity all operations are permitted.  */
+  unsigned perm = PROT_READ | PROT_WRITE | PROT_EXEC;
+
+  mapped_addr = (uint8_t *) mmap ((void *) GETADDR (phdr, p_vaddr),
+  GET (phdr, p_memsz), perm,
+  MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+
+  from = (void *) (addr + GET (phdr, p_offset));
+  to = (void *) mapped_addr;
+
+  memcpy (to, from, GET (phdr, p_filesz));
+
+  seg = (struct segment *) malloc (sizeof (struct segment));
+
+  if (seg == 0)
+    return 0;
+
+  seg->mapped_addr = mapped_addr;
+  seg->phdr = phdr;
+  seg->next = 0;
+
+  if (tail_seg != 0)
+    tail_seg->next = seg;
+
+  return seg;
+}
+
+/* Mini shared library loader.  No reallocation
+   is performed for the sake of simplicity.  */
+
+int
+load_shlib (const char *file, Elf_External_Ehdr **ehdr_out,
+    struct segment **seg_out)
+{
+  uint64_t i;
+  int fd;
+  off_t fsize;
+  uint8_t *addr;
+  Elf_External_Ehdr *ehdr;
+  Elf_External_Phdr *phdr;
+  struct segment *head_seg = NULL;
+  struct segment *tail_seg = NULL;
+
+  /* Map the lib in memory for reading.  */
+  fd = open (file, O_RDONLY);
+  if (fd < 0)
+    {
+      perror ("fopen failed.");
+      return -1;
+    }
+
+  fsize = lseek (fd, 0, SEEK_END);
+
+  if (fsize < 0)
+    {
+      perror ("lseek failed.");
+      return -1;
+    }
+
+  addr = (uint8_t *) mmap (NULL, fsize, PROT_READ, MAP_PRIVATE, fd, 0);
+  if (addr == (uint8_t *) -1)
+    {
+      perror ("mmap failed.");
+      return -1;
+    }
+
+  /* Check if the lib is an ELF file.  */
+  ehdr = (Elf_External_Ehdr *) addr;
+  if (ehdr->e_ident[EI_MAG0] != ELFMAG0
+      || ehdr->e_ident[EI_MAG1] != ELFMAG1
+      || ehdr->e_ident[EI_MAG2] != ELFMAG2
+      || ehdr->e_ident[EI_MAG3] != ELFMAG3)
+    {
+      printf ("Not an ELF file: %x\n", ehdr->e_ident[EI_MAG0]);
+      return -1;
+    }
+
+  if (ehdr->e_ident[EI_CLASS] == ELFCLASS32)
+    {
+      if (sizeof (void *) != 4)
+ {
+  printf ("Architecture mismatch.");
+  return -1;
+ }
+    }
+  else if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+    {
+      if (sizeof (void *) != 8)
+ {
+  printf ("Architecture mismatch.");
+  return -1;
+ }
+    }
+
+  /* Load the program segments.  For the sake of simplicity
+     assume that no reallocation is needed.  */
+  phdr = (Elf_External_Phdr *) (addr + GET (ehdr, e_phoff));
+  for (i = 0; i < GET (ehdr, e_phnum); i++, phdr++)
+    {
+      if (GET (phdr, p_type) == PT_LOAD)
+ {
+  struct segment *next_seg = load (addr, phdr, tail_seg);
+  if (next_seg == 0)
+    continue;
+  tail_seg = next_seg;
+  if (head_seg == 0)
+    head_seg = next_seg;
+ }
+    }
+  *ehdr_out = ehdr;
+  *seg_out = head_seg;
+  return 0;
+}
+
+/* Return the section-header table.  */
+
+Elf_External_Shdr *
+find_shdrtab (Elf_External_Ehdr *ehdr)
+{
+  return (Elf_External_Shdr *) (((uint8_t *) ehdr) + GET (ehdr, e_shoff));
+}
+
+/* Return the string table of the section headers.  */
+
+const char *
+find_shstrtab (Elf_External_Ehdr *ehdr, uint64_t *size)
+{
+  const Elf_External_Shdr *shdr;
+  const Elf_External_Shdr *shstr;
+
+  if (GET (ehdr, e_shnum) <= GET (ehdr, e_shstrndx))
+    {
+      printf ("The index of the string table is corrupt.");
+      return NULL;
+    }
+
+  shdr = find_shdrtab (ehdr);
+
+  shstr = &shdr[GET (ehdr, e_shstrndx)];
+  *size = GET (shstr, sh_size);
+  return ((const char *) ehdr) + GET (shstr, sh_offset);
+}
+
+/* Return the string table named SECTION.  */
+
+const char *
+find_strtab (Elf_External_Ehdr *ehdr,
+     const char *section, uint64_t *strtab_size)
+{
+  uint64_t shstrtab_size = 0;
+  const char *shstrtab;
+  uint64_t i;
+  const Elf_External_Shdr *shdr = find_shdrtab (ehdr);
+
+  /* Get the string table of the section headers.  */
+  shstrtab = find_shstrtab (ehdr, &shstrtab_size);
+  if (shstrtab == NULL)
+    return NULL;
+
+  for (i = 0; i < GET (ehdr, e_shnum); i++)
+    {
+      uint64_t name = GET (shdr + i, sh_name);
+      if (GET (shdr + i, sh_type) == SHT_STRTAB && name <= shstrtab_size
+  && strcmp ((const char *) &shstrtab[name], section) == 0)
+ {
+  *strtab_size = GET (shdr + i, sh_size);
+  return ((const char *) ehdr) + GET (shdr + i, sh_offset);
+ }
+
+    }
+  return NULL;
+}
+
+/* Return the section header named SECTION.  */
+
+Elf_External_Shdr *
+find_shdr (Elf_External_Ehdr *ehdr, const char *section)
+{
+  uint64_t shstrtab_size = 0;
+  const char *shstrtab;
+  uint64_t i;
+
+  /* Get the string table of the section headers.  */
+  shstrtab = find_shstrtab (ehdr, &shstrtab_size);
+  if (shstrtab == NULL)
+    return NULL;
+
+  Elf_External_Shdr *shdr = find_shdrtab (ehdr);
+  for (i = 0; i < GET (ehdr, e_shnum); i++)
+    {
+      uint64_t name = GET (shdr + i, sh_name);
+      if (name <= shstrtab_size)
+ {
+  if (strcmp ((const char *) &shstrtab[name], section) == 0)
+    return &shdr[i];
+ }
+
+    }
+  return NULL;
+}
+
+/* Return the symbol table.  */
+
+Elf_External_Sym *
+find_symtab (Elf_External_Ehdr *ehdr, uint64_t *symtab_size)
+{
+  uint64_t i;
+  const Elf_External_Shdr *shdr = find_shdrtab (ehdr);
+
+  for (i = 0; i < GET (ehdr, e_shnum); i++)
+    {
+      if (GET (shdr + i, sh_type) == SHT_SYMTAB)
+ {
+  *symtab_size = GET (shdr + i, sh_size) / sizeof (Elf_External_Sym);
+  return (Elf_External_Sym *) (((const char *) ehdr) +
+       GET (shdr + i, sh_offset));
+ }
+    }
+  return NULL;
+}
+
+/* Translate a file offset to an address in a loaded segment.  */
+
+int
+translate_offset (uint64_t file_offset, struct segment *seg, void **addr)
+{
+  while (seg)
+    {
+      uint64_t p_from, p_to;
+
+      Elf_External_Phdr *phdr = seg->phdr;
+
+      if (phdr == NULL)
+ {
+  seg = seg->next;
+  continue;
+ }
+
+      p_from = GET (phdr, p_offset);
+      p_to = p_from + GET (phdr, p_filesz);
+
+      if (p_from <= file_offset && file_offset < p_to)
+ {
+  *addr = (void *) (seg->mapped_addr + (file_offset - p_from));
+  return 0;
+ }
+      seg = seg->next;
+    }
+
+  return -1;
+}
+
+/* Lookup the address of FUNC.  */
+
+int
+lookup_function (const char *func,
+ Elf_External_Ehdr *ehdr, struct segment *seg, void **addr)
+{
+  const char *strtab;
+  uint64_t strtab_size = 0;
+  Elf_External_Sym *symtab;
+  uint64_t symtab_size = 0;
+  uint64_t i;
+
+  /* Get the string table for the symbols.  */
+  strtab = find_strtab (ehdr, ".strtab", &strtab_size);
+  if (strtab == NULL)
+    {
+      printf (".strtab not found.");
+      return -1;
+    }
+
+  /* Get the symbol table.  */
+  symtab = find_symtab (ehdr, &symtab_size);
+  if (symtab == NULL)
+    {
+      printf ("symbol table not found.");
+      return -1;
+    }
+
+  for (i = 0; i < symtab_size; i++)
+    {
+      Elf_External_Sym *sym = &symtab[i];
+
+      if (elf_st_type (GET (sym, st_info)) != STT_FUNC)
+ continue;
+
+      if (GET (sym, st_name) < strtab_size)
+ {
+  const char *name = &strtab[GET (sym, st_name)];
+  if (strcmp (name, func) == 0)
+    {
+
+      uint64_t offset = GET (sym, st_value);
+      return translate_offset (offset, seg, addr);
+    }
+ }
+    }
+
+  return -1;
+}
diff --git a/gdb/testsuite/gdb.mi/sym-file-loader.h b/gdb/testsuite/gdb.mi/sym-file-loader.h
new file mode 100644
index 0000000..03dc7e1
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/sym-file-loader.h
@@ -0,0 +1,99 @@
+/* Copyright 2013-2014 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef __SYM_FILE_LOADER__
+#define __SYM_FILE_LOADER__
+
+#include <inttypes.h>
+#include <ansidecl.h>
+#include <elf/common.h>
+#include <elf/external.h>
+
+#ifdef TARGET_LP64
+
+typedef Elf64_External_Phdr Elf_External_Phdr;
+typedef Elf64_External_Ehdr Elf_External_Ehdr;
+typedef Elf64_External_Shdr Elf_External_Shdr;
+typedef Elf64_External_Sym Elf_External_Sym;
+typedef uint64_t Elf_Addr;
+
+#elif defined TARGET_ILP32
+
+typedef Elf32_External_Phdr Elf_External_Phdr;
+typedef Elf32_External_Ehdr Elf_External_Ehdr;
+typedef Elf32_External_Shdr Elf_External_Shdr;
+typedef Elf32_External_Sym Elf_External_Sym;
+typedef uint32_t Elf_Addr;
+
+#endif
+
+#define GET(hdr, field) (\
+sizeof ((hdr)->field) == 1 ? (uint64_t) (hdr)->field[0] : \
+sizeof ((hdr)->field) == 2 ? (uint64_t) *(uint16_t *) (hdr)->field : \
+sizeof ((hdr)->field) == 4 ? (uint64_t) *(uint32_t *) (hdr)->field : \
+sizeof ((hdr)->field) == 8 ? *(uint64_t *) (hdr)->field : \
+*(uint64_t *) NULL)
+
+#define GETADDR(hdr, field) (\
+sizeof ((hdr)->field) == sizeof (Elf_Addr) ? *(Elf_Addr *) (hdr)->field : \
+*(Elf_Addr *) NULL)
+
+struct segment
+{
+  uint8_t *mapped_addr;
+  Elf_External_Phdr *phdr;
+  struct segment *next;
+};
+
+/* Mini shared library loader.  No reallocation is performed
+   for the sake of simplicity.  */
+
+int
+load_shlib (const char *file, Elf_External_Ehdr **ehdr_out,
+    struct segment **seg_out);
+
+/* Return the section-header table.  */
+
+Elf_External_Shdr *find_shdrtab (Elf_External_Ehdr *ehdr);
+
+/* Return the string table of the section headers.  */
+
+const char *find_shstrtab (Elf_External_Ehdr *ehdr, uint64_t *size);
+
+/* Return the string table named SECTION.  */
+
+const char *find_strtab (Elf_External_Ehdr *ehdr,
+ const char *section, uint64_t *strtab_size);
+
+/* Return the section header named SECTION.  */
+
+Elf_External_Shdr *find_shdr (Elf_External_Ehdr *ehdr, const char *section);
+
+/* Return the symbol table.  */
+
+Elf_External_Sym *find_symtab (Elf_External_Ehdr *ehdr,
+       uint64_t *symtab_size);
+
+/* Translate a file offset to an address in a loaded segment.  */
+
+int translate_offset (uint64_t file_offset, struct segment *seg, void **addr);
+
+/* Lookup the address of FUNC.  */
+
+int
+lookup_function (const char *func, Elf_External_Ehdr* ehdr,
+ struct segment *seg, void **addr);
+
+#endif
diff --git a/gdb/testsuite/gdb.mi/sym-file-main.c b/gdb/testsuite/gdb.mi/sym-file-main.c
new file mode 100644
index 0000000..8260824
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/sym-file-main.c
@@ -0,0 +1,84 @@
+/* Copyright 2013-2014 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "sym-file-loader.h"
+
+// Global variable
+int count = 0;
+
+void
+gdb_add_symbol_file (void *addr, const char *file)
+{
+  return;
+}
+
+void
+gdb_remove_symbol_file (void *addr)
+{
+  return;
+}
+
+/* Load a shared library without relying on the standard
+   loader to test GDB's commands for adding and removing
+   symbol files at runtime.  */
+
+int
+main (int argc, const char *argv[])
+{
+  const char *file = SHLIB_NAME;
+  Elf_External_Ehdr *ehdr = NULL;
+  struct segment *head_seg = NULL;
+  Elf_External_Shdr *text;
+  char *text_addr = NULL;
+  int (*pbar) () = NULL;
+  int (*pfoo) (int) = NULL;
+
+  if (load_shlib (file, &ehdr, &head_seg) != 0)
+    return -1;
+
+  /* Get the text section.  */
+  text = find_shdr (ehdr, ".text");
+  if (text == NULL)
+    return -1;
+
+  /* Notify GDB to add the symbol file.  */
+  if (translate_offset (GET (text, sh_offset), head_seg, (void **) &text_addr)
+      != 0)
+    return -1;
+
+  gdb_add_symbol_file (text_addr, file);
+
+  /* Call bar from SHLIB_NAME.  */
+  if (lookup_function ("bar", ehdr, head_seg, (void *) &pbar) != 0)
+    return -1;
+
+  (*pbar) ();
+
+  /* Call foo from SHLIB_NAME.  */
+  if (lookup_function ("foo", ehdr, head_seg, (void *) &pfoo) != 0)
+    return -1;
+
+  (*pfoo) (2);
+
+  count++;
+
+  /* Notify GDB to remove the symbol file.  */
+  gdb_remove_symbol_file (text_addr);
+
+  return 0;
+}
--
1.7.9.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Testsuite for varobj updation after symbol removal

Vladimir Prus-3


On 04/16/2015 06:32 AM, Taimoor Mirza wrote:
 > This patch provides testcases for variable object updation after removing
 > symbols. Test programs are same as used for testing remove-symbol-file
 > command in gdb.base. sym-file-main.c is modified to just add a global
 > variable for which varible object is created in testsuite.
 >

Taimoor,

thanks for the patch, and testsuite change. Looking at it, it appears that 'count' is
a global variable, and it's modified in one place, in the main function, and after that
you do 'remove-symbol-files' and verify that var-update reports reports 'count' as
changed. But what does it test exactly? Since count is explicitly modified in the
test, I'd imagine that -var-update will report it as changed even without
'remove-symbol-file'. In other words, does this testcase fail if the code patch is not
applied?

Even if so, does it test exactly what is your real case? It's easy to see why 'remove-symbol-files'
can make some global variables no longer present. But how it can change value of some expression?

Thanks,
Volodya

--
Vladimir Prus
CodeSourcery / Mentor Embedded
http://vladimirprus.com

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Testsuite for varobj updation after symbol removal

Taimoor Mirza-2
Hi Vladimir,

Thanks for reviewing the patch.

On 04/17/2015 08:18 PM, Vladimir Prus wrote:

>
>
> On 04/16/2015 06:32 AM, Taimoor Mirza wrote:
>  > This patch provides testcases for variable object updation after
> removing
>  > symbols. Test programs are same as used for testing remove-symbol-file
>  > command in gdb.base. sym-file-main.c is modified to just add a global
>  > variable for which varible object is created in testsuite.
>  >
>
> Taimoor,
>
> thanks for the patch, and testsuite change. Looking at it, it appears
> that 'count' is
> a global variable, and it's modified in one place, in the main function,
> and after that
> you do 'remove-symbol-files' and verify that var-update reports reports
> 'count' as
> changed. But what does it test exactly? Since count is explicitly
> modified in the
> test, I'd imagine that -var-update will report it as changed even without
> 'remove-symbol-file'. In other words, does this testcase fail if the
> code patch is not
> applied?

Yes. This testcase fails without applying this patch. Below is
problematic scenario:


1) Run GDB session and load symbols from file a.
2) Create varobj for some global variable test.
3) put some breakpoint in some function and run to that breakpoint.
4) Use add-symbol-file to load symbols of file b.
5) Put breakpoint on some other function that is called after updating
global variable test.
6) When this breakpoint gets hit, remove symbols of b using
'remove-symbol-file'.
7) print value of test. Its updated and different from initial value.
8) give 'var-update' command for variable object of 'test' and
changelist will be empty.

I know its not a common scenario but as long as it is valid
according to individual command specifications we have all the right to
expect it will produce the intended results.

>
> Even if so, does it test exactly what is your real case? It's easy to
> see why 'remove-symbol-files'
> can make some global variables no longer present. But how it can change
> value of some expression?

Its not the case of global variable not present. Its problem with
properly displaying changeset for a valid global variable whose value is
changed.
Step 7 in above scenario displays updated value of variable but
-var-update for varobj of that variable gives empty changeset. The
reason for this is mentioned in source code patch [PATCH 1/2].
>
> Thanks,
> Volodya
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Testsuite for varobj updation after symbol removal

Vladimir Prus-2
On 04/23/2015 11:04 AM, Taimoor wrote:

> Hi Vladimir,
>
> Thanks for reviewing the patch.
>
> On 04/17/2015 08:18 PM, Vladimir Prus wrote:
>>
>>
>> On 04/16/2015 06:32 AM, Taimoor Mirza wrote:
>>  > This patch provides testcases for variable object updation after
>> removing
>>  > symbols. Test programs are same as used for testing remove-symbol-file
>>  > command in gdb.base. sym-file-main.c is modified to just add a global
>>  > variable for which varible object is created in testsuite.
>>  >
>>
>> Taimoor,
>>
>> thanks for the patch, and testsuite change. Looking at it, it appears
>> that 'count' is
>> a global variable, and it's modified in one place, in the main function,
>> and after that
>> you do 'remove-symbol-files' and verify that var-update reports reports
>> 'count' as
>> changed. But what does it test exactly? Since count is explicitly
>> modified in the
>> test, I'd imagine that -var-update will report it as changed even without
>> 'remove-symbol-file'. In other words, does this testcase fail if the
>> code patch is not
>> applied?
>
> Yes. This testcase fails without applying this patch. Below is problematic scenario:
>
>
> 1) Run GDB session and load symbols from file a.
> 2) Create varobj for some global variable test.
> 3) put some breakpoint in some function and run to that breakpoint.
> 4) Use add-symbol-file to load symbols of file b.
> 5) Put breakpoint on some other function that is called after updating
> global variable test.
> 6) When this breakpoint gets hit, remove symbols of b using
> 'remove-symbol-file'.
> 7) print value of test. Its updated and different from initial value.
> 8) give 'var-update' command for variable object of 'test' and
> changelist will be empty.
>
> I know its not a common scenario but as long as it is valid
> according to individual command specifications we have all the right to
> expect it will produce the intended results.
>
>>
>> Even if so, does it test exactly what is your real case? It's easy to
>> see why 'remove-symbol-files'
>> can make some global variables no longer present. But how it can change
>> value of some expression?
>
> Its not the case of global variable not present. Its problem with properly displaying changeset for a valid global variable whose value is
> changed.
> Step 7 in above scenario displays updated value of variable but -var-update for varobj of that variable gives empty changeset. The reason
> for this is mentioned in source code patch [PATCH 1/2].

Hi Taimoor,

thanks, so the issue is that removing a symbol file recreates *all* global varobjs, and in the process clears any change of value
since the last update. I agree the patch is fixing that.

There's a second change, it seems - to invalidate varobj that refers to types in unloaded objfile - is there a test for that?

Thanks,


--
Vladimir Prus
CodeSourcery / Mentor Embedded
http://vladimirprus.com
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Testsuite for varobj updation after symbol removal

Taimoor Mirza-2


On 04/27/2015 11:46 PM, Vladimir Prus wrote:
>
> thanks, so the issue is that removing a symbol file recreates
> *all* global varobjs, and in the process clears any change of value
> since the last update. I agree the patch is fixing that.
>
> There's a second change, it seems - to invalidate varobj that refers to types in unloaded objfile - is there a test for that?
>

Hi Vladimir,

Sorry for delayed response. There are no separate tests for invalidation
of varobjs in unloaded objfile as it gets tested by existing varobj
tests. If we don't invalidate varobjs in unloaded objfile, a lot GDB
tests will fail. This is because while trying to retrieve old varobj
value for already removed objfile (to compare with current value), it
will try to access pointers that are no longer valid and that will
result in failure.
Marking varobjs that refers to types in unloaded objfile invalid makes
sure we do not accidentally use dangling pointers to obtain varobj value
for unloaded objfile.

IMO, GDB should avoid using pointers whose target has been already freed
and this change specifically marks var->type to NULL to make sure no
dangling pointers remain after objfile is unloaded so any subsequent
call to varobj_get_value does not result in segmentation fault.


Thanks,
Taimoor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Testsuite for varobj updation after symbol removal

Taimoor Mirza-2
Hi Vladimir,

Do you have any further comments on the change or I should push it in?

Thanks,
Taimoor

On 05/11/2015 06:49 PM, Taimoor wrote:

>
>
> On 04/27/2015 11:46 PM, Vladimir Prus wrote:
>>
>> thanks, so the issue is that removing a symbol file recreates
>> *all* global varobjs, and in the process clears any change of value
>> since the last update. I agree the patch is fixing that.
>>
>> There's a second change, it seems - to invalidate varobj that refers
>> to types in unloaded objfile - is there a test for that?
>>
>
> Hi Vladimir,
>
> Sorry for delayed response. There are no separate tests for invalidation
> of varobjs in unloaded objfile as it gets tested by existing varobj
> tests. If we don't invalidate varobjs in unloaded objfile, a lot GDB
> tests will fail. This is because while trying to retrieve old varobj
> value for already removed objfile (to compare with current value), it
> will try to access pointers that are no longer valid and that will
> result in failure.
> Marking varobjs that refers to types in unloaded objfile invalid makes
> sure we do not accidentally use dangling pointers to obtain varobj value
> for unloaded objfile.
>
> IMO, GDB should avoid using pointers whose target has been already freed
> and this change specifically marks var->type to NULL to make sure no
> dangling pointers remain after objfile is unloaded so any subsequent
> call to varobj_get_value does not result in segmentation fault.
>
>
> Thanks,
> Taimoor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [PING]Testsuite for varobj updation after symbol removal

Taimoor Mirza-2
In reply to this post by Taimoor Mirza-2
ping.

If there are no further comments, Can I push this change?

Thanks,
Taimoor

On 05/11/2015 06:49 PM, Taimoor wrote:

>
>
> On 04/27/2015 11:46 PM, Vladimir Prus wrote:
>>
>> thanks, so the issue is that removing a symbol file recreates
>> *all* global varobjs, and in the process clears any change of value
>> since the last update. I agree the patch is fixing that.
>>
>> There's a second change, it seems - to invalidate varobj that refers
>> to types in unloaded objfile - is there a test for that?
>>
>
> Hi Vladimir,
>
> Sorry for delayed response. There are no separate tests for invalidation
> of varobjs in unloaded objfile as it gets tested by existing varobj
> tests. If we don't invalidate varobjs in unloaded objfile, a lot GDB
> tests will fail. This is because while trying to retrieve old varobj
> value for already removed objfile (to compare with current value), it
> will try to access pointers that are no longer valid and that will
> result in failure.
> Marking varobjs that refers to types in unloaded objfile invalid makes
> sure we do not accidentally use dangling pointers to obtain varobj value
> for unloaded objfile.
>
> IMO, GDB should avoid using pointers whose target has been already freed
> and this change specifically marks var->type to NULL to make sure no
> dangling pointers remain after objfile is unloaded so any subsequent
> call to varobj_get_value does not result in segmentation fault.
>
>
> Thanks,
> Taimoor