Subject: Re: [PATCH v7 05/28] powerpc: Change calling convention for create_branch() et. al.

From: Alistair Popple <[email-masked]>
Date: Mon, 04 May 2020 12:55:30 +1000

On Friday, 1 May 2020 1:41:57 PM AEST Jordan Niethe wrote:
> create_branch(), create_cond_branch() and translate_branch() return the
> instruction that they create, or return 0 to signal an error. Separate
> these concerns in preparation for an instruction type that is not just
> an unsigned int.  Fill the created instruction to a pointer passed as
> the first parameter to the function and use a non-zero return value to
> signify an error.

We're not adding any new checks for error cases here, but this patch doesn't 
change existing behaviour as far as I can tell and adding checks would be 
difficult (and could introduce other bugs) so would be best done as a separate 
series anyway if required at all.
 
> @@ -403,6 +407,7 @@ static void __init test_trampoline(void)
> 
>  static void __init test_branch_iform(void)
>  {
> +	int err;
>  	unsigned int instr;
>  	unsigned long addr;
> 
> @@ -443,35 +448,35 @@ static void __init test_branch_iform(void)
>  	check(instr_is_branch_to_addr(&instr, addr - 0x2000000));
> 
>  	/* Branch to self, with link */
> -	instr = create_branch(&instr, addr, BRANCH_SET_LINK);
> +	err = create_branch(&instr, &instr, addr, BRANCH_SET_LINK);
>  	check(instr_is_branch_to_addr(&instr, addr));

The pointer alias above initially caught my eye, but it's ok because 
create_branch() doesn't actually dereference the second instance. Arguably the 
argument type could be changed to an unsigned long but then we'd just end up 
with more casts so this is ok to me at least.

Reviewed-by: Alistair Popple <[email-masked]>