(build) Patch to fix cp/cfns.gperf building issues

(build) Patch to fix cp/cfns.gperf building issues

[Nicola Pero]

This patch fixes a building annoyance that I had when building on a new
machine (an x86_64 gnu/linux box).

The building failed.  It was down to two problems:

 * due to how I got a copy of the GCC source code on the machine, the timestamp
of each source file was the timestamp of when it was copied to the machine (ie,
a random timestamp from the point of view of the building system).  This caused
the build system to decide that $(srcdir)/cp/cfns.h needed to be rebuilt from
$(srcdir)/cp/cfns.gperf (it didn't, obviously; the source code was trunk with
no changes).

 * when the automated rebuild failed because gperf was not there, it also created
an empty cp/cfns.h, basically corrupting my source tree.  Further attempts to
rebuild (with or without gperf) would then fail due to a missing definition of
"libc_name_p" (because cp/cfns.h now existed, and was newer than cp/cfns.gperf,
it wouldn't be rebuilt, and I was stuck with the empty cp/cfns.h).  Searching
on the internet, I found that a number of people got stuck at this stage and
couldn't finish their build.  So, rather than just manually fixing my source
tree, I'm posting a patch that will remove the problems at the root.

This patch fixes both problems, making the build more robust:

 * it only rebuilds cp/cfns.h when a specific command ("make rebuild-cp-cfns") is used.
   According to the ChangeLogs, cp/cfns.gperf was edited about 5 times in 10 years,
   so it does not seem unreasonable to require developers/maintainers to use an explicit
   command to rebuild cp/cfns.h from cp/cfns.gperf for the once-every-two-years occasion when
   it is changed.  I documented the process clearly in cfns.gperf itself.
   This makes the build more robust for end users, who are not bugged by timestamp
   issues in the source tree causing unexpected rebuilds or complications requiring
   gperf.
   In general, I personally feel that the building system should not depend on the relative
   timestamps of source files unless it's doing something in "maintainer mode" where it's being
   explicitly asked to rebuild one source file from the other.  The average user who downloads
   the GCC source code to build it should not be expected to keep the timestamps of each
   source file religiously unchanged!  Timestamps may change as the source code is moved
   around by users and the building system should not have a problem with that.

 * it fails gracefully when the rebuild is triggered and gperf is not available.  Instead
   of creating en empty cp/cfns.h file, it creates no file at all, so that if you then install
   gperf, it will rebuild cp/cfns.h correctly, and if you don't, you'll keep getting the
   error the gperf is not available (not an error about libc_name_p not being defined),
   which is the correct one.

Ok to commit ?

Thanks

Index: cfns.gperf
===================================================================
--- cfns.gperf  (revision 172858)
+++ cfns.gperf  (working copy)
@@ -15,7 +15,18 @@ for more details.
 
 You should have received a copy of the GNU General Public License
 along with GCC; see the file COPYING3.  If not see
-<http://www.gnu.org/licenses/>.  */
+<http://www.gnu.org/licenses/>.
+
+You need to use gperf to generate cfns.h from cfns.gperf.  This is
+the process:
+
+ have a GCC source/build tree already checked out
+ edit cfns.gperf
+ cd $(buildir)/gcc
+ make rebuild-cp-cfns
+
+Alternatively, just look into cp/Make-lang.in and run the gperf
+command in the rebuild-cp-cfns rule manually.  */
 #ifdef __GNUC__
 __inline
 #endif
Index: Make-lang.in
===================================================================
--- Make-lang.in        (revision 172858)
+++ Make-lang.in        (working copy)
@@ -104,11 +104,19 @@ cc1plus$(exeext): $(CXX_OBJS) cc1plus-checksum.o $
        +$(LINKER) $(ALL_LINKERFLAGS) $(LDFLAGS) -o $@ \
              $(CXX_OBJS) cc1plus-checksum.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
 
-# Special build rules.
-$(srcdir)/cp/cfns.h: $(srcdir)/cp/cfns.gperf
+# Special build rule.  This is a maintainer rule, that needs to be
+# invoked manually (as in "cd $(builddir)/gcc; make rebuild-cp-cfns")
+# to rebuild cfns.h from cfns.gperf any time that cfns.gperf is
+# edited.  We don't trigger it automatically when
+# $(srcdir)/cp/cfns.gperf is newer than $(srcdir)/cp/cfns.h because we
+# don't want to depend on users getting the source code with
+# particular timestamps.
+rebuild-cp-cfns:
        gperf -o -C -E -k '1-6,$$' -j1 -D -N 'libc_name_p' -L ANSI-C \
-               $(srcdir)/cp/cfns.gperf > $(srcdir)/cp/cfns.h
+               $(srcdir)/cp/cfns.gperf --output-file $(srcdir)/cp/cfns.h
 
+.PHONY: rebuild-cp-cfns
+
 #^L
 # Build hooks:
 
Index: ChangeLog
===================================================================
--- ChangeLog   (revision 172858)
+++ ChangeLog   (working copy)
@@ -1,3 +1,13 @@
+2011-04-22  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * Make-lang.in (rebuild-cp-cfns): New rule replacing the one to
+       build $(srcdir)/cp/cfns.h to prevent unwanted rebuilds.  Use
+       --output-file option of gperf instead of > to prevent creating an
+       empty cp/cfns.h when gperf is not available.
+       (.PHONY): New, to make rebuild-cp-cfns as phony.
+       * cfns.gperf: Explain how to run 'make rebuild-cp-cfns'.
+       * cfns.h: Regenerated
+
 2011-04-20  Jason Merrill  <jason@redhat.com>
 
        * semantics.c (finish_compound_literal): Don't put an array
Index: cfns.h
===================================================================
--- cfns.h      (revision 172858)
+++ cfns.h      (working copy)
@@ -1,5 +1,5 @@
 /* ANSI-C code produced by gperf version 3.0.3 */
-/* Command-line: gperf -o -C -E -k '1-6,$' -j1 -D -N libc_name_p -L ANSI-C cfns.gperf  */
+/* Command-line: gperf -o -C -E -k '1-6,$' -j1 -D -N libc_name_p -L ANSI-C --output-file ../../trunk2/gcc/cp/cfns.h ../../trunk2/gcc/cp/cfns.gperf  */
 
 #if !((' ' == 32) && ('!' == 33) && ('"' == 34) && ('#' == 35) \
       && ('%' == 37) && ('&' == 38) && ('\'' == 39) && ('(' == 40) \
@@ -28,7 +28,7 @@
 #error "gperf generated tables don't work with this execution character set. Please report a bug to <bug-gnu-gperf@gnu.org>."
 #endif
 
-#line 1 "cfns.gperf"
+#line 1 "../../trunk2/gcc/cp/cfns.gperf"
 
 /* Copyright (C) 2000, 2003 Free Software Foundation, Inc.
 
@@ -46,7 +46,18 @@ for more details.
 
 You should have received a copy of the GNU General Public License
 along with GCC; see the file COPYING3.  If not see
-<http://www.gnu.org/licenses/>.  */
+<http://www.gnu.org/licenses/>.
+
+You need to use gperf to generate cfns.h from cfns.gperf.  This is
+the process:
+
+ have a GCC source/build tree already checked out
+ edit cfns.gperf
+ cd $(buildir)/gcc
+ make rebuild-cfns
+
+Alternatively, just look into cp/Make-lang.in and run the gperf
+command in the rebuild-cfns rule manually.  */
 #ifdef __GNUC__
 __inline
 #endif

Re: (build) Patch to fix cp/cfns.gperf building issues

[Joseph S. Myers]

On Fri, 22 Apr 2011, Nicola Pero wrote:

> In general, I personally feel that the building system should not depend 
> on the relative timestamps of source files unless it's doing something 
> in "maintainer mode" where it's being explicitly asked to rebuild one 
> source file from the other.

We have a --enable-maintainer-mode configure option.  I think you should 
use that instead of having a special rule for a particular file.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: (build) Patch to fix cp/cfns.gperf building issues

[Nicola Pero]

> We have a --enable-maintainer-mode configure option.

Thanks - I had missed that option.  It's an excellent suggestion - here is
a new patch that uses it. :-)

Ok to commit ?

Thanks

PS: Regarding how I detect --enable-maintainer-mode in this new patch, cp/Make-lang.in
is used at it is, without going through configure, so I can't use @MAINT@ in it.  I
can't use MAINT either, because it's always empty.  I added a FIXME as the intention
in the code was probably different.

I could actually fix MAINT to end up being exactly '#' when --disable-maintainer-mode
is used, but why would we want that ?  It would be a literal value, which you can't
use to comment out bits of code in the same way as you can with @MAINT@.  Of course,
people may still try, and waste time trying to figure out why it doesn't work.

It just doesn't make sense; you can't comment out make code by putting a variable
with a value of '#' in front of something.  If you ask me, I'd just remove the MAINT
variable altogether - it's a bad idea and would/will confuse people.

I added a new readable variable instead, ENABLE_MAINTAINER_RULES, which is 'true' or
empty, so you can do

ifeq ($(ENABLE_MAINTAINER_RULES), true)
  ... maintainer rules ...
endif

which is readable, expressive, simple and robust.

Index: ChangeLog
===================================================================
--- ChangeLog   (revision 172860)
+++ ChangeLog   (working copy)
@@ -1,3 +1,7 @@
+2011-04-22  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * Makefile.in (ENABLE_MAINTAINER_RULES): New.
+
 2011-04-22  Jakub Jelinek  <jakub@redhat.com>
 
        PR c/48716
Index: cp/Make-lang.in
===================================================================
--- cp/Make-lang.in     (revision 172860)
+++ cp/Make-lang.in     (working copy)
@@ -104,10 +104,15 @@ cc1plus$(exeext): $(CXX_OBJS) cc1plus-checksum.o $
        +$(LINKER) $(ALL_LINKERFLAGS) $(LDFLAGS) -o $@ \
              $(CXX_OBJS) cc1plus-checksum.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
 
-# Special build rules.
+ifeq ($(ENABLE_MAINTAINER_RULES), true)
+# Special build rule.  This is a maintainer rule, that is only
+# available when GCC is configured with --enable-maintainer-mode.  In
+# other cases, it is not available to avoid triggering rebuilds if a
+# user has the source checked out with unusual timestamps.
 $(srcdir)/cp/cfns.h: $(srcdir)/cp/cfns.gperf
        gperf -o -C -E -k '1-6,$$' -j1 -D -N 'libc_name_p' -L ANSI-C \
-               $(srcdir)/cp/cfns.gperf > $(srcdir)/cp/cfns.h
+               $(srcdir)/cp/cfns.gperf --output-file $(srcdir)/cp/cfns.h
+endif
 
 #^L
 # Build hooks:
Index: cp/ChangeLog
===================================================================
--- cp/ChangeLog        (revision 172860)
+++ cp/ChangeLog        (working copy)
@@ -1,3 +1,10 @@
+2011-04-23  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * Make-lang.in ($(srcdir)/cp/cfns.h): Enable the rule only in
+       maintainer mode.  Use the --output-file option of gperf instead of
+       > to prevent creating an empty cp/cfns.h when gperf is not
+       available.
+
 2011-04-20  Jason Merrill  <jason@redhat.com>
 
        * semantics.c (finish_compound_literal): Don't put an array
Index: Makefile.in
===================================================================
--- Makefile.in (revision 172860)
+++ Makefile.in (working copy)
@@ -165,8 +165,19 @@ C_STRICT_WARN = @c_strict_warn@
 NOCOMMON_FLAG = @nocommon_flag@
 
 # This is set by --disable-maintainer-mode (default) to "#"
+# FIXME: 'MAINT' will always be set to an empty string, no matter if
+# --disable-maintainer-mode is used or not.  This is because the
+# following will expand to "MAINT := " in maintainer mode, and to
+# "MAINT := #" in non-maintainer mode, but because '#' starts a comment,
+# they mean exactly the same thing for make.
 MAINT := @MAINT@
 
+# 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
+
 # These are set by --enable-checking=valgrind.
 RUN_GEN = @valgrind_command@
 VALGRIND_DRIVER_DEFINES = @valgrind_path_defines@

Re: (build) Patch to fix cp/cfns.gperf building issues

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]


On Apr 22, 2011, at 8:12 AM, Nicola Pero wrote:
> This patch fixes a building annoyance that I had when building on a new
> machine (an x86_64 gnu/linux box).
> 
> The building failed.  It was down to two problems:
> 
> * due to how I got a copy of the GCC source code on the machine, the timestamp
> of each source file was the timestamp of when it was copied to the machine (ie,
> a random timestamp from the point of view of the building system).  This caused
> the build system to decide that $(srcdir)/cp/cfns.h needed to be rebuilt from
> $(srcdir)/cp/cfns.gperf (it didn't, obviously; the source code was trunk with
> no changes).

Additionally,

  contrib/gcc_update --touch

can be used to to fix the time stamps until such time as someone changes the gperf rule to be under maintainer mode.

So, only the dependency should go away under a maintainer rules, as in the below, not the entire rule.

Ok?


[-- Attachment #2: maint.diffs.txt --]
[-- Type: text/plain, Size: 637 bytes --]

2011-04-22  Mike Stump  <mikestump@comcast.net>

	* Make-lang.in: Only run gperf in maintainer mode.

Index: cp/Make-lang.in
===================================================================
--- cp/Make-lang.in	(revision 172670)
+++ cp/Make-lang.in	(working copy)
@@ -105,7 +105,10 @@ cc1plus$(exeext): $(CXX_OBJS) cc1plus-ch
 	      $(CXX_OBJS) cc1plus-checksum.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
 
 # Special build rules.
+ifneq ($(MAINT),)
 $(srcdir)/cp/cfns.h: $(srcdir)/cp/cfns.gperf
+endif
+$(srcdir)/cp/cfns.h:
 	gperf -o -C -E -k '1-6,$$' -j1 -D -N 'libc_name_p' -L ANSI-C \
 		$(srcdir)/cp/cfns.gperf > $(srcdir)/cp/cfns.h
 

Re: (build) Patch to fix cp/cfns.gperf building issues

[Nicola Pero]

> Additionally,
>
>  contrib/gcc_update --touch
>
> can be used to to fix the time stamps until such time as someone changes the gperf rule to be under maintainer mode.

Thanks Mike,

good to know. :-)


> So, only the dependency should go away under a maintainer rules, as in the below, not the entire rule.

What is the reason to keep the rule without the dependency ?  Is it so that even with --disable-maintainer-mode
you can force the file to be recreated by manually deleting it ?

That would make sense ... it sounds like a good idea, so here is yet another refinement of the patch with
that refinement added too :-)

Ok to commit ?

Thanks


Index: ChangeLog
===================================================================
--- ChangeLog   (revision 172860)
+++ ChangeLog   (working copy)
@@ -1,3 +1,7 @@
+2011-04-22  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * Makefile.in (ENABLE_MAINTAINER_RULES): New.
+
 2011-04-22  Jakub Jelinek  <jakub@redhat.com>
 
        PR c/48716
Index: cp/Make-lang.in
===================================================================
--- cp/Make-lang.in     (revision 172860)
+++ cp/Make-lang.in     (working copy)
@@ -104,10 +104,20 @@ cc1plus$(exeext): $(CXX_OBJS) cc1plus-checksum.o $
        +$(LINKER) $(ALL_LINKERFLAGS) $(LDFLAGS) -o $@ \
              $(CXX_OBJS) cc1plus-checksum.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
 
-# Special build rules.
+ifeq ($(ENABLE_MAINTAINER_RULES), true)
+# Special build rule.  This is a maintainer rule, that is only
+# available when GCC is configured with --enable-maintainer-mode.  In
+# other cases, it is not available to avoid triggering rebuilds if a
+# user has the source checked out with unusual timestamps.
 $(srcdir)/cp/cfns.h: $(srcdir)/cp/cfns.gperf
+else
+# We keep the rule so that you can still force a rebuild, even if you
+# didn't configure GCC with --enable-maintainer-mode, by manually
+# deleting the $(srcdir)/cp/cfns.h file.
+$(srcdir)/cp/cfns.h:
+endif
        gperf -o -C -E -k '1-6,$$' -j1 -D -N 'libc_name_p' -L ANSI-C \
-               $(srcdir)/cp/cfns.gperf > $(srcdir)/cp/cfns.h
+               $(srcdir)/cp/cfns.gperf --output-file $(srcdir)/cp/cfns.h
 
 #^L
 # Build hooks:
Index: cp/ChangeLog
===================================================================
--- cp/ChangeLog        (revision 172860)
+++ cp/ChangeLog        (working copy)
@@ -1,3 +1,11 @@
+2011-04-23  Nicola Pero  <nicola.pero@meta-innovation.com>,
+           Mike Stump <mikestump@comcast.net>
+
+       * Make-lang.in ($(srcdir)/cp/cfns.h): Enable the dependency only
+       in maintainer mode.  Use the --output-file option of gperf instead
+       of > to prevent creating an empty cp/cfns.h when gperf is not
+       available.
+
 2011-04-20  Jason Merrill  <jason@redhat.com>
 
        * semantics.c (finish_compound_literal): Don't put an array
Index: Makefile.in
===================================================================
--- Makefile.in (revision 172860)
+++ Makefile.in (working copy)
@@ -165,8 +165,19 @@ C_STRICT_WARN = @c_strict_warn@
 NOCOMMON_FLAG = @nocommon_flag@
 
 # This is set by --disable-maintainer-mode (default) to "#"
+# FIXME: 'MAINT' will always be set to an empty string, no matter if
+# --disable-maintainer-mode is used or not.  This is because the
+# following will expand to "MAINT := " in maintainer mode, and to
+# "MAINT := #" in non-maintainer mode, but because '#' starts a comment,
+# they mean exactly the same thing for make.
 MAINT := @MAINT@
 
+# 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
+
 # These are set by --enable-checking=valgrind.
 RUN_GEN = @valgrind_command@
 VALGRIND_DRIVER_DEFINES = @valgrind_path_defines@

Re: (build) Patch to fix cp/cfns.gperf building issues

[Mike Stump]

On Apr 23, 2011, at 4:34 AM, Nicola Pero wrote:
> What is the reason to keep the rule without the dependency ?  Is it so that even with --disable-maintainer-mode you can force the file to be recreated by manually deleting it ?

Yes.  Think of it as a really cheap maintainer mode.  Another way to look at it, would be consistency with the existing style.

I think the patch is ready for a build person to review it.  :-)

Re: (build) Patch to fix cp/cfns.gperf building issues

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

On Apr 22, 2011, at 3:54 PM, Mike Stump wrote:
> On Apr 22, 2011, at 8:12 AM, Nicola Pero wrote:
>> This patch fixes a building annoyance that I had when building on a new
>> machine (an x86_64 gnu/linux box).
>> 
>> The building failed.  It was down to two problems:
>> 
>> * due to how I got a copy of the GCC source code on the machine, the timestamp
>> of each source file was the timestamp of when it was copied to the machine (ie,
>> a random timestamp from the point of view of the building system).  This caused
>> the build system to decide that $(srcdir)/cp/cfns.h needed to be rebuilt from
>> $(srcdir)/cp/cfns.gperf (it didn't, obviously; the source code was trunk with
>> no changes).

> Ok?

Ping?



[-- Attachment #2: maint.diffs.txt --]
[-- Type: text/plain, Size: 637 bytes --]

2011-04-22  Mike Stump  <mikestump@comcast.net>

	* Make-lang.in: Only run gperf in maintainer mode.

Index: cp/Make-lang.in
===================================================================
--- cp/Make-lang.in	(revision 172670)
+++ cp/Make-lang.in	(working copy)
@@ -105,7 +105,10 @@ cc1plus$(exeext): $(CXX_OBJS) cc1plus-ch
 	      $(CXX_OBJS) cc1plus-checksum.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
 
 # Special build rules.
+ifneq ($(MAINT),)
 $(srcdir)/cp/cfns.h: $(srcdir)/cp/cfns.gperf
+endif
+$(srcdir)/cp/cfns.h:
 	gperf -o -C -E -k '1-6,$$' -j1 -D -N 'libc_name_p' -L ANSI-C \
 		$(srcdir)/cp/cfns.gperf > $(srcdir)/cp/cfns.h
 

Re: (build) Patch to fix cp/cfns.gperf building issues

[Nicola Pero]

>> Ok?
>
> Ping?

PS: For the maintainer who will (eventually) review this patch,
the latest version, tested and with all the comments and contributions
from Joseph and Mike merged in, is --

http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01930.html

So, that's the one to review.

Re: (build) Patch to fix cp/cfns.gperf building issues

[Alexandre Oliva]

On Apr 23, 2011, "Nicola Pero" <nicola.pero@meta-innovation.com> wrote:

> Ok to commit ?

Yeah, thanks.

> Index: ChangeLog
> +2011-04-22  Nicola Pero  <nicola.pero@meta-innovation.com>
> +
> +       * Makefile.in (ENABLE_MAINTAINER_RULES): New.
> +
> Index: cp/ChangeLog
> +2011-04-23  Nicola Pero  <nicola.pero@meta-innovation.com>,
> +           Mike Stump <mikestump@comcast.net>
> +
> +       * Make-lang.in ($(srcdir)/cp/cfns.h): Enable the dependency only
> +       in maintainer mode.  Use the --output-file option of gperf instead
> +       of > to prevent creating an empty cp/cfns.h when gperf is not
> +       available.
> +

-pedantic review: how about outputting to a temporary file (say
cp/cfns.hT) and only renaming to the intended name on success, so that,
if gperf crashes or we reboot part-way through it, we don't end up with
a partially-generated file that will seem to be up to date?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: (build) Patch to fix cp/cfns.gperf building issues

[Nicola Pero]

Alexandre

thanks for the review :-)

> -pedantic review: how about outputting to a temporary file (say
> cp/cfns.hT) and only renaming to the intended name on success, so that,
> if gperf crashes or we reboot part-way through it, we don't end up with
> a partially-generated file that will seem to be up to date?

It's a good suggestion.

On the other hand, now that the rule is maintainer-only, and considering
it's used once every few years, I wouldn't personally want to spend more
time on it. ;-)

Thanks