[patch+7.4] reread.exp 7.3->7.4 regression

[patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

Hi Joel,

this patch should be tested on ARM, I do not yet have some ARM testing
environment (I probably could build one).

There is now crash regression 7.3->7.4, seen on ia64 but it apparently applies
to all archs:

Program terminated with signal 11, Segmentation fault.
#0  in freehook () from /lib/libc.so.6.1
(gdb) bt
#0  in freehook () from /lib/libc.so.6.1
#1  in free () from /lib/libc.so.6.1
#2  in xfree (ptr=0x9393939300000000) at ./common/common-utils.c:109
#3  in VEC_arm_exidx_entry_s_free (vec_=0x6000000000cb01e0) at arm-tdep.c:2143
#4  in arm_exidx_data_free (objfile=0x6000000000ca4650, arg=0x6000000000cb00b0) at arm-tdep.c:2157
#5  in clear_objfile_data (objfile=0x6000000000ca4650) at objfiles.c:1463
#6  in reread_symbols () at symfile.c:2489
#7  in run_command_1 (args=0x0, from_tty=1, tbreak_at_main=0) at infcmd.c:528
#8  in run_command (args=0x0, from_tty=1) at infcmd.c:622

(gdb) file /root/jkratoch/redhat/gdb74/gdb/testsuite/gdb.base/reread
Reading symbols from /root/jkratoch/redhat/gdb74/gdb/testsuite/gdb.base/reread...done.
arm_exidx_new_objfile objfile=0x6000000000ca4650 section_count=36
[...]
`/root/jkratoch/redhat/gdb74/gdb/testsuite/gdb.base/reread' has changed; re-reading symbols.
arm_exidx_data_free objfile=0x6000000000ca4650 section_count=37
FAIL: gdb.base/reread.exp: run to foo() second time (timeout)


This is because objfile->OBFD can change underneath registered objfile data.

There are multiple such problems because objfile is not destroyed + recreated
on objfile reload, there was a patch for it but it has never been checked in:
	[patch] Fix a reread_symbols regression by mmap
	http://sourceware.org/ml/gdb-patches/2009-06/msg00606.html
	http://sourceware.org/ml/gdb-patches/2009-08/msg00207.html
	http://sourceware.org/ml/gdb-patches/2011-12/msg00596.html

As I believe the patch above is not suitable for the 7.4 branch offering at
least this very ugly patch which breaks ARM functionality on rereads but it at
least does not break ARM-unrelated arches.


Thanks,
Jan


gdb/
2011-12-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* arm-tdep.c (struct arm_per_objfile): Add field section_count.
	(arm_find_mapping_symbol): Sanity check INDEX against SECTION_COUNT.
	(struct arm_exidx_data): Add field section_count.
	(arm_exidx_data_free): Use SECTION_COUNT from struct arm_exidx_data.
	(arm_exidx_new_objfile): Initialize arm_exidx_data->section_count.
	Use it.
	(arm_find_exidx_entry): Sanity check INDEX against SECTION_COUNT.
	(arm_objfile_data_free): Use SECTION_COUNT from struct arm_per_objfile.
	(arm_record_special_symbol): Initialize arm_per_objfile->section_count.
	Use it.  Sanity check INDEX against SECTION_COUNT.

--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -90,6 +90,10 @@ DEF_VEC_O(arm_mapping_symbol_s);
 struct arm_per_objfile
 {
   VEC(arm_mapping_symbol_s) **section_maps;
+
+  /* A copy from OBFD.  OBFD can change underneath by reread_symbols.  It is
+     wrong then to use arm_per_objfile but at least do not crash.  */
+  unsigned int section_count;
 };
 
 /* The list of available "set arm ..." and "show arm ..." commands.  */
@@ -330,7 +334,7 @@ arm_find_mapping_symbol (CORE_ADDR memaddr, CORE_ADDR *start)
       unsigned int idx;
 
       data = objfile_data (sec->objfile, arm_objfile_data_key);
-      if (data != NULL)
+      if (data != NULL && sec->the_bfd_section->index < data->section_count)
 	{
 	  map = data->section_maps[sec->the_bfd_section->index];
 	  if (!VEC_empty (arm_mapping_symbol_s, map))
@@ -2145,6 +2149,10 @@ DEF_VEC_O(arm_exidx_entry_s);
 struct arm_exidx_data
 {
   VEC(arm_exidx_entry_s) **section_maps;
+
+  /* A copy from OBFD.  OBFD can change underneath by reread_symbols.  It is
+     wrong then to use arm_per_objfile but at least do not crash.  */
+  unsigned int section_count;
 };
 
 static void
@@ -2153,7 +2161,7 @@ arm_exidx_data_free (struct objfile *objfile, void *arg)
   struct arm_exidx_data *data = arg;
   unsigned int i;
 
-  for (i = 0; i < objfile->obfd->section_count; i++)
+  for (i = 0; i < data->section_count; i++)
     VEC_free (arm_exidx_entry_s, data->section_maps[i]);
 }
 
@@ -2250,8 +2258,9 @@ arm_exidx_new_objfile (struct objfile *objfile)
   /* Allocate exception table data structure.  */
   data = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct arm_exidx_data);
   set_objfile_data (objfile, arm_exidx_data_key, data);
+  data->section_count = objfile->obfd->section_count;
   data->section_maps = OBSTACK_CALLOC (&objfile->objfile_obstack,
-				       objfile->obfd->section_count,
+				       data->section_count,
 				       VEC(arm_exidx_entry_s) *);
 
   /* Fill in exception table.  */
@@ -2425,7 +2434,7 @@ arm_find_exidx_entry (CORE_ADDR memaddr, CORE_ADDR *start)
       unsigned int idx;
 
       data = objfile_data (sec->objfile, arm_exidx_data_key);
-      if (data != NULL)
+      if (data != NULL && sec->the_bfd_section->index < data->section_count)
 	{
 	  map = data->section_maps[sec->the_bfd_section->index];
 	  if (!VEC_empty (arm_exidx_entry_s, map))
@@ -9398,7 +9407,7 @@ arm_objfile_data_free (struct objfile *objfile, void *arg)
   struct arm_per_objfile *data = arg;
   unsigned int i;
 
-  for (i = 0; i < objfile->obfd->section_count; i++)
+  for (i = 0; i < data->section_count; i++)
     VEC_free (arm_mapping_symbol_s, data->section_maps[i]);
 }
 
@@ -9421,10 +9430,13 @@ arm_record_special_symbol (struct gdbarch *gdbarch, struct objfile *objfile,
       data = OBSTACK_ZALLOC (&objfile->objfile_obstack,
 			     struct arm_per_objfile);
       set_objfile_data (objfile, arm_objfile_data_key, data);
+      data->section_count = objfile->obfd->section_count;
       data->section_maps = OBSTACK_CALLOC (&objfile->objfile_obstack,
-					   objfile->obfd->section_count,
+					   data->section_count,
 					   VEC(arm_mapping_symbol_s) *);
     }
+  else if (bfd_get_section (sym)->index >= data->section_count)
+    return;
   map_p = &data->section_maps[bfd_get_section (sym)->index];
 
   new_map_sym.value = sym->value;

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Joel Brobecker]

Hi Jan,

> this patch should be tested on ARM, I do not yet have some ARM testing
> environment (I probably could build one).

Unfortunately, I don't have an arm environment either. Perhaps
CodeSourcery does?

> As I believe the patch above is not suitable for the 7.4 branch
> offering at least this very ugly patch which breaks ARM functionality
> on rereads but it at least does not break ARM-unrelated arches.

Yeah, I'm a little concerned by your patch. I might be OK putting it
in the branch, but I'd rather not do that until we have found a proper
fix on the head.

Have we explored the idea of not registering the arm_exidx_data_free
routine? We'd leak some memory, but the way I read arm_exidx_new_objfile,
it wouldn't be very much except maybe when dealing with arm object files.

WDYT?

-- 
Joel

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

Hi Joel,

On Mon, 19 Dec 2011 04:48:07 +0100, Joel Brobecker wrote:
> Unfortunately, I don't have an arm environment either. Perhaps
> CodeSourcery does?

OK, I will run it today.


> Yeah, I'm a little concerned by your patch.

It should go definitely also for the HEAD in such case until some other proper
fix lands there, it is a memory data corruption.


> Have we explored the idea of not registering the arm_exidx_data_free
> routine? We'd leak some memory,

The freeing is not the only problem.  The same problem applies on the lines
accessing the elements, those statements can also crash now without this
patch.  It is just not shown in this crash backtrace.


Thanks,
Jan

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Joel Brobecker]

> The freeing is not the only problem.  The same problem applies on the lines
> accessing the elements, those statements can also crash now without this
> patch.  It is just not shown in this crash backtrace.

Ugh, this is really nasty. I know you've delved into this more than
I have, so I am Ok with your proposed initial work around. Others
might have an opinion too. Is it worth labeling all the places that
we ended up modifying? I guess not - they all reference the new
field which is clearly documented as being here to work around
a deficiency in the design.

I think this problem ties us with the problem Tom was trying to
figure out, where BFD opens and closes bfd's without telling us.

-- 
Joel

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

On Mon, 19 Dec 2011 10:54:27 +0100, Joel Brobecker wrote:
> I think this problem ties us with the problem Tom was trying to
> figure out, where BFD opens and closes bfd's without telling us.

Not sure if you refer to Tom's pointing at bfd/cache.c and its

/* The maximum number of files which the cache will keep open at one time.  */
#define BFD_CACHE_MAX_OPEN 10

which possibly negatively affects performance in GNU/Linux but it closes only
bfd->iostream, not bfd itself.  The problem of this mail thread occurs due to
bfd->section_count changes.  Therefore I find it as an unrelated issue.


Thanks,
Jan

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Ulrich Weigand]

Jan Kratochvil wrote:

> this patch should be tested on ARM, I do not yet have some ARM testing
> environment (I probably could build one).

I can test on ARM if you want.

> This is because objfile->OBFD can change underneath registered objfile data.
> 
> There are multiple such problems because objfile is not destroyed + recreated
> on objfile reload, there was a patch for it but it has never been checked in:
> 	[patch] Fix a reread_symbols regression by mmap
> 	http://sourceware.org/ml/gdb-patches/2009-06/msg00606.html
> 	http://sourceware.org/ml/gdb-patches/2009-08/msg00207.html
> 	http://sourceware.org/ml/gdb-patches/2011-12/msg00596.html
> 
> As I believe the patch above is not suitable for the 7.4 branch offering at
> least this very ugly patch which breaks ARM functionality on rereads but it at
> least does not break ARM-unrelated arches.

I really think this ought to be fixed in reread_symbols; freeing the old
OBFD needs to be done *after* all the callbacks to cleanup objfile data
have completed.   Your initial patch already moved the callbacks calls
up a bit; I think it needs to be moved up even further.  (There is the
issue of what state the objfile is left in if any of the "error" calls
is triggered, though.)   In addition, we should probably call 
observer_notify_new_objfile so that new tables can be built up for
the re-read file ...

>  struct arm_per_objfile
>  {
>    VEC(arm_mapping_symbol_s) **section_maps;
> +
> +  /* A copy from OBFD.  OBFD can change underneath by reread_symbols.  It is
> +     wrong then to use arm_per_objfile but at least do not crash.  */
> +  unsigned int section_count;
>  };

That said, I guess this is OK as a workaround.  However, even with the current
broken code, it seems only the _free routines ever see the "wrong" OBFD.  Thus
I'd prefer for only the _free routines to rely on this new value ...

> @@ -330,7 +334,7 @@ arm_find_mapping_symbol (CORE_ADDR memaddr, CORE_ADDR *start)
>        unsigned int idx;
>  
>        data = objfile_data (sec->objfile, arm_objfile_data_key);
> -      if (data != NULL)
> +      if (data != NULL && sec->the_bfd_section->index < data->section_count)
>  	{
>  	  map = data->section_maps[sec->the_bfd_section->index];

... and for checks in other users like this to be turned into assertions instead.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> -      if (data != NULL)
Jan> +      if (data != NULL && sec->the_bfd_section->index < data->section_count)

I don't understand the need for this hunk.
When can this be called in a context where the BFD doesn't match the
data registered with the objfile?

Tom

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> I really think this ought to be fixed in reread_symbols; freeing the old
Ulrich> OBFD needs to be done *after* all the callbacks to cleanup objfile data
Ulrich> have completed.

I agree.

Also see free_objfile, which calls objfile_free_data before closing the
BFD.

This, btw, is the main reason that I like Jan's old patch to unify
re-reading with freeing -- it automatically means consistency.

Perhaps Pedro's problems with that patch could be alleviated by having
"destroy in place" and "initialize in place" functions that both
free_objfile and reread_symbols could use.

Tom

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

On Mon, 19 Dec 2011 20:04:30 +0100, Tom Tromey wrote:
> Perhaps Pedro's problems with that patch could be alleviated by having
> "destroy in place" and "initialize in place" functions that both
> free_objfile and reread_symbols could use.

The problem is that if one uses file A with separate debug info and new file
A no longer has split debug info then one (or more on OSX) objfile will need
to be removed anyway.  So `struct objfile *' cannot work.

Maybe one could make a compromise it is valid to hold `struct objfile *' iff
it is not a separate debug info file and review all the `struct objfile *'
holders with this rule.


Regards,
Jan

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

On Mon, 19 Dec 2011 11:30:54 +0100, Ulrich Weigand wrote:
> I can test on ARM if you want.

I have tested it on fedora13beta.  Just on

Hardware        : OMAP3 Beagle Board
Revision        : 0020
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 506.27

it takes 57m28.760s to build GDB (although with full symbols) and 51m13.889s
to run the testsuite, is it normal to develop on ARM with this performance?


> I really think this ought to be fixed in reread_symbols;

I did not want to touch it anymore but I see it is currently the most feasible
way how to fix the trunk.


> freeing the old OBFD needs to be done *after* all the callbacks to cleanup
> objfile data have completed.

Done, it follow the free_objfile order now.


> Your initial patch already moved the callbacks calls up a bit;

My patch was using free_objfile, I do not understand which patch of mine do
you refer to now.
	Re: [patch] Replace reread_symbols by load+free calls
	http://sourceware.org/ml/gdb-patches/2009-06/msg00696.html


> In addition, we should probably call observer_notify_new_objfile so that new
> tables can be built up for the re-read file ...

I can post it afterwards, that is probably not for 7.4, I will have to run the
ARM testsuite again.



> > -      if (data != NULL)
> > +      if (data != NULL && sec->the_bfd_section->index < data->section_count)
[...]
> ... and for checks in other users like this to be turned into assertions instead.

On Mon, 19 Dec 2011 16:15:23 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> -      if (data != NULL)
> Jan> +      if (data != NULL && sec->the_bfd_section->index < data->section_count)
> 
> I don't understand the need for this hunk.
> When can this be called in a context where the BFD doesn't match the
> data registered with the objfile?

I did not know this part of code is not affected by the problem.


No regressions on {x86_64,x86_64-m32,i686}-fedora16-linux-gnu and on
arm-fedora13beta-linux-gnu.  OK to check it in and for 7.4?


Thanks,
Jan


gdb/
2011-12-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (reread_symbols): Move free_objfile_separate_debug,
	preserve_values, sym_finish and clear_objfile_data calls before BFD
	close.  Move free_objfile_separate_debug as the very first call.  New
	comment on the ordering.

gdb/testsuite/
2011-12-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/reread.exp: If srcfile2 fails to build retry it with
	-DNO_SECTIONS.
	* gdb.base/reread2.c <!NO_SECTIONS>: New sections block.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2446,6 +2446,29 @@ reread_symbols (void)
 	      exec_file_attach (bfd_get_filename (objfile->obfd), 0);
 	    }
 
+	  /* Keep the calls order approx. the same as in free_objfile.  */
+
+	  /* Free the separate debug objfiles.  It will be
+	     automatically recreated by sym_read.  */
+	  free_objfile_separate_debug (objfile);
+
+	  /* Remove any references to this objfile in the global
+	     value lists.  */
+	  preserve_values (objfile);
+
+	  /* Nuke all the state that we will re-read.  Much of the following
+	     code which sets things to NULL really is necessary to tell
+	     other parts of GDB that there is nothing currently there.
+
+	     Try to keep the freeing order compatible with free_objfile.  */
+
+	  if (objfile->sf != NULL)
+	    {
+	      (*objfile->sf->sym_finish) (objfile);
+	    }
+
+	  clear_objfile_data (objfile);
+
 	  /* Clean up any state BFD has sitting around.  We don't need
 	     to close the descriptor but BFD lacks a way of closing the
 	     BFD without closing the descriptor.  */
@@ -2471,27 +2494,6 @@ reread_symbols (void)
 	  memcpy (offsets, objfile->section_offsets,
 		  SIZEOF_N_SECTION_OFFSETS (num_offsets));
 
-	  /* Remove any references to this objfile in the global
-	     value lists.  */
-	  preserve_values (objfile);
-
-	  /* Nuke all the state that we will re-read.  Much of the following
-	     code which sets things to NULL really is necessary to tell
-	     other parts of GDB that there is nothing currently there.
-
-	     Try to keep the freeing order compatible with free_objfile.  */
-
-	  if (objfile->sf != NULL)
-	    {
-	      (*objfile->sf->sym_finish) (objfile);
-	    }
-
-	  clear_objfile_data (objfile);
-
-	  /* Free the separate debug objfiles.  It will be
-	     automatically recreated by sym_read.  */
-          free_objfile_separate_debug (objfile);
-
 	  /* FIXME: Do we have to free a whole linked list, or is this
 	     enough?  */
 	  if (objfile->global_psymbols.list)
--- a/gdb/testsuite/gdb.base/reread.exp
+++ b/gdb/testsuite/gdb.base/reread.exp
@@ -38,7 +38,8 @@ set testfile2 "reread2"
 set srcfile2 ${testfile2}.c
 set binfile2 ${objdir}/${subdir}/${testfile2}$EXEEXT
 
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug nowarnings}] != "" } {
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug nowarnings}] != ""
+      && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug nowarnings additional_flags=-DNO_SECTIONS}] != ""} {
     untested reread.exp
     return -1
 }
--- a/gdb/testsuite/gdb.base/reread2.c
+++ b/gdb/testsuite/gdb.base/reread2.c
@@ -15,3 +15,14 @@ int main()
   foo();
   return 0;
 }
+
+/* Ensure the new file will have more sections.  It may exploit code not
+   updating its SECTION_COUNT on reread_symbols.  */
+
+#ifndef NO_SECTIONS
+# define VAR0(n) __attribute__ ((section ("sect" #n))) int var##n;
+# define VAR1(n) VAR0 (n ## 0) VAR0(n ## 1) VAR0(n ## 2) VAR0(n ## 3)
+# define VAR2(n) VAR1 (n ## 0) VAR1(n ## 1) VAR1(n ## 2) VAR1(n ## 3)
+# define VAR3(n) VAR2 (n ## 0) VAR2(n ## 1) VAR2(n ## 2) VAR2(n ## 3)
+VAR3 (0)
+#endif

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> The problem is that if one uses file A with separate debug info and
Jan> new file A no longer has split debug info then one (or more on OSX)
Jan> objfile will need to be removed anyway.  So `struct objfile *'
Jan> cannot work.

I still favor your original patch but at this point I am looking for a
compromise that would still improve the situation.

Tom

Re: [patch+7.4] reread.exp 7.3->7.4 regression

[Ulrich Weigand]

Jan Kratochvil wrote:
> On Mon, 19 Dec 2011 11:30:54 +0100, Ulrich Weigand wrote:
> > I can test on ARM if you want.
> 
> I have tested it on fedora13beta.  Just on
> 
> Hardware        : OMAP3 Beagle Board
> Revision        : 0020
> Processor       : ARMv7 Processor rev 2 (v7l)
> BogoMIPS        : 506.27
> 
> it takes 57m28.760s to build GDB (although with full symbols) and 51m13.889s
> to run the testsuite, is it normal to develop on ARM with this performance?

I guess so.  A Panda is a bit faster than the Beagle, but still ...

> > Your initial patch already moved the callbacks calls up a bit;
> 
> My patch was using free_objfile, I do not understand which patch of mine do
> you refer to now.
> 	Re: [patch] Replace reread_symbols by load+free calls
> 	http://sourceware.org/ml/gdb-patches/2009-06/msg00696.html

Sorry, I was refering to the first patch in list you had in the mail I replied to.
Specifically, I was talking about this patch:
http://sourceware.org/ml/gdb-patches/2009-06/msg00606.html
which seems to have gone upstream 2009-06-23.

> > In addition, we should probably call observer_notify_new_objfile so that new
> > tables can be built up for the re-read file ...
> 
> I can post it afterwards, that is probably not for 7.4, I will have to run the
> ARM testsuite again.

OK, thanks.

> No regressions on {x86_64,x86_64-m32,i686}-fedora16-linux-gnu and on
> arm-fedora13beta-linux-gnu.  OK to check it in and for 7.4?
> 
> gdb/
> 2011-12-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* symfile.c (reread_symbols): Move free_objfile_separate_debug,
> 	preserve_values, sym_finish and clear_objfile_data calls before BFD
> 	close.  Move free_objfile_separate_debug as the very first call.  New
> 	comment on the ordering.

This looks reasonable to me, and seems preferable to the ARM-specific hack ...


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[commit+7.4] [patch+7.4] reread.exp 7.3->7.4 regression

[Jan Kratochvil]

On Mon, 19 Dec 2011 22:37:04 +0100, Ulrich Weigand wrote:
> > No regressions on {x86_64,x86_64-m32,i686}-fedora16-linux-gnu and on
> > arm-fedora13beta-linux-gnu.  OK to check it in and for 7.4?
> > 
> > gdb/
> > 2011-12-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* symfile.c (reread_symbols): Move free_objfile_separate_debug,
> > 	preserve_values, sym_finish and clear_objfile_data calls before BFD
> > 	close.  Move free_objfile_separate_debug as the very first call.  New
> > 	comment on the ordering.
> 
> This looks reasonable to me, and seems preferable to the ARM-specific hack ...

Checked in and for 7.4:
	http://sourceware.org/ml/gdb-cvs/2011-12/msg00197.html
	http://sourceware.org/ml/gdb-cvs/2011-12/msg00198.html


Thanks for the review,
Jan

[patch] reread_symbols observer_notify_new_objfile - fix ARM unwinding after reread [Re: [patch+7.4] reread.exp 7.3->7.4 regression]

[Jan Kratochvil]

On Mon, 19 Dec 2011 11:30:54 +0100, Ulrich Weigand wrote:
> In addition, we should probably call observer_notify_new_objfile so that new
> tables can be built up for the re-read file ...

Implemented.

I have tested it really fixes the problem of ARM unwinding after reread on
stripped GDB binary.

Before this fix:

./gdb -nx ./test-gdb-strip -ex 'b gdb_do_one_event' -ex r -ex bt
Breakpoint 1, 0x00372154 in gdb_do_one_event ()
#0  0x00372154 in gdb_do_one_event ()
#1  0x003722c8 in start_event_loop ()
#2  0x003741bc in cli_command_loop ()
#3  0x0036b9e0 in current_interp_command_loop ()
#4  0x0007a054 in ?? ()
#5  0x0036af08 in catch_errors ()
#6  0x0007b58c in ?? ()
#7  0x0036af08 in catch_errors ()
#8  0x0007b5c4 in gdb_main ()
#9  0x00079cd8 in main ()
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
`/opt/jkratoch/redhat/archer-git/gdb/test-gdb-strip' has changed; re-reading symbols.
(no debugging symbols found)
[...]
Breakpoint 1, 0x00372154 in gdb_do_one_event ()
(gdb) bt
#0  0x00372154 in gdb_do_one_event ()
#1  0x003722c8 in start_event_loop ()
#2  0x003741bc in cli_command_loop ()
#3  0x0036b9e0 in current_interp_command_loop ()
#4  0x0007a054 in ?? ()
#5  0x0007a054 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

After this fix both backtraces are the same.

Testcase is not provided as I do not have some small enough ARM testcase which
unwinds with .ARM.exidx but does not unwind without it.

No regressions on {x86_64,x86_64-m32,i686}-fedora16-linux-gnu and on
arm-fedora13beta-linux-gnueabi.


Thanks,
Jan


gdb/
2011-12-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (objfilep): New typedef and new DEF_VEC_P.
	(reread_symbols): Remove variable reread_one, new variables
	new_objfiles, all_cleanups and ix.  Use new_objfiles instead of
	reread_one.  Push changed objfiles to new_objfiles, call
	observer_notify_new_objfile for them later.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2371,15 +2371,22 @@ add_symbol_file_command (char *args, int from_tty)
 }
 \f
 
+typedef struct objfile *objfilep;
+
+DEF_VEC_P (objfilep);
+
 /* Re-read symbols if a symbol-file has changed.  */
 void
 reread_symbols (void)
 {
   struct objfile *objfile;
   long new_modtime;
-  int reread_one = 0;
   struct stat new_statbuf;
   int res;
+  VEC (objfilep) *new_objfiles = NULL;
+  struct cleanup *all_cleanups;
+
+  all_cleanups = make_cleanup (VEC_cleanup (objfilep), &new_objfiles);
 
   /* With the addition of shared libraries, this should be modified,
      the load time should be saved in the partial symbol tables, since
@@ -2594,21 +2601,33 @@ reread_symbols (void)
 	     and now, we *want* this to be out of date, so don't call stat
 	     again now.  */
 	  objfile->mtime = new_modtime;
-	  reread_one = 1;
 	  init_entry_point_info (objfile);
+
+	  VEC_safe_push (objfilep, new_objfiles, objfile);
 	}
     }
 
-  if (reread_one)
+  if (new_objfiles)
     {
+      int ix;
+
       /* Notify objfiles that we've modified objfile sections.  */
       objfiles_changed ();
 
       clear_symtab_users (0);
+
+      /* clear_objfile_data for each objfile was called before freeing it and
+	 observer_notify_new_objfile (NULL) has been called by
+	 clear_symtab_users above.  Notify the new files now.  */
+      for (ix = 0; VEC_iterate (objfilep, new_objfiles, ix, objfile); ix++)
+	observer_notify_new_objfile (objfile);
+
       /* At least one objfile has changed, so we can consider that
          the executable we're debugging has changed too.  */
       observer_notify_executable_changed ();
     }
+
+  do_cleanups (all_cleanups);
 }
 \f
 

Re: [patch] reread_symbols observer_notify_new_objfile - fix ARM unwinding after reread [Re: [patch+7.4] reread.exp 7.3->7.4 regres

[Ulrich Weigand]

Jan Kratochvil wrote:

> 2011-12-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* symfile.c (objfilep): New typedef and new DEF_VEC_P.
> 	(reread_symbols): Remove variable reread_one, new variables
> 	new_objfiles, all_cleanups and ix.  Use new_objfiles instead of
> 	reread_one.  Push changed objfiles to new_objfiles, call
> 	observer_notify_new_objfile for them later.

Looks good to me.   Thanks for fixing this!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[commit] [patch] reread_symbols observer_notify_new_objfile - fix ARM unwinding after reread

[Jan Kratochvil]

On Wed, 21 Dec 2011 14:12:21 +0100, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> 
> > 2011-12-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* symfile.c (objfilep): New typedef and new DEF_VEC_P.
> > 	(reread_symbols): Remove variable reread_one, new variables
> > 	new_objfiles, all_cleanups and ix.  Use new_objfiles instead of
> > 	reread_one.  Push changed objfiles to new_objfiles, call
> > 	observer_notify_new_objfile for them later.
> 
> Looks good to me.   Thanks for fixing this!

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-12/msg00211.html


Thanks,
Jan