aarch64: Restrict aarch64-tune.md regeneration to --enable-maintainer-mode [PR105144]

  Hi!

Normally updates to the source directory files are guarded with
--enable-maintainer-mode, e.g. we don't regenerate configure, config.h,
Makefile.in in directories that use automake etc. unless gcc is configured
that way.  Otherwise the source tree can't be e.g. stored on a read-only
filesystem etc.
In gcc/Makefile.in we use @MAINT@ for that but that works because
gcc/Makefile is generated by configure.  In config/*/t-* files we need to
check $(ENABLE_MAINTAINER_RULES):
# The following provides the variable ENABLE_MAINTAINER_RULES that can
# be used in language Make-lang.in makefile fragments to enable
# maintainer rules.  So, ENABLE_MAINTAINER_RULES is 'true' in
# maintainer mode, and '' otherwise.
@MAINT@ ENABLE_MAINTAINER_RULES = true

This is incremental patch does that, tested again on aarch64-linux and
x86_64-linux (cross in that case), ok for trunk?

2022-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR target/105144
	* config/aarch64/t-aarch64 ($(srcdir)/config/aarch64/aarch64-tune.md,
	s-aarch64-tune-md, s-mddeps): Only enable the rules if
	$(ENABLE_MAINTAINER_RULES) is non-empty.


	Jakub
  

Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> Normally updates to the source directory files are guarded with
> --enable-maintainer-mode, e.g. we don't regenerate configure, config.h,
> Makefile.in in directories that use automake etc. unless gcc is configured
> that way.  Otherwise the source tree can't be e.g. stored on a read-only
> filesystem etc.
> In gcc/Makefile.in we use @MAINT@ for that but that works because
> gcc/Makefile is generated by configure.  In config/*/t-* files we need to
> check $(ENABLE_MAINTAINER_RULES):
> # The following provides the variable ENABLE_MAINTAINER_RULES that can
> # be used in language Make-lang.in makefile fragments to enable
> # maintainer rules.  So, ENABLE_MAINTAINER_RULES is 'true' in
> # maintainer mode, and '' otherwise.
> @MAINT@ ENABLE_MAINTAINER_RULES = true
>
> This is incremental patch does that, tested again on aarch64-linux and
> x86_64-linux (cross in that case), ok for trunk?
>
> 2022-04-04  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/105144
> 	* config/aarch64/t-aarch64 ($(srcdir)/config/aarch64/aarch64-tune.md,
> 	s-aarch64-tune-md, s-mddeps): Only enable the rules if
> 	$(ENABLE_MAINTAINER_RULES) is non-empty.

OK.  But I guess the risk is that it will become even easier to forget
to commit an updated aarch64-tune.md.  Perhaps we should have a
non-maintainer rule to build aarch64-tune.md locally and check it
against the source-directory version, and fail the build if there's
a mismatch.  Or maybe we should just generate aarch64-tune.md in the
build directory and remove the source directory version.

That's all future work though.  The patch is still an improvement
of the status quo.

Thanks,
Richard

>
> --- gcc/config/aarch64/t-aarch64.jj	2022-04-04 10:14:30.256323070 +0200
> +++ gcc/config/aarch64/t-aarch64	2022-04-04 10:32:55.591651822 +0200
> @@ -24,6 +24,7 @@ OPTIONS_H_EXTRA += $(srcdir)/config/aarc
>  		   $(srcdir)/config/aarch64/aarch64-fusion-pairs.def \
>  		   $(srcdir)/config/aarch64/aarch64-tuning-flags.def
>  
> +ifneq ($(strip $(ENABLE_MAINTAINER_RULES)),)
>  $(srcdir)/config/aarch64/aarch64-tune.md: s-aarch64-tune-md; @true
>  s-aarch64-tune-md: $(srcdir)/config/aarch64/gentune.sh \
>  	$(srcdir)/config/aarch64/aarch64-cores.def
> @@ -35,6 +36,7 @@ s-aarch64-tune-md: $(srcdir)/config/aarc
>  	$(STAMP) s-aarch64-tune-md
>  
>  s-mddeps: s-aarch64-tune-md
> +endif
>  
>  aarch64-builtins.o: $(srcdir)/config/aarch64/aarch64-builtins.cc $(CONFIG_H) \
>    $(SYSTEM_H) coretypes.h $(TM_H) \
>
> 	Jakub
  

On Mon, Apr 04, 2022 at 11:10:14AM +0100, Richard Sandiford wrote:
> > Normally updates to the source directory files are guarded with
> > --enable-maintainer-mode, e.g. we don't regenerate configure, config.h,
> > Makefile.in in directories that use automake etc. unless gcc is configured
> > that way.  Otherwise the source tree can't be e.g. stored on a read-only
> > filesystem etc.
> > In gcc/Makefile.in we use @MAINT@ for that but that works because
> > gcc/Makefile is generated by configure.  In config/*/t-* files we need to
> > check $(ENABLE_MAINTAINER_RULES):
> > # The following provides the variable ENABLE_MAINTAINER_RULES that can
> > # be used in language Make-lang.in makefile fragments to enable
> > # maintainer rules.  So, ENABLE_MAINTAINER_RULES is 'true' in
> > # maintainer mode, and '' otherwise.
> > @MAINT@ ENABLE_MAINTAINER_RULES = true
> >
> > This is incremental patch does that, tested again on aarch64-linux and
> > x86_64-linux (cross in that case), ok for trunk?
> >
> > 2022-04-04  Jakub Jelinek  <jakub@redhat.com>
> >
> > 	PR target/105144
> > 	* config/aarch64/t-aarch64 ($(srcdir)/config/aarch64/aarch64-tune.md,
> > 	s-aarch64-tune-md, s-mddeps): Only enable the rules if
> > 	$(ENABLE_MAINTAINER_RULES) is non-empty.
> 
> OK.  But I guess the risk is that it will become even easier to forget
> to commit an updated aarch64-tune.md.  Perhaps we should have a
> non-maintainer rule to build aarch64-tune.md locally and check it
> against the source-directory version, and fail the build if there's
> a mismatch.  Or maybe we should just generate aarch64-tune.md in the
> build directory and remove the source directory version.

I've tried if aarch64-tune.md will be read from the build dir, but it is
not.  The gen* files can use -I options to add additional directories, but
they don't use them.

Here is a variant patch which will complain and fail if there is a change
and --enable-maintainer-mode is not enabled:

2022-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR target/105144
	* config/aarch64/t-aarch64 (s-aarch64-tune-md): Do move-if-change
	only if configured with --enable-maintainer-mode, otherwise compare
	tmp-aarch64-tune.md with $(srcdir)/config/aarch64/aarch64-tune.md and
	if they differ, emit a message and fail.

--- gcc/config/aarch64/t-aarch64.jj	2022-04-04 12:09:18.530859281 +0200
+++ gcc/config/aarch64/t-aarch64	2022-04-04 12:44:35.878930189 +0200
@@ -30,8 +30,18 @@ s-aarch64-tune-md: $(srcdir)/config/aarc
 	$(SHELL) $(srcdir)/config/aarch64/gentune.sh \
 		$(srcdir)/config/aarch64/aarch64-cores.def > \
 		tmp-aarch64-tune.md
+ifneq ($(strip $(ENABLE_MAINTAINER_RULES)),)
 	$(SHELL) $(srcdir)/../move-if-change tmp-aarch64-tune.md \
 		$(srcdir)/config/aarch64/aarch64-tune.md
+else
+	@if ! cmp -s tmp-aarch64-tune.md \
+	  $(srcdir)/config/aarch64/aarch64-tune.md; then \
+	  echo "aarch64-tune.md has changed; either"; \
+	  echo "configure with --enable-maintainer-mode"; \
+	  echo "or copy tmp-aarch64-tune.md to $(srcdir)/config/aarch64/aarch64-tune.md"; \
+	  exit 1; \
+	fi
+endif
 	$(STAMP) s-aarch64-tune-md
 
 s-mddeps: s-aarch64-tune-md


	Jakub
  

On 04/04/2022 11:49, Jakub Jelinek via Gcc-patches wrote:
> On Mon, Apr 04, 2022 at 11:10:14AM +0100, Richard Sandiford wrote:
>>> Normally updates to the source directory files are guarded with
>>> --enable-maintainer-mode, e.g. we don't regenerate configure, config.h,
>>> Makefile.in in directories that use automake etc. unless gcc is configured
>>> that way.  Otherwise the source tree can't be e.g. stored on a read-only
>>> filesystem etc.
>>> In gcc/Makefile.in we use @MAINT@ for that but that works because
>>> gcc/Makefile is generated by configure.  In config/*/t-* files we need to
>>> check $(ENABLE_MAINTAINER_RULES):
>>> # The following provides the variable ENABLE_MAINTAINER_RULES that can
>>> # be used in language Make-lang.in makefile fragments to enable
>>> # maintainer rules.  So, ENABLE_MAINTAINER_RULES is 'true' in
>>> # maintainer mode, and '' otherwise.
>>> @MAINT@ ENABLE_MAINTAINER_RULES = true
>>>
>>> This is incremental patch does that, tested again on aarch64-linux and
>>> x86_64-linux (cross in that case), ok for trunk?
>>>
>>> 2022-04-04  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR target/105144
>>> 	* config/aarch64/t-aarch64 ($(srcdir)/config/aarch64/aarch64-tune.md,
>>> 	s-aarch64-tune-md, s-mddeps): Only enable the rules if
>>> 	$(ENABLE_MAINTAINER_RULES) is non-empty.
>>
>> OK.  But I guess the risk is that it will become even easier to forget
>> to commit an updated aarch64-tune.md.  Perhaps we should have a
>> non-maintainer rule to build aarch64-tune.md locally and check it
>> against the source-directory version, and fail the build if there's
>> a mismatch.  Or maybe we should just generate aarch64-tune.md in the
>> build directory and remove the source directory version.
> 
> I've tried if aarch64-tune.md will be read from the build dir, but it is
> not.  The gen* files can use -I options to add additional directories, but
> they don't use them.
> 
> Here is a variant patch which will complain and fail if there is a change
> and --enable-maintainer-mode is not enabled:
> 
> 2022-04-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/105144
> 	* config/aarch64/t-aarch64 (s-aarch64-tune-md): Do move-if-change
> 	only if configured with --enable-maintainer-mode, otherwise compare
> 	tmp-aarch64-tune.md with $(srcdir)/config/aarch64/aarch64-tune.md and
> 	if they differ, emit a message and fail.
> 
> --- gcc/config/aarch64/t-aarch64.jj	2022-04-04 12:09:18.530859281 +0200
> +++ gcc/config/aarch64/t-aarch64	2022-04-04 12:44:35.878930189 +0200
> @@ -30,8 +30,18 @@ s-aarch64-tune-md: $(srcdir)/config/aarc
>   	$(SHELL) $(srcdir)/config/aarch64/gentune.sh \
>   		$(srcdir)/config/aarch64/aarch64-cores.def > \
>   		tmp-aarch64-tune.md
> +ifneq ($(strip $(ENABLE_MAINTAINER_RULES)),)
>   	$(SHELL) $(srcdir)/../move-if-change tmp-aarch64-tune.md \
>   		$(srcdir)/config/aarch64/aarch64-tune.md
> +else
> +	@if ! cmp -s tmp-aarch64-tune.md \
> +	  $(srcdir)/config/aarch64/aarch64-tune.md; then \
> +	  echo "aarch64-tune.md has changed; either"; \
> +	  echo "configure with --enable-maintainer-mode"; \
> +	  echo "or copy tmp-aarch64-tune.md to $(srcdir)/config/aarch64/aarch64-tune.md"; \
> +	  exit 1; \
> +	fi
> +endif
>   	$(STAMP) s-aarch64-tune-md
>   
>   s-mddeps: s-aarch64-tune-md
> 
> 
> 	Jakub
> 

OK.

I think we have a similar issue for arm with arm-tune.md and 
arm-tables.opt.  Perhaps we should adopt a similar approach for those as 
well.

R.
  

On Mon, Apr 04, 2022 at 12:32:27PM +0100, Richard Earnshaw via Gcc-patches wrote:
> OK.

Thanks, now committed.

> I think we have a similar issue for arm with arm-tune.md and arm-tables.opt.
> Perhaps we should adopt a similar approach for those as well.

From what I can see, both arm and c6x suffer from the point 3) in the PR,
i.e. they regenerate files in the source tree regardless of
--enable-maintainer-mode.
As for 2), both arm and c6x are ok, but handle it in a different way from
what I did (s-mddeps dependency addition) - they instead set
MD_INCLUDES = long-list-of-*.md-files
s-config s-conditions s-flags s-codes s-constants s-emit s-recog s-preds \
        s-opinit s-extract s-peep s-attr s-attrtab s-output: $(MD_INCLUDES)
The MD_INCLUDES variable is overwritten later if mddeps.mk exists and
is included, so the one in t-arm etc. doesn't need to be accurate and can
just contain the files that are generated.  The MD_INCLUDES approach
has the disadvantage that people will try to add stuff to it even when
it isn't needed.
rs6000 is the only target that uses MD_INCLUDES beyond arm and c6x, but
in that case the rule to regenerate it is commented out (should that be
enabled for maintainer mode?).

	Jakub
  

On 04/04/2022 13:12, Jakub Jelinek via Gcc-patches wrote:
> On Mon, Apr 04, 2022 at 12:32:27PM +0100, Richard Earnshaw via Gcc-patches wrote:
>> OK.
> 
> Thanks, now committed.
> 
>> I think we have a similar issue for arm with arm-tune.md and arm-tables.opt.
>> Perhaps we should adopt a similar approach for those as well.
> 
>  From what I can see, both arm and c6x suffer from the point 3) in the PR,
> i.e. they regenerate files in the source tree regardless of
> --enable-maintainer-mode.

Well the read-only tree issue will simply result in a build failure if 
the file needs to change - I don't see that as a major issue.

The only risk is if multiple builds are running from the same sources at 
the same time and you somehow end up with a race condition.

> As for 2), both arm and c6x are ok, but handle it in a different way from
> what I did (s-mddeps dependency addition) - they instead set
> MD_INCLUDES = long-list-of-*.md-files
> s-config s-conditions s-flags s-codes s-constants s-emit s-recog s-preds \
>          s-opinit s-extract s-peep s-attr s-attrtab s-output: $(MD_INCLUDES)
> The MD_INCLUDES variable is overwritten later if mddeps.mk exists and
> is included, so the one in t-arm etc. doesn't need to be accurate and can
> just contain the files that are generated.  The MD_INCLUDES approach
> has the disadvantage that people will try to add stuff to it even when
> it isn't needed.
> rs6000 is the only target that uses MD_INCLUDES beyond arm and c6x, but
> in that case the rule to regenerate it is commented out (should that be
> enabled for maintainer mode?).
> 
> 	Jakub
> 

Really the long-term solution is to fix these so that both the md 
reading machinery and the opt machinery can read generated files from 
the build directories, then we wouldn't need to copy anything other than 
config files back to the source tree.

R.