546402 - Remove interior pointer workarounds from String code

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P4)

Description

[Edwin Smith]

Bug 541338 has made the GC able to pin objects when interior pointers to them are found on the stack.  This should enable us to remove workarounds from the String code that aimed to preserve the header pointer to string objects.  (grep for volatile in the code)

Comment 1

[Lars T Hansen]

Appears to apply only to 

    Stringp const volatile  m_str;

in StringIndexer in StringObject.h.  Since there are no comments there pointing to the bug that forced the 'volatile' to be inserted, it's tricky to exactly verify that we can remove it, but I'd be happy to offer a patch to do that...

Comment 2

[Dan Smith]

Lars, please submit the patch and close this out.

Comment 3

[Lars T Hansen]

Some searching in Bugzilla has failed to turn up any bug that might be the source of the 'volatile' being added, so I'm just going to remove it and we're going to wait and see if our test suite trips over it on any platform, which is probably what happened last time.

Comment 4

[Tamarin Bot]

changeset: 6561:d72ea908d9f0
user:      Lars T Hansen <[e-mail]>
summary:   Fix 546402 - Remove interior pointer workarounds from String code: strip out a now-redundant 'volatile' (authorization=dansmith)

http://hg.mozilla.org/tamarin-redux/rev/d72ea908d9f0

Comment 5

[Brent Baker]

(In reply to Lars T Hansen from comment #3)
> Some searching in Bugzilla has failed to turn up any bug that might be the
> source of the 'volatile' being added, so I'm just going to remove it and
> we're going to wait and see if our test suite trips over it on any platform,
> which is probably what happened last time.

This is the changeset that introduced the volatile:
http://hg.mozilla.org/tamarin-redux/rev/8b3c7bde9273


changeset:   2418:8b3c7bde9273
user:        Steven Johnson <[e-mail]>
date:        Wed Sep 02 10:53:41 2009 -0700
files:       core/MultinameHashtable.h core/StringObject.h core/Traits.h
description:
StMNHTIterator needs to keep a reference to the MNHT it is using to keep it
from being collected.

StMNHTIterator, StTraitsBindingsIterator, StringIndexer all need their "keeper" fields to be marked volatile; turns out that agressive optimizing compilers can notice that these fields aren't ever used and omit the store. (r=treilly)