Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 26 Mar 2020 14:00:01 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     tglx@linutronix.de, jpoimboe@redhat.com
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, mhiramat@kernel.org,
        mbenes@suse.cz
Subject: Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case
Message-ID: <20200326130001.GE20760@hirez.programming.kicks-ass.net>
References: <20200325174525.772641599@infradead.org>
 <20200325174605.369570202@infradead.org>
 <20200326113049.GD20696@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200326113049.GD20696@hirez.programming.kicks-ass.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> Subject: objtool: Remove CFI save/restore special case
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 25 12:58:16 CET 2020
> 
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
> 
> The reason for this special case comes apparent when we remove it;
> code like:
> 
> 	if (cond) {
> 		UNWIND_HINT_SAVE
> 		// do stuff
> 		UNWIND_HINT_RESTORE
> 	}
> 	// more stuff
> 
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
> 
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.

> @@ -727,6 +727,13 @@ static inline void sync_core(void)
>  #else
>  	unsigned int tmp;
>  
> +	/*
> +	 * The trailing NOP is required to make this an extended basic block,
> +	 * such that we can argue about it locally. Specifically this is
> +	 * important for the UNWIND_HINTs, without this the UNWIND_HINT_RESTORE
> +	 * can fall outside our extended basic block and objtool gets
> +	 * (rightfully) confused.
> +	 */
>  	asm volatile (
>  		UNWIND_HINT_SAVE
>  		"mov %%ss, %0\n\t"
> @@ -739,7 +746,7 @@ static inline void sync_core(void)
>  		"pushq $1f\n\t"
>  		"iretq\n\t"
>  		UNWIND_HINT_RESTORE
> -		"1:"
> +		"1: nop\n\t"
>  		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");

Note that the special case very much relies on the HINT_SAVE being the
first instruction of the (extended) basic block, which is only true in
this one usage anyway.