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@xxxxxxxxxx/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@xxxxxxxxxx> wrote: > > Hi Nathan, > > Please find my comments inlined. > > Thanks, > Jian > > On Wed, May 5, 2021 at 2:07 PM Nathan Chancellor <nathan@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > 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 > > >
