ARM64: Fix and improve MacroAssembler::Printf.

ARM64: Fix and improve MacroAssembler::Printf.

  - W-sized values passed to Printf are now handled correctly by the
    simulator. In AAPCS64, int32_t and int64_t are passed in the same
    way, so this didn't affect non-simulator builds.
  - Since Printf now records the type and size of each argument, it is
    possible to mix argument types.
  - It is now possible to print the stack pointer. There is only one
    remaining restriction: The `csp` register cannot be printed unless
    it is the current stack pointer. This is because it is modified by
    BumpSystemStackPointer when the caller-saved registers are
    preserved.

BUG=
R=rmcilroy@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21272

[jbramley, 2014-05-07 16:20:34]

This ports improvements and fixes to Printf that were made in VIXL 1.4.

[rmcilroy, 2014-05-08 09:32:13]

Overall looks good.  One main comment in simulator-arm64.cc, but other than that
just nits.

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.cc:4871: CPURegister pcs[kPrintfMaxArgCount] =
{NoReg, NoReg, NoReg, NoReg};
I'm guessing pcs stands for procedure call standard? Maybe it would be clearer
as in_args and out_args? If not, please expand PCS in the comment above.

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.cc:4899: // Copies of the arg lists that we can
iterate through.
Nit - "Copies of the printf  vararg registers that we can pop from."

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3625: // format placeholder at a time.
It's sad you need to go to all this bother since you can't just portably create
a varargs list in C :(.

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3651: if (placeholder_count > 0) *format_scratch++
= format_base[i];
Doesn't this mean you can overrun the size of the format buffer?  As far as I
can tell you are compensating for the the extra '\0' by having format be length
+ arg_count, but not for the escaping of %%'s.  

Also should you be double escaping %'s anyway?  Surely if code calls  
 __ PrintF("%%\n")
the expected output is just "%" just like it would be with normal printf, right?
 Surely this double escaping would lead to the output of "%%"?

[jbramley, 2014-05-08 14:48:01]

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.cc:4871: CPURegister pcs[kPrintfMaxArgCount] =
{NoReg, NoReg, NoReg, NoReg};
On 2014/05/08 09:32:13, rmcilroy wrote:
> I'm guessing pcs stands for procedure call standard? Maybe it would be clearer
> as in_args and out_args? If not, please expand PCS in the comment above.

I'll expand PCS in the comment.

https://codereview.chromium.org/268353005/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.cc:4899: // Copies of the arg lists that we can
iterate through.
On 2014/05/08 09:32:13, rmcilroy wrote:
> Nit - "Copies of the printf  vararg registers that we can pop from."

Done.

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3625: // format placeholder at a time.
On 2014/05/08 09:32:13, rmcilroy wrote:
> It's sad you need to go to all this bother since you can't just portably
create
> a varargs list in C :(.

Yes, it is! I tried a few approaches and this was the only one that was even
slightly portable. It's a pain, but hopefully worth it.

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3651: if (placeholder_count > 0) *format_scratch++
= format_base[i];
On 2014/05/08 09:32:13, rmcilroy wrote:
> Doesn't this mean you can overrun the size of the format buffer?  As far as I
> can tell you are compensating for the the extra '\0' by having format be
length
> + arg_count, but not for the escaping of %%'s.  
> 
> Also should you be double escaping %'s anyway?  Surely if code calls  
>  __ PrintF("%%\n")
> the expected output is just "%" just like it would be with normal printf,
right?
>  Surely this double escaping would lead to the output of "%%"?

  - For the first chunk (which contains no format placeholders and is printed
with `printf("%s", format)`), "%%" is converted to "%".
  - For subsequent chunks (which each contain exactly one placeholder), "%%" is
converted to "%%" (and is not counted as a placeholder). These chunks are
printed with `printf(chunk, arg)`.

It's a bit ugly that the first chunk is printed differently, but this avoids a
GCC warning about code like `printf(string)`: it doesn't like the non-literal
format string without an arguments list.

There is no double-escaping: `Printf("%%")` prints "%". (Note the `i++` on the
previous line.)

If you like, I can add an assertion after the loop:
ASSERT(format_scratch <= (format + length + arg_count));

(That assertion doesn't fail in the assembler tests, where Printf is tested.)

[rmcilroy, 2014-05-08 15:38:31]

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3651: if (placeholder_count > 0) *format_scratch++
= format_base[i];
On 2014/05/08 14:48:01, jbramley wrote:
> On 2014/05/08 09:32:13, rmcilroy wrote:
> > Doesn't this mean you can overrun the size of the format buffer?  As far as
I
> > can tell you are compensating for the the extra '\0' by having format be
> length
> > + arg_count, but not for the escaping of %%'s.  
> > 
> > Also should you be double escaping %'s anyway?  Surely if code calls  
> >  __ PrintF("%%\n")
> > the expected output is just "%" just like it would be with normal printf,
> right?
> >  Surely this double escaping would lead to the output of "%%"?
> 
>   - For the first chunk (which contains no format placeholders and is printed
> with `printf("%s", format)`), "%%" is converted to "%".
>   - For subsequent chunks (which each contain exactly one placeholder), "%%"
is
> converted to "%%" (and is not counted as a placeholder). These chunks are
> printed with `printf(chunk, arg)`.
> 
> It's a bit ugly that the first chunk is printed differently, but this avoids a
> GCC warning about code like `printf(string)`: it doesn't like the non-literal
> format string without an arguments list.
> 
> There is no double-escaping: `Printf("%%")` prints "%". (Note the `i++` on the
> previous line.)
> 
> If you like, I can add an assertion after the loop:
> ASSERT(format_scratch <= (format + length + arg_count));
> 
> (That assertion doesn't fail in the assembler tests, where Printf is tested.)

Ahh I see - I missed the 'i++'.  I think this is confusing though.  I would
prefer to avoid the special casing of the first chunk if you can manage too
somehow.  Otherwise, it might be clearer if you did it the other way round,
i.e.:

 if (format_base[i + 1] == '%') {
   // Ignore explicit "%%" sequences.
   *format_scratch++ = format_base[i];

   if (placeholder_count > 0) {
     // The first chunk is passed as an %s argument to printf, so we
     // need to unescape '%' characters in this chunk. Just skip the next '%'
     i++;
   } else {
     // Otherwise pass through the '%%' as-is.
     *format_scratch++ = format_base[++i];
   }
 }

[jbramley, 2014-05-12 10:11:28]

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
src/arm64/simulator-arm64.cc:3651: if (placeholder_count > 0) *format_scratch++
= format_base[i];
On 2014/05/08 15:38:32, rmcilroy wrote:
> On 2014/05/08 14:48:01, jbramley wrote:
> > On 2014/05/08 09:32:13, rmcilroy wrote:
> > > Doesn't this mean you can overrun the size of the format buffer?  As far
as
> I
> > > can tell you are compensating for the the extra '\0' by having format be
> > length
> > > + arg_count, but not for the escaping of %%'s.  
> > > 
> > > Also should you be double escaping %'s anyway?  Surely if code calls  
> > >  __ PrintF("%%\n")
> > > the expected output is just "%" just like it would be with normal printf,
> > right?
> > >  Surely this double escaping would lead to the output of "%%"?
> > 
> >   - For the first chunk (which contains no format placeholders and is
printed
> > with `printf("%s", format)`), "%%" is converted to "%".
> >   - For subsequent chunks (which each contain exactly one placeholder), "%%"
> is
> > converted to "%%" (and is not counted as a placeholder). These chunks are
> > printed with `printf(chunk, arg)`.
> > 
> > It's a bit ugly that the first chunk is printed differently, but this avoids
a
> > GCC warning about code like `printf(string)`: it doesn't like the
non-literal
> > format string without an arguments list.
> > 
> > There is no double-escaping: `Printf("%%")` prints "%". (Note the `i++` on
the
> > previous line.)
> > 
> > If you like, I can add an assertion after the loop:
> > ASSERT(format_scratch <= (format + length + arg_count));
> > 
> > (That assertion doesn't fail in the assembler tests, where Printf is
tested.)
> 
> Ahh I see - I missed the 'i++'.  I think this is confusing though.  I would
> prefer to avoid the special casing of the first chunk if you can manage too
> somehow.  Otherwise, it might be clearer if you did it the other way round,
> i.e.:
> 
>  if (format_base[i + 1] == '%') {
>    // Ignore explicit "%%" sequences.
>    *format_scratch++ = format_base[i];
> 
>    if (placeholder_count > 0) {
>      // The first chunk is passed as an %s argument to printf, so we
>      // need to unescape '%' characters in this chunk. Just skip the next '%'
>      i++;
>    } else {
>      // Otherwise pass through the '%%' as-is.
>      *format_scratch++ = format_base[++i];
>    }
>  }

Done.

[rmcilroy, 2014-05-12 10:23:43]

On 2014/05/12 10:11:28, jbramley wrote:
> https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc
> File src/arm64/simulator-arm64.cc (right):
> 
>
https://codereview.chromium.org/268353005/diff/1/src/arm64/simulator-arm64.cc...
> src/arm64/simulator-arm64.cc:3651: if (placeholder_count > 0)
*format_scratch++
> = format_base[i];
> On 2014/05/08 15:38:32, rmcilroy wrote:
> > On 2014/05/08 14:48:01, jbramley wrote:
> > > On 2014/05/08 09:32:13, rmcilroy wrote:
> > > > Doesn't this mean you can overrun the size of the format buffer?  As far
> as
> > I
> > > > can tell you are compensating for the the extra '\0' by having format be
> > > length
> > > > + arg_count, but not for the escaping of %%'s.  
> > > > 
> > > > Also should you be double escaping %'s anyway?  Surely if code calls  
> > > >  __ PrintF("%%\n")
> > > > the expected output is just "%" just like it would be with normal
printf,
> > > right?
> > > >  Surely this double escaping would lead to the output of "%%"?
> > > 
> > >   - For the first chunk (which contains no format placeholders and is
> printed
> > > with `printf("%s", format)`), "%%" is converted to "%".
> > >   - For subsequent chunks (which each contain exactly one placeholder),
"%%"
> > is
> > > converted to "%%" (and is not counted as a placeholder). These chunks are
> > > printed with `printf(chunk, arg)`.
> > > 
> > > It's a bit ugly that the first chunk is printed differently, but this
avoids
> a
> > > GCC warning about code like `printf(string)`: it doesn't like the
> non-literal
> > > format string without an arguments list.
> > > 
> > > There is no double-escaping: `Printf("%%")` prints "%". (Note the `i++` on
> the
> > > previous line.)
> > > 
> > > If you like, I can add an assertion after the loop:
> > > ASSERT(format_scratch <= (format + length + arg_count));
> > > 
> > > (That assertion doesn't fail in the assembler tests, where Printf is
> tested.)
> > 
> > Ahh I see - I missed the 'i++'.  I think this is confusing though.  I would
> > prefer to avoid the special casing of the first chunk if you can manage too
> > somehow.  Otherwise, it might be clearer if you did it the other way round,
> > i.e.:
> > 
> >  if (format_base[i + 1] == '%') {
> >    // Ignore explicit "%%" sequences.
> >    *format_scratch++ = format_base[i];
> > 
> >    if (placeholder_count > 0) {
> >      // The first chunk is passed as an %s argument to printf, so we
> >      // need to unescape '%' characters in this chunk. Just skip the next
'%'
> >      i++;
> >    } else {
> >      // Otherwise pass through the '%%' as-is.
> >      *format_scratch++ = format_base[++i];
> >    }
> >  }
> 
> Done.

Thanks Jacob.  LGTM

[jbramley, 2014-05-12 15:44:33]

Committed patchset #3 manually as r21272 (presubmit successful).