Re: [PATCH v3] arm64: vdso: remove commas between macro name and arguments

[Jian Cai]

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
> > >