[PATCH i-g-t 14/33] cocci: Add report and patch modes
Lyude
lyude at redhat.com
Fri Jun 7 23:08:02 UTC 2019
From: Lyude Paul <lyude at redhat.com>
Currently the main SmPL file we use, igt.cocci, is only able to generate
diffs for errors that are discovered. However, there's plenty of
usecases where a diff is nowhere near as useful as a simple list of
errors. Also, it's far faster then diff mode.
So, in order to implement a report-only mode we add support for two
different virtual SmPL rules: report and patch. Using these allows us to
toggle specific SmPL rules to only be applied during patch orreport
mode. From there, we go through and make all existing rules depend on
patch. Then, we go through and add a corresponding report mode rule and
python script for each patch rule we have.
Similarly to the Linux kernel, we have two different severities of
reported issues: WARNING, and ERROR. Eventually, we'll be able to hook
this up to our Gitlab CI to fail CI checks when errors are found with
high confidence rules.
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
lib/igt.cocci | 314 ++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 294 insertions(+), 20 deletions(-)
diff --git a/lib/igt.cocci b/lib/igt.cocci
index c02d85b0..8ba294cd 100644
--- a/lib/igt.cocci
+++ b/lib/igt.cocci
@@ -1,13 +1,38 @@
// Semantic patch for common patterns and their replacement by igt
-// infrastructure and macros. Please run with
-//
-// spatch --in-place --dir tests/
-//
-// on your new testcase.
+// infrastructure and macros. This file supports both report mode, and patch
+// mode.
+// See spatch --help for more information.
+virtual report
+virtual patch
-// Replace open coded calls to igt_fail() with macro versions
+ at initialize:python@
@@
+op_str_map = {
+ '==': 'eq',
+ '!=': 'neq',
+ '<': 'lt',
+ '<=': 'lte',
+ '>': 'lt',
+ '>=': 'lte',
+}
+
+def warn_cmp_macro(p, type_prefix, op, e1, e2):
+ op = op.strip()
+ op_str = op_str_map[op]
+ if type_prefix != '':
+ type_prefix = type_prefix + '_'
+
+ if op.startswith('>'):
+ args = type_prefix, op_str, e2, e1
+ else:
+ args = type_prefix, op_str, e1, e2
+
+ msg = 'WARNING: igt_assert_%s%s(%s, %s) should be used instead' % args
+ coccilib.report.print_report(p[0], msg)
+
+// Replace open coded calls to igt_fail() with macro versions
+ at depends on patch@
expression Ec;
expression list[n] Ep;
@@
@@ -22,14 +47,30 @@ expression list[n] Ep;
- igt_fail(...);
- }
+ igt_fail_on_f(Ec, Ep);
+
+ at opencode_log_then_fail_report depends on report@
+identifier func =~ "^igt_(warn|info|debug)$";
+position p;
+@@
+if (...) {
+ func at p(...);
+ igt_fail(...);
+}
+
+ at script:python depends on report@
+p << opencode_log_then_fail_report.p;
@@
+coccilib.report.print_report(p[0], "WARNING: igt_fail_on_f() should be used instead")
+
+ at depends on patch@
expression Ec;
@@
- if (Ec) {
- igt_fail(...);
- }
+ igt_fail_on(Ec);
-@@
+
+ at depends on patch@
expression Ec;
expression list[n] Ep;
@@
@@ -37,7 +78,8 @@ expression list[n] Ep;
- igt_skip(Ep);
- }
+ igt_skip_on_f(Ec, Ep);
-@@
+
+ at depends on patch@
expression Ec;
expression list[n] Ep;
@@
@@ -46,8 +88,23 @@ expression list[n] Ep;
- }
+ igt_warn_on_f(Ec, Ep);
+ at warn_skip_opencode_report depends on report@
+identifier func =~ "^igt_(warn|skip)$";
+position p;
+@@
+if (...) {
+ func at p(...);
+}
+
+ at script:python depends on report@
+func << warn_skip_opencode_report.func;
+p << warn_skip_opencode_report.p;
+@@
+msg="WARNING: %s_on_f() should be used instead" % func
+coccilib.report.print_report(p[0], msg)
+
// Enforce use of logging functions
- at depends on !(file in "igt_core.c")@
+ at depends on patch && !(file in "igt_core.c")@
expression list[n] Ep;
@@
(
@@ -58,6 +115,34 @@ expression list[n] Ep;
+ igt_info(Ep);
)
+ at no_fprintf_report depends on report && !(file in "igt_core.c")@
+position p;
+identifier i =~ "^std(out|err)$";
+@@
+fprintf at p(i, ...);
+
+ at script:python depends on report@
+p << no_fprintf_report.p;
+i << no_fprintf_report.i;
+@@
+should_use = "igt_info" if i == "stdout" else "igt_warn"
+msg = "WARNING: %s() should be used instead" % should_use
+coccilib.report.print_report(p[0], msg)
+
+ at no_printf_perror_report depends on report && !(file in "igt_core.c")@
+position p;
+identifier func =~ "^p(rintf|error)$";
+@@
+func at p(...);
+
+ at script:python depends on report@
+p << no_printf_perror_report.p;
+func << no_printf_perror_report.func;
+@@
+should_use = "igt_info" if func == "printf" else "igt_warn"
+msg = "WARNING: %s() should be used instead" % should_use
+coccilib.report.print_report(p[0], msg)
+
// No abort for tests, really. Should only be used for internal library checks
// in lib/*
@ignore_internal_abort depends on file in "tests/"@
@@ -65,13 +150,24 @@ position p;
@@
void __igt_fail_assert(...) { <... abort at p(); ...> }
- at depends on file in "tests/"@
+ at depends on patch && file in "tests/"@
position p != ignore_internal_abort.p;
@@
-abort at p();
+igt_fail(IGT_EXIT_FAILURE);
+ at abort_report depends on report && file in "tests/"@
+position p != ignore_internal_abort.p;
+@@
+abort at p();
+
+ at script:python depends on report@
+p << abort_report.p;
@@
+msg="WARNING: abort() is not allowed, use igt_fail(IGT_EXIT_FAILURE) instead"
+coccilib.report.print_report(p[0], msg)
+
+ at depends on patch@
iterator name for_each_pipe;
igt_display_t *display;
expression pipe;
@@ -81,15 +177,37 @@ expression pipe;
...
}
-// Tests really shouldn't use plain assert!
- at depends on file in "tests/"@
+ at for_each_pipe_report depends on report@
+igt_display *display;
+expression pipe;
+position p;
+@@
+for (pipe = 0; pipe < igt_display_get_n_pipes(display); pipe++)@p { ... }
+
+ at script:python depends on report@
+p << for_each_pipe_report.p;
+@@
+coccilib.report.print_report(p[0], "WARNING: for_each_pipe() should be used instead")
+
+// Tests and test libraries really shouldn't use plain assert!
+ at depends on report && file in "tests/"@
expression E;
@@
- assert(E);
+ igt_assert(E);
-// Replace open-coded igt_swap()
+ at no_assert_report depends on report && file in "tests/"@
+position p;
@@
+assert at p(...);
+
+ at script:python depends on report@
+p << no_assert_report.p;
+@@
+coccilib.report.print_report(p[0], "ERROR: plain assert() calls are not allowed")
+
+// Replace open-coded igt_swap()
+ at depends on patch@
type T;
T a, b, tmp;
@@
@@ -98,6 +216,23 @@ T a, b, tmp;
- b = tmp;
+ igt_swap(a, b);
+ at igt_swap_report depends on report@
+position p;
+type T;
+T a, b, tmp;
+@@
+tmp = a;
+a = b;
+b = tmp at p;
+
+ at script:python depends on report@
+p << igt_swap_report.p;
+a << igt_swap_report.a;
+b << igt_swap_report.b;
+@@
+msg = "WARNING: igt_swap(%s, %s) should be used instead" % (a, b)
+coccilib.report.print_report(p[0], msg)
+
// So we don't complain about #define min(), #define max(), etc.
@min_max_ignore@
identifier i =~ "^m(in|ax)$";
@@ -107,7 +242,7 @@ position p;
#define i(...) <+...E at p...+>
// Replace open-coded min/max()
-@@
+ at depends on patch@
position p != min_max_ignore.p;
expression a;
expression b;
@@ -122,8 +257,25 @@ binary operator gt_op = {>,>=};
+ max(a, b)
)
-// Use comparison macros instead of raw igt_assert when possible
+ at no_opencoded_min_max_report depends on report@
+position p != min_max_ignore.p;
+expression a, b;
+binary operator op = {<,<=,>,>=};
@@
+ (a op b) ? a : b at p
+
+ at script:python depends on report@
+a << no_opencoded_min_max_report.a;
+b << no_opencoded_min_max_report.b;
+p << no_opencoded_min_max_report.p;
+op << no_opencoded_min_max_report.op;
+@@
+should_use = "min" if op.strip().startswith("<") else "max"
+msg = "WARNING: %s(%s, %s) should be used instead" % (should_use, a, b)
+coccilib.report.print_report(p[0], msg)
+
+// Use comparison macros instead of raw igt_assert when possible
+ at depends on patch@
typedef uint32_t;
typedef uint64_t;
typedef int64_t;
@@ -201,8 +353,86 @@ int64_t s64_1, s64_2;
+ igt_assert_lte_u64(u64_2, u64_1);
)
-// avoid unused-result warnings when compiling with _FORTIFY_SOURCE defined
+ at igt_assert_cmp_report depends on report@
+position p;
+int e1, e2;
+binary operator op = {==,!=,<,<=,>,>=};
+@@
+igt_assert at p(e1 op e2);
+
+ at script:python depends on report@
+p << igt_assert_cmp_report.p;
+op << igt_assert_cmp_report.op;
+e1 << igt_assert_cmp_report.e1;
+e2 << igt_assert_cmp_report.e2;
+@@
+warn_cmp_macro(p, '', op, e1, e2)
+
+ at igt_assert_cmp_u32_report depends on report@
+position p;
+typedef uint32_t;
+uint32_t e1, e2;
+binary operator op = {==,!=};
+@@
+igt_assert at p(e1 op e2);
+
+ at script:python depends on report@
+p << igt_assert_cmp_u32_report.p;
+op << igt_assert_cmp_u32_report.op;
+e1 << igt_assert_cmp_u32_report.e1;
+e2 << igt_assert_cmp_u32_report.e2;
+@@
+warn_cmp_macro(p, 'u32', op, e1, e2)
+
+ at igt_assert_cmp_u64_report depends on report@
+position p;
+typedef uint64_t;
+uint64_t e1, e2;
+binary operator op = {==,!=,<,<=,>,>=};
+@@
+igt_assert at p(e1 op e2);
+
+ at script:python depends on report@
+p << igt_assert_cmp_u64_report.p;
+op << igt_assert_cmp_u64_report.op;
+e1 << igt_assert_cmp_u64_report.e1;
+e2 << igt_assert_cmp_u64_report.e2;
+@@
+warn_cmp_macro(p, 'u64', op, e1, e2)
+
+ at igt_assert_cmp_s64_report depends on report@
+position p;
+typedef int64_t;
+int64_t e1, e2;
+binary operator op = {==,!=,<,<=,>,>=};
+@@
+igt_assert at p(e1 op e2);
+
+ at script:python depends on report@
+p << igt_assert_cmp_s64_report.p;
+op << igt_assert_cmp_s64_report.op;
+e1 << igt_assert_cmp_s64_report.e1;
+e2 << igt_assert_cmp_s64_report.e2;
+@@
+warn_cmp_macro(p, 's64', op, e1, e2)
+
+ at igt_assert_cmp_double_report depends on report@
+position p;
+double e1, e2;
+binary operator op = {==,!=};
@@
+igt_assert at p(e1 op e2);
+
+ at script:python depends on report@
+p << igt_assert_cmp_double_report.p;
+op << igt_assert_cmp_double_report.op;
+e1 << igt_assert_cmp_double_report.e1;
+e2 << igt_assert_cmp_double_report.e2;
+@@
+warn_cmp_macro(p, "double", op, e1, e2)
+
+// avoid unused-result warnings when compiling with _FORTIFY_SOURCE defined
+ at depends on patch@
identifier func =~ "^(read|write)$";
expression list[2] E;
expression size;
@@ -210,27 +440,40 @@ expression size;
-func(E, size);
+igt_assert_eq(func(E, size), size);
-@@
+ at depends on patch@
expression ptr, size, nmemb, stream;
@@
-fread(ptr, size, nmemb, stream);
+igt_assert_eq(fread(ptr, size, nmemb, stream), nmemb);
-@@
+ at depends on patch@
expression list E;
@@
-fgets(E);
+igt_assert(fgets(E));
-@@
+ at depends on patch@
identifier func =~ "^v?asprintf$";
expression list E;
@@
-func(E);
+igt_assert_neq(func(E), -1);
-// replace open-coded do_ioctl
+ at unused_res_fortify_source_report depends on report@
+identifier func =~ "^(f?read|write|fgets|v?asprintf)$";
+position p;
+@@
+func at p(...);
+
+ at script:python depends on report@
+p << unused_res_fortify_source_report.p;
+func << unused_res_fortify_source_report.func;
@@
+msg = "WARNING: %s() result unused, will throw warning with _FORTIFY_SOURCE defined" % func
+coccilib.report.print_report(p[0], msg)
+
+// replace open-coded do_ioctl
+ at depends on patch@
expression a, b, c, e;
@@
(
@@ -246,3 +489,34 @@ expression a, b, c, e;
-igt_assert(drmIoctl(a, b, c) < 0 && errno == e);
+do_ioctl_err(a, b, c, e);
)
+
+ at opencode_do_ioctl_report depends on report@
+expression list[3] Ep;
+position p;
+@@
+(
+do_or_die(drmIoctl at p(Ep));
+|
+igt_assert_eq(drmIoctl at p(Ep), 0);
+)
+
+ at script:python depends on report@
+p << opencode_do_ioctl_report.p;
+@@
+coccilib.report.print_report(p[0], "WARNING: do_ioctl() should be used instead")
+
+ at opencode_do_ioctl_err_report depends on report@
+expression list[3] Ep;
+expression err;
+position p;
+@@
+(
+igt_assert(drmIoctl at p(Ep) == -1 && errno == err);
+|
+igt_assert(drmIoctl at p(Ep) < 0 && errno == err);
+)
+
+ at script:python depends on report@
+p << opencode_do_ioctl_err_report.p;
+@@
+coccilib.report.print_report(p[0], "WARNING: do_ioctl_err() should be used instead")
--
2.21.0
More information about the Intel-gfx-trybot
mailing list