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;
Subject: Re: [PATCH 4.19 ONLY v4] arm64: vdso: remove commas between macro
 name and arguments
To:     Jian Cai <jiancai@google.com>, gregkh@linuxfoundation.org,
        sashal@kernel.org, will@kernel.org, catalin.marinas@arm.com
Cc:     stable@vger.kernel.org, ndesaulniers@google.com,
        manojgupta@google.com, llozano@google.com,
        clang-built-linux@googlegroups.com,
        Will Deacon <will.deacon@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20210506012508.3822221-1-jiancai@google.com>
From:   Nathan Chancellor <nathan@kernel.org>
Message-ID: <fd08dce2-71c0-3414-d661-d065480c04ff@kernel.org>
Date:   Thu, 6 May 2021 10:43:33 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210506012508.3822221-1-jiancai@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/5/2021 6:25 PM, Jian Cai wrote:
> LLVM's integrated assembler appears to assume an argument with default
> value is passed whenever it sees a comma right after the macro name.
> It will be fine if the number of following arguments is one less than
> the number of parameters specified in the macro definition. Otherwise,
> it fails. For example, the following code works:
> 
> $ cat foo.s
> .macro  foo arg1=2, arg2=4
>          ldr r0, [r1, #\arg1]
>          ldr r0, [r1, #\arg2]
> .endm
> 
> foo, arg2=8
> 
> $ llvm-mc -triple=armv7a -filetype=obj foo.s -o ias.o
> arm-linux-gnueabihf-objdump -dr ias.o
> 
> ias.o:     file format elf32-littlearm
> 
> Disassembly of section .text:
> 
> 00000000 <.text>:
>     0: e5910001 ldr r0, [r1, #2]
>     4: e5910003 ldr r0, [r1, #8]
> 
> While the the following code would fail:
> 
> $ cat foo.s
> .macro  foo arg1=2, arg2=4
>          ldr r0, [r1, #\arg1]
>          ldr r0, [r1, #\arg2]
> .endm
> 
> foo, arg1=2, arg2=8
> 
> $ llvm-mc -triple=armv7a -filetype=obj foo.s -o ias.o
> foo.s:6:14: error: too many positional arguments
> foo, arg1=2, arg2=8
> 
> This causes build failures as follows:
> 
> arch/arm64/kernel/vdso/gettimeofday.S:230:24: error: too many positional
> arguments
>   clock_gettime_return, shift=1
>                         ^
> arch/arm64/kernel/vdso/gettimeofday.S:253:24: error: too many positional
> arguments
>   clock_gettime_return, shift=1
>                         ^
> arch/arm64/kernel/vdso/gettimeofday.S:274:24: error: too many positional
> arguments
>   clock_gettime_return, shift=1
> 
> This error is not in mainline because commit 28b1a824a4f4 ("arm64: vdso:
> Substitute gettimeofday() with C implementation") rewrote this assembler
> file in C as part of a 25 patch series that is unsuitable for stable.
> Just remove the comma in the clock_gettime_return invocations in 4.19 so
> that GNU as and LLVM's integrated assembler work the same.
> 
> Link:
> https://github.com/ClangBuiltLinux/linux/issues/1349
> 
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Jian Cai <jiancai@google.com>

Thanks for the updated example and explanation, this looks good to me now.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
> Changes v1 -> v2:
>    Keep the comma in the macro definition to be consistent with other
>    definitions.
> 
> Changes v2 -> v3:
>    Edit tags.
> 
> Changes v3 -> v4:
>    Update the commit message based on Nathan's comments.
> 
>   arch/arm64/kernel/vdso/gettimeofday.S | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
> index 856fee6d3512..b6faf8b5d1fe 100644
> --- a/arch/arm64/kernel/vdso/gettimeofday.S
> +++ b/arch/arm64/kernel/vdso/gettimeofday.S
> @@ -227,7 +227,7 @@ realtime:
>   	seqcnt_check fail=realtime
>   	get_ts_realtime res_sec=x10, res_nsec=x11, \
>   		clock_nsec=x15, xtime_sec=x13, xtime_nsec=x14, nsec_to_sec=x9
> -	clock_gettime_return, shift=1
> +	clock_gettime_return shift=1
>   
>   	ALIGN
>   monotonic:
> @@ -250,7 +250,7 @@ monotonic:
>   		clock_nsec=x15, xtime_sec=x13, xtime_nsec=x14, nsec_to_sec=x9
>   
>   	add_ts sec=x10, nsec=x11, ts_sec=x3, ts_nsec=x4, nsec_to_sec=x9
> -	clock_gettime_return, shift=1
> +	clock_gettime_return shift=1
>   
>   	ALIGN
>   monotonic_raw:
> @@ -271,7 +271,7 @@ monotonic_raw:
>   		clock_nsec=x15, nsec_to_sec=x9
>   
>   	add_ts sec=x10, nsec=x11, ts_sec=x13, ts_nsec=x14, nsec_to_sec=x9
> -	clock_gettime_return, shift=1
> +	clock_gettime_return shift=1
>   
>   	ALIGN
>   realtime_coarse:
>