llvm dev - Sep 2009 - [LLVMdev] Verifier should not make any assumptions about calls to "malloc"

Hi Victor, this code from the verifier broke the Ada front-end build:

   const Module* M = CI.getParent()->getParent()->getParent();
   Constant *MallocFunc = M->getFunction("malloc");

   if (CI.getOperand(0) == MallocFunc) {
     const PointerType *PTy  
PointerType::getUnqual(Type::getInt8Ty(CI.getParent()->getContext()));
     Assert1(CI.getType() == PTy, "Malloc call must return i8*",
&CI);
   }

I think it is completely wrong for the verifier to be checking anything
about calls to functions that happen to be called "malloc".  What if I
have my own runtime in which "malloc" is completely different to the
usual one?  From my reading of the gcc docs, malloc is not provided
in a freestanding environment and thus cannot be assumed to be the
normal malloc.

I think this code should be removed from the verifier.  Instead,
isMalloc should also check the number of parameters and their types,
as well as the return value.

Actually isMalloc also seems bogus.  In a freestanding environment
there is no reason that a function that happens to be called "malloc"
should have anything to do with memory allocation.  Do you have a
plan to handle this?  Shouldn't all malloc manipulations be done from
SimplifyLibcalls?

Ciao,

Duncan.

On Sep 22, 2009, at 6:11 AM, Duncan Sands wrote:
> Hi Victor, this code from the verifier broke the Ada front-end build:
>
>   const Module* M = CI.getParent()->getParent()->getParent();
>   Constant *MallocFunc = M->getFunction("malloc");
>
>   if (CI.getOperand(0) == MallocFunc) {
>     const PointerType *PTy >
> PointerType::getUnqual(Type::getInt8Ty(CI.getParent()->getContext()));
>     Assert1(CI.getType() == PTy, "Malloc call must return i8*",
&CI);
>   }
Yes, I agree, the verifier shouldn't worry about malloc calls.
> Actually isMalloc also seems bogus.  In a freestanding environment
> there is no reason that a function that happens to be called
"malloc"
> should have anything to do with memory allocation.  Do you have a
> plan to handle this?  Shouldn't all malloc manipulations be done from
> SimplifyLibcalls?
We violate this in other places. If someone is worried about  
freestanding environments and -fno-builtin-malloc, we should add a new  
function attribute "not builtin" and have the optimizer respect that.

-Chris

Duncan,

Thanks for brining the Ada issue to my attention.

On Sep 22, 2009, at 6:11 AM, Duncan Sands wrote:
> Hi Victor, this code from the verifier broke the Ada front-end build:
>
>  const Module* M = CI.getParent()->getParent()->getParent();
>  Constant *MallocFunc = M->getFunction("malloc");
>
>  if (CI.getOperand(0) == MallocFunc) {
>    const PointerType *PTy >
PointerType::getUnqual(Type::getInt8Ty(CI.getParent()->getContext()));
>    Assert1(CI.getType() == PTy, "Malloc call must return i8*",
&CI);
>  }
>
> I think it is completely wrong for the verifier to be checking  
> anything
> about calls to functions that happen to be called "malloc".  What
if I
> have my own runtime in which "malloc" is completely different to
the
> usual one?  From my reading of the gcc docs, malloc is not provided
> in a freestanding environment and thus cannot be assumed to be the
> normal malloc.
What does the Ada front-end declare malloc as?
>
> I think this code should be removed from the verifier.  Instead,
> isMalloc should also check the number of parameters and their types,
> as well as the return value.
The verifier code is needed because when a malloc return value is used  
directly, not via a bitcast, the type of the malloc is determined to  
be i8*.  But that could be updated to use the declared return type of  
malloc.  I need to understand more about how this is breaking Ada to  
determine how to resolve this.  Removing this check from the verifier  
could end up being the resolution.
>
> Actually isMalloc also seems bogus.  In a freestanding environment
> there is no reason that a function that happens to be called
"malloc"
> should have anything to do with memory allocation.  Do you have a
> plan to handle this?  Shouldn't all malloc manipulations be done from
> SimplifyLibcalls?
The free-standing environment with a non-memory-allocating malloc  
function will be handled via -fno-builtins.  I don't believe that  
isMalloc is bogus, just that llvm does not support the ability to turn  
it off.  Here is some previous discussion about this topic:

On Sep 11, 2009, at 11:30 AM, Chris Lattner wrote:
> On Sep 11, 2009, at 10:22 AM, Dan Gohman wrote:
>
>> If MallocHelper is going to replace MallocInst, how will it support
>> -fno-builtins?  With MallocInst, one at least had the option of
>> omitting the RaiseAllocations pass (though it seems llvm-gcc and
>> clang don't actually do this, which is a bug). With MallocHelper,
>> how can -fno-builtins be implemented?
>
> In order to support -fno-builtin-foo, we'd want to introduce a  
> function attribute saying "not a builtin" and stick it on foo. 
This
> doesn't exist today, but shouldn't be particularly hard.

Victor
>
> Ciao,
>
> Duncan.

On Sep 22, 2009, at 11:23 AM, Victor Hernandez wrote:
> On Sep 22, 2009, at 6:11 AM, Duncan Sands wrote:
>
>> Hi Victor, this code from the verifier broke the Ada front-end build:
>>
>> const Module* M = CI.getParent()->getParent()->getParent();
>> Constant *MallocFunc = M->getFunction("malloc");
>>
>> if (CI.getOperand(0) == MallocFunc) {
>>   const PointerType *PTy >>
PointerType::getUnqual(Type::getInt8Ty(CI.getParent()->getContext
>> ()));
>>   Assert1(CI.getType() == PTy, "Malloc call must return i8*",
&CI);
>> }
>>
>> I think it is completely wrong for the verifier to be checking
>> anything
>> about calls to functions that happen to be called "malloc". 
What
>> if I
>> have my own runtime in which "malloc" is completely different
to the
>> usual one?  From my reading of the gcc docs, malloc is not provided
>> in a freestanding environment and thus cannot be assumed to be the
>> normal malloc.
>
> What does the Ada front-end declare malloc as?
>
>>
>> I think this code should be removed from the verifier.  Instead,
>> isMalloc should also check the number of parameters and their types,
>> as well as the return value.
>
> The verifier code is needed because when a malloc return value is used
> directly, not via a bitcast, the type of the malloc is determined to
> be i8*.  But that could be updated to use the declared return type of
> malloc.  I need to understand more about how this is breaking Ada to
> determine how to resolve this.  Removing this check from the verifier
> could end up being the resolution.
Just looked at getMallocType() and it does not assume that the return  
type of a malloc call is i8*, so I have removed this check from the  
verifier.

Victor
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090922/6baa52c9/attachment.html>

Hi Victor,
> What does the Ada front-end declare malloc as?
I don't really want to tell you because a correct solution should work
no matter what malloc is defined to be :)  What I mean by "work" is
that
if malloc has the standard prototype then you perform transforms on it,
and otherwise you should probably just ignore it.

That said, Ada outputs malloc as: i32 @malloc(i32)
I'm perfectly happy for this not to be optimized - if the Ada
frontend wants malloc to be optimized I think it is reasonable
to require it to define malloc in a more conventional way.
>> I think this code should be removed from the verifier.  Instead,
>> isMalloc should also check the number of parameters and their types,
>> as well as the return value.
> 
> The verifier code is needed because when a malloc return value is used 
> directly, not via a bitcast, the type of the malloc is determined to be 
> i8*.  But that could be updated to use the declared return type of 
> malloc.  I need to understand more about how this is breaking Ada to 
> determine how to resolve this.  Removing this check from the verifier 
> could end up being the resolution.
I think you should just not even try to do transforms on a function that
happens to be called "malloc" if it doesn't have the conventional
prototype.  I reckon isMalloc should just return false for something
called "malloc" but that doesn't return i8* or has too many
parameters
etc.
>> Actually isMalloc also seems bogus.  In a freestanding environment
>> there is no reason that a function that happens to be called
"malloc"
>> should have anything to do with memory allocation.  Do you have a
>> plan to handle this?  Shouldn't all malloc manipulations be done
from
>> SimplifyLibcalls?
> 
> The free-standing environment with a non-memory-allocating malloc 
> function will be handled via -fno-builtins.  I don't believe that 
> isMalloc is bogus, just that llvm does not support the ability to turn 
> it off.
Yes, isMalloc is not the only sinner here - fixing this would be a
whole new project.

Ciao,

Duncan.

Chris Lattner wrote:
> Yes, I agree, the verifier shouldn't worry about malloc calls.
> 
>> Actually isMalloc also seems bogus.  In a freestanding environment
>> there is no reason that a function that happens to be called
"malloc"
>> should have anything to do with memory allocation.  Do you have a
>> plan to handle this?  Shouldn't all malloc manipulations be done
from
>> SimplifyLibcalls?
> 
> We violate this in other places. If someone is worried about  
> freestanding environments and -fno-builtin-malloc, we should add a new  
> function attribute "not builtin" and have the optimizer respect
that.
I'm worried about this, too. Pure does its own extern declarations which
do not line up with malloc() returning int8* either. That's because I
need to distinguish between void* and char* in my frontend, and the
easiest way to do that is to create void* as a different type (it's a
pointer to an empty struct in Pure IIRC). As a consequence, Pure doesn't
work with the assumption that malloc() returns int8*. Now I could
probably work around this in some way, but that's very awkward.

Albert

-- 
Dr. Albert Gr"af
Dept. of Music-Informatics, University of Mainz, Germany
Email:  Dr.Graef at t-online.de, ag at muwiinfa.geschichte.uni-mainz.de
WWW:    http://www.musikinformatik.uni-mainz.de/ag