Re: [PATCH v3] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sat, Oct 24, 2020 at 2:36 PM Dwaipayan Ray <dwaipayanray1@xxxxxxxxx> wrote:
>
> It is generally preferred that the macros from
> include/linux/compiler_attributes.h are used, unless there
> is a reason not to.
>
> checkpatch currently checks __attribute__ for each of
> packed, aligned, printf, scanf, and weak. Other declarations
> in compiler_attributes.h are not handled.
>
> Add a generic test to check the presence of such attributes.
> Some attributes require more specific handling and are kept
> separate.
>
> New attributes which are now handled are:
>
> __alias__(#symbol)
> __always_inline__
> __assume_aligned__(a, ## __VA_ARGS__)
> __cold__
> __const__
> __copy__(symbol)
> __designated_init__
> __externally_visible__
> __gnu_inline__
> __malloc__
> __mode__(x)
> __no_caller_saved_registers__
> __noclone__
> __fallthrough__
> __noinline__
> __nonstring__
> __noreturn__
> __pure__
> __unused__
> __used__
>
> Link: https://lore.kernel.org/linux-kernel-mentees/3ec15b41754b01666d94b76ce51b9832c2dd577a.camel@xxxxxxxxxxx/
> Suggested-by: Joe Perches <joe@xxxxxxxxxxx>
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@xxxxxxxxx>
> ---
>  scripts/checkpatch.pl | 105 +++++++++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 36 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7e505688257a..01d83d345b4c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6155,50 +6155,83 @@ sub process {
>                         }
>                 }
>
> -# Check for __attribute__ packed, prefer __packed
> +# Check for compiler attributes
>                 if ($realfile !~ m@\binclude/uapi/@ &&
> -                   $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> -                       WARN("PREFER_PACKED",
> -                            "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> -               }
> +                   $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
> +                       my $attr = $1;
> +                       $attr =~ s/\s*\(\s*(.*)\)\s*/$1/;
> +
> +                       my %attr_list = (
> +                               "alias"                         => "__alias",
> +                               "aligned"                       => "__aligned",
> +                               "always_inline"                 => "__always_inline",
> +                               "assume_aligned"                => "__assume_aligned",
> +                               "cold"                          => "__cold",
> +                               "const"                         => "__const",
> +                               "copy"                          => "__copy",
> +                               "designated_init"               => "__designated_init",
> +                               "externally_visible"            => "__visible",
> +                               "fallthrough"                   => "fallthrough",
> +                               "gnu_inline"                    => "__gnu_inline",
> +                               "malloc"                        => "__malloc",
> +                               "mode"                          => "__mode",
> +                               "no_caller_saved_registers"     => "__no_caller_saved_registers",
> +                               "noclone"                       => "__noclone",
> +                               "noinline"                      => "noinline",
> +                               "nonstring"                     => "__nonstring",
> +                               "noreturn"                      => "__noreturn",
> +                               "packed"                        => "__packed",
> +                               "pure"                          => "__pure",
> +                               "used"                          => "__used"
> +                       );
> +
> +                       if ($attr =~ /^(\w+)\s*(${balanced_parens})?/) {
> +                               my $curr_attr = $1;
> +                               my $params = '';
> +                               $params = $2 if defined($2);
> +                               $curr_attr =~ s/^[\s_]+|[\s_]+$//g;
> +
> +                               if (exists($attr_list{$curr_attr})) {
> +                                       my $new = $attr_list{$curr_attr};
> +                                       WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +                                            "$new$params is preffered over __attribute__(($attr))\n" . $herecurr);
> +                               }
> +                       }
>
> -# Check for __attribute__ aligned, prefer __aligned
> -               if ($realfile !~ m@\binclude/uapi/@ &&
> -                   $line =~ /\b__attribute__\s*\(\s*\(.*aligned/) {
> -                       WARN("PREFER_ALIGNED",
> -                            "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr);
> -               }
> +                       # Check for __attribute__ format(printf, prefer __printf
> +                       if ($attr =~ /^_*format_*\s*\(\s*printf/) {
> +                               if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +                                        "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
> +                                       $fix) {
> +                                       $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
>
> -# Check for __attribute__ section, prefer __section
> -               if ($realfile !~ m@\binclude/uapi/@ &&
> -                   $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) {
> -                       my $old = substr($rawline, $-[1], $+[1] - $-[1]);
> -                       my $new = substr($old, 1, -1);
> -                       if (WARN("PREFER_SECTION",
> -                                "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
> -                           $fix) {
> -                               $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
> +                               }
>                         }
> -               }
>
> -# Check for __attribute__ format(printf, prefer __printf
> -               if ($realfile !~ m@\binclude/uapi/@ &&
> -                   $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) {
> -                       if (WARN("PREFER_PRINTF",
> -                                "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
> -                           $fix) {
> -                               $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
> +                       # Check for __attribute__ format(scanf, prefer __scanf
> +                       if ($attr =~ /^_*format_*\s*\(\s*scanf\b/) {
> +                               if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +                                        "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
> +                                       $fix) {
> +                                       $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
> +                               }
> +                       }
>
> +                       # Check for __attribute__ section, prefer __section
> +                       if ($attr =~ /^_*section_*\s*\(\s*("[^"]*")/) {
> +                               my $old = substr($rawline, $-[1], $+[1] - $-[1]);
> +                               my $new = substr($old, 1, -1);
> +                               if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +                                        "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
> +                                       $fix) {
> +                                       $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
> +                               }
>                         }
> -               }
>
> -# Check for __attribute__ format(scanf, prefer __scanf
> -               if ($realfile !~ m@\binclude/uapi/@ &&
> -                   $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\b/) {
> -                       if (WARN("PREFER_SCANF",
> -                                "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
> -                           $fix) {
> -                               $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
> +                       # Check for __attribute__ unused, prefer __always_unused or __maybe_unused
> +                       if ($attr =~ /^_*unused/) {
> +                               WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +                                    "__always_unused or __maybe_unused is preferred over __attribute__((__unused__))\n" . $herecurr);
>                         }
>                 }
>
> --
> 2.27.0
>
Hi,
In this version there is only one generic test for both parameterized
and non parameterized attributes.

And four attribute checks are kept separate and unaltered from the
original implementation. Those are the format(printf/scanf), section
and unused(which can be resolved to __always_unused or __maybe_unused).

Thanks,
Dwaipayan.