Bug 43061 - 47 new GCC HEAD@156527 regressions
Summary: 47 new GCC HEAD@156527 regressions
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: objc (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-14 13:03 UTC by Dominique d'Humieres
Modified: 2010-02-21 12:46 UTC (History)
4 users (show)

See Also:
Host: *-apple-darwin*
Target: *-apple-darwin*
Build: *-apple-darwin*
Known to work:
Known to fail:
Last reconfirmed:


Attachments
generated asm differences with/without r156519 (539 bytes, text/plain)
2010-02-14 16:32 UTC, IainS
Details
optimized tree diffs. (362 bytes, text/plain)
2010-02-14 16:33 UTC, IainS
Details
diffs for all fdump-tree-all output (2.22 KB, text/plain)
2010-02-14 16:45 UTC, IainS
Details
gdb-output for CLASS_REFERENCES_0 (609 bytes, text/plain)
2010-02-14 21:13 UTC, IainS
Details
gimple for cascading-1.m @ trunk 156760 (430 bytes, text/plain)
2010-02-14 21:55 UTC, IainS
Details
-fdump-ipa-all/static-var cascading-1.m @ trunk 156760 (495 bytes, text/plain)
2010-02-14 22:11 UTC, IainS
Details
cascading-1.m/-fdump-ipa-all/pure-const @ trunk 15670 (646 bytes, text/plain)
2010-02-14 22:16 UTC, IainS
Details
asm out from -O1 -g cascading-1.m @trunk 156760 (3.89 KB, text/plain)
2010-02-14 23:22 UTC, IainS
Details
attach "used" attribute as well as marking vars used. (349 bytes, patch)
2010-02-15 23:10 UTC, IainS
Details | Diff
patch with CLASS and SELECTOR refs. marked as TREE_ADDRESSABLE & DECL_PRESERVE_P (for NeXT runtime) (561 bytes, patch)
2010-02-16 14:58 UTC, IainS
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominique d'Humieres 2010-02-14 13:03:33 UTC
Between revisions156515 and 156527 many new failures have appeared in the objc test suite (see http://gcc.gnu.org/ml/gcc-regression/2010-02/msg00013.html ). These errors are still there at revision 156749 (see http://gcc.gnu.org/ml/gcc-testresults/2010-02/msg01291.html or http://gcc.gnu.org/ml/gcc-testresults/2010-02/msg01290.html ) and seem to need '-On  -fnext-runtime' with n!=0.

Note that the errors do not appear on the Jack Howarth tests for x86_64-apple-darwin10.3.0 (see http://gcc.gnu.org/ml/gcc-testresults/2010-02/msg01258.html ) while they do on  x86_64-apple-darwin10.2 (see http://gcc.gnu.org/ml/gcc-testresults/2010-02/msg00747.html ).
Comment 1 IainS 2010-02-14 13:46:44 UTC
confirmed, this can be reproduced outside the testsuite framework:
for example... [ppc/darwin9/156749];

$ ./gcc/xgcc -B gcc ../gcc-4-5-trunk/gcc/testsuite/objc/execute/cascading-1.m -fnext-runtime -O1 -o tc -lobjc -g -save-temps

$ gdb tc
GNU gdb 6.3.50-20050815 (Apple version gdb-967) (Tue Jul 14 02:15:14 UTC 2009)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "powerpc-apple-darwin"...Reading symbols for shared libraries ..... done

(gdb) run
Starting program: /Volumes/ScratchCS/gcc-4-5-trunk-build/tc 
Reading symbols for shared libraries +++... done

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x466f6f20
0x9622dfd8 in objc_msgSend ()
(gdb) backtrace
#0  0x9622dfd8 in objc_msgSend ()
#1  0x00001f8c in main (argc=<value temporarily unavailable, due to optimizations>, argv=<value temporarily unavailable, due to optimizations>) at ../gcc-4-5-trunk/gcc/testsuite/objc/execute/cascading-1.m:33
(gdb) 
Comment 2 Richard Biener 2010-02-14 15:25:27 UTC
Track down the regression that caused this and see what actually is the
difference in generated code and/or tree/rtl dumps.  Access to non-free
operating systems is restricted.
Comment 3 IainS 2010-02-14 16:04:51 UTC
(In reply to comment #2)
> Track down the regression that caused this
 
r156519

>and see what actually is the  difference in generated code and/or tree/rtl dumps. 

what output would be the most useful?
Comment 4 Richard Biener 2010-02-14 16:28:04 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Track down the regression that caused this
> 
> r156519
> 
> >and see what actually is the  difference in generated code and/or tree/rtl dumps. 
> 
> what output would be the most useful?

Make one directory for each revision for outputs from -fdump-tree-all,
tar them up and attach them.  A single testcase that passes before and
fails after the revision should be enough.  You can leave out dumps
that are identical for both revisions.
Comment 5 IainS 2010-02-14 16:32:55 UTC
Created attachment 19860 [details]
generated asm differences with/without r156519
Comment 6 IainS 2010-02-14 16:33:39 UTC
Created attachment 19861 [details]
optimized tree diffs.
Comment 7 IainS 2010-02-14 16:45:34 UTC
Created attachment 19862 [details]
diffs for all fdump-tree-all output
Comment 8 Richard Biener 2010-02-14 17:44:47 UTC
Hm.  So CCP through get_symbol_constant_value causes

 <bb 2>:
-  _OBJC_CLASS_REFERENCES_0.2_1 = _OBJC_CLASS_REFERENCES_0;
+  _OBJC_CLASS_REFERENCES_0.2_1 = (struct objc_class *) &_OBJC_CLASS_NAME_0;
   _OBJC_CLASS_REFERENCES_0.3_2 = (struct objc_object *) _OBJC_CLASS_REFERENCES_0.2_1;
-  _OBJC_SELECTOR_REFERENCES_0.4_3 = _OBJC_SELECTOR_REFERENCES_0;
+  _OBJC_SELECTOR_REFERENCES_0.4_3 = (struct objc_selector *) &_OBJC_METH_VAR_NAME_1;
   D.3683_4 = OBJ_TYPE_REF(objc_msgSend;_OBJC_CLASS_REFERENCES_0.3_2->0) (_OBJC_CLASS_REFERENCES_0.3_2, _OBJC_SELECTOR_REFERENCES_0.4_3);
-  _OBJC_SELECTOR_REFERENCES_1.5_5 = _OBJC_SELECTOR_REFERENCES_1;
+  _OBJC_SELECTOR_REFERENCES_1.5_5 = (struct objc_selector *) &_OBJC_METH_VAR_NAME_0;
   OBJ_TYPE_REF(objc_msgSend;D.3683_4->0) (D.3683_4, _OBJC_SELECTOR_REFERENCES_1.5_5);
   return 0;

which means that _OBJC_CLASS_REFERENCES_0 must be a TREE_READONLY static
with an initializer.  I wonder how exactly that tree looks like, so can
you run the compile inside gdb, break on get_symbol_constant_value
until you get _OBJC_CLASS_REFERENCES_0 as argument (may happen multiple
times) and paste the output of

(gdb) call debug_tree (sym)
(gdb) call debug_tree (sym->decl_common.initial)

?

Thanks.
Comment 9 IainS 2010-02-14 18:50:05 UTC
(In reply to comment #8)
> Hm.  So CCP through get_symbol_constant_value causes

> you run the compile inside gdb, break on get_symbol_constant_value

maybe I've messed something up - but setting a break on get_symbol_constant_value ..
.. the cc1obj just completes with "Program exited normally'.
Comment 10 Richard Biener 2010-02-14 19:00:38 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Hm.  So CCP through get_symbol_constant_value causes
> 
> > you run the compile inside gdb, break on get_symbol_constant_value
> 
> maybe I've messed something up - but setting a break on
> get_symbol_constant_value ..
> .. the cc1obj just completes with "Program exited normally'.

That can't be.  Make sure to enable optimization though.
Comment 11 Richard Biener 2010-02-14 19:01:48 UTC
Btw, I cannot make -fnext-runtime work on i?86-linux, it errors at link time with
undefined references.  Any configure options I need to supply?
Comment 12 Andrew Pinski 2010-02-14 19:07:39 UTC
(In reply to comment #11)
> Btw, I cannot make -fnext-runtime work on i?86-linux, it errors at link time
> with
> undefined references.  Any configure options I need to supply?
> 

The next runtime only works on Darwin.
Comment 13 IainS 2010-02-14 21:13:00 UTC
Created attachment 19864 [details]
gdb-output for CLASS_REFERENCES_0
Comment 14 Richard Biener 2010-02-14 21:29:41 UTC
That doesn't make sense.  The symbol is not TREE_READONLY.

Was that dump from inside get_symbol_constant_value?

As the extract only happens from CCP2 I suppose that ipa-reference might
be setting TREE_READONLY on the decl becaue it's static and not written to?
So, can you try with -fno-ipa-reference?  (-fdump-ipa-reference should
show "read-only var OBJC_CLASS_REFERENCES_0" if that is the problem)

Can it be that the next runtime causes functions to be emitted in the
back of cgraph?  Or maybe the objc frontend fails to set TREE_ADDRESSABLE
on those vars? Or it forgets to pass them to the varpool?

Can you attach the full ipa-reference dump and the full 004.gimple dump?
Comment 15 IainS 2010-02-14 21:53:44 UTC
(In reply to comment #14)
> That doesn't make sense.  The symbol is not TREE_READONLY.
> 
> Was that dump from inside get_symbol_constant_value?

yes.
that was from a clean bootstrap of trunk 156760.

> As the extract only happens from CCP2 I suppose that ipa-reference might
> be setting TREE_READONLY on the decl becaue it's static and not written to?
> So, can you try with -fno-ipa-reference?  (-fdump-ipa-reference should
> show "read-only var OBJC_CLASS_REFERENCES_0" if that is the problem)

-fdump-ipa-reference - is unrecognized on my build - do I need a config. option to enable?
-fdump-ipa-all doesn't seem to give a file named as reference...
Comment 16 IainS 2010-02-14 21:55:59 UTC
Created attachment 19866 [details]
gimple for cascading-1.m @ trunk 156760
Comment 17 Richard Biener 2010-02-14 21:56:28 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > That doesn't make sense.  The symbol is not TREE_READONLY.
> > 
> > Was that dump from inside get_symbol_constant_value?
> 
> yes.
> that was from a clean bootstrap of trunk 156760.
> 
> > As the extract only happens from CCP2 I suppose that ipa-reference might
> > be setting TREE_READONLY on the decl becaue it's static and not written to?
> > So, can you try with -fno-ipa-reference?  (-fdump-ipa-reference should
> > show "read-only var OBJC_CLASS_REFERENCES_0" if that is the problem)
> 
> -fdump-ipa-reference - is unrecognized on my build - do I need a config. option
> to enable?
> -fdump-ipa-all doesn't seem to give a file named as reference...

Ah, sorry.  The dump file is called static-var.
Comment 18 IainS 2010-02-14 22:11:58 UTC
Created attachment 19867 [details]
-fdump-ipa-all/static-var cascading-1.m @ trunk 156760

this is with normal options (i.e. the default for ipa-reference)
Comment 19 IainS 2010-02-14 22:16:13 UTC
Created attachment 19868 [details]
cascading-1.m/-fdump-ipa-all/pure-const @ trunk 15670

this is with -fno-ipa-reference.
Comment 20 Richard Biener 2010-02-14 22:25:03 UTC
Not TREE_ADDRESSABLE var _OBJC_CLASS_REFERENCES_0
Not TREE_ADDRESSABLE var _OBJC_SELECTOR_REFERENCES_0
Not TREE_ADDRESSABLE var _OBJC_SELECTOR_REFERENCES_1
read-only var _OBJC_CLASS_REFERENCES_0
read-only var _OBJC_SELECTOR_REFERENCES_0
read-only var _OBJC_SELECTOR_REFERENCES_1

So it's indeed ipa-reference that promotes the static vars and CCP that
propagates the initializer.  I see nothing wrong here, so the ObjC frontend
must do something behind the middle-ends back.

If you look at the final assembler code, is there anything modifying
the contents of those three variables?
Comment 21 IainS 2010-02-14 23:22:23 UTC
Created attachment 19870 [details]
asm out from -O1 -g cascading-1.m @trunk 156760

this is the current asm output - it segfaults on the objc msg call.
Comment 22 Richard Biener 2010-02-14 23:26:57 UTC
There are no modifications visible in the assembly.  But there is magic:

	.objc_cls_refs
	.align 2
L_OBJC_CLASS_REFERENCES_0:
	.long	L_OBJC_CLASS_NAME_0

what is .objc_cls_refs?

Well, obviously some Objective-C maintainer needs to chime in and explain
things.
Comment 23 IainS 2010-02-14 23:57:21 UTC
(In reply to comment #22)
> There are no modifications visible in the assembly.  But there is magic:
> 
>         .objc_cls_refs
>         .align 2
> L_OBJC_CLASS_REFERENCES_0:
>         .long   L_OBJC_CLASS_NAME_0
> 
> what is .objc_cls_refs?

DEF_SECTION (objc_cls_refs_section, SECTION_MERGE, ".objc_cls_refs", 1)

If I've got this correct - this is implementing the constraint that class names are global in the NeXT runtime [at least at release 0/1].  I assume that the section merge arranges that all references to class FOO point to the same and unique class definition. 

> > Well, obviously some Objective-C maintainer needs to chime in and explain
> things.

indeed.
Comment 24 Mike Stump 2010-02-15 17:38:34 UTC
Yes, I think IainS is on the right track, all things in objc_cls_refs escape and can be read and written to in unexpected ways by the runtime.  Setting TREE_ADDRESSABLE sounds like a reasonable step forward.
Comment 25 IainS 2010-02-15 19:54:29 UTC
Hm.  I tried this trivial patch:
Index: gcc/objc/objc-act.c
===================================================================
--- gcc/objc/objc-act.c	(revision 156760)
+++ gcc/objc/objc-act.c	(working copy)
@@ -2533,7 +2533,8 @@
 
   sprintf (buf, "_OBJC_SELECTOR_REFERENCES_%d", selector_reference_idx++);
   decl = start_var_decl (objc_selector_type, buf);
-
+  if (flag_next_runtime) 
+    TREE_ADDRESSABLE (decl) = 1;
   return decl;
 }
 
@@ -2733,6 +2734,8 @@
 
   sprintf (buf, "_OBJC_CLASS_REFERENCES_%d", class_reference_idx++);
   decl = start_var_decl (objc_class_type, buf);
+  if (flag_next_runtime) 
+    TREE_ADDRESSABLE (decl) = 1;
 
   return decl;
 }

and when I check in gdb the trees are marked as addressable [checking in objc-act.c/finish_var_decl() ].
... but something is un-doing this later - and _OBJC_CLASS_REFERENCES_0 is ending up marked as not tree addressable according to the ipa dump.

any ideas where I'm going wrong?
Comment 26 Jakub Jelinek 2010-02-15 21:09:38 UTC
Addressability is recomputed several times.  What you probably want is mark the decl with the used attribute (i.e. add "used" attribute to it, set TREE_USED (decl) = 1 and DECL_PRESERVE_P (decl) = 1).
Comment 27 IainS 2010-02-15 21:51:50 UTC
(In reply to comment #26)
> Addressability is recomputed several times.  What you probably want is mark the
> decl with the used attribute (i.e. add "used" attribute to it, set TREE_USED
> (decl) = 1 and DECL_PRESERVE_P (decl) = 1).

finish_var_decl() contains:
mark_decl_referenced(var); - although this doesn't do anything for csts.
and
TREE_USED (var) = 1;

adding  DECL_PRESERVE_P (decl) = 1 doesn't solve the problem either..

I'll have to dig deeper into what's actually happening/required.
Comment 28 Jakub Jelinek 2010-02-15 22:37:29 UTC
    DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("used"), NULL, DECL_ATTRIBUTES (decl));
is also needed (no idea why ipa*.c/cgraphunit.c use lookup_attribute instead of testing DECL_PRESERVED_P, but they do).
Comment 29 IainS 2010-02-15 23:10:41 UTC
Created attachment 19884 [details]
attach "used" attribute as well as marking vars used.

Jakub's comment seems to do the trick - thanks.
I've applied a "used" attribute in the same place that the TREE_USED is placed.
Comment 30 IainS 2010-02-16 09:23:30 UTC
apropos 
http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00587.html

do you think that _OBJC_CLASS_REFERENCES_* and _OBJC_SELECTOR_REFERENCES_* should be marked as TREE_ADDRESSABLE and DECL_PRESERVE_P in anticipation that this change (ipa* and cgraphunit responding to PRESERVE_P) will occur in the future?

(I took those changes out of this patch on the grounds that they didn't currently achieve anything).
Comment 31 Jakub Jelinek 2010-02-16 10:06:01 UTC
Not sure about TREE_ADDRESSABLE, but certainly DECL_PRESERVED_P should be set as well.
Comment 32 Richard Biener 2010-02-16 10:14:28 UTC
(In reply to comment #24)
> Yes, I think IainS is on the right track, all things in objc_cls_refs escape
> and can be read and written to in unexpected ways by the runtime.  Setting
> TREE_ADDRESSABLE sounds like a reasonable step forward.

Why are they marked TREE_READONLY then?  That sounds like the bug.
Comment 33 Richard Biener 2010-02-16 10:14:57 UTC
(In reply to comment #32)
> (In reply to comment #24)
> > Yes, I think IainS is on the right track, all things in objc_cls_refs escape
> > and can be read and written to in unexpected ways by the runtime.  Setting
> > TREE_ADDRESSABLE sounds like a reasonable step forward.
> 
> Why are they marked TREE_READONLY then?  That sounds like the bug.

Err, forget that ;)  Still early today...
Comment 34 IainS 2010-02-16 14:58:39 UTC
Created attachment 19889 [details]
patch with CLASS and SELECTOR refs. marked as TREE_ADDRESSABLE & DECL_PRESERVE_P (for NeXT runtime)

In view of the comments in http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00597.html and http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00587.html it's clear that people feel that the bug is actually in ipa* and cgraphunit.c.  

However, a grep of lookup_attribute ipa* etc. suggests that altering these has possibly wide-reaching effects, which might not be welcome at this stage.

I've added back in the marking of CLASS_REFERENCES_* and SELECTOR_REFERENCES_* as TREE_ADDRESSABLE and DECL_PRESERVE_P.  Of course, this doesn't solve the problem unless the addition of the "used" attribute in finish_var_decl() is left in place too (although that can be removed once the underlying problem is resolved).

Would it be best to close this bug with the lookup_attribute fix - and open a new bug for ipa*.c and cgraphunit.c to support DECL_PRESERVE_P and/or TREE_USED? 

In fact, I'm a bit confused about what combination of flags is really needed.  TREE_USED appears to match in intent with "used" attribute - which is placed on the vars in objc-act.c/finish_var_decl().

DECL_PRESERVE_P presumably should only be used on the CLASS_REFERENCES/SELECTOR_REFERENCES - and thus I've placed it local to those two definitions.
Comment 35 Mike Stump 2010-02-16 16:25:21 UTC
Ok to the last patch for now.  Feel free to file a bug about checking DECL_PRESERVE_P and add a pointer to remove the setting of DECL_ATTRIBUTES from the frontend, thanks.
Comment 36 Mike Stump 2010-02-18 22:06:21 UTC
I've checked in a slightly updated fix in r156877.  Let us know if all the regressions are fixed now.
Comment 37 IainS 2010-02-19 08:43:52 UTC
(In reply to comment #36)
> I've checked in a slightly updated fix in r156877.  Let us know if all the
> regressions are fixed now.
i686/powerpc-darwin9 - YES, thanks.

Comment 38 mrs@gcc.gnu.org 2010-02-19 19:06:52 UTC
Subject: Bug 43061

Author: mrs
Date: Fri Feb 19 19:06:38 2010
New Revision: 156907

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156907
Log:
        PR objc/43061
        * cgraphunit.c (process_function_and_variable_attributes): Check
	DECL_PRESERVE_P instead of looking up attribute "used".
        * ipa-pure-const.c (check_decl): Likewise.
        * ipa-reference.c (has_proper_scope_for_analysis): Likewise.
        * ipa-type-escape.c (has_proper_scope_for_analysis): Likewise.
        * config/sol2.c (solaris_insert_attributes): Set DECL_PRESERVE_P
        instead of attribute "used".
        * config/sol2-c.c (solaris_pragma_init): Likewise.
        (solaris_pragma_fini): Likewise.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraphunit.c
    trunk/gcc/config/sol2-c.c
    trunk/gcc/config/sol2.c
    trunk/gcc/ipa-pure-const.c
    trunk/gcc/ipa-reference.c
    trunk/gcc/ipa-type-escape.c
    trunk/gcc/objc/objc-act.c

Comment 39 Mike Stump 2010-02-19 19:15:16 UTC
I checked in the real back end change in r156907.
Comment 40 IainS 2010-02-19 23:28:05 UTC
(In reply to comment #39)
> I checked in the real back end change in r156907.
OK on i686/powerpc d9

Comment 41 Dominique d'Humieres 2010-02-21 12:46:11 UTC
AFAICT this pr and the side effects have been fixed, hence closing. If someone disagree, please reopen.
Thanks all for the work.