[v2,01/11] s390: Remove duplicate checks for cached gdbarch at init

  When initializing the gdbarch there is a check whether an appropriate
gdbarch already exists in the gdbarch_list.  If any of the checks fails
this would lead to a different target description.  However
gdbarch_list_lookup_by_info already checks for

	if (info->target_desc != arches->gdbarch->target_desc)
	  continue;

So it never returns a gdbarch where any of the checks can fail.

Remove the duplicate check.  This also allows to move the lookup at the
start of the function.

gdb/ChangeLog:

	* s390-linux-tdep.c (s390_gdbarch_init): Remove douplicate checks when
	looking for cached gdbarch and move to start of function
---
 gdb/s390-linux-tdep.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)
  

Philipp Rudo <prudo@linux.vnet.ibm.com> writes:

> -  /* Find a candidate among extant architectures.  */
> -  for (arches = gdbarch_list_lookup_by_info (arches, &info);
> -       arches != NULL;
> -       arches = gdbarch_list_lookup_by_info (arches->next, &info))
> -    {
> -      tdep = gdbarch_tdep (arches->gdbarch);
> -      if (!tdep)
> -	continue;
> -      if (tdep->abi != tdep_abi)
> -	continue;
> -      if (tdep->vector_abi != vector_abi)
> -	continue;

Is it possible that we have two instances of gdbarch, with the same
target description, but different vector_abi?  Two different executables
compiled with different vector abis, and GDB can debug them together
(multi-process debugging.  If we consider multi-target debugging in the
future, these two executable can from different targets).

> -      if ((tdep->gpr_full_regnum != -1) != have_upper)
> -	continue;
> -      if (tdep->have_gs != have_gs)
> -	continue;
> -      if (tdesc_data != NULL)
> -	tdesc_data_cleanup (tdesc_data);
> -      return arches->gdbarch;
> -    }
  

Hi Yao,


On Tue, 05 Dec 2017 16:16:07 +0000
Yao Qi <qiyaoltc@gmail.com> wrote:

> Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
> 
> > -  /* Find a candidate among extant architectures.  */
> > -  for (arches = gdbarch_list_lookup_by_info (arches, &info);
> > -       arches != NULL;
> > -       arches = gdbarch_list_lookup_by_info (arches->next, &info))
> > -    {
> > -      tdep = gdbarch_tdep (arches->gdbarch);
> > -      if (!tdep)
> > -	continue;
> > -      if (tdep->abi != tdep_abi)
> > -	continue;
> > -      if (tdep->vector_abi != vector_abi)
> > -	continue;  
> 
> Is it possible that we have two instances of gdbarch, with the same
> target description, but different vector_abi?  Two different executables
> compiled with different vector abis, and GDB can debug them together
> (multi-process debugging.  If we consider multi-target debugging in the
> future, these two executable can from different targets).

For s390 there only is one vector abi (or non at all) at the time.  If you were
debugging two different executables at the same time you would have two
inferiors each with its own gdbarch (same would be for multi-target debugging).
So I don't think those are the reasons.

For me it more looks like a mix of copy&paste code combined with an improper
clean up when the gdbarch_info->tdesc field was introduced.  This pattern was
introduced in 2000 (7a78ae4e6b0) for ppc/rs6k (in rs6000-tdep.c) and copy&pasted
to s390 in 2010 (7803799a0fb).  Between the two commits in 2006 (424163ea152)
the tdesc field (with the corresponding check) was added to gdbarch_info
without cleaning up the uses of gdbarch_list_lookup_by_info.  Unfortunately
none of the commits have a commit message and the ChangeLog isn't very
helpful...

Thanks
Philipp

> > -      if ((tdep->gpr_full_regnum != -1) != have_upper)
> > -	continue;
> > -      if (tdep->have_gs != have_gs)
> > -	continue;
> > -      if (tdesc_data != NULL)
> > -	tdesc_data_cleanup (tdesc_data);
> > -      return arches->gdbarch;
> > -    }  
>
  

Hi Yao,

I quickly talked to Uli yesterday about this and you and Uli are right.  There
is a possibility that a program chooses not to use the vector registers for
their abi even when they are present.  I fixed the patch locally.

Thanks for catching this!
Philipp


On Wed, 6 Dec 2017 11:28:23 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> Philipp Rudo wrote:
> 
> > On Tue, 05 Dec 2017 16:16:07 +0000
> > Yao Qi <qiyaoltc@gmail.com> wrote:  
> > > Is it possible that we have two instances of gdbarch, with the same
> > > target description, but different vector_abi?  Two different executables
> > > compiled with different vector abis, and GDB can debug them together
> > > (multi-process debugging.  If we consider multi-target debugging in the
> > > future, these two executable can from different targets).  
> > 
> > For s390 there only is one vector abi (or non at all) at the time.  If you were
> > debugging two different executables at the same time you would have two
> > inferiors each with its own gdbarch (same would be for multi-target debugging).
> > So I don't think those are the reasons.  
> 
> Actually, I think Yao is right here.  As you say, we can have two executables,
> one using the vector ABI and one not.  These will require two different gdbarch
> structures.  But with the patch you propose, when trying to allocate the second
> of those two, GDB would see the first one that was already created earlier,
> and incorrectly assume that it can simply be reused.
> 
> Basically, the problem is that there *can* be different gdbarchs that share
> the *same* tdesc, but differ in vector ABI.  Therefore *only* checking for
> tdesc does not suffice to correctly identify cached gdbarch structures.
> 
> I agree that it is redundant to again check differences (e.g. in register set)
> that would already have led to a different tdesc; but the vector ABI at least
> is not one of those.
> 
> Bye,
> Ulrich
>
  

On Thu, Dec 7, 2017 at 9:17 AM, Philipp Rudo <prudo@linux.vnet.ibm.com> wrote:
> Hi Yao,
>
> I quickly talked to Uli yesterday about this and you and Uli are right.  There
> is a possibility that a program chooses not to use the vector registers for
> their abi even when they are present.  I fixed the patch locally.
>
> Thanks for catching this!

Great, happy to help :)