From: Nathan Chancellor <nathan@kernel.org> To: Jian Cai <jiancai@google.com> Cc: stable@vger.kernel.org, gregkh@linuxfoundation.org, sashal@kernel.org, ndesaulniers@google.com, manojgupta@google.com, llozano@google.com, clang-built-linux@googlegroups.com, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments Date: Wed, 5 May 2021 14:07:14 -0700 [thread overview] Message-ID: <YJMJAiwMPqlWmr8Y@archlinux-ax161> (raw) In-Reply-To: <20210416232341.2421342-1-jiancai@google.com> Hi Jian, On Fri, Apr 16, 2021 at 04:23:41PM -0700, Jian Cai wrote: > LLVM's integrated assembler does not support using commas separating > the name and arguments in .macro. However, only spaces are used in the > manual page. This replaces commas between macro names and the subsequent > arguments with space in calls to clock_gettime_return to make it > compatible with IAS. > > Link: > https://sourceware.org/binutils/docs/as/Macro.html#Macro > https://github.com/ClangBuiltLinux/linux/issues/1349 > > Signed-off-by: Jian Cai <jiancai@google.com> The actual patch itself looks fine to me but there should be some more explanation in the commit message that this patch is for 4.19 only and why it is not applicable upstream. Additionally, I would recommend using the '--subject-prefix=' flag to 'git format-patch' to clarify that as well, something like '--subject-prefix="PATCH 4.19 ONLY"'? My explanation would be something like (take bits and pieces as you feel necessary): ======================================================================== [PATCH 4.19 ONLY] arm64: vdso: remove commas between macro name and arguments LLVM's integrated assembler does not support using a comma to separate a macro name and its arguments when there is only one argument with a default value: 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. ======================================================================== I worded the first sentence the way that I did because correct me if I am wrong but it seems that the integrated assembler has no issues with the use of commas separating the arguments in a .macro definition as that is done everywhere in arch/arm64, just not when there is a single parameter with a default value because essentially what it is evaluating it to is "clock_gettime_return ,shift=1", which according to the GAS manual [1] means that "shift" is actually being set to 0 then there is an other parameter, when it expects only one. [1]: After the definition is complete, you can call the macro either as ‘reserve_str a,b’ (with ‘\p1’ evaluating to a and ‘\p2’ evaluating to b), or as ‘reserve_str ,b’ (with ‘\p1’ evaluating as the default, in this case ‘0’, and ‘\p2’ evaluating to b). Lastly, Will or Catalin should ack this as an explicitly out of mainline patch so that Greg or Sasha can take it. I would put them on the "To:" line in addition to Greg and Sasha. Hopefully this is helpful! Cheers, Nathan > --- > > Changes v1 -> v2: > Keep the comma in the macro definition to be consistent with other > definitions. > > Changes v2 -> v3: > Edit tags. > > 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: > -- > 2.31.1.368.gbe11c130af-goog >
WARNING: multiple messages have this Message-ID (diff)
From: Nathan Chancellor <nathan@kernel.org> To: Jian Cai <jiancai@google.com> Cc: stable@vger.kernel.org, gregkh@linuxfoundation.org, sashal@kernel.org, ndesaulniers@google.com, manojgupta@google.com, llozano@google.com, clang-built-linux@googlegroups.com, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments Date: Wed, 5 May 2021 14:07:14 -0700 [thread overview] Message-ID: <YJMJAiwMPqlWmr8Y@archlinux-ax161> (raw) In-Reply-To: <20210416232341.2421342-1-jiancai@google.com> Hi Jian, On Fri, Apr 16, 2021 at 04:23:41PM -0700, Jian Cai wrote: > LLVM's integrated assembler does not support using commas separating > the name and arguments in .macro. However, only spaces are used in the > manual page. This replaces commas between macro names and the subsequent > arguments with space in calls to clock_gettime_return to make it > compatible with IAS. > > Link: > https://sourceware.org/binutils/docs/as/Macro.html#Macro > https://github.com/ClangBuiltLinux/linux/issues/1349 > > Signed-off-by: Jian Cai <jiancai@google.com> The actual patch itself looks fine to me but there should be some more explanation in the commit message that this patch is for 4.19 only and why it is not applicable upstream. Additionally, I would recommend using the '--subject-prefix=' flag to 'git format-patch' to clarify that as well, something like '--subject-prefix="PATCH 4.19 ONLY"'? My explanation would be something like (take bits and pieces as you feel necessary): ======================================================================== [PATCH 4.19 ONLY] arm64: vdso: remove commas between macro name and arguments LLVM's integrated assembler does not support using a comma to separate a macro name and its arguments when there is only one argument with a default value: 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. ======================================================================== I worded the first sentence the way that I did because correct me if I am wrong but it seems that the integrated assembler has no issues with the use of commas separating the arguments in a .macro definition as that is done everywhere in arch/arm64, just not when there is a single parameter with a default value because essentially what it is evaluating it to is "clock_gettime_return ,shift=1", which according to the GAS manual [1] means that "shift" is actually being set to 0 then there is an other parameter, when it expects only one. [1]: After the definition is complete, you can call the macro either as ‘reserve_str a,b’ (with ‘\p1’ evaluating to a and ‘\p2’ evaluating to b), or as ‘reserve_str ,b’ (with ‘\p1’ evaluating as the default, in this case ‘0’, and ‘\p2’ evaluating to b). Lastly, Will or Catalin should ack this as an explicitly out of mainline patch so that Greg or Sasha can take it. I would put them on the "To:" line in addition to Greg and Sasha. Hopefully this is helpful! Cheers, Nathan > --- > > Changes v1 -> v2: > Keep the comma in the macro definition to be consistent with other > definitions. > > Changes v2 -> v3: > Edit tags. > > 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: > -- > 2.31.1.368.gbe11c130af-goog > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-05-05 21:07 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-16 18:14 [PATCH] arm64: vdso: remove commas between macro name and arguments Jian Cai 2021-04-16 18:14 ` Jian Cai 2021-04-16 18:23 ` Jian Cai 2021-04-16 18:23 ` Jian Cai 2021-04-16 19:08 ` Ard Biesheuvel 2021-04-16 19:08 ` Ard Biesheuvel 2021-04-16 20:35 ` [PATCH v2] " Jian Cai 2021-04-16 20:35 ` Jian Cai 2021-04-16 21:40 ` Ard Biesheuvel 2021-04-16 21:40 ` Ard Biesheuvel 2021-04-16 23:23 ` [PATCH v3] " Jian Cai 2021-04-16 23:23 ` Jian Cai 2021-05-05 21:07 ` Nathan Chancellor [this message] 2021-05-05 21:07 ` Nathan Chancellor 2021-05-05 21:16 ` Nathan Chancellor 2021-05-05 21:16 ` Nathan Chancellor 2021-05-06 1:03 ` Jian Cai 2021-05-06 1:03 ` Jian Cai 2021-05-06 1:30 ` Jian Cai 2021-05-06 1:30 ` Jian Cai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YJMJAiwMPqlWmr8Y@archlinux-ax161 \ --to=nathan@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=clang-built-linux@googlegroups.com \ --cc=gregkh@linuxfoundation.org \ --cc=jiancai@google.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=llozano@google.com \ --cc=manojgupta@google.com \ --cc=ndesaulniers@google.com \ --cc=sashal@kernel.org \ --cc=stable@vger.kernel.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.