Last Comment Bug 676577 - IPFX is confusing, causing a crash
: IPFX is confusing, causing a crash
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Marty Rosenberg [:mjrosenb]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-04 10:15 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2011-08-11 04:28 PDT (History)
2 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix_spew.diff (1.15 KB, patch)
2011-08-04 10:15 PDT, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review
perma_fix_spew.diff (650 bytes, patch)
2011-08-04 10:19 PDT, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review
perma_fix_spew-r2.diff (680 bytes, patch)
2011-08-04 16:52 PDT, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-08-04 10:15:00 PDT
Created attachment 550735 [details] [diff] [review]
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] 2011-08-04 10:19:19 PDT
Created attachment 550737 [details] [diff] [review]
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] 2011-08-04 14:10:37 PDT
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] 2011-08-04 14:34:13 PDT
(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] 2011-08-04 16:52:35 PDT
Created attachment 550894 [details] [diff] [review]
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] 2011-08-04 17:01:42 PDT
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))

Note You need to log in before you can comment on or make changes to this bug.