676577 - IPFX is confusing, causing a crash

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: fix_spew.diff

in ARMAssembler.h#550 
        void fnegd_r(int dd, int dm, Condition cc = AL)
        {
            js::JaegerSpew(js::JSpew_Insns,
                    IPFX   "%-15s %s, %s, %s, %s\n", MAYBE_PAD, "fnegd", nameFpRegD(dd), nameFpRegD(dm));

this is causing a segv in strlen, since the last *two* %s's do not have matching arguments.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: perma_fix_spew.diff

This patch turns on gnu printf-style argument checking on our spew function, since IPFX seems to be confusing enough that we have gotten it wrong in several places (ok, just one, and I got it wrong a whole bunch when I was adding instructions).

This patch will cause gcc to throw a whole lot of warnings, all of which are trivial to fix with casts, most of which probably are not actual bugs.

I didn't put it under and #ifdef's since I assume that we have __attribute__() properly defined so it does not confuse non-gnu compilers

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 550735 [details] [diff] [review]
fix_spew.diff

Review of attachment 550735 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/assembler/assembler/ARMAssembler.h
@@ +543,5 @@
>  
>          void fnegd_r(int dd, int dm, Condition cc = AL)
>          {
>              js::JaegerSpew(js::JSpew_Insns,
> +                    IPFX   "%-15s %s, %s\n", MAYBE_PAD, "fnegd", nameFpRegD(dd), nameFpRegD(dm));

This has one more argument than %s.
r+ with additional ", %s".

Comment 3

[Marty Rosenberg [:mjrosenb]]

(In reply to comment #2)
> Comment on attachment 550735 [details] [diff] [review] [diff] [details] [review]
> fix_spew.diff
> 
> Review of attachment 550735 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/assembler/assembler/ARMAssembler.h
> @@ +543,5 @@
> >  
> >          void fnegd_r(int dd, int dm, Condition cc = AL)
> >          {
> >              js::JaegerSpew(js::JSpew_Insns,
> > +                    IPFX   "%-15s %s, %s\n", MAYBE_PAD, "fnegd", nameFpRegD(dd), nameFpRegD(dm));
> 
> This has one more argument than %s.
> r+ with additional ", %s".

1) more arguments than placeholders is not going to cause a crash
2) "IPFX is confusing"-- I meant it.
js/src/assembler/assembler/ARMAssembler.h
45:#define IPFX    "        %s"

there are 4 %s's, and 4 arguments

Comment 4

[Marty Rosenberg [:mjrosenb]]

Attachment: perma_fix_spew-r2.diff

guess we don't automatically do the right thing with __attribute__ on non-gnu platforms.  Added in #ifdef __GNUC__ ...

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 550894 [details] [diff] [review]
perma_fix_spew-r2.diff

Review of attachment 550894 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/methodjit/Logging.h
@@ +74,5 @@
>  
>  void JMCheckLogging();
>  
>  bool IsJaegerSpewChannelActive(JaegerSpewChannel channel);
> +#ifdef __GNUC__

Apparently __attribute__((format)) is valid in GCC 3 and GCC 4, so:
&& (__GNUC__ >= 3))

Comment 6

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/66d6bdbdd200

Comment 7

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8eb72ae5ac5e

Comment 8

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/66d6bdbdd200
http://hg.mozilla.org/mozilla-central/rev/8eb72ae5ac5e