[RFC,v2] checkpatch: extend attributes check to handle more patterns
diff mbox series

Message ID 20201023094307.20820-1-dwaipayanray1@gmail.com
State New, archived
Headers show
Series
  • [RFC,v2] checkpatch: extend attributes check to handle more patterns
Related show

Commit Message

Dwaipayan Ray Oct. 23, 2020, 9:43 a.m. UTC
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@perches.com/
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 130 ++++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 36 deletions(-)

Comments

Joe Perches Oct. 23, 2020, 11:04 a.m. UTC | #1
On Fri, 2020-10-23 at 15:13 +0530, Dwaipayan Ray 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

checkpatch, no need for capitalization

and non-trivially:

> +			my $attr_list = qr {
> +				__alias__|
> +				__aligned__$|
> +				__aligned__\(.*\)|
> +				__always_inline__|
> +				__assume_aligned__\(.*\)|
> +				__cold__|
> +				__const__|
> +				__copy__\(.*\)|
> +				__designated_init__|
> +				__externally_visible__|
> +				__fallthrough__|
> +				__gnu_inline__|
> +				__malloc__|
> +				__mode__\(.*\)|
> +				__no_caller_saved_registers__|
> +				__noclone__|
> +				__noinline__|
> +				__nonstring__|
> +				__noreturn__|
> +				__packed__|
> +				__pure__|
> +				__used__
> +			}x;
[]
> +			my %attr_replace = (
> +				"__alias__"			=> "__alias",
> +				"__aligned__"		=> "__aligned_largest",
> +				"__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 =~/^($attr_list)/) {

I would remove the __ from the entries here.

And you could check using

	$line =~ /__attribute__\s*\(\s*($balanced_parens)\s*)/

and check for all attributes in $1 after
stripping the leading and trailing parens
and any leading and trailing underscores
from each attribute.

And you only need one hash and you should 
check for existence of the key rather than
have multiple lists.

	if (exists($attributes($attr))) {

> +				my $old = $1;
> +				if ($old =~ /^(__.+__\()(.*)\)/) {
> +					my $new = $attr_replace{$1};
> +					WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +					     "$new($2) is preffered over __attribute__(($old))\n" . $herecurr);

preferred

> +				} else {
> +					my $new = $attr_replace{$old};
> +					WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +					     "$new is preffered over __attribute__(($old))\n" . $herecurr);
> +				}
> +			}
Dwaipayan Ray Oct. 23, 2020, 11:40 a.m. UTC | #2
On Fri, Oct 23, 2020 at 4:34 PM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2020-10-23 at 15:13 +0530, Dwaipayan Ray 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
>
> checkpatch, no need for capitalization
>
> and non-trivially:
>
> > +                     my $attr_list = qr {
> > +                             __alias__|
> > +                             __aligned__$|
> > +                             __aligned__\(.*\)|
> > +                             __always_inline__|
> > +                             __assume_aligned__\(.*\)|
> > +                             __cold__|
> > +                             __const__|
> > +                             __copy__\(.*\)|
> > +                             __designated_init__|
> > +                             __externally_visible__|
> > +                             __fallthrough__|
> > +                             __gnu_inline__|
> > +                             __malloc__|
> > +                             __mode__\(.*\)|
> > +                             __no_caller_saved_registers__|
> > +                             __noclone__|
> > +                             __noinline__|
> > +                             __nonstring__|
> > +                             __noreturn__|
> > +                             __packed__|
> > +                             __pure__|
> > +                             __used__
> > +                     }x;
> []
> > +                     my %attr_replace = (
> > +                             "__alias__"                     => "__alias",
> > +                             "__aligned__"           => "__aligned_largest",
> > +                             "__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 =~/^($attr_list)/) {
>
> I would remove the __ from the entries here.
>
> And you could check using
>
>         $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*)/
>
> and check for all attributes in $1 after
> stripping the leading and trailing parens
> and any leading and trailing underscores
> from each attribute.
>
> And you only need one hash and you should
> check for existence of the key rather than
> have multiple lists.
>
>         if (exists($attributes($attr))) {
>

Okay thanks!
But what should be done for the attributes which are
parameterized, like __aligned__(x). Should all the __
for these entries be trimmed too? There are also
cases where there are multiple versions like:

__aligned__
__aligned__(x)

To help differentiate between them what can be done?
Should i make the keys as:

aligned
aligned__(

instead of

__aligned__
__aligned__(

Thanks,
Dwaipayan.
Joe Perches Oct. 23, 2020, 12:08 p.m. UTC | #3
On Fri, 2020-10-23 at 17:10 +0530, Dwaipayan Ray wrote:
> On Fri, Oct 23, 2020 at 4:34 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Fri, 2020-10-23 at 15:13 +0530, Dwaipayan Ray 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
> > 
> > checkpatch, no need for capitalization
> > 
> > and non-trivially:
> > 
> > > +                     my $attr_list = qr {
> > > +                             __alias__|
> > > +                             __aligned__$|
> > > +                             __aligned__\(.*\)|
> > > +                             __always_inline__|
> > > +                             __assume_aligned__\(.*\)|
> > > +                             __cold__|
> > > +                             __const__|
> > > +                             __copy__\(.*\)|
> > > +                             __designated_init__|
> > > +                             __externally_visible__|
> > > +                             __fallthrough__|
> > > +                             __gnu_inline__|
> > > +                             __malloc__|
> > > +                             __mode__\(.*\)|
> > > +                             __no_caller_saved_registers__|
> > > +                             __noclone__|
> > > +                             __noinline__|
> > > +                             __nonstring__|
> > > +                             __noreturn__|
> > > +                             __packed__|
> > > +                             __pure__|
> > > +                             __used__
> > > +                     }x;
> > []
> > > +                     my %attr_replace = (
> > > +                             "__alias__"                     => "__alias",
> > > +                             "__aligned__"           => "__aligned_largest",
> > > +                             "__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 =~/^($attr_list)/) {
> > 
> > I would remove the __ from the entries here.
> > 
> > And you could check using
> > 
> >         $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*)/
> > 
> > and check for all attributes in $1 after
> > stripping the leading and trailing parens
> > and any leading and trailing underscores
> > from each attribute.
> > 
> > And you only need one hash and you should
> > check for existence of the key rather than
> > have multiple lists.
> > 
> >         if (exists($attributes($attr))) {
> > 
> 
> Okay thanks!
> But what should be done for the attributes which are
> parameterized, like __aligned__(x). Should all the __
> for these entries be trimmed too?

yes

> There are also
> cases where there are multiple versions like:
> 
> __aligned__
> __aligned__(x)

$ git grep __aligned__ | grep -v -P '__aligned__\s*\('

AFAIK: There is only one use of bare __aligned__ and
that's in compiler_attributes.h

> To help differentiate between them what can be done?
> Should i make the keys as:
> 
> aligned
> aligned__(
> 
> instead of
> 
> __aligned__
> __aligned__(

Just use aligned

Just fyi:

these are the uses of __attribute__ in the kernel
with all the underscores and spaces removed so there's
some value in finding the multiple actual attributes .

$ git grep -ohP '__attribute__\s*\(\s*\(.*\)\s*\)' | \
  sed -r -e 's/\s//g' -r -e  's/__(\w+)__/\1/g' | \
  sort | uniq -c | sort -rn
   2181 attribute((packed))
    212 attribute((unused))
    206 attribute((packed,aligned(4)))
    131 attribute((aligned(8)))
     77 attribute((aligned(32)))
     74 attribute((always_inline))
     68 attribute((noinline))
     52 attribute((preserve_access_index))
     48 attribute((weak))
     38 attribute((noreturn))
     38 attribute((aligned(16)))
     25 attribute((packed,aligned(8)))
     25 attribute((const))
     18 attribute((aligned(64)))
     17 attribute((aligned(256)))
     17 attribute((aligned(128)))
     16 attribute((used))
     16 attribute((format(printf,1,2)))
     15 attribute((aligned(4)))
     14 attribute((constructor))
     10 attribute((unused)))
     10 attribute((pure))
     10 attribute((mode(DI)))
     10 attribute((aligned(SMP_CACHE_BYTES)))
      9 attribute((format(printf,4,5)))
      9 attribute((format(printf,2,3)))
      9 attribute((aligned(sizeof(long))))
      8 attribute((mode(SI)))
      8 attribute((aligned(PAGE_SIZE)))
      7 attribute((visibility("hidden")))
      7 attribute((packed,aligned(2)))
      6 attribute((section(NAME),used))
      6 attribute((section("_ftrace_events")))
      6 attribute((section(".data..read_mostly")))
      6 attribute((section(".data")))
      6 attribute((mode(word)))
      5 attribute((format(printf,a,b)))
      5 attribute((always_inline))typeof(name(0))
      5 attribute((aligned(sizeof(unsignedlong))))
      5 attribute((aligned(alignof(structebt_replace))))
      5 attribute((aligned(AESNI_ALIGN)))
      4 attribute((weak,alias("__vdso_clock_gettime")))
      4 attribute((section("__trace_printk_fmt")))
      4 attribute((section(".inittext")))
      4 attribute((packed,aligned(256)))
      4 attribute((packed,aligned(16)))
      4 attribute((nonnull))
      4 attribute((may_alias))
      4 attribute((format(printf,3,4)))
      4 attribute((fallthrough))
      4 attribute((aligned(x)))
      4 attribute((aligned(sizeof(void*))))
      4 attribute((aligned(BITS_PER_LONG/8)))
      4 attribute((aligned(alignof(u64))))
      3 attribute((weak,alias("__vdso_gettimeofday")))
      3 attribute((visibility("default")))
      3 attribute((section("data_sec")))
      3 attribute((regparm(0)))
      3 attribute((nonnull(2,3)))
      3 attribute((mode(TI)))
      3 attribute((flatten))
      3 attribute((destructor))
      3 attribute((bitwise))
      3 attribute((aligned(sizeof(uint64_t))))
      3 attribute((aligned(sizeof(__u64))))
      3 attribute((aligned(PADLOCK_ALIGNMENT)))
      3 attribute((aligned(L1_CACHE_BYTES)))
      3 attribute((aligned(a)))
      3 attribute((alias("thermal_genl_event_tz")))
      2 attribute((weak,unused))
      2 attribute((weak,section(".rodata")))
      2 attribute((weak,alias("__vdso_time")))
      2 attribute((weak,alias("__vdso_getcpu")))
      2 attribute((weak,alias("__vdso_clock_getres")))
      2 attribute((warn_unused_result,nonnull))
      2 attribute((vector_size(16)))
      2 attribute((user))
      2 attribute((section(".__syscall_stub")))
      2 attribute((section(".maps."#name),used))
      2 attribute((section("license"),used))
      2 attribute((section("_ftrace_eval_map")))
      2 attribute((section(".cpuidle.text")))
      2 attribute((section("__builtin_cmdline")))
      2 attribute((section(".arch.info.init")))
      2 attribute((packed,aligned(PMCRAID_IOARCB_ALIGNMENT)))
      2 attribute((packed,aligned(64)))
      2 attribute((no_sanitize_address))
      2 attribute((noreturn,unused))
      2 attribute((nonnull(2)))
      2 attribute((no_instrument_function))
      2 attribute((model(small)))
      2 attribute((malloc))
      2 attribute((latent_entropy))
      2 attribute((gnu_inline))
      2 attribute((format(scanf,a,b)))
      2 attribute((format(printf,1,2)))attribute((noreturn))
      2 attribute((force))
      2 attribute((externally_visible))
      2 attribute((error(message)))
      2 attribute((common))
      2 attribute((aligned(XCHAL_NCP_SA_ALIGN)))
      2 attribute((aligned(VRING_USED_ALIGN_SIZE)))
      2 attribute((aligned((sizeof(long)))))
      2 attribute((aligned(sizeof(Elf##size##_Word))))
      2 attribute((aligned(65536)))
      2 attribute((aligned(512)))
      2 attribute((aligned(4*sizeof(__u64))))
      2 attribute((aligned(32),packed))
      2 attribute((aligned(0x20000)))
      2 attribute((alias(__stringify(__se_sys##name))))
      2 attribute((alias(__stringify(__se_compat_sys##name))))
      2 attribute((alias("cfi_cmdset_0002")))
      2 attribute((alias("cfi_cmdset_0001")))
      1 attribute((weakref("dsdt_aml_code")))
      1 attribute((weakref("AmlCode")))
      1 attribute((weak,alias("__vdso_clock_gettime64")))
      1 attribute((weak,alias("__platform_"#f)))
      1 attribute((weak,alias("__mips_cm_phys_base")))
      1 attribute((weak,alias("__mips_cm_l2sync_phys_base")))
      1 attribute((weak,alias("__alloc_bootmem_huge_page")))
      1 attribute((warn_unused_result))
      1 attribute((warning(message)))
      1 attribute((vector_size(8)))
      1 attribute((used,section(".videocards")))
      1 attribute((used,section(".taglist")))
      1 attribute((unused,section("__param"),aligned(sizeof(void*))))
      1 attribute((unused,alias(__stringify(name))))
      1 attribute((tls_model("initial-exec"),unused))
      1 attribute((tls_model("initial-exec")))
      1 attribute((target("no-vsx")))
      1 attribute((syscall_linkage))
      1 attribute((section(".xiptext")))
      1 attribute((section(".x86_intel_mid_dev.init")))
      1 attribute((section(".x86_cpu_dev.init")))
      1 attribute((section(".vvar_"#name),aligned(16)))
      1 attribute((section(".text.hot")))
      1 attribute((section(".taglist.init")))
      1 attribute((section("__syscalls_metadata")))
      1 attribute((section("__stop_sched_class")))
      1 attribute((section(".spinlock.text")))
      1 attribute((section(".softirqentry.text")))
      1 attribute((section(#section),aligned((sizeof(void*)))))
      1 attribute((section(#__sec".init")))
      1 attribute((section(".sched.text")))
      1 attribute((section(#S)))
      1 attribute((section("__rt_sched_class")))
      1 attribute((section(PER_CPU_BASE_SECTIONsec)))
      1 attribute((section("__modver")))
      1 attribute((section(".modinfo"),unused,aligned(1)))
      1 attribute((section(".machine.desc")))
      1 attribute((section("___ksymtab"sec"+"#sym),used))
      1 attribute((section(".ksyms")))
      1 attribute((section(".kprobes.text")))
      1 attribute((section("_kprobe_blacklist")))
      1 attribute((section("___kentry+"#sym)))
      1 attribute((section(".kconfig")))
      1 attribute((section(".irqentry.text")))
      1 attribute((section(".initcall"level".init")))
      1 attribute((section("__idle_sched_class")))
      1 attribute((section("__fair_sched_class")))
      1 attribute((section("_error_injection_whitelist")))
      1 attribute((section("__dl_sched_class")))
      1 attribute((section(".discard"),unused))
      1 attribute((section(".data..vm0.pte"),aligned(PAGE_SIZE)))
      1 attribute((section(".data..vm0.pmd"),aligned(PAGE_SIZE)))
      1 attribute((section(".data..vm0.pgd"),aligned(PAGE_SIZE)))
      1 attribute((section(".data..ro_after_init")))
      1 attribute((section(".data..init_thread_info")))
      1 attribute((section(".data..init_task")))
      1 attribute((section(".data..init_irqstack")))
      1 attribute((section(".bss..decrypted")))
      1 attribute((section("__bpf_raw_tp_map")))
      1 attribute((section("action-ok"),used))
      1 attribute((section("action-ko"),used))
      1 attribute((section($old)))
      1 attribute((safe))
      1 attribute((returns_twice))
      1 attribute((require_context(x,1,999,"write")))
      1 attribute((require_context(x,1,999,"read")))
      1 attribute((require_context(x,1,999,"rdwr")))
      1 attribute((randomize_layout))
      1 attribute((patchable_function_entry(0,0)))
      1 attribute((packed,deprecated))
      1 attribute((packed,aligned(XSAVE_ALIGNMENT)))
      1 attribute((packed,aligned(PMCRAID_IOADL_ALIGNMENT)))
      1 attribute((packed,aligned(PAGE_SIZE)))
      1 attribute((packed,aligned(2048)))
      1 attribute((packed,aligned(1024)))
      1 attribute((optimize("no-stack-protector")))
      1 attribute((optimize("no-optimize-sibling-calls")))
      1 attribute((optimize("-fno-gcse")))
      1 attribute((no_sanitize_undefined))
      1 attribute((no_sanitize("undefined")))
      1 attribute((no_sanitize_thread))
      1 attribute((no_sanitize("thread")))
      1 attribute((no_sanitize("shadow-call-stack")))
      1 attribute((no_sanitize("address","hwaddress")))
      1 attribute((noreturn))__attribute((format(printf,1,2)))
      1 attribute((no_randomize_layout))
      1 attribute((nonstring))
      1 attribute((nonnull(2,3,4,5)))
      1 attribute((nonnull(2,3,4)))
      1 attribute((nonnull(1)))
      1 attribute((noderef,address_space(__user)))
      1 attribute((noderef,address_space(__rcu)))
      1 attribute((noderef,address_space(__percpu)))
      1 attribute((noderef,address_space(__iomem)))
      1 attribute((noderef))
      1 attribute((noclone))
      1 attribute((nocast))
      1 attribute((no_caller_saved_registers))
      1 attribute((name))
      1 attribute((naked))
      1 attribute((ms_abi))
      1 attribute((mode(x)))
      1 attribute((mode(TF)))
      1 attribute((mode(QI)))
      1 attribute((mode(HI)))
      1 attribute((long_call))
      1 attribute((indirect_branch("keep")))
      1 attribute((hotpatch(0,0)))
      1 attribute((hot))
      1 attribute((format(scanf,string-index,first-to-check)))
      1 attribute((format(printf,string-index,first-to-check)))
      1 attribute((format(printf,(pos_fmtstr),(pos_fmtargs))))
      1 attribute((format(printf,i,j)))
      1 attribute((format(printf,c,c+1)))
      1 attribute((format(printf,5,6)))
      1 attribute((format(printf,2,0)))
      1 attribute((format(gnu_printf,i,j)))
      1 attribute((disable_tail_calls))
      1 attribute((designated_init))
      1 attribute((deprecated(msg)))
      1 attribute((deprecated))
      1 attribute((copy(symbol)))
      1 attribute((context(x,1,1)))
      1 attribute((context(x,1,0)))
      1 attribute((context(x,0,1)))
      1 attribute((cold))
      1 attribute((cleanup(kcsan_end_scoped_access)))
      1 attribute((assume_aligned(a,##VA_ARGS)))
      1 attribute((align_value(8)))
      1 attribute((aligned(XCHAL_CP7_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP6_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP5_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP4_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP3_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP2_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP1_SA_ALIGN)))
      1 attribute((aligned(XCHAL_CP0_SA_ALIGN)))
      1 attribute((aligned(VRING_DESC_ALIGN_SIZE)))
      1 attribute((aligned(VRING_AVAIL_ALIGN_SIZE)))
      1 attribute((aligned(TSB_ENTRY_ALIGNMENT)))
      1 attribute((aligned(TPACKET_ALIGN(x))))
      1 attribute((aligned(TPACKET_ALIGNMENT)))
      1 attribute((aligned(stackalign)))
      1 attribute((aligned(sizeof(u64))))
      1 attribute((aligned(sizeof(u32))))
      1 attribute((aligned(sizeof(__u32))))
      1 attribute((aligned(sizeof(s64))))
      1 attribute((aligned(sizeof(kernel_ulong_t))))
      1 attribute((aligned(size)))
      1 attribute((aligned(PS3_BMP_MINALIGN)))
      1 attribute((aligned(_K_SS_ALIGNSIZE)))
      1 attribute((aligned(INTERNODE_CACHE_BYTES)))
      1 attribute((aligned(GRU_CACHE_LINE_BYTES)))
      1 attribute((aligned(DM_IO_MAX_REGIONS)))
      1 attribute((aligned(CVMX_CACHE_LINE_SIZE)))
      1 attribute((aligned(CRYPTO_MINALIGN)))
      1 attribute((aligned(CPPI_DESCRIPTOR_ALIGN)))
      1 attribute((aligned(ALIGN_SIZE)))
      1 attribute((aligned(alignof(u32))))
      1 attribute((aligned(alignof(structnft_expr))))
      1 attribute((aligned(alignof(structhash_alg_common))))
      1 attribute((aligned(4096)))
      1 attribute((aligned(2*sizeof(long))))
      1 attribute((aligned(2),packed))
      1 attribute((aligned(256),packed))
      1 attribute((aligned(2048)))
      1 attribute((aligned(2)))
      1 attribute((aligned(1<<(INTERNODE_CACHE_SHIFT))))
      1 attribute((aligned(1024),packed))
      1 attribute((aligned(0x80)))
      1 attribute((aligned(0x2000)))
      1 attribute((aligned))
      1 attribute((alias("vtime_account_irq_enter")))
      1 attribute((alias("thermal_genl_event_tz_trip_up")))
      1 attribute((alias("thermal_genl_event_tz_trip_add")))
      1 attribute((alias(#system"_mv")))
      1 attribute((alias(#symbol)))
      1 attribute((alias(__stringify(__s390x_sys_##sname))))
      1 attribute((alias("octeon_board_type_string")))
      1 attribute((alias("native_sched_clock")))
      1 attribute((alias(istringify(SIG_EXPR_LIST_SYM(sig,group)))))
      1 attribute((alias(#internal_name)))
      1 attribute((alias(#initfn)))
      1 attribute((alias(#exitfn)))
      1 attribute((address_space(257))
      1 attribute((address_space(256))
      1 attribute((address_space(0)))
Dwaipayan Ray Oct. 23, 2020, 7:14 p.m. UTC | #4
>>> And you could check using
>>>
>>>          $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*)/
>>>
>>> and check for all attributes in $1 after
>>> stripping the leading and trailing parens
>>> and any leading and trailing underscores
>>> from each attribute.
>>>
>>> And you only need one hash and you should
>>> check for existence of the key rather than
>>> have multiple lists.
>>>
>>>          if (exists($attributes($attr))) {
>>>
>> Okay thanks!
>> But what should be done for the attributes which are
>> parameterized, like __aligned__(x). Should all the __
>> for these entries be trimmed too?
> yes
>
>> There are also
>> cases where there are multiple versions like:
>>
>> __aligned__
>> __aligned__(x)
> $ git grep __aligned__ | grep -v -P '__aligned__\s*\('
>
> AFAIK: There is only one use of bare __aligned__ and
> that's in compiler_attributes.h
>
>> To help differentiate between them what can be done?
>> Should i make the keys as:
>>
>> aligned
>> aligned__(
>>
>> instead of
>>
>> __aligned__
>> __aligned__(
> Just use aligned
>
> Just fyi:
>
> these are the uses of __attribute__ in the kernel
> with all the underscores and spaces removed so there's
> some value in finding the multiple actual attributes .


Hi,
I modified the check to check the attributes from the map.
There are two checks - one for the normal attributes and
one for the ones with arguments, which needs just a bit more processing.

So attributes like __packed__ as well as those like
__aligned__(x) are handled.

What do you think?

---
+            $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
+            my $attr = trim($1);
+            $attr =~ s/\(\s*_*(.*)\)/$1/;
+            while($attr =~ s/(.*)_$/$1/) {}  # Remove trailing underscores
+
+            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 (exists($attr_list{$attr})) {
+                my $new = $attr_list{$attr};
+                WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+                     "$new is preffered over 
__attribute__((__${attr}__))\n" . $herecurr);
+            }
+
+            # Check for attributes with parameters, like copy__(symbol)
+            if ($attr =~ /(.*)__(\(.*\))/) {
+                if (exists($attr_list{$1})) {
+                    my $new = $attr_list{$1};
+                    WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+                         "$new$2 is preffered over 
__attribute__((__${attr}))\n" . $herecurr);
+                }
+            }
---

If this is okay I would like to send in a proper v3.

Thanks,
Dwaipayan.
Joe Perches Oct. 23, 2020, 8:42 p.m. UTC | #5
On Sat, 2020-10-24 at 00:44 +0530, Dwaipayan Ray wrote:
> Hi,

Hi again.

> I modified the check to check the attributes from the map.
> There are two checks - one for the normal attributes and
> one for the ones with arguments, which needs just a bit more
> processing.
> 
> So attributes like __packed__ as well as those like
> __aligned__(x) are handled.
> 
> What do you think?
> 
> ---
> +            $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*\)/)
> {
> +            my $attr = trim($1);
> +            $attr =~ s/\(\s*_*(.*)\)/$1/;
> +            while($attr =~ s/(.*)_$/$1/) {}  # Remove trailing
> underscores

	I think this could be a single test like:

		while ($attr =~ /\s*(\w+)\s*(${balanced_parens})?/g) {
			my $curr_attr = $1;
			my $parens = $2;
			$curr_attr = s/^_*(.*)_*$/$1/;
			
> +            my %attr_list = (
> +                "alias"            => "__alias",
> +                "aligned"        => "__aligned",

These might be better using tab alignment.

And you might special case format(printf/scanf here too
Dwaipayan Ray Oct. 23, 2020, 8:57 p.m. UTC | #6
On Sat, Oct 24, 2020 at 2:12 AM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2020-10-24 at 00:44 +0530, Dwaipayan Ray wrote:
> > Hi,
>
> Hi again.
>
> > I modified the check to check the attributes from the map.
> > There are two checks - one for the normal attributes and
> > one for the ones with arguments, which needs just a bit more
> > processing.
> >
> > So attributes like __packed__ as well as those like
> > __aligned__(x) are handled.
> >
> > What do you think?
> >
> > ---
> > +            $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*\)/)
> > {
> > +            my $attr = trim($1);
> > +            $attr =~ s/\(\s*_*(.*)\)/$1/;
> > +            while($attr =~ s/(.*)_$/$1/) {}  # Remove trailing
> > underscores
>
>         I think this could be a single test like:
>
>                 while ($attr =~ /\s*(\w+)\s*(${balanced_parens})?/g) {
>                         my $curr_attr = $1;
>                         my $parens = $2;
>                         $curr_attr = s/^_*(.*)_*$/$1/;
>

Thanks, I will do that.

Also I tried the pattern  attr =~ s/^_*(.*)_*$/$1/
for trimming the _ earlier. I think it doesn't trim the
trailing underscores in the suffix as (.*) captures everything greedily.

Is the iterative one perhaps okay instead?
while($attr =~ s/(.*)_$/$1/) {}

> > +            my %attr_list = (
> > +                "alias"            => "__alias",
> > +                "aligned"        => "__aligned",
>
> These might be better using tab alignment.
>
> And you might special case format(printf/scanf here too

Yes I think they are special cases now too. I didn't post the
entire context, so my mistake.

Thanks,
Dwaipayan.
Joe Perches Oct. 23, 2020, 9:04 p.m. UTC | #7
On Sat, 2020-10-24 at 02:27 +0530, Dwaipayan Ray wrote:
> Also I tried the pattern  attr =~ s/^_*(.*)_*$/$1/
> for trimming the _ earlier. I think it doesn't trim the
> trailing underscores in the suffix as (.*) captures everything greedily.
>
> Is the iterative one perhaps okay instead?
> while($attr =~ s/(.*)_$/$1/) {}

Then perhaps
 
	$curr_attr =~ s/^_+//; $curr_attr =~ s/_+$//;
Dwaipayan Ray Oct. 23, 2020, 9:09 p.m. UTC | #8
On Sat, Oct 24, 2020 at 2:34 AM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2020-10-24 at 02:27 +0530, Dwaipayan Ray wrote:
> > Also I tried the pattern  attr =~ s/^_*(.*)_*$/$1/
> > for trimming the _ earlier. I think it doesn't trim the
> > trailing underscores in the suffix as (.*) captures everything greedily.
> >
> > Is the iterative one perhaps okay instead?
> > while($attr =~ s/(.*)_$/$1/) {}
>
> Then perhaps
>
>         $curr_attr =~ s/^_+//; $curr_attr =~ s/_+$//;
>
Okay thanks!
I will send in a new version when everything looks good
and tested.

Regards,
Dwaipayan.

Patch
diff mbox series

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7e505688257a..69324c2e55d6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6155,50 +6155,108 @@  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 =~ /\b__attribute__\s*\(\s*\((.*)\)\s*\)/) {
+			my $attr = trim($1);
+
+			my $attr_list = qr {
+				__alias__|
+				__aligned__$|
+				__aligned__\(.*\)|
+				__always_inline__|
+				__assume_aligned__\(.*\)|
+				__cold__|
+				__const__|
+				__copy__\(.*\)|
+				__designated_init__|
+				__externally_visible__|
+				__fallthrough__|
+				__gnu_inline__|
+				__malloc__|
+				__mode__\(.*\)|
+				__no_caller_saved_registers__|
+				__noclone__|
+				__noinline__|
+				__nonstring__|
+				__noreturn__|
+				__packed__|
+				__pure__|
+				__used__
+			}x;
 
-# 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);
-		}
+			my %attr_replace = (
+				"__alias__"			=> "__alias",
+				"__aligned__"		=> "__aligned_largest",
+				"__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 =~/^($attr_list)/) {
+				my $old = $1;
+				if ($old =~ /^(__.+__\()(.*)\)/) {
+					my $new = $attr_replace{$1};
+					WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+					     "$new($2) is preffered over __attribute__(($old))\n" . $herecurr);
+				} else {
+					my $new = $attr_replace{$old};
+					WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+					     "$new is preffered over __attribute__(($old))\n" . $herecurr);
+				}
+			}
 
-# 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 ($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__ 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);
 			}
 		}