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.

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.

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:

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

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

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 :slight_smile: 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.

Hi Victor,

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.

what if the declaration with the name "malloc" returns nothing or a
struct or an array or something like that? LLVM shouldn't fall over
in a heap.

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

Duncan,