[analyzer] Add option for AddrSpace in core.NullDereference check

[vabridgers]

Summary

This change adds an option to detect all null dereferences for
non-default address spaces, except for address spaces 256, 257 and 258.
Those address spaces are special since null dereferences are not errors.

All address spaces can be considered (except for 256, 257, and 258) by
using -analyzer-config
core.NullDereference:DetectAllNullDereferences=true. This option is
false by default, retaining the original behavior.

A LIT test was enhanced to cover this case, and the rst documentation
was updated to describe this behavior.

[steakhal]

Couls you please provide some context about why did we have this suppression in the first place?

Other than that, I'm on board with this, but we should really investigate the historical reasons why we had this.

If it turns out to be a valid usecase, but a rather special one we could still have an analyzer config option making this change optional.

@NoQ do you have any background on this?

[vabridgers]

@steakhal, good question. Digging into the change archaeology, I see this change was made by Anna Zaks in Jan 2016 - see commit header below.

There's no reference to a review with comments, and I don't know how to find those if they exist, perhaps @NoQ could help with that?

In any case, judging by the changes I had to make to the test cases, there were no regressions in the current test set, and at least our build tests ran ok - seems like this is no longer necessary. But I understand the need to approach this carefully.

Thanks

commit c9f16fe48c40e7683d0029b65090d0007414d7af
Author: Anna Zaks <[e-mail]>
Date:   Wed Jan 6 00:32:49 2016 +0000

    [analyzer] Don't report null dereferences on address_space annotated memory
    
    llvm-svn: 256885

[NoQ]

I looked up the history. I believe this refers to https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments:

Annotating a pointer with address space #256 causes it to be code generated relative to the X86 GS segment register, address space #257 causes it to be relative to the X86 FS segment, and address space #258 causes it to be relative to the X86 SS segment. Note that this is a very very low-level feature that should only be used if you know what you’re doing (for example in an OS kernel).

So basically ((void *__attribute__((address_space(256))) *)0) is a valid pointer that can be safely dereferenced.

I'm not aware of other situations of this kind. Probably this address space range can be hardcoded, and/or a run-time checker option can be provided to enable/disable this behavior.

[steakhal]

In D122841#3431617, @NoQ wrote:

I looked up the history. I believe this refers to https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments:

Annotating a pointer with address space #256 causes it to be code generated relative to the X86 GS segment register, address space #257 causes it to be relative to the X86 FS segment, and address space #258 causes it to be relative to the X86 SS segment. Note that this is a very very low-level feature that should only be used if you know what you’re doing (for example in an OS kernel).

So basically ((void *__attribute__((address_space(256))) *)0) is a valid pointer that can be safely dereferenced.

Well, it seems like, on Windows, the FS register points to the Win32 Thread Environment Block - TEB (wiki). And at offset 0x0, resides the pointer to the Current Structured Exception Handling (SEH) frame, making it perfectly valid to load and dereference FS:[0]. Using FS and GS registers are frequently used for implementing anti-debug tricks in practice, but I think it's fairly safe to say that they are not our primary audience.

I'm not aware of other situations of this kind. Probably this address space range can be hardcoded, and/or a run-time checker option can be provided to enable/disable this behavior.

That being said, I think it might deserve a user-facing analyzer config option.

[vabridgers]

Ahhh, I see - thanks for the references and discussion. I'll craft a patch to comprehend the use case and user-facing option, and post for review. We'll see how it goes :)

[vabridgers]

add option, retain legacy behavior per comments. update rst

[vabridgers]

clean up a few debug edits

[steakhal]

clang/docs/analyzer/checkers.rst

69–74

IMO this should be in a separate paragraph. It's such a niche use-case.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

119

Shouldn't we check for this target?
Add a test if we have x86 and a non-x86 target.

125–133

Even though DetectAllNullDereferences=true, address_space(256) would be suppressed, which seems to be a contradiction to me.

We should either pick a different name or implement a different semantic.
I would vote for both, personally.

Then name should encode that it's purely about address-space attributes.
The implementation should check if we have attributes or not, and bail out in the generic case. I would do something like this:

QualType Ty = E->getType();
if (!Ty.hasAddressSpace())
  return false;
if (SuppressAddressSpaces)
  return true;
// On X86 addr spaces ...., thus ignored.
return target == x86 && addressspace in {256,257,258};

331

dead-code?

[vabridgers]

I'll address, thanks for the comments.

clang/docs/analyzer/checkers.rst

69–74

I'll address.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

119

I wasn't sure it was needed, but easy enough to add I think. I'll address both.

125–133

I'll clean up the semantics. Thanks.

[vabridgers]

update per @steakhal's comments

[vabridgers]

I believe all points have been addressed. Thanks for the comments.

[steakhal]

No major issues left.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

55

If I'm correct, we tend to use the 'DefaultBool' instead of plain old bools for checker options. Could you please check if this is the case and consolidate this?

331

This still looks dead.

clang/test/Analysis/cast-value-notes.cpp

19

Nit: the default triple is the host/native triple, but you did actually pin it to x86. Hence it might not be the default on other platforms.

[vabridgers]

Thanks for the the comments. I'll make the changes and resubmit. Please let me know your preferences for the test case (default vs pinned x86 or same).

Best!

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

55

I'll address. FYI, looks like I chose the red pill when I found an example to go by :/ Looks like there's opportunity for some cleanup NFC type of patches. I'll pick these up in a separate patch.

120 namespace {
121 class DeadStoresChecker : public Checker<check::ASTCodeBody> {
122 public:
123   bool ShowFixIts = false;
124   bool WarnForDeadNestedAssignments = true;
125 
126   void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
127                         BugReporter &BR) const;
128 };

331

arg, missed that. I'll clean this up.

clang/test/Analysis/cast-value-notes.cpp

19

Yeah, the result is the same (as far as what the test code would look like). Not sure what your preferences are here.

Would you prefer to see separate cases (default and pinned x86), or perhaps the macro names renamed to something that means both (instead of implying default)?

[vabridgers]

update per @steakhal's latest comments

[vabridgers]

The two small issues are addressed, I think the only outstanding issue is the question about the test case. Please let me know a preference and I'll implement accordingly. Best!

[vabridgers]

Update test cases, address comments

[vabridgers]

I believe all comments have been addressed. After discussion with @steakhal, we're leaning towards removing the DefaultBool class and refactoring in favor of the language native bool type - mainly because DefaultBool doesn't add anything. Minimal yet effective is usually better.

[vabridgers]

add back amdgcn cases

clang/docs/analyzer/checkers.rst

void test(int *p) {
    if (!p)
      f(p); // warn
  }
 
 .. _core-NullDereference:
 
 core.NullDereference (C, C++, ObjC)
 """""""""""""""""""""""""""""""""""
-Check for dereferences of null pointers.
+Check for dereferences of null pointers. This checker specifically does
+not report null pointer dereferences for x86 and x86-64 targets when the
+address space is 256 (x86 GS Segment), 257 (x86 FS Segment), or 258 (x86 SS
+segment). See `X86/X86-64 Language Extensions
+<https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments>`__
+for reference.
+
+The ``DetectAllNullDereferences`` option enables the checker to emit
+warnings for null dereferences of all pointers. You can enable with the
+``-analyzer-config core.NullDereference:DetectAllNullDereferences=true``.
+*Defaults to false*.
 
 .. code-block:: objc
 
  // C
  void test(int *p) {
    if (p)
      return;
 

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

CmdLineOption<Boolean,
                   "true",
                   Released>,
   ]>,
   Documentation<HasDocumentation>,
   Dependencies<[CallAndMessageModeling]>;
 
 def DereferenceChecker : Checker<"NullDereference">,
   HelpText<"Check for dereferences of null pointers">,
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "DetectAllNullDereferences",
+                  "Enables detection of all dereferences, regardless of address space",
+                  "false",
+                  Released>
+  ]>,
   Documentation<HasDocumentation>;
 
 def NonNullParamChecker : Checker<"NonNullParamChecker">,
   HelpText<"Check for null pointers passed as arguments to a function whose "
            "arguments are references or marked with the 'nonnull' attribute">,
   Documentation<HasDocumentation>;
 
 def VLASizeChecker : Checker<"VLASize">,

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

class DereferenceChecker
 
   BugType BT_Null{this, "Dereference of null pointer", categories::LogicError};
   BugType BT_Undef{this, "Dereference of undefined pointer value",
                    categories::LogicError};
 
   void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
                  CheckerContext &C) const;
 
+  bool suppressReport(const Expr *E) const;
+
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
                      CheckerContext &C) const;
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
 
   static void AddDerefSource(raw_ostream &os,
                              SmallVectorImpl<SourceRange> &Ranges,
                              const Expr *Ex, const ProgramState *state,
                              const LocationContext *LCtx,
                              bool loadedFrom = false);
+
+  bool DetectAllNullDereferences = false;
 };
 } // end anonymous namespace
 
 void
 DereferenceChecker::AddDerefSource(raw_ostream &os,
                                    SmallVectorImpl<SourceRange> &Ranges,
                                    const Expr *Ex,
                                    const ProgramState *state,
if (IsBind) {
     const Expr *Init;
     std::tie(VD, Init) = parseAssignment(S);
     if (VD && Init)
       E = Init;
   }
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
+bool DereferenceChecker::suppressReport(const Expr *E) const {
+  // Do not report dereferences on memory that use address space #256, #257,
+  // and #258. Those address spaces are used when dereferencing address spaces
+  // relative to the GS, FS, and SS segments on x86/x86-64 targets.
+  // Dereferencing a null pointer in these address spaces is not defined
+  // as an error. All other null dereferences in other address spaces
+  // are defined as an error unless explicitly defined.
+  // See https://clang.llvm.org/docs/LanguageExtensions.html, the section
+  // "X86/X86-64 Language Extensions"
+  if (isTargetAddressSpace(E->getType().getAddressSpace())) {
+    switch (toTargetAddressSpace(E->getType().getAddressSpace())) {
+    case 256:
+    case 257:
+    case 258:
+      return true;
+    }
+  }
+  return (E->getType().hasAddressSpace() && !DetectAllNullDereferences);
 }
 
 static bool isDeclRefExprToReference(const Expr *E) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
     return DRE->getDecl()->getType()->isReferenceType();
   return false;
 }
 
void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   // of this example.
   //
   // We do need to add a transition because we may have generated a sink for
   // the "implicit" null dereference.
   C.addTransition(State, this);
 }
 
 void ento::registerDereferenceChecker(CheckerManager &mgr) {
-  mgr.registerChecker<DereferenceChecker>();
+  auto *Chk = mgr.registerChecker<DereferenceChecker>();
+  // auto *Chk = mgr.getChecker<DereferenceChecker>();
+  Chk->DetectAllNullDereferences =
+      mgr.getAnalyzerOptions().getCheckerBooleanOption(
+          mgr.getCurrentCheckerName(), "DetectAllNullDereferences");
 }
 
 bool ento::shouldRegisterDereferenceChecker(const CheckerManager &mgr) {
   return true;
 }

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: core.CallAndMessage:ArgInitializedness = true
 // CHECK-NEXT: core.CallAndMessage:ArgPointeeInitializedness = false
 // CHECK-NEXT: core.CallAndMessage:CXXDeallocationArg = true
 // CHECK-NEXT: core.CallAndMessage:CXXThisMethodCall = true
 // CHECK-NEXT: core.CallAndMessage:FunctionPointer = true
 // CHECK-NEXT: core.CallAndMessage:NilReceiver = true
 // CHECK-NEXT: core.CallAndMessage:ParameterCount = true
 // CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
+// CHECK-NEXT: core.NullDereference:DetectAllNullDereferences = false
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: cplusplus.SmartPtrModeling:ModelSmartPtrDereference = false
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-import-cpp-threshold = 8
 // CHECK-NEXT: ctu-import-threshold = 24
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
 // CHECK-NEXT: ctu-invocation-list = invocations.yaml

clang/test/Analysis/cast-value-notes.cpp

 // RUN: %clang_analyze_cc1 -std=c++14 \
 // RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
 // RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
 // RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:DetectAllNullDereferences=true -analyzer-output=text\
+// RUN: -verify -DDETECT_ALL_NULL_DEREFERENCES  -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=AMDGCN-CHECK-WARNADDRSPACE
+
 #include "Inputs/llvm.h"
 
 // The amggcn triple case uses an intentionally different address space.
 // The core.NullDereference checker intentionally ignores checks
 // that use address spaces, so the case is differentiated here.
 //
 // From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
 // select address space 3 (local), since the pointer size is
 // different than Generic.
 #define DEVICE __attribute__((address_space(3)))
 
 namespace clang {
 struct Shape {
void evalReferences(const Shape &S) {
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
   // DEFAULT-CHECK: "dynamic_types": [
   // DEFAULT-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
+#if defined(DETECT_ALL_NULL_DEREFERENCES)
+void evalReferences(const Shape &S) {
+  const auto &C = dyn_cast<DEVICE Circle>(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // AMDGCN-CHECK-WARNADDRSPACE: "dynamic_types": [
+  // AMDGCN-CHECK-WARNADDRSPACE-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#else
 void evalReferences(const Shape &S) {
   const auto &C = dyn_cast<DEVICE Circle>(S);
   clang_analyzer_printState();
   // AMDGCN-CHECK: "dynamic_types": [
   // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
   (void)C;
 }
+#endif
 #else
 #error Target must be specified, and must be pinned
 #endif
 
 void evalNonNullParamNonNullReturnReference(const Shape &S) {
   const auto *C = dyn_cast_or_null<Circle>(S);
   // expected-note@-1 {{'C' initialized here}}