checkpatch.pl incorrect ERROR:POINTER_LOCATION

[andrewboie]

Describe the bug

-:1733: ERROR:POINTER_LOCATION: "foo * bar" should be "foo *bar"
#1733: FILE: arch/x86/core/x86_mmu.c:535:
+	page_pool[CONFIG_MMU_PAGE_SIZE * CONFIG_X86_MMU_PAGE_POOL_PAGES];

-:4233: ERROR:POINTER_LOCATION: "foo * bar" should be "foo *bar"
#4233: FILE: include/arch/x86/mmustructs.h:45:
+#define Z_X86_PT_AREA  ((uintptr_t)CONFIG_MMU_PAGE_SIZE * Z_X86_NUM_PT_ENTRIES)

Found in checks for #27001.

Impact
It's leaving a -1 on my patch because this is treated as an error and not a warning.

[andrewboie]

@pabigot is this an upstream checkpatch.pl issue or did one of our local changes cause this? I don't recall ever running into this before, but I recall we recently pulled up to a new version..

[pabigot]

I don't know, but I can tell you that checkpatch thinks that in the given contexts CONFIG_MMU_PAGE_SIZE is a non-pointer type, and that the version of checkpatch before 5b10fac emits the same diagnostic.

It treats CONFIG_MMU_PAGE_SIZE as a possible type because it appears in an __aligned() specifier in:

static uint8_t __noinit __aligned(CONFIG_MMU_PAGE_SIZE)
	page_pool[CONFIG_MMU_PAGE_SIZE * CONFIG_X86_MMU_PAGE_POOL_PAGES];

If you change that to:

static uint8_t __noinit __aligned(0+CONFIG_MMU_PAGE_SIZE)

it becomes unconfused.

[andrewboie]

I sent a patch to turn off the check until this sorts out.

Wondering why it flagged Z_X86_NUM_PT_ENTRIES?
Also seems weird that being inside __aligned() would make this thing think it's a type, since __aligned() takes a size. Anyway, I hate reading Perl code so maybe I'll never know why

[pabigot]

checkpatch doesn't care that it's inside __aligned(); it parses that horrific declaration and misinterprets it as a prototype for a function __aligned with a single CONFIG_MMU_PAGE_SIZE parameter. Which would mean that CONFIG_MMU_PAGE_SIZE must be a type.

The diagnostic can also be eliminated by moving the __aligned() attribute to the end of the declaration, though I haven't confirmed gcc interprets it properly there.

This is upstream checkpatch behavior, not specific to Zephyr. Modifying the script to detect this situation isn't something I'd take on.

[pabigot]

Also from #26785:

-:767: ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
117
#767: FILE: drivers/pinmux/pinmux_mcux_lpc.c:47:
118
+	IOPCTL_Type *base = config->base;

is an expected failure. Because Linux and Zephyr disallow typedefs checkpatch can't recognize that IOPCTL_Type is a type, so it complains about a multiplication that's spaced as A *B.

[pabigot]

Fixing the diagnostics to be correct is not reasible with checkpatch.pl which is a perl script that cannot accurately parse C code. It may be possible to disable the check entirely if/when we move to clang-format, but that's some months away.

2020-07-21 Triage suggested two paths to resolving this:

• Convert the error to a warning so that failures do not mark a PR as unmergeable;
• Acknowledge that checkpatch.pl is not perfect and document known situations where its diagnostic is incorrect and may be ignored.

Taking the first path risks increased introduction of constructs that violate Zephyr's coding style by not attaching the * symbol to the declarator that follows.

The second is addressed by #27035.

[andrewboie]

Taking the first path risks increased introduction of constructs that violate Zephyr's coding style by not attaching the * symbol to the declarator that follows.

How much of a concern is this really, given the costs of false positives in CI runs which leave a -1? Converting to a warning seems very reasonable to me. But it's also true that this issue has been around for a while and works most of the time, at least.

Ultimately I leave to your discretion how we want to deal with it, I appreciate you taking time to look into this.

I can try to massage my patch to get the warning to go away in the immediate term.