> > 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 <[email protected]> wrote: > > On Jan 25, 2013, at 6:31 PM, Jordan Rose <[email protected]> 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 > > >
02-update-malloc-checker.patch
Description: Binary data
01-modify-checkPointerEscape.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
