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

[Branden Archer]

>
> I'm still not sure why this enum needs any explicit integer values (the "=
> 1").
>

No reason really. Easy enough to remove.

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

Oops.

Also, please divide by ASTContext's getCharWidth(), not 8.
>

Is getCharWidth() guaranteed to always be 8? If not, the result will be
incorrect. RegionOffset.getOffset() returns a number in bits, and if the
char width on the system is more than 8 bits then the printed result will
not make sense.

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

I like it!

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

I am not sure what sort of test case would be appropriate. I have added two
test cases, one for the malloc checker and the other for the simple stream
checker, for the case where the reference of interest is passed to a
function that accepts it as a const. Is this maybe what you had in mind?

- Branden


On Fri, Jan 25, 2013 at 10:44 PM, Anna Zaks <ganna at apple.com> wrote:

>
> 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/20130128/b648343a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-update-malloc-checker.patch
Type: application/octet-stream
Size: 9683 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130128/b648343a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-modify-checkPointerEscape.patch
Type: application/octet-stream
Size: 11494 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130128/b648343a/attachment-0001.obj>