Re: [PATCH v2 6/8] Create xml from target descriptions

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

On Thu, 25 Jan 2018 16:13:16 +0000
Alan Hayward <Alan.Hayward@arm.com> wrote:

> > On 25 Jan 2018, at 13:14, Philipp Rudo <prudo@linux.vnet.ibm.com> wrote:
> > 
> > 
> > i don't really like print_xml_feature::visit_pre being declared in arch/tdesc.h
> > with different implementations in target-descriptions.c and gdbserver/tdesc.c
> > while all other visit_* implementations are in arch/tdesc.c.  I would prefere
> > visit_pre being implemented in arch/tdesc.c, too.  Even when it means to add
> > some #ifdef GDBSERVER ... #else ... #endif blocks until there is a common tdesc.  
> 
> Thanks for the review!
> 
> A common print_xml_feature would require 4 ifdefs, which would be too messy.
> I’ll take a look see if I can communise the tdesc_architecture and tdesc_osabi
> interfaces - it should be fairly simple to do.
> Once done, I should be able to add a common print_xml_feature without any
> ifdefs. Hopefully.
> 

Having a common interface would be even better :)

> 
> > On 25 Jan 2018, at 15:44, Yao Qi <qiyaoltc@gmail.com> wrote:
> > 
> > Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
> >   
> >> Same for tdesc_type::make_gdb_type (patch #5).  But here i would prefer to not
> >> even declare the method for GDBserver, i.e.
> >> 
> >> struct tdesc_type
> >> {
> >> 
> >>  [...]
> >> 
> >> #ifndef GDBSERVER
> >>  virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0;
> >> #endif
> >> };
> >> 
> >> The problem i see with implementing stubs calling error is that you cannot find
> >> out you made a mistake until you call the function during run-time.  This gives
> >> room to nasty bugs which could easily be prevented when there is a compile bug.  
> > 
> > 
> > make_gdb_type and gdbarch shouldn't be put into arch/tdesc.h at all, if
> > possible.  You can create an sub-class of tdesc_element_visitor in gdb
> > side, and create the gdb type by visiting these elements, like this,
> > 
> > class gdb_type_creator : public tdesc_element_visitor  
> ….
> 
> Nice idea. I can add an extra patch into the series to do this.

Yeah, Yao's solution is better than mine.

Oh i forgot to tell you, great series.

Philipp