[cfe-commits] Add PointerEscapeKind to checkPointerEscape callback

[Anna Zaks]

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

>> Bring it on!   (: 
> 
> *cracks knuckles*
> 
> Sorry I haven't been following your thread so closely; been drawn away by other things. I agree that adding the PointerEscapeKind enum is the right way to go, and the patch is pretty straightforward. So, a lot of spot comments...
> 
> 
> +                     const enum PointerEscapeKind Kind) {
> 
> No "const" needed for value types, and conventionally no "enum" in LLVM-style C++. This shows up in a couple of places.
> 
> 
> +/// \brief Describes the different reasons a pointer escapes
> +/// during analysis.
> +enum PointerEscapeKind {
> +  /// A pointer escapes due to binding its value to a location,
> +  /// preventing further reasoning on the pointer.
> +  PSK_EscapeOnBind = 1,
> 
> This makes it sound like this happens for any location, which isn't true. How about "a location the analyzer can't track"?
> 
> 
> +  /// 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 accessable through an
> +  /// argument to a function.
> +  PSK_IndirectEscapeOnCall,
> 
> Typo: "accessable"
> 
> 
> +  ///The reason for pointer escape is unknown. For example, a checker
> +  ///invalidates a region and intends the invalidation to cause a
> +  ///pointer escape event.
> +  PSK_EscapeOther
> 
> Missing spaces after the slashes. Also, I feel like this is too explicit—if a region is invalidated and the checker writer doesn't think about whether the contents escape, this still happens. How about "For example, a region containing this pointer is invalidated."?
> 
> +};
> 
> (I do want to say "nice comments", though! Thank you for putting in the time to document everything.)
> 
> 
> +  /// \param Kind The kind of escape the pointers have undergone.
> 
> Clever use of English. Simpler: "How the symbols have escaped."
> 
> 
>    ProgramStateRef checkPointerEscape(ProgramStateRef State,
> -                                     const InvalidatedSymbols &Escaped,
> -                                     const CallEvent *Call) const {
> +                            const InvalidatedSymbols &Escaped,
> +                            const CallEvent *Call,
> +                            const enum PointerEscapeKind Kind) const {
> 
> Please don't change all the indentation; before the parameters lined up, now they don't. I realize that the last line didn't fit, but so far in the analyzer we've been just tweaking that one line. If you want to change all the lines, please put the first parameter on its own line and indent everything twice only.
> 
> ...although the removal of "const" and "enum" may make this short enough to fit.
> 
> 
> -  if (Call && doesNotFreeMemory(Call, State))
> +  if ((Kind == PSK_DirectEscapeOnCall ||
> +      Kind == PSK_IndirectEscapeOnCall) &&
> +      doesNotFreeMemory(Call, State)) {
> 
> This is not correct. Before, this branch was only taken if the Kind is PSK_DirectEscapeOnCall. Indirect escapes can still possibly free memory (although it's unlikely).
> 

Jordan,

I think the condition is correct. In fact, this is the whole point of this commit.
For example, previously, we would assume that a call to foo_that_does_not_free_memory(p+1), could free memory.

Branden, can you add a test case that shows this? I know the test cases in the second patch rely on this, but a self contained example would make the change explicit. (Ex: you could call a function from a system header and pass it p+1.)

> 
> -  if (Call && guaranteedNotToCloseFile(*Call))
> +  if ((Kind == PSK_DirectEscapeOnCall ||
> +      Kind == PSK_IndirectEscapeOnCall) &&
> +      guaranteedNotToCloseFile(*Call)) {
> 
> Ditto.
> 
> 
> +  assert((Kind == PSK_DirectEscapeOnCall ||
> +          Kind == PSK_IndirectEscapeOnCall) ?
> +          Call != NULL : true);
> 
> What Anna said about implication.
> 
> 
> -                                                          EscapedSymbols,
> -                                                          /*CallEvent*/ 0);
> +                                                      EscapedSymbols,
> +                                                      /*CallEvent*/ 0,
> +                                                      PSK_EscapeOnBind);
> 
> -                                                           *Invalidated, 0);
> +                                                       *Invalidated,
> +                                                       0,
> +                                                       PSK_EscapeOther);
> 
> These do fit in 80 columns. Please do not reformat arbitrarily.
> 
> I'll review the malloc patch in a separate e-mail.

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