Re: [PATCH] warn on returning alloca and VLA (PR 71924, 90549)

[Jeff Law <law@redhat.com>]

On 6/30/19 3:50 PM, Martin Sebor wrote:
> On 6/26/19 6:11 PM, Jeff Law wrote:
[ Another big snip ]
> 
>> Is there a strong need for an overloaded operator?  Our guidelines
>> generally discourage operator overloads.  This one seems on the border
>> to me.  Others may have different ideas where the line is.  If we really
>> don't need the overloaded operator, then we can avoid the controversy
>> completely.
> 
> The copy assignment operator is necessary for the type to be stored
> in a hash_map.  Otherwise, the implicitly generated assignment will
> be used instead which is unsafe in vec and results in memory
> corription (I opened bug 90904 for this).  After working around this
> bug by providing the assignment operator I then ran into the hash_map
> bug 90959 that also results in memory corruption.  I spent a few fun
> hours tracking down the GCC miscompilations that they led to.
OK.  THanks for explaining why we need the copy assignment operator.


> 
> The golden rule in C++ is that every type should either be safe to
> copy and assign with the expected semantics or made not assignable
> and not copyable by declaring the copy ctor and copy assignment
> operator private (or deleted in more modern C++).  It would be very
> helpful to detect this kind of problem (classes that aren't safely
> assignable and copyable because they acquire/release resources in
> ctors/dtors).  Not just to us in GCC but I'm sure to others as well.
> It's something I've been hoping to implement for a while now.
You've got my blessing to do that :-)

>>
>> So how do you deal with the case where one operand of a
>> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not?  It
>> seems like we'll end up returning true from this function in that case
>> and potentially trigger changing the return value to NULL.  Or am I
>> missing something?
> 
> I only briefly considered MIN/MAX but because in C and C++ the less
> than and greater than expressions are only defined for pointers into
> the same array/object or subobject and I didn't ponder it too hard.
> (In C they're undefined otherwise, in C++ the result is unspecified.)
> 
> But you bring it up because you think the address should be returned
> regardless, even if the expression is invalid (or if it's COND_EXPR
> with mixed local/nonlocal addresses) so I've changed it to do that
> for all these explicitly handled codes and added tests to verify it.
THanks.

So in the thread for strlen/sprintf the issue around unbounded walks up
through PHI argument's SSA_NAME_DEF_STMT was raised.

I think we've got two options here.

One would be to hold this patch until we sort out what the bounds should
look like, then adjust this patch to do something similar.

Or go forward with this patch, then come back and adjust the walker once
we have settled on solution in the other thread.

I'm OK with either.  Your choice.

jeff