Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201212165431.150750-1-masahiroy@kernel.org> <20201212165431.150750-3-masahiroy@kernel.org>
 <20201212174828.GA17179@nautica>
In-Reply-To: <20201212174828.GA17179@nautica>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Mon, 21 Dec 2020 23:23:02 +0900
Message-ID: <CAK7LNATOMnNDt5E8j1qkj=FELezZ551Amfz3i=eMiSUg0nygCg@mail.gmail.com>
Subject: Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Michal Marek <michal.lkml@markovi.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Dec 13, 2020 at 2:48 AM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Masahiro Yamada wrote on Sun, Dec 13, 2020:
> > This script was written in awk in spite of the file extension '.sh'.
> > Rewrite it as a shell script.
>
> Wow! I wasn't expecting so much, would have sent some rework after the
> upcoming merge window.
> Thank you.
>
> Some comments below that you can probably ignore, this works for me.
>
>
> > diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> > index 0f8a2c0f9502..c214aeb3200d 100755
> > --- a/scripts/ld-version.sh
> > +++ b/scripts/ld-version.sh
> > @@ -1,11 +1,22 @@
> > -#!/usr/bin/awk -f
> > +#!/bin/sh
> >  # SPDX-License-Identifier: GPL-2.0
> > -# extract linker version number from stdin and turn into single number
> > -     {
> > -     gsub(".*\\)", "");
> > -     gsub(".*version ", "");
> > -     gsub("-.*", "");
> > -     split($1,a, ".");
> > -     print a[1]*10000 + a[2]*100 + a[3];
> > -     exit
> > -     }
> > +#
> > +# Usage: $ ./scripts/ld-version.sh ld
> > +#
> > +# Print the linker version of `ld' in a 5 or 6-digit form
> > +# such as `23501' for GNU ld 2.35.1 etc.
> > +
> > +first_line="$($* --version | head -n 1)"
>
> Just nitpicking: this ($*) would fail if the argument contains spaces,
> it's generally better to use "$@" or "$1" (with quotes)

This is just a copy-paste work based on scripts/lld-version.sh.

"$@" is better, I agree.


> Probably doesn't matter here as gcc/clang-version scripts have the same
> problem, so if someone had a problem with that they probably would have
> reported it there.

Talking about gcc/clang-version, "$1" is not acceptable because the first
word of the arguments may not be the compiler.

For example, when CC="ccache gcc" is passed in,
scripts/gcc-version.sh  ccache  gcc
must return the gcc version.


Difference between "$@" and "$*" matters only
when it is directly passed to some command.


masahiro@grover:~$ set  "a   b"  c  d
masahiro@grover:~$ ls  "$*"
ls: cannot access 'a   b c d': No such file or directory
masahiro@grover:~$ ls  "$@"
ls: cannot access 'a   b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory


"$*" was expanded into a single string, 'a   b c d'.
It forgot which spaces were the part of the argument,
and which spaces were argument delimiters.

In contrast, "$@" was expanded into three arguments,
'a   b', 'c', and 'd'.
It correctly preserved the original arguments.


Let me continue some more experiments...


masahiro@grover:~$ set  "a   b"  c  d
masahiro@grover:~$ compiler="$*"
masahiro@grover:~$ ls "$compiler"
ls: cannot access 'a   b c d': No such file or directory
masahiro@grover:~$ ls $compiler
ls: cannot access 'a': No such file or directory
ls: cannot access 'b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory


masahiro@grover:~$ set  "a   b"  c  d
masahiro@grover:~$ compiler="$@"
masahiro@grover:~$ ls "$compiler"
ls: cannot access 'a   b c d': No such file or directory
masahiro@grover:~$ ls $compiler
ls: cannot access 'a': No such file or directory
ls: cannot access 'b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory


The result is the same.
So, whichever we use, after it is assigned to the variable "compiler",
it forgets the origin of the spaces.


If we really want to preserve the passed arguments,
we need to use "$@" directly in scripts/gcc-version.sh
like this:

MAJOR=$(echo __GNUC__ | "$@" -E -x c - | tail -n 1)

If we do this,

  scripts/gcc-version.sh  ccache gcc

will succeed, and

  scripts/gcc-version.sh  "ccache gcc"

will fail.


> > +
> > +if ! ( echo $first_line | grep -q "GNU ld"); then
> > +     echo 0
> > +     exit 1
> > +fi
> > +
> > +# Distributions may append an extra string like 2.35-15.fc33
> > +# Take the part that consists of numbers and dots.
> > +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed 's/^\(^[0-9.]*\).*/\1/')
> > +MAJOR=$(echo $VERSION | cut -d . -f 1)
> > +MINOR=$(echo $VERSION | cut -d . -f 2)
> > +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3)
> > +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
>
> There is a bug if there is no dot at all (e.g. if binutils ever releases
> a version 3 and call it version 3 and not 3.0, the script would print
> 30303 because cut when no delimiter is found always returns the whole
> string)
> This can be fixed by artificially appending a dot to VERSION:
> VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed 's/^\(^[0-9.]*\).*/\1./')
>
> I'm not sure it's worth worrying about either.

Ah, good catch.


--
Best Regards
Masahiro Yamada