GCC 4.2.0 Status Report (2007-03-22)
There are still a number of GCC 4.2.0 P1s, including the following which are new in GCC 4.2.0 (i.e., did not occur in GCC 4.1.x), together with -- as near as I can tell, based on Bugzilla -- the responsibility parties. PR 29585 (Novillo): ICE-on-valid PR 30700 (Sayle): Incorrect constant generation PR 31136 : Bitfield code-generation bug PR 31187 (Merrill): C++ declaration incorrectly considered to have internal linkage PR 31273 (Mitchell): C++ bitfield conversion problem Diego, Roger, Jason, would you please let me know if you can work on the issues above? I'm going to try to test Jim's patch for PR 31273 tonight. Joseph, would you please take a look at PR 31136? Andrew believes this to be a front-end bug. My hope is that we can fix these PRs, and then declare victory on the GCC 4.2.0 release. Thanks, -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: GCC 4.2.0 Status Report (2007-03-22)
Mark Mitchell wrote: > There are still a number of GCC 4.2.0 P1s, including the following which > are new in GCC 4.2.0 (i.e., did not occur in GCC 4.1.x), together with > -- as near as I can tell, based on Bugzilla -- the responsibility parties. > > PR 29585 (Novillo): ICE-on-valid > PR 30700 (Sayle): Incorrect constant generation Sorry, that's PR 30704. PR 30700 is also critical: PR 30700 (Guenther): Incorrect undefined reference Thanks, -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: GCC 4.2.0 Status Report (2007-03-22)
On Thu, 22 Mar 2007, Mark Mitchell wrote:
> Joseph, would you please take a look at PR 31136? Andrew believes this
> to be a front-end bug.
I don't think this is a front-end bug.
I applied the patch below to make the dumps give more meaningful
information than . The format of output is the same as
used by the C pretty-printer. (OK to commit this patch to mainline
subject to the usual testing?) With it, the .gimple dump is:
main ()
{
D.1530;
D.1531;
D.1532;
D.1533;
int D.1534;
short unsigned int D.1535;
short unsigned int D.1536;
s.b6 = 31;
D.1530 = s.b6;
D.1531 = () D.1530;
s.b4 = D.1531;
D.1532 = s.b4;
D.1533 = () D.1532;
s.b6 = D.1533;
D.1535 = BIT_FIELD_REF ;
D.1536 = D.1535 & 1008;
D.1534 = D.1536 != 240;
return D.1534;
}
As far as I can see, this has all the required conversions. The
conversions seem correct as of the .ccp dump, so I think the problem is in
the FRE pass as identified in the original bug report.
Specifically, I blame use of STRIP_NOPS or STRIP_SIGN_NOPS somewhere in
the optimizers, removing a conversion that preserves the mode when such
conversion is necessary to truncate to a narrower bit-field type. If I
make those macros require the precision to be unchanged then the testcase
passes. But such a change clearly needs more testing, and it should
probably be more conservative (conversions to a wider precision should be
OK to remove as well as those to the same precision, except when they
convert signed to unsigned). If this is the cause, the problem is
probably present in 4.1 as well, though whether it can be triggered
depends on how much the tree optimizers do with conversions to bit-field
types (which in normal code only arise through stores to bit-fields).
Note, I haven't tested at all for C++, and the bug is described as for
both C and C++.
Index: tree-pretty-print.c
===
--- tree-pretty-print.c (revision 123147)
+++ tree-pretty-print.c (working copy)
@@ -539,6 +539,14 @@
dump_generic_node (buffer, TREE_TYPE (node),
spc, flags, false);
}
+ else if (TREE_CODE (node) == INTEGER_TYPE)
+ {
+ pp_string (buffer, (TYPE_UNSIGNED (node)
+ ? "");
+ }
else
pp_string (buffer, "");
}
--
Joseph S. Myers
[EMAIL PROTECTED]
Re: GCC 4.2.0 Status Report (2007-03-22)
On 3/23/07, Mark Mitchell <[EMAIL PROTECTED]> wrote: Mark Mitchell wrote: > There are still a number of GCC 4.2.0 P1s, including the following which > are new in GCC 4.2.0 (i.e., did not occur in GCC 4.1.x), together with > -- as near as I can tell, based on Bugzilla -- the responsibility parties. > > PR 29585 (Novillo): ICE-on-valid > PR 30700 (Sayle): Incorrect constant generation Sorry, that's PR 30704. PR 30700 is also critical: PR 30700 (Guenther): Incorrect undefined reference Unfortunately Honza suggested my proposed fix is not the right one and I'm out of cgraph-fu here. Honza, can you have a look at that PR please? Thanks, Richard.
Re: GCC 4.2.0 Status Report (2007-03-22)
Joseph S. Myers wrote:
> On Thu, 22 Mar 2007, Mark Mitchell wrote:
>
>> Joseph, would you please take a look at PR 31136? Andrew believes this
>> to be a front-end bug.
>
> I don't think this is a front-end bug.
Thank you for investigating.
> (OK to commit this patch to mainline
> subject to the usual testing?)
Yes.
> Index: tree-pretty-print.c
> ===
> --- tree-pretty-print.c (revision 123147)
> +++ tree-pretty-print.c (working copy)
> @@ -539,6 +539,14 @@
> dump_generic_node (buffer, TREE_TYPE (node),
> spc, flags, false);
> }
> + else if (TREE_CODE (node) == INTEGER_TYPE)
> + {
> + pp_string (buffer, (TYPE_UNSIGNED (node)
> + ? " + : " + pp_decimal_int (buffer, TYPE_PRECISION (node));
> + pp_string (buffer, ">");
> + }
> else
>pp_string (buffer, "");
> }
--
Mark Mitchell
CodeSourcery
[EMAIL PROTECTED]
(650) 331-3385 x713
Re: GCC 4.2.0 Status Report (2007-03-22)
Mark Mitchell wrote on 03/22/07 22:10: > Diego, Roger, Jason, would you please let me know if you can work on the > issues above? I'm going to try to test Jim's patch for PR 31273 tonight. I'm looking at 29585 today.
Re: GCC 4.2.0 Status Report (2007-03-22)
Mark Mitchell wrote on 03/22/07 22:10: > PR 29585 (Novillo): ICE-on-valid This one seems to be a bug in the C++ FE, compounded by alias analysis papering over the issue. We are failing to mark DECLs in vtbl initializers as addressable. This causes the failure during aliasing because it is added to a points-to set but not marked for renaming. Since the variable does not have its address taken, we initially do not consider it interesting in the setup routines of alias analysis. However, the variable ends up inside points-to sets and later on we put it inside may-alias sets. This causes it to appear in virtual operands, but since it had not been marked for renaming, we fail. I traced the problem back to the building of vtables. I'm simply calling cxx_mark_addressable after building the ADDR_EXPR (I'm wondering if building ADDR_EXPR shouldn't just call langhooks.mark_addressable). Another way of addressing this would be to mark symbols addressable during referenced var discovery. But that is a bit hacky. Mark, does this look OK? (not tested yet) Index: cp/class.c === --- cp/class.c (revision 123332) +++ cp/class.c (working copy) @@ -7102,6 +7102,7 @@ /* Figure out the position to which the VPTR should point. */ vtbl = TREE_PURPOSE (l); vtbl = build1 (ADDR_EXPR, vtbl_ptr_type_node, vtbl); + cxx_mark_addressable (vtbl); index = size_binop (PLUS_EXPR, size_int (non_fn_entries), size_int (list_length (TREE_VALUE (l;
Re: GCC 4.2.0 Status Report (2007-03-22)
Diego Novillo wrote: I traced the problem back to the building of vtables. I'm simply calling cxx_mark_addressable after building the ADDR_EXPR (I'm wondering if building ADDR_EXPR shouldn't just call langhooks.mark_addressable). Looks fine to me. Many places in the front end use build_address rather than build1 (ADDR_EXPR) to avoid this issue. Jason
Re: GCC 4.2.0 Status Report (2007-03-22)
Jason Merrill wrote on 03/30/07 11:45:
> Looks fine to me. Many places in the front end use build_address rather
> than build1 (ADDR_EXPR) to avoid this issue.
Yeah, I found other cases in Java and in c-*.c. In one case, we are
building the address of a LABEL_DECL for a computed goto
(finish_label_address_expr).
Interestingly enough, mark_addressable refuses to mark the label as
addressable, but we need the label addressable so that it's processed
properly by the compute_may_aliases machinery.
Given that we need to be very consistent about addressability marking in
the FEs, wouldn't we be better off doing this in build1_stat()?
Index: tree.c
===
--- tree.c (revision 123332)
+++ tree.c (working copy)
@@ -2922,7 +2922,11 @@ build1_stat (enum tree_code code, tree t
case ADDR_EXPR:
if (node)
- recompute_tree_invariant_for_addr_expr (t);
+ {
+ recompute_tree_invariant_for_addr_expr (t);
+ if (DECL_P (node))
+ TREE_ADDRESSABLE (node) = 1;
+ }
break;
default:
Thanks.
Re: GCC 4.2.0 Status Report (2007-03-22)
Diego Novillo wrote: Interestingly enough, mark_addressable refuses to mark the label as addressable, but we need the label addressable so that it's processed properly by the compute_may_aliases machinery. Given that we need to be very consistent about addressability marking in the FEs, wouldn't we be better off doing this in build1_stat()? + if (DECL_P (node)) + TREE_ADDRESSABLE (node) = 1; I'd rather fix mark_addressable and use the langhook. Jason
Re: GCC 4.2.0 Status Report (2007-03-22)
Diego Novillo wrote: > This one seems to be a bug in the C++ FE, compounded by alias analysis > papering over the issue. Doh! Thank you for tracking this down. > Mark, does this look OK? (not tested yet) > > Index: cp/class.c > === > --- cp/class.c (revision 123332) > +++ cp/class.c (working copy) > @@ -7102,6 +7102,7 @@ >/* Figure out the position to which the VPTR should point. */ >vtbl = TREE_PURPOSE (l); >vtbl = build1 (ADDR_EXPR, vtbl_ptr_type_node, vtbl); > + cxx_mark_addressable (vtbl); >index = size_binop (PLUS_EXPR, > size_int (non_fn_entries), > size_int (list_length (TREE_VALUE (l; Echoing Jason, I think it would be best to use: vtbl = build_nop (vtbl_ptr_type_node, build_address (vtbl)); That will also make the tree type-correct. Right now, we're building an ADDR_EXPR with type "void (*)(void)", even though the function may be "int ()(S* this, double f)". So, by using build_address and build_nop, we'll get more correct code, plus get the addressability thing right. My only concern is that we seem to have gone to some lengths to avoid doing this. I think that the reason is that we create vtable initializers even for vtables we aren't going to emit in this translation unit. So, if you make the change above, you'll mark functions addressable which are never going to be emitted. I don't see anything wrong with that; I just wonder why we didn't do it. (You can see that mark_vtable_entries, which is called for emitted vtables, sets TREE_ADDRESSABLE by hand.) So, I think the right fix is (a) the change above, (b) remove the TREE_ADDRESSABLE setting from mark_vtable_entries (possibly replacing it with an assert.) If that works, it's OK. If not, and you want to punt it back, go ahead. Thanks again for working on this! -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: GCC 4.2.0 Status Report (2007-03-22)
Mark Mitchell wrote on 03/30/07 12:22:
> So, I think the right fix is (a) the change above, (b) remove the
> TREE_ADDRESSABLE setting from mark_vtable_entries (possibly replacing it
> with an assert.)
After removing the papering over TREE_ADDRESSABLE we were doing in the
aliaser, I found that other users of ADDR_EXPR are not consistently
setting the addressability bit.
This led me to this patch, which I'm now testing. This removes the
workaround we had in the aliaser and consistently marks every DECL
that's put in an ADDR_EXPR as addressable.
One thing that I'm wondering about this patch is why hasn't this been
done before? We seem to purposely separate TREE_ADDRESSABLE from
ADDR_EXPR. Perhaps to prevent pessimistic assumptions? The current
aliasing code removes addressability when it can prove otherwise.
This patch bootstraps all default languages. I'll test Ada later on,
but I need input from all the FE folks.
Thanks.
2007-03-30 Diego Novillo <[EMAIL PROTECTED]>
* tree.c (build1_stat): When building ADDR_EXPR of a DECL,
mark it addressable.
* tree-ssa-alias.c (add_may_alias): Assert that ALIAS may be
aliased.
* c-typeck.c (c_mark_addressable): Handle LABEL_DECL.
Index: tree.c
===
--- tree.c (revision 123332)
+++ tree.c (working copy)
@@ -2922,7 +2922,11 @@ build1_stat (enum tree_code code, tree t
case ADDR_EXPR:
if (node)
- recompute_tree_invariant_for_addr_expr (t);
+ {
+ recompute_tree_invariant_for_addr_expr (t);
+ if (DECL_P (node))
+ lang_hooks.mark_addressable (node);
+ }
break;
default:
Index: tree-ssa-alias.c
===
--- tree-ssa-alias.c (revision 123332)
+++ tree-ssa-alias.c (working copy)
@@ -2045,11 +2045,7 @@ add_may_alias (tree var, tree alias)
gcc_assert (var != alias);
/* ALIAS must be addressable if it's being added to an alias set. */
-#if 1
- TREE_ADDRESSABLE (alias) = 1;
-#else
gcc_assert (may_be_aliased (alias));
-#endif
if (v_ann->may_aliases == NULL)
v_ann->may_aliases = VEC_alloc (tree, gc, 2);
Index: c-typeck.c
===
--- c-typeck.c (revision 123332)
+++ c-typeck.c (working copy)
@@ -3247,6 +3247,7 @@ c_mark_addressable (tree exp)
/* drops in */
case FUNCTION_DECL:
+ case LABEL_DECL:
TREE_ADDRESSABLE (x) = 1;
/* drops out */
default:
Re: GCC 4.2.0 Status Report (2007-03-22)
Diego Novillo wrote on 03/30/07 13:21: > This patch bootstraps all default languages. I'll test Ada later on, > but I need input from all the FE folks. Sigh. I forgot to include Mark's suggestion in the patch. With this patch, calling build_address in dfs_accumulate_vtbl_inits is not strictly required (because we mark the DECL addressable in build1 now), but I will include it in the final version.
Re: GCC 4.2.0 Status Report (2007-03-22)
> One thing that I'm wondering about this patch is why hasn't this been > done before? We seem to purposely separate TREE_ADDRESSABLE from > ADDR_EXPR. Perhaps to prevent pessimistic assumptions? The current > aliasing code removes addressability when it can prove otherwise. One concern I have in marking a DECL addressable that early on is that it may stay "stuck" even if the ADDR_EXPR is later eliminated. This can be common in inlined situations, I thought. We *do* have to make up our mind, of course, on a precise time when it's set and be very clear about whether we can reset it (and how) if we discover later that the address actually isn't being taken.
Re: GCC 4.2.0 Status Report (2007-03-22)
Richard Kenner wrote on 03/30/07 13:45: > One concern I have in marking a DECL addressable that early on is that > it may stay "stuck" even if the ADDR_EXPR is later eliminated. This can > be common in inlined situations, I thought. The aliaser is fairly aggressive at removing TREE_ADDRESSABLE from variables that do not need it anymore, so that should not be a problem. > We *do* have to make up our mind, of course, on a precise time when it's > set and be very clear about whether we can reset it (and how) if we > discover later that the address actually isn't being taken. Agreed.
Re: GCC 4.2.0 Status Report (2007-03-22)
> The aliaser is fairly aggressive at removing TREE_ADDRESSABLE from > variables that do not need it anymore, so that should not be a problem. Yes, but you're calling the lang hook, which in theory, is allowed to do all sorts of different things. How do those get undone when we find *they* aren't needed? Moreover, what if what you're taking the address of is a COMPONENT_REF or some such. Are we saying that's not allowed anymore? Some lang-hooks look through them to set TREE_ADDRESSABLE on the underlying decl, but your patch only calls it on a DECL_P. I think we need to step back a bit and get a definition we can all agree on here first.
Re: GCC 4.2.0 Status Report (2007-03-22)
On 3/30/07, Richard Kenner <[EMAIL PROTECTED]> wrote: > The aliaser is fairly aggressive at removing TREE_ADDRESSABLE from > variables that do not need it anymore, so that should not be a problem. Yes, but you're calling the lang hook, which in theory, is allowed to do all sorts of different things. How do those get undone when we find *they* aren't needed? The lang hook is supposed to mark the variable as addressable. The lang hook should not be changing other things that have an affect on the *middle end*. No exceptions. Moreover, what if what you're taking the address of is a COMPONENT_REF or some such. Are we saying that's not allowed anymore? Some lang-hooks look through them to set TREE_ADDRESSABLE on the underlying decl, but your patch only calls it on a DECL_P. What are you talking about? The aliaser will remove tree addressable from DECL's if they do not have their address taken. This includes removing addressable from "a" if we had &a.b.c.d, it was the only address taking of a, and later eliminated it. I think we need to step back a bit and get a definition we can all agree on here first. A definition of what, exactly?
Re: GCC 4.2.0 Status Report (2007-03-22)
> The lang hook is supposed to mark the variable as addressable. > The lang hook should not be changing other things that have an affect > on the *middle end*. No exceptions. But how is it "supposed to mark the variable as addressable"? If this just means setting TREE_ADDRESSABLE, what's the point of having the hook? And if it does something *else*, how is the middle-end supposed to know how to undo it? I guess what I'm arguing for here is either the removal of the lang hook or the addition of one to set a decl *non-addressable*. As far as I'm concerned, I think the removal is better, but both are OK with me. > > Moreover, what if what you're taking the address of is a COMPONENT_REF > > or some such. Are we saying that's not allowed anymore? Some lang-hooks > > look through them to set TREE_ADDRESSABLE on the underlying decl, but > > your patch only calls it on a DECL_P. > What are you talking about? > The aliaser will remove tree addressable from DECL's if they do not > have their address taken. This includes removing addressable from "a" > if we had &a.b.c.d, it was the only address taking of a, and later > eliminated it. I'm talking about *setting* TREE_ADDRESSABLE on "a" in your example. Exactly who will do that with your patch? > > I think we need to step back a bit and get a definition we can all agree > > on here first. > > A definition of what, exactly? Of the precise relationship between TREE_ADDRESSABLE and the lang hook and the issue of how to "undo" what the hook might have done.
Re: GCC 4.2.0 Status Report (2007-03-22)
On 3/30/07, Richard Kenner <[EMAIL PROTECTED]> wrote: > The lang hook is supposed to mark the variable as addressable. > The lang hook should not be changing other things that have an affect > on the *middle end*. No exceptions. But how is it "supposed to mark the variable as addressable"? If this just means setting TREE_ADDRESSABLE, what's the point of having the hook? It also issues language specific warnings And if it does something *else*, how is the middle-end supposed to know how to undo it? It's not supposed to be doing other things for languages that don't want to emit warnings (or do other front-endy things with the answer) I guess what I'm arguing for here is either the removal of the lang hook or the addition of one to set a decl *non-addressable*. As far as I'm concerned, I think the removal is better, but both are OK with me. You haven't explained what you think needs undoing in *any current language that uses the hook*. We shouldn't be adding hooks just because some theoretical language that doesn't exist might want to do things we don't want it to do *anyway*. No wonder we have so many langhooks.
Re: GCC 4.2.0 Status Report (2007-03-22)
> > But how is it "supposed to mark the variable as addressable"? If this > > just means setting TREE_ADDRESSABLE, what's the point of having the hook? > > It also issues language specific warnings Then one suggestion is that we rename the langhook to "warn_addressable" and set TREE_ADDRESSABLE explicitly in the middle-end. But another is to move the entire processing into the front end and get rid of the langhook entirely! It's the front end that's emitting the ADDR_EXPR in the first place, so it certainly knows it's taking the address of something! > It's not supposed to be doing other things for languages that don't > want to emit warnings (or do other front-endy things with the answer) And where is this documented? > You haven't explained what you think needs undoing in *any current > language that uses the hook*. > We shouldn't be adding hooks just because some theoretical language > that doesn't exist might want to do things we don't want it to do > *anyway*. > No wonder we have so many langhooks. I was suggesting *removing* a langhook. But the fact that "no current frontend does something" is not a reason why "something" isn't valid! To the greatest extent possible, we are trying to form a documented interace between the front end and the middle-end. To do that, we need to know precisely what a front-end can and can't do and not rely on what some particular set of them do now. Specifically, I suggest that we remove the present mark_addressable langhook. Have the middle-end set TREE_ADDRESSABLE. If necessary (and I don't think it is: see above), call a new langhook that *notifies* the front end about the addressability for the purpose of a warning, but the front end isn't allowed to change anything else or to rely on or assume that the address will still be taken when final code is general. I don't think the alternative of making a new langhook to unmark addressable is the best because then you'd actually have to make the marking "private" and have yet another lang hook to see if something is marked!
Re: GCC 4.2.0 Status Report (2007-03-22)
> "Diego" == Diego Novillo <[EMAIL PROTECTED]> writes: Diego> This patch bootstraps all default languages. I'll test Ada later on, Diego> but I need input from all the FE folks. I don't think this should hurt gcj. I don't think we test TREE_ADDRESSABLE for anything important. Rebuilding libgcj & running the test suite is probably the best way to be sure. Tom
