[clang][analyzer] Add checker 'alpha.core.FixedAddressDereference' by balazske · Pull Request #127191 · llvm/llvm-project (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/127191.diff

5 Files Affected:

• (modified) clang/docs/analyzer/checkers.rst (+38)
• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+15-11)
• (modified) clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp (+57-6)
• (modified) clang/test/Analysis/concrete-address.c (+159-3)
• (modified) clang/test/Analysis/misc-ps.m (+2-2)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 707067358fdfe..c5e10f5a8bbf2 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -129,6 +129,8 @@ The SuppressAddressSpaces option suppresses warnings for null dereferences of all pointers with address spaces. You can disable this behavior with the option -analyzer-config core.NullDereference:SuppressAddressSpaces=false. +Value of this option is used for checker :ref:_core-FixedAddressDereference +too. Defaults to true. .. code-block:: objc @@ -2919,6 +2921,42 @@ Check for assignment of a fixed address to a pointer. p = (int *) 0x10000; // warn } +.. alpha-core-FixedAddressDereference: + +alpha.core.FixedAddressDereference (C, C++, ObjC) +""""""""""""""""""""""""""""""""""""""""""""""""" +Check for dereferences of fixed values used as pointers. + +Similarly as at :ref:_core-NullDereference, the checker specifically does +not report 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.) + +If you want to disable this behavior, set the SuppressAddressSpaces option +of checker core.NullDereference to false, like +-analyzer-config core.NullDereference:SuppressAddressSpaces=false. The value +of this option is used for both checkers. + +.. code-block:: c + + void test1() { + int *p = (int *)0x020; + int x = p[0]; // warn + } + + void test2(int *p) { + if (p == (int *)-1) + *p = 0; // warn + } + + void test3() { + int (p_function)(char, char); + p_function = (int ()(char, char))0x04080; + int x = (*p_function)('x', 'y'); // NO warning yet at functon pointer calls + } + .. _alpha-core-PointerArithm: alpha.core.PointerArithm (C) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 9bf491eac1e0e..44ca28c003b06 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -211,17 +211,15 @@ def DereferenceModeling : Checker<"DereferenceModeling">, Documentation, Hidden; -def NullDereferenceChecker : Checker<"NullDereference">, - HelpText<"Check for dereferences of null pointers">, - CheckerOptions<[ - CmdLineOption<Boolean, - "SuppressAddressSpaces", - "Suppresses warning when pointer dereferences an address space", - "true", - Released> - ]>, - Documentation, - Dependencies<[DereferenceModeling]>; +def NullDereferenceChecker + : Checker<"NullDereference">, + HelpText<"Check for dereferences of null pointers">, + CheckerOptions<[CmdLineOption< + Boolean, "SuppressAddressSpaces", + "Suppresses warning when pointer dereferences an address space", + "true", Released>]>, + Documentation, + Dependencies<[DereferenceModeling]>; def NonNullParamChecker : Checker<"NonNullParamChecker">, HelpText<"Check for null pointers passed as arguments to a function whose " @@ -285,6 +283,12 @@ def FixedAddressChecker : Checker<"FixedAddr">, HelpText<"Check for assignment of a fixed address to a pointer">, Documentation; +def FixedAddressDereferenceChecker + : Checker<"FixedAddressDereference">, + HelpText<"Check for dereferences of fixed values used as pointers">, + Documentation, + Dependencies<[DereferenceModeling]>; + def PointerArithChecker : Checker<"PointerArithm">, HelpText<"Check for pointer arithmetic on locations other than array " "elements">, diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp index e9e2771c739b6..6a3f70e62e77b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -31,7 +31,12 @@ class DereferenceChecker : public Checker< check::Location, check::Bind, EventDispatcher > { - enum DerefKind { NullPointer, UndefinedPointerValue, AddressOfLabel }; + enum DerefKind { + NullPointer, + UndefinedPointerValue, + AddressOfLabel, + FixedAddress + }; void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S, CheckerContext &C) const; @@ -52,10 +57,12 @@ class DereferenceChecker bool SuppressAddressSpaces = false; bool CheckNullDereference = false; + bool CheckFixedDereference = false; std::unique_ptr BT_Null; std::unique_ptr BT_Undef; std::unique_ptr BT_Label; + std::unique_ptr BT_FixedAddress; }; } // end anonymous namespace @@ -155,30 +162,47 @@ static bool isDeclRefExprToReference(const Expr *E) { void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State, const Stmt *S, CheckerContext &C) const { - if (!CheckNullDereference) { - C.addSink(); - return; - }

const BugType *BT = nullptr; llvm::StringRef DerefStr1; llvm::StringRef DerefStr2; switch (K) { case DerefKind::NullPointer:

• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Null.get(); DerefStr1 = " results in a null pointer dereference"; DerefStr2 = " results in a dereference of a null pointer"; break; case DerefKind::UndefinedPointerValue:
• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Undef.get(); DerefStr1 = " results in an undefined pointer dereference"; DerefStr2 = " results in a dereference of an undefined pointer value"; break; case DerefKind::AddressOfLabel:
• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Label.get(); DerefStr1 = " results in an undefined pointer dereference"; DerefStr2 = " results in a dereference of an address of a label"; break;
• case DerefKind::FixedAddress:
• // Deliberately don't add a sink node if check is disabled.
• // This situation may be valid in special cases.
• if (!CheckFixedDereference)

•  return;

• BT = BT_FixedAddress.get();
• DerefStr1 = " results in a dereference of a fixed address";
• DerefStr2 = " results in a dereference of a fixed address";
• break;

};

// Generate an error node. @@ -289,6 +313,13 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S, } }

• if (location.isConstant()) {

• const Expr *DerefExpr = getDereferenceExpr(S, isLoad);

• if (!suppressReport(C, DerefExpr))

•  reportBug(DerefKind::FixedAddress, notNullState, DerefExpr, C);

• return;

• }

• // From this point forward, we know that the location is not null. C.addTransition(notNullState); } @@ -337,6 +368,13 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S, } }

• if (V.isConstant()) {

• const Expr *DerefExpr = getDereferenceExpr(S, true);

• if (!suppressReport(C, DerefExpr))

•  reportBug(DerefKind::FixedAddress, State, DerefExpr, C);

• return;

• }

• // Unlike a regular null dereference, initializing a reference with a // dereferenced null pointer does not actually cause a runtime exception in // Clang's implementation of references. @@ -383,3 +421,16 @@ void ento::registerNullDereferenceChecker(CheckerManager &Mgr) { bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &) { return true; }

• +void ento::registerFixedAddressDereferenceChecker(CheckerManager &Mgr) {

• auto *Chk = Mgr.getChecker();

• Chk->CheckFixedDereference = true;

• Chk->BT_FixedAddress.reset(new BugType(Mgr.getCurrentCheckerName(),

•                                     "Dereference of a fixed address",

•                                     categories::LogicError));

+} + +bool ento::shouldRegisterFixedAddressDereferenceChecker(

• const CheckerManager &) {
• return true; +} diff --git a/clang/test/Analysis/concrete-address.c b/clang/test/Analysis/concrete-address.c index 346f5093e44f7..af99aa43a98a8 100644 --- a/clang/test/Analysis/concrete-address.c +++ b/clang/test/Analysis/concrete-address.c @@ -1,7 +1,163 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -verify %s -// expected-no-diagnostics

-void foo(void) { +extern void __assert_fail (__const char *__assertion, __const char *__file,

• unsigned int __line, __const char *__function)

• __attribute__ ((__noreturn__));

• +#define assert(expr) \
• ((expr) ? (void)(0) : __assert_fail (#expr, FILE, LINE, func))
• +typedef unsigned long uintptr_t;
• +void f0(void) { int p = (int) 0x10000; // Should not crash here.

• *p = 3;

• *p = 3; // expected-warning{{Dereference of a fixed address}} +}
• +void f1(int *p) {
• if (p != (int *)-1)
• *p = 1;
• else
• *p = 0; // expected-warning{{Dereference of a fixed address}}

+} + +struct f2_struct {

• int x; +};
• +int f2(struct f2_struct* p) {
• if (p != (struct f2_struct *)1)
• p->x = 1;
• return p->x++; // expected-warning{{Access to field 'x' results in a dereference of a fixed address (loaded from variable 'p')}} +}
• +int f3_1(char* x) {
• int i = 2;
• if (x != (char *)1)
• return x[i - 1];
• return x[i+1]; // expected-warning{{Array access (from variable 'x') results in a dereference of a fixed address}} +}
• +int f3_2(char* x) {
• int i = 2;
• if (x != (char *)1)
• return x[i - 1];
• return x[i+1]++; // expected-warning{{Array access (from variable 'x') results in a dereference of a fixed address}} +}
• +int f4_1(int *p) {
• uintptr_t x = (uintptr_t) p;
• if (x != (uintptr_t)1)
• return 1;
• int q = (int) x;
• return *q; // expected-warning{{Dereference of a fixed address (loaded from variable 'q')}} +}
• +int f4_2(void) {
• short array[2];
• uintptr_t x = (uintptr_t)array;
• short *p = (short *)x;
• // The following branch should be infeasible.
• if (!(p == &array[0])) {
• p = (short *)1;
• *p = 1; // no-warning
• }
• if (p != (short *)1) {
• *p = 5; // no-warning
• p = (short *)1; // expected-warning {{Using a fixed address is not portable}}
• }
• else return 1;
• *p += 10; // expected-warning{{Dereference of a fixed}}
• return 0; +}
• +int f5(void) {
• char *s = "hello world";
• return s[0]; // no-warning +}
• +void f6(int *p, int *q) {
• if (p != (int *)1)
• if (p == (int *)1)

•  *p = 1; // no-warning

• if (q == (int *)1)
• if (q != (int *)1)

•  *q = 1; // no-warning

+} + +int* qux(int); + +int f7_1(unsigned len) {

• assert (len != 0);
• int *p = (int *)1;
• unsigned i;
• for (i = 0; i < len; ++i)
• p = qux(i);
• return *p++; // no-warning +}
• +int f7_2(unsigned len) {
• assert (len > 0); // note use of '>'
• int *p = (int *)1;
• unsigned i;
• for (i = 0; i < len; ++i)
• p = qux(i);
• return *p++; // no-warning +}
• +struct f8_s {
• int x;
• int y[2]; +};
• +void f8(struct f8_s *s, int coin) {
• if (s != (struct f8_s *)7)
• return;
• if (coin)
• s->x = 5; // expected-warning{{Access to field 'x' results in a dereference of a fixed address (loaded from variable 's')}}
• else
• s->y[1] = 6; // expected-warning{{Array access (via field 'y') results in a dereference of a fixed address}}

+} + +void f9() {

• int (p_function) (char, char) = (int ()(char, char))0x04040; // FIXME: warn at this initialization
• p_function = (int (*)(char, char))0x04080; // expected-warning {{Using a fixed address is not portable}}
• // FIXME: there should be a warning from calling the function pointer with fixed address
• int x = (*p_function) ('x', 'y'); +}
• +#define AS_ATTRIBUTE volatile attribute((address_space(256)))

+#define _get_base() ((void * AS_ATTRIBUTE )0x10) + +void test_address_space_array(unsigned long slot) {

• return _get_base()[slot]; // no-warning +} +void test_address_space_condition(int AS_ATTRIBUTE *cpu_data) {
• if (cpu_data == (int *)0x10) {
• *cpu_data = 3; // no-warning
• } +} +struct X { int member; }; +int test_address_space_member(void) {
• struct X AS_ATTRIBUTE *data = (struct X AS_ATTRIBUTE *)0x10UL;
• int ret;
• ret = data->member; // no-warning
• return ret; } diff --git a/clang/test/Analysis/misc-ps.m b/clang/test/Analysis/misc-ps.m index 0a8a30cb6175c..841f618fbdfab 100644 --- a/clang/test/Analysis/misc-ps.m +++ b/clang/test/Analysis/misc-ps.m @@ -1,6 +1,6 @@ // NOTE: Use '-fobjc-gc' to test the analysis being run twice, and multiple reports are not issued. -// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s -// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s +// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -analyzer-disable-checker=alpha.core.FixedAddressDereference -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -analyzer-disable-checker=alpha.core.FixedAddressDereference -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s

#ifndef clang_analyzer #error clang_analyzer not defined

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Kéri (balazske)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/127191.diff

5 Files Affected:

• (modified) clang/docs/analyzer/checkers.rst (+38)
• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+15-11)
• (modified) clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp (+57-6)
• (modified) clang/test/Analysis/concrete-address.c (+159-3)
• (modified) clang/test/Analysis/misc-ps.m (+2-2)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 707067358fdfe..c5e10f5a8bbf2 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -129,6 +129,8 @@ The SuppressAddressSpaces option suppresses warnings for null dereferences of all pointers with address spaces. You can disable this behavior with the option -analyzer-config core.NullDereference:SuppressAddressSpaces=false. +Value of this option is used for checker :ref:_core-FixedAddressDereference +too. Defaults to true. .. code-block:: objc @@ -2919,6 +2921,42 @@ Check for assignment of a fixed address to a pointer. p = (int *) 0x10000; // warn } +.. alpha-core-FixedAddressDereference: + +alpha.core.FixedAddressDereference (C, C++, ObjC) +""""""""""""""""""""""""""""""""""""""""""""""""" +Check for dereferences of fixed values used as pointers. + +Similarly as at :ref:_core-NullDereference, the checker specifically does +not report 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.) + +If you want to disable this behavior, set the SuppressAddressSpaces option +of checker core.NullDereference to false, like +-analyzer-config core.NullDereference:SuppressAddressSpaces=false. The value +of this option is used for both checkers. + +.. code-block:: c + + void test1() { + int *p = (int *)0x020; + int x = p[0]; // warn + } + + void test2(int *p) { + if (p == (int *)-1) + *p = 0; // warn + } + + void test3() { + int (p_function)(char, char); + p_function = (int ()(char, char))0x04080; + int x = (*p_function)('x', 'y'); // NO warning yet at functon pointer calls + } + .. _alpha-core-PointerArithm: alpha.core.PointerArithm (C) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 9bf491eac1e0e..44ca28c003b06 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -211,17 +211,15 @@ def DereferenceModeling : Checker<"DereferenceModeling">, Documentation, Hidden; -def NullDereferenceChecker : Checker<"NullDereference">, - HelpText<"Check for dereferences of null pointers">, - CheckerOptions<[ - CmdLineOption<Boolean, - "SuppressAddressSpaces", - "Suppresses warning when pointer dereferences an address space", - "true", - Released> - ]>, - Documentation, - Dependencies<[DereferenceModeling]>; +def NullDereferenceChecker + : Checker<"NullDereference">, + HelpText<"Check for dereferences of null pointers">, + CheckerOptions<[CmdLineOption< + Boolean, "SuppressAddressSpaces", + "Suppresses warning when pointer dereferences an address space", + "true", Released>]>, + Documentation, + Dependencies<[DereferenceModeling]>; def NonNullParamChecker : Checker<"NonNullParamChecker">, HelpText<"Check for null pointers passed as arguments to a function whose " @@ -285,6 +283,12 @@ def FixedAddressChecker : Checker<"FixedAddr">, HelpText<"Check for assignment of a fixed address to a pointer">, Documentation; +def FixedAddressDereferenceChecker + : Checker<"FixedAddressDereference">, + HelpText<"Check for dereferences of fixed values used as pointers">, + Documentation, + Dependencies<[DereferenceModeling]>; + def PointerArithChecker : Checker<"PointerArithm">, HelpText<"Check for pointer arithmetic on locations other than array " "elements">, diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp index e9e2771c739b6..6a3f70e62e77b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -31,7 +31,12 @@ class DereferenceChecker : public Checker< check::Location, check::Bind, EventDispatcher > { - enum DerefKind { NullPointer, UndefinedPointerValue, AddressOfLabel }; + enum DerefKind { + NullPointer, + UndefinedPointerValue, + AddressOfLabel, + FixedAddress + }; void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S, CheckerContext &C) const; @@ -52,10 +57,12 @@ class DereferenceChecker bool SuppressAddressSpaces = false; bool CheckNullDereference = false; + bool CheckFixedDereference = false; std::unique_ptr BT_Null; std::unique_ptr BT_Undef; std::unique_ptr BT_Label; + std::unique_ptr BT_FixedAddress; }; } // end anonymous namespace @@ -155,30 +162,47 @@ static bool isDeclRefExprToReference(const Expr *E) { void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State, const Stmt *S, CheckerContext &C) const { - if (!CheckNullDereference) { - C.addSink(); - return; - }

const BugType *BT = nullptr; llvm::StringRef DerefStr1; llvm::StringRef DerefStr2; switch (K) { case DerefKind::NullPointer:

• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Null.get(); DerefStr1 = " results in a null pointer dereference"; DerefStr2 = " results in a dereference of a null pointer"; break; case DerefKind::UndefinedPointerValue:
• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Undef.get(); DerefStr1 = " results in an undefined pointer dereference"; DerefStr2 = " results in a dereference of an undefined pointer value"; break; case DerefKind::AddressOfLabel:
• if (!CheckNullDereference) {

•  C.addSink();

•  return;

• } BT = BT_Label.get(); DerefStr1 = " results in an undefined pointer dereference"; DerefStr2 = " results in a dereference of an address of a label"; break;
• case DerefKind::FixedAddress:
• // Deliberately don't add a sink node if check is disabled.
• // This situation may be valid in special cases.
• if (!CheckFixedDereference)

•  return;

• BT = BT_FixedAddress.get();
• DerefStr1 = " results in a dereference of a fixed address";
• DerefStr2 = " results in a dereference of a fixed address";
• break;

};

// Generate an error node. @@ -289,6 +313,13 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S, } }

• if (location.isConstant()) {

• const Expr *DerefExpr = getDereferenceExpr(S, isLoad);

• if (!suppressReport(C, DerefExpr))

•  reportBug(DerefKind::FixedAddress, notNullState, DerefExpr, C);

• return;

• }

• // From this point forward, we know that the location is not null. C.addTransition(notNullState); } @@ -337,6 +368,13 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S, } }

• if (V.isConstant()) {

• const Expr *DerefExpr = getDereferenceExpr(S, true);

• if (!suppressReport(C, DerefExpr))

•  reportBug(DerefKind::FixedAddress, State, DerefExpr, C);

• return;

• }

• // Unlike a regular null dereference, initializing a reference with a // dereferenced null pointer does not actually cause a runtime exception in // Clang's implementation of references. @@ -383,3 +421,16 @@ void ento::registerNullDereferenceChecker(CheckerManager &Mgr) { bool ento::shouldRegisterNullDereferenceChecker(const CheckerManager &) { return true; }

• +void ento::registerFixedAddressDereferenceChecker(CheckerManager &Mgr) {

• auto *Chk = Mgr.getChecker();

• Chk->CheckFixedDereference = true;

• Chk->BT_FixedAddress.reset(new BugType(Mgr.getCurrentCheckerName(),

•                                     "Dereference of a fixed address",

•                                     categories::LogicError));

+} + +bool ento::shouldRegisterFixedAddressDereferenceChecker(

• const CheckerManager &) {
• return true; +} diff --git a/clang/test/Analysis/concrete-address.c b/clang/test/Analysis/concrete-address.c index 346f5093e44f7..af99aa43a98a8 100644 --- a/clang/test/Analysis/concrete-address.c +++ b/clang/test/Analysis/concrete-address.c @@ -1,7 +1,163 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -verify %s -// expected-no-diagnostics

-void foo(void) { +extern void __assert_fail (__const char *__assertion, __const char *__file,

• unsigned int __line, __const char *__function)

• __attribute__ ((__noreturn__));

• +#define assert(expr) \
• ((expr) ? (void)(0) : __assert_fail (#expr, FILE, LINE, func))
• +typedef unsigned long uintptr_t;
• +void f0(void) { int p = (int) 0x10000; // Should not crash here.

• *p = 3;

• *p = 3; // expected-warning{{Dereference of a fixed address}} +}
• +void f1(int *p) {
• if (p != (int *)-1)
• *p = 1;
• else
• *p = 0; // expected-warning{{Dereference of a fixed address}}

+} + +struct f2_struct {

• int x; +};
• +int f2(struct f2_struct* p) {
• if (p != (struct f2_struct *)1)
• p->x = 1;
• return p->x++; // expected-warning{{Access to field 'x' results in a dereference of a fixed address (loaded from variable 'p')}} +}
• +int f3_1(char* x) {
• int i = 2;
• if (x != (char *)1)
• return x[i - 1];
• return x[i+1]; // expected-warning{{Array access (from variable 'x') results in a dereference of a fixed address}} +}
• +int f3_2(char* x) {
• int i = 2;
• if (x != (char *)1)
• return x[i - 1];
• return x[i+1]++; // expected-warning{{Array access (from variable 'x') results in a dereference of a fixed address}} +}
• +int f4_1(int *p) {
• uintptr_t x = (uintptr_t) p;
• if (x != (uintptr_t)1)
• return 1;
• int q = (int) x;
• return *q; // expected-warning{{Dereference of a fixed address (loaded from variable 'q')}} +}
• +int f4_2(void) {
• short array[2];
• uintptr_t x = (uintptr_t)array;
• short *p = (short *)x;
• // The following branch should be infeasible.
• if (!(p == &array[0])) {
• p = (short *)1;
• *p = 1; // no-warning
• }
• if (p != (short *)1) {
• *p = 5; // no-warning
• p = (short *)1; // expected-warning {{Using a fixed address is not portable}}
• }
• else return 1;
• *p += 10; // expected-warning{{Dereference of a fixed}}
• return 0; +}
• +int f5(void) {
• char *s = "hello world";
• return s[0]; // no-warning +}
• +void f6(int *p, int *q) {
• if (p != (int *)1)
• if (p == (int *)1)

•  *p = 1; // no-warning

• if (q == (int *)1)
• if (q != (int *)1)

•  *q = 1; // no-warning

+} + +int* qux(int); + +int f7_1(unsigned len) {

• assert (len != 0);
• int *p = (int *)1;
• unsigned i;
• for (i = 0; i < len; ++i)
• p = qux(i);
• return *p++; // no-warning +}
• +int f7_2(unsigned len) {
• assert (len > 0); // note use of '>'
• int *p = (int *)1;
• unsigned i;
• for (i = 0; i < len; ++i)
• p = qux(i);
• return *p++; // no-warning +}
• +struct f8_s {
• int x;
• int y[2]; +};
• +void f8(struct f8_s *s, int coin) {
• if (s != (struct f8_s *)7)
• return;
• if (coin)
• s->x = 5; // expected-warning{{Access to field 'x' results in a dereference of a fixed address (loaded from variable 's')}}
• else
• s->y[1] = 6; // expected-warning{{Array access (via field 'y') results in a dereference of a fixed address}}

+} + +void f9() {

• int (p_function) (char, char) = (int ()(char, char))0x04040; // FIXME: warn at this initialization
• p_function = (int (*)(char, char))0x04080; // expected-warning {{Using a fixed address is not portable}}
• // FIXME: there should be a warning from calling the function pointer with fixed address
• int x = (*p_function) ('x', 'y'); +}
• +#define AS_ATTRIBUTE volatile attribute((address_space(256)))

+#define _get_base() ((void * AS_ATTRIBUTE )0x10) + +void test_address_space_array(unsigned long slot) {

• return _get_base()[slot]; // no-warning +} +void test_address_space_condition(int AS_ATTRIBUTE *cpu_data) {
• if (cpu_data == (int *)0x10) {
• *cpu_data = 3; // no-warning
• } +} +struct X { int member; }; +int test_address_space_member(void) {
• struct X AS_ATTRIBUTE *data = (struct X AS_ATTRIBUTE *)0x10UL;
• int ret;
• ret = data->member; // no-warning
• return ret; } diff --git a/clang/test/Analysis/misc-ps.m b/clang/test/Analysis/misc-ps.m index 0a8a30cb6175c..841f618fbdfab 100644 --- a/clang/test/Analysis/misc-ps.m +++ b/clang/test/Analysis/misc-ps.m @@ -1,6 +1,6 @@ // NOTE: Use '-fobjc-gc' to test the analysis being run twice, and multiple reports are not issued. -// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s -// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s +// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -analyzer-disable-checker=alpha.core.FixedAddressDereference -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core,osx.cocoa.AtSync -analyzer-disable-checker=alpha.core.FixedAddressDereference -Wno-strict-prototypes -Wno-pointer-to-int-cast -verify -fblocks -Wno-unreachable-code -Wno-null-dereference -Wno-objc-root-class %s

#ifndef clang_analyzer #error clang_analyzer not defined

The checker is alpha because there are known problems with it which I plan to fix later.
A problem is with the bugpath messages where a constant pointer was assumed to be a null pointer but after this checker (or even before it?) this is not true.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the impression that the core dereference checker already checks this.

It may cause confusion that "NullDereference" checker checks not only null dereference but undefined pointer and label address too. Probably these checks (specially label address) can be moved into this checker. (Or add the new check to NullDereference without a new checker?)

It may cause confusion that "NullDereference" checker checks not only null dereference but undefined pointer and label address too. Probably these checks (specially label address) can be moved into this checker. (Or add the new check to NullDereference without a new checker?)

I see. Thanks.
I dont have a firm opinion on the architecture. Usually we should go with what is more natural. If there would be signifficat semantic sharing, then a single checker works usually better. Its up to you.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Thank you!

I've marked the resolved comments. There are a few unresolved conversations left.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with your PR. I think it looks great.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like the change, but I feel that the analyzer option name suppress-all-address-spaces is too misleading (see inline comment for more detailed suggestion).

Comment on lines 165 to 168

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to mention "(the default value)" immediately when you describe it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -verify argument can accept multiple comma-separated labels and this can be used to simplify the code in cases where the same warning is emitted in many run lines. For example if you add the same string (e.g. common as in the suggested edit), you can write common-warning {{....}} instead of separately listing each of the four different labels.

Comment on lines 2947 to 2950

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same paragraph that I suggested for the NullDereference checker.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this analyzer option is misleading: the natural guess is that it makes the analyzer ignore the address space annotations -- and it's completely counter-intuitive that it suppresses certain results that come from two particular checkers. Of course the attached documentation describes its meaning, but I don't think that's enough (especially considering that these option docs are currently very obscure -- e.g. they don't appear on the webpage.

Note that this name was already misleading when it belonged to just a checker option, but moving it into the global namespace exacerbates the problem.

I know that it's difficult to find a more accurate name, but perhaps something like ignore-deref-from-all-address-spaces could work.

@steakhal What do you think about this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that "ignore-all-address-spaces" would be really problematic but "suppress" is somehow related to eliminate reporting of specific errors (or path suppression). Probably "suppress-dereferences-from-any-address-space" is better.

Labels