Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 12 Dec 2023 12:30:31 -0800
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Dimitri John Ledkov <dimitri.ledkov@canonical.com>
Cc:     Peter Zijlstra <peterz@infradead.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] objtool: Make objtool check actually fatal upon fatal
 errors
Message-ID: <20231212203031.uudk57xhegi5yziz@treble>
References: <20231212185339.1562967-1-dimitri.ledkov@canonical.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231212185339.1562967-1-dimitri.ledkov@canonical.com>
Precedence: bulk

On Tue, Dec 12, 2023 at 06:53:38PM +0000, Dimitri John Ledkov wrote:
> Currently function calls within check() are sensitive to fatal errors
> (negative return codes) and abort execution prematurely. However, in
> all such cases the check() function still returns 0, and thus
> resulting in a successful kernel build.
> 
> The only correct code paths were the ones that escpae the control flow
> with `return ret`.
> 
> Make the check() function return `ret` status code, and make all
> negative return codes goto that instruction. This makes fatal errors
> (not warnings) from various function calls actually fail the
> build. E.g. if create_retpoline_sites_sections() fails to create elf
> section pair retpoline_sites the tool now exits with an error code.
> 
> Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>

We had problems trying this before, but we can try again.  Maybe we'll
have better luck now :-)

>  	if (opts.orc && nr_insns) {
>  		ret = orc_create(file);
>  		if (ret < 0)
>  			goto out;
>  		warnings += ret;
>  	}
>  
>  	free_insns(file);
>  
>  	if (opts.verbose)
>  		disas_warned_funcs(file);
>  
>  	if (opts.stats) {
>  		printf("nr_insns_visited: %ld\n", nr_insns_visited);
>  		printf("nr_cfi: %ld\n", nr_cfi);
>  		printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
>  		printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
>  	}
>  
>  out:
>  	/*
>  	 *  For now, don't fail the kernel build on fatal warnings.  These
>  	 *  errors are still fairly common due to the growing matrix of
>  	 *  supported toolchains and their recent pace of change.
>  	 */
> -	return 0;
> +	return ret;

Here it should check for ret < 0, since orc_create() or some other
function might have returned non-fatal warnings (ret > 0).

Also the comment is no longer relevant.

-- 
Josh