[06/25] Generate c for feature instead of tdesc

  This patch changes Makefile and command "maint print c-files" so
that GDB can print c files for features instead target description.
Previously, we feed GDB a target description xml file, which generate
c files including multiple features.

With this patch, in Makefile, we wrap each feature xml file, and
create a temp target description which include only one feature.
Then, adjust the target description printer for them, and print
a c function for each given feature, so that we can use these
c functions later to create target description in a flexible way.

Before GDB prints c-tdesc, we need to set tdesc.  However, this involves
some validations in each gdbarch on these target descriptions, and this
make troubles to generate c file for each target feature when we feed GDB
a target description only including an optional feature (without some
mandatory feature), GDB just reject it, and don't print the c file.

What we need here is to translate xml file to object target_desc, and
translate the object target_desc to c file.  We don't have to involve
gdbarch validation in this process at all, so I modify command
"maint print c-tdesc" to have an optional argument which is the filename
of xml target description.  If the file name is provided, parse the
xml target description, create target_desc object, and print c file from
it.

gdb:

2017-05-22  Yao Qi  <yao.qi@linaro.org>

	* features/Makefile (FEATURE_XMLFILES): New.
	(FEATURE_CFILES): New.
	New rules.
	* features/i386/32bit-avx.c: Generated.
 	* features/i386/32bit-avx512.c: Generated.
 	* features/i386/32bit-core.c: Generated.
 	* features/i386/32bit-linux.c: Generated.
 	* features/i386/32bit-mpx.c: Generated.
 	* features/i386/32bit-pkeys.c: Generated.
 	* features/i386/32bit-sse.c: Generated.
 	* target-descriptions.c: Include algorithm.
	(tdesc_element_visitor): Add method visit_end.
	(print_c_tdesc): Implement visit_end.
	(print_c_tdesc:: m_filename_after_features): Move it to
	protected.
	(print_c_feature): New class.
	(maint_print_c_tdesc_cmd): Use print_c_feature if XML file
	name starts with "i386/32bit-".

gdb/doc:

2017-06-08  Yao Qi  <yao.qi@linaro.org>

	* gdb.texinfo (Maintenance Commands): Document optional
	argument of "maint print c-tdesc".
---
 gdb/doc/gdb.texinfo              |  10 ++--
 gdb/features/Makefile            |  31 ++++++++++-
 gdb/features/i386/32bit-avx.c    |  23 ++++++++
 gdb/features/i386/32bit-avx512.c |  35 ++++++++++++
 gdb/features/i386/32bit-core.c   |  68 +++++++++++++++++++++++
 gdb/features/i386/32bit-linux.c  |  16 ++++++
 gdb/features/i386/32bit-mpx.c    |  53 ++++++++++++++++++
 gdb/features/i386/32bit-pkeys.c  |  16 ++++++
 gdb/features/i386/32bit-sse.c    |  76 ++++++++++++++++++++++++++
 gdb/target-descriptions.c        | 114 +++++++++++++++++++++++++++++++++++----
 10 files changed, 425 insertions(+), 17 deletions(-)
 create mode 100644 gdb/features/i386/32bit-avx.c
 create mode 100644 gdb/features/i386/32bit-avx512.c
 create mode 100644 gdb/features/i386/32bit-core.c
 create mode 100644 gdb/features/i386/32bit-linux.c
 create mode 100644 gdb/features/i386/32bit-mpx.c
 create mode 100644 gdb/features/i386/32bit-pkeys.c
 create mode 100644 gdb/features/i386/32bit-sse.c
  

> From: Yao Qi <qiyaoltc@gmail.com>
> Date: Mon, 12 Jun 2017 09:41:37 +0100
> 
> gdb/doc:
> 
> 2017-06-08  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.texinfo (Maintenance Commands): Document optional
> 	argument of "maint print c-tdesc".

Thanks.

> -@kindex maint print c-tdesc
> +@kindex maint print c-tdesc @r{[}@var{file}@r{]}
>  @item maint print c-tdesc
> -Print the current target description (@pxref{Target Descriptions}) as
> -a C source file.  The created source file can be used in @value{GDBN}
> -when an XML parser is not available to parse the description.
> +Print the target description (@pxref{Target Descriptions}) as
> +a C source file.  The target description is got from the optional
                                            ^^^^^^
"is produced", I guess?  Or maybe I don't understand what you mean by
"is got" here.

> +argument @var{file} or the current target description.

I think this should be described in the reverse order:

  By default, the target description is for the current target, but if
  the optional argument @var{file} is provided, that file is used to
  produce the description.

>                                                        The created
> +source file can be used in @value{GDBN} when an XML parser is not
> +available to parse the description.

I also have some clarification requests (and I do realize that the
original text had the same issues):

 . I think we should say a few words regarding the argument FILE: I
   don't believe _any_ file will do in this role, so we should tell
   what kind of files are expected here.
 . "The created source file can be used in GDB ..." -- used how?  I'd
   prefer that we said a sentence or tow about that as well, or had a
   cross-reference to where that is described.

Also, what about a NEWS entry?
  

Eli Zaretskii <eliz@gnu.org> writes:

>>                                                        The created
>> +source file can be used in @value{GDBN} when an XML parser is not
>> +available to parse the description.
>
> I also have some clarification requests (and I do realize that the
> original text had the same issues):
>
>  . I think we should say a few words regarding the argument FILE: I
>    don't believe _any_ file will do in this role, so we should tell
>    what kind of files are expected here.

It is an XML target description.

>  . "The created source file can be used in GDB ..." -- used how?  I'd
>    prefer that we said a sentence or tow about that as well, or had a
>    cross-reference to where that is described.

The created source file is built into GDB.  How is the doc and NEWS
entry below?  Note that I removed the last sentence "... when an XML
parser is not available to parse the description", because these created
c files are builtin target descriptions which are used too when an XML
parser is available.

@kindex maint print c-tdesc @r{[}@var{file}@r{]}
@item maint print c-tdesc
Print the target description (@pxref{Target Descriptions}) as
a C source file.  By default, the target description is for the current
target, but if the optional argument @var{file} is provided, that file
is used to produce the description.  The @var{file} is an XML document,
of the form described in @ref{Target Description Format}.  The created
source file is built in @value{GDBN}.

>
> Also, what about a NEWS entry?

* The "maintenance print c-tdesc" command now takes an optional
  argument which is the file name of XML target description.
  

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 13 Jun 2017 13:07:20 +0100
> 
> >  . "The created source file can be used in GDB ..." -- used how?  I'd
> >    prefer that we said a sentence or tow about that as well, or had a
> >    cross-reference to where that is described.
> 
> The created source file is built into GDB.

Sorry, I don't understand: built into GDB how?  The GDB in which the
user runs this command can then turn around and have the produced file
built into it?

> How is the doc and NEWS entry below?

OK, with two comments, see below.

> @kindex maint print c-tdesc @r{[}@var{file}@r{]}
> @item maint print c-tdesc
> Print the target description (@pxref{Target Descriptions}) as
> a C source file.  By default, the target description is for the current
> target, but if the optional argument @var{file} is provided, that file
> is used to produce the description.  The @var{file} is an XML document,
                                                      ^^
"should be"

> of the form described in @ref{Target Description Format}.  The created
> source file is built in @value{GDBN}.
                       ^^
"into"

> > Also, what about a NEWS entry?
> 
> * The "maintenance print c-tdesc" command now takes an optional
>   argument which is the file name of XML target description.

This is OK.

Thanks.
  

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, I don't understand: built into GDB how?  The GDB in which the
> user runs this command can then turn around and have the produced file
> built into it?

Yes,
We feed GDB the XML target description file, let GDB print the c file,
and then include these generated c files in *-tdep.c.  That is, in
gdb/features/Makefile, we have,

%.c: %.xml
        $(GDB) -nx -q -batch \
          -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
        sh ../../move-if-change $@.tmp $@

it generates gdb/features/*.c files from the corresponding .xml files.
They are included in gdb source, like, in gdb/i386-linux-tdep.c,

#include "features/i386/i386-linux.c"
#include "features/i386/i386-mmx-linux.c"
#include "features/i386/i386-mpx-linux.c"
#include "features/i386/i386-avx-mpx-linux.c"
#include "features/i386/i386-avx-linux.c"
#include "features/i386/i386-avx-avx512-linux.c"
#include "features/i386/i386-avx-mpx-avx512-pku-linux.c"
  

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 13 Jun 2017 16:31:16 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sorry, I don't understand: built into GDB how?  The GDB in which the
> > user runs this command can then turn around and have the produced file
> > built into it?
> 
> Yes,
> We feed GDB the XML target description file, let GDB print the c file,
> and then include these generated c files in *-tdep.c.

But that means the produced file can only be used by rebuilding GDB,
doesn't it?  If so, the text should make this clear, I think.

(In general, since GDB is a compiled program which cannot easily be
modified without rebuilding it, I wonder how useful this feature will
be.  But I'll yield to your expertise on that.)
  

Eli Zaretskii <eliz@gnu.org> writes:

> But that means the produced file can only be used by rebuilding GDB,
> doesn't it?  If so, the text should make this clear, I think.
>

Yes.  The last sentence is about it, "The created source file is built
into @value{GDBN}", but I didn't explicitly say GDB needs rebuild.

> (In general, since GDB is a compiled program which cannot easily be
> modified without rebuilding it, I wonder how useful this feature will
> be.  But I'll yield to your expertise on that.)

This command is only used by developers after they add or modify XML
target descriptions.  I don't think GDB users will use this command.
A question in general, do we really need to document these commands,
which are used for GDB development, in GDB user manual?  These
commands are "maint print c-tdesc", "maint check xml-descriptions"
(added by one patch in this series), and "maint selftest".  Can we
remove them from the user manual?
  

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Wed, 14 Jun 2017 17:21:36 +0100
> 
> > But that means the produced file can only be used by rebuilding GDB,
> > doesn't it?  If so, the text should make this clear, I think.
> >
> 
> Yes.  The last sentence is about it, "The created source file is built
> into @value{GDBN}", but I didn't explicitly say GDB needs rebuild.

I think we should say that.

> > (In general, since GDB is a compiled program which cannot easily be
> > modified without rebuilding it, I wonder how useful this feature will
> > be.  But I'll yield to your expertise on that.)
> 
> This command is only used by developers after they add or modify XML
> target descriptions.

Then I think we should say that as well.

> A question in general, do we really need to document these commands,
> which are used for GDB development, in GDB user manual?  These
> commands are "maint print c-tdesc", "maint check xml-descriptions"
> (added by one patch in this series), and "maint selftest".  Can we
> remove them from the user manual?

If we remove them, there will be no place where they are documented,
right?  GDB developers are GDB users as well, e.g. I read the manual
quite a lot.

So I see no reason to remove these commands.  We just need to indicate
when a command makes sense only for GDB developers.

Thanks.
  

Eli Zaretskii <eliz@gnu.org> writes:

>> Yes.  The last sentence is about it, "The created source file is built
>> into @value{GDBN}", but I didn't explicitly say GDB needs rebuild.
>
> I think we should say that.
>
>> > (In general, since GDB is a compiled program which cannot easily be
>> > modified without rebuilding it, I wonder how useful this feature will
>> > be.  But I'll yield to your expertise on that.)
>> 
>> This command is only used by developers after they add or modify XML
>> target descriptions.
>
> Then I think we should say that as well.
>

I update the paragraph for this command a little bit,

@kindex maint print c-tdesc @r{[}@var{file}@r{]}
@item maint print c-tdesc
Print the target description (@pxref{Target Descriptions}) as
a C source file.  By default, the target description is for the current
target, but if the optional argument @var{file} is provided, that file
is used to produce the description.  The @var{file} should be an XML
document, of the form described in @ref{Target Description Format}.
The created source file is built into @value{GDBN} when @value{GDBN} is
built again.  This command is used by developers after they add or
modify XML target descriptions.

>> A question in general, do we really need to document these commands,
>> which are used for GDB development, in GDB user manual?  These
>> commands are "maint print c-tdesc", "maint check xml-descriptions"
>> (added by one patch in this series), and "maint selftest".  Can we
>> remove them from the user manual?
>
> If we remove them, there will be no place where they are documented,
> right?  GDB developers are GDB users as well, e.g. I read the manual
> quite a lot.
>
> So I see no reason to remove these commands.  We just need to indicate
> when a command makes sense only for GDB developers.

but GDB users are not GDB developers.  Why does user manual documents
some commands users never use?  IMO, these documents should be
documented in GDB internals.
  

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: gdb-patches@sourceware.org
> Date: Thu, 15 Jun 2017 14:19:25 +0100
> 
> I update the paragraph for this command a little bit,
> 
> @kindex maint print c-tdesc @r{[}@var{file}@r{]}
> @item maint print c-tdesc
> Print the target description (@pxref{Target Descriptions}) as
> a C source file.  By default, the target description is for the current
> target, but if the optional argument @var{file} is provided, that file
> is used to produce the description.  The @var{file} should be an XML
> document, of the form described in @ref{Target Description Format}.
> The created source file is built into @value{GDBN} when @value{GDBN} is
> built again.  This command is used by developers after they add or
> modify XML target descriptions.

Thanks, this is good.

> > If we remove them, there will be no place where they are documented,
> > right?  GDB developers are GDB users as well, e.g. I read the manual
> > quite a lot.
> >
> > So I see no reason to remove these commands.  We just need to indicate
> > when a command makes sense only for GDB developers.
> 
> but GDB users are not GDB developers.  Why does user manual documents
> some commands users never use?

Because I expect GDB developers to look for commands they need to use
in this manual.

> IMO, these documents should be documented in GDB internals.

AFAIU, GDB internals documents the GDB implementation, not commands
and options that are used interactively within GDB.
  

(this is not a full review)

On 06/12/2017 09:41 AM, Yao Qi wrote:
> +$(FEATURE_CFILES): %.c: %.xml.tmp
> +	$(GDB) -nx -q -batch \
> +	  -ex 'maint print c-tdesc $<' > $@.tmp
> +	sh ../../move-if-change $@.tmp $@
> +	rm $<
> +
> +%.xml.tmp: %.xml
> +	echo "<?xml version=\"1.0\"?>" > $@
> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
> +	echo "<target>" >> $@
> +	echo "  <architecture>" >> $@
> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;
> +	echo "  </architecture>" >> $@
> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
> +	echo "</target>" >> $@
> +

Don't we need move-if-change here?

The findstring bits warrants a comment.

> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
> +  Original: 32bit-avx512.xml.tmp */

I don't think we should be pointing "Original" at a 
temporary file that does not exist in the repo?

On 06/12/2017 09:41 AM, Yao Qi wrote:
> -  print_c_tdesc v (filename_after_features);
> +  if (strncmp (filename_after_features.c_str(), "i386/32bit-", 11) == 0)

startswith

But again, this looks like needs at least a comment.  I'd
like to see an expanded rationale for this.  It's not
immediately obvious why/what's this special casing for.

> +    {
> +      print_c_feature v (filename_after_features);
> +
> +      tdesc->accept (v);
> +    }
> +  else
> +    {
> +      print_c_tdesc v (filename_after_features);
>  
> -  tdesc->accept (v);
> +      tdesc->accept (v);
> +    }


Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> On 06/12/2017 09:41 AM, Yao Qi wrote:
>> +$(FEATURE_CFILES): %.c: %.xml.tmp
>> +	$(GDB) -nx -q -batch \
>> +	  -ex 'maint print c-tdesc $<' > $@.tmp
>> +	sh ../../move-if-change $@.tmp $@
>> +	rm $<
>> +
>> +%.xml.tmp: %.xml
>> +	echo "<?xml version=\"1.0\"?>" > $@
>> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
>> +	echo "<target>" >> $@
>> +	echo "  <architecture>" >> $@
>> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;
>> +	echo "  </architecture>" >> $@
>> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
>> +	echo "</target>" >> $@
>> +
>
> Don't we need move-if-change here?
>

move-if-change from what to what?  *.xml.tmp is removed after *.c is
generated.

> The findstring bits warrants a comment.
>

Yes, comment is needed.

>> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
>> +  Original: 32bit-avx512.xml.tmp */
>
> I don't think we should be pointing "Original" at a 
> temporary file that does not exist in the repo?

Fixed locally.

>
> On 06/12/2017 09:41 AM, Yao Qi wrote:
>> -  print_c_tdesc v (filename_after_features);
>> +  if (strncmp (filename_after_features.c_str(), "i386/32bit-", 11) == 0)
>
> startswith
>
> But again, this looks like needs at least a comment.  I'd
> like to see an expanded rationale for this.  It's not
> immediately obvious why/what's this special casing for.

OK.  Note that the special case and the findstring in Makefile are
needed during the target description transition.  Once we move all
targets to the flexible target description, we don't need them.
  

On 06/22/2017 03:49 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> > On 06/12/2017 09:41 AM, Yao Qi wrote:
>>> >> +$(FEATURE_CFILES): %.c: %.xml.tmp
>>> >> +	$(GDB) -nx -q -batch \
>>> >> +	  -ex 'maint print c-tdesc $<' > $@.tmp
>>> >> +	sh ../../move-if-change $@.tmp $@
>>> >> +	rm $<
>>> >> +
>>> >> +%.xml.tmp: %.xml
>>> >> +	echo "<?xml version=\"1.0\"?>" > $@
>>> >> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
>>> >> +	echo "<target>" >> $@
>>> >> +	echo "  <architecture>" >> $@
>>> >> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;
>>> >> +	echo "  </architecture>" >> $@
>>> >> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
>>> >> +	echo "</target>" >> $@
>>> >> +
>> >
>> > Don't we need move-if-change here?
>> >
> move-if-change from what to what?  *.xml.tmp is removed after *.c is
> generated.

"*.xml.tmp.tmp" to "*.xml.tmp".  I.e., from $@.tmp to $@, just
like the rule further above.

I may be missing something, but AFAICS, the "%.xml.tmp: %.xml"
rule is writing to the target directly.  If the build
is interrupted in the middle of that rule, we end up with an
incomplete "%.xml.tmp" file left over in the file system.
Then the next time you invoke make, make will think that
the "%.xml.tmp" file is already up to date and won't rebuild it.
As a result, you'll pass a broken xml.tmp file to
"maint print c-tdesc".

Avoiding these issues is the whole point of move-if-change,
by atomically creating the target file.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> "*.xml.tmp.tmp" to "*.xml.tmp".  I.e., from $@.tmp to $@, just
> like the rule further above.
>

Oh, I know what you mean.

> I may be missing something, but AFAICS, the "%.xml.tmp: %.xml"
> rule is writing to the target directly.  If the build
> is interrupted in the middle of that rule, we end up with an
> incomplete "%.xml.tmp" file left over in the file system.
> Then the next time you invoke make, make will think that
> the "%.xml.tmp" file is already up to date and won't rebuild it.
> As a result, you'll pass a broken xml.tmp file to
> "maint print c-tdesc".
>
> Avoiding these issues is the whole point of move-if-change,
> by atomically creating the target file.

move-if-change is needed here.  Let me add it.
  

On 2017-06-12 10:41, Yao Qi wrote:
> diff --git a/gdb/features/Makefile b/gdb/features/Makefile
> index 3bc8b5a..28a7e20 100644
> --- a/gdb/features/Makefile
> +++ b/gdb/features/Makefile
> @@ -252,12 +252,39 @@ $(outdir)/%.dat: %.xml number-regs.xsl
> sort-regs.xsl gdbserver-regs.xsl
>  	  $(XSLTPROC) gdbserver-regs.xsl - >> $(outdir)/$*.tmp
>  	sh ../../move-if-change $(outdir)/$*.tmp $(outdir)/$*.dat
> 
> -cfiles: $(CFILES)
> -%.c: %.xml
> +FEATURE_XMLFILES = i386/32bit-core.xml \
> +	i386/32bit-sse.xml \
> +	i386/32bit-linux.xml \
> +	i386/32bit-avx.xml \
> +	i386/32bit-mpx.xml \
> +	i386/32bit-avx512.xml \
> +	i386/32bit-pkeys.xml
> +
> +FEATURE_CFILES = $(patsubst %.xml,%.c,$(FEATURE_XMLFILES))
> +
> +cfiles: $(CFILES) $(FEATURE_CFILES)

If we are going to have different kinds of CFILES, it would be nice to 
rename CFILES (to TDESC_CFILES I suppose) to clarify the purpose of each 
variable.

> +
> +$(CFILES): %.c: %.xml
>  	$(GDB) -nx -q -batch \
>  	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
>  	sh ../../move-if-change $@.tmp $@
> 
> +$(FEATURE_CFILES): %.c: %.xml.tmp
> +	$(GDB) -nx -q -batch \
> +	  -ex 'maint print c-tdesc $<' > $@.tmp
> +	sh ../../move-if-change $@.tmp $@
> +	rm $<
> +
> +%.xml.tmp: %.xml
> +	echo "<?xml version=\"1.0\"?>" > $@
> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
> +	echo "<target>" >> $@
> +	echo "  <architecture>" >> $@
> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;

Should this be

   test -n "$(findstring ...)"

Quotes would be important here, if findstring returns en empty string,

> +	echo "  </architecture>" >> $@
> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
> +	echo "</target>" >> $@
> +

Instead of creating this temporary file, wouldn't it be simpler to make 
"maint print c-tdesc" (or a new "maint print c-feature") take directly a 
path to a feature XML, and generate the C from that?
> @@ -2033,38 +2040,123 @@ public:
>      printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type);
>    }
> 
> +protected:
> +  std::string m_filename_after_features;
> +
>  private:
>    char *m_function;
> -  std::string m_filename_after_features;
>    bool m_printed_field_type = false;
>    bool m_printed_type = false;
>  };
> 
> +class print_c_feature : public print_c_tdesc

I am really not convinced that such an inheritance is appropriate here.  
To make print_c_feature inherit from print_c_tdesc, we should be able to 
say that a print_c_feature object _is_ a print_c_tdesc with further 
specialization.  I don't think it's the case here.  If they can share 
some code, I think it would be appropriate for them to have a common 
base class though.

Simon
  

Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +FEATURE_CFILES = $(patsubst %.xml,%.c,$(FEATURE_XMLFILES))
>> +
>> +cfiles: $(CFILES) $(FEATURE_CFILES)
>
> If we are going to have different kinds of CFILES, it would be nice to
> rename CFILES (to TDESC_CFILES I suppose) to clarify the purpose of
> each variable.
>

OK, I'll rename CFILES.

>> +
>> +$(CFILES): %.c: %.xml
>>  	$(GDB) -nx -q -batch \
>>  	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
>>  	sh ../../move-if-change $@.tmp $@
>>
>> +$(FEATURE_CFILES): %.c: %.xml.tmp
>> +	$(GDB) -nx -q -batch \
>> +	  -ex 'maint print c-tdesc $<' > $@.tmp
>> +	sh ../../move-if-change $@.tmp $@
>> +	rm $<
>> +
>> +%.xml.tmp: %.xml
>> +	echo "<?xml version=\"1.0\"?>" > $@
>> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
>> +	echo "<target>" >> $@
>> +	echo "  <architecture>" >> $@
>> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;
>
> Should this be
>
>   test -n "$(findstring ...)"
>
> Quotes would be important here, if findstring returns en empty string,
>

I update the code here, and find the "test findstring" is no longer needed.

>> +	echo "  </architecture>" >> $@
>> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
>> +	echo "</target>" >> $@
>> +
>
> Instead of creating this temporary file, wouldn't it be simpler to
> make "maint print c-tdesc" (or a new "maint print c-feature") take
> directly a path to a feature XML, and generate the C from that?

Actually, I thought about adding a new command, but I find adding temp
file is simpler than modifying GDB to add a new command and accept a
feature XML.  It is just some lines of code in Makefile, with the "test"
"findstring" stuff removed, they are quite simple.

>> @@ -2033,38 +2040,123 @@ public:
>>      printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type);
>>    }
>>
>> +protected:
>> +  std::string m_filename_after_features;
>> +
>>  private:
>>    char *m_function;
>> -  std::string m_filename_after_features;
>>    bool m_printed_field_type = false;
>>    bool m_printed_type = false;
>>  };
>>
>> +class print_c_feature : public print_c_tdesc
>
> I am really not convinced that such an inheritance is appropriate
> here.  To make print_c_feature inherit from print_c_tdesc, we should
> be able to say that a print_c_feature object _is_ a print_c_tdesc with
> further specialization.  I don't think it's the case here.  If they

No, there is no is-a relationship between print_c_feature and
print_c_tdesc, so it is inheritance, not subtying.

https://en.wikipedia.org/wiki/Inheritance_(object-oriented_programming)

"Inheritance should not be confused with subtyping....in general,
subtyping establishes an is-a relationship, whereas inheritance only
reuses implementation and establishes a syntactic relationship..."

and

"In object-oriented programming, inheritance is when an object or class
is based on another object (prototypal inheritance) or class
(class-based inheritance), using the same implementation (...) or
specifying a new implementation to maintain the same behavior (...)",

> can share some code, I think it would be appropriate for them to have
> a common base class though.