Skip to content

Commit f1c2ebd

Browse files
lcarteylcartey
authored
Merge pull request #240 from github/jsinglet/1224-1150-1223-1151
Fixes for #1224, 1150, 1223, 1151 compiler compatibility issues
2 parents 170095a + 151930c commit f1c2ebd

12 files changed

+110
-48
lines changed

‎c/cert/src/rules/CON40-C/AtomicVariableTwiceInExpression.ql

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import cpp
1616
import codingstandards.c.cert
17+
import codingstandards.cpp.Concurrency
1718

1819
from MacroInvocation mi, Variable v, Locatable whereFound
1920
where
@@ -22,13 +23,13 @@ where
2223
// There isn't a way to safely use this construct in a way that is also
2324
// possible the reliably detect so advise against using it.
2425
(
25-
mi.getMacroName() = ["atomic_store", "atomic_store_explicit"]
26+
mi instanceof AtomicStore
2627
or
2728
// This construct is generally safe, but must be used in a loop. To lower
2829
// the false positive rate we don't look at the conditions of the loop and
2930
// instead assume if it is found in a looping construct that it is likely
3031
// related to the safety property.
31-
mi.getMacroName() = ["atomic_compare_exchange_weak", "atomic_compare_exchange_weak_explicit"] and
32+
mi instanceof AtomicCompareExchange and
3233
not exists(Loop l | mi.getAGeneratedElement().(Expr).getParent*() = l)
3334
) and
3435
whereFound = mi

‎c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -12,28 +12,18 @@
1212
* external/cert/obligation/rule
1313
*/
1414

15-
import cpp
16-
import codingstandards.c.cert
15+
import cpp
16+
import codingstandards.c.cert
17+
import codingstandards.cpp.Concurrency
18+
1719

18-
/**
19-
* Models calls to routines in the `stdatomic` library. Note that these
20-
* are typically implemented as macros within Clang and GCC's standard
21-
* libraries.
22-
*/
23-
class SpuriouslyFailingFunctionCallType extends MacroInvocation {
24-
SpuriouslyFailingFunctionCallType() {
25-
getMacroName() = ["atomic_compare_exchange_weak", "atomic_compare_exchange_weak_explicit"]
26-
}
27-
}
28-
29-
from SpuriouslyFailingFunctionCallType fc
30-
where
31-
not isExcluded(fc, Concurrency3Package::wrapFunctionsThatCanFailSpuriouslyInLoopQuery()) and
32-
(
33-
exists(StmtParent sp | sp = fc.getStmt() and not sp.(Stmt).getParentStmt*() instanceof Loop)
34-
or
35-
exists(StmtParent sp |
36-
sp = fc.getExpr() and not sp.(Expr).getEnclosingStmt().getParentStmt*() instanceof Loop
37-
)
38-
)
39-
select fc, "Function that can spuriously fail not wrapped in a loop."
20+
from AtomicCompareExchange ace
21+
where
22+
not isExcluded(ace, Concurrency3Package::wrapFunctionsThatCanFailSpuriouslyInLoopQuery()) and
23+
(
24+
forex(StmtParent sp | sp = ace.getStmt() | not sp.(Stmt).getParentStmt*() instanceof Loop) or
25+
forex(Expr e | e = ace.getExpr() | not e.getEnclosingStmt().getParentStmt*()
26+
instanceof Loop)
27+
)
28+
select ace, "Function that can spuriously fail not wrapped in a loop."
29+

‎c/cert/test/rules/CON40-C/AtomicVariableTwiceInExpression.expected

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
| test.c:6:19:6:40 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:32:3:32:10 | ... += ... | expression |
2-
| test.c:6:19:6:40 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:33:3:33:13 | ... = ... | expression |
3-
| test.c:10:3:10:23 | atomic_store(a,b) | Atomic variable possibly referred to twice in an $@. | test.c:10:3:10:23 | atomic_store(a,b) | expression |
4-
| test.c:11:3:11:35 | atomic_store_explicit(a,b,c) | Atomic variable possibly referred to twice in an $@. | test.c:11:3:11:35 | atomic_store_explicit(a,b,c) | expression |
5-
| test.c:24:3:24:48 | atomic_compare_exchange_weak(a,b,c) | Atomic variable possibly referred to twice in an $@. | test.c:24:3:24:48 | atomic_compare_exchange_weak(a,b,c) | expression |
6-
| test.c:25:3:26:45 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Atomic variable possibly referred to twice in an $@. | test.c:25:3:26:45 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | expression |
1+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:33:3:33:10 | ... += ... | expression |
2+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:34:3:34:13 | ... = ... | expression |
3+
| test.c:11:3:11:23 | atomic_store(a,b) | Atomic variable possibly referred to twice in an $@. | test.c:11:3:11:23 | atomic_store(a,b) | expression |
4+
| test.c:12:3:12:35 | atomic_store_explicit(a,b,c) | Atomic variable possibly referred to twice in an $@. | test.c:12:3:12:35 | atomic_store_explicit(a,b,c) | expression |
5+
| test.c:25:3:25:49 | atomic_compare_exchange_weak(a,b,c) | Atomic variable possibly referred to twice in an $@. | test.c:25:3:25:49 | atomic_compare_exchange_weak(a,b,c) | expression |
6+
| test.c:26:3:27:42 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Atomic variable possibly referred to twice in an $@. | test.c:26:3:27:42 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | expression |

‎c/cert/test/rules/CON40-C/AtomicVariableTwiceInExpression.expected.clang

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:33:3:33:10 | ... += ... | expression |
2+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(value) | Atomic variable possibly referred to twice in an $@. | test.c:34:3:34:13 | ... = ... | expression |
3+
| test.c:11:3:11:23 | atomic_store(object,desired) | Atomic variable possibly referred to twice in an $@. | test.c:11:3:11:23 | atomic_store(object,desired) | expression |
4+
| test.c:12:3:12:23 | atomic_store_explicit | Atomic variable possibly referred to twice in an $@. | test.c:12:3:12:23 | atomic_store_explicit | expression |
5+
| test.c:25:3:25:49 | atomic_compare_exchange_weak(object,expected,desired) | Atomic variable possibly referred to twice in an $@. | test.c:25:3:25:49 | atomic_compare_exchange_weak(object,expected,desired) | expression |
6+
| test.c:26:3:26:39 | atomic_compare_exchange_weak_explicit | Atomic variable possibly referred to twice in an $@. | test.c:26:3:26:39 | atomic_compare_exchange_weak_explicit | expression |

‎c/cert/test/rules/CON40-C/AtomicVariableTwiceInExpression.expected.gcc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(VALUE) | Atomic variable possibly referred to twice in an $@. | test.c:33:3:33:10 | ... += ... | expression |
2+
| test.c:7:18:7:39 | ATOMIC_VAR_INIT(VALUE) | Atomic variable possibly referred to twice in an $@. | test.c:34:3:34:13 | ... = ... | expression |
3+
| test.c:11:3:11:23 | atomic_store(PTR,VAL) | Atomic variable possibly referred to twice in an $@. | test.c:11:3:11:23 | atomic_store(PTR,VAL) | expression |
4+
| test.c:12:3:12:35 | atomic_store_explicit(PTR,VAL,MO) | Atomic variable possibly referred to twice in an $@. | test.c:12:3:12:35 | atomic_store_explicit(PTR,VAL,MO) | expression |
5+
| test.c:25:3:25:49 | atomic_compare_exchange_weak(PTR,VAL,DES) | Atomic variable possibly referred to twice in an $@. | test.c:25:3:25:49 | atomic_compare_exchange_weak(PTR,VAL,DES) | expression |
6+
| test.c:26:3:27:42 | atomic_compare_exchange_weak_explicit(PTR,VAL,DES,SUC,FAIL) | Atomic variable possibly referred to twice in an $@. | test.c:26:3:27:42 | atomic_compare_exchange_weak_explicit(PTR,VAL,DES,SUC,FAIL) | expression |

‎c/cert/test/rules/CON40-C/test.c

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
#include <stdatomic.h>
22
#include <stdbool.h>
33

4-
static bool fl1 = ATOMIC_VAR_INIT(false);
5-
static bool fl2 = ATOMIC_VAR_INIT(false);
6-
static bool fl3 = ATOMIC_VAR_INIT(false);
7-
static bool fl4 = ATOMIC_VAR_INIT(false);
4+
static _Atomic int fl1 = ATOMIC_VAR_INIT(false);
5+
static _Atomic int fl2 = ATOMIC_VAR_INIT(false);
6+
static int fl2a = ATOMIC_VAR_INIT(false);
7+
static int fl3 = ATOMIC_VAR_INIT(false);
8+
static int fl4 = ATOMIC_VAR_INIT(false);
89

910
void f1() {
1011
atomic_store(&fl1, 0); // NON_COMPLIANT
@@ -13,17 +14,17 @@ void f1() {
1314

1415
void f2() {
1516
do {
16-
} while (!atomic_compare_exchange_weak(&fl2, &fl2, &fl2)); // COMPLIANT
17+
} while (!atomic_compare_exchange_weak(&fl2, &fl2a, fl2a)); // COMPLIANT
1718

1819
do {
19-
} while (!atomic_compare_exchange_weak_explicit(&fl2, &fl2, &fl2, &fl2,
20-
&fl2)); // COMPLIANT
20+
} while (!atomic_compare_exchange_weak_explicit(&fl2, &fl2a, fl2a, 0,
21+
0)); // COMPLIANT
2122
}
2223

2324
void f3() {
24-
atomic_compare_exchange_weak(&fl2, &fl2, &fl2); // NON_COMPLIANT
25-
atomic_compare_exchange_weak_explicit(&fl2, &fl2, &fl2, &fl2,
26-
&fl2); // NON_COMPLIANT
25+
atomic_compare_exchange_weak(&fl2, &fl2a, fl2a); // NON_COMPLIANT
26+
atomic_compare_exchange_weak_explicit(&fl2, &fl2a, fl2a, 0,
27+
0); // NON_COMPLIANT
2728
}
2829

2930
void f4() { fl3 ^= true; }

‎c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
| test.c:5:8:5:46 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. |
2-
| test.c:9:3:9:41 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. |
3-
| test.c:11:8:12:47 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. |
4-
| test.c:16:3:16:56 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. |
1+
| test.c:6:8:6:46 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. |
2+
| test.c:10:3:10:41 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. |
3+
| test.c:12:8:13:47 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. |
4+
| test.c:17:3:17:56 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. |

‎c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected.clang

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
| test.c:6:8:6:46 | atomic_compare_exchange_weak(object,expected,desired) | Function that can spuriously fail not wrapped in a loop. |
2+
| test.c:10:3:10:41 | atomic_compare_exchange_weak(object,expected,desired) | Function that can spuriously fail not wrapped in a loop. |
3+
| test.c:12:8:12:44 | atomic_compare_exchange_weak_explicit | Function that can spuriously fail not wrapped in a loop. |
4+
| test.c:17:3:17:39 | atomic_compare_exchange_weak_explicit | Function that can spuriously fail not wrapped in a loop. |

‎c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected.gcc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
| test.c:6:8:6:46 | atomic_compare_exchange_weak(PTR,VAL,DES) | Function that can spuriously fail not wrapped in a loop. |
2+
| test.c:10:3:10:41 | atomic_compare_exchange_weak(PTR,VAL,DES) | Function that can spuriously fail not wrapped in a loop. |
3+
| test.c:12:8:13:47 | atomic_compare_exchange_weak_explicit(PTR,VAL,DES,SUC,FAIL) | Function that can spuriously fail not wrapped in a loop. |
4+
| test.c:17:3:17:56 | atomic_compare_exchange_weak_explicit(PTR,VAL,DES,SUC,FAIL) | Function that can spuriously fail not wrapped in a loop. |

‎c/cert/test/rules/CON41-C/test.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
#include "stdatomic.h"
22

33
void f1() {
4-
int a, b, c;
4+
_Atomic int a;
5+
int b, c;
56
if (!atomic_compare_exchange_weak(&a, &b, c)) { // NON_COMPLIANT
67
(void)0; /* no-op */
78
}
@@ -17,7 +18,8 @@ void f1() {
1718
}
1819

1920
void f2() {
20-
int a, b, c;
21+
_Atomic int a;
22+
int b, c;
2123
while (1 == 1) {
2224
if (!atomic_compare_exchange_weak(&a, &b, c)) { // COMPLIANT
2325
(void)0; /* no-op */

‎change_notes/2023-03-06-better-modeling-of-stdatomic.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
- `CON41-C`: Refactored to address compiler compatibility issues. More accurate
2+
modeling of cases where macros are modeled against other macros such as
3+
`atomic_compare_exchange_weak` and `atomic_store`.
4+
- `CON40-C`: Refactored to address compiler compatibility issues. More accurate
5+
modeling of cases where macros are modeled against other macros such as
6+
`atomic_compare_exchange_weak` and `atomic_store`.

‎cpp/common/src/codingstandards/cpp/Concurrency.qll

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -876,3 +876,45 @@ predicate getAThreadSpecificStorageDeallocationCall(C11ThreadCreateCall tcc, Dea
876876
DataFlow::localFlow(DataFlow::exprNode(tsg), DataFlow::exprNode(dexp.getFreedExpr()))
877877
)
878878
}
879+
880+
/**
881+
* Models calls to routines `atomic_compare_exchange_weak` and
882+
* `atomic_compare_exchange_weak_explicit` in the `stdatomic` library.
883+
* Note that these are typically implemented as macros within Clang and
884+
* GCC's standard libraries.
885+
*/
886+
class AtomicCompareExchange extends MacroInvocation {
887+
AtomicCompareExchange() {
888+
getMacroName() = "atomic_compare_exchange_weak"
889+
or
890+
// some compilers model `atomic_compare_exchange_weak` as a macro that
891+
// expands to `atomic_compare_exchange_weak_explicit` so this defeats that
892+
// and other similar modeling.
893+
getMacroName() = "atomic_compare_exchange_weak_explicit" and
894+
not exists(MacroInvocation m |
895+
m.getMacroName() = "atomic_compare_exchange_weak" and
896+
m.getAnExpandedElement() = getAnExpandedElement()
897+
)
898+
}
899+
}
900+
901+
/**
902+
* Models calls to routines `atomic_store` and
903+
* `atomic_store_explicit` in the `stdatomic` library.
904+
* Note that these are typically implemented as macros within Clang and
905+
* GCC's standard libraries.
906+
*/
907+
class AtomicStore extends MacroInvocation {
908+
AtomicStore() {
909+
getMacroName() = "atomic_store"
910+
or
911+
// some compilers model `atomic_compare_exchange_weak` as a macro that
912+
// expands to `atomic_compare_exchange_weak_explicit` so this defeats that
913+
// and other similar modeling.
914+
getMacroName() = "atomic_store_explicit" and
915+
not exists(MacroInvocation m |
916+
m.getMacroName() = "atomic_store" and
917+
m.getAnExpandedElement() = getAnExpandedElement()
918+
)
919+
}
920+
}

0 commit comments

Comments
 (0)