[llvm-commits] [llvm] r122959 - in /llvm/trunk: lib/Target/README.txt lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/objsize.ll

[Chris Lattner]

On Jan 8, 2011, at 1:12 AM, Frits van Bommel wrote:

> On Sat, Jan 8, 2011 at 7:52 AM, Chris Lattner <clattner at apple.com> wrote:
>> 
>> On Jan 7, 2011, at 12:49 PM, Benjamin Kramer wrote:
>>> I reverted the change for now, there is another issue with this as it changes the semantics of __builtin_objectsize.
>>> Unless we want to extend this GCC extension even more my patch is not safe in it's current form :(
>> 
>> What exactly is the issue here?  While I'm sympathetic to the "cdecl.c/ds" regression, I still think that your patch is a good route forward.  Optimizing to "know" the strlen size seems like a nice second step.
>> 
>> However, if your patch wasn't safe, then it clearly isn't good.  What is the issue?
> 
> The safety concern he refers to is that it changes the semantics of
> llvm.objectsize. LangRef.html states:
> "The llvm.objectsize intrinsic is lowered to either a constant
> representing the size of the object concerned, or i32/i64 -1 or 0,
> depending on the type argument, if the size cannot be determined at
> compile time."
> This patch would instead replace it with a non-constant value.

Ok, I think that it is just over-specified.  The result of this gets passed into memset_chk etc as a dynamic parameter.  Passing in a variable is just fine, and is actually good for safety (an abort will happen if the dynamic size is less than the dynamic # bytes to copy). 

> It might therefore not be unreasonable to replace it with a
> non-constant value if this is *known* to significantly simplify all
> uses, for instance by requiring that they all simplify to constants
> (or in the case of terminators, to an unconditional branch or
> unreachable).
> This would still require adjusting the semantics first, though, as
> it's currently not legal to do any of that.
> 
> P.S. Is it safe to return a constant malloc argument anyway? malloc()
> might return null, in which case the only safe-to-use size would be
> 0...

Good point.  I'd say "no, it's not" unless there is something else in the code that tells you that the result is assumed to not be null.  For example a store through the returned pointer or a dynamic null check.

-Chris