Bug 99422 - [11 Regression] ICE in extract_constrain_insn building glibc pthread_create
Summary: [11 Regression] ICE in extract_constrain_insn building glibc pthread_create
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 11.0
: P1 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code
: 99432 99438 99461 99467 (view as bug list)
Depends on:
Blocks: 99432
  Show dependency treegraph
 
Reported: 2021-03-06 01:31 UTC by Joseph S. Myers
Modified: 2021-03-18 19:59 UTC (History)
9 users (show)

See Also:
Host:
Target: i686-*-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-03-06 00:00:00


Attachments
preprocessed source (86.23 KB, text/plain)
2021-03-06 01:31 UTC, Joseph S. Myers
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph S. Myers 2021-03-06 01:31:42 UTC
Created attachment 50314 [details]
preprocessed source

Commit 9105757a59b890194ebf5b4fcbacd58db34ef332 ("[PR99378] LRA: Skip decomposing address for asm insn operand with unknown constraint.") introduced the following ICE building glibc for i686-linux-gnu. Tested with an x86_64 compiler; compile the attached .i file with the following options: -m32 -march=i686 -O2 -pg -S

In file included from pthread_create.c:59:
allocatestack.c: In function '__nptl_setxid':
allocatestack.c:1173:1: error: unrecognizable insn:
(insn 103 534 535 21 (parallel [
            (set (mem/v:SI (plus:SI (mem/f/c:SI (plus:SI (reg/f:SI 6 bp)
                                (const_int 8 [0x8])) [85 cmdp+0 S4 A32])
                        (const_int 16 [0x10])) [5 cmdp_52(D)->cntr+0 S4 A32])
                (asm_operands/v:SI ("lock;incl %0") ("=m") 0 [
                        (mem/v:SI (plus:SI (reg:SI 0 ax [208])
                                (const_int 16 [0x10])) [5 cmdp_52(D)->cntr+0 S4 A32])
                        (const_int 12 [0xc])
                    ]
                     [
                        (asm_input:SI ("m") allocatestack.c:1036)
                        (asm_input:SI ("i") allocatestack.c:1036)
                    ]
                     [] allocatestack.c:1036))
            (clobber (reg:CC 17 flags))
        ]) "allocatestack.c":1036:47 -1
     (nil))
during RTL pass: reload
allocatestack.c:1173:1: internal compiler error: in extract_constrain_insn, at recog.c:2670
0x63ae76 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*)
        /scratch/jmyers/glibc/many11/src/gcc/gcc/rtl-error.c:108
0x63ae92 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
        /scratch/jmyers/glibc/many11/src/gcc/gcc/rtl-error.c:116
0xd1149d extract_constrain_insn(rtx_insn*)
        /scratch/jmyers/glibc/many11/src/gcc/gcc/recog.c:2670
0xbec6a7 check_rtl
        /scratch/jmyers/glibc/many11/src/gcc/gcc/lra.c:2087
0xbf14ac lra(_IO_FILE*)
        /scratch/jmyers/glibc/many11/src/gcc/gcc/lra.c:2505
0xba1489 do_reload
        /scratch/jmyers/glibc/many11/src/gcc/gcc/ira.c:5827
0xba1489 execute
        /scratch/jmyers/glibc/many11/src/gcc/gcc/ira.c:6013
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
Comment 1 Jakub Jelinek 2021-03-06 09:51:12 UTC
Seems the latest change resulted in various inline asms with "=m" constraint to fall through into return false rather than decompose_address.
The problem to me seems to be that constraint is "=m" (and generally, it is the whole constraint string, rather than some particular constraint in it),
and lookup_constraint looks up only a particular constraint (say "m") etc.
So it only returns something other than CONSTRAINT__UNKNOWN if it is an input operand rather than output (because that necessarily starts with = or +) and doesn't have any other modifiers etc. in it and it has a single variant or the first variant is the one that is considered.
There are other spots that do the same though, e.g. simplify_operand_subreg:
          || ((get_constraint_type (lookup_constraint
                                    (curr_static_id->operand[nop].constraint))
               != CT_SPECIAL_MEMORY)
While e.g. process_alt_operands walks the whole constraint string and handles everything in there.

I'm afraid I must be missing something important.
Comment 2 Jakub Jelinek 2021-03-06 10:08:50 UTC
I see the function is called before selecting a particular alternative, so perhaps it means to care only about constraints like "X" and "" and not say that mixed with other constraints etc.
But, shouldn't the code at least skip the =, +, &, % and whitespace from the start?  What about other modifiers (the various disparage slightly etc. chars)?
And only consider as empty constraint if after those skips constraint is ""?
Not really sure if ",,," constraint is valid...
And, regarding of Eric's change to handle "X" that way, does that really apply just to MEM and not SUBREG of MEM too?
Comment 3 David Binderman 2021-03-06 18:57:23 UTC
I also see this when compiling linux kernel 5.12-rc2.

net/netfilter/utils.c: In function ‘nf_checksum’:
net/netfilter/utils.c:139:1: error: unrecognizable insn:
  139 | }
      | ^
(insn 120 466 414 12 (parallel [
            (set (reg:SI 0 ax [orig:200 sum ] [200])
                (asm_operands:SI ("  addl %1, %0
  adcl %2, %0
  adcl %3, %0
  adcl $0, %0
") ("=r") 0 [
                        (mem/c:SI (plus:DI (reg/f:DI 7 sp)
                                (const_int 24 [0x18])) [301 %sfp+-8 S4 A64])
                        (mem:SI (plus:DI (mem/c:DI (plus:DI (reg/f:DI 7 sp)
                                        (const_int 16 [0x10])) [301 %sfp+-16 S8 A64])
                                (const_int 12 [0xc])) [1 MEM[(const struct iphdr *)_19].saddr+0 S4 A32])
                        (reg:SI 1 dx [203])
                        (reg:SI 0 ax [orig:200 sum ] [200])
                    ]
                     [
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("0") ./arch/x86/include/asm/checksum_64.h:91)
                    ]
                     [] ./arch/x86/include/asm/checksum_64.h:91))
            (clobber (reg:CC 17 flags))
        ]) "./arch/x86/include/asm/checksum_64.h":91:2 -1
     (nil))
Comment 4 Andrew Pinski 2021-03-07 02:27:53 UTC
*** Bug 99432 has been marked as a duplicate of this bug. ***
Comment 5 Andrew Pinski 2021-03-07 02:28:04 UTC
*** Bug 99438 has been marked as a duplicate of this bug. ***
Comment 6 Vladimir Makarov 2021-03-07 19:01:52 UTC
(In reply to Joseph S. Myers from comment #0)
> Created attachment 50314 [details]
> preprocessed source
> 
> Commit 9105757a59b890194ebf5b4fcbacd58db34ef332 ("[PR99378] LRA: Skip
> decomposing address for asm insn operand with unknown constraint.")
> introduced the following ICE building glibc for i686-linux-gnu. Tested with
> an x86_64 compiler; compile the attached .i file with the following options:
> -m32 -march=i686 -O2 -pg -S
> 

Sorry for causing the troubles.  I'll fix this tomorrow.
Comment 7 Vladimir Makarov 2021-03-07 19:49:45 UTC
(In reply to Jakub Jelinek from comment #2)
> I see the function is called before selecting a particular alternative, so
> perhaps it means to care only about constraints like "X" and "" and not say
> that mixed with other constraints etc.
> But, shouldn't the code at least skip the =, +, &, % and whitespace from the
> start?  What about other modifiers (the various disparage slightly etc.
> chars)?
> And only consider as empty constraint if after those skips constraint is ""?
> Not really sure if ",,," constraint is valid...
> And, regarding of Eric's change to handle "X" that way, does that really
> apply just to MEM and not SUBREG of MEM too?

Yes.  It seems my bad job on reviewing Richard Sandiford's patch 777e635f1a6c.  Before this patch constraint string was checked only for 'p' which can not have modifiers (although spaces are still possible).

I am afraid that fixing this mess can result in new failures.  But we should do this anyway.
Comment 8 Andrew Pinski 2021-03-07 20:34:59 UTC
*** Bug 99454 has been marked as a duplicate of this bug. ***
Comment 9 Andrew Pinski 2021-03-07 21:00:12 UTC
*** Bug 99455 has been marked as a duplicate of this bug. ***
Comment 10 CVS Commits 2021-03-08 14:26:18 UTC
The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

https://gcc.gnu.org/g:04b4828c6dd215385fde6964a5e13da8a01a78ba

commit r11-7554-g04b4828c6dd215385fde6964a5e13da8a01a78ba
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Mon Mar 8 09:24:57 2021 -0500

    [PR99422] LRA: Skip modifiers when processing memory address.
    
      Function process_address_1 can wrongly look at constraint modifiers
    instead of the 1st constraint itself.  The patch solves the problem.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (skip_contraint_modifiers): New function.
            (process_address_1): Use it before lookup_constraint call.
Comment 11 Vladimir Makarov 2021-03-08 14:31:51 UTC
I think I fixed the PR.  Although there may be necessity for one more patch to solve other process_address_1 issues. I did not decide this yet.
Comment 12 Rainer Orth 2021-03-08 18:53:29 UTC
Unfortunately, I'm still seeing the ICE reported in PR target/99432 on
i386-pc-solaris2.11.
Comment 13 Vladimir Makarov 2021-03-08 18:58:58 UTC
*** Bug 99461 has been marked as a duplicate of this bug. ***
Comment 14 Vladimir Makarov 2021-03-08 19:05:50 UTC
*** Bug 99467 has been marked as a duplicate of this bug. ***
Comment 15 Gerald Pfeifer 2021-03-08 19:33:52 UTC
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99438 still happens,
i.e. i586-unknown-freebsd11.2.

Is anyone able to successfully build any 32-bit x86 target?
Comment 16 Khem Raj 2021-03-08 20:45:17 UTC
the kernel build failures still happen. I am re-opened #99454 and #99455
Comment 17 ro@CeBiTec.Uni-Bielefeld.DE 2021-03-08 21:16:48 UTC
> --- Comment #12 from Rainer Orth <ro at gcc dot gnu.org> ---
> Unfortunately, I'm still seeing the ICE reported in PR target/99432 on
> i386-pc-solaris2.11.

While I could build yesterday's tree with the PR 99378 patch included on
sparc-sun-solaris2.11, today with the PR 99422 patch in tree as well,
bootstrap is broken:

a-ztflau.adb: In function 'Ada.Float_Wide_Wide_Text_Io.Aux_Float.Puts':
a-ztflau.adb:132:8: error: insn does not satisfy its constraints:
(insn 174 39 180 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
                (const_int -5216 [0xffffffffffffeba0])) [37 %sfp+-5216 S8 A64])
        (reg:DF 40 %f8 [203])) "a-ztflau.adb":117:7 153 {*movdf_insn_sp32}
     (nil))
during RTL pass: reload
+===========================GNAT BUG DETECTED==============================+
| 11.0.1 20210308 (experimental) [master revision 0d9a70ea3881c284b7689b691d54d047b55b486d] (sparc-sun-solaris2.11) GCC error:|
| in extract_constrain_insn, at recog.c:2670                               |
| Error detected around a-ztflau.adb:132:8                 

Reverting both patches locally allowed both sparc-sun-solaris2.11 and
i386-pc-solaris2.11 bootstraps to succeed.
Comment 18 Rainer Orth 2021-03-10 14:06:22 UTC
With the two patches for PR 99454, a i386-pc-solaris2.11 bootstraps succeeds.
Thanks.

Unfortunately, on the SPARC side I still get the ICE reported in Comment 17.
Comment 19 Eric Botcazou 2021-03-10 16:10:13 UTC
> a-ztflau.adb: In function 'Ada.Float_Wide_Wide_Text_Io.Aux_Float.Puts':
> a-ztflau.adb:132:8: error: insn does not satisfy its constraints:
> (insn 174 39 180 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
>                 (const_int -5216 [0xffffffffffffeba0])) [37 %sfp+-5216 S8
> A64])
>         (reg:DF 40 %f8 [203])) "a-ztflau.adb":117:7 153 {*movdf_insn_sp32}
>      (nil))

The offset is out of the allowed range [-4096, 4095] so something is broken.
Comment 20 Vladimir Makarov 2021-03-10 18:46:13 UTC
I started to work on another, more safe approach to solve the problems.  I hope the patch will be ready today or tomorrow.
Comment 21 CVS Commits 2021-03-10 21:16:28 UTC
The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

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

commit r11-7613-gd38bbb85117a9947797f10d459fe0c23ea479313
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Wed Mar 10 16:15:08 2021 -0500

    [PR99422] LRA: Don't check unknown constraint, use X for empty constraint
    
    Using CONSTRAINT__UNKNOWN was a bad idea, although it triggered a lot
    hidden bugs.  It is better to use X instead of empty constraint.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (process_address_1): Don't check unknown
            constraint, use X for empty constraint.
Comment 22 Vladimir Makarov 2021-03-10 21:21:43 UTC
Could you check the patch on the failed bootstraps.  I have no access to solaris machines.
Thank you.
Comment 23 ro@CeBiTec.Uni-Bielefeld.DE 2021-03-11 09:44:08 UTC
> --- Comment #22 from Vladimir Makarov <vmakarov at gcc dot gnu.org> ---
> Could you check the patch on the failed bootstraps.  I have no access to

Unfortunately, it didn't help.  I continue to revert the patches for PRs
99378, 99422, and 99454 locally for now.

> solaris machines.

Technically, you have ;-)  There's gcc211 in the cfarm, but
unfortunately the installation is highly nonstandard, doesn't follow
cfarm conventions, and lacks GNAT.  While I do have a local gcc
installation on the box without these issues, I agree it would be quite
tedious to use.  I suspect that the same issue occurs on Linux/sparc64,
too (gcc202), but trying that on a machine that's pretty unreliable
wouldn't be my idea of fun.
Comment 24 Eric Botcazou 2021-03-11 10:13:34 UTC
> Technically, you have ;-)  There's gcc211 in the cfarm, but
> unfortunately the installation is highly nonstandard, doesn't follow
> cfarm conventions, and lacks GNAT.  While I do have a local gcc
> installation on the box without these issues, I agree it would be quite
> tedious to use.  I suspect that the same issue occurs on Linux/sparc64,
> too (gcc202), but trying that on a machine that's pretty unreliable
> wouldn't be my idea of fun.

This can be reproduced with a minimal Ada cross-compiler, i.e. you just need the gnat1 compiler, the skeleton of libada and the command line:
  $(srcdir)/configure --target=sparc-sun-solaris2.11 --enable-languages=c,c++,ada
  make CFLAGS=-g CXXFLAGS=-g ADAFLAGS="-gnatpga -gnatws"
  cd gcc/ada; make gnatlib
  cd rts; ../../gnat1 -quiet a-lfztio.ads -gnatpg

a-ztflau.adb: In function 'Ada.Long_Float_Wide_Wide_Text_Io.Aux_Float.Put':
a-ztflau.adb:101:8: error: insn does not satisfy its constraints:
(insn 32 31 88 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
                (const_int -5232 [0xffffffffffffeb90])) [29 %sfp+-5232 S8 A64])
        (reg:DF 40 %f8 [orig:109 _1 ] [109])) "a-ztflau.adb":99:7 153 {*movdf_insn_sp32}
     (nil))
during RTL pass: reload
+===========================GNAT BUG DETECTED==============================+
| 11.0.1 20210310 (experimental) [master revision 1c3c12b0a6f:f62bd375583:47403a0eefac52636db768dc46c3c88a2cd4b28e] (sparc-sun-solaris2.11) GCC error:|
| in extract_constrain_insn, at recog.c:2670                               |
| Error detected around a-ztflau.adb:101:8
Comment 25 Vladimir Makarov 2021-03-11 19:11:53 UTC
(In reply to Eric Botcazou from comment #24)
 
> This can be reproduced with a minimal Ada cross-compiler, i.e. you just need
> the gnat1 compiler, the skeleton of libada and the command line:
>   $(srcdir)/configure --target=sparc-sun-solaris2.11
> --enable-languages=c,c++,ada
>   make CFLAGS=-g CXXFLAGS=-g ADAFLAGS="-gnatpga -gnatws"
>   cd gcc/ada; make gnatlib
>   cd rts; ../../gnat1 -quiet a-lfztio.ads -gnatpg
> 
> a-ztflau.adb: In function 'Ada.Long_Float_Wide_Wide_Text_Io.Aux_Float.Put':
> a-ztflau.adb:101:8: error: insn does not satisfy its constraints:
> (insn 32 31 88 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
>                 (const_int -5232 [0xffffffffffffeb90])) [29 %sfp+-5232 S8
> A64])
>         (reg:DF 40 %f8 [orig:109 _1 ] [109])) "a-ztflau.adb":99:7 153
> {*movdf_insn_sp32}
>      (nil))
>

Thank you.  I reproduced it.
The patches probably triggered a hidden bug.
I'll investigate more and write my findings today.
Comment 26 Vladimir Makarov 2021-03-11 21:34:45 UTC
Here are my findings.

Before the patches function process_address_1 used CONSTRAINT__UNKNOWN (taken from '=' of constraint "=T,..." and this is wrong) to check validity address.  It was invalid and LRA added reloads for the address.

After the patches, the function uses CONTSTRAINT_T (taken from 'T').  For constraint T sparc code says that the memory address is ok and LRA keeps the address and does not generate reloads.

That is wrong.  Sparc code should say LRA that the address is wrong.  Function sparc.c:memory_ok_for_ldd is responsible for this.

If I apply the following patch

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c                                                                                                                                                                                              
index f1504172022..ac83f900964 100644                                                                                                                                                                                                                         
--- a/gcc/config/sparc/sparc.c                                                                                                                                                                                                                                
+++ b/gcc/config/sparc/sparc.c                                                                                                                                                                                                                                
@@ -9230,6 +9230,9 @@ memory_ok_for_ldd (rtx op)                                                                                                                                                                                                              
   if (! can_create_pseudo_p ()                                                                                                                                                                                                                               
       && !strict_memory_address_p (Pmode, XEXP (op, 0)))                                                                                                                                                                                                     
     return 0;                                                                                                                                                                                                                                                
+  if (can_create_pseudo_p ()                                                                                                                                                                                                                                 
+      && !memory_address_p (Pmode, XEXP (op, 0)))                                                                                                                                                                                                            
+    return 0;                                                                                                                                                                                                                                                

   return 1;
 }

the problem is gone.

I think target code is responsible for the bug and fix should be there not in LRA.
Comment 27 Eric Botcazou 2021-03-11 23:38:09 UTC
> Before the patches function process_address_1 used CONSTRAINT__UNKNOWN
> (taken from '=' of constraint "=T,..." and this is wrong) to check validity
> address.  It was invalid and LRA added reloads for the address.

Is that because T is a special_memory_constraint or did it happen with a regular memory_constraint as well?

> After the patches, the function uses CONTSTRAINT_T (taken from 'T').  For
> constraint T sparc code says that the memory address is ok and LRA keeps the
> address and does not generate reloads.

So LRA does not check that the memory is valid for special_memory_constraint or memory_constraint, and the constraint must do it on its own?  Then it's probably inherited from reload and I guess that

  if (! can_create_pseudo_p ()
      && !strict_memory_address_p (Pmode, XEXP (op, 0)))
    return 0;

was supposed to do it, in which case David and I missed it when we converted the port to LRA.  This would point to:

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index f1504172022..d8860f908e9 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -9227,7 +9227,7 @@ memory_ok_for_ldd (rtx op)
   if (TARGET_ARCH32 && !mem_min_alignment (op, 8))
     return 0;
 
-  if (! can_create_pseudo_p ()
+  if ((lra_in_progress || reload_in_progress)
       && !strict_memory_address_p (Pmode, XEXP (op, 0)))
     return 0;

Thanks for the investigation, I'll take over from there.
Comment 28 CVS Commits 2021-03-12 17:52:23 UTC
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

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

commit r11-7650-gd8b84e2771fc2495493d0c66c3cba714484757d7
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Mar 12 17:07:20 2021 +0100

    Fix memory constraint bug in SPARC back-end
    
    It's a bug exposed by the recent LRA changes, whereby the T constraint
    fails to behave properly when LRA is enabled (unlike when reload is
    enabled).  The patch also gets rid of the awkward W constraint, which
    is strictly equivalent to m in 64-bit mode and, as a result, renames
    the w constraint into W.
    
    gcc/
            PR target/99422
            * config/sparc/constraints.md (w): Rename to...
            (W): ... this and ditch previous implementation.
            * config/sparc/sparc.md (*movdi_insn_sp64): Replace W with m.
            (*movdf_insn_sp64): Likewise.
            (*mov<VM64:mode>_insn_sp64): Likewise.
            * config/sparc/sync.md (*atomic_compare_and_swap<mode>_1): Replace
            w with W.
            (atomic_compare_and_swap_leon3_1): Likewise.
            (*atomic_compare_and_swapdi_v8plus): Likewise.
            * config/sparc/sparc.c (memory_ok_for_ldd): Remove useless test on
            architecture and add missing address validity check during LRA.
Comment 29 Jakub Jelinek 2021-03-16 10:36:03 UTC
So fixed?
Comment 30 Jakub Jelinek 2021-03-16 19:26:04 UTC
Assuming it is.
Comment 31 CVS Commits 2021-03-18 19:59:31 UTC
The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

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

commit r11-7725-ga4670f58ebff805e35268542aac35f9791980954
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Thu Mar 18 15:58:26 2021 -0400

    [PR99422] LRA: Use lookup_constraint only for a single constraint in process_address_1.
    
    This is an additional patch for PR99422.  In process_address_1 we
    look only at the first constraint in the 1st alternative
    and ignore all other possibilities.  As we don't know what
    alternative and constraint will be used at this stage, we can be sure
    only for a single constraint with one alternative and should use unknown
    constraint for all other cases.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (process_address_1): Use lookup_constraint
            only for a single constraint.