IPFX is confusing, causing a crash

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Assigned: mjrosenb)

Tracking

Trunk
mozilla8
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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
Assignee: general → mrosenberg
(Assignee)

Updated

6 years ago
Attachment #550735 - Flags: review?(sstangl)
(Assignee)

Updated

6 years ago
Attachment #550737 - Flags: review?(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".
Attachment #550735 - Flags: review?(sstangl) → review+

Updated

6 years ago
Attachment #550737 - Flags: review?(sstangl) → review+
(Assignee)

Comment 3

6 years ago
(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
(Assignee)

Comment 4

6 years ago
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__ ...
Attachment #550737 - Attachment is obsolete: true
Attachment #550894 - Flags: review?(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))
Attachment #550894 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/66d6bdbdd200
http://hg.mozilla.org/integration/mozilla-inbound/rev/8eb72ae5ac5e
Flags: in-testsuite?
Merged:
http://hg.mozilla.org/mozilla-central/rev/66d6bdbdd200
http://hg.mozilla.org/mozilla-central/rev/8eb72ae5ac5e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Version: unspecified → Trunk
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.