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

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

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;
> > -    }  
>