kbuild: Update ld-version.sh for change in LLD version output

Commit Message

Nathan Chancellor July 4, 2024, 4:18 p.m. UTC

After [1] in upstream LLVM, ld.lld's version output is slightly
different when the cmake configuration option LLVM_APPEND_VC_REV is
disabled.

Before:

  Debian LLD 19.0.0 (compatible with GNU linkers)

After:

  Debian LLD 19.0.0, compatible with GNU linkers

This results in ld-version.sh failing with

  scripts/ld-version.sh: 19: arithmetic expression: expecting EOF: "10000 * 19 + 100 * 0 + 0,"

because the trailing comma is included in the patch level part of the
expression. Remove the trailing comma when assigning the version
variable in the LLD block to resolve the error, resulting in the proper
output:

  LLD 190000

With LLVM_APPEND_VC_REV enabled, there is no issue with the new output
because it is treated the same as the prior LLVM_APPEND_VC_REV=OFF
version string was.

  ClangBuiltLinux LLD 19.0.0 (https://github.com/llvm/llvm-project a3c5c83273358a85a4e02f5f76379b1a276e7714), compatible with GNU linkers

Cc: [e-mail]
Fixes: 02aff8592204 ("kbuild: check the minimum linker version in Kconfig")
Link: https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb [1]
Signed-off-by: Nathan Chancellor <[e-mail]>
---
 scripts/ld-version.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


---
base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
change-id: 20240704-update-ld-version-for-new-lld-ver-str-b7a4afbbd5f1

Best regards,

Comments

Fangrui Song July 4, 2024, 9:23 p.m. UTC | #1

On Thu, Jul 4, 2024 at 9:19 AM Nathan Chancellor <[e-mail]> wrote:
>
> After [1] in upstream LLVM, ld.lld's version output is slightly
> different when the cmake configuration option LLVM_APPEND_VC_REV is
> disabled.
>
> Before:
>
>   Debian LLD 19.0.0 (compatible with GNU linkers)
>
> After:
>
>   Debian LLD 19.0.0, compatible with GNU linkers
>
> This results in ld-version.sh failing with
>
>   scripts/ld-version.sh: 19: arithmetic expression: expecting EOF: "10000 * 19 + 100 * 0 + 0,"
>
> because the trailing comma is included in the patch level part of the
> expression. Remove the trailing comma when assigning the version
> variable in the LLD block to resolve the error, resulting in the proper
> output:
>
>   LLD 190000
>
> With LLVM_APPEND_VC_REV enabled, there is no issue with the new output
> because it is treated the same as the prior LLVM_APPEND_VC_REV=OFF
> version string was.
>
>   ClangBuiltLinux LLD 19.0.0 (https://github.com/llvm/llvm-project a3c5c83273358a85a4e02f5f76379b1a276e7714), compatible with GNU linkers
>
> Cc: [e-mail]
> Fixes: 02aff8592204 ("kbuild: check the minimum linker version in Kconfig")
> Link: https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb [1]
> Signed-off-by: Nathan Chancellor <[e-mail]>
> ---
>  scripts/ld-version.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> index a78b804b680c..f2f425322524 100755
> --- a/scripts/ld-version.sh
> +++ b/scripts/ld-version.sh
> @@ -47,7 +47,9 @@ else
>         done
>
>         if [ "$1" = LLD ]; then
> -               version=$2
> +               # LLD after https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb
> +               # may have a trailing comma on the patch version with LLVM_APPEND_VC_REV=off.
> +               version=${2%,}
>                 min_version=$($min_tool_version llvm)
>                 name=LLD
>                 disp_name=LLD
>
> ---
> base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
> change-id: 20240704-update-ld-version-for-new-lld-ver-str-b7a4afbbd5f1
>
> Best regards,
> --
> Nathan Chancellor <[e-mail]>
>

Thanks for catching the issue.
If we want to minimize the number of special cases, perhaps we can
adjust `version=${version%-*}` below to

version=${version%%[^0-9.]*}

(POSIX shell doc:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#:~:text=Remove%20Largest)

${version%%[^0-9.]*} is a simpler form than what glibc uses:

  "LLD"*)
  # Accept LLD 13.0.0 or higher
    AC_CHECK_PROG_VER(LD, $LD, --version,
                    [LLD.* \([0-9][0-9]*\.[0-9.]*\)],
                    [1[3-9].*|[2-9][0-9].*],

Nathan Chancellor July 5, 2024, 4 p.m. UTC | #2

On Thu, Jul 04, 2024 at 02:23:46PM -0700, Fangrui Song wrote:
> On Thu, Jul 4, 2024 at 9:19 AM Nathan Chancellor <[e-mail]> wrote:
> >
> > After [1] in upstream LLVM, ld.lld's version output is slightly
> > different when the cmake configuration option LLVM_APPEND_VC_REV is
> > disabled.
> >
> > Before:
> >
> >   Debian LLD 19.0.0 (compatible with GNU linkers)
> >
> > After:
> >
> >   Debian LLD 19.0.0, compatible with GNU linkers
> >
> > This results in ld-version.sh failing with
> >
> >   scripts/ld-version.sh: 19: arithmetic expression: expecting EOF: "10000 * 19 + 100 * 0 + 0,"
> >
> > because the trailing comma is included in the patch level part of the
> > expression. Remove the trailing comma when assigning the version
> > variable in the LLD block to resolve the error, resulting in the proper
> > output:
> >
> >   LLD 190000
> >
> > With LLVM_APPEND_VC_REV enabled, there is no issue with the new output
> > because it is treated the same as the prior LLVM_APPEND_VC_REV=OFF
> > version string was.
> >
> >   ClangBuiltLinux LLD 19.0.0 (https://github.com/llvm/llvm-project a3c5c83273358a85a4e02f5f76379b1a276e7714), compatible with GNU linkers
> >
> > Cc: [e-mail]
> > Fixes: 02aff8592204 ("kbuild: check the minimum linker version in Kconfig")
> > Link: https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb [1]
> > Signed-off-by: Nathan Chancellor <[e-mail]>
> > ---
> >  scripts/ld-version.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> > index a78b804b680c..f2f425322524 100755
> > --- a/scripts/ld-version.sh
> > +++ b/scripts/ld-version.sh
> > @@ -47,7 +47,9 @@ else
> >         done
> >
> >         if [ "$1" = LLD ]; then
> > -               version=$2
> > +               # LLD after https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb
> > +               # may have a trailing comma on the patch version with LLVM_APPEND_VC_REV=off.
> > +               version=${2%,}
> >                 min_version=$($min_tool_version llvm)
> >                 name=LLD
> >                 disp_name=LLD
> >
> > ---
> > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
> > change-id: 20240704-update-ld-version-for-new-lld-ver-str-b7a4afbbd5f1
> >
> > Best regards,
> > --
> > Nathan Chancellor <[e-mail]>
> >
> 
> Thanks for catching the issue.
> If we want to minimize the number of special cases, perhaps we can
> adjust `version=${version%-*}` below to
> 
> version=${version%%[^0-9.]*}

Thanks for the suggestion! I think this wants to be

  version=${version%%[!0-9.]*}

because of "If an open bracket introduces a bracket expression as in XBD
RE Bracket Expression, except that the <exclamation-mark> character
('!') shall replace the <circumflex> character ('^') in its role in a
non-matching list in the regular expression notation, it shall introduce
a pattern bracket expression." from the link that you have below.

That does work for me with all the different linker versions that I can
easily access (Arch, Debian, Fedora) along with my own self built
toolchains, so it seems like it should be pretty robust.

Masahiro, would you be okay with me sending a v2 with that change or do
you foresee any issues where it would not be sufficient? I would
probably change the comment to:

  # There may be something after the version, such as a distribution's
  # package release number (2.34-4.fc32) or a comma (like LLD adds
  # before the "compatible with GNU linkers" string), so remove anything
  # that is not a number or a period.

> (POSIX shell doc:
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#:~:text=Remove%20Largest)
> 
> ${version%%[^0-9.]*} is a simpler form than what glibc uses:
> 
>   "LLD"*)
>   # Accept LLD 13.0.0 or higher
>     AC_CHECK_PROG_VER(LD, $LD, --version,
>                     [LLD.* \([0-9][0-9]*\.[0-9.]*\)],
>                     [1[3-9].*|[2-9][0-9].*],

Cheers,
Nathan

Fangrui Song July 6, 2024, 3:23 a.m. UTC | #3

On Fri, Jul 5, 2024 at 9:00 AM Nathan Chancellor <[e-mail]> wrote:
>
> On Thu, Jul 04, 2024 at 02:23:46PM -0700, Fangrui Song wrote:
> > On Thu, Jul 4, 2024 at 9:19 AM Nathan Chancellor <[e-mail]> wrote:
> > >
> > > After [1] in upstream LLVM, ld.lld's version output is slightly
> > > different when the cmake configuration option LLVM_APPEND_VC_REV is
> > > disabled.
> > >
> > > Before:
> > >
> > >   Debian LLD 19.0.0 (compatible with GNU linkers)
> > >
> > > After:
> > >
> > >   Debian LLD 19.0.0, compatible with GNU linkers
> > >
> > > This results in ld-version.sh failing with
> > >
> > >   scripts/ld-version.sh: 19: arithmetic expression: expecting EOF: "10000 * 19 + 100 * 0 + 0,"
> > >
> > > because the trailing comma is included in the patch level part of the
> > > expression. Remove the trailing comma when assigning the version
> > > variable in the LLD block to resolve the error, resulting in the proper
> > > output:
> > >
> > >   LLD 190000
> > >
> > > With LLVM_APPEND_VC_REV enabled, there is no issue with the new output
> > > because it is treated the same as the prior LLVM_APPEND_VC_REV=OFF
> > > version string was.
> > >
> > >   ClangBuiltLinux LLD 19.0.0 (https://github.com/llvm/llvm-project a3c5c83273358a85a4e02f5f76379b1a276e7714), compatible with GNU linkers
> > >
> > > Cc: [e-mail]
> > > Fixes: 02aff8592204 ("kbuild: check the minimum linker version in Kconfig")
> > > Link: https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb [1]
> > > Signed-off-by: Nathan Chancellor <[e-mail]>
> > > ---
> > >  scripts/ld-version.sh | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> > > index a78b804b680c..f2f425322524 100755
> > > --- a/scripts/ld-version.sh
> > > +++ b/scripts/ld-version.sh
> > > @@ -47,7 +47,9 @@ else
> > >         done
> > >
> > >         if [ "$1" = LLD ]; then
> > > -               version=$2
> > > +               # LLD after https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb
> > > +               # may have a trailing comma on the patch version with LLVM_APPEND_VC_REV=off.
> > > +               version=${2%,}
> > >                 min_version=$($min_tool_version llvm)
> > >                 name=LLD
> > >                 disp_name=LLD
> > >
> > > ---
> > > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
> > > change-id: 20240704-update-ld-version-for-new-lld-ver-str-b7a4afbbd5f1
> > >
> > > Best regards,
> > > --
> > > Nathan Chancellor <[e-mail]>
> > >
> >
> > Thanks for catching the issue.
> > If we want to minimize the number of special cases, perhaps we can
> > adjust `version=${version%-*}` below to
> >
> > version=${version%%[^0-9.]*}
>
> Thanks for the suggestion! I think this wants to be
>
>   version=${version%%[!0-9.]*}
>
> because of "If an open bracket introduces a bracket expression as in XBD
> RE Bracket Expression, except that the <exclamation-mark> character
> ('!') shall replace the <circumflex> character ('^') in its role in a
> non-matching list in the regular expression notation, it shall introduce
> a pattern bracket expression." from the link that you have below.

Yes! ! should be used instead.

> That does work for me with all the different linker versions that I can
> easily access (Arch, Debian, Fedora) along with my own self built
> toolchains, so it seems like it should be pretty robust.
>
> Masahiro, would you be okay with me sending a v2 with that change or do
> you foresee any issues where it would not be sufficient? I would
> probably change the comment to:
>
>   # There may be something after the version, such as a distribution's
>   # package release number (2.34-4.fc32) or a comma (like LLD adds
>   # before the "compatible with GNU linkers" string), so remove anything
>   # that is not a number or a period.

The v2 change LGTM :)

Reviewed-by: Fangrui Song <[e-mail]>

> > (POSIX shell doc:
> > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#:~:text=Remove%20Largest)
> >
> > ${version%%[^0-9.]*} is a simpler form than what glibc uses:
> >
> >   "LLD"*)
> >   # Accept LLD 13.0.0 or higher
> >     AC_CHECK_PROG_VER(LD, $LD, --version,
> >                     [LLD.* \([0-9][0-9]*\.[0-9.]*\)],
> >                     [1[3-9].*|[2-9][0-9].*],
>
> Cheers,
> Nathan
>

Masahiro Yamada July 6, 2024, 5:08 a.m. UTC | #4

On Sat, Jul 6, 2024 at 1:00 AM Nathan Chancellor <[e-mail]> wrote:
>
> On Thu, Jul 04, 2024 at 02:23:46PM -0700, Fangrui Song wrote:
> > On Thu, Jul 4, 2024 at 9:19 AM Nathan Chancellor <[e-mail]> wrote:
> > >
> > > After [1] in upstream LLVM, ld.lld's version output is slightly
> > > different when the cmake configuration option LLVM_APPEND_VC_REV is
> > > disabled.
> > >
> > > Before:
> > >
> > >   Debian LLD 19.0.0 (compatible with GNU linkers)
> > >
> > > After:
> > >
> > >   Debian LLD 19.0.0, compatible with GNU linkers
> > >
> > > This results in ld-version.sh failing with
> > >
> > >   scripts/ld-version.sh: 19: arithmetic expression: expecting EOF: "10000 * 19 + 100 * 0 + 0,"
> > >
> > > because the trailing comma is included in the patch level part of the
> > > expression. Remove the trailing comma when assigning the version
> > > variable in the LLD block to resolve the error, resulting in the proper
> > > output:
> > >
> > >   LLD 190000
> > >
> > > With LLVM_APPEND_VC_REV enabled, there is no issue with the new output
> > > because it is treated the same as the prior LLVM_APPEND_VC_REV=OFF
> > > version string was.
> > >
> > >   ClangBuiltLinux LLD 19.0.0 (https://github.com/llvm/llvm-project a3c5c83273358a85a4e02f5f76379b1a276e7714), compatible with GNU linkers
> > >
> > > Cc: [e-mail]
> > > Fixes: 02aff8592204 ("kbuild: check the minimum linker version in Kconfig")
> > > Link: https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb [1]
> > > Signed-off-by: Nathan Chancellor <[e-mail]>
> > > ---
> > >  scripts/ld-version.sh | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> > > index a78b804b680c..f2f425322524 100755
> > > --- a/scripts/ld-version.sh
> > > +++ b/scripts/ld-version.sh
> > > @@ -47,7 +47,9 @@ else
> > >         done
> > >
> > >         if [ "$1" = LLD ]; then
> > > -               version=$2
> > > +               # LLD after https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb
> > > +               # may have a trailing comma on the patch version with LLVM_APPEND_VC_REV=off.
> > > +               version=${2%,}
> > >                 min_version=$($min_tool_version llvm)
> > >                 name=LLD
> > >                 disp_name=LLD
> > >
> > > ---
> > > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
> > > change-id: 20240704-update-ld-version-for-new-lld-ver-str-b7a4afbbd5f1
> > >
> > > Best regards,
> > > --
> > > Nathan Chancellor <[e-mail]>
> > >
> >
> > Thanks for catching the issue.
> > If we want to minimize the number of special cases, perhaps we can
> > adjust `version=${version%-*}` below to
> >
> > version=${version%%[^0-9.]*}
>
> Thanks for the suggestion! I think this wants to be
>
>   version=${version%%[!0-9.]*}
>
> because of "If an open bracket introduces a bracket expression as in XBD
> RE Bracket Expression, except that the <exclamation-mark> character
> ('!') shall replace the <circumflex> character ('^') in its role in a
> non-matching list in the regular expression notation, it shall introduce
> a pattern bracket expression." from the link that you have below.
>
> That does work for me with all the different linker versions that I can
> easily access (Arch, Debian, Fedora) along with my own self built
> toolchains, so it seems like it should be pretty robust.
>
> Masahiro, would you be okay with me sending a v2 with that change or do
> you foresee any issues where it would not be sufficient? I would
> probably change the comment to:
>
>   # There may be something after the version, such as a distribution's
>   # package release number (2.34-4.fc32) or a comma (like LLD adds
>   # before the "compatible with GNU linkers" string), so remove anything
>   # that is not a number or a period.



Thanks. V2 seems much cleaner.

Patch

diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
index a78b804b680c..f2f425322524 100755
--- a/scripts/ld-version.sh
+++ b/scripts/ld-version.sh
@@ -47,7 +47,9 @@  else
 	done
 
 	if [ "$1" = LLD ]; then
-		version=$2
+		# LLD after https://github.com/llvm/llvm-project/commit/0f9fbbb63cfcd2069441aa2ebef622c9716f8dbb
+		# may have a trailing comma on the patch version with LLVM_APPEND_VC_REV=off.
+		version=${2%,}
 		min_version=$($min_tool_version llvm)
 		name=LLD
 		disp_name=LLD