[PATCH] scripts: reduce false positives in the macro_checker script.

[PATCH] scripts: reduce false positives in the macro_checker script.

[Julian Sun]

Reduce false positives in the macro_checker
in the following scenarios:
  1. Conditional compilation
  2. Macro definitions with only a single character
  3. Macro definitions as (0) and (1)

Before this patch:
	sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
	99

After this patch:
	sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
	11

Most of the current warnings are valid now.

Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
---
 scripts/macro_checker.py | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/scripts/macro_checker.py b/scripts/macro_checker.py
index cd10c9c10d31..8195339ea5b5 100755
--- a/scripts/macro_checker.py
+++ b/scripts/macro_checker.py
@@ -9,9 +9,12 @@ import os
 import re
 
 macro_pattern = r"#define\s+(\w+)\(([^)]*)\)"
-# below two vars were used to reduce false positives
-do_while0_pattern = r"\s*do\s*\{\s*\}\s*while\s*\(\s*0\s*\)"
+# below vars were used to reduce false positives
+fp_patterns = [r"\s*do\s*\{\s*\}\s*while\s*\(\s*0\s*\)",
+               r"\(?0\)?", r"\(?1\)?"]
 correct_macros = []
+cond_compile_mark = "#if"
+cond_compile_end = "#endif"
 
 def check_macro(macro_line, report):
     match = re.match(macro_pattern, macro_line)
@@ -21,15 +24,25 @@ def check_macro(macro_line, report):
         content = match.group(2)
         arguments = [item.strip() for item in content.split(',') if item.strip()]
 
-        if (re.match(do_while0_pattern, macro_def)):
+        macro_def = macro_def.strip()
+        if not macro_def:
             return
+        # used to reduce false positives, like #define endfor_nexthops(rt) }
+        if len(macro_def) == 1:
+            return
+
+        for fp_pattern in fp_patterns:
+            if (re.match(fp_pattern, macro_def)):
+                return
 
         for arg in arguments:
             # used to reduce false positives
             if "..." in arg:
-                continue
+                return
+        for arg in arguments:
             if not arg in macro_def and report == False:
                 return
+            # if there is a correct macro with the same name, do not report it.
             if not arg in macro_def and identifier not in correct_macros:
                 print(f"Argument {arg} is not used in function-line macro {identifier}")
                 return
@@ -49,6 +62,8 @@ def macro_strip(macro):
     return macro
 
 def file_check_macro(file_path, report):
+    # number of conditional compiling
+    cond_compile = 0
     # only check .c and .h file
     if not file_path.endswith(".c") and not file_path.endswith(".h"):
         return
@@ -57,7 +72,14 @@ def file_check_macro(file_path, report):
         while True:
             line = f.readline()
             if not line:
-                return
+                break
+            line = line.strip()
+            if line.startswith(cond_compile_mark):
+                cond_compile += 1
+                continue
+            if line.startswith(cond_compile_end):
+                cond_compile -= 1
+                continue
 
             macro = re.match(macro_pattern, line)
             if macro:
@@ -67,6 +89,11 @@ def file_check_macro(file_path, report):
                     macro = macro.strip()
                     macro += f.readline()
                     macro = macro_strip(macro)
+                if file_path.endswith(".c")  and cond_compile != 0:
+                    continue
+                # 1 is for #ifdef xxx at the beginning of the header file
+                if file_path.endswith(".h") and cond_compile != 1:
+                    continue
                 check_macro(macro, report)
 
 def get_correct_macros(path):
-- 
2.39.2

Re: [PATCH] scripts: reduce false positives in the macro_checker script.

[Jan Kara]

On Thu 25-07-24 03:58:30, Julian Sun wrote:
> Reduce false positives in the macro_checker
> in the following scenarios:
>   1. Conditional compilation
>   2. Macro definitions with only a single character
>   3. Macro definitions as (0) and (1)
> 
> Before this patch:
> 	sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> 	99
> 
> After this patch:
> 	sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> 	11
> 
> Most of the current warnings are valid now.
> 
> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
...
>  def file_check_macro(file_path, report):
> +    # number of conditional compiling
> +    cond_compile = 0
>      # only check .c and .h file
>      if not file_path.endswith(".c") and not file_path.endswith(".h"):
>          return
> @@ -57,7 +72,14 @@ def file_check_macro(file_path, report):
>          while True:
>              line = f.readline()
>              if not line:
> -                return
> +                break
> +            line = line.strip()
> +            if line.startswith(cond_compile_mark):
> +                cond_compile += 1
> +                continue
> +            if line.startswith(cond_compile_end):
> +                cond_compile -= 1
> +                continue
>  
>              macro = re.match(macro_pattern, line)
>              if macro:
> @@ -67,6 +89,11 @@ def file_check_macro(file_path, report):
>                      macro = macro.strip()
>                      macro += f.readline()
>                      macro = macro_strip(macro)
> +                if file_path.endswith(".c")  and cond_compile != 0:
> +                    continue
> +                # 1 is for #ifdef xxx at the beginning of the header file
> +                if file_path.endswith(".h") and cond_compile != 1:
> +                    continue
>                  check_macro(macro, report)
>  
>  def get_correct_macros(path):

So I don't think this is right. As far as I understand this skips any macros
that are conditionally defined? Why? There is a lot of them and checking
them is beneficial... The patterns you have added should be dealing with
most of the conditional defines anyway.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] scripts: reduce false positives in the macro_checker script.

[Julian Sun]

Jan Kara <jack@suse.cz> 于2024年7月25日周四 04:52写道：
>
> On Thu 25-07-24 03:58:30, Julian Sun wrote:
> > Reduce false positives in the macro_checker
> > in the following scenarios:
> >   1. Conditional compilation
> >   2. Macro definitions with only a single character
> >   3. Macro definitions as (0) and (1)
> >
> > Before this patch:
> >       sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> >       99
> >
> > After this patch:
> >       sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> >       11
> >
> > Most of the current warnings are valid now.
> >
> > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
> ...
> >  def file_check_macro(file_path, report):
> > +    # number of conditional compiling
> > +    cond_compile = 0
> >      # only check .c and .h file
> >      if not file_path.endswith(".c") and not file_path.endswith(".h"):
> >          return
> > @@ -57,7 +72,14 @@ def file_check_macro(file_path, report):
> >          while True:
> >              line = f.readline()
> >              if not line:
> > -                return
> > +                break
> > +            line = line.strip()
> > +            if line.startswith(cond_compile_mark):
> > +                cond_compile += 1
> > +                continue
> > +            if line.startswith(cond_compile_end):
> > +                cond_compile -= 1
> > +                continue
> >
> >              macro = re.match(macro_pattern, line)
> >              if macro:
> > @@ -67,6 +89,11 @@ def file_check_macro(file_path, report):
> >                      macro = macro.strip()
> >                      macro += f.readline()
> >                      macro = macro_strip(macro)
> > +                if file_path.endswith(".c")  and cond_compile != 0:
> > +                    continue
> > +                # 1 is for #ifdef xxx at the beginning of the header file
> > +                if file_path.endswith(".h") and cond_compile != 1:
> > +                    continue
> >                  check_macro(macro, report)
> >
> >  def get_correct_macros(path):
>
>
> > So I don't think this is right. As far as I understand this skips any macros
> > that are conditionally defined? Why? There is a lot of them and checking
> > them is beneficial... The patterns you have added should be dealing with
> > most of the conditional defines anyway.
Yes, this skips all checks for conditional macro. This is because I
observed that almost all false positives come from conditional
compilation. Testing showed that skipping them does not cause the
genuine warnings to disappear.
Also as you said, it may still lead to skipping checks for genuinely
problematic macro definitions. Perhaps we could provide an option that
allows users to control whether or not to check macros under
conditional compilation?
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR


Thanks,
-- 
Julian Sun <sunjunchao2870@gmail.com>

Re: [PATCH] scripts: reduce false positives in the macro_checker script.

[Jan Kara]

On Thu 25-07-24 05:15:34, Julian Sun wrote:
> Jan Kara <jack@suse.cz> 于2024年7月25日周四 04:52写道：
> >
> > On Thu 25-07-24 03:58:30, Julian Sun wrote:
> > > Reduce false positives in the macro_checker
> > > in the following scenarios:
> > >   1. Conditional compilation
> > >   2. Macro definitions with only a single character
> > >   3. Macro definitions as (0) and (1)
> > >
> > > Before this patch:
> > >       sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> > >       99
> > >
> > > After this patch:
> > >       sjc@sjc:linux$ ./scripts/macro_checker.py  fs | wc -l
> > >       11
> > >
> > > Most of the current warnings are valid now.
> > >
> > > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
> > ...
> > >  def file_check_macro(file_path, report):
> > > +    # number of conditional compiling
> > > +    cond_compile = 0
> > >      # only check .c and .h file
> > >      if not file_path.endswith(".c") and not file_path.endswith(".h"):
> > >          return
> > > @@ -57,7 +72,14 @@ def file_check_macro(file_path, report):
> > >          while True:
> > >              line = f.readline()
> > >              if not line:
> > > -                return
> > > +                break
> > > +            line = line.strip()
> > > +            if line.startswith(cond_compile_mark):
> > > +                cond_compile += 1
> > > +                continue
> > > +            if line.startswith(cond_compile_end):
> > > +                cond_compile -= 1
> > > +                continue
> > >
> > >              macro = re.match(macro_pattern, line)
> > >              if macro:
> > > @@ -67,6 +89,11 @@ def file_check_macro(file_path, report):
> > >                      macro = macro.strip()
> > >                      macro += f.readline()
> > >                      macro = macro_strip(macro)
> > > +                if file_path.endswith(".c")  and cond_compile != 0:
> > > +                    continue
> > > +                # 1 is for #ifdef xxx at the beginning of the header file
> > > +                if file_path.endswith(".h") and cond_compile != 1:
> > > +                    continue
> > >                  check_macro(macro, report)
> > >
> > >  def get_correct_macros(path):
> >
> >
> > > So I don't think this is right. As far as I understand this skips any macros
> > > that are conditionally defined? Why? There is a lot of them and checking
> > > them is beneficial... The patterns you have added should be dealing with
> > > most of the conditional defines anyway.
> Yes, this skips all checks for conditional macro. This is because I
> observed that almost all false positives come from conditional
> compilation. Testing showed that skipping them does not cause the
> genuine warnings to disappear.
> Also as you said, it may still lead to skipping checks for genuinely
> problematic macro definitions. Perhaps we could provide an option that
> allows users to control whether or not to check macros under
> conditional compilation?

Yes, that could be useful.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR