From:   Dodji Seketeli <dodji@seketeli.org>
To:     John Moon via Libabigail <libabigail@sourceware.org>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        "Nicolas Schier" <nicolas@fjasle.eu>,
        John Moon <quic_johmoo@quicinc.com>,
        <linux-kbuild@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-arm-msm@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        "Arnd Bergmann" <arnd@arndb.de>,
        Bjorn Andersson <andersson@kernel.org>,
        Todd Kjos <tkjos@google.com>,
        Matthias Maennich <maennich@google.com>,
        Giuliano Procida <gprocida@google.com>,
        <kernel-team@android.com>, Jordan Crouse <jorcrous@amazon.com>,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>,
        Elliot Berman <quic_eberman@quicinc.com>
Subject: Re: [PATCH v2 1/2] check-uapi: Introduce check-uapi.sh
Organization: Me, myself and I
References: <20230301075402.4578-1-quic_johmoo@quicinc.com>
        <20230301075402.4578-2-quic_johmoo@quicinc.com>
Date:   Fri, 03 Mar 2023 22:09:11 +0100
In-Reply-To: <20230301075402.4578-2-quic_johmoo@quicinc.com> (John Moon via
        Libabigail's message of "Tue, 28 Feb 2023 23:54:01 -0800")
Message-ID: <87zg8t5yq0.fsf@seketeli.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello John,

John Moon via Libabigail <libabigail@sourceware.org> a =C3=A9crit:

> While the kernel community has been good at maintaining backwards
> compatibility with kernel UAPIs, it would be helpful to have a tool
> to check if a commit introduces changes that break backwards
> compatibility.
>
> To that end, introduce check-uapi.sh: a simple shell script that
> checks for changes to UAPI headers using libabigail.
>
> libabigail is "a framework which aims at helping developers and
> software distributors to spot some ABI-related issues like interface
> incompatibility in ELF shared libraries by performing a static
> analysis of the ELF binaries at hand."
>
> The script uses one of libabigail's tools, "abidiff", to compile the
> changed header before and after the commit to detect any changes.
>
> abidiff "compares the ABI of two shared libraries in ELF format. It
> emits a meaningful report describing the differences between the two
> ABIs."
>
> The script also includes the ability to check the compatibilty of
> all UAPI headers across commits. This allows developers to inspect
> the stability of the UAPIs over time.

Thank you for working on this.

The libabigail bits look good to me, for what it's worth.  I just have
some general considerations to discuss.

[...]

> +# Perform the A/B compilation and compare output ABI
> +compare_abi() {

[...]

> +	if "$ABIDIFF" --non-reachable-types "${ref_header}.bin" "${base_header}=
.bin" > "$log"; then
> +		printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$r=
ef" "${base_ref:-dirty tree}"
> +	else
> +		# If the only changes were additions (not modifications to existing AP=
Is), then
> +		# there's no problem. Ignore these diffs.
> +		if grep "Unreachable types summary" "$log" | grep -q "0 removed" &&
> +		   grep "Unreachable types summary" "$log" | grep -q "0 changed"; then
> +			return 0

There is no problem in parsing the output of the tool like this.
However, the return code of the tool has been designed as a bit field that
could be analysed to know more about the kind of changes that were
reported: https://sourceware.org/libabigail/manual/abidiff.html#return-valu=
es.

Right now, there is no bit assigned to detect new types (or interface)
addition, but do you think that it would be a helpful new feature to add
to abidiff for this use case?  We can discuss this in a separate thread
if you prefer, so that we don't pollute others with this minutiae.

> +		fi
> +		{
> +			printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$f=
ile" "$ref" "${base_ref:-dirty tree}"
> +			sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "=
$log"

Here again, if you'd like to have a particular output format emitted by
the tool, we'd be glad to discuss how to improve the plasticity of the
tool enough to emit the right output for you.  For instance, we could
add a new --no-summary that would let the tool display the change
directly without the summary header that you are strimming out with this
sed script.

[...]

Thanks again for this tool that I think might be very useful.

Cheers,

--=20
		Dodji