This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch for Bug No-8081


"Sitikant Sahu, Noida" <sitikants@noida.hcltech.com> writes:

>> Now, looking at your patch, I do not believe it is correct.
>> Allocating space in the callee means that it will get deallocated
>> immediately, when the callee returns, and may then be trashed by
>> further stack operations in the caller.  It's true that the caller
>> failed to provide space because the return value is not used, 
>> but that
>> may not be the only way to get a null target at this point, and you
>> would then have turned an ICE into a silent miscompilation.
>
> The space is allocated in the caller. The expand_call () is being 
> processed for the caller. The values within the returned block can be
> accessed if we write something like
>     func ().val[i]

Have you verified that you get a null target in expand_call for this
construct too?  Your test case doesn't do that.

> Am I missing something, if thats not the case ?

I misunderstood the code.  This is indeed being executed in the
caller.  So that isn't a problem, and I agree that allocating dynamic
stack space at this point is the right thing to do in the abstract.

However, I think there's still a problem here - there wasn't anything
wrong with the code you replaced, it was just being run in the wrong
set of circumstances.  What the old comment is trying to say is, "This
block of code only works for fixed size structure types.  This is fine
because we can't ever get here with a variable sized type because our
caller should have given us a target."  Which, it turns out, is not true.

So what you actually want to do is

        ...
        else if (target && GET_CODE (target) == MEM)
          structure_value_addr = XEXP (target, 0);
        else if (variably_modified_type_p (TREE_TYPE (exp))
          /* your new code here */
        else
          /* old code here */

The reason this matters is, using assign_stack_temp when it'll work
(i.e. for a fixed size structure) produces much better code than
always using allocate_dynamic_stack_space.

> /* There was an assumption here that for variable-sized objects,
>    we must be called with a target specified. The assumption is
>    not valid when we call a  nested function  returning an  
>    variable size object, not used after this call. 
>    So we must allocate return value dynamically.  */

In comments, you should not talk about bogus assumptions made by code
that no longer exists.  Comments should document the code as it is
now.  I suggest you say only

 /* If the return type is variably modified and we have no target to
    initialize, we must use allocate_dynamic_stack_space to allocate a
    temporary.  It becomes inaccessible at the end of the surrounding
    expression, so its lifetime does not matter (it will be at least
    that long).  */

Except, is that statement about its lifetime true?  There are a number
of constructs that could be used to extend the life of the object,
such as 

  int *p = &func().val[i];

Bleah ill-thought-out extensions...

zw


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]