[Bug bootstrap/111021] New: [14 Regression] Serial build broken for CRIS, ARM, and others

[Bug bootstrap/111021] New: [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

            Bug ID: 111021
           Summary: [14 Regression] Serial build broken for CRIS, ARM, and
                    others
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: bootstrap
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hp at gcc dot gnu.org
                CC: linkw at gcc dot gnu.org
  Target Milestone: ---

Since r14-3092-g165b1f6ad1d396 "targhooks: Extend legitimate_address_p with
code_helper [PR110248]", build has been broken (at least with "GNU Make 4.3")
for *serial* builds (no "-j" option) for those target where that commit added
an include of tree.h was added, exposed by building for example cris-elf and
arm-eabi (wrapped lines copy-pasted):

g++ -c -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE
-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W
-Wall -Wno-narrowing -Wwrite-strings -Wcast-qual
-Wmissing-format-attribute -Wconditionally-supported
-Woverloaded-virtual -pedantic -Wno-long-long
-Wno-variadic-macros -Wno-overlength-strings -fno-common
-DHAVE_CONFIG_H -DGENERATOR_FILE -I. -Ibuild -I/srctop/gcc
-I/srctop/gcc/build -I/srctop/gcc/../include
-I/srctop/gcc/../libcpp/include \ -o build/gencondmd.o
build/gencondmd.cc
In file included from /srctop/gcc/tree.h:23,
                 from /srctop/gcc/config/arm/arm-protos.h:26,
                 from ./tm_p.h:5,
                 from build/gencondmd.cc:29:
/srctop/gcc/tree-core.h:145:10: fatal error: all-tree.def: No such file or
directory
  145 | #include "all-tree.def"
      |          ^~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:2918: build/gencondmd.o] Error 1

At a glance, (at least) build/gencondmd.o lacks a dependency on the generated
all-tree.def.  But adding only that dependency leads to a similar error for a
missing of tree-check.h; looks like TREE_H contains the right bits.

Maybe it's as "simple" as including $(TREE_H) in TM_P (or a new intermediary
macro) or add $(TREE_H) to all build/gen*.o rules where the gen*.cc includes
"tm_p.h" (but within the filter-out for build/gencondmd.o).

I *did* try the latter; simply adding $(TREE_H) (within the filter-out call)
for build/gencondmd.o and that seemed to work, but it doesn't seem *right* and
may work just by happenstance, as other build/gen*.o seems like they could fail
given unfortunate timing or Makefile tweak.  It seems proper to leave that
headache to the author of that the offending commit.  (I'm going to bail out
for CRIS with a patch to cris-protos.h, wrapping the function that needs to be
prototyped so it doesn't need to refer to ERROR_MARK.)

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627435.html

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2023-August/
                   |                            |627435.html
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-08-15
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
Thanks for reporting, I think the culprit is r14-3093 instead of r14-3092? 

I think the other build/gen*.cc building don't have this issue, since none of
them includes recog.h themselves, but emit the source files which includ
recog.h (those insn-*.cc, I had a double check, they are fine).
build/gencondmd.cc is special, unlike the other build/gen*.cc, it's generated
by genconditions and it include "recog.h". Only build/gencondmd.o depends on
RECOG_H.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #3 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #2)
> Thanks for reporting, I think the culprit is r14-3093 instead of r14-3092? 

Not for this PR!

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #4 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627435.html

Nope, that just fixes recog.h.  See the quoted error; it's the patched
*-protos.h.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #5 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #4)
> (In reply to Andrew Pinski from comment #1)
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627435.html
> 
> Nope, that just fixes recog.h.  See the quoted error; it's the patched
> *-protos.h.

...but I'm "guessing" that it will appear to work even for the
tm-protos.h-on-tree.h dependency, just incidentally.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org,
                   |                            |rsandifo at gcc dot gnu.org

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #5)
> (In reply to Hans-Peter Nilsson from comment #4)
> > (In reply to Andrew Pinski from comment #1)
> > > https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627435.html
> > 
> > Nope, that just fixes recog.h.  See the quoted error; it's the patched
> > *-protos.h.

Thanks for clarifying, yeah, some ${port}-protos.h includes tree.h in that
commit.

> 
> ...but I'm "guessing" that it will appear to work even for the
> tm-protos.h-on-tree.h dependency, just incidentally.

It can work for this build failure on build/gencondmd.o since the same required
dependency on tree.h is supported by checking the dependence of recog.h, but
the concern is that one day some file just includes tm_p.h but not recog.h, the
issue will show up again. I'm thinking how to deal with the problems on
${port}-protos.h, adding $(TREE_H) in TM_P is an alternative, but not sure if
people would think it's an overkill since the current affected ports are:

gcc/config/arm/arm-protos.h:#include "tree.h" /* For ERROR_MARK.  */
gcc/config/cris/cris-protos.h:#include "tree.h" /* For ERROR_MARK.  */
gcc/config/microblaze/microblaze-protos.h:#include "tree.h"  /* For ERROR_MARK.
 */
gcc/config/rl78/rl78-protos.h:#include "tree.h"  /* For ERROR_MARK.  */
gcc/config/stormy16/stormy16-protos.h:#include "tree.h"  /* For ERROR_MARK.  */

I checked the existing tm_p_file_list and tm_p_include_list which are specific
for port, but tm_p_include_list is generated based on tm_p_file_list, it seems
not for this purpose. 

Hi Richi & Richard, what do you think of this?

btw, thanks for the suggestion on "defaulting to NULL or something" in ML, but
I guess a default value like zero rather than ERROR_MARK has to base on the
assertion that the default value is equal to ERROR_MARK, IMHO it isn't quite
maintainable.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #7 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #6)
> (In reply to Hans-Peter Nilsson from comment #5)
> [...]
> but the concern is that one day some file just includes tm_p.h but
> not recog.h, the issue will show up again. I'm thinking how to deal with the
> problems on ${port}-protos.h, adding $(TREE_H) in TM_P is an alternative,
> but not sure if people would think it's an overkill since the current
> affected ports are:
> [...]

Exactly; I'm happy that we seem to be on the same page here.

I'm testing a patch for CRIS (making the hook function just a wrapper,
reverting the cris-protos.h change) that may be re-usable for the other targets
similarly affected.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #8 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #6)
> 
> btw, thanks for the suggestion on "defaulting to NULL or something" in ML,
> but I guess a default value like zero rather than ERROR_MARK has to base on
> the assertion that the default value is equal to ERROR_MARK, IMHO it isn't
> quite maintainable.

I believe you could add a code_helper constructor for another type, maybe even
"int"?  Then the "default" value for the code_helper argument for the
legitimate address hook function would just look like "code_helper = 0".

Maybe that's too error-prone and a pointer-type or some entirely different type
would be better. I might have misunderstood things of course, but that's what I
mean by "or something".

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #9 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627444.html

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #10 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #7)
> Exactly; I'm happy that we seem to be on the same page here.
> 
> I'm testing a patch for CRIS (making the hook function just a wrapper,
> reverting the cris-protos.h change) that may be re-usable for the other
> targets similarly affected.

> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627444.html

Thanks for the suggestion and doing the patch! If Richi & Richard prefer this,
I'll follow up with a patch for the other affected ports.

> I believe you could add a code_helper constructor for another type, maybe
> even "int"?  Then the "default" value for the code_helper argument for the
> legitimate address hook function would just look like "code_helper = 0".
> 
> Maybe that's too error-prone and a pointer-type or some entirely different
> type would be better. I might have misunderstood things of course, but
> that's what I mean by "or something".

OK, thanks for further clarifying, I agree it's doable, I didn't consider this
before because it's a "code" helper, as its comments says it's for tree codes
and builtin function codes, shouldn't make it support things that isn't "code".
Even for a int which can be equivalent to a tree_code (with an appropriate
assertion check for unexpected range), it's still easy to be questioned like
why not just use tree_code directly (having good readability and without any
expected range checking etc.).

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #11 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #8)
> I might have misunderstood things of course, 
...like still having to include tree.h to get the code_helper class definition.
Doh!

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think a "too broad" dependence isn't bad.  The cris specific solution also
looks manageable, though I wonder what's special about x-protos.h, knowing very
little of that area.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #13 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #12)
> I think a "too broad" dependence isn't bad.  The cris specific solution also
> looks manageable, though I wonder what's special about x-protos.h, knowing
> very little of that area.

Thanks for the comments, I'll follow up with the former if no objections. :)

About {port}-protos.h, the documentation says "The ‘machine.h’ header is
included very early in GCC’s standard sequence of header files, while
‘machine-protos.h’ is included late in the sequence. Thus ‘machine-protos.h’
can include declarations referencing types that are not defined when
‘machine.h’ is included, specifically including those from ‘rtl.h’ and
‘tree.h’."

One special thing I noticed is that for the functions used in md files but
defined in ${port}.cc or others, they needs to be declared in {port}-protos.h,
since those generated insn-*.cc includes tm_p.h which includes {port}-protos.h
(and some other in tm_p_file_list like tm-preds.h target specific predicates).

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #14 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #13)
> (In reply to Richard Biener from comment #12)
> > I think a "too broad" dependence isn't bad.  The cris specific solution also
> > looks manageable, though I wonder what's special about x-protos.h, knowing
> > very little of that area.
> 
> Thanks for the comments, I'll follow up with the former if no objections. :)
> 
> About {port}-protos.h, the documentation says "The ‘machine.h’ header is
> included very early in GCC’s standard sequence of header files, while
> ‘machine-protos.h’ is included late in the sequence. Thus ‘machine-protos.h’
> can include declarations referencing types that are not defined when
> ‘machine.h’ is included, specifically including those from ‘rtl.h’ and
> ‘tree.h’."

insn-automata.cc have both tm.h ($port.h) and tm_p.h (${port}-protos.h), the
order looks like:

#define IN_TARGET_CODE 1
#include "config.h"
#include "system.h"
#include "coretypes.h"
#include "tm.h"
#include "alias.h"
#include "tree.h"
#include "varasm.h"
#include "stor-layout.h"
#include "calls.h"
#include "rtl.h"
#include "memmodel.h"
#include "tm_p.h"
#include "insn-config.h"
#include "recog.h"
#include "regs.h"
#include "output.h"
#include "insn-attr.h"
#include "diagnostic-core.h"
#include "flags.h"
#include "function.h"
#include "emit-rtl.h"

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:ecb95399f43873e1f34119ac260bbea2ef358e53

commit r14-3215-gecb95399f43873e1f34119ac260bbea2ef358e53
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Aug 15 03:01:20 2023 -0500

    Makefile.in: Make recog.h depend on $(TREE_H) [PR111021]

    Commit r14-3093 introduced a random build failure on
    build/gencondmd.cc building.  Since r14-3093 make recog.h
    include tree.h, which further includes (depends on) some
    files that are generated during the building, such as:
    all-tree.def, tree-check.h etc, when building file
    build/gencondmd.cc, the build can fail if these dependences
    are not ready.  So this patch is to teach this dependence.

    Thank Jan-Benedict Glaw for testing this!

            PR bootstrap/111021

    gcc/ChangeLog:

            * Makefile.in (RECOG_H): Add $(TREE_H) as dependence.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |build
   Target Milestone|---                         |14.0

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:

https://gcc.gnu.org/g:eef192b181b8777d708671e2541896e7e31293aa

commit r14-3218-geef192b181b8777d708671e2541896e7e31293aa
Author: Hans-Peter Nilsson <hp@axis.com>
Date:   Tue Aug 15 06:35:43 2023 +0200

    CRIS: Don't include tree.h in cris-protos.h, PR bootstrap/111021

    While there's another patch that fixes the immediate error
    in the PR by other means, the include of tree.h here is
    something I prefer to avoid.

            PR bootstrap/111021
            * config/cris/cris-protos.h: Revert recent change.
            * config/cris/cris.cc (cris_legitimate_address_p): Remove
            code_helper unused parameter.
            (cris_legitimate_address_p_hook): New wrapper function.
            (TARGET_LEGITIMATE_ADDRESS_P): Change to
            cris_legitimate_address_p_hook.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #17 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #12)
> I think a "too broad" dependence isn't bad.  The cris specific solution also
> looks manageable, though I wonder what's special about x-protos.h, knowing
> very little of that area.

Not very special, it's just that there's a now dependence on tree.h and its
generated contents where for tm-protos.h there has been none before.  The
reasons seem avoidable at this time, but I think it's nominally valid to have
that dependency.  On the one hand, introducing a new dependency where there has
been none before and dealing with the fallout we now see is awkward, but on the
other hand pragmatically the dependence on tree.h for $(TM_P) is trivial enough
that a parallel build "raced" to fulfill it. IOW, shouldn't affect build time
by any measure.

(Not closing this PR yet in case maintainers go forward for the other
tm-protos.h-affected targets)

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:8353b9c5f5af0e65cb04fcbb7325d6a566006cd4

commit r14-3322-g8353b9c5f5af0e65cb04fcbb7325d6a566006cd4
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Fri Aug 18 05:03:40 2023 -0500

    Makefile.in: Make TM_P_H depend on $(TREE_H) [PR111021]

    As PR111021 shows, the below ${port}-protos.h include tree.h
    for code_helper and tree_code:

      arm/arm-protos.h:#include "tree.h"
      cris/cris-protos.h:#include "tree.h" (H-P removed this in r14-3218)
      microblaze/microblaze-protos.h:#include "tree.h"
      rl78/rl78-protos.h:#include "tree.h"
      stormy16/stormy16-protos.h:#include "tree.h"

    , when compiling build/gencondmd.cc, the include hierarchy
    makes it depend on tm_p.h -> ${port}-protos.h -> tree.h,
    which further includes (depends on) some files that are
    generated during the building, such as: all-tree.def,
    tree-check.h and so on.  The previous commit r14-3215
    should already force build/gencondmd.cc to depend on
    ${TREE_H}, so the reported build failure should be gone.

    But for a long term maintenance, especially one day some
    build/xxx.cc requires tm_p.h but not recog.h, the ${TREE_H}
    dependence could be missed and a build failure will show
    up.  So this patch is to make TM_P_H depend on $(TREE_H),
    any new build/xxx.cc depending on tm_p.h will be able to
    consider ${TREE_H}.

    It's tested with cross-builds for the affected ports with
    steps:
     1) dropped the fix r14-3215;
     2) reproduced the build failure with serial build;
     3) applied this patch, serial built and verified all passed;
     4) added back r14-3215, serial built and verified all passed;

            PR bootstrap/111021

    gcc/ChangeLog:

            * Makefile.in (TM_P_H): Add $(TREE_H) as dependence.

[Bug bootstrap/111021] [14 Regression] Serial build broken for CRIS, ARM, and others since r14-3092

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111021

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[14 Regression] Serial      |[14 Regression] Serial
                   |build broken for CRIS, ARM, |build broken for CRIS, ARM,
                   |and others                  |and others since r14-3092
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #19 from Kewen Lin <linkw at gcc dot gnu.org> ---
Should be fixed on trunk, thanks to all for the comments and helps!