Re: ICE in change_address at emit_rtl.c

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    This is one reason why fold causes problems as it sometimes
    creates SAVE_EXPR.  Using  a scoped temporary:
       let tmp := expr in BODY
    would be preferable for us.

But is it realistic for fold to make one?  fold has no idea where the
expression being folded is going.

    Another problem with fold is that it does not obey Java rules for
    expression evaluation order.  We would probably want to add a flag to
    force it to do that.

This is very much a separate issue, of course.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    This is one reason why fold causes problems as it sometimes
>    creates SAVE_EXPR.  Using  a scoped temporary:
>       let tmp := expr in BODY
>    would be preferable for us.
>
>But is it realistic for fold to make one?  fold has no idea where the
>expression being folded is going.
>
I'm not sure what the problem is.  My proposal is to use some kind of
LET_EXPR, which takes a declaration and two expressions.  It seems
plausible to me that any place that creates a SAVE_EXPR could use a
LET_EXPR instead. For example:
  arr[index] += expr
Here of course arr and index should only be evaluated once.  So this
would be
  LET t1 :=arr IN LET t2 := index IN t1[t2] = t1[t2] + expr
Similarly, where fold creates a SAVE_EXPR because some sub-expression
may be evaluated more than once, you can wrap the expression (the argument
to fold) in a LET_EXPR, or whatever makes sense.  I.e. you can control
the lifetime of the temporary as it is needed.
    --Per Bothner

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    This is one reason why fold causes problems as it sometimes
>    creates SAVE_EXPR.  Using  a scoped temporary:
>       let tmp := expr in BODY
>    would be preferable for us.
>
>But is it realistic for fold to make one?  fold has no idea where the
>expression being folded is going.
>
I'm not sure what the problem is.  My proposal is to use some kind of
LET_EXPR, which takes a declaration and two expressions.  It seems
plausible to me that any place that creates a SAVE_EXPR could use a
LET_EXPR instead. For example:
  arr[index] += expr
Here of course arr and index should only be evaluated once.  So this
would be
  LET t1 :=arr IN LET t2 := index IN t1[t2] = t1[t2] + expr
Similarly, where fold creates a SAVE_EXPR because some sub-expression
may be evaluated more than once, you can wrap the expression (the argument
to fold) in a LET_EXPR, or whatever makes sense.  I.e. you can control
the lifetime of the temporary as it is needed.
    --Per Bothner

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    This is one reason why fold causes problems as it sometimes
    creates SAVE_EXPR.  Using  a scoped temporary:
       let tmp := expr in BODY
    would be preferable for us.

But is it realistic for fold to make one?  fold has no idea where the
expression being folded is going.

    Another problem with fold is that it does not obey Java rules for
    expression evaluation order.  We would probably want to add a flag to
    force it to do that.

This is very much a separate issue, of course.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sun, 25 Nov 2001 16:12:36 -0800
> From: Per Bothner <per@bothner.com>

> Using a LET_EXPR makes the scope explicit.  It is clear that the
> scope of the tmp lasts during the evaluation of the let-body, which
> is during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the
> scope of the temporary is "from the first time the SAVE_EXPR is
> evaluated until the last time the SAVE_EXPR is evaluated".  We
> cannot know what this is without analyzing the entire body of the
> function.

This wrong.  The scope for the storage ends, like it or not, at the
end of the block in which it was expanded in, which is defined by a
call to push_temp_slots_for_block.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

mike stump wrote:

>>Using a LET_EXPR makes the scope explicit.  It is clear that the
>>scope of the tmp lasts during the evaluation of the let-body, which
>>is during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the
>>scope of the temporary is "from the first time the SAVE_EXPR is
>>evaluated until the last time the SAVE_EXPR is evaluated".  We
>>cannot know what this is without analyzing the entire body of the
>>function.
>>
>This wrong.  The scope for the storage ends, like it or not, at the
>end of the block in which it was expanded in, which is defined by a
>call to push_temp_slots_for_block.
>
You are talking implementation details; I am talking semantics.
    --Per

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

mike stump wrote:

>>Using a LET_EXPR makes the scope explicit.  It is clear that the
>>scope of the tmp lasts during the evaluation of the let-body, which
>>is during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the
>>scope of the temporary is "from the first time the SAVE_EXPR is
>>evaluated until the last time the SAVE_EXPR is evaluated".  We
>>cannot know what this is without analyzing the entire body of the
>>function.
>>
>This wrong.  The scope for the storage ends, like it or not, at the
>end of the block in which it was expanded in, which is defined by a
>call to push_temp_slots_for_block.
>
You are talking implementation details; I am talking semantics.
    --Per

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sun, 25 Nov 2001 16:12:36 -0800
> From: Per Bothner <per@bothner.com>

> Using a LET_EXPR makes the scope explicit.  It is clear that the
> scope of the tmp lasts during the evaluation of the let-body, which
> is during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the
> scope of the temporary is "from the first time the SAVE_EXPR is
> evaluated until the last time the SAVE_EXPR is evaluated".  We
> cannot know what this is without analyzing the entire body of the
> function.

This wrong.  The scope for the storage ends, like it or not, at the
end of the block in which it was expanded in, which is defined by a
call to push_temp_slots_for_block.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    If you increase the likely hood that the compiler will just work in
    the face of changing code, and random maintenance and decrease the
    maintenance code, and make the code easier to read and understand,
    then you gained more, than just renaming it, you've improved it.

Of course, but I'm not convinced this will happen.

    It is reasonable to find a way to `improve' SAVE_EXPR.  Check out
    UNSAVE_EXPR, and the `grossness' of it.  It is truely worth
    eliminating by design.  Unfortunately to get rid of it, you need to
    revamp SAVE_EXPR.

But those things are there because there's a semantic need for that
processing.  If you eliminate SAVE_EXPR, you have to replace it with
something that's similarly 'gross'.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    If you increase the likely hood that the compiler will just work in
    the face of changing code, and random maintenance and decrease the
    maintenance code, and make the code easier to read and understand,
    then you gained more, than just renaming it, you've improved it.

Of course, but I'm not convinced this will happen.

    It is reasonable to find a way to `improve' SAVE_EXPR.  Check out
    UNSAVE_EXPR, and the `grossness' of it.  It is truely worth
    eliminating by design.  Unfortunately to get rid of it, you need to
    revamp SAVE_EXPR.

But those things are there because there's a semantic need for that
processing.  If you eliminate SAVE_EXPR, you have to replace it with
something that's similarly 'gross'.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sun, 25 Nov 01 09:23:28 EST
> From: kenner@vlsi1.ultra.nyu.edu (Richard Kenner)
> To: neil@daikokuya.demon.co.uk
> Cc: gcc@gcc.gnu.org

>     And why can't you look inside a temporary the same way you look inside
>     a SAVE_EXPR?

> Right now we have no mechanism for doing so.

> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?  If you do everything
> that SAVE_EXPR can do, all you've done is renamed it!

If you increase the likely hood that the compiler will just work in
the face of changing code, and random maintenance and decrease the
maintenance code, and make the code easier to read and understand,
then you gained more, than just renaming it, you've improved it.

It is reasonable to find a way to `improve' SAVE_EXPR.  Check out
UNSAVE_EXPR, and the `grossness' of it.  It is truely worth
eliminating by design.  Unfortunately to get rid of it, you need to
revamp SAVE_EXPR.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sun, 25 Nov 01 09:23:28 EST
> From: kenner@vlsi1.ultra.nyu.edu (Richard Kenner)
> To: neil@daikokuya.demon.co.uk
> Cc: gcc@gcc.gnu.org

>     And why can't you look inside a temporary the same way you look inside
>     a SAVE_EXPR?

> Right now we have no mechanism for doing so.

> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?  If you do everything
> that SAVE_EXPR can do, all you've done is renamed it!

If you increase the likely hood that the compiler will just work in
the face of changing code, and random maintenance and decrease the
maintenance code, and make the code easier to read and understand,
then you gained more, than just renaming it, you've improved it.

It is reasonable to find a way to `improve' SAVE_EXPR.  Check out
UNSAVE_EXPR, and the `grossness' of it.  It is truely worth
eliminating by design.  Unfortunately to get rid of it, you need to
revamp SAVE_EXPR.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sat, 24 Nov 2001 23:05:30 -0800
> From: Zack Weinberg <zack@codesourcery.com>
> To: mike stump <mrs@windriver.com>

> int main(argc, argv)
>    int argc;
>    string argv[];
> {
>    int i;
>    for(i=1; i<argc; i++)
>     puts(argv[i]);
>    return 0;
> }

> $ gcc -S test.c
> test.c: In function `main':
> test.c:3: parse error before `string'
> test.c:7: subscripted value is neither array nor pointer

> Here, we have no clue what the type of an element of argv is, but we
> ought to be able to call it "array of <unknown type>" instead of
> just "<unknown type>".

unknown type is fine, this is a trivial little bug that should be able
to be fixed with 2 lines of change.  It might be marginally better
with EMN[], but, the cost is once EMN is inside of `things' (such as
[]), the code that has to check stuff as you rip apart stuff
increases.  And without the checks in place before you begin, you
increase the faulting before you improve the diagnostics, which, isn't
an improvement.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sat, 24 Nov 2001 23:05:30 -0800
> From: Zack Weinberg <zack@codesourcery.com>
> To: mike stump <mrs@windriver.com>

> int main(argc, argv)
>    int argc;
>    string argv[];
> {
>    int i;
>    for(i=1; i<argc; i++)
>     puts(argv[i]);
>    return 0;
> }

> $ gcc -S test.c
> test.c: In function `main':
> test.c:3: parse error before `string'
> test.c:7: subscripted value is neither array nor pointer

> Here, we have no clue what the type of an element of argv is, but we
> ought to be able to call it "array of <unknown type>" instead of
> just "<unknown type>".

unknown type is fine, this is a trivial little bug that should be able
to be fixed with 2 lines of change.  It might be marginally better
with EMN[], but, the cost is once EMN is inside of `things' (such as
[]), the code that has to check stuff as you rip apart stuff
increases.  And without the checks in place before you begin, you
increase the faulting before you improve the diagnostics, which, isn't
an improvement.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Being able to optimize at the tree level.  I disbelieve we'll
    be able to do that properly given the less than precise
    semantics of SAVE_EXPR.

I don't follow.  For most purposes of tree-level optimization they are
nops, except that they aren't invalidated by stores to things that are in
the expression in their operand.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Being able to optimize at the tree level.  I disbelieve we'll
    be able to do that properly given the less than precise
    semantics of SAVE_EXPR.

I don't follow.  For most purposes of tree-level optimization they are
nops, except that they aren't invalidated by stores to things that are in
the expression in their operand.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Perhaps to you.  But not for C, nor to my knowledge C++
    or Java.  (I weasel a bit about Java, because it is possible
    to model Java arrays as variable-sized types.  I don't think
    we do that, though.)

I don't know about Java, but you certainly can have variable-sized objects
in C.  That's what the variable_size routine is for.  It is the major
creator of SAVE_EXPRs.  The few in fold are, by far, the least common.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Perhaps to you.  But not for C, nor to my knowledge C++
    or Java.  (I weasel a bit about Java, because it is possible
    to model Java arrays as variable-sized types.  I don't think
    we do that, though.)

I don't know about Java, but you certainly can have variable-sized objects
in C.  That's what the variable_size routine is for.  It is the major
creator of SAVE_EXPRs.  The few in fold are, by far, the least common.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Using a LET_EXPR makes the scope explicit.  It is clear that the scope
    of the tmp lasts during the evaluation of the let-body, which is
    during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the scope
    of the temporary is "from the first time the SAVE_EXPR is evaluated
    until the last time the SAVE_EXPR is evaluated".  We cannot know what
    this is without analyzing the entire body of the function.

OK, I see what you mean.  But this is very much the exceptional case of
SAVE_EXPR.  The most common case is when you are putting it in the size
of offset inside a type.  You have the same scoping issues no matter what you
call it.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>OK, I see what you mean.  But this is very much the exceptional case of
>SAVE_EXPR.  The most common case is when you are putting it in the size
>of offset inside a type.
>
Perhaps to you.  But not for C, nor to my knowledge C++
or Java.  (I weasel a bit about Java, because it is possible
to model Java arrays as variable-sized types.  I don't think
we do that, though.)
    --Per Bothner

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>OK, I see what you mean.  But this is very much the exceptional case of
>SAVE_EXPR.  The most common case is when you are putting it in the size
>of offset inside a type.
>
Perhaps to you.  But not for C, nor to my knowledge C++
or Java.  (I weasel a bit about Java, because it is possible
to model Java arrays as variable-sized types.  I don't think
we do that, though.)
    --Per Bothner

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Using a LET_EXPR makes the scope explicit.  It is clear that the scope
    of the tmp lasts during the evaluation of the let-body, which is
    during the evaluation of the PLUS_EXPR.  Using a SAVE_EXPR, the scope
    of the temporary is "from the first time the SAVE_EXPR is evaluated
    until the last time the SAVE_EXPR is evaluated".  We cannot know what
    this is without analyzing the entire body of the function.

OK, I see what you mean.  But this is very much the exceptional case of
SAVE_EXPR.  The most common case is when you are putting it in the size
of offset inside a type.  You have the same scoping issues no matter what you
call it.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    The big difference is that the temporary used by LET_EXPR (actually
    LET_EXPR can be used to encode user-level declarations too) has a
    well-defined scope.  It is clear when the temporary register needs to
    be allocated (on entrance to the BODY expression) to be allocated, and
    it is clear when it can be freed (on exit from the BODY expression).
    Neither are true for SAVE_EXPR, which is much less well defined.

I disagree.  The scopes of both need to be exactly the same: the scope of
the underlying expression.  It's not possible for fold to know that so it
would have to do the same thing with the new LET_EXPR as for the present
SAVE_EXPR.  I don't see how this changes things at all.

In both cases we are allocating a temporary and we are guessing as to its
scope.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    The big difference is that the temporary used by LET_EXPR (actually
>    LET_EXPR can be used to encode user-level declarations too) has a
>    well-defined scope.  It is clear when the temporary register needs to
>    be allocated (on entrance to the BODY expression) to be allocated, and
>    it is clear when it can be freed (on exit from the BODY expression).
>    Neither are true for SAVE_EXPR, which is much less well defined.
>
>I disagree.  The scopes of both need to be exactly the same: the scope of
>the underlying expression.  It's not possible for fold to know that so it
>would have to do the same thing with the new LET_EXPR as for the present
>SAVE_EXPR.  I don't see how this changes things at all.
>
Consider the code in fold that converts 2*x to x+x:
      /* x*2 is x+x */
      if (! wins && real_twop (arg1) && global_bindings_p () == 0
          && ! contains_placeholder_p (arg0))
        {
          tree arg = save_expr (arg0);
          return build (PLUS_EXPR, type, arg, arg);
        }

Under my proposal this might be:
      /* x*2 is x+x */
      if (! wins && real_twop (arg1) && global_bindings_p () == 0
          && ! contains_placeholder_p (arg0))        {
        {
          tree tmp = make_temp_decl (type);
          return build (LET_EXPR, type, tmp, arg0,
            build (PLUS_EXPR, type, tmp, tmp));
        }

Using a LET_EXPR makes the scope explicit.  It is clear that
the scope of the tmp lasts during the evaluation of the let-body,
which is during the evaluation of the PLUS_EXPR.  Using a
SAVE_EXPR, the scope of the temporary is "from the first
time the SAVE_EXPR is evaluated until the last time the
SAVE_EXPR is evaluated".  We cannot know what this is without
analyzing the entire body of the function.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    The big difference is that the temporary used by LET_EXPR (actually
>    LET_EXPR can be used to encode user-level declarations too) has a
>    well-defined scope.  It is clear when the temporary register needs to
>    be allocated (on entrance to the BODY expression) to be allocated, and
>    it is clear when it can be freed (on exit from the BODY expression).
>    Neither are true for SAVE_EXPR, which is much less well defined.
>
>I disagree.  The scopes of both need to be exactly the same: the scope of
>the underlying expression.  It's not possible for fold to know that so it
>would have to do the same thing with the new LET_EXPR as for the present
>SAVE_EXPR.  I don't see how this changes things at all.
>
Consider the code in fold that converts 2*x to x+x:
      /* x*2 is x+x */
      if (! wins && real_twop (arg1) && global_bindings_p () == 0
          && ! contains_placeholder_p (arg0))
        {
          tree arg = save_expr (arg0);
          return build (PLUS_EXPR, type, arg, arg);
        }

Under my proposal this might be:
      /* x*2 is x+x */
      if (! wins && real_twop (arg1) && global_bindings_p () == 0
          && ! contains_placeholder_p (arg0))        {
        {
          tree tmp = make_temp_decl (type);
          return build (LET_EXPR, type, tmp, arg0,
            build (PLUS_EXPR, type, tmp, tmp));
        }

Using a LET_EXPR makes the scope explicit.  It is clear that
the scope of the tmp lasts during the evaluation of the let-body,
which is during the evaluation of the PLUS_EXPR.  Using a
SAVE_EXPR, the scope of the temporary is "from the first
time the SAVE_EXPR is evaluated until the last time the
SAVE_EXPR is evaluated".  We cannot know what this is without
analyzing the entire body of the function.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    The big difference is that the temporary used by LET_EXPR (actually
    LET_EXPR can be used to encode user-level declarations too) has a
    well-defined scope.  It is clear when the temporary register needs to
    be allocated (on entrance to the BODY expression) to be allocated, and
    it is clear when it can be freed (on exit from the BODY expression).
    Neither are true for SAVE_EXPR, which is much less well defined.

I disagree.  The scopes of both need to be exactly the same: the scope of
the underlying expression.  It's not possible for fold to know that so it
would have to do the same thing with the new LET_EXPR as for the present
SAVE_EXPR.  I don't see how this changes things at all.

In both cases we are allocating a temporary and we are guessing as to its
scope.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    I'm not sure what the problem is.  My proposal is to use some kind of
    LET_EXPR, which takes a declaration and two expressions.  It seems
    plausible to me that any place that creates a SAVE_EXPR could use a
    LET_EXPR instead. 

So how are you changing things?  You're replacing SAVE_EXPR with LET_EXPR
with similar semantics.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    I'm not sure what the problem is.  My proposal is to use some kind of
>    LET_EXPR, which takes a declaration and two expressions.  It seems
>    plausible to me that any place that creates a SAVE_EXPR could use a
>    LET_EXPR instead. 
>
>So how are you changing things?  You're replacing SAVE_EXPR with LET_EXPR
>with similar semantics.
>
The big difference is that the temporary used by LET_EXPR (actually LET_EXPR
can be used to encode user-level declarations too) has a well-defined scope.
It is clear when the temporary register needs  to be allocated (on 
entrance to the
BODY expression) to be allocated, and it is clear when it can be freed 
(on exit
from the BODY expression).  Neither are true for SAVE_EXPR, which is much
less well defined.

    --Per

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

Richard Kenner wrote:

>    I'm not sure what the problem is.  My proposal is to use some kind of
>    LET_EXPR, which takes a declaration and two expressions.  It seems
>    plausible to me that any place that creates a SAVE_EXPR could use a
>    LET_EXPR instead. 
>
>So how are you changing things?  You're replacing SAVE_EXPR with LET_EXPR
>with similar semantics.
>
The big difference is that the temporary used by LET_EXPR (actually LET_EXPR
can be used to encode user-level declarations too) has a well-defined scope.
It is clear when the temporary register needs  to be allocated (on 
entrance to the
BODY expression) to be allocated, and it is clear when it can be freed 
(on exit
from the BODY expression).  Neither are true for SAVE_EXPR, which is much
less well defined.

    --Per

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    I'm not sure what the problem is.  My proposal is to use some kind of
    LET_EXPR, which takes a declaration and two expressions.  It seems
    plausible to me that any place that creates a SAVE_EXPR could use a
    LET_EXPR instead. 

So how are you changing things?  You're replacing SAVE_EXPR with LET_EXPR
with similar semantics.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    No, I think you'd remove a lot of the special-casing that SAVE_EXPR
    causes (and weirdness).  Also, the tree->RTL layer wouldn't have to
    special case it either.

Try it and I think you'll find that all the complexity is semantically
required: if you don't put it in one way, it'll have to go in some
other way.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    No, I think you'd remove a lot of the special-casing that SAVE_EXPR
    causes (and weirdness).  Also, the tree->RTL layer wouldn't have to
    special case it either.

Try it and I think you'll find that all the complexity is semantically
required: if you don't put it in one way, it'll have to go in some
other way.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    And why can't you look inside a temporary the same way you look inside
    a SAVE_EXPR?

Right now we have no mechanism for doing so.

However, my question is what do we gain by removing SAVE_EXPR if we add
temporaries that have exactly the same semantics?  If you do everything
that SAVE_EXPR can do, all you've done is renamed it!

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?  If you do everything
> that SAVE_EXPR can do, all you've done is renamed it!

No, I think you'd remove a lot of the special-casing that SAVE_EXPR
causes (and weirdness).  Also, the tree->RTL layer wouldn't have to
special case it either.

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?  If you do everything
> that SAVE_EXPR can do, all you've done is renamed it!

No, I think you'd remove a lot of the special-casing that SAVE_EXPR
causes (and weirdness).  Also, the tree->RTL layer wouldn't have to
special case it either.

Neil.

Re: ICE in change_address at emit_rtl.c

[Richard Henderson]

On Sun, Nov 25, 2001 at 09:23:28AM -0500, Richard Kenner wrote:
> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?

Being able to optimize at the tree level.  I disbelieve we'll
be able to do that properly given the less than precise
semantics of SAVE_EXPR.

The *only* thing that is good for is holding a temporary during
conversion of *one* expression into rtl.  In any other context
we have to play distinctly ugly games.


r~

Re: ICE in change_address at emit_rtl.c

[Richard Henderson]

On Sun, Nov 25, 2001 at 09:23:28AM -0500, Richard Kenner wrote:
> However, my question is what do we gain by removing SAVE_EXPR if we add
> temporaries that have exactly the same semantics?

Being able to optimize at the tree level.  I disbelieve we'll
be able to do that properly given the less than precise
semantics of SAVE_EXPR.

The *only* thing that is good for is holding a temporary during
conversion of *one* expression into rtl.  In any other context
we have to play distinctly ugly games.


r~

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    And why can't you look inside a temporary the same way you look inside
    a SAVE_EXPR?

Right now we have no mechanism for doing so.

However, my question is what do we gain by removing SAVE_EXPR if we add
temporaries that have exactly the same semantics?  If you do everything
that SAVE_EXPR can do, all you've done is renamed it!

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    > Right, but note I said *performance*.  Even if we do that, we'd lose
    > optimization potential in knowing the form of the values.

    Why?  Can you give a simple example?

No.  I said *complex* objects.

Basically if you can look inside a SAVE_EXPR you may be able to deduce that
the value is always a multiple of some power-of-two.  Or you may be
able to simplify the expression by merging some divides and multiplies.
It's this sort of thing I'm talking about.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> Basically if you can look inside a SAVE_EXPR you may be able to deduce that
> the value is always a multiple of some power-of-two.  Or you may be
> able to simplify the expression by merging some divides and multiplies.
> It's this sort of thing I'm talking about.

And why can't you look inside a temporary the same way you look inside
a SAVE_EXPR?

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> Basically if you can look inside a SAVE_EXPR you may be able to deduce that
> the value is always a multiple of some power-of-two.  Or you may be
> able to simplify the expression by merging some divides and multiplies.
> It's this sort of thing I'm talking about.

And why can't you look inside a temporary the same way you look inside
a SAVE_EXPR?

Neil.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    > Right, but note I said *performance*.  Even if we do that, we'd lose
    > optimization potential in knowing the form of the values.

    Why?  Can you give a simple example?

No.  I said *complex* objects.

Basically if you can look inside a SAVE_EXPR you may be able to deduce that
the value is always a multiple of some power-of-two.  Or you may be
able to simplify the expression by merging some divides and multiplies.
It's this sort of thing I'm talking about.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Can't we create temporaries, and then refer to those?  I know there is
    no framework in place for doing this at present.

Right, but note I said *performance*.  Even if we do that, we'd lose
optimization potential in knowing the form of the values.  It's quite common
for there to be multiple nestings of SAVE_EXPRs in position and size
computations in complex variable-sized objects.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

>     Can't we create temporaries, and then refer to those?  I know there is
>     no framework in place for doing this at present.
> 
> Right, but note I said *performance*.  Even if we do that, we'd lose
> optimization potential in knowing the form of the values.

Why?  Can you give a simple example?

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

>     Can't we create temporaries, and then refer to those?  I know there is
>     no framework in place for doing this at present.
> 
> Right, but note I said *performance*.  Even if we do that, we'd lose
> optimization potential in knowing the form of the values.

Why?  Can you give a simple example?

Neil.

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

SAVE_EXPR causes problems when generating Java bytecodes.  We do
this by using the Java parser to build a tree, but then bypassing
the  backend to emit .class files directly.  The latter does not have
a register allocation pass.  That is ok, because we cn use as many
"registers" as we need, and all local variables can be assigned to
"registers" using a trivial stack discipline.  However, SAVE_EXPRs
cause problems.  It is easy enough to assign a jvm register to a
SAVE_EXPR, but there is no way to reclaim it.  This is not a
fatal flaw, but it is wasteful, as it would bloat the stack frame
of a naive interpreter.

This is one reason why fold causes problems as it sometimes
creates SAVE_EXPR.  Using  a scoped temporary:
   let tmp := expr in BODY
would be preferable for us.

Another problem with fold is that it does not obey Java rules
for expression evaluation order.  We would probably want to add
a flag to force it to do that.

    --Per

Re: ICE in change_address at emit_rtl.c

[Per Bothner]

SAVE_EXPR causes problems when generating Java bytecodes.  We do
this by using the Java parser to build a tree, but then bypassing
the  backend to emit .class files directly.  The latter does not have
a register allocation pass.  That is ok, because we cn use as many
"registers" as we need, and all local variables can be assigned to
"registers" using a trivial stack discipline.  However, SAVE_EXPRs
cause problems.  It is easy enough to assign a jvm register to a
SAVE_EXPR, but there is no way to reclaim it.  This is not a
fatal flaw, but it is wasteful, as it would bloat the stack frame
of a naive interpreter.

This is one reason why fold causes problems as it sometimes
creates SAVE_EXPR.  Using  a scoped temporary:
   let tmp := expr in BODY
would be preferable for us.

Another problem with fold is that it does not obey Java rules
for expression evaluation order.  We would probably want to add
a flag to force it to do that.

    --Per

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    Can't we create temporaries, and then refer to those?  I know there is
    no framework in place for doing this at present.

Right, but note I said *performance*.  Even if we do that, we'd lose
optimization potential in knowing the form of the values.  It's quite common
for there to be multiple nestings of SAVE_EXPRs in position and size
computations in complex variable-sized objects.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    I'd also like to see us go down the road that rth mentioned,
    i.e. losing all trees with side effects

You mean like MODIFY_EXPR and CALL_EXPR?  :-)

    That would get rid of {UN,}SAVE_EXPR 

I don't see how to get rid of SAVE_EXPR without taking some preformance hits
on complex types.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> I don't see how to get rid of SAVE_EXPR without taking some preformance hits
> on complex types.

Can't we create temporaries, and then refer to those?  I know there is
no framework in place for doing this at present.

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Richard Kenner wrote:-

> I don't see how to get rid of SAVE_EXPR without taking some preformance hits
> on complex types.

Can't we create temporaries, and then refer to those?  I know there is
no framework in place for doing this at present.

Neil.

Re: ICE in change_address at emit_rtl.c

[Richard Kenner]

    I'd also like to see us go down the road that rth mentioned,
    i.e. losing all trees with side effects

You mean like MODIFY_EXPR and CALL_EXPR?  :-)

    That would get rid of {UN,}SAVE_EXPR 

I don't see how to get rid of SAVE_EXPR without taking some preformance hits
on complex types.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sat, 24 Nov 2001 16:41:55 -0800
> From: Zack Weinberg <zack@codesourcery.com>
> To: Gabriel Dos Reis <gdr@codesourcery.com>

> 2. As few spurious errors reported to the user as possible.  This is
> one place where ERROR_MARK_NODE really falls down.  Consider these
> ill-formed declarations:

> double int number;
> typedef struct foo foo_t[;

> These will both provoke error cascades in the current compiler,

?

double int number;

int maif() {
   number = 1;
   number1 = 1;
}

seems to do the right thing?

> mainly because 'number' and 'foo_t' don't get entered into the
> symbol table, so we issue errors every time they are used, even if
> the use would have been valid given a valid declaration.

> For instance,

> inline int foo(void) { syntax error; }

> should become "extern int foo(void);" for purpose of further processing.

It already does:

double int number;

void maif() {
   number = 1;
   number1 = 1;
}

void sdf() {
  syntax error
}

void mae() {
  int a = sdf();
}

terr.c:1: two or more data types in declaration of `number'
terr.c: In function `maif':
terr.c:5: `number1' undeclared (first use in this function)
terr.c:5: (Each undeclared identifier is reported only once
terr.c:5: for each function it appears in.)
terr.c: In function `sdf':
terr.c:9: `syntax' undeclared (first use in this function)
terr.c:9: parse error before "error"
terr.c: In function `mae':
terr.c:13: void value not ignored as it ought to be

Did I construct a testcase that was too simple?

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sat, Nov 24, 2001 at 09:13:11PM -0800, mike stump wrote:
> > Date: Sat, 24 Nov 2001 16:41:55 -0800
> > From: Zack Weinberg <zack@codesourcery.com>
> > To: Gabriel Dos Reis <gdr@codesourcery.com>
> 
> > 2. As few spurious errors reported to the user as possible.  This is
> > one place where ERROR_MARK_NODE really falls down.  Consider these
> > ill-formed declarations:
> 
> > double int number;
> > typedef struct foo foo_t[;
> 
> > These will both provoke error cascades in the current compiler,
> 
> ?
> 
> double int number;
> 
> int maif() {
>    number = 1;
>    number1 = 1;
> }
> 
> seems to do the right thing?

Blah, my examples are too simple.

extern void fprint(int, const char *);

void print(str)
   string str;
{
  fprint(1, str);
}

$ gcc -Wall test.c
test.c:5: parse error before `string'
test.c:5: warning: type defaults to `int' in declaration of `str'
test.c:5: warning: data definition has no type or storage class

We should be able to figure out that "string" is expected to be a type
name, and that if it _were_ then there would be no problem with the
parameter declaration, and issue only an error such as

test.c:5: "string" is not a type name

I'm surprised to see it not complain about the fprint() line; I was
expecting a warning about an invalid conversion to pointer type.
It certainly should _not_ give that warning, since it does not know
whether "string" is a pointer type.

Or, 

int main(argc, argv)
   int argc;
   string argv[];
{
   int i;
   for(i=1; i<argc; i++)
    puts(argv[i]);
   return 0;
}

$ gcc -S test.c
test.c: In function `main':
test.c:3: parse error before `string'
test.c:7: subscripted value is neither array nor pointer

Here, we have no clue what the type of an element of argv is, but we
ought to be able to call it "array of <unknown type>" instead of just
"<unknown type>".

zw

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sat, Nov 24, 2001 at 09:13:11PM -0800, mike stump wrote:
> > Date: Sat, 24 Nov 2001 16:41:55 -0800
> > From: Zack Weinberg <zack@codesourcery.com>
> > To: Gabriel Dos Reis <gdr@codesourcery.com>
> 
> > 2. As few spurious errors reported to the user as possible.  This is
> > one place where ERROR_MARK_NODE really falls down.  Consider these
> > ill-formed declarations:
> 
> > double int number;
> > typedef struct foo foo_t[;
> 
> > These will both provoke error cascades in the current compiler,
> 
> ?
> 
> double int number;
> 
> int maif() {
>    number = 1;
>    number1 = 1;
> }
> 
> seems to do the right thing?

Blah, my examples are too simple.

extern void fprint(int, const char *);

void print(str)
   string str;
{
  fprint(1, str);
}

$ gcc -Wall test.c
test.c:5: parse error before `string'
test.c:5: warning: type defaults to `int' in declaration of `str'
test.c:5: warning: data definition has no type or storage class

We should be able to figure out that "string" is expected to be a type
name, and that if it _were_ then there would be no problem with the
parameter declaration, and issue only an error such as

test.c:5: "string" is not a type name

I'm surprised to see it not complain about the fprint() line; I was
expecting a warning about an invalid conversion to pointer type.
It certainly should _not_ give that warning, since it does not know
whether "string" is a pointer type.

Or, 

int main(argc, argv)
   int argc;
   string argv[];
{
   int i;
   for(i=1; i<argc; i++)
    puts(argv[i]);
   return 0;
}

$ gcc -S test.c
test.c: In function `main':
test.c:3: parse error before `string'
test.c:7: subscripted value is neither array nor pointer

Here, we have no clue what the type of an element of argv is, but we
ought to be able to call it "array of <unknown type>" instead of just
"<unknown type>".

zw

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Sat, 24 Nov 2001 11:30:47 +0000
> From: Neil Booth <neil@daikokuya.demon.co.uk>

> Do other compilers written in C have something equivalent?

You can tell if they do, even if you don't have source code, by
compiling something like:

struct A {
  char int j;
} a;

const int * ip;

main() {
  *ip = 1;
  a = a;
  a.i = 1;
  a.j = a.j;
}

and get messages like:

terr.c:2: two or more data types in declaration of `j'
terr.c: In function `main':
terr.c:8: warning: assignment of read-only location
terr.c:10: structure has no member named `i'

If you fail to get at least all of these, or if you get extra ones,
then the compiler is inferior and doesn't have anything like
error_mark_node (in this context).  If you get roughly exactly these
messages, then the compiler must.

This is a fundemental feature of the compilers.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

mike stump wrote:-

> You can tell if they do, even if you don't have source code, by
> compiling something like:
> 
> struct A {
>   char int j;
> } a;
> 
> const int * ip;
> 
> main() {
>   *ip = 1;
>   a = a;
>   a.i = 1;
>   a.j = a.j;
> }
> 
> and get messages like:
> 
> terr.c:2: two or more data types in declaration of `j'
> terr.c: In function `main':
> terr.c:8: warning: assignment of read-only location
> terr.c:10: structure has no member named `i'
>
> If you fail to get at least all of these, or if you get extra ones,
> then the compiler is inferior and doesn't have anything like
> error_mark_node (in this context).  If you get roughly exactly these
> messages, then the compiler must.

Why?  How do you know that the compiler doesn't just have (in
pseudo-code)

void grokdeclarator (decl)
{
  [...]
  if (multiple_type_declspecs_given)
    {
      output_error_msg;
      assume_the_latter_type;
    }
  [...]
}

and so enters it in its symbol table as if it saw

struct A {
  int j;
} a;

I think this would produce the same error messages you describe, and I
see no need for an error_mark thingy.  This is just what I'm saying we
should do - have some kind of intelligent recovery to something sane
that fits what the user might have wanted as closely as we can guess,
and proceed as normal.

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

mike stump wrote:-

> You can tell if they do, even if you don't have source code, by
> compiling something like:
> 
> struct A {
>   char int j;
> } a;
> 
> const int * ip;
> 
> main() {
>   *ip = 1;
>   a = a;
>   a.i = 1;
>   a.j = a.j;
> }
> 
> and get messages like:
> 
> terr.c:2: two or more data types in declaration of `j'
> terr.c: In function `main':
> terr.c:8: warning: assignment of read-only location
> terr.c:10: structure has no member named `i'
>
> If you fail to get at least all of these, or if you get extra ones,
> then the compiler is inferior and doesn't have anything like
> error_mark_node (in this context).  If you get roughly exactly these
> messages, then the compiler must.

Why?  How do you know that the compiler doesn't just have (in
pseudo-code)

void grokdeclarator (decl)
{
  [...]
  if (multiple_type_declspecs_given)
    {
      output_error_msg;
      assume_the_latter_type;
    }
  [...]
}

and so enters it in its symbol table as if it saw

struct A {
  int j;
} a;

I think this would produce the same error messages you describe, and I
see no need for an error_mark thingy.  This is just what I'm saying we
should do - have some kind of intelligent recovery to something sane
that fits what the user might have wanted as closely as we can guess,
and proceed as normal.

Neil.

Re: ICE in change_address at emit_rtl.c

[mike stump]

> Date: Fri, 23 Nov 2001 19:14:38 +0000
> From: Neil Booth <neil@daikokuya.demon.co.uk>
> To: Gabriel Dos Reis <gdr@codesourcery.com>

> > | I've not been able to find a satisfactory fix; since I'm not a fan of
> > | error_mark_node-s I don't want to introduce yet more checks for them.
> > | I hate the error_mark_node stuff; it would be nice if it could go
> > | away.
> > 
> > What should be the alternative?

> Generate something that is valid, even if it's not what the user
> specified.

Then you generate a spew of bad messages making gcc substantially
worse from the users perspective, or you in the end rediscover why
error_mark_node exists.  In the end, you can merely replace
error_mark_node with something equivalent, but in the end I can't help
but wonder that you'll have exactly what you started with, just with
all the code shaken up, or something substantially worse.

The theoretics are simple and trivial, for all X, for all OP, nil OP X
--> nil, and X OP nil --> nil.  In our case, nil == error_mark_node.

We hand optimize the checks for nil, opps, I mean, error_mark_node, to
just a few places where they can be (assuming that they don't happen
in other places), which is icky and unmaintainable and what I feel you
are reeling against.  Instead, imagine a language where this check for
nil was part of the language and the user didn't have to write any
code for it, and you will `see' the beauty in the code.  Kinda like
reading the callbacks in C as virtual functions and abstract object
factories and the like, and not being phased by the spelling.
Callbacks in C are ugly to the untrained eye, to a C++ person, a few
specific styles of callbacks are less than gross, even, dare say,
beautiful.

You want it to be better, well, a language upgrade that manages the
code for you (proves where checks for nil need not go, and puts them
everywhere else) would be the best way and then port the code of the
compiler to that system.  By language upgrade, I also would include a
pure library solution to manage the complexity.

> I can only see one good thing they help with: suppression of
> cascading errors.  I can't believe it's too hard to find another way
> of solving that.

It isn't.  error_mark_node does that.  :-) If you can offer a
refinement on the system that appeals to the eye and doesn't kill
compiler performance and preserves the non-cascading nature of
error_mark_node, we are all ears.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

mike stump wrote:-

> Then you generate a spew of bad messages making gcc substantially
> worse from the users perspective, or you in the end rediscover why
> error_mark_node exists.

I'm happy to waste a few days rediscovering that, then 8-) It's just
got too invasive and ill-defined for my liking.  e.g. you mentioned it
in operations, but it also creeps into types and other things.

Do other compilers written in C have something equivalent?  The only
other compiler whose code I've glanced at is LCC, and I don't recall
something equivalent there, but I've never used it either so I can't
comment on the quality of its diagnostics.  It is recursive descent
rather than yacc, so that might make a difference.

Neil.

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Neil Booth <neil@daikokuya.demon.co.uk> writes:

| but it also creeps into types and other things.

That is why is was mentionned in the past to introduce error_type_node
to renforce the tree type system.

As I mentionned earlier, there are parts of the compiler where
error_mark_node enhances readability and robustness.

-- Gaby
CodeSourcery, LLC                       http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Saturday, November 24, 2001 11:30:47 AM +0000 Neil Booth 
<neil@daikokuya.demon.co.uk> wrote:

> mike stump wrote:-
>
>> Then you generate a spew of bad messages making gcc substantially
>> worse from the users perspective, or you in the end rediscover why
>> error_mark_node exists.
>
> I'm happy to waste a few days rediscovering that, then 8-) It's just
> got too invasive and ill-defined for my liking.  e.g. you mentioned it
> in operations, but it also creeps into types and other things.
>
> Do other compilers written in C have something equivalent?

Yes.  I know two other successful compiler front ends quite well,
and both have a similar notion.  However, the places in which it
can appear are better documented, and they use the approach I
advocate (i.e., ERROR_MARK_TYPE, ERROR_MARK_DECL, etc.) rather
than just a single node.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Saturday, November 24, 2001 11:30:47 AM +0000 Neil Booth 
<neil@daikokuya.demon.co.uk> wrote:

> mike stump wrote:-
>
>> Then you generate a spew of bad messages making gcc substantially
>> worse from the users perspective, or you in the end rediscover why
>> error_mark_node exists.
>
> I'm happy to waste a few days rediscovering that, then 8-) It's just
> got too invasive and ill-defined for my liking.  e.g. you mentioned it
> in operations, but it also creeps into types and other things.
>
> Do other compilers written in C have something equivalent?

Yes.  I know two other successful compiler front ends quite well,
and both have a similar notion.  However, the places in which it
can appear are better documented, and they use the approach I
advocate (i.e., ERROR_MARK_TYPE, ERROR_MARK_DECL, etc.) rather
than just a single node.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[dewar]

<<Careful! "confused by earlier errors, bailing out" means that there
_was_ an ICE but we pretended it didn't happen because there was an
earlier error.  In this case, the undeclared strcmp and exit are
unlikely to have corrupted data structures down the line and provoked
the ICE, which means there's still a real bug here.  Put declarations
of strcmp and exit just above main() in that test file, and I bet you
the bug comes back.

This is why ICE suppression is a Bad Thing.
>>

What we do in the equivalent case in Ada is to suppress by default, but have
a switch to unsuppress, and that has proved a useful compromise.

ICE in change_address at emit_rtl.c

[Craig Rodrigues]

Hi,

Can someone with access to Solaris look at PR 4821
in GNATS and tell me if it is still a problem:

http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view&pr=4821&database=gcc

I've found 19 reports in GNATS with the same ICE under Solaris:
Internal compiler error in change_address, at emit-rtl.c:1635

Thanks.
-- 
Craig Rodrigues        
http://www.gis.net/~craigr    
rodrigc@mediaone.net          

Re: ICE in change_address at emit_rtl.c

[Gerald Pfeifer]

On Thu, 22 Nov 2001, Craig Rodrigues wrote:
> Can someone with access to Solaris look at PR 4821
> in GNATS and tell me if it is still a problem:
>
> http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view&pr=4821&database=gcc
>
> I've found 19 reports in GNATS with the same ICE under Solaris:
> Internal compiler error in change_address, at emit-rtl.c:1635

This specific instance it quite weird:

 o First, why is the file called fits.E?

 o Second, the file contains references to lib/gcc-lib/sparc-sun-solaris2/
   instead of ...solaris2.8 which the reporter is using.

 o Third, when I rename it to fits.ii and compile it with current CVS,
   on sparc-sun-solaris2.8 I get parse errors:

   In file included from ../../../src/ImageMagick-5.3.8/magick/api.h:85,
                 from ../../../src/ImageMagick-5.3.8/magick/magick.h:74,
                 from ../../../src/ImageMagick-5.3.8/coders/fits.c:55:
   ../../../src/ImageMagick-5.3.8/magick/image.h:296: parse error before `;' token
   In file included from ../../../src/ImageMagick-5.3.8/magick/api.h:93,
                 from ../../../src/ImageMagick-5.3.8/magick/magick.h:74,
                 from ../../../src/ImageMagick-5.3.8/coders/fits.c:55:
   ../../../src/ImageMagick-5.3.8/magick/cache.h:47: parse error before `;' token
  ../../../src/ImageMagick-5.3.8/coders/fits.c: In function `Image*
   ReadFITSImage(const ImageInfo*, ExceptionInfo*)':
  ../../../src/ImageMagick-5.3.8/coders/fits.c:335: parse error before `
   PseudoClass'

  When I compile with 3.0.3-snapshot, I get:

  nunki[63]:~% gccvs -O3 -D_PTHREADS fits.ii
  In file included from ../../../src/ImageMagick-5.3.8/magick/api.h:85,
                   from ../../../src/ImageMagick-5.3.8/magick/magick.h:74,
                  from ../../../src/ImageMagick-5.3.8/coders/fits.c:55:
  ../../../src/ImageMagick-5.3.8/magick/image.h:296: parse error before `;' token
  In file included from ../../../src/ImageMagick-5.3.8/magick/api.h:93,
                 from ../../../src/ImageMagick-5.3.8/magick/magick.h:74,
                 from ../../../src/ImageMagick-5.3.8/coders/fits.c:55:
  ../../../src/ImageMagick-5.3.8/magick/cache.h:47: parse error before `;' token
  ../../../src/ImageMagick-5.3.8/coders/fits.c: In function `Image*
     ReadFITSImage(const ImageInfo*, ExceptionInfo*)':
  ../../../src/ImageMagick-5.3.8/coders/fits.c:335: parse error before `
     PseudoClass'
  ../../../src/ImageMagick-5.3.8/coders/fits.c:482: confused by earlier
  errors, bailing out

Given the error message, it seems this was an instance of ICE on invalid
code, and the report can be closed (for mainline and apparently also
branch).

HTH,

Gerald
-- 
Gerald "Jerry" pfeifer@dbai.tuwien.ac.at http://www.dbai.tuwien.ac.at/~pfeifer/

Re: ICE in change_address at emit_rtl.c

[Craig Rodrigues]

On Thu, Nov 22, 2001 at 12:40:15PM +0100, Gerald Pfeifer wrote:
> On Thu, 22 Nov 2001, Craig Rodrigues wrote:
> > Can someone with access to Solaris look at PR 4821
> > in GNATS and tell me if it is still a problem:
> >
> > http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view&pr=4821&database=gcc
> 
> Given the error message, it seems this was an instance of ICE on invalid
> code, and the report can be closed (for mainline and apparently also
> branch).


OK, that one might be a bad report.   These ones have
the same ICE error message.  Can they be closed?

4916, 4821, 4734, 4684, 4613, 4612, 4611, 4410, 4297, 4916, 
4821, 4734, 4684, 4613, 4612, 4611, 4581, 4438, 4410, 4297

Thanks.
-- 
Craig Rodrigues        
http://www.gis.net/~craigr    
rodrigc@mediaone.net          

Re: ICE in change_address at emit_rtl.c

[Gerald Pfeifer]

On Thu, 22 Nov 2001, Craig Rodrigues wrote:
> OK, that one might be a bad report.   These ones have
> the same ICE error message.  Can they be closed?
>
> 4916, 4821, 4734, 4684, 4613, 4612, 4611, 4410, 4297, 4916,
> 4821, 4734, 4684, 4613, 4612, 4611, 4581, 4438, 4410, 4297

4297 can be closed (verified on sparc-sun-solaris2.8 for current 3.0.3-CVS):

  nunki[95]:~% gccvs main.ii -O3
  main.c: In function `int main(int, char**)':
  main.c:169: `strcmp' undeclared (first use this function)
  main.c:169: (Each undeclared identifier is reported only once for each function
   it appears in.)
  main.c:171: `exit' undeclared (first use this function)
  main.c:249: confused by earlier errors, bailing out

4410 can be closed (verified with -01 to -O3):

  nunki[103]:~% gccvs d3.i -O3
  d3.i:16: redeclaration of C++ built-in type `bool'
  d3.i: In function `fint drawaxis_c(fchar, double*, double*, double*, double*,
   bool*, fchar, fint*, fint*, double*, fchar, fchar, fint*, fchar, double*,
   fint*, double*, fchar, double*, fchar)':
  d3.i:440: `proco_correct' undeclared (first use this function)
  d3.i:440: (Each undeclared identifier is reported only once for each function
   it appears in.)
  d3.i:592: `poserr' undeclared (first use this function)
  d3.i:614: `cotranserr' undeclared (first use this function)
  d3.i:665: `dcderr' undeclared (first use this function)
  d3.i:815: `procoerr' undeclared (first use this function)
  d3.i:995: `hmsdms' undeclared (first use this function)

4438 can be closed: Lots of parser errors, no crash.

4581 is a bootstrap error; I'd put this in feedback mode and ask whether
it still exists.

4611, ask for feedack, as this is an older version of Solaris which I do
not have access to.

4612, without various -O options:

  In file included from ../magick/api.h:85,
                 from ../magick/magick.h:74,
                 from fits.c:55:
  ../magick/image.h:296: parse error before `;' token
  In file included from ../magick/api.h:95,
                 from ../magick/magick.h:74,
                 from fits.c:55:
  ../magick/cache.h:47: parse error before `;' token
  fits.c: In function `Image* ReadFITSImage(const ImageInfo*, ExceptionInfo*)':
  fits.c:335: parse error before `PseudoClass'


As I'll be out of town next week, I won't be able to have a look at the
other reports, I'm afraid.

Gerald
-- 
Gerald "Jerry" pfeifer@dbai.tuwien.ac.at http://www.dbai.tuwien.ac.at/~pfeifer/

Re: ICE in change_address at emit_rtl.c

[Gerald Pfeifer]

On Thu, 22 Nov 2001, Gerald Pfeifer wrote:
>> 4916, 4821, 4734, 4684, 4613, 4612, 4611, 4410, 4297, 4916,
>> 4821, 4734, 4684, 4613, 4612, 4611, 4581, 4438, 4410, 4297

I found a bit more time:

4916: annoying report, apparently no attempt to minimize problem,
incorrect GNATS categories, could not properly obtain .i file from
web page.

4821: duplicate (ImageMagick, as usual); does not crash any longer.
I closed this report.

4734: Incomplete report and actually a duplicate.
I closed this report.

4684: A nice complete report, still a duplicate, and still solved.
I closed this report.

4613: I could reproduce this with the 2001-11-22 version of GCC on
the 3.0-branch on sparc-sun-solaris2.8.

I put this into "analysed" state, and you might want to escalate this,
Craig.

4612: Fixed on the 3.0-branch and thus GCC 3.0.3.
Invalid code according to the compiler, so I changed this to
ice-on-illegal-code as well, and closed the report.

4611: I believe this to be fixed as well, but I do not have a
sparc-sun-solaris2.5.1 box left, so I cannot completely verify
that. We might put this into feedback mode after 3.0.3 is released
to have the submitter verify whether it's fixed or not.

4410 is a duplicate, I'm quite sure, but I didn't have time to find
the original.


Hopefully others will take care of the other reports. If not, I suggest
to send another mail to gcc@gcc.gnu.org, and if nobody takes care of
testing these PRs, I'll try to do the remaining ones after my return.

Gerald
-- 
Gerald "Jerry" pfeifer@dbai.tuwien.ac.at http://www.dbai.tuwien.ac.at/~pfeifer/

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Thu, Nov 22, 2001 at 01:18:02PM +0100, Gerald Pfeifer wrote:
> On Thu, 22 Nov 2001, Craig Rodrigues wrote:
> > OK, that one might be a bad report.   These ones have
> > the same ICE error message.  Can they be closed?
> >
> > 4916, 4821, 4734, 4684, 4613, 4612, 4611, 4410, 4297, 4916,
> > 4821, 4734, 4684, 4613, 4612, 4611, 4581, 4438, 4410, 4297
> 
> 4297 can be closed (verified on sparc-sun-solaris2.8 for current 3.0.3-CVS):
> 
>   nunki[95]:~% gccvs main.ii -O3
>   main.c: In function `int main(int, char**)':
>   main.c:169: `strcmp' undeclared (first use this function)
>   main.c:169: (Each undeclared identifier is reported only once for each function
>    it appears in.)
>   main.c:171: `exit' undeclared (first use this function)
>   main.c:249: confused by earlier errors, bailing out

Careful! "confused by earlier errors, bailing out" means that there
_was_ an ICE but we pretended it didn't happen because there was an
earlier error.  In this case, the undeclared strcmp and exit are
unlikely to have corrupted data structures down the line and provoked
the ICE, which means there's still a real bug here.  Put declarations
of strcmp and exit just above main() in that test file, and I bet you
the bug comes back.

This is why ICE suppression is a Bad Thing.

zw

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> Careful! "confused by earlier errors, bailing out" means that there
> _was_ an ICE but we pretended it didn't happen because there was an
> earlier error.  In this case, the undeclared strcmp and exit are
> unlikely to have corrupted data structures down the line and provoked
> the ICE, which means there's still a real bug here.  Put declarations
> of strcmp and exit just above main() in that test file, and I bet you
> the bug comes back.

I've traced many of these "confused ... bailing out" in the C front
end; they are usually in grokdeclarator.  Something gets set to NULL
and then derefenced (caused by an unexpected type of error_mark_node
IIRC).

I've not been able to find a satisfactory fix; since I'm not a fan of
error_mark_node-s I don't want to introduce yet more checks for them.
I hate the error_mark_node stuff; it would be nice if it could go
away.

Neil.

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Neil Booth <neil@daikokuya.demon.co.uk> writes:

[...]

| I've not been able to find a satisfactory fix; since I'm not a fan of
| error_mark_node-s I don't want to introduce yet more checks for them.
| I hate the error_mark_node stuff; it would be nice if it could go
| away.

What should be the alternative?

I think the error_mark_node thingy is used when it at some stages of
the parsing, it is not appropriate to return a NULL_TREE.

-- Gaby
CodeSourcery, LLC                       http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Gabriel Dos Reis wrote:-

> Neil Booth <neil@daikokuya.demon.co.uk> writes:
> 
> [...]
> 
> | I've not been able to find a satisfactory fix; since I'm not a fan of
> | error_mark_node-s I don't want to introduce yet more checks for them.
> | I hate the error_mark_node stuff; it would be nice if it could go
> | away.
> 
> What should be the alternative?

Generate something that is valid, even if it's not what the user
specified.  e.g. if it's an invalid typedef, typedef it to int (or
something more intelligent if possible) rather than error_mark_node.
Or don't enter it in the symbol table at all.  The problem with
error_mark_node is that it's too easy to forget to check for them, and
that it's not documented where they can and can't appear.  Some
routines do check, some don't.  If I write a new routine, how can I be
sure?  You end up checking all the time, redundantly, which pessimizes
and obfuscates the code.

I can only see one good thing they help with: suppression of cascading
errors.  I can't believe it's too hard to find another way of solving
that.  It's not as if it's fully solved anyway; the existing C parser
often leads to cascading errors.

Neil.

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Fri, Nov 23, 2001 at 07:14:38PM +0000, Neil Booth wrote:
> Gabriel Dos Reis wrote:-
> 
> > Neil Booth <neil@daikokuya.demon.co.uk> writes:
> > 
> > [...]
> > 
> > | I've not been able to find a satisfactory fix; since I'm not a fan of
> > | error_mark_node-s I don't want to introduce yet more checks for them.
> > | I hate the error_mark_node stuff; it would be nice if it could go
> > | away.
> > 
> > What should be the alternative?
> 
> Generate something that is valid, even if it's not what the user
> specified.  e.g. if it's an invalid typedef, typedef it to int (or
> something more intelligent if possible) rather than error_mark_node.

FWIW I would like to see this too.

Regarding ICE suppression, I'm in favor of one or more of the
following changes:

- Disable it entirely when ENABLE_CHECKING is defined.  It would then
  be off in the mainline and on in the releases.

- Add a command line switch to disable it, as Ada does.

- Attempt to be more intelligent about when an ICE should be
  suppressed; only some errors produce malformed internal data
  structures as a side effect, and most of those would only affect
  compilation of the containing function, not the rest of the source.
  (The latter of course becomes less true as we do more
  interprocedural analysis.)

zw

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> > > What should be the alternative?
> > 
> > Generate something that is valid, even if it's not what the user
> > specified.  e.g. if it's an invalid typedef, typedef it to int (or
> > something more intelligent if possible) rather than error_mark_node.
> 
> FWIW I would like to see this too.

Hmm, OK, if you think it's possible too then it probably is.  I might
have a stab at gradually removing it from the C and ObjC front ends
after 3.1 has branched; it's not worth it now.  Does that seem a
reasonable goal to you Joseph?

Neil.

Re: ICE in change_address at emit_rtl.c

[Joseph S. Myers]

On Fri, 23 Nov 2001, Neil Booth wrote:

> Hmm, OK, if you think it's possible too then it probably is.  I might
> have a stab at gradually removing it from the C and ObjC front ends
> after 3.1 has branched; it's not worth it now.  Does that seem a
> reasonable goal to you Joseph?

What of the other idea that has been discussed in the past, of typed error
marks (separate ones for erroneous types, erroneous decls, ...)?

We should first define the goals:

* How much do we want to help users by finding multiple errors in a single
compilation run (the point of not just stopping after the first error)?

* What should we not try to do after error?  For example, after error we
should not attempt to optimize or convert to RTL any later part of the
file, even if this means we miss some diagnostics; we should simply do the
basic parsing and error checking.  Are there other warning checks - beyond
the basic constraint checking required by the standard - which should be
disabled after error?

* On error, how far should we skip before trying to make sense of the code
again?

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Joseph S. Myers wrote:-

> On Fri, 23 Nov 2001, Neil Booth wrote:
> 
> > Hmm, OK, if you think it's possible too then it probably is.  I might
> > have a stab at gradually removing it from the C and ObjC front ends
> > after 3.1 has branched; it's not worth it now.  Does that seem a
> > reasonable goal to you Joseph?
> 
> What of the other idea that has been discussed in the past, of typed error
> marks (separate ones for erroneous types, erroneous decls, ...)?

Ugh, no, I want to get rid of them completely, not to add more.  They
just get in the way, and make obviously correct subroutines possibly
incorrect.

> We should first define the goals:
> 
> * How much do we want to help users by finding multiple errors in a single
> compilation run (the point of not just stopping after the first error)?

As much as reasonably possible.  I don't see that removing error marks
affects this (indeed, I think it might even improve it, since things
that might have been error marks before might now be something
sensible, maybe finding other syntax errors).

> * What should we not try to do after error?  For example, after error we
> should not attempt to optimize or convert to RTL any later part of the
> file, even if this means we miss some diagnostics; we should simply do the
> basic parsing and error checking.  Are there other warning checks - beyond
> the basic constraint checking required by the standard - which should be
> disabled after error?

I don't know.  I think we should aim for simplification, and when
something undesirable comes up, fix it.  I don't want to overdesign
this.

However, there are easy ways to be less drastic than you suggest.  For
example, if something has gone wrong (with decls, say) local to a
function, we could avoid converting that _function_ to RTL or
whatever, but it shouldn't normally prevent us from continuing with
the rest of the file.

> * On error, how far should we skip before trying to make sense of the code
> again?

I think we should try not skipping at all, and fix any nasty effects
of this as they arise.  I might be naive here, but I don't think it
can be that hard.  For example, if a subexpression of an arithmetic
operator goes wonky, use an int or double of 1; it shouldn't cause any
new errors or warnings (0 could, of course).

I imagine the hardest stuff is when you have a certain kind of syntax
error, and you don't know whether you saw a decl, or struct, or ...
We should just continue to do what we do now, which appears to be to
skip to an appropriate looking ';' '}' or whatever.

Neil.

Re: ICE in change_address at emit_rtl.c

[Joseph S. Myers]

On Sat, 24 Nov 2001, Neil Booth wrote:

> However, there are easy ways to be less drastic than you suggest.  For
> example, if something has gone wrong (with decls, say) local to a
> function, we could avoid converting that _function_ to RTL or
> whatever, but it shouldn't normally prevent us from continuing with
> the rest of the file.

Since we're not in the end going to generate object code from a file with
errors in it, I'd think we might as well save CPU time and display errors
in other functions more quickly without making attempts at optimizing.

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Neil Booth <neil@daikokuya.demon.co.uk> writes:

| Joseph S. Myers wrote:-
| 
| > On Fri, 23 Nov 2001, Neil Booth wrote:
| > 
| > > Hmm, OK, if you think it's possible too then it probably is.  I might
| > > have a stab at gradually removing it from the C and ObjC front ends
| > > after 3.1 has branched; it's not worth it now.  Does that seem a
| > > reasonable goal to you Joseph?
| > 
| > What of the other idea that has been discussed in the past, of typed error
| > marks (separate ones for erroneous types, erroneous decls, ...)?
| 
| Ugh, no, I want to get rid of them completely, not to add more.  They
| just get in the way, and make obviously correct subroutines possibly
| incorrect.

It depends on your definition of "obviously correct".  Various parts
of cp/pt.c which use error_mark_node which are clear as written.  I
don't know how they would be "obvious correct" without them.

-- Gaby

Re: ICE in change_address at emit_rtl.c

[Michal Moskal]

On Sat, Nov 24, 2001 at 12:21:02AM +0000, Neil Booth wrote:
> Joseph S. Myers wrote:-
> > * On error, how far should we skip before trying to make sense of the code
> > again?
> 
> I think we should try not skipping at all, and fix any nasty effects
> of this as they arise.  I might be naive here, but I don't think it
> can be that hard.  For example, if a subexpression of an arithmetic
> operator goes wonky, use an int or double of 1; it shouldn't cause any
> new errors or warnings (0 could, of course).

12 / ((foo - bar) - 1), when (foo - bar) is invalid? I guess that it
would be better to insert refernce to some bogus, artificial, variable,
then constant.

-- 
: Michal ``,/\/\,       '' Moskal    | |            : GCS {C,UL}++++$
:          |    |alekith      @    |)|(| . org . pl : {E--, W, w-,M}-
:    Linux: We are dot in .ORG.    |                : {b,e>+}++ !tv h
: CurProj: ftp://ftp.pld.org.pl/people/malekith/ksi : PLD Team member

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Neil Booth <neil@daikokuya.demon.co.uk> writes:

| Zack Weinberg wrote:-
| 
| > > > What should be the alternative?
| > > 
| > > Generate something that is valid, even if it's not what the user
| > > specified.  e.g. if it's an invalid typedef, typedef it to int (or
| > > something more intelligent if possible) rather than error_mark_node.
| > 
| > FWIW I would like to see this too.
| 
| Hmm, OK, if you think it's possible too then it probably is.

At this point, I feel I have to clarify my previous point.
I'm not saying it is impossible -- I didn't try, so I can't make that
assertion.  I was merely asking for the alternative.

Since merging the C and C++ front-ends is a goal repeatedly asserted,
I'm worried about the alternative.  There are various parts of the
compiler (the obvious one is cp/pt.c) where using a default type like
int just does not make sense -- it might be even worse than
error_mark_node. 

-- Gaby
CodeSourcery, LLC                       http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Gabriel Dos Reis wrote:-

> Since merging the C and C++ front-ends is a goal repeatedly asserted,
> I'm worried about the alternative.  There are various parts of the
> compiler (the obvious one is cp/pt.c) where using a default type like
> int just does not make sense -- it might be even worse than
> error_mark_node. 

But templates are part of C++ not C, so C needn't worry about them.

Like I said, maybe I'm naive, but I think it's worth a try if it
doesn't interfere with others' development.  In other words, I'm happy
to muck around only on my local tree and fail.  But if it succeeds,
then I think it should be seriously considered as a change worth
making.

Neil.

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Neil Booth <neil@daikokuya.demon.co.uk> writes:

| Gabriel Dos Reis wrote:-
| 
| > Since merging the C and C++ front-ends is a goal repeatedly asserted,
| > I'm worried about the alternative.  There are various parts of the
| > compiler (the obvious one is cp/pt.c) where using a default type like
| > int just does not make sense -- it might be even worse than
| > error_mark_node. 
| 
| But templates are part of C++ not C, so C needn't worry about them.

I agree that templates are not part of C. But in the merged front-end
where do you put the line?  Please remember that template
instantiation sort of is a degenerated form of parsing where some
routines are shared by the two front-ends.

-- Gaby

Re: ICE in change_address at emit_rtl.c

[Phil Edwards]

On Fri, Nov 23, 2001 at 03:37:09PM -0800, Zack Weinberg wrote:
> Regarding ICE suppression, I'm in favor of one or more of the
> following changes:
> 
> - Disable it entirely when ENABLE_CHECKING is defined.  It would then
>   be off in the mainline and on in the releases.

That sounds good.

I've wondered why this idea hasn't gotten more attention:

    http://gcc.gnu.org/ml/gcc-patches/2001-04/msg00708.html

Phil

-- 
If ye love wealth greater than liberty, the tranquility of servitude greater
than the animating contest for freedom, go home and leave us in peace.  We seek
not your counsel, nor your arms.  Crouch down and lick the hand that feeds you;
and may posterity forget that ye were our countrymen.            - Samuel Adams

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Friday, November 23, 2001 07:14:38 PM +0000 Neil Booth 
<neil@daikokuya.demon.co.uk> wrote:

> Gabriel Dos Reis wrote:-
>
>> Neil Booth <neil@daikokuya.demon.co.uk> writes:
>>
>> [...]
>>
>> | I've not been able to find a satisfactory fix; since I'm not a fan of
>> | error_mark_node-s I don't want to introduce yet more checks for them.
>> | I hate the error_mark_node stuff; it would be nice if it could go
>> | away.
>>
>> What should be the alternative?

A solution I have advocated in the past is to have ERROR_MARK_TYPE,
ERROR_MARK_DECL, etc.  This would help, since using DECL_ARTIFICIAL,
say, on the ERROR_MARK_DECL would work.

Neil's approach is possible as well.

Neil is right that the current strategy actually makes GCC harder
to work on, and does little to improve reliability.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Gabriel Dos Reis]

Mark Mitchell <mark@codesourcery.com> writes:

| --On Friday, November 23, 2001 07:14:38 PM +0000 Neil Booth 
| <neil@daikokuya.demon.co.uk> wrote:
| 
| > Gabriel Dos Reis wrote:-
| >
| >> Neil Booth <neil@daikokuya.demon.co.uk> writes:
| >>
| >> [...]
| >>
| >> | I've not been able to find a satisfactory fix; since I'm not a fan of
| >> | error_mark_node-s I don't want to introduce yet more checks for them.
| >> | I hate the error_mark_node stuff; it would be nice if it could go
| >> | away.
| >>
| >> What should be the alternative?
| 
| A solution I have advocated in the past is to have ERROR_MARK_TYPE,
| ERROR_MARK_DECL, etc.  This would help, since using DECL_ARTIFICIAL,
| say, on the ERROR_MARK_DECL would work.
| 
| Neil's approach is possible as well.
| 
| Neil is right that the current strategy actually makes GCC harder
| to work on, and does little to improve reliability.

I share Neil's sentiment. 

What worries is the alternative he's proposing.  My personnal view is
to have ERROR_MARK_XXX for TYPE, DECL and such in order to enhance
the tree type system and maintainability.  But Neil is proposing to
get rid of those nodes.  Maybe it is appropriate to have an actual
patch to comment on, although I'm skeptical about the long-term
viability of getting rid of ERROR_MARK_XXX thingies.

-- Gaby
CodeSourcery, LLC                       http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Gabriel Dos Reis wrote:-

> What worries is the alternative he's proposing.  My personnal view is
> to have ERROR_MARK_XXX for TYPE, DECL and such in order to enhance
> the tree type system and maintainability.

Changing tack a little now, but still on the topic of violent changes
to improve readability 8-)

I've often wondered whether we might do better type-wise to actually
use separate types, and refer to them as separate types, rather than
having every function take an effectively typeless "tree" as input.
This alone would help type safety quite a bit.  If we used separate
types, it would be a good idea, I suspect, to not use macros as
accessors as frequently as we do now, but instead to have each member
of the tree union have its own names for the common fields.  I realise
that the macros improve maintainability for moving stuff around, but I
think a lot of that problem is caused by extreme overloading of
e.g. the bitfields in tree_common whose name doesn't correspond to
what they are used for for most types.

An illustrative example of what is wrong with the current approach:
can you tell me exactly which bit fields in tree_common are used by,
say, an IDENTIFIER_NODE across front ends, and which are free, just by
looking at the documentation in tree.h?  [You have a better chance now
because I updated the documentation in tree.h a few months back from
my own investigations; I found it didn't mention at least three of the
bits.]  If identifiers had their own struct without attempting to
share everything with tree_common, this question would not even arise.

It would also help towards solving the space-issue for identifiers,
which currently take up far more memory than they really need, and
pushes them into the next GC allocation bucket.

Am I right in thinking that the only common thing that every "tree"
really needs to have is the chain element and tree code?

Neil.

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sat, Nov 24, 2001 at 11:02:56PM +0000, Neil Booth wrote:
> 
> I've often wondered whether we might do better type-wise to actually
> use separate types, and refer to them as separate types, rather than
> having every function take an effectively typeless "tree" as input.

Once again you seem to be thinking along parallel lines to me - I was
coming at it from the tack of trying to cut down memory consumption,
but it's the same problem.

I see definite benefits from using independent structures for all
constants, identifiers, and possibly types.  I don't think it makes
sense to go as far as separating all the current members of tree_union.
It is genuinely useful to be able to treat the operands of any expression
with the same logic, which argues for e.g. leaving declarations as part of
the tree union (but then, it does the same for constants...)

> Am I right in thinking that the only common thing that every "tree"
> really needs to have is the chain element and tree code?

Less! An identifier has no use for the chain, or for most of the flags.

There are some counter-pressures against shrinking trees too far.  One is
the real danger of running out of tree_code values.  That's an 8-bit field
and C++ has more than 200 tree codes.  Another is the demand for flag bits.
And there are very few trees that don't need at least a cons-cell worth of
pointers; that's eight bytes (on a 32-bit machine) right there.

(Oh, can we please #if 0 out the aux field in tree_common?  The only
code that's using it is on the ast-optimizer-branch which will not be
merged in 3.1 - all it's doing is wasting memory.)

zw

Re: ICE in change_address at emit_rtl.c

[Joseph S. Myers]

On Sat, 24 Nov 2001, Zack Weinberg wrote:

> It is genuinely useful to be able to treat the operands of any expression
> with the same logic, which argues for e.g. leaving declarations as part of
> the tree union (but then, it does the same for constants...)

With (declarations, expressions, constants, ...) separated, one of the
types of expression would be a thin wrapper around a declaration to refer
to that declaration (where at present we just put the decl in the
expression).  One of the types would be a thin wrapper around a constant,
that just adds a type (and maybe flags - such as whether the constant,
which may have arisen by folding, is actually a constant expression within
the meaning of the language standard) to the constant; the constant itself
would be one of (fixed-width representation with sign, GCC-local
arbitrary-precision, GMP) and could be used in RTL as well.  (Actually,
there might be separate types of expression to wrap integer constants and
to wrap floating-point constants.)

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sun, Nov 25, 2001 at 01:31:33AM +0000, Joseph S. Myers wrote:
> On Sat, 24 Nov 2001, Zack Weinberg wrote:
> 
> > It is genuinely useful to be able to treat the operands of any expression
> > with the same logic, which argues for e.g. leaving declarations as part of
> > the tree union (but then, it does the same for constants...)
> 
> With (declarations, expressions, constants, ...) separated, one of the
> types of expression would be a thin wrapper around a declaration to refer
> to that declaration (where at present we just put the decl in the
> expression).

Okay, but then I'm not sure we wind up saving any memory.

zw

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

> (Oh, can we please #if 0 out the aux field in tree_common?  The only
> code that's using it is on the ast-optimizer-branch which will not be
> merged in 3.1 - all it's doing is wasting memory.)

Just remove it, until the branch is merged.  And then we might want
to use an on-the-side table, depending.

Patch pre-approved.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sun, Nov 25, 2001 at 12:38:33AM -0800, Mark Mitchell wrote:
> >(Oh, can we please #if 0 out the aux field in tree_common?  The only
> >code that's using it is on the ast-optimizer-branch which will not be
> >merged in 3.1 - all it's doing is wasting memory.)
> 
> Just remove it, until the branch is merged.  And then we might want
> to use an on-the-side table, depending.
> 
> Patch pre-approved.

Neil tried to do this a few weeks back and got shot down because
apparently it would make trunk->branch merges harder.  What has
changed?

zw

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Monday, November 26, 2001 11:45:51 AM -0800 Zack Weinberg 
<zack@codesourcery.com> wrote:

> On Sun, Nov 25, 2001 at 12:38:33AM -0800, Mark Mitchell wrote:
>> > (Oh, can we please #if 0 out the aux field in tree_common?  The only
>> > code that's using it is on the ast-optimizer-branch which will not be
>> > merged in 3.1 - all it's doing is wasting memory.)
>>
>> Just remove it, until the branch is merged.  And then we might want
>> to use an on-the-side table, depending.
>>
>> Patch pre-approved.
>
> Neil tried to do this a few weeks back and got shot down because
> apparently it would make trunk->branch merges harder.  What has
> changed?

That I didn't see the original discussion, and therefore waded in
with a new, unilateral decision, thinking that nobody had made a
decision before.

But, I justify my decision like this:

1. The trunk should be focused on the 3.1 release, and this field
   has no value in that release.

2. It is unclear that this is even the right data structure to use.
   Inflating the size of all trees, just for use during the AST
   optimization phase, is not necessarily a good design.

3. How much harder is the merge?  The next time you merge to the
   branch, you manually keep the aux field.  Then you have it
   forever more.

Certainly, in general, people working on branches have to be willing
to tolerate shakeups on the mainline.  Recall that I have a new C++
parser branch sitting in limbo like this, so I feel the pain along
with everyone else.

Since there is apparently conflict, we should give the people who
originally opposed removing aux a chance to debate.

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Mon, Nov 26, 2001 at 11:50:10AM -0800, Mark Mitchell wrote:
> >>Patch pre-approved.
> >
> >Neil tried to do this a few weeks back and got shot down because
> >apparently it would make trunk->branch merges harder.  What has
> >changed?
> 
> That I didn't see the original discussion, and therefore waded in
> with a new, unilateral decision, thinking that nobody had made a
> decision before.
> 
> But, I justify my decision like this:
> 
> 1. The trunk should be focused on the 3.1 release, and this field
>   has no value in that release.
> 
> 2. It is unclear that this is even the right data structure to use.
>   Inflating the size of all trees, just for use during the AST
>   optimization phase, is not necessarily a good design.
> 
> 3. How much harder is the merge?  The next time you merge to the
>   branch, you manually keep the aux field.  Then you have it
>   forever more.

For the record, I agree with all of this reasoning.  I just wanted to
make sure that the constituents of the ast-optimizer-branch didn't get
railroaded.

zw

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Mon, Nov 26, 2001 at 11:50:10AM -0800, Mark Mitchell wrote:
> >>Patch pre-approved.
> >
> >Neil tried to do this a few weeks back and got shot down because
> >apparently it would make trunk->branch merges harder.  What has
> >changed?
> 
> That I didn't see the original discussion, and therefore waded in
> with a new, unilateral decision, thinking that nobody had made a
> decision before.
> 
> But, I justify my decision like this:
> 
> 1. The trunk should be focused on the 3.1 release, and this field
>   has no value in that release.
> 
> 2. It is unclear that this is even the right data structure to use.
>   Inflating the size of all trees, just for use during the AST
>   optimization phase, is not necessarily a good design.
> 
> 3. How much harder is the merge?  The next time you merge to the
>   branch, you manually keep the aux field.  Then you have it
>   forever more.

For the record, I agree with all of this reasoning.  I just wanted to
make sure that the constituents of the ast-optimizer-branch didn't get
railroaded.

zw

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Monday, November 26, 2001 11:45:51 AM -0800 Zack Weinberg 
<zack@codesourcery.com> wrote:

> On Sun, Nov 25, 2001 at 12:38:33AM -0800, Mark Mitchell wrote:
>> > (Oh, can we please #if 0 out the aux field in tree_common?  The only
>> > code that's using it is on the ast-optimizer-branch which will not be
>> > merged in 3.1 - all it's doing is wasting memory.)
>>
>> Just remove it, until the branch is merged.  And then we might want
>> to use an on-the-side table, depending.
>>
>> Patch pre-approved.
>
> Neil tried to do this a few weeks back and got shot down because
> apparently it would make trunk->branch merges harder.  What has
> changed?

That I didn't see the original discussion, and therefore waded in
with a new, unilateral decision, thinking that nobody had made a
decision before.

But, I justify my decision like this:

1. The trunk should be focused on the 3.1 release, and this field
   has no value in that release.

2. It is unclear that this is even the right data structure to use.
   Inflating the size of all trees, just for use during the AST
   optimization phase, is not necessarily a good design.

3. How much harder is the merge?  The next time you merge to the
   branch, you manually keep the aux field.  Then you have it
   forever more.

Certainly, in general, people working on branches have to be willing
to tolerate shakeups on the mainline.  Recall that I have a new C++
parser branch sitting in limbo like this, so I feel the pain along
with everyone else.

Since there is apparently conflict, we should give the people who
originally opposed removing aux a chance to debate.

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sun, Nov 25, 2001 at 12:38:33AM -0800, Mark Mitchell wrote:
> >(Oh, can we please #if 0 out the aux field in tree_common?  The only
> >code that's using it is on the ast-optimizer-branch which will not be
> >merged in 3.1 - all it's doing is wasting memory.)
> 
> Just remove it, until the branch is merged.  And then we might want
> to use an on-the-side table, depending.
> 
> Patch pre-approved.

Neil tried to do this a few weeks back and got shot down because
apparently it would make trunk->branch merges harder.  What has
changed?

zw

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

> (Oh, can we please #if 0 out the aux field in tree_common?  The only
> code that's using it is on the ast-optimizer-branch which will not be
> merged in 3.1 - all it's doing is wasting memory.)

Just remove it, until the branch is merged.  And then we might want
to use an on-the-side table, depending.

Patch pre-approved.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> I see definite benefits from using independent structures for all
> constants, identifiers, and possibly types.  I don't think it makes
> sense to go as far as separating all the current members of tree_union.
> It is genuinely useful to be able to treat the operands of any expression
> with the same logic, which argues for e.g. leaving declarations as part of
> the tree union (but then, it does the same for constants...)

Certainly, I'm not advocating taking them out of the tree union.  I
suppose what I am advocating, but in different words, is the death of
tree_common.  We then have

struct tree_identifier
{
  tree chain;
  ENUM_BITFIELD(tree_code) code:8;
  [...]
};

with the same two common fields for other trees, but everything else
is specific to the struct tree_xxx in question.

> Less! An identifier has no use for the chain, or for most of the flags.

Are you certain of that?  Nothing anywhere chains identifiers together
into lists?  Since everything else requires the chain (I think), I
imagine it'd be better to keep it so that identifiers remain trees
(assuming that the state of "being a tree" means to have the two
fields above, and GCC code can assume that).  Or do you have another
plan?

> There are some counter-pressures against shrinking trees too far.  One is
> the real danger of running out of tree_code values.  That's an 8-bit field
> and C++ has more than 200 tree codes.  Another is the demand for flag bits.
> And there are very few trees that don't need at least a cons-cell worth of
> pointers; that's eight bytes (on a 32-bit machine) right there.

What I had in mind wouldn't add any tree_code values, at least I don't
think so and not initially.

I'd also like to see us go down the road that rth mentioned,
i.e. losing all trees with side effects - something else that causes
no end of special cases to be scattered all over the place.  That
would get rid of {UN,}SAVE_EXPR and {PRE,POST}{DECREMENT,INCREMENT}
etc., and some bugs that cause existing outstanding PRs I think.  If
we are in danger of having more than 256 tree codes, I think that's a
sign that something is wrong.

Neil.

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sun, Nov 25, 2001 at 11:51:13AM +0000, Neil Booth wrote:
> 
> Certainly, I'm not advocating taking them out of the tree union.  I
> suppose what I am advocating, but in different words, is the death of
> tree_common.  We then have
> 
> struct tree_identifier
> {
>   tree chain;
>   ENUM_BITFIELD(tree_code) code:8;
>   [...]
> };
> 
> with the same two common fields for other trees, but everything else
> is specific to the struct tree_xxx in question.

Ah, I understand.  I'm in favor of the same thing, although I think
the list of required fields is different.

> > Less! An identifier has no use for the chain, or for most of the flags.
> 
> Are you certain of that?  Nothing anywhere chains identifiers together
> into lists?

Nothing anywhere chains identifiers together into lists.  I know,
because I killed the code that used to use TREE_CHAIN of
IDENTIFIER_NODEs.  It was the bucket chain pointer for the old
non-expandable symbol hash table.

Identifiers really *aren't* trees.  They're bags of language-specific
data hanging off of tokens.  Notice how, throughout the compiler, a
slot in a data structure that carries an IDENTIFIER_NODE never carries
any other kind of tree.  Notice how the only fields of tree_common
that are used by IDENTIFIER_NODEs are flag bits - for decidedly
C-specific purposes, and for information that belongs to a DECL, not
the identifier.

So my plan is to make identifiers not be trees anymore, at all; hash
table entries point to (or contain) just the struct lang_identifier.
Language independent code that sets flag bits in IDENTIFIER_NODEs is
either nuked, or changed to use lookaside tables.  Language dependent
code that does that can put its flags in lang_identifier, or else in
the appropriate DECL node.

This doesn't really affect anything that you do - you're looking more
at expression and constant nodes, it sounds like.

> > There are some counter-pressures against shrinking trees too far.  One is
> > the real danger of running out of tree_code values.  That's an 8-bit field
> > and C++ has more than 200 tree codes.  Another is the demand for flag bits.
> > And there are very few trees that don't need at least a cons-cell worth of
> > pointers; that's eight bytes (on a 32-bit machine) right there.
> 
> What I had in mind wouldn't add any tree_code values, at least I don't
> think so and not initially.
> 
> I'd also like to see us go down the road that rth mentioned,
> i.e. losing all trees with side effects - something else that causes
> no end of special cases to be scattered all over the place.  That
> would get rid of {UN,}SAVE_EXPR and {PRE,POST}{DECREMENT,INCREMENT}
> etc., and some bugs that cause existing outstanding PRs I think.  If
> we are in danger of having more than 256 tree codes, I think that's a
> sign that something is wrong.

Eliminating SAVE_EXPR and friends would be a good idea (kenner's
opinion notwithstanding) but doesn't truly address the problem.  There
are other reasons why we might need more tree codes.  For instance,
languages with lots of intrinsic functions (Fortran 90 comes to mind)
may want to assign each their own tree code so that tree optimizers
can deal with them directly.

There's already a collision between some tree code and the byte value
written by GC object poisoning, which can make debugging harder than
it has to be.

Also, think about padding.  struct { char; tree; } is no smaller than
struct { short; tree; } on any modern host.

zw

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> So my plan is to make identifiers not be trees anymore, at all; hash
> table entries point to (or contain) just the struct lang_identifier.
> Language independent code that sets flag bits in IDENTIFIER_NODEs is
> either nuked, or changed to use lookaside tables.  Language dependent
> code that does that can put its flags in lang_identifier, or else in
> the appropriate DECL node.

Sounds good, I look forward to it 8-) What do you intend to do about
all the code that does expect them to appear in tree slots?  It sounds
like a lot of work.

> Also, think about padding.  struct { char; tree; } is no smaller than
> struct { short; tree; } on any modern host.

? That's why I want e.g.

struct tree_decl
{
  tree chain;   // Maybe struct tree_decl chain;
  enum code;    // Single byte
  [...]		// Decl-specific stuff
};

and there is minimal padding, assuming the tree is laid out
intelligently so other char-ish things like flags come straight after
"code", like now.

Neil.

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> So my plan is to make identifiers not be trees anymore, at all; hash
> table entries point to (or contain) just the struct lang_identifier.
> Language independent code that sets flag bits in IDENTIFIER_NODEs is
> either nuked, or changed to use lookaside tables.  Language dependent
> code that does that can put its flags in lang_identifier, or else in
> the appropriate DECL node.

Sounds good, I look forward to it 8-) What do you intend to do about
all the code that does expect them to appear in tree slots?  It sounds
like a lot of work.

> Also, think about padding.  struct { char; tree; } is no smaller than
> struct { short; tree; } on any modern host.

? That's why I want e.g.

struct tree_decl
{
  tree chain;   // Maybe struct tree_decl chain;
  enum code;    // Single byte
  [...]		// Decl-specific stuff
};

and there is minimal padding, assuming the tree is laid out
intelligently so other char-ish things like flags come straight after
"code", like now.

Neil.

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sun, Nov 25, 2001 at 11:51:13AM +0000, Neil Booth wrote:
> 
> Certainly, I'm not advocating taking them out of the tree union.  I
> suppose what I am advocating, but in different words, is the death of
> tree_common.  We then have
> 
> struct tree_identifier
> {
>   tree chain;
>   ENUM_BITFIELD(tree_code) code:8;
>   [...]
> };
> 
> with the same two common fields for other trees, but everything else
> is specific to the struct tree_xxx in question.

Ah, I understand.  I'm in favor of the same thing, although I think
the list of required fields is different.

> > Less! An identifier has no use for the chain, or for most of the flags.
> 
> Are you certain of that?  Nothing anywhere chains identifiers together
> into lists?

Nothing anywhere chains identifiers together into lists.  I know,
because I killed the code that used to use TREE_CHAIN of
IDENTIFIER_NODEs.  It was the bucket chain pointer for the old
non-expandable symbol hash table.

Identifiers really *aren't* trees.  They're bags of language-specific
data hanging off of tokens.  Notice how, throughout the compiler, a
slot in a data structure that carries an IDENTIFIER_NODE never carries
any other kind of tree.  Notice how the only fields of tree_common
that are used by IDENTIFIER_NODEs are flag bits - for decidedly
C-specific purposes, and for information that belongs to a DECL, not
the identifier.

So my plan is to make identifiers not be trees anymore, at all; hash
table entries point to (or contain) just the struct lang_identifier.
Language independent code that sets flag bits in IDENTIFIER_NODEs is
either nuked, or changed to use lookaside tables.  Language dependent
code that does that can put its flags in lang_identifier, or else in
the appropriate DECL node.

This doesn't really affect anything that you do - you're looking more
at expression and constant nodes, it sounds like.

> > There are some counter-pressures against shrinking trees too far.  One is
> > the real danger of running out of tree_code values.  That's an 8-bit field
> > and C++ has more than 200 tree codes.  Another is the demand for flag bits.
> > And there are very few trees that don't need at least a cons-cell worth of
> > pointers; that's eight bytes (on a 32-bit machine) right there.
> 
> What I had in mind wouldn't add any tree_code values, at least I don't
> think so and not initially.
> 
> I'd also like to see us go down the road that rth mentioned,
> i.e. losing all trees with side effects - something else that causes
> no end of special cases to be scattered all over the place.  That
> would get rid of {UN,}SAVE_EXPR and {PRE,POST}{DECREMENT,INCREMENT}
> etc., and some bugs that cause existing outstanding PRs I think.  If
> we are in danger of having more than 256 tree codes, I think that's a
> sign that something is wrong.

Eliminating SAVE_EXPR and friends would be a good idea (kenner's
opinion notwithstanding) but doesn't truly address the problem.  There
are other reasons why we might need more tree codes.  For instance,
languages with lots of intrinsic functions (Fortran 90 comes to mind)
may want to assign each their own tree code so that tree optimizers
can deal with them directly.

There's already a collision between some tree code and the byte value
written by GC object poisoning, which can make debugging harder than
it has to be.

Also, think about padding.  struct { char; tree; } is no smaller than
struct { short; tree; } on any modern host.

zw

Re: ICE in change_address at emit_rtl.c

[Neil Booth]

Zack Weinberg wrote:-

> I see definite benefits from using independent structures for all
> constants, identifiers, and possibly types.  I don't think it makes
> sense to go as far as separating all the current members of tree_union.
> It is genuinely useful to be able to treat the operands of any expression
> with the same logic, which argues for e.g. leaving declarations as part of
> the tree union (but then, it does the same for constants...)

Certainly, I'm not advocating taking them out of the tree union.  I
suppose what I am advocating, but in different words, is the death of
tree_common.  We then have

struct tree_identifier
{
  tree chain;
  ENUM_BITFIELD(tree_code) code:8;
  [...]
};

with the same two common fields for other trees, but everything else
is specific to the struct tree_xxx in question.

> Less! An identifier has no use for the chain, or for most of the flags.

Are you certain of that?  Nothing anywhere chains identifiers together
into lists?  Since everything else requires the chain (I think), I
imagine it'd be better to keep it so that identifiers remain trees
(assuming that the state of "being a tree" means to have the two
fields above, and GCC code can assume that).  Or do you have another
plan?

> There are some counter-pressures against shrinking trees too far.  One is
> the real danger of running out of tree_code values.  That's an 8-bit field
> and C++ has more than 200 tree codes.  Another is the demand for flag bits.
> And there are very few trees that don't need at least a cons-cell worth of
> pointers; that's eight bytes (on a 32-bit machine) right there.

What I had in mind wouldn't add any tree_code values, at least I don't
think so and not initially.

I'd also like to see us go down the road that rth mentioned,
i.e. losing all trees with side effects - something else that causes
no end of special cases to be scattered all over the place.  That
would get rid of {UN,}SAVE_EXPR and {PRE,POST}{DECREMENT,INCREMENT}
etc., and some bugs that cause existing outstanding PRs I think.  If
we are in danger of having more than 256 tree codes, I think that's a
sign that something is wrong.

Neil.

Re: ICE in change_address at emit_rtl.c

[Mark Mitchell]

--On Saturday, November 24, 2001 11:02:56 PM +0000 Neil Booth 
<neil@daikokuya.demon.co.uk> wrote:

> Gabriel Dos Reis wrote:-
>
>> What worries is the alternative he's proposing.  My personnal view is
>> to have ERROR_MARK_XXX for TYPE, DECL and such in order to enhance
>> the tree type system and maintainability.
>
> Changing tack a little now, but still on the topic of violent changes
> to improve readability 8-)
>
> I've often wondered whether we might do better type-wise to actually
> use separate types, and refer to them as separate types

There is no question that this would be better.  Other major front
ends use this approach, with considerable success.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: ICE in change_address at emit_rtl.c

[Fergus Henderson]

On 24-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> I've often wondered whether we might do better type-wise to actually
> use separate types, and refer to them as separate types, rather than
> having every function take an effectively typeless "tree" as input.
> This alone would help type safety quite a bit.

FWIW I took this approach when writing the Mercury interface to the
GCC "tree" type -- I had separate Mercury types for tree nodes
representing expressions, declarations, types, initializers, etc.
For my purposes this approach worked fairly well.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.

Re: ICE in change_address at emit_rtl.c

[Daniel Berlin]

On Mon, 26 Nov 2001, Fergus Henderson wrote:

> On 24-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> > I've often wondered whether we might do better type-wise to actually
> > use separate types, and refer to them as separate types, rather than
> > having every function take an effectively typeless "tree" as input.
> > This alone would help type safety quite a bit.
>
> FWIW I took this approach when writing the Mercury interface to the
> GCC "tree" type -- I had separate Mercury types for tree nodes
> representing expressions, declarations, types, initializers, etc.
> For my purposes this approach worked fairly well.

FWIW also (the idea of generating the tree structures using one of the
various ast/tree tools around  usually comes up in these discussions )
, over the past year, i've tried to rewrite our tree datatype
using various AST/tree code producerss (IE kimwitu, ASDL, etc. If it's
out there, I've tried it.).
Not only to ease serialization, but because most of these tools also let
you specify tree walkers/etc in nice forms.

They either had limitations that were unacceptable (ASDL's picklers only
handle acyclic structures), generated code that was unacceptable (either
too many support libs required,  not integratable into what we have now
without a *lot* of work, etc), or couldn't handle the complex structure we
have now in an elegant way.

However, today i ran across treecc.
It was written by the portable.net people for their c# compiler, but can
generate readable C/C++/Java/C# code.
Although it doesn't produce code to serialize the structure, the tool is
easily modifable to do so (i can actually understand the source code
easily, unlike some of the other tools).
It also seems to already support most, if not all, of the features we
would need to generate our tree structures (tree structure inheritance,
abstract bases,and fields that are marked as not required to be
passed to the constructor it generates).
It also is easy to write the various walkers in, if you wanted to (though
it's not required).
The resulting c code could stand a little factoring, but that's no biggie
to implement.
It's not too difficult at all to integrate with what we have now (if it
requires any changes at all to start)

In other words, it would be a usable tool for the purposes of generating
the tree structures gcc uses.
First one i've come across.

Just thought i'd point it out,
Dan

 > --
> Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
> The University of Melbourne         |  of excellence is a lethal habit"
> WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
>

Re: ICE in change_address at emit_rtl.c

[Daniel Berlin]

On Mon, 26 Nov 2001, Fergus Henderson wrote:

> On 24-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> > I've often wondered whether we might do better type-wise to actually
> > use separate types, and refer to them as separate types, rather than
> > having every function take an effectively typeless "tree" as input.
> > This alone would help type safety quite a bit.
>
> FWIW I took this approach when writing the Mercury interface to the
> GCC "tree" type -- I had separate Mercury types for tree nodes
> representing expressions, declarations, types, initializers, etc.
> For my purposes this approach worked fairly well.

FWIW also (the idea of generating the tree structures using one of the
various ast/tree tools around  usually comes up in these discussions )
, over the past year, i've tried to rewrite our tree datatype
using various AST/tree code producerss (IE kimwitu, ASDL, etc. If it's
out there, I've tried it.).
Not only to ease serialization, but because most of these tools also let
you specify tree walkers/etc in nice forms.

They either had limitations that were unacceptable (ASDL's picklers only
handle acyclic structures), generated code that was unacceptable (either
too many support libs required,  not integratable into what we have now
without a *lot* of work, etc), or couldn't handle the complex structure we
have now in an elegant way.

However, today i ran across treecc.
It was written by the portable.net people for their c# compiler, but can
generate readable C/C++/Java/C# code.
Although it doesn't produce code to serialize the structure, the tool is
easily modifable to do so (i can actually understand the source code
easily, unlike some of the other tools).
It also seems to already support most, if not all, of the features we
would need to generate our tree structures (tree structure inheritance,
abstract bases,and fields that are marked as not required to be
passed to the constructor it generates).
It also is easy to write the various walkers in, if you wanted to (though
it's not required).
The resulting c code could stand a little factoring, but that's no biggie
to implement.
It's not too difficult at all to integrate with what we have now (if it
requires any changes at all to start)

In other words, it would be a usable tool for the purposes of generating
the tree structures gcc uses.
First one i've come across.

Just thought i'd point it out,
Dan

 > --
> Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
> The University of Melbourne         |  of excellence is a lethal habit"
> WWW: < http://www.cs.mu.oz.au/~fjh >  |     -- the last words of T. S. Garp.
>

Re: ICE in change_address at emit_rtl.c

[Fergus Henderson]

On 24-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> I've often wondered whether we might do better type-wise to actually
> use separate types, and refer to them as separate types, rather than
> having every function take an effectively typeless "tree" as input.
> This alone would help type safety quite a bit.

FWIW I took this approach when writing the Mercury interface to the
GCC "tree" type -- I had separate Mercury types for tree nodes
representing expressions, declarations, types, initializers, etc.
For my purposes this approach worked fairly well.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: < http://www.cs.mu.oz.au/~fjh >  |     -- the last words of T. S. Garp.

Re: ICE in change_address at emit_rtl.c

[Zack Weinberg]

On Sat, Nov 24, 2001 at 11:37:01PM +0100, Gabriel Dos Reis wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> | A solution I have advocated in the past is to have ERROR_MARK_TYPE,
> | ERROR_MARK_DECL, etc.  This would help, since using DECL_ARTIFICIAL,
> | say, on the ERROR_MARK_DECL would work.
> | 
> | Neil's approach is possible as well.
> | 
> | Neil is right that the current strategy actually makes GCC harder
> | to work on, and does little to improve reliability.
> 
> I share Neil's sentiment. 
> 
> What worries is the alternative he's proposing.  My personnal view is
> to have ERROR_MARK_XXX for TYPE, DECL and such in order to enhance
> the tree type system and maintainability.  But Neil is proposing to
> get rid of those nodes.

I think it's fruitless to argue over precisely how we'll make this work
better without sitting down and going through the code, but we should
be able to lay down some design principles - and whatever winds up
fulfilling them, will be the right thing.  So here's what I think we
ought to be going for:

1. Language independent code never sees ill-formed trees.  The front end
is responsible for replacing erroneous input with harmless constructs.
"Harmless" means "won't cause anything downstream to crash, and needs
no special casing."  How exactly we implement that is open to discussion,
but answers to the remaining questions may affect this.

1a. Within language dependent code, ill-formed trees should have as short
lifetimes as possible.  For instance, the code which generates statement-
level trees should be able to rely on the expression parser to produce only
well-formed expressions.

2. As few spurious errors reported to the user as possible.  This is one
place where ERROR_MARK_NODE really falls down.  Consider these ill-formed
declarations:

double int number;
typedef struct foo foo_t[;

These will both provoke error cascades in the current compiler, mainly
because 'number' and 'foo_t' don't get entered into the symbol table,
so we issue errors every time they are used, even if the use would have
been valid given a valid declaration.  What we need here is a way of saying,
well, we know "number" is a scalar non-pointer variable so we'll let the user
do things with it that would be acceptable of such variables.  It's harder to
say what foo_t is supposed to be - but we can say that a forward declaration
of struct foo has happened, and it will probably be okay to treat foo_t
as a typedef for struct foo.

We also need to do better recovering from parser errors - here I think the
additional power of recursive descent should eliminate the problem, and we
don't need any funky data structures.  But take that typedef as an example
of what needs to be handled - there is no construct which contains a
semicolon inside square brackets (without further nesting) so we should
_not_ parse the rest of the file as if we are inside square brackets, which
is (very close to) what happens right now.

3. Make efforts to get back in sync and continue processing the rest of the
file.  At the very least, errors in one top level construct should not
propagate outside that construct.  We may, of course, have to prevent
later operations from digging too deeply inside that construct.  For instance,

inline int foo(void) { syntax error; }

should become "extern int foo(void);" for purpose of further processing.

zw

Re: ICE in change_address at emit_rtl.c

[Fergus Henderson]

On 23-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> Gabriel Dos Reis wrote:-
> > Neil Booth <neil@daikokuya.demon.co.uk> writes:
> > 
> > | I've not been able to find a satisfactory fix; since I'm not a fan of
> > | error_mark_node-s I don't want to introduce yet more checks for them.
> > | I hate the error_mark_node stuff; it would be nice if it could go
> > | away.
> > 
> > What should be the alternative?
> 
> Generate something that is valid, even if it's not what the user
> specified.  e.g. if it's an invalid typedef, typedef it to int (or
> something more intelligent if possible) rather than error_mark_node.

But that will cause spurious flow-on errors when the
type is used in some way for which `int' is not valid.

The only way to avoid all such spurious flow-on errors is to have a type
(or expression, declaration, etc.) for which *all* operations are valid.

And if you have such a type (or expression, declaration, etc.), then
whatever you end up calling it, you will have reinvented something
very similar to the error_mark_node approach.

The main way in which it could usefully vary, IMHO, is that you might
want to have different nodes for error_mark_type, error_mark_expr,
error_mark_decl, etc.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.

Re: ICE in change_address at emit_rtl.c

[Fergus Henderson]

On 23-Nov-2001, Neil Booth <neil@daikokuya.demon.co.uk> wrote:
> Gabriel Dos Reis wrote:-
> > Neil Booth <neil@daikokuya.demon.co.uk> writes:
> > 
> > | I've not been able to find a satisfactory fix; since I'm not a fan of
> > | error_mark_node-s I don't want to introduce yet more checks for them.
> > | I hate the error_mark_node stuff; it would be nice if it could go
> > | away.
> > 
> > What should be the alternative?
> 
> Generate something that is valid, even if it's not what the user
> specified.  e.g. if it's an invalid typedef, typedef it to int (or
> something more intelligent if possible) rather than error_mark_node.

But that will cause spurious flow-on errors when the
type is used in some way for which `int' is not valid.

The only way to avoid all such spurious flow-on errors is to have a type
(or expression, declaration, etc.) for which *all* operations are valid.

And if you have such a type (or expression, declaration, etc.), then
whatever you end up calling it, you will have reinvented something
very similar to the error_mark_node approach.

The main way in which it could usefully vary, IMHO, is that you might
want to have different nodes for error_mark_type, error_mark_expr,
error_mark_decl, etc.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: < http://www.cs.mu.oz.au/~fjh >  |     -- the last words of T. S. Garp.