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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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