[cfe-commits] unix.Malloc static checker improvement: memory.LeakPtrValChanged

[Anna Zaks]

On Jan 25, 2013, at 6:31 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Almost there!
> 
> +/// \brief Describes the different reasons a pointer escapes
> +/// during analysis.
> +enum PointerEscapeKind {
> +  /// A pointer escapes due to binding its value to a location
> +  /// that the analyzer cannot track.
> +  PSK_EscapeOnBind = 1,
> +
> +  /// The pointer has been passed to a function call directly.
> +  PSK_DirectEscapeOnCall,
> +
> +  /// The pointer has been passed to a function indirectly.
> +  /// For example, the pointer is accessible through an
> +  /// argument to a function.
> +  PSK_IndirectEscapeOnCall,
> +
> +  /// The reason for pointer escape is unknown. For example, 
> +  /// a region containing this pointer is invalidated.
> +  PSK_EscapeOther
> +};
> 
> I'm still not sure why this enum needs any explicit integer values (the "= 1").
> 
> 
> -        (RS->isReleased() ? "Attempt to free released memory" : 
> +        (RsBase->isReleased() ? "Attempt to free released memory" :
>                              "Attempt to free non-owned memory"), N);
> 
> I was going to say that the following line needs to be lined up as well, but really I think this is an odd case where the prevailing style is to put the : on the next line and line it up with the ?.
> 
> 
> +      Offset.hasSymbolicOffset() == false &&
> 
> Should just be !Offset.hasSymbolicOffset()
> 
> 
> +void MallocChecker::ReportOffsetFree(CheckerContext &C, SVal ArgVal,
> +                                     SourceRange range) const {
> 
> Capital "range", please!
> 
> 
> +    const MemRegion *MR = ArgVal.getAsRegion();
> +    assert((MR) && "Only MemRegion based symbols can have offset free errors");
> 
> Good assertion; conventionally no parens around a single assertion condition.
> 
> 
> +    // Get the offset into the memory region before
> +    // getting the super region, as retrieving the
> +    // super region will ignore the offset.
> 
> The comment's (still?) wrapped very aggressively here; you can unwind it to 80 columns.
> 

No need for the comment - we are no longer getting the super region.

> 
> +    os << "Argument to free() was not allocated by malloc(), but instead is " <<
> +          "a pointer offset by " << Offset.getOffset()/8  <<
> +          " byte(s) from the start of allocated memory";
> 
> Another case of operator inconsistency in the LLVM standards. We tend to break before << and then line them up with the previous <<.
> 
> Also, please divide by ASTContext's getCharWidth(), not 8.
> 
>> The message emitted from ReportOffsetFree is a little different from the one that was emitted in ReportBadFree. Take a look and see if you agree with its phrasing and the information it is providing.
> 
> I like the increased information! I think you can go even further, though, and just directly say what's wrong, rather than leading with "was not allocated by malloc()".
> 
> "Argument to free() is offset by # bytes from the start of allocated memory"

+1

Or, if we want to mention "malloc".

"Argument to free() is offset by # bytes from the start of memory allocated by malloc()"

> 
> (Also, we can totally get "byte"/"bytes" correct!)
> 
> 
>> However, no expected-warning comment resulted in a failure. Is there a special format for the expected-warning comment that I am missing, or is the comparison that is taking place not quite right?
> 
> You were very close! expected-warning deliberately matches a substring of the actual warning emitted, so that you can choose how sensitive it is about the particular phrasing of the warning. Since this is the first test for the offset warning, you probably want to use the full string.
> 
> 
>>  I sort of agree with part of both of the possibilities. I would rather the assert be its own contained unit, but I do not like inverting the logic in the assert, as it may make it a little harder to parse. How about this?
>> 
>>   assert((Call != NULL ||
>>           (Kind != PSK_DirectEscapeOnCall &&
>>            Kind != PSK_IndirectEscapeOnCall)) &&
>>          "Call must not be NULL when escaping on call");
>> 
>> I will probably be called on spacing rules though, as I tried to indent pass the ( only if within the same set of (). That is, the Kinds should line up, but the Call and "Call should not, as the first Call is within an extra ().
> 
> That's probably the best we're going to get (and the best indentation), and Anna agrees with you on preferring to keep everything inside the assert. Let's go with this.
> 
> After this I think it'll be ready to commit! Anna?

We need to add a test case to the first patch that exercises the new behavior.

Otherwise, looks good.
Thanks Branden!

> 
> Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130125/a331b420/attachment.html>