* [PATCH] arm64: vdso: remove commas between macro name and arguments @ 2021-04-16 18:14 Jian Cai 2021-04-16 18:23 ` Jian Cai ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Jian Cai @ 2021-04-16 18:14 UTC (permalink / raw) Cc: stable, gregkh, sashal, ndesaulniers, natechancellor, manojgupta, llozano, clang-built-linux, Jian Cai, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel 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 Signed-off-by: Jian Cai <jiancai@google.com> --- arch/arm64/kernel/vdso/gettimeofday.S | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S index 856fee6d3512..7ee685d9adfc 100644 --- a/arch/arm64/kernel/vdso/gettimeofday.S +++ b/arch/arm64/kernel/vdso/gettimeofday.S @@ -122,7 +122,7 @@ x_tmp .req x8 9998: .endm - .macro clock_gettime_return, shift=0 + .macro clock_gettime_return shift=0 .if \shift == 1 lsr x11, x11, x12 .endif @@ -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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: vdso: remove commas between macro name and arguments 2021-04-16 18:14 [PATCH] arm64: vdso: remove commas between macro name and arguments Jian Cai @ 2021-04-16 18:23 ` Jian Cai 2021-04-16 19:08 ` Ard Biesheuvel 2021-04-16 20:35 ` [PATCH v2] " Jian Cai 2 siblings, 0 replies; 10+ messages in thread From: Jian Cai @ 2021-04-16 18:23 UTC (permalink / raw) Cc: # 3.4.x, Greg KH, Sasha Levin, Nick Desaulniers, Nathan Chancellor, Manoj Gupta, Luis Lozano, clang-built-linux, Catalin Marinas, Will Deacon, Linux ARM, Linux Kernel Mailing List I should've mentioned this patch is for 4.19 only as the code has been rewritten in later versions, but requires a large amount of backports (https://github.com/ClangBuiltLinux/linux/issues/1349). This is the only blocker for 4.19 migration to LLVM's integrated assembler on CrOS that I am aware of. Thanks, Jian On Fri, Apr 16, 2021 at 11:14 AM Jian Cai <jiancai@google.com> 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 > Signed-off-by: Jian Cai <jiancai@google.com> > --- > arch/arm64/kernel/vdso/gettimeofday.S | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S > index 856fee6d3512..7ee685d9adfc 100644 > --- a/arch/arm64/kernel/vdso/gettimeofday.S > +++ b/arch/arm64/kernel/vdso/gettimeofday.S > @@ -122,7 +122,7 @@ x_tmp .req x8 > 9998: > .endm > > - .macro clock_gettime_return, shift=0 > + .macro clock_gettime_return shift=0 > .if \shift == 1 > lsr x11, x11, x12 > .endif > @@ -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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm64: vdso: remove commas between macro name and arguments 2021-04-16 18:14 [PATCH] arm64: vdso: remove commas between macro name and arguments Jian Cai 2021-04-16 18:23 ` Jian Cai @ 2021-04-16 19:08 ` Ard Biesheuvel 2021-04-16 20:35 ` [PATCH v2] " Jian Cai 2 siblings, 0 replies; 10+ messages in thread From: Ard Biesheuvel @ 2021-04-16 19:08 UTC (permalink / raw) To: Jian Cai Cc: # 3.4.x, Greg Kroah-Hartman, Sasha Levin, Nick Desaulniers, Nathan Chancellor, Manoj Gupta, Luis Lozano, clang-built-linux, Catalin Marinas, Will Deacon, Linux ARM, Linux Kernel Mailing List On Fri, 16 Apr 2021 at 20:16, Jian Cai <jiancai@google.com> 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 > Signed-off-by: Jian Cai <jiancai@google.com> > --- > arch/arm64/kernel/vdso/gettimeofday.S | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S > index 856fee6d3512..7ee685d9adfc 100644 > --- a/arch/arm64/kernel/vdso/gettimeofday.S > +++ b/arch/arm64/kernel/vdso/gettimeofday.S > @@ -122,7 +122,7 @@ x_tmp .req x8 > 9998: > .endm > > - .macro clock_gettime_return, shift=0 > + .macro clock_gettime_return shift=0 Are you sure the definition needs to be changed as well? The majority of GAS macros in arch/arm64 (if not all) use a comma after the identifier, so I would prefer not to deviate from that arbitrarily. Just look at $ git grep -E \.macro\\s+\\S+, arch/arm64/*.S > .if \shift == 1 > lsr x11, x11, x12 > .endif > @@ -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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] arm64: vdso: remove commas between macro name and arguments 2021-04-16 18:14 [PATCH] arm64: vdso: remove commas between macro name and arguments Jian Cai 2021-04-16 18:23 ` Jian Cai 2021-04-16 19:08 ` Ard Biesheuvel @ 2021-04-16 20:35 ` Jian Cai 2021-04-16 21:40 ` Ard Biesheuvel 2021-04-16 23:23 ` [PATCH v3] " Jian Cai 2 siblings, 2 replies; 10+ messages in thread From: Jian Cai @ 2021-04-16 20:35 UTC (permalink / raw) Cc: stable, gregkh, sashal, ndesaulniers, natechancellor, manojgupta, llozano, clang-built-linux, Jian Cai, Ard Biesheuvel, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel 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 Suggested-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Jian Cai <jiancai@google.com> --- Changes v1 -> v2: Keep the comma in the macro definition to be consistent with other definitions. 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: vdso: remove commas between macro name and arguments 2021-04-16 20:35 ` [PATCH v2] " Jian Cai @ 2021-04-16 21:40 ` Ard Biesheuvel 2021-04-16 23:23 ` [PATCH v3] " Jian Cai 1 sibling, 0 replies; 10+ messages in thread From: Ard Biesheuvel @ 2021-04-16 21:40 UTC (permalink / raw) To: Jian Cai Cc: # 3.4.x, Greg Kroah-Hartman, Sasha Levin, Nick Desaulniers, Nathan Chancellor, Manoj Gupta, Luis Lozano, clang-built-linux, Catalin Marinas, Will Deacon, Linux ARM, Linux Kernel Mailing List On Fri, 16 Apr 2021 at 22:35, Jian Cai <jiancai@google.com> 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 > > Suggested-by: Ard Biesheuvel <ardb@kernel.org> Please remove this tag - the only thing I suggested was to drop part of the original patch. > Signed-off-by: Jian Cai <jiancai@google.com> > --- > > Changes v1 -> v2: > Keep the comma in the macro definition to be consistent with other > definitions. > > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] arm64: vdso: remove commas between macro name and arguments 2021-04-16 20:35 ` [PATCH v2] " Jian Cai 2021-04-16 21:40 ` Ard Biesheuvel @ 2021-04-16 23:23 ` Jian Cai 2021-05-05 21:07 ` Nathan Chancellor 1 sibling, 1 reply; 10+ messages in thread From: Jian Cai @ 2021-04-16 23:23 UTC (permalink / raw) Cc: stable, gregkh, sashal, ndesaulniers, natechancellor, manojgupta, llozano, clang-built-linux, Jian Cai, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel 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> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments 2021-04-16 23:23 ` [PATCH v3] " Jian Cai @ 2021-05-05 21:07 ` Nathan Chancellor 2021-05-05 21:16 ` Nathan Chancellor 2021-05-06 1:03 ` Jian Cai 0 siblings, 2 replies; 10+ messages in thread From: Nathan Chancellor @ 2021-05-05 21:07 UTC (permalink / raw) To: Jian Cai Cc: stable, gregkh, sashal, ndesaulniers, manojgupta, llozano, clang-built-linux, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments 2021-05-05 21:07 ` Nathan Chancellor @ 2021-05-05 21:16 ` Nathan Chancellor 2021-05-06 1:03 ` Jian Cai 1 sibling, 0 replies; 10+ messages in thread From: Nathan Chancellor @ 2021-05-05 21:16 UTC (permalink / raw) To: Jian Cai Cc: stable, gregkh, sashal, ndesaulniers, manojgupta, llozano, clang-built-linux, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel Fixing Will's email (make sure to run get_maintainer.pl against the latest kernel tree so that .mailmap can do its thing). Original thread: https://lore.kernel.org/r/20210416232341.2421342-1-jiancai@google.com/ On Wed, May 05, 2021 at 02:07:14PM -0700, Nathan Chancellor wrote: > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments 2021-05-05 21:07 ` Nathan Chancellor 2021-05-05 21:16 ` Nathan Chancellor @ 2021-05-06 1:03 ` Jian Cai 2021-05-06 1:30 ` Jian Cai 1 sibling, 1 reply; 10+ messages in thread From: Jian Cai @ 2021-05-06 1:03 UTC (permalink / raw) To: Nathan Chancellor Cc: # 3.4.x, Greg KH, Sasha Levin, Nick Desaulniers, Manoj Gupta, Luis Lozano, clang-built-linux, Catalin Marinas, Will Deacon, Linux ARM, Linux Kernel Mailing List Hi Nathan, Please find my comments inlined. Thanks, Jian On Wed, May 5, 2021 at 2:07 PM Nathan Chancellor <nathan@kernel.org> wrote: > > 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). Ah you are right! I played with IAS a little and it did not have problem parsing commas between the name and its arguments in a macro expansion. However, IAS 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 with the reporter error. This happens to macros with multiple parameters as well. 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] ``` But the the following code fails, ``` $ 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 ``` I will update the commit message accordingly. > 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! Thanks for the information! > > 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments 2021-05-06 1:03 ` Jian Cai @ 2021-05-06 1:30 ` Jian Cai 0 siblings, 0 replies; 10+ messages in thread From: Jian Cai @ 2021-05-06 1:30 UTC (permalink / raw) To: Nathan Chancellor Cc: # 3.4.x, Greg KH, Sasha Levin, Nick Desaulniers, Manoj Gupta, Luis Lozano, clang-built-linux, Catalin Marinas, Will Deacon, Linux ARM, Linux Kernel Mailing List Sorry I sent V4 but forgot to reply to specify message ID, so it is in its own thread right now: https://lore.kernel.org/stable/20210506012508.3822221-1-jiancai@google.com/T/#u. Please let me know if I should also send it to this thread. Jian On Wed, May 5, 2021 at 6:03 PM Jian Cai <jiancai@google.com> wrote: > > Hi Nathan, > > Please find my comments inlined. > > Thanks, > Jian > > On Wed, May 5, 2021 at 2:07 PM Nathan Chancellor <nathan@kernel.org> wrote: > > > > 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). > > Ah you are right! I played with IAS a little and it did not have > problem parsing commas between the name and its arguments in a macro > expansion. However, IAS 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 with the reporter error. This happens to macros with multiple > parameters as well. 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] > ``` > > But the the following code fails, > > ``` > $ 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 > ``` > > I will update the commit message accordingly. > > > > 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! > > Thanks for the information! > > > > > 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-06 1:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-16 18:14 [PATCH] arm64: vdso: remove commas between macro name and arguments Jian Cai 2021-04-16 18:23 ` Jian Cai 2021-04-16 19:08 ` Ard Biesheuvel 2021-04-16 20:35 ` [PATCH v2] " Jian Cai 2021-04-16 21:40 ` Ard Biesheuvel 2021-04-16 23:23 ` [PATCH v3] " Jian Cai 2021-05-05 21:07 ` Nathan Chancellor 2021-05-05 21:16 ` Nathan Chancellor 2021-05-06 1:03 ` Jian Cai 2021-05-06 1:30 ` Jian Cai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).