From e176bb10607c5321edf5facc6f99056d7164fe40 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 21:19:34 +0000 Subject: [PATCH 1/6] Remove redundant import --- c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index 4ae6619227..7974c4d601 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -15,7 +15,6 @@ import cpp import codingstandards.c.cert import codingstandards.cpp.Macro import codingstandards.cpp.SideEffect -import codingstandards.cpp.StructuralEquivalence import codingstandards.cpp.sideeffect.DefaultEffects import codingstandards.cpp.sideeffect.Customizations import semmle.code.cpp.valuenumbering.HashCons From 92f5d5b8ea4d37141200dc6b563c197e2bb1c5d4 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 22:32:50 +0000 Subject: [PATCH 2/6] A5-1-9: Address param confusion The hash cons value for parameters was incorrectly calculated with parameter uses (e.g accesses to the parameter). The correct approach is to use the variable name and type. This caused performance issues, because the hash cons for a function was made up of all combinations of the accesses to the parameters. For lambdas with many parameters and many accesses, this was problematic. --- .../src/rules/A5-1-9/LambdaEquivalence.qll | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll b/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll index 040777e321..c9ea4a0fd8 100644 --- a/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll +++ b/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll @@ -211,7 +211,7 @@ private module HashCons { private newtype HC_Params = HC_NoParams() or - HC_ParamCons(HashConsExpr hc, int i, HC_Params list) { mk_ParamCons(hc, i, list, _) } + HC_ParamCons(Type t, string name, int i, HC_Params list) { mk_ParamCons(t, name, i, list, _) } /** * HashConsExpr is the hash-cons of an expression. The relationship between `Expr` @@ -1275,13 +1275,14 @@ private module HashCons { mk_DeclConsInner(_, _, s.getNumDeclarations() - 1, hc, s) } - private predicate mk_ParamCons(HashConsExpr hc, int i, HC_Params list, Function f) { - hc = hashConsExpr(f.getParameter(i).getAnAccess()) and - ( - exists(HashConsExpr head, HC_Params tail | - mk_ParamConsInner(head, tail, i - 1, list, f) and - i > 0 - ) + private predicate mk_ParamCons(Type t, string name, int i, HC_Params list, Function f) { + exists(Parameter p | + p = f.getParameter(i) and + t = p.getType() and + name = p.getName() + | + mk_ParamConsInner(_, _, _, i - 1, list, f) and + i > 0 or i = 0 and list = HC_NoParams() @@ -1289,10 +1290,10 @@ private module HashCons { } private predicate mk_ParamConsInner( - HashConsExpr head, HC_Params tail, int i, HC_Params list, Function f + Type t, string name, HC_Params tail, int i, HC_Params list, Function f ) { - list = HC_ParamCons(head, i, tail) and - mk_ParamCons(head, i, tail, f) + list = HC_ParamCons(t, name, i, tail) and + mk_ParamCons(t, name, i, tail, f) } private predicate mk_FunctionCons( @@ -1302,7 +1303,7 @@ private module HashCons { name = f.getName() and body = hashConsStmt(f.getBlock()) and if f.getNumberOfParameters() > 0 - then mk_ParamConsInner(_, _, f.getNumberOfParameters() - 1, params, f) + then mk_ParamConsInner(_, _, _, f.getNumberOfParameters() - 1, params, f) else params = HC_NoParams() } From a568b887fda9945a2f40984a2034a23e9438f829 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 23:09:05 +0000 Subject: [PATCH 3/6] A5-1-9: Filter blocks to lambdas in hash cons calc Move the exclusion of non-lambda blocks to the calculation of HC_BlockStmt, to avoid generating newtype instances for non-lambda instances. --- .../src/rules/A5-1-9/LambdaEquivalence.qll | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll b/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll index c9ea4a0fd8..cab93608c5 100644 --- a/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll +++ b/cpp/autosar/src/rules/A5-1-9/LambdaEquivalence.qll @@ -624,11 +624,21 @@ private module HashCons { strictcount(access.getTarget()) = 1 } + /** + * Gets the name of a variable. + * + * Extracted for performance reasons, to avoid magic, which was causing performance issues in getParameter(int i). + */ + pragma[nomagic] + private string getVariableName(Variable v) { result = v.getName() } + /* Note: This changed from the original HashCons module to be able to find structural equivalent expression. */ private predicate mk_Variable(Type t, string name, VariableAccess access) { analyzableVariable(access) and exists(Variable v | - v = access.getTarget() and t = v.getUnspecifiedType() and name = v.getName() + v = access.getTarget() and + t = v.getUnspecifiedType() and + name = getVariableName(v) ) } @@ -1104,7 +1114,14 @@ private module HashCons { nee.getExpr().getFullyConverted() = child.getAnExpr() } - private predicate mk_StmtCons(HashConsStmt hc, int i, HC_Stmts list, BlockStmt block) { + private class LambdaBlockStmt extends BlockStmt { + LambdaBlockStmt() { + // Restricting to statements inside a lambda expressions. + this.getParentScope*() = any(LambdaExpression le).getLambdaFunction() + } + } + + private predicate mk_StmtCons(HashConsStmt hc, int i, HC_Stmts list, LambdaBlockStmt block) { hc = hashConsStmt(block.getStmt(i)) and ( exists(HashConsStmt head, HC_Stmts tail | @@ -1118,13 +1135,13 @@ private module HashCons { } private predicate mk_StmtConsInner( - HashConsStmt head, HC_Stmts tail, int i, HC_Stmts list, BlockStmt block + HashConsStmt head, HC_Stmts tail, int i, HC_Stmts list, LambdaBlockStmt block ) { list = HC_StmtCons(head, i, tail) and mk_StmtCons(head, i, tail, block) } - private predicate mk_BlockStmtCons(HC_Stmts hc, BlockStmt s) { + private predicate mk_BlockStmtCons(HC_Stmts hc, LambdaBlockStmt s) { if s.getNumStmt() > 0 then exists(HashConsStmt head, HC_Stmts tail | @@ -1487,8 +1504,6 @@ private module HashCons { cached HashConsStmt hashConsStmt(Stmt s) { - // Restricting to statements inside a lambda expressions. - s.getParentScope*() = any(LambdaExpression le).getLambdaFunction() and exists(HC_Stmts list | mk_BlockStmtCons(list, s) and result = HC_BlockStmt(list) From c1d45c3411d23501c205bea8c6a521426187b4b9 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 23:49:13 +0000 Subject: [PATCH 4/6] A5-1-9: Exclude duplication through macros --- .../src/rules/A5-1-9/IdenticalLambdaExpressions.ql | 10 +++++++++- cpp/autosar/test/rules/A5-1-9/test.cpp | 8 +++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/cpp/autosar/src/rules/A5-1-9/IdenticalLambdaExpressions.ql b/cpp/autosar/src/rules/A5-1-9/IdenticalLambdaExpressions.ql index 8717fd000e..1520955716 100644 --- a/cpp/autosar/src/rules/A5-1-9/IdenticalLambdaExpressions.ql +++ b/cpp/autosar/src/rules/A5-1-9/IdenticalLambdaExpressions.ql @@ -24,6 +24,14 @@ where not lambdaExpression = otherLambdaExpression and not lambdaExpression.isFromTemplateInstantiation(_) and not otherLambdaExpression.isFromTemplateInstantiation(_) and - getLambdaHashCons(lambdaExpression) = getLambdaHashCons(otherLambdaExpression) + getLambdaHashCons(lambdaExpression) = getLambdaHashCons(otherLambdaExpression) and + // Do not report lambdas produced by the same macro in different invocations + not exists(Macro m, MacroInvocation m1, MacroInvocation m2 | + m1 = m.getAnInvocation() and + m2 = m.getAnInvocation() and + not m1 = m2 and // Lambdas in the same macro can be reported + m1.getAnExpandedElement() = lambdaExpression and + m2.getAnExpandedElement() = otherLambdaExpression + ) select lambdaExpression, "Lambda expression is identical to $@ lambda expression.", otherLambdaExpression, "this" diff --git a/cpp/autosar/test/rules/A5-1-9/test.cpp b/cpp/autosar/test/rules/A5-1-9/test.cpp index 466cf14dfa..511be302a0 100644 --- a/cpp/autosar/test/rules/A5-1-9/test.cpp +++ b/cpp/autosar/test/rules/A5-1-9/test.cpp @@ -104,4 +104,10 @@ class Test_issue468 { LogError("Error"); LogFatal("Fatal"); } -}; \ No newline at end of file +}; + +#define MACRO() [](int i) -> int { return i + 3; } +void test_macros() { + MACRO(); // COMPLIANT + MACRO(); // COMPLIANT - no duplication +} \ No newline at end of file From 2d8beac88da84e404bddf654f53867934088b305 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 23:52:18 +0000 Subject: [PATCH 5/6] Add change note --- change_notes/2025-02-10-improve-perf-a5-1-9.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change_notes/2025-02-10-improve-perf-a5-1-9.md diff --git a/change_notes/2025-02-10-improve-perf-a5-1-9.md b/change_notes/2025-02-10-improve-perf-a5-1-9.md new file mode 100644 index 0000000000..ae7ea6f240 --- /dev/null +++ b/change_notes/2025-02-10-improve-perf-a5-1-9.md @@ -0,0 +1,2 @@ + - `A5-1-9` - `IdenticalLambdaExpressions.ql`: + - Performance has been improved. \ No newline at end of file From 1bd7dab926f5c94f13c3685e240af3e41385d01c Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 10 Feb 2025 23:53:26 +0000 Subject: [PATCH 6/6] Include fp update in change note --- change_notes/2025-02-10-improve-perf-a5-1-9.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change_notes/2025-02-10-improve-perf-a5-1-9.md b/change_notes/2025-02-10-improve-perf-a5-1-9.md index ae7ea6f240..5d355bab49 100644 --- a/change_notes/2025-02-10-improve-perf-a5-1-9.md +++ b/change_notes/2025-02-10-improve-perf-a5-1-9.md @@ -1,2 +1,3 @@ - `A5-1-9` - `IdenticalLambdaExpressions.ql`: - - Performance has been improved. \ No newline at end of file + - Performance has been improved. + - False positives due to repeated invocation of macros containing lambdas have been excluded. \ No newline at end of file