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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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