Looking over this one last time...
> - os << "Argument to free() is offset by "
> + os << "Argument to ";
> + if (!printAllocDeallocName(os, C, DeallocExpr))
> + os << "deallocator";
> + os << " is offset by "
> << offsetBytes
> << " "
> << ((abs(offsetBytes) > 1) ? "bytes" : "byte")
> - << " from the start of memory allocated by malloc()";
> + << " from the start of ";
> + if (AllocExpr) {
> + SmallString<100> TempBuf;
> + llvm::raw_svector_ostream TempOs(TempBuf);
>
> + if (printAllocDeallocName(TempOs, C, AllocExpr))
> + os << "memory allocated by " << TempOs.str();
> + else
> + os << "allocated memory";
> + } else
> + printExpectedAllocName(os, C, DeallocExpr);
> +
The way you've set it up, AllocExpr will never be NULL (which is good, because
otherwise you'd get "...from the start of malloc()" rather than "from the start
of memory allocated by malloc()").
> +@interface Wrapper : NSData
> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len;
> +@end
As I discovered with the rest of the ObjC patch, this isn't a great test case
because the analyzer doesn't consider it a system method. However, I don't see
you use it anywhere in the file anyway, so you can probably just remove it.
> +void testNew11(NSUInteger dataLength) {
> + int *data = new int;
> + NSData *nsdata = [NSData dataWithBytesNoCopy:data length:sizeof(int)
> freeWhenDone:1]; // expected-warning{{Memory allocated by 'new' should be
> deallocated by 'delete', not +dataWithBytesNoCopy:length:freeWhenDone:}}
> +}
Hm, that is rather unwieldy, but what bothers me more is that
+dataWithBytesNoCopy:length:freeWhenDone: doesn't free the memory; it just
takes ownership of it. I guess it's okay to leave that as a FIXME for now, but
in the long run we should say something like
"+dataWithBytesNoCopy:length:freeWhenDone: cannot take ownership of memory
allocated by 'new'." (In the "hold" cases, most likely the user wasn't
intending to free
But, this doesn't have to block the patch; you/we can fix it post-commit.
> + delete x; // FIXME: Shoud detect pointer escape and keep silent.
> checkPointerEscape() is not currently invoked for delete.
Pedantic note: the real issue here is that we don't model delete at all (see
ExprEngine::VisitCXXDeleteExpr). The correct model won't explicitly invoke
checkPointerEscape, but it will construct a CallEvent for the deletion operator
and then try to evaluate that call—or at least invalidate the arguments like
VisitCXXNewExpr does for placement args—which will cause the argument region to
get invalidated and checkPointerEscape to be triggered.
Jordan_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
