Closed Bug 1002878 Opened 6 years ago Closed 6 years ago

SIGBUS alignment fault in TypeSet::setBaseObjectCount due to gcc optimizing two 32 bit writes to a 64 bit write

Categories

(Core :: JavaScript Engine, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: dougc, Assigned: jchen)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Using gcc 4.8 from ndk r9d results in a SIGBUS crash in setBaseObjectCount.

The problem appears to be due to two 32 bit memory writes being combined into a 64 bit write instruction.  The 32 bit writes using STR need not be aligned, but the 64 bit write using STM requires word alignment and the 'flags' slot appears not to be word aligned.

Here's the optimize instruction sequence that fails.
	mov	r3, #0
	ldr	r2, [r6]
	bic	r2, r2, #7936
	stm	r6, {r2, r3}   <<< SIGBUS here.  r6 is 0x7c2645d9

It's a combination of the write to the 'flags' slot and the 'objectSet' slot in the functions below:
 
inline void
TypeSet::setBaseObjectCount(uint32_t count)
{
    JS_ASSERT(count <= TYPE_FLAG_OBJECT_COUNT_LIMIT);
    flags = (flags & ~TYPE_FLAG_OBJECT_COUNT_MASK)
          | (count << TYPE_FLAG_OBJECT_COUNT_SHIFT);
}

void
TypeSet::clearObjects()
{
    setBaseObjectCount(0);
    objectSet = nullptr;
}

Looks like a gcc bug.  A workaround is
__attribute__ ((noinline)) void TypeSet::setBaseObjectCount(uint32_t count)
I ran into this bug too. Changing the title to make it easier to find.
Summary: Alignment fault due to gcc optimizing two 32 bit writes to a 64 bit write. → SIGBUS alignment fault in TypeSet::setBaseObjectCount due to gcc optimizing two 32 bit writes to a 64 bit write
The problem is bug 995336 removed the last data member from TypeScript. In C++ an empty class has a size of 1. This fact, coupled with pointer arithmetic in TypeScript that depends on the size of TypeScript [1], meant we're causing the StackTypeSet array to become misaligned. Misalignment is bad on ARM. Depending on the device configuration, you get either a SIGBUS fault or a perf hit.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jsinfer.h?rev=aa534ca9cea5#1248
Blocks: 995336
Keywords: regression
Make sure TypeScript is not empty by adding some padding. Also removed MOZ_NEVER_INLINE from CheckDefinitePropertiesTypeSet because I think it was added due to this bug.
Attachment #8417472 - Flags: review?(bhackett1024)
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment on attachment 8417472 [details] [diff] [review]
Make sure TypeScript is not empty (v1)

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

I think the better fix here is to remove the uses of sizeof(TypeScript) from JSScript::makeTypes and TypeScript::typeArray.  Does that fix this problem?
Attachment #8417472 - Flags: review?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #4)
> Comment on attachment 8417472 [details] [diff] [review]
> Make sure TypeScript is not empty (v1)
> 
> Review of attachment 8417472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the better fix here is to remove the uses of sizeof(TypeScript) from
> JSScript::makeTypes and TypeScript::typeArray.  Does that fix this problem?

It does. That seems like a foot-gun though as it doesn't prevent other code from using sizeof(TypeScript).
(In reply to Jim Chen [:jchen :nchen] from comment #5)
> (In reply to Brian Hackett (:bhackett) from comment #4)
> > Comment on attachment 8417472 [details] [diff] [review]
> > Make sure TypeScript is not empty (v1)
> > 
> > Review of attachment 8417472 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think the better fix here is to remove the uses of sizeof(TypeScript) from
> > JSScript::makeTypes and TypeScript::typeArray.  Does that fix this problem?
> 
> It does. That seems like a foot-gun though as it doesn't prevent other code
> from using sizeof(TypeScript).

Well, we don't want to waste memory here needlessly, there are always lots of ways to write broken code.  TypeScript::typeArray could also assert that the pointer it is returning is correctly aligned.
(In reply to Brian Hackett (:bhackett) from comment #6)
> (In reply to Jim Chen [:jchen :nchen] from comment #5)
> > (In reply to Brian Hackett (:bhackett) from comment #4)
> > > Comment on attachment 8417472 [details] [diff] [review]
> > > Make sure TypeScript is not empty (v1)
> > > 
> > > Review of attachment 8417472 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I think the better fix here is to remove the uses of sizeof(TypeScript) from
> > > JSScript::makeTypes and TypeScript::typeArray.  Does that fix this problem?
> > 
> > It does. That seems like a foot-gun though as it doesn't prevent other code
> > from using sizeof(TypeScript).
> 
> Well, we don't want to waste memory here needlessly, there are always lots
> of ways to write broken code.  TypeScript::typeArray could also assert that
> the pointer it is returning is correctly aligned.

Okay, how about this patch? It doesn't use extra memory and ensures correct behavior in all cases, even if things are re-added to TypeScript later.
Attachment #8417472 - Attachment is obsolete: true
Attachment #8417599 - Flags: review?(bhackett1024)
Comment on attachment 8417599 [details] [diff] [review]
Fix misalignment caused by TypeScript being empty (v2)

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

Thanks!

::: js/src/jsinfer.h
@@ +1267,5 @@
>  {
>      friend class ::JSScript;
>  
> +    // Variable-size array
> +    StackTypeSet mTypeArray[1];

Maybe name this typeArray_ to fit in better with SM naming conventions.
Attachment #8417599 - Flags: review?(bhackett1024) → review+
For checkin. Fix a build bustage on Windows.
Attachment #8417599 - Attachment is obsolete: true
Attachment #8418150 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f60a35a375f6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Looks like beta 31 is crashing on this problem. The change in bug 995336 landed on 31 so can this fix be uplifted to 31 too?
Flags: needinfo?(nchen)
Comment on attachment 8418150 [details] [diff] [review]
Fix misalignment caused by TypeScript being empty (v2.1)

Yup. Sorry I should have realized it's in 31.

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 995336

User impact if declined: Random crash

Testing completed (on m-c, etc.): Locally, m-c, m-a

Risk to taking this patch (and alternatives if risky): Very little; small change and well tested.

String or IDL/UUID changes made by this patch: None
Attachment #8418150 - Flags: approval-mozilla-beta?
Flags: needinfo?(nchen)
Attachment #8418150 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.