Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[ariccio]

Summary

Instead of one long function, MemRegionManager::getVarRegion now calls getMemRegionGloballyStored for global, non-static variables, and getMemRegionStaticLocal for static locals. The behavior of MemRegionManager::getVarRegion is thus clearer, and easier to reason about.

[a.sidorin]

Thanks ariccio! Some inline comments are below.

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

1186

How about make these helper functions return const MemSpaceRegion * to make their signatures more meaningful?

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

834

get[Global/StaticLocal]MemSpaceForVariable?

853

const MemSpaceRegion *StorageSpace?

[ariccio]

Responded to comments.

Will happily make changes when questions are answered.

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

1186

Would that change their behavior functionally?

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

834

Ahh, that might make more sense. I did this refactoring without any sense of context, as the unnecessary complexity was a hindrance thereto.

834

How about getGlobalMemSpaceForGlobalVariable? I'm hesitant to drop the second global, to be clear about the scope of the variable that's stored globally. Is that a reasonable concern, or is that redundant?

853

Same question as above: Would that change their behavior functionally?

(if not, then I'll happily change it)

[dcoughlin]

I guess this is a reasonable refactoring. Although someone with different tastes might come along and inline it back in since the two extracted functions each only have single callers.

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

1185

I think describing this as taking a "globally stored, non-static local, VarDecl" is ambiguous. This method operates on global variables. It does not act on local variables (static or otherwise). How about "Retrieve or create the memory region associated with a VarDecl for a global variable."

Also, the recommended style these days is to not prefix the doc comment with the name of the member, so I would remove "getMemRegionGloballyStored - " even though getVarRegion() has the same thing. The doc comment style in this file is sufficiently mismatched that I think it is better to do the now-recommend thing rather than try to match its surrounding context.

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

834

The rest of the method in class follow a pattern of getAdjectiveNoun, so I would suggest something like getGlobalVarRegion() and getLocalVarRegion()

838–839

It looks to me like this function operates on both locals *and* static locals. I would change the name to reflect that.

858

This comment was wrong before and very misleading. I would just remove it so no one else trips on it!

[a.sidorin]

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

1186

No, it will not change it. You will need to change static_cast type to const MemSpaceRegion *, however (lines 809-810). There is const MemSpaceRegion * and its children classes everywhere in return statements already.

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

853

No, this will not.

[a.sidorin]

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

845

Oops.
Lines above should stay in the caller function or be refactored. Otherwise, we'll get the subregion of V.get<const VarRegion * instead of returning it directly. This looks like a behaviour change. (And this is the only place where return type is not a const MemSpaceRegion *, btw).
This issue may also obsolete my comments about return type.

[ariccio]

Whoops, I didn't submit my comments.

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

1185

I like that. Done.

1186

What're the static_cast<const MemRegion*>s for anyways? Shouldn't the const StackArgumentsSpaceRegion*s convert to const MemRegion* automatically?

StackArgumentsSpaceRegion derives from StackSpaceRegion:

class StackArgumentsSpaceRegion : public StackSpaceRegion

...StackSpaceRegion derives from MemSpaceRegion:

class StackSpaceRegion : public MemSpaceRegion

...and MemSpaceRegion derives from MemRegion:

class MemSpaceRegion : public MemRegion

...which I think should work, because of the kooky rules of covariant return types (or something)?

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

834

Whatever. I've changed the global one to getGlobalVarRegion.

838–839

So, getMemRegionStaticLocalOrLocal?

845

Nice catch! That's exactly why we do code review :)

This issue may also obsolete my comments about return type.

We might not want to change the return type, I see.

[ariccio]

Marked some inline comments as done.

[ariccio]

Bump?

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

838–839

Bump?

[dcoughlin]

I think Aleksei is right and I was wrong.

If you follow his advice and move the call to getStackOrCaptureRegionForDeclContext() to getVarRegion() and then pass the StackFrameContext to what you currently name "getMemRegionStaticLocal" then both methods have the very nice property that they return memory space regions and all the VarRegion stuff is kept in the caller.

In my opinion getMemSpaceForGlobalVariable() and getMemSpaceForLocalVariable() are good names for these two new methods. If you prefer 'getMemSpaceForStaticLocalOrLocalVariable()' I think that would be fine too.

[ariccio]

Alrighty, will update the diff momentarily.

[ariccio]

Responded to comments (fixed the bug noticed in review), and changed names.

[ariccio]

All clear. Ready for landing?

[a.sidorin]

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

792

That needs to be formatted. Also, you may pass const StackFrameContext * instead of PointerUnion (as Devin pointed) because it is only used once to get SFC.
And I still think that making these function return const MemSpaceRegion * is a good idea.

[ariccio]

One of two comments addressed, one question asked.

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

792

If I change it to a MemSpaceRegion*, do I need to change the static_casts, from:

? static_cast<const MemRegion*>(getStackArgumentsRegion(STC))
: static_cast<const MemRegion*>(getStackLocalsRegion(STC)));

to:

? static_cast<const MemSpaceRegion*>(getStackArgumentsRegion(STC))
: static_cast<const MemSpaceRegion*>(getStackLocalsRegion(STC)));

...?

I'm still not quite sure what they're for in the first place?

[ariccio]

I've reformatted, and changed the PointerUnion to a const StackFrameContext*.

Built successfully, etc...

(I hope I don't sound too pushy!)

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

792

I'm sorry, I think you're right, but I wanna get this patch over with, so I can move on to other things. Some other time, maybe?

[a.sidorin]

Hello Alexander,

Yes, types in static_cast should be changed. So, you need only to change return type and types in static_cast, and it should work.

[a.sidorin]

Alexander, could you update status of this review?

[ariccio]

In D16873#371736, @a.sidorin wrote:

Alexander, could you update status of this review?

It's become a zombie review. I should just abandon it, as I'll be away for two weeks starting tomorrow.

llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

public:
   const StringRegion *getStringRegion(const StringLiteral* Str);
 
   const ObjCStringRegion *getObjCStringRegion(const ObjCStringLiteral *Str);
 
   /// getVarRegion - Retrieve or create the memory region associated with
   ///  a specified VarDecl and LocationContext.
   const VarRegion* getVarRegion(const VarDecl *D, const LocationContext *LC);
 
+private:
+  /// Retrieve or create the memory region
+  ///  associated with a VarDecl for a global variable.
+  const MemRegion *getMemSpaceForGlobalVariable(const VarDecl *D);
+
+  /// Retrieve or create the memory region
+  ///  associated with a specified VarDecl and LocationContext.
+  const MemRegion *getMemSpaceForLocalVariable(const VarDecl *D, llvm::PointerUnion<const StackFrameContext *, const VarRegion *> &V);
+
+public:
   /// getVarRegion - Retrieve or create the memory region associated with
   ///  a specified VarDecl and super region.
   const VarRegion* getVarRegion(const VarDecl *D, const MemRegion *superR);
   
   /// getElementRegion - Retrieve the memory region associated with the
   ///  associated element type, index, and super region.
   const ElementRegion *getElementRegion(QualType elementType, NonLoc Idx,
                                         const MemRegion *superRegion,

llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

if (const BlockInvocationContext *BC =
       }
     }
 
     LC = LC->getParent();
   }
   return (const StackFrameContext *)nullptr;
 }
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-                                                const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
-
-  if (D->hasGlobalStorage() && !D->isStaticLocal()) {
-
-    // First handle the globals defined in system headers.
-    if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
-      // Whitelist the system globals which often DO GET modified, assume the
-      // rest are immutable.
-      if (D->getName().find("errno") != StringRef::npos)
-        sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
-      else
-        sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+const MemRegion* MemRegionManager::getMemSpaceForGlobalVariable(const VarDecl *D) {
+  assert(D->hasGlobalStorage());
+  assert(!D->isStaticLocal());
+      return getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
 
-    // Treat other globals as GlobalInternal unless they are constants.
-    } else {
-      QualType GQT = D->getType();
-      const Type *GT = GQT.getTypePtrOrNull();
-      // TODO: We could walk the complex types here and see if everything is
-      // constified.
-      if (GT && GQT.isConstQualified() && GT->isArithmeticType())
-        sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-      else
-        sReg = getGlobalsRegion();
-    }
+    return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+  }
+    return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
 
-  // Finally handle static locals.
-  } else {
-    // FIXME: Once we implement scope handling, we will need to properly lookup
-    // 'D' to the proper LocationContext.
-    const DeclContext *DC = D->getDeclContext();
-    llvm::PointerUnion<const StackFrameContext *, const VarRegion *> V =
-      getStackOrCaptureRegionForDeclContext(LC, DC, D);
-
-    if (V.is<const VarRegion*>())
-      return V.get<const VarRegion*>();
+  return getGlobalsRegion();
+}
 
-    const StackFrameContext *STC = V.get<const StackFrameContext*>();
+const MemRegion* MemRegionManager::getMemSpaceForLocalVariable(const VarDecl *D, llvm::PointerUnion<const StackFrameContext *, const VarRegion *> &V) {
 
-    if (!STC)
-      sReg = getUnknownRegion();
-    else {
-      if (D->hasLocalStorage()) {
-        sReg = isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D)
-               ? static_cast<const MemRegion*>(getStackArgumentsRegion(STC))
-               : static_cast<const MemRegion*>(getStackLocalsRegion(STC));
-      }
-      else {
-        assert(D->isStaticLocal());
-        const Decl *STCD = STC->getDecl();
-        if (isa<FunctionDecl>(STCD) || isa<ObjCMethodDecl>(STCD))
-          sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
-                                  getFunctionCodeRegion(cast<NamedDecl>(STCD)));
-        else if (const BlockDecl *BD = dyn_cast<BlockDecl>(STCD)) {
-          // FIXME: The fallback type here is totally bogus -- though it should
-          // never be queried, it will prevent uniquing with the real
-          // BlockCodeRegion. Ideally we'd fix the AST so that we always had a
-          // signature.
-          QualType T;
-          if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
-            T = TSI->getType();
-          if (T.isNull())
-            T = getContext().VoidTy;
-          if (!T->getAs<FunctionType>())
-            T = getContext().getFunctionNoProtoType(T);
-          T = getContext().getBlockPointerType(T);
+    return getUnknownRegion();
+
+    return (isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D)
+           : static_cast<const MemRegion*>(getStackLocalsRegion(STC)));
+    return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+
 
-          const BlockCodeRegion *BTR =
-            getBlockCodeRegion(BD, C.getCanonicalType(T),
-                               STC->getAnalysisDeclContext());
-          sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
-                                  BTR);
-        }
-        else {
-          sReg = getGlobalsRegion();
-        }
-      }
-    }
-  }
+    return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+  return getGlobalsRegion();
 
+const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
+                                                const LocationContext *LC) {
+  const MemRegion *sReg = nullptr;
+
+  if (D->hasGlobalStorage() && !D->isStaticLocal()) {
+    sReg = getMemSpaceForGlobalVariable(D);
+
+  } else {
+    // FIXME: Once we implement scope handling, we will need to properly lookup
+    // 'D' to the proper LocationContext.
+    const DeclContext *DC = D->getDeclContext();
+    llvm::PointerUnion<const StackFrameContext *, const VarRegion *> V =
+      getStackOrCaptureRegionForDeclContext(LC, DC, D);
+
+    if (V.is<const VarRegion*>())
+      return V.get<const VarRegion*>();
+
+    sReg = getMemSpaceForLocalVariable(D, V);
+  }
   return getSubRegion<VarRegion>(D, sReg);
 }
 
 const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D,
                                                 const MemRegion *superR) {
   return getSubRegion<VarRegion>(D, superR);
 }
 
 const BlockDataRegion *
 MemRegionManager::getBlockDataRegion(const BlockCodeRegion *BC,
                                      const LocationContext *LC,
                                      unsigned blockCount) {
   const MemRegion *sReg = nullptr;
   const BlockDecl *BD = BC->getDecl();
   if (!BD->hasCaptures()) {
     // This handles 'static' blocks.