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 [details]
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 [details]
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

(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.

*** 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 kiko@linaro.org.

-- 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.i:41 198 {*arm_cmpsi_insn}
     (nil))

(insn 189 188 191 13 (set (reg/v:SI 136 [ badarg ])
        (if_then_else:SI (eq (reg:CC 24 cc)
                (const_int 0 [0]))
            (reg:SI 168)
            (const_int 0 [0]))) ./t.i:41 220 {*movsicc_insn}
     (expr_list:REG_DEAD (reg:SI 168)
        (nil)))

(insn 191 189 153 13 (set (reg/v:SI 135 [ noout ])
        (if_then_else:SI (ne (reg:CC 24 cc)
                (const_int 0 [0]))
            (reg/v:SI 135 [ noout ])
            (const_int 1 [0x1]))) ./t.i:41 220 {*movsicc_insn}
     (expr_list:REG_DEAD (reg:CC 24 cc)
        (nil)))
 

is transformed to :

(note 97 96 188 13 NOTE_INSN_DELETED)

(note 188 97 189 13 NOTE_INSN_DELETED)

(insn 189 188 191 13 (set (reg/v:SI 136 [ badarg ])
        (const_int 0 [0])) ./t.i:41 624 {*arm_movsi_vfp}
     (nil))

(insn 191 189 153 13 (parallel [
            (set (reg/v:SI 135 [ noout ])
                (if_then_else:SI (ne (reg:SI 168)
                        (const_int 0 [0]))
                    (reg/v:SI 135 [ noout ])
                    (const_int 1 [0x1])))
            (clobber (reg:CC 24 cc))
        ]) ./t.i:41 273 {movcond}
     (expr_list:REG_UNUSED (reg:CC 24 cc)
        (expr_list:REG_DEAD (reg:SI 168)
            (nil))))


Look at the use of reg:SI 168  in insn 191 rather than reg:SI r0 . That's where it all goes pear shaped as we've now used reg:SI 168 which we've not defined before. The other bits of info from .combine is as below.  

Trying 97 -> 188:
Successfully matched this instruction:
(parallel [
        (set (reg:CC 24 cc)
            (compare:CC (reg:SI 0 r0)
                (const_int 0 [0])))
        (set (reg:SI 168)
            (reg:SI 0 r0))
    ])

and then further trying to combine insn 188 into insn 189 we get the following 


Trying 188 -> 189:
Failed to match this instruction:
(parallel [
        (set (reg/v:SI 136 [ badarg ])
            (const_int 0 [0]))
        (set (reg:CC 24 cc)
            (compare:CC (reg:SI 168)
                (const_int 0 [0])))
    ])
Failed to match this instruction:
(parallel [
        (set (reg/v:SI 136 [ badarg ])
            (const_int 0 [0]))
        (set (reg:CC 24 cc)
            (compare:CC (reg:SI 168)
                (const_int 0 [0])))
    ])
Successfully matched this instruction:
(set (reg:CC 24 cc)
    (compare:CC (reg:SI 168)
        (const_int 0 [0])))
Successfully matched this instruction:
(set (reg/v:SI 136 [ badarg ])
    (const_int 0 [0]))
deferring deletion of insn with uid = 188.
modifying insn i2   188 cc:CC=cmp(r168:SI,0)
      REG_DEAD: r168:SI
deferring rescan insn with uid = 188.
modifying insn i3   189 r136:SI=0
deferring rescan insn with uid = 189.
 
 I still need to debug through try_combine to figure out where everything goes pear shaped but that's my understanding on this bug as of today. 

Ofcourse, the simple workaround is to simply disable the transformation in this form in the ARM backend (*movsi_compare0) but I don't want to do that before I've finished stepping through combine. 

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 794b865..01c2a87 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -5380,7 +5380,7 @@
                    (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
        (match_dup 1))]
-  "TARGET_32BIT"
+  "TARGET_32BIT && 0"
   "@
    cmp%?\\t%0, #0
    sub%.\\t%0, %1, #0"

(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

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  <ramana.radhakrishnan@linaro.org>

	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

GCC 4.6.3 is being released.

A quick comment for folks trying to work around Buildroot problems related to this:

As of this date, this problem still occurs using current Buildroot materials and attempt to build for, for instance, the AT91SAM9M10-G45-EK.  The failure occurs in the SSL library (presumably a stress point due to how heavily optimized it is).

For the moment, the workaround is to set the GCC compiler version to 4.5.x, instead of 4.6.x.  This seems to work.

FWIW, I would recommend testing a fairly thorough (lots of packages, including SSL and Xorg) Buildroot test for an ARM9 target as a good way to check whether or not this bug has been resolved.  I'd be happy to try that myself when there's a fix.

(In reply to comment #20)
Did the patch which was applied at revision 183512 fix this bug?  If so please mark this bug as being fixed for 4.7.0 and 4.8.0 and not a regression.

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

http://gcc.gnu.org/ml/gcc-patches/2012-01/msg00666.html and follow-ups
claim to have fixed this for GCC 4.7 and GCC 4.8.

Ramana, you should take care of the bug reports assigned to you.  Also,
you should have added a test case.

http://gcc.gnu.org/PR56561#c4 has a patch for GCC 4.6.

In case this won't be fixed in 4.6, then it is probably good to set the status to WONTFIX so people know that they have to deal with this on their own.

(In reply to comment #26)
> In case this won't be fixed in 4.6, then it is probably good to set the status
> to WONTFIX so people know that they have to deal with this on their own.

It looks like I missed this backport when I did it earlier - I'll look into a backport over the weekend.


Thanks,
Ramana

Fixed for 4.6 by http://gcc.gnu.org/ml/gcc-cvs/2013-04/msg00218.html