diff mbox series

[RFC,2/2] kbuild: Rename and use BINUTILS_VERSION where needed

Message ID 20200419132142.173861-1-sedat.dilek@gmail.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Sedat Dilek April 19, 2020, 1:21 p.m. UTC 
In the first patch I introduced LD_IS_BINUTILS Kconfig.

To be consistent in naming convention I renamed from LD_VERSION
to BINUTILS_VERSION.

So, we have the double "LD_IS_BINUTILS" and "BINUTILS_VERSION"
like "CC_IS_GCC" and "GCC_VERSION".

For the same reason I renamed the shell script to detect the GNU ld
linker version.

In Documentation/process/changes.rst we use "binutils" and GNU ld
binary is part of it (see [3]).

The patches "init/kconfig: Add LD_VERSION Kconfig" (see [1]) and
"arm64: Kconfig: ptrauth: Add binutils version check to fix mismatch"
(see [2]) added checks for binutils >=2.23.1 whereas binutils
version 2.23 is minimum supported version (see [3]).

I have renamed to BINUTILS_VERSION where needed.

[1] https://git.kernel.org/linus/9553d16fa671b9621c5e2847d08bd90d3be3349c
[2] https://git.kernel.org/linus/15cd0e675f3f76b4d21c313795fe0c23df0ee20f
[3] https://git.kernel.org/linus/Documentation/process/changes.rst#n79

Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
---
 arch/arm64/Kconfig                             | 2 +-
 init/Kconfig                                   | 4 ++--
 scripts/{ld-version.sh => binutils-version.sh} | 0
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename scripts/{ld-version.sh => binutils-version.sh} (100%)

Comments

Nathan Chancellor April 19, 2020, 4:40 p.m. UTC | #1 
Hi Sedat,

On Sun, Apr 19, 2020 at 03:21:42PM +0200, Sedat Dilek wrote:
> In the first patch I introduced LD_IS_BINUTILS Kconfig.

This sentence can removed, this offers no context when applied to the
git tree.

> To be consistent in naming convention I renamed from LD_VERSION
> to BINUTILS_VERSION.

Is this the only motivation behind this patch? Is this fixing a bug that
you have notice or is it just because there is a linker aside from GNU
ld that works with the kernel?

> So, we have the double "LD_IS_BINUTILS" and "BINUTILS_VERSION"
> like "CC_IS_GCC" and "GCC_VERSION".
> 
> For the same reason I renamed the shell script to detect the GNU ld
> linker version.
> 
> In Documentation/process/changes.rst we use "binutils" and GNU ld
> binary is part of it (see [3]).

                            ^ Kind of weird to have 3 come before 1 and 2

I also don't think that linking to the documentation like that is
necessary.

> The patches "init/kconfig: Add LD_VERSION Kconfig" (see [1]) and
> "arm64: Kconfig: ptrauth: Add binutils version check to fix mismatch"
> (see [2]) added checks for binutils >=2.23.1 whereas binutils

                                       ^2.33.1

> version 2.23 is minimum supported version (see [3]).

I am not sure what relevance this parapgraph has to the patch?

If it is indeed relevant, it should be reworked to use the standard
kernel way of referring to commits:

    9553d16fa671 ("init/kconfig: Add LD_VERSION Kconfig")
    15cd0e675f3f ("arm64: Kconfig: ptrauth: Add binutils version check to fix mismatch")

instead of the title of the patch with a link to the web interface.

> I have renamed to BINUTILS_VERSION where needed.
> 
> [1] https://git.kernel.org/linus/9553d16fa671b9621c5e2847d08bd90d3be3349c
> [2] https://git.kernel.org/linus/15cd0e675f3f76b4d21c313795fe0c23df0ee20f
> [3] https://git.kernel.org/linus/Documentation/process/changes.rst#n79
> 
> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
>  arch/arm64/Kconfig                             | 2 +-
>  init/Kconfig                                   | 4 ++--
>  scripts/{ld-version.sh => binutils-version.sh} | 0
>  3 files changed, 3 insertions(+), 3 deletions(-)
>  rename scripts/{ld-version.sh => binutils-version.sh} (100%)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 40fb05d96c60..274ba9b3ac95 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1504,7 +1504,7 @@ config ARM64_PTR_AUTH
>  	depends on (CC_HAS_SIGN_RETURN_ADDRESS || CC_HAS_BRANCH_PROT_PAC_RET) && AS_HAS_PAC
>  	# GCC 9.1 and later inserts a .note.gnu.property section note for PAC
>  	# which is only understood by binutils starting with version 2.33.1.
> -	depends on !CC_IS_GCC || GCC_VERSION < 90100 || LD_VERSION >= 233010000
> +	depends on !CC_IS_GCC || GCC_VERSION < 90100 || BINUTILS_VERSION >= 233010000
>  	depends on !CC_IS_CLANG || AS_HAS_CFI_NEGATE_RA_STATE
>  	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
>  	help
> diff --git a/init/Kconfig b/init/Kconfig
> index 520116efea0f..946db4786951 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -19,9 +19,9 @@ config GCC_VERSION
>  config LD_IS_BINUTILS
>  	def_bool $(success,$(LD) -v | head -n 1 | grep -q 'GNU ld')
>  
> -config LD_VERSION
> +config BINUTILS_VERSION
>  	int
> -	default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh) if LD_IS_BINUTILS
> +	default $(shell,$(LD) --version | $(srctree)/scripts/binutils-version.sh) if LD_IS_BINUTILS

This is not the only place that ld-version.sh is used, all of these
spots need to be updated with the move.

$ rg --no-heading "ld-version.sh"
scripts/coccicheck:SPATCH_VERSION_NUM=$(echo $SPATCH_VERSION | ${DIR}/scripts/ld-version.sh)
scripts/coccicheck:    REQ_NUM=$(echo $REQ | ${DIR}/scripts/ld-version.sh)
scripts/nsdeps:SPATCH_REQ_VERSION_NUM=$(echo $SPATCH_REQ_VERSION | ${DIR}/scripts/ld-version.sh)
scripts/nsdeps:SPATCH_VERSION_NUM=$(echo $SPATCH_VERSION | ${DIR}/scripts/ld-version.sh)
scripts/Kbuild.include:ld-version = $(shell $(LD) --version | $(srctree)/scripts/ld-version.sh)
init/Kconfig:	default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh)

>  config CC_IS_CLANG
>  	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> diff --git a/scripts/ld-version.sh b/scripts/binutils-version.sh
> similarity index 100%
> rename from scripts/ld-version.sh
> rename to scripts/binutils-version.sh
> -- 
> 2.26.1
> 

For what it's worth, I think the idea behind this patch is fine but I do
not think the first patch in the series is necessary. The only reason
that LD_IS_BINUTILS even exists is to hide BINUTILS_VERSION. However,
the only reason I have seen from you to do this is to match GCC_VERSION.
The reason that CONFIG_GCC_VERSION does not run gcc-version.sh is to
avoid mistaking clang for gcc due to the use of the __GNUC*__ macros.
scripts/ld-version.sh already returns zero for ld.lld so I think that
it is just sufficient to add CONFIG_LD_IS_LLD whenever we are ready for
it (I might have a reason to send it out today, we'll see).

$ ld.lld --version | ./scripts/ld-version.sh
0

Cheers,
Nathan
Masahiro Yamada April 24, 2020, 2:38 a.m. UTC | #2 
On Sun, Apr 19, 2020 at 10:21 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> In the first patch I introduced LD_IS_BINUTILS Kconfig.
>
> To be consistent in naming convention I renamed from LD_VERSION
> to BINUTILS_VERSION.
>
> So, we have the double "LD_IS_BINUTILS" and "BINUTILS_VERSION"
> like "CC_IS_GCC" and "GCC_VERSION".
>
> For the same reason I renamed the shell script to detect the GNU ld
> linker version.
>
> In Documentation/process/changes.rst we use "binutils" and GNU ld
> binary is part of it (see [3]).
>
> The patches "init/kconfig: Add LD_VERSION Kconfig" (see [1]) and
> "arm64: Kconfig: ptrauth: Add binutils version check to fix mismatch"
> (see [2]) added checks for binutils >=2.23.1 whereas binutils
> version 2.23 is minimum supported version (see [3]).
>
> I have renamed to BINUTILS_VERSION where needed.



I do not think this is the right thing to do.

As the doc implies
https://lore.kernel.org/lkml/20200224174129.2664-1-ndesaulniers@google.com/T/
we support overriding CC, LD, ... etc. individually.
(such a usage might be rare with LLVM=1 syntax supported, though)



I'd rather want to stick to LD_VERSION
instead of the version of the tool suite.



config BINUTILS_VERSION
       int
       default $(shell,$(LD) --version |
$(srctree)/scripts/binutils-version.sh) if LD_IS_BINUTILS


This will leave BINUTILS_VERSION empty when LD=ld.lld,
but it looks strange if binutils is still used for other tools.




>
> [1] https://git.kernel.org/linus/9553d16fa671b9621c5e2847d08bd90d3be3349c
> [2] https://git.kernel.org/linus/15cd0e675f3f76b4d21c313795fe0c23df0ee20f
> [3] https://git.kernel.org/linus/Documentation/process/changes.rst#n79
>
> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
>  arch/arm64/Kconfig                             | 2 +-
>  init/Kconfig                                   | 4 ++--
>  scripts/{ld-version.sh => binutils-version.sh} | 0
>  3 files changed, 3 insertions(+), 3 deletions(-)
>  rename scripts/{ld-version.sh => binutils-version.sh} (100%)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 40fb05d96c60..274ba9b3ac95 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1504,7 +1504,7 @@ config ARM64_PTR_AUTH
>         depends on (CC_HAS_SIGN_RETURN_ADDRESS || CC_HAS_BRANCH_PROT_PAC_RET) && AS_HAS_PAC
>         # GCC 9.1 and later inserts a .note.gnu.property section note for PAC
>         # which is only understood by binutils starting with version 2.33.1.
> -       depends on !CC_IS_GCC || GCC_VERSION < 90100 || LD_VERSION >= 233010000
> +       depends on !CC_IS_GCC || GCC_VERSION < 90100 || BINUTILS_VERSION >= 233010000
>         depends on !CC_IS_CLANG || AS_HAS_CFI_NEGATE_RA_STATE
>         depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
>         help
> diff --git a/init/Kconfig b/init/Kconfig
> index 520116efea0f..946db4786951 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -19,9 +19,9 @@ config GCC_VERSION
>  config LD_IS_BINUTILS
>         def_bool $(success,$(LD) -v | head -n 1 | grep -q 'GNU ld')
>
> -config LD_VERSION
> +config BINUTILS_VERSION
>         int
> -       default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh) if LD_IS_BINUTILS
> +       default $(shell,$(LD) --version | $(srctree)/scripts/binutils-version.sh) if LD_IS_BINUTILS
>
>  config CC_IS_CLANG
>         def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> diff --git a/scripts/ld-version.sh b/scripts/binutils-version.sh
> similarity index 100%
> rename from scripts/ld-version.sh
> rename to scripts/binutils-version.sh
> --
> 2.26.1
>
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 40fb05d96c60..274ba9b3ac95 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1504,7 +1504,7 @@  config ARM64_PTR_AUTH
 	depends on (CC_HAS_SIGN_RETURN_ADDRESS || CC_HAS_BRANCH_PROT_PAC_RET) && AS_HAS_PAC
 	# GCC 9.1 and later inserts a .note.gnu.property section note for PAC
 	# which is only understood by binutils starting with version 2.33.1.
-	depends on !CC_IS_GCC || GCC_VERSION < 90100 || LD_VERSION >= 233010000
+	depends on !CC_IS_GCC || GCC_VERSION < 90100 || BINUTILS_VERSION >= 233010000
 	depends on !CC_IS_CLANG || AS_HAS_CFI_NEGATE_RA_STATE
 	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
 	help
diff --git a/init/Kconfig b/init/Kconfig
index 520116efea0f..946db4786951 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -19,9 +19,9 @@  config GCC_VERSION
 config LD_IS_BINUTILS
 	def_bool $(success,$(LD) -v | head -n 1 | grep -q 'GNU ld')
 
-config LD_VERSION
+config BINUTILS_VERSION
 	int
-	default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh) if LD_IS_BINUTILS
+	default $(shell,$(LD) --version | $(srctree)/scripts/binutils-version.sh) if LD_IS_BINUTILS
 
 config CC_IS_CLANG
 	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
diff --git a/scripts/ld-version.sh b/scripts/binutils-version.sh
similarity index 100%
rename from scripts/ld-version.sh
rename to scripts/binutils-version.sh