Closed Bug 1252075 Opened 4 years ago Closed 4 years ago

use UniquePtr instead of ScopedDeletePtr in testGCHeapPostBarriers

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
UniquePtr is more standard than ScopedDeletePtr; using standard
constructs whenever possible is preferable.
Attachment #8724721 - Flags: review?(terrence)
Comment on attachment 8724721 [details] [diff] [review]
use UniquePtr instead of ScopedDeletePtr in testGCHeapPostBarriers

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

Thanks!
Attachment #8724721 - Flags: review?(terrence) → review+
Heh, sorry, this is a result of the analysis being too smart. It is interfering with a test that is trying to mock up a real situation, but the analysis sees through it and can tell that not everything is being done correctly.

Specifically: the analysis knows that UniquePtr points to a chunk of heap memory that is much like a stack, in that it is memory that can hang onto a pointer across a GC and isn't part of another data structure that knows how to trace pointers contained in it. So it yells at you if you store movable GC pointers into it and then do something that could GC.

I forgot about ScopedDeletePtr when I implemented that, so it isn't treated the same way.

I'm really not sure what the best way is to go about this. 'typedef' and 'using' won't work; the analysis strips those. I *think* subclassing would work? But is UniquePtr subclassable? That's kinda weird.

Using new/malloc directly would work, but obviously you'd lose the automatic cleanup then.

Optimally, you could just mark that variable as __attribute__((tag("non-gc pointer"))). I could maybe make that work, but it would take some time (I need to land some prerequisite stuff first.)
(In reply to Steve Fink [:sfink, :s:] from comment #5)
> I'm really not sure what the best way is to go about this. 'typedef' and
> 'using' won't work; the analysis strips those. I *think* subclassing would
> work? But is UniquePtr subclassable? That's kinda weird.

UniquePtr is indeed subclassable:

http://mxr.mozilla.org/mozilla-central/source/dom/media/systemservices/MediaUtils.h#354

Well, OK, that shows ScopedDeletePtr, but bug 1251715 replaced that with UniquePtr and it works OK.

> Using new/malloc directly would work, but obviously you'd lose the automatic
> cleanup then.

That does not seem so bad for a one-off case, though I'm a little surprised the analysis wouldn't yell about that.

> Optimally, you could just mark that variable as __attribute__((tag("non-gc
> pointer"))). I could maybe make that work, but it would take some time (I
> need to land some prerequisite stuff first.)

I am inclined to do simple things to get this to land. :)
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Steve Fink [:sfink, :s:] from comment #5)
> > I'm really not sure what the best way is to go about this. 'typedef' and
> > 'using' won't work; the analysis strips those. I *think* subclassing would
> > work? But is UniquePtr subclassable? That's kinda weird.
> 
> UniquePtr is indeed subclassable:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/media/systemservices/
> MediaUtils.h#354
> 
> Well, OK, that shows ScopedDeletePtr, but bug 1251715 replaced that with
> UniquePtr and it works OK.
> 
> > Using new/malloc directly would work, but obviously you'd lose the automatic
> > cleanup then.
> 
> That does not seem so bad for a one-off case, though I'm a little surprised
> the analysis wouldn't yell about that.

The analysis uses types, and knows nothing about data flow. malloc returns a void*. If you were to memcpy a GC pointer into a malloced buffer, the analysis would lose track of it.

It doesn't work in this case to grab out the pointer from the UniquePtr, because the UniquePtr itself is used in its destructor, so its live range extends all the way from its construction to its destruction, and any GC call within there will complain.

> 
> > Optimally, you could just mark that variable as __attribute__((tag("non-gc
> > pointer"))). I could maybe make that work, but it would take some time (I
> > need to land some prerequisite stuff first.)
> 
> I am inclined to do simple things to get this to land. :)

Agreed.
https://hg.mozilla.org/mozilla-central/rev/65a12991543f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Not actually fixed. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Steve Fink [:sfink, :s:] from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > Using new/malloc directly would work, but obviously you'd lose the automatic
> > > cleanup then.
> > 
> > That does not seem so bad for a one-off case, though I'm a little surprised
> > the analysis wouldn't yell about that.
> 
> The analysis uses types, and knows nothing about data flow. malloc returns a
> void*. If you were to memcpy a GC pointer into a malloced buffer, the
> analysis would lose track of it.
> 
> It doesn't work in this case to grab out the pointer from the UniquePtr,
> because the UniquePtr itself is used in its destructor, so its live range
> extends all the way from its construction to its destruction, and any GC
> call within there will complain.

Hm.  Does something like:

  auto x = MakeUnique<char[]>(...);
  new (x.get()) JS::Heap<T>();
  JS::Heap<T>* y = x.get();

slide past the analysis?  Or does explicitly having the raw pointer there alert the analysis to start tracking something?

It looks like any kind of smart pointer is going to be subject to the hazard analysis's smarts, so maybe malloc + leaking the pointer in case of failure is the way to go?  Unless:

  void* x = malloc(sizeof(JS::Heap<T>));
  new (x) JS::Heap<T>();
  JS::Heap<T>* y = x;

is subject to the same problems with the analysis...

ni? to Terrence for being OK with leaking a pointer on failure paths, sfink for expert analysis on how to evade the hazard analysis.
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Yup, we're fine with leaking in failure paths in jsapi-tests. We certainly use CHECK(did not leak) all of the place, so I expect that the opposite will be fine too.
Flags: needinfo?(terrence)
(In reply to Nathan Froyd [:froydnj] from comment #11)
> (In reply to Steve Fink [:sfink, :s:] from comment #7)
> > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > > Using new/malloc directly would work, but obviously you'd lose the automatic
> > > > cleanup then.
> > > 
> > > That does not seem so bad for a one-off case, though I'm a little surprised
> > > the analysis wouldn't yell about that.
> > 
> > The analysis uses types, and knows nothing about data flow. malloc returns a
> > void*. If you were to memcpy a GC pointer into a malloced buffer, the
> > analysis would lose track of it.
> > 
> > It doesn't work in this case to grab out the pointer from the UniquePtr,
> > because the UniquePtr itself is used in its destructor, so its live range
> > extends all the way from its construction to its destruction, and any GC
> > call within there will complain.
> 
> Hm.  Does something like:
> 
>   auto x = MakeUnique<char[]>(...);
>   new (x.get()) JS::Heap<T>();
>   JS::Heap<T>* y = x.get();
> 
> slide past the analysis?  Or does explicitly having the raw pointer there
> alert the analysis to start tracking something?
> 
> It looks like any kind of smart pointer is going to be subject to the hazard
> analysis's smarts, so maybe malloc + leaking the pointer in case of failure
> is the way to go?  Unless:
> 
>   void* x = malloc(sizeof(JS::Heap<T>));
>   new (x) JS::Heap<T>();
>   JS::Heap<T>* y = x;
> 
> is subject to the same problems with the analysis...

Both of those should evade the analysis. The analysis is not smart, and particular does next to no data flow analysis, so both those cases are fine. (Heap<JSObject*> is a JSObject**, which GC would not directly invalidate, so it's fine. And the blacklisting of UniquePtr storage doesn't matter here since you're not storing any GC-controlled stuff in it.)
Flags: needinfo?(sphink)
A little finagling later, and we have something that makes the hazard analysis
content:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e6bc71d407
Attachment #8726267 - Flags: review?(terrence)
Attachment #8724721 - Attachment is obsolete: true
Comment on attachment 8726267 [details] [diff] [review]
use UniquePtr instead of ScopedDeletePtr in testGCHeapPostBarriers

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

Looks good. Sorry for the grief getting this in!
Attachment #8726267 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/e49424ca0ddf
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.