This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 06/25] Generate c for feature instead of tdesc
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Tue, 20 Jun 2017 11:59:46 +0100
- Subject: Re: [PATCH 06/25] Generate c for feature instead of tdesc
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C221B19D37D
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C221B19D37D
- References: <1497256916-4958-1-git-send-email-yao.qi@linaro.org> <1497256916-4958-7-git-send-email-yao.qi@linaro.org>
(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