armel FTBFS with gcc 4.5 org 4.6 O2 and fPIC

Created attachment 23786
reduced testcase

Hello,

I tried to crosscompile openssl to arm and it stopped with the following error message:

{standard input}: Assembler messages:
{standard input}:113: Error: can't resolve `.LC4' {.rodata.str1.1 section} - `.LPIC4' {*UND* section}

This happens only if I try to compile the file with "-Os -fPIC".

/data/br-build/host/usr/bin/arm-unknown-linux-uclibcgnueabi-gcc -fPIC -Os -c pkeyparam.out.i

Output of arm-unknown-linux-uclibcgnueabi-gcc -v:

COLLECT_GCC=/data/br-build/host/usr/bin/arm-unknown-linux-uclibcgnueabi-gcc
COLLECT_LTO_WRAPPER=/data/br-build/host/usr/libexec/gcc/arm-unknown-linux-uclibcgnueabi/4.6.0/lto-wrapper
Target: arm-unknown-linux-uclibcgnueabi
Configured with: /data/br-build/toolchain/gcc-4.6.0/configure --prefix=/home/data/br-build/host/usr --build=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu --target=arm-unknown-linux-uclibcgnueabi --enable-languages=c --with-sysroot=/data/br-build/host/usr/arm-unknown-linux-uclibcgnueabi/sysroot --with-build-time-tools=/data/br-build/host/usr/arm-unknown-linux-uclibcgnueabi/bin --disable-__cxa_atexit --enable-target-optspace --with-gnu-ld --disable-libssp --disable-multilib --disable-tls --enable-shared --with-gmp=/data/br-build/host/usr --with-mpfr=/data/br-build/host/usr --with-mpc=/data/br-build/host/usr --enable-threads --disable-decimal-float --with-float=soft --with-abi=aapcs-linux --with-pkgversion='Buildroot 2011.05-git-dirty' --with-bugurl=http://bugs.buildroot.net/
Thread model: posix
gcc version 4.6.0 (Buildroot 2011.05-git-dirty)

Output of arm-unknown-linux-uclibcgnueabi-as -version:

GNU assembler (GNU Binutils) 2.21
Copyright 2010 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `arm-unknown-linux-uclibcgnueabi'

Regards,
  Gerhard

Works for me on armv5tel-linux-gnueabi with gcc-4.6-20110325 (r171529) and binutils-2.20.1 (heavily updated). Please attach the intermediate assembly file.

Created attachment 23791
generated assembler

On line 114, the generates assembler code refers to .LPIC4, which does not exists.

(In reply to comment #2)
> Created attachment 23791 [details]
> generated assembler
>
> On line 114, the generates assembler code refers to .LPIC4, which does not
> exists.

Indeed. If I compile with -mcpu=arm9tdmi as your assembly file indicates then I lose several lines of code, including the .LPIC4 label and a strcmp() call, but the reference to .LPIC4 remained. Normally I have -march=armv5te -mtune=xscale, and in that case the .LPIC4 label and surrounding code is not lost.

Works(*) with gcc-4.4.5 and 4.5.2, so it's a regression.

(*) Had to eliminate some apparent C1X-isms from the test case though.

It's triggered by r163998:

Author: matz
Date: Wed Sep 8 12:34:52 2010
New Revision: 163998

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163998
Log:
 PR tree-optimization/33244
 * tree-ssa-sink.c (statement_sink_location): Don't sink into
 empty loop latches.

Created attachment 23793
reduced^2 test case

Using trunk, r171422, I have compiled the reduced test case as follows:

arm-none-linux-gnueabi-gcc -fPIC -Os -c pr48308.c -mcpu=arm9tdmi -marm
pr48308.s: Assembler messages:
pr48308.s:97: Error: can't resolve `.LC4' {.rodata.str1.1 section} - `.LPIC4' {*UND* section}

With --save-temps, you can see the missing .LPIC4 in the .s.

This is therefore confirmed in trunk as well.

FYI: the problem doesn't happen with -mthumb.

I hit this with Linaro GCC 4.6 (4.6.1-based) and the same pkeyparam.c from OpenSSL. I am also compiling with -Os and -fPIC, and implicitly with -mthumb (the default in my toolchain); so it's not specific to ARM mode.

The situation appears to be that two pc-relative fetches (artifacts of -fPIC and string literals) get folded together, losing one of the labels (needed for calculation of the offset in the table of PIC indirections).

Reverting http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163998 makes the problem go away, at least at compile time; I should be able to run a test suite soon.

Not sure if this is a target or tree-optimization issue yet.

Ramana

Created attachment 25211
Preprocessed source

Building gmime with GCC 4.6.1 for ARM gives the following error:

michaelh@ursa1:~/linaro/bugs$ gcc -c -O2 -fPIC -save-temps gmime2.i
gmime2.s: Assembler messages:
gmime2.s:1007: Error: can't resolve `.rodata' {.rodata section} - `.LPIC18' {*UND* section}

The assembler is correct - the code references a label which doesn't exist.

The suspicious code is around:
 bl g_hash_table_new(PLT)
 mov sl, r0
 add r0, sp, #32
 bl g_mime_decode_lwsp(PLT)
 ldr r3, .L146+24
 ldr r1, [sp, #4]
 str r3, [sp, #16]
 ldr r3, [sp, #32]
...
.L146:
...
 .word .LANCHOR0-(.LPIC18+4)

The 'ldr r3, .L146+24' references the offset. The value is spilt to the stack before the normal `add r3, pc` and doesn't seem to be used afterwards.

The work-around is to compile at -O1 or -O3. The fault also exists in gcc-4.6.1. It does not exist in gcc-4.5.3 or trunk r178025.

This is in a Cortex-A9+NEON+Thumb-2 configuration.

I suspect this is a dupe of PR48308.

(In reply to comment #8)
> Not sure if this is a target or tree-optimization issue yet.

The tree level change just exposed the issue really. I think you need to look at the RTL level to see where the label disappears.

Looks like a DUP of PR48308.

*** This bug has been marked as a duplicate of bug 48308 ***

*** Bug 50313 has been marked as a duplicate of this bug. ***

GCC 4.6.2 is being released.

Please tell me this bug is fixed in gcc 4.6.2 release? If this is fixed in gcc4.6.2 release can I use the fix patch into gcc 4.6-linaro?

Thanks & Regards,
Madhu

(In reply to comment #12)
> Please tell me this bug is fixed in gcc 4.6.2 release? If this is fixed in
> gcc4.6.2 release can I use the fix patch into gcc 4.6-linaro?
>
> Thanks & Regards,
> Madhu

This has not been fixed yet. That's evident from the status of the bug.

Ramana

Note, can't be reproduced on the trunk, the strcmp isn't DCEd there, but guess the problem is just latent there.

It looks like a target bug to me. Before RTL loop opts we have:
(insn 91 90 92 13 (set (reg:SI 167)
        (unspec:SI [
                (const:SI (unspec:SI [
                            (symbol_ref/v/f:SI ("*.LC4") [flags 0x82] <var_decl 0x7f5ebb0a5500 *.LC4>)
                            (const:SI (plus:SI (unspec:SI [
                                            (const_int 4 [0x4])
                                        ] 21)
                                    (const_int 8 [0x8])))
                        ] 27))
            ] 3)) pr48308.i:228 170 {pic_load_addr_32bit}
     (nil))

(insn 92 91 94 13 (set (reg:SI 167)
        (unspec:SI [
                (reg:SI 167)
                (const_int 8 [0x8])
                (const_int 4 [0x4])
            ] 4)) pr48308.i:228 173 {pic_add_dot_plus_eight}
     (expr_list:REG_EQUAL (symbol_ref/v/f:SI ("*.LC4") [flags 0x82] <var_decl 0x7f5ebb0a5500 *.LC4>)
        (nil)))

and the pseudo 167 is then used to load one of the strcmp parameters.
Then (probably loop invariant motion) moves insn 91 before the loop, as it looks to be loop invariant, but insn 92 is kept in the loop.
Next during RA, the register pressure is high and thus pseudo 167 is spilled, so
before the loop there is a store. Then during the *.csa pass DCE is performed and the strcmp is removed, which means insn 92 is removed as well, but the store before the loop of course is kept. And there is no further DSE pass that would optimize that (now dead) store away.
So, IMHO arm_reorg needs to handle this case, find out what minipool entries don't have the corresponding UNSPEC_PIC_BASE insn and handle them somehow (either by emitting there a dummy 0 or similar, or trying to replace the insn with UNSPEC_PIC_SYM with something else, ...).
That said, perhaps it would be nice to help the loop optimizers somehow figure out that even the UNSPEC_PIC_BASE is loop invariant (wrap it into CONST?).

I'm away of holiday until the 4th of January. For anything urgent,
please contact <email address hidden>.

-- Michael

Ramana (or some other ARM maintainer), could you please try to implement this? Thanks.

I think it would be better to just generate a single insn early on and then to split it once the loop optimizers have had a chance to run. Then the optimizers should be able to throw the whole expression away if it's not needed and hoist it as a whole if it's inside a loop.

Splitting it before sched1 would be ideal, but the split could be delayed until after reload if necessary.

(In reply to comment #14)
> Note, can't be reproduced on the trunk, the strcmp isn't DCEd there, but guess
> the problem is just latent there.
>
> It looks like a target bug to me. Before RTL loop opts we have:
> (insn 91 90 92 13 (set (reg:SI 167)
> (unspec:SI [
> (const:SI (unspec:SI [
> (symbol_ref/v/f:SI ("*.LC4") [flags 0x82]
> <var_decl 0x7f5ebb0a5500 *.LC4>)
> (const:SI (plus:SI (unspec:SI [
> (const_int 4 [0x4])
> ] 21)
> (const_int 8 [0x8])))
> ] 27))
> ] 3)) pr48308.i:228 170 {pic_load_addr_32bit}
> (nil))
>
> (insn 92 91 94 13 (set (reg:SI 167)
> (unspec:SI [
> (reg:SI 167)
> (const_int 8 [0x8])
> (const_int 4 [0x4])
> ] 4)) pr48308.i:228 173 {pic_add_dot_plus_eight}
> (expr_list:REG_EQUAL (symbol_ref/v/f:SI ("*.LC4") [flags 0x82] <var_decl
> 0x7f5ebb0a5500 *.LC4>)
> (nil)))
>
> and the pseudo 167 is then used to load one of the strcmp parameters.
> Then (probably loop invariant motion) moves insn 91 before the loop, as it
> looks to be loop invariant, but insn 92 is kept in the loop.
> Next during RA, the register pressure is high and thus pseudo 167 is spilled,
> so
> before the loop there is a store. Then during the *.csa pass DCE is performed
> and the strcmp is removed, which means insn 92 is removed as well, but the
> store before the loop of course is kept. And there is no further DSE pass that
> would optimize that (now dead) store away.
> So, IMHO arm_reorg needs to handle this case, find out what minipool entries
> don't have the corresponding UNSPEC_PIC_BASE insn and handle them somehow
> (either by emitting there a dummy 0 or similar, or trying to replace the insn
> with UNSPEC_PIC_SYM with something else, ...).
> That said, perhaps it would be nice to help the loop optimizers somehow figure
> out that even the UNSPEC_PIC_BASE is loop invariant (wrap it into CONST?).

Found it I think- the problem is way before any of this . A transformation
in combine looks suspicious.

The first clue to all this was that the compiler was removing a call of strcmp with .LC4 which is the constant string "-noout" . If you look at the testcase, there is a use of noout afterwards which is to call :

   if (!noout)
     PEM_write_bio_Parameters(out,pkey);

However the condition

 else if (strcmp(*args,"-text") == 0)
       text=1;

has a value of text set to 1.

However given text is not used later that is the only call
to strcmp that can be removed safely. However the strcmp of "-text" is retained in the final code generated but the call to strcmp of "-noout" is the one that's getting eliminated.

Here's the suspicious transformation after combine:

(insn 97 96 188 13 (set (reg:SI 168)
        (reg:SI 0 r0)) ./t.i:41 624 {*arm_movsi_vfp}
     (expr_list:REG_DEAD (reg:SI 0 r0)
        (nil)))

(insn 188 97 189 13 (set (reg:CC 24 cc)
        (compare:CC (reg:SI 168)
            (const_int 0 [0]))) ./t....

(In reply to comment #14)
> Note, can't be reproduced on the trunk, the strcmp isn't DCEd there, but guess
> the problem is just latent there.

Latent still in trunk with the testcase - you can try it with -fdbg-cnt=cprop:0 with the first reduced testcase and you should be able to see the same behaviour with trunk. The other testcase which is in PR50313 also fails with the same error on trunk (with -Os -fPIC -mcpu=arm9tdmi -marm )

The problem with reduced^2 testcase or reduced testcase appears to get fixed if we add LOG_LINKS to keep track of the dummy i1 in the form of a patch of the following nature.

combine.c: try_combine attempts to create a dummy i1 instruction in case i1 is NULL and i2 looks like

 (parallel [(set (reg:CC X) (compare:CC OP (const_int 0)))
     (set Y OP)])

which is exactly how the intermediate instruction looks

(parallel [
        (set (reg:CC 24 cc)
            (compare:CC (reg:SI 0 r0)
                (const_int 0 [0])))
        (set (reg:SI 168)
            (reg:SI 0 r0))
    ])

i1 now becomes

  (set (reg:SI 168)
            (reg:SI 0 r0))

and i2 becomes

    (set (reg:CC 24 cc)
            (compare:CC (reg:SI 0 r168)
                (const_int 0 [0])))

but there are no LOG_LINKS to indicate that even when we've created the dummy i1 - i2 actually feeds into i1. After changing try_combine[1] to create a LOG_LINKS to indicate this relationship between i1 and i2 we no longer eliminate the wrong strcmp call and therefore this should IMHO be the handled in combine rather than faffing around in the backend.

However this doesn't fix the testcase from PR50313 (gmime.i) which prima-facie appeared to exhibit the same behaviour as this particular testcase and that needs reopening and further investigation to be sure that the IR is valid when cfgcleanup decides to eliminate the PIC insn in.

cheers
Ramana

diff --git a/gcc/combine.c b/gcc/combine.c
index 4178870..f6b8769 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2865,6 +2865,8 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int *new_direct_jump_p,
          SUBST (PATTERN (i2), XVECEXP (PATTERN (i2), 0, 0));
          SUBST (XEXP (SET_SRC (PATTERN (i2)), 0),
                 SET_DEST (PATTERN (i1)));
+ LOG_LINKS (i2) = alloc_insn_link (i1, LOG_LINKS (i2));
+
        }
     }
 #endif

reopening this as the proposed fix for 48308 doesn't appear to fix the issue and the same problem in combine doesn't seem to happen in this case.

Ramana

Created attachment 26314
smaller testcase

Better reduced testcase. Fails on trunk with -Os -fPIC -mcpu=arm9tdmi or -Os -fPIC -mthumb -mcpu=cortex-a9

Author: ramana
Date: Fri Jan 20 09:22:14 2012
New Revision: 183328

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183328
Log:

Fix PR target/50313

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
    trunk/gcc/config/arm/arm.md

Author: ramana
Date: Wed Jan 25 08:52:39 2012
New Revision: 183512

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183512
Log:

2012-01-25 Ramana Radhakrishnan <email address hidden>

 PR rtl-optimization/48308
 * combine.c (enum undo_kind): Add UNDO_LINKS.
 (struct undo): Add member l to other_contents and where.
 (do_SUBST_LINK): New.
 (SUBST_LINK): New.
 (try_combine): Handle LOG_LINKS for the dummy i1 case.
 (undo_all): Handle UNDO_LINKS.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c

Author: ramana
Date: Mon Jan 30 14:35:05 2012
New Revision: 183727

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183727
Log:

Fix PR target/50313

2012-01-30 Ramana Radhakrishnan <email address hidden>

 Backport from mainline.
        2012-01-20 Ramana Radhakrishnan <email address hidden>

 PR target/50313
 * config/arm/arm.c (arm_load_pic_register): Use
 gen_pic_load_addr_unified. Delete calls to gen_pic_load_addr_32bit
 , gen_pic_add_dot_plus_eight and gen_pic_add_dot_plus_four.
 (arm_pic_static_addr): Likewise.
 (arm_rtx_costs_1): Adjust cost for UNSPEC_PIC_UNIFIED.
 (arm_note_pic_base): Handle UNSPEC_PIC_UNIFIED.
 * config/arm/arm.md (UNSPEC_PIC_UNIFIED): Define.
 (pic_load_addr_unified): New.

Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/arm/arm.c
    branches/gcc-4_6-branch/gcc/config/arm/arm.md

Backported now to GCC 4.6 branch.

Ramana