[PATCH] D14933: Add the allocsize attribute to LLVM

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: include/llvm/Analysis/MemoryBuiltins.h:170
@@ -169,3 +169,3 @@
 
-  bool bothKnown(SizeOffsetType &SizeOffset) {
+  bool bothKnown(SizeOffsetType &SizeOffset) const {
     return knownSize(SizeOffset) && knownOffset(SizeOffset);
----------------
joker.eph wrote:
> It seems unrelated to the rest of this patch, can you commit now?
r261144

================
Comment at: include/llvm/IR/Attributes.h:154
@@ -149,1 +153,3 @@
 
+  /// \brief Returns the argument numbers form the allocsize attribute (or
+  /// pair(0, 0) if not known).
----------------
joker.eph wrote:
> s/form/for/
> 
> Also no `\brief` anywhere unless you want multiple sentences as part of the brief (the code that has \brief is legacy).
Is there something that I should be using instead of `\brief`, or should I just remove it entirely?

================
Comment at: lib/Analysis/MemoryBuiltins.cpp:487
@@ +486,3 @@
+    return std::make_pair(S, Zero);
+  };
+
----------------
joker.eph wrote:
> This is not clear to me, is this if we overflow the 32 bits that the pointer can represent? 
> Is it consider UB or...?
This is a remnant of two things:
- Trying to handle overflow sanely for `llvm.objectsize` (the user can call 32 and 64 bit variants of `objectsize`)
- Trying to handle sizes larger than `IntTyBits` properly

...The first part of that has been moved to InstCombineCalls, so this can mostly go away. The interesting bit is when the user gives us e.g. `void *special_malloc(uint64_t N)` on 32-bit platforms. Other places in the code expect the `APInt`s you hand back to be `IntTyBits` bits wide, so we need to shrink the `uint64_t` to be 32 bits.

An argument can be made for doing a checked truncation of `N` or for doing an unchecked truncation. Given that checking if `N` requires more than 32 bits is cheap, I think having well-defined behavior (read: failing) in this case is harmless.

I rewrote this bit of code so it's hopefully more clear (and added a test to be sure we get this right). :)


http://reviews.llvm.org/D14933