Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>

On Fri, Jun 18, 2021 at 04:18:35PM +0100, Mark Rutland wrote:

On Thu, Jun 17, 2021 at 06:25:27PM -0700, Nathan Chancellor wrote:

quoted

Hi Mark,

Hi Nathan,

quoted

On Wed, Jun 09, 2021 at 11:23:01AM +0100, Mark Rutland wrote:

quoted

For histroical reasons, we define AARCH64_INSN_SIZE in
<asm/alternative-macros.h>, but it would make more sense to do so in
<asm/insn.h>. Let's move it into <asm/insn.h>, and add the necessary
include directives for this.

quoted

I bisected a CONFIG_LTO_CLANG_THIN=y build failure that our CI reported
to this patch:

https://builds.tuxbuild.com/1u4Fpx2FQkkgkyPxWtq0Ke4YFCQ/build.log

Thanks for reporting this; the lopg is really helpful!

quoted

I have not had a whole ton of time to look into this (dealing with a
million fires it seems :^) but it is not immediately obvious to me why
this fails because include/linux/build_bug.h is included within
arch/arm64/include/asm/insn.h.

The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
includes <asm/insn.h>, creating a circular include chain:

	<linux/build_bug.h>
	<linux/compiler.h>
	<asm/rwonce.h>
	<asm/alternative-macros.h>
	<asm/insn.h>
	<linux/build-bug.h>

... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
BUILD_BUG* definitions have happened yet.

Aha, that would certainly explain it. I figured something like this
would be the root cause but figuring out header dependencies is not my
cup of tea.

Will, are you happy to take the fixup patch below, or would you prefer
to drop this patch for now?

Thanks,
Mark.

---->8----

quoted

From 0acc3d92302f54475d938f55749805adf74faec1 Mon Sep 17 00:00:00 2001

From: Mark Rutland <mark.rutland@arm.com>
Date: Fri, 18 Jun 2021 16:11:22 +0100
Subject: [PATCH] arm64: insn: avoid circular include dependency

Nathan reports that when building with CONFIG_LTO_CLANG_THIN=y, the
build fails due to BUILD_BUG_ON() not being defined before its uss in
<asm/insn.h>.

The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
includes <asm/insn.h>, creating a circular include chain:

        <linux/build_bug.h>
        <linux/compiler.h>
        <asm/rwonce.h>
        <asm/alternative-macros.h>
        <asm/insn.h>
        <linux/build-bug.h>

... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
BUILD_BUG* definitions have happened yet.

To avoid this, let's move AARCH64_INSN_SIZE into a header without any
dependencies, such that it can always be safely included. At the same
time, avoid including <asm/alternative.h> in <asm/insn.h>, which should
no longer be necessary (and doesn't make sense when insn.h is consumed
by userspace).

Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/alternative-macros.h | 2 +-
 arch/arm64/include/asm/insn.h               | 5 +----

Looks like arch/arm64/include/asm/insn-def.h is missing from this patch?

If I add one with just the two deleted lines plus a header guard, the
build passes.

Tested-by: Nathan Chancellor <nathan@kernel.org>

Thank you for the quick fix!

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel