[llvm] r340807 - Fix in getAllocationDataForFunction

Wed Aug 29 12:11:19 PDT 2018

Sorry, I think you are correct.  Please revert my change.	

-----Original Message-----
From: Kaylor, Andrew <andrew.kaylor at intel.com> 
Sent: 29 August 2018 19:49
To: Matthew Parkinson <mattpark at microsoft.com>; David Chisnall <David.Chisnall at cl.cam.ac.uk>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: RE: [llvm] r340807 - Fix in getAllocationDataForFunction

> The original test
>  `if ((FnData->AllocTy & AllocTy) != AllocTy) return None` Will return None for everything. Nothing has all those bits set.

But that wasn't the original test. The original test was

if ((FnData->AllocTy & AllocTy) != FnData->AllocTy)

So it's testing that all of the bits in FnData->AllocTy (the alloc type in the table for the function) are set in AllocTy (the query mask), not the reverse. If you call getAllocationDataForFunction with MallocOrCallocLike, then the original test will match for MallocLike or CallocLike.

> Andy, I think the issue for your code is that you want a
> 
> enum AllocType : uint8_t {
>   OpNewLike          = 1<<0, // allocates; never returns null
>   MallocLike           = 1<<1,
>   MallocOrNewLike         = MallocLike  | OpNewLike,
>   ...
> };

I don't think that's right. There are entries in the table in MemoryBuiltins.cpp that describe non-throwing operator new as MallocLike. It seems clear from this that these should match if I call getAllocationDataForFunction with OpNewLike.  That is, an operator new which may return null is MallocLike but it is also OpNewLike. At least, that's what the table and the current enum seem to be saying.

-Andy

-----Original Message-----
From: Matthew Parkinson [mailto:mattpark at microsoft.com]
Sent: Wednesday, August 29, 2018 2:23 AM
To: David Chisnall <David.Chisnall at cl.cam.ac.uk>; Kaylor, Andrew <andrew.kaylor at intel.com>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: RE: [llvm] r340807 - Fix in getAllocationDataForFunction

So we were seeing issues with requesting something to be 
   MallocOrCallocLike

So if you query 
   getAllocationDataForFunction
with 
   MallocOrCallocLike

Then
  `if ((FnData->AllocTy & AllocTy) == 0) return None` The current fix, will return None, for anything that doesn't have one of the bits set. So matches lots of stuff.

The original test
  `if ((FnData->AllocTy & AllocTy) != AllocTy) return None` Will return None for everything. Nothing has all those bits set.

Andy, I think the issue for your code is that you want a 

enum AllocType : uint8_t {
  OpNewLike          = 1<<0, // allocates; never returns null
  MallocLike           = 1<<1,
  MallocOrNewLike         = MallocLike  | OpNewLike,
  ...
};

Sorry if I have misunderstood something.

Matt

-----Original Message-----
From: David Chisnall <dc552 at hermes.cam.ac.uk> On Behalf Of David Chisnall
Sent: 28 August 2018 18:05
To: Kaylor, Andrew <andrew.kaylor at intel.com>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r340807 - Fix in getAllocationDataForFunction

Please revert it if it’s causing problems for you, and reopen the review.  We’re seeing false negatives with the version from before the change, which is breaking memory safety analyses.  It may be that we need to add a second API to handle this correctly.

David

> On 28 Aug 2018, at 17:46, Kaylor, Andrew <andrew.kaylor at intel.com> wrote:
> 
> I believe this change is incorrect. It causes us to report that malloc is a new-like function.
> 
> The way that the allocation kinds are defined, each allocation kind has a unique bit except for malloc-like which shares a bit with new-like. With the old implementation, this allowed us to recognize new-like functions as also being malloc-like, but not the other way around. The other combination masks in the AllocType enum allow for matching any of multiple types, but no function will be defined as being, for instance, MallocOrCallocLike.
> 
> So the old logic in getAllocationDataForFunction was applying the incoming AllocTy as a mask, and if the FnData->AllocTy was unchanged after applying that mask, then it matched.  That is, if all of the bits in the FnData->AllocTy were in the callers mask, then it matched. Otherwise, it did not. I think that's the behavior we want.
> 
> Can this change be reverted?
> 
> -Andy
> 
> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On 
> Behalf Of David Chisnall via llvm-commits
> Sent: Tuesday, August 28, 2018 1:59 AM
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r340807 - Fix in getAllocationDataForFunction
> 
> Author: theraven
> Date: Tue Aug 28 01:59:06 2018
> New Revision: 340807
> 
> URL: 
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%3Frev%3D340807%26view%3Drev&data=02%7C0
> 1%7Cmattpark%40microsoft.com%7C022b5d243e5d40b8517d08d60d089ae4%7C72f9
> 88bf86f141af91ab2d7cd011db47%7C1%7C0%7C636710727960849332&sdata=wS
> QOx0KWrMSL0MHwpCoASEVC2keN5TajZJpeCe%2FC95U%3D&reserved=0
> Log:
> Fix in getAllocationDataForFunction
> 
> Summary:
> Correct to use set like behaviour of AllocType.  Should check for subset, not precise value.
> 
> Reviewers: theraven
> 
> Reviewed By: theraven
> 
> Subscribers: hiraditya, llvm-commits
> 
> Differential Revision: 
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Frevie
> ws.llvm.org%2FD50959&data=02%7C01%7Cmattpark%40microsoft.com%7C022
> b5d243e5d40b8517d08d60d089ae4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
> C0%7C636710727960849332&sdata=%2FpJHJlER6z%2FvPxSWyE9jE%2BvOEpZ5Mh
> DKK2rp0p4QwwY%3D&reserved=0
> 
> Modified:
>    llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
> 
> Modified: llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
> URL: 
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Fllvm%2Ftrunk%2Flib%2FAnalysis%2FMemoryBui
> ltins.cpp%3Frev%3D340807%26r1%3D340806%26r2%3D340807%26view%3Ddiff&amp
> ;data=02%7C01%7Cmattpark%40microsoft.com%7C022b5d243e5d40b8517d08d60d0
> 89ae4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636710727960849332&
> amp;sdata=yF5eflZbKra5ZjjhlB0XTFWUXQbq3hoUyEToqIgpq7E%3D&reserved=
> 0
> ======================================================================
> ========
> --- llvm/trunk/lib/Analysis/MemoryBuiltins.cpp (original)
> +++ llvm/trunk/lib/Analysis/MemoryBuiltins.cpp Tue Aug 28 01:59:06
> +++ 2018
> @@ -150,7 +150,7 @@ getAllocationDataForFunction(const Funct
>     return None;
> 
>   const AllocFnsTy *FnData = &Iter->second;
> -  if ((FnData->AllocTy & AllocTy) != FnData->AllocTy)
> +  if ((FnData->AllocTy & AllocTy) == 0)
>     return None;
> 
>   // Check function prototype.
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.
> llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-commits&data=02%7C0
> 1%7Cmattpark%40microsoft.com%7C022b5d243e5d40b8517d08d60d089ae4%7C72f9
> 88bf86f141af91ab2d7cd011db47%7C1%7C0%7C636710727960849332&sdata=wg
> %2BKrs7BPB4arlrM8J0MQnoRBoi7mvI7bT6kd71i8gw%3D&reserved=0
> 