[Bug c/27445] New: create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[Bug c/27445] New: create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

(see also: http://gcc.gnu.org/ml/gcc/2006-05/msg00158.html)

While following GCC's handling of 'volatile' and other type
qualifiers, I noticed that the gimplify pass created temporaries
with a type with 'volatile' asserted if the underlying type also
had 'volatile' asserted.

Temporaries are created by the create_tmp_var_raw() procedure
in gimplify.c, which reads as follows:

tree
create_tmp_var_raw (tree type, const char *prefix)
{
  tree tmp_var;
  tree new_type;

  /* Make the type of the variable writable.  */
  new_type = build_type_variant (type, 0, 0);
  TYPE_ATTRIBUTES (new_type) = TYPE_ATTRIBUTES (type);

  tmp_var = build_decl (VAR_DECL, prefix ? create_tmp_var_name (prefix) :
NULL,
                        type);
[...]

Note above that an unqualified type, new_type, is created but
then subsequently not used in the call to build_decl.  Because of
this omission, if 'type' originally had any qualifiers set
(such as volatile), they'll be propagated to the temporary, which
might have some unexpected effects on subsequent optimizations
and code generation.

The fix, I think, is to pass 'new_type'.


-- 
           Summary: create_tmp_var_raw (gimplify.c) inadventently asserts
                    'volatile' on temps
           Product: gcc
           Version: 4.0.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: gary at intrepid dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug c/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2006-05-05 16:04 -------
Do you have a testcase this will better understand the problem and to see if
your patch is correct?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug c/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #2 from gary at intrepid dot com  2006-05-05 16:06 -------
Created an attachment (id=11382)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=11382&action=view)
create_tmp_var_raw patch: remove type qualifiers

This patch compiles, but hasn't been tested.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug c/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #3 from gary at intrepid dot com  2006-05-05 16:12 -------
(In reply to comment #1)
> Do you have a testcase this will better understand the problem and to see if
> your patch is correct?

I'm not sure how to demonstrate that there is a problem.  I think it
is clear that the author of the function planned on using new_type
instaed of type, and that it is incorrect to assert 'volatile' on
a compiler temporary, but am uncertain as to whether asserting
'volatile' leads to any particular difficulties in the present
compiler.  I'm hoping one of the gimplify developers can help
construct a test case, if applicable.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[pinskia at gcc dot gnu dot org]

------- Comment #4 from pinskia at gcc dot gnu dot org  2006-05-05 16:16 -------
Then the real question is why do you think this is a bug?


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |middle-end


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #5 from gary at intrepid dot com  2006-05-05 16:37 -------
(In reply to comment #4)
> Then the real question is why do you think this is a bug?

1. it is a bug to create temporaries and assert 'volatile' on them
2. there is code in create_tmp_var_raw() that creates a type with
qualifiers removed, but this new_type is then not used at the
moment.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

Re: [Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[Andrew Pinski]

> 
> 
> 
> ------- Comment #5 from gary at intrepid dot com  2006-05-05 16:37 -------
> (In reply to comment #4)
> > Then the real question is why do you think this is a bug?
> 
> 1. it is a bug to create temporaries and assert 'volatile' on them

Why do you say that?

> 2. there is code in create_tmp_var_raw() that creates a type with
> qualifiers removed, but this new_type is then not used at the
> moment.

I don't think create_tmp_var_raw should remove the quals.

-- Pinski

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[pinskia at physics dot uc dot edu]

------- Comment #6 from pinskia at physics dot uc dot edu  2006-05-05 16:39 -------
Subject: Re:  create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile'
on temps

> 
> 
> 
> ------- Comment #5 from gary at intrepid dot com  2006-05-05 16:37 -------
> (In reply to comment #4)
> > Then the real question is why do you think this is a bug?
> 
> 1. it is a bug to create temporaries and assert 'volatile' on them

Why do you say that?

> 2. there is code in create_tmp_var_raw() that creates a type with
> qualifiers removed, but this new_type is then not used at the
> moment.

I don't think create_tmp_var_raw should remove the quals.

-- Pinski


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #7 from gary at intrepid dot com  2006-05-05 16:58 -------
Consider the following:

volatile int j;
void p()
{
  ++j;
}

Gimplify in its present form might transform the statement above into
something like the following:

volatile int j;
void p()
{
  volatile int T1;
  T1 = j + 1;
  j = T1;
}

Here, gimplify has created a local temporary but has tagged it as
volatile, because j is volatile.

Taken literally, (j + 1) must be evaluated, and its value stored
into T1, and T1 must be a _memory_ location, not a register,
which looks like this on an x86:

        movl    j, %eax
        addl    $1, %eax
        movl    %eax, -4(%ebp)
        movl    -4(%ebp), %eax
        movl    %eax, j

Now as it turns out, somehow gimplify and the follow-on optimization
passes don't seem to target T1 into a memory location, probably because
they "know" that temporary variables for scalars should live in registers.
But still the compiler does create a temporary like T1 above and
qualifies T1 as volatile.  The fact that subsequent passes seem to
ignore the qualifier is logically inconsistent, and at some level
incorrect.  Will they always ignore that qualifier?  Who can say?

Note also that at present "const" and "restricted" (on pointers)
can also be specified.  Can't say if applying them to temporaries
causes any real harm, but again, it is logically inconsistent.
If we believe that gimplify should create trees that are well-formed
unambiguous and that precisely describe the semantics of the program
that is being translated, then I think the type qualifiers should be
dropped when declaring compiler temporaries.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[pinskia at gcc dot gnu dot org]

------- Comment #8 from pinskia at gcc dot gnu dot org  2006-05-05 17:10 -------
(In reply to comment #7)
> Consider the following:
> 
> volatile int j;
> void p()
> {
>   ++j;
> }
> 
> Gimplify in its present form might transform the statement above into
> something like the following:

It will never get the wrong type. because create_tmp_from_val uses
TYPE_MAIN_VARIANT.

Your issue with ++j not using incr is not related to the gimplifier at all as
it also happens in every compiler version since at least 2.95.3.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #9 from gary at intrepid dot com  2006-05-05 17:55 -------
> Your issue with ++j not using incr is not related to the gimplifier at all as
> it also happens in every compiler version since at least 2.95.3.

I agree that is unrelated.  I didn't file this bug thinking that it would help
"fix" the "incr issue".  In fact, I don't really have an "incr issue" -- it
was just something that came up while I was writing some tests.

> > Gimplify in its present form might transform the statement above into
> > something like the following:
[...]
> It will never get the wrong type. because create_tmp_from_val uses
> TYPE_MAIN_VARIANT.

Well, I originally noticed the problem in 4.0.1, which has the following
code:

static inline tree
create_tmp_from_val (tree val)
{
  return create_tmp_var (TREE_TYPE (val), get_name (val));
}

The dev tree now how this code:

static inline tree
create_tmp_from_val (tree val)
{
  return create_tmp_var (TYPE_MAIN_VARIANT (TREE_TYPE (val)), get_name (val));
}

I haven't looked into the rev. history, to see why/when this fix was made,
but will ask the hypothetical: was this fix made to workaround the
misbehavior in create_tmp_var_raw()?  Note that create_tmp_var_raw()
is exported from gimplify.c and appears to be called from quite a few
places.  The question arises: what are the preconditions for calling
create_tmp_var_raw()?  If you want to assert that it uses whatever
type was passed in and all the callers have to remove qualifiers
as necessary that's fine, but requires some knowledge of the original
intent behind create_tmp_var_raw() and the assumptions its callers make.
I'd be temtpted to add an assert that the type passed in has no qualifiers
if that is a pre-condition.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

Re: [Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[Daniel Berlin]

> I haven't looked into the rev. history, to see why/when this fix was made,
> but will ask the hypothetical: was this fix made to workaround the
> misbehavior in create_tmp_var_raw()?  Note that create_tmp_var_raw()
> is exported from gimplify.c and appears to be called from quite a few
> places.  The question arises: what are the preconditions for calling
> create_tmp_var_raw()?  If you want to assert that it uses whatever
> type was passed in and all the callers have to remove qualifiers
> as necessary that's fine, but requires some knowledge of the original
> intent behind create_tmp_var_raw() and the assumptions its callers make.
> I'd be temtpted to add an assert that the type passed in has no qualifiers
> if that is a pre-condition.
> 
Compiler temporaries we generate explicitly, have the same qualifiers as
the expression they are generated from.  This is by design.

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[dberlin at dberlin dot org]

------- Comment #10 from dberlin at gcc dot gnu dot org  2006-05-05 18:06 -------
Subject: Re:  create_tmp_var_raw (gimplify.c)
        inadventently asserts 'volatile' on temps


> I haven't looked into the rev. history, to see why/when this fix was made,
> but will ask the hypothetical: was this fix made to workaround the
> misbehavior in create_tmp_var_raw()?  Note that create_tmp_var_raw()
> is exported from gimplify.c and appears to be called from quite a few
> places.  The question arises: what are the preconditions for calling
> create_tmp_var_raw()?  If you want to assert that it uses whatever
> type was passed in and all the callers have to remove qualifiers
> as necessary that's fine, but requires some knowledge of the original
> intent behind create_tmp_var_raw() and the assumptions its callers make.
> I'd be temtpted to add an assert that the type passed in has no qualifiers
> if that is a pre-condition.
> 
Compiler temporaries we generate explicitly, have the same qualifiers as
the expression they are generated from.  This is by design.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[rth at gcc dot gnu dot org]

------- Comment #11 from rth at gcc dot gnu dot org  2006-05-06 01:17 -------
(In reply to comment #9)
> I haven't looked into the rev. history, to see why/when this fix was made,
> but will ask the hypothetical: was this fix made to workaround the
> misbehavior in create_tmp_var_raw()?

One could perhaps argue this, but I would expect that a function named
"raw" would not do anything in the way of preprocessing.  That it would
create a variable of exactly the type given.

I'm comfortable leaving the volatile removal in create_tmp_from_val; 
that's where the bulk of our temporaries come from anyway.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445

[Bug middle-end/27445] create_tmp_var_raw (gimplify.c) inadventently asserts 'volatile' on temps

[gary at intrepid dot com]

------- Comment #12 from gary at intrepid dot com  2006-05-06 01:42 -------
Given the above, I suggest that my bug report be marked closed,
because the problem I was seeing in 4.0.1 is fixed by the
change to create_tmp_from_val that passes in TYPE_MAIN_VARIANT().
Note that the use of the main variant is stronger (removes more
qualifiers/attirbutes) than the code that sets new_type anyway.

It might be a good idea to remove the setting of new_type since
it isn't used.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27445