1267551 - Use MOZ_MUST_USE more in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I plan to add a *lot* more uses of MOZ_CHECK (added in bug 1267550 as a synonym
for MOZ_WARN_UNUSED_RESULT) to SpiderMonkey. My plan is to be generous:

- Use it for any function returning bool that isn't obviously a predicate (i.e.
  it's answering a yes/no question and has no outparams).

- Don't limit it just to public interfaces, but use it in private interfaces
  where appropriate.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use MOZ_CHECK more in jsnum.h

This one found a missing check in jsopcode.cpp.

Comment 2

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e791431d1864b4609b43533fe7d4351e192a9e46
Bug 1267551 (part 1) - Use MOZ_MUST_USE more in jsnum.h. r=jonco.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e791431d1864

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use MOZ_MUST_USE more in js/src/ds/

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use MOZ_MUST_USE more in js/src/builtin/

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use MOZ_MUST_USE more in js/src/asmjs/

The patch also removes some unnecessary MOZ_MUST_USE annotations on function
definitions (because the function declaration is already annotated).

And it changes the return type of ModuleGenerator::initFuncSig() to void,
because is always returns true.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use MOZ_MUST_USE more in js/src/ctypes/

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove dead GenerateBlockId declaration

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Use MOZ_MUST_USE more in js/src/ctypes/

This found two missing checks for PackedScopedCoordinate::setSlot(). Fixing one
of them required making ParseContext::updateDecl() fallible.

The patch also changes the return type of addToCallSiteObject() to void.

Comment 11

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8748970 [details] [diff] [review]
(part 3) - Use MOZ_MUST_USE more in js/src/builtin/

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

::: js/src/builtin/TypedObject.h
@@ +157,5 @@
>      type::Kind kind() const {
>          return (type::Kind) getReservedSlot(JS_DESCR_SLOT_KIND).toInt32();
>      }
>  
> +    MOZ_MUST_USE bool opaque() const {

This one, transparent() and hasTraceList() below are predicates - comment 1 says you were not intending to annotate these.

@@ +560,5 @@
>          MOZ_ASSERT(offset <= (size_t) size());
>          return typedMem() + offset;
>      }
>  
> +    inline MOZ_MUST_USE bool opaque() const;

Ditto here.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8749021 [details] [diff] [review]
(part 7) - Use MOZ_MUST_USE more in js/src/ctypes/

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

::: js/src/frontend/BytecodeEmitter.h
@@ +37,5 @@
>      Vector<Value> list;
>    public:
>      explicit CGConstList(ExclusiveContext* cx) : list(cx) {}
> +    MOZ_MUST_USE bool append(Value v) {
> +        MOZ_ASSERT_IF(v.isString(), v.toString()->isAtom()); return list.append(v);

Style nit: Line break after the first `;`, please.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use MOZ_MUST_USE more in js/src/gc/

terrence, there are numerous GC functions that appear to be falliable but
aren't checked at all callsites, and I can't tell if they are dangerous or not.
Can you please take a look? They're all marked with "njn: ?" comments and a
|(void)| cast, which is enough to stop clang from complaining, though not
enough to stop GCC complaining.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8748987 [details] [diff] [review]
(part 4) - Use MOZ_MUST_USE more in js/src/asmjs/

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

Thank you for doing this.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6254d5c31524b7acbad7662c2ec656c3315952
Bug 1267551 (part 2) - Use MOZ_MUST_USE more in js/src/ds/. r=jonco.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eeeb82335526f5a2c6226c5317b9b691f4ed9c
Bug 1267551 (part 3) - Use MOZ_MUST_USE more in js/src/builtin/. r=jonco.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b115b854171f01c1a521e389f2c52c700e9f935
Bug 1267551 (part 4) - Use MOZ_MUST_USE more in js/src/asmjs/. r=bbouvier.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75ceb95a4cb101b3b706780ecbdc825696891dcd
Bug 1267551 (part 5) - Use MOZ_MUST_USE more in js/src/ctypes/. r=sfink.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6a8e6f170fbd1028f0231f7c651644915159a7
Bug 1267551 (part 6) - Remove dead GenerateBlockId declaration. r=jorendorff.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e457582d0089f8efa6b89f112298373aa50e6e5f
Bug 1267551 (part 7) - Use MOZ_MUST_USE more in js/src/frontend/. r=jorendorff.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Use MOZ_MUST_USE more in js/src/jit/

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8750624 [details] [diff] [review]
(part 9) - Use MOZ_MUST_USE more in js/src/jit/

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

Nice work! And you found already some places that weren't testing the return value.
Thanks for fixing the arguments list length within the rules throughout the patch.

::: js/src/jit/IonAnalysis.cpp
@@ +3523,5 @@
>      for (size_t i = 0; i < lhs.numTerms(); i++) {
>          if (lhs.term(i).scale == -1) {
>              rhsDef = lhs.term(i).term;
> +            // njn: ?
> +            (void)lhs.add(rhsDef, 1);

Can you add above
AutoEnterOOMUnsafeRegion oomUnsafe;

and crash here?
if (!lhs.add(rhsDef, 1))
   oomUnsafe.crash("ConvertLinearInequality");

I agree it is not the best solution, but it is better than ignoring it. We should probably try to remove all these crash places. Note: loopunroller is not enabled. So we shouldn't see any crashes here.

::: js/src/jit/IonBuilder.h
@@ +341,5 @@
>      void rewriteParameter(uint32_t slotIdx, MDefinition* param, int32_t argIndex);
>      void rewriteParameters();
> +    MOZ_MUST_USE bool initScopeChain(MDefinition* callee = nullptr);
> +    MOZ_MUST_USE bool initArgumentsObject();
> +    bool pushConstant(const Value& v);  // always returns true

Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making it "easier" not to forget checking the return value.

Comment 18

[Nicholas Nethercote [inactive]]

> > +    bool pushConstant(const Value& v);  // always returns true
> 
> Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making
> it "easier" not to forget checking the return value.

I did that, but there are quite a few (~10 or so) places where the return value isn't checked. Which is fair enough, given that it always returns true, and I didn't want to add a bunch of redundant checks.

I'm not much of a fan of the "always return the same value just so you can use the function in a return statement" approach. It's one of those "clever" approaches that makes code more concise but harder to understand -- you have to look at the definition of pushConstant() to see that it always returns true. I could just change the return type to void...

Comment 19

[Hannes Verschore [:h4writer]]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> > > +    bool pushConstant(const Value& v);  // always returns true
> > 
> > Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making
> > it "easier" not to forget checking the return value.
> 
> I did that, but there are quite a few (~10 or so) places where the return
> value isn't checked. Which is fair enough, given that it always returns
> true, and I didn't want to add a bunch of redundant checks.
> 
> I'm not much of a fan of the "always return the same value just so you can
> use the function in a return statement" approach. It's one of those "clever"
> approaches that makes code more concise but harder to understand -- you have
> to look at the definition of pushConstant() to see that it always returns
> true. I could just change the return type to void...

Just to be clear. The "clever" part was not "to use the function in a return statement", but similar function calls all needed to be "return checked". And this "pushConstant" was misfit. Having it also return checked made sure that it was the default approach to return check all those function (in theory). Like you saw that didn't always happen.

I'm fine to make it void, since we now have asserts that will make sure the functions are return checked.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b6254d5c315
https://hg.mozilla.org/mozilla-central/rev/a0eeeb823355
https://hg.mozilla.org/mozilla-central/rev/5b115b854171
https://hg.mozilla.org/mozilla-central/rev/75ceb95a4cb1
https://hg.mozilla.org/mozilla-central/rev/bd6a8e6f170f
https://hg.mozilla.org/mozilla-central/rev/e457582d0089

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 8750166 [details] [diff] [review]
(part 8) - Use MOZ_MUST_USE more in js/src/gc/

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

Wow, this is great: it sheds light on the really, really ancient and ugly parts of the GC.

::: js/src/gc/Allocator.cpp
@@ +31,5 @@
>      // Invoking the interrupt callback can fail and we can't usefully
>      // handle that here. Just check in case we need to collect instead.
>      if (rt->hasPendingInterrupt())
> +        // njn: ?
> +        (void)gcIfRequested(cx);

The return of gcIfRequested is whether or not we did a major GC. (void) is correct here as we don't care if we GC'd in this particular case, but we do in other cases where we can skip additional need-to-GC checks.

::: js/src/gc/GCRuntime.h
@@ +626,5 @@
>      void triggerFullGCForAtoms() {
>          MOZ_ASSERT(fullGCForAtomsRequested_);
>          fullGCForAtomsRequested_ = false;
> +        // njn: ?
> +        (void)triggerGC(JS::gcreason::ALLOC_TRIGGER);

I believe this should be MOZ_RELEASE_ASSERT(triggerGC(...)). I have no idea what this interface is even for. I'll file a new bug to figure it out.

::: js/src/gc/Heap.h
@@ +1267,5 @@
>          MOZ_ASSERT(tmp == thing);
>      }
>      if (thing->isMarked(GRAY))
> +        // njn: ?
> +        (void)UnmarkGrayCellRecursively(thing, thing->getTraceKind());

UnmarkGray et.al. return whether anything was actually unmarked. The Cycle Collector uses this to UnmarkGray WeakMaps to a fixed point. In this case we don't care: if the reference is stored in a weakmap, the later pass will handle it. I think (void) is the right approach here.

::: js/src/gc/Marking.cpp
@@ +578,5 @@
>      // permanent atoms, so likewise require no subsquent marking.
>      CheckTracedThing(trc, *ConvertToBase(&thing));
>      if (trc->isMarkingTracer())
> +        // njn: ?
> +        (void)thing->markIfUnmarked(gc::BLACK);

|markIfUnmarked| returns true if the thing went from unmarked to marked: e.g. if we need to mark subsequent edges. We know we do not in this case because atoms and symbols do not have children, as the comment immediately above explains. I think (void) on is the right choice here.

@@ +1909,5 @@
>          for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
>              TenuredCell* t = i.getCell();
>              if (always || t->isMarked()) {
> +                // njn: ?
> +                (void)t->markIfUnmarked();

In this case we don't care about the prior mark status because we're do TraceChildren immediately afterwards unconditionally. (void) is fine here too.

::: js/src/gc/Nursery.cpp
@@ +393,5 @@
>           * safe to keep these entries as they may refer to tenured cells which
>           * may be freed after this point.
>           */
> +        // njn: ?
> +        (void)sb.clear();

Clear used to be fallible. It looks like I did not fix the wrapper call when I moved the child buffers to infallible interfaces. The proper solution is to make StoreBuffer::clear return void.

@@ +492,5 @@
>      TIME_END(sweep);
>  
>      TIME_START(clearStoreBuffer);
> +    // njn: ?
> +    (void)rt->gc.storeBuffer.clear();

As above.

::: js/src/gc/StoreBuffer.h
@@ +409,5 @@
>      bool enable();
>      void disable();
>      bool isEnabled() const { return enabled_; }
>  
> +    MOZ_MUST_USE bool clear();

Make this return void. The implementation will be trivial to fix.

::: js/src/gc/Verifier.cpp
@@ +366,5 @@
>  gc::GCRuntime::verifyPreBarriers()
>  {
>      if (verifyPreData)
> +        // njn: ?
> +        (void)endVerifyPreBarriers();

It seems like the return is the same as |bool(verifyPreData)|, so I think we should make this return void, as for endVerifyPostBarriers.

::: js/src/jsgc.cpp
@@ +804,5 @@
>  void Chunk::decommitAllArenas(JSRuntime* rt)
>  {
>      decommittedArenas.clear(true);
> +    // njn: ?
> +    (void)MarkPagesUnused(&arenas[0], ArenasPerChunk * ArenaSize);

The madvise we do here is, as per the name, advisory. If we fail to decommit, it's not a problem from our side, the system just has less available memory. We used to MOZ_RELEASE_ASSERT here, but it turns out that linux can fail the madvise syscall if there is not enough memory available. I think a (void) annotation is probably the right way to go.

@@ +868,5 @@
>      decommittedArenas.unset(offset);
>  
>      Arena* arena = &arenas[offset];
> +    // njn: ?
> +    (void)MarkPagesInUse(arena, ArenaSize);

This is also advisory. The system will fault our pages in (or oom kill the process) as soon as we touch the pages regardless of the status of the call. At least in this case the platform could feasibly decide to use less memory. We don't really have that option though, so we want to just crash when we touch the page next. I think (void) is the right annotation here, although MOZ_RELEASE_ASSERT might also work. I think I'd prefer the (void) annotation -- systems with <4KiB page size could live a little longer that way.

@@ +3305,5 @@
>      if (usedBytes >= thresholdBytes) {
>          // The threshold has been surpassed, immediately trigger a GC,
>          // which will be done non-incrementally.
> +        // njn: ?
> +        (void)triggerZoneGC(zone, JS::gcreason::ALLOC_TRIGGER);

The return of triggerZoneGC tells us whether we actually were able to do the GC. These allocation triggers are advisory and might commonly fail, so I think (void) is the right approach here.

@@ +3319,5 @@
>              // to try to avoid performing non-incremental GCs on zones
>              // which allocate a lot of data, even when incremental slices
>              // can't be triggered via scheduling in the event loop.
> +            // njn: ?
> +            (void)triggerZoneGC(zone, JS::gcreason::ALLOC_TRIGGER);

Same here.

@@ +3357,5 @@
>              fullGCForAtomsRequested_ = true;
>              return false;
>          }
> +        // njn: ?
> +        (void)triggerGC(reason);

And this where we might set the weird fullGCForAtomsRequested flag. We don't care about the return here either.

@@ +4286,5 @@
>  
>      // TODO bug 1167452: Make weak marking incremental
>      SliceBudget budget = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)marker.drainMarkStack(budget);

We can only return false if we are over-budget, which should be impossible with an unlimited budget. This should be MOZ_RELEASE_ASSERT and |budget| should be called |unlimited|.

@@ +4306,5 @@
>              break;
>  
>          auto unlimited = SliceBudget::unlimited();
> +        // njn: ?
> +        (void)marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4334,5 @@
>              (*op)(&marker, grayRootTracer.data);
>      }
>      auto unlimited = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4498,5 @@
>  
>          auto unlimited = SliceBudget::unlimited();
>          gc->incrementalState = MARK;
> +        // njn: ?
> +        (void)gc->marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4960,5 @@
>      }
>  
>      auto unlimited = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)rt->gc.marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

::: js/src/jspropertytree.cpp
@@ +174,5 @@
>              parent->removeChild(existingShape);
>              existingShape = nullptr;
>          } else if (existingShape->isMarked(gc::GRAY)) {
> +            // njn: ?
> +            (void)UnmarkGrayShapeRecursively(existingShape);

As for the other other instance, a (void) annotation is fine.

::: js/src/vm/Interpreter.cpp
@@ +433,5 @@
>  {
>      JSRuntime* runtime;
>      explicit AutoGCIfRequested(JSRuntime* rt) : runtime(rt) {}
> +    // njn: ?
> +    ~AutoGCIfRequested() { (void)runtime->gc.gcIfRequested(); }

As with the other instance, a (void) annotation is fine.

::: js/src/vm/Runtime.cpp
@@ +571,5 @@
>  {
>      MOZ_ASSERT(cx->runtime()->requestDepth >= 1);
>  
> +    // njn: ?
> +    (void)cx->runtime()->gc.gcIfRequested();

The following checks do not care if we GC here, so a (void) annotation is fine.

Comment 22

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47dcb8959eed895257ede693cc7b10d8ebceaf50
Bug 1267551 (part 9) - Use MOZ_MUST_USE more in js/src/jit/. r=h4writer.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47dcb8959eed

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use MOZ_MUST_USE more in js/src/gc/

Thank you for the detailed response. I've addressed all your comments.

For the following functions, I removed the MOZ_MUST_USE annotation and added
comments to the following to make clear that the bool return value indicates
something other than success/failure:
- GCRuntime::gcIfRequested()
- GCRuntime::triggerZoneGC()
- UnmarkGray{Cell,GCThing,Shape,Recursively}()
- ChunkBitmap::markIfUnmarked()
- TenuredCell::markIfUnmarked()

For this one I just removed the MOZ_MUST_USE annotation, because not checking
for failure is reasonable:
- MarkPagesUnused()

I changed these ones to return void:
- StoreBuffer::clear()
- endVerifyPreBarriers, which required tweaking AutoStopVerifyingBarriers'
  constructor
- MarkPagesInUse: all instances always returned true, and the one call site
  didn't check the return value.

I used MOZ_RELEASE_ASSERT on these ones:
- drainMarkStack() (with unlimited budget)
- triggerGC()

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 8752002 [details] [diff] [review]
(part 8) - Use MOZ_MUST_USE more in js/src/gc/

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

Thanks for doing this work! It's going to be a pain to rebase around, but well worth it.

::: js/src/gc/GCRuntime.h
@@ +1054,5 @@
>       */
>      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> numArenasFreeCommitted;
>      VerifyPreTracer* verifyPreData;
> +  public:
> +    bool hasVerifyPreData() const { return !!verifyPreData; }

This should be added just under startVerifyPreBarrier() in the public section above. Actually, it looks like it already exists with a longer name: isVerifyPreBarriersEnabled Please just use that.

::: js/src/gc/StoreBuffer.cpp
@@ +80,5 @@
>      bufferSlot.clear();
>      bufferWholeCell.clear();
>      bufferGeneric.clear();
>  
> +    return;

Please just elide the trailing return.

Comment 26

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/742dca602ca36ee05959b271db99a0f676bba436
Bug 1267551 (part 8) - Use MOZ_MUST_USE more in js/src/gc/. r=terrence.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/742dca602ca3

Comment 28

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Use MOZ_MUST_USE in AutoVectorRooteBase

This patch fixes numerous unchecked calls.

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 8758557 [details] [diff] [review]
(part 10) - Use MOZ_MUST_USE in AutoVectorRooteBase

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

Good find!

Comment 30

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3c2afcc1d78153c4ace5b41e7c238c4d54ee8d
Bug 1267551 (part 10) - Use MOZ_MUST_USE in AutoVectorRooterBase. r=terrence.

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Use MOZ_MUST_USE in js/src/vm/Printer.h

This catches a missing check.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac3c2afcc1d7

Comment 33

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc00bd9f0d0919c7b350862b9b44239a7d5bd837
Bug 1267551 (part 11) - Use MOZ_MUST_USE in js/src/vm/Printer.h. r=h4writer.

Comment 34

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ed5532b9e49a8ada850d80b39ca12f512c5dc
Bug 1267551 (part 11b) - Follow-up to fix Android bustage. r=me.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc00bd9f0d09
https://hg.mozilla.org/mozilla-central/rev/a87ed5532b9e

Comment 36

[Nicholas Nethercote [inactive]]

Attachment: (part 12b) - Use MOZ_MUST_USE even more in js/src/jit/

This patch contains all the new MOZ_MUST_USE annotations, and I just used
|(void)| to fix the places that currently miss checks. The next patch will fix
those up, and I will merge the patches before landing. They're currently
separate to help make reviewing easier.

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: (part 12a) - Use MOZ_MUST_USE even more in js/src/jit/

Sorry, I uploaded the two patches in the wrong order. *This* one adds the
annotations, the other one fixes things up.

Comment 38

[Hannes Verschore [:h4writer]]

Comment on attachment 8761426 [details] [diff] [review]
(part 12b) - Use MOZ_MUST_USE even more in js/src/jit/

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

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +530,5 @@
>          alloc.setNeedSideEffect();
>  
> +    AutoEnterOOMUnsafeRegion oomUnsafe;
> +    if (!snapshots_.add(alloc))
> +       oomUnsafe.crash("CodeGeneratorShared::encodeAllocation");

No need to crash here.
Please use: masm.propagateOOM();

Comment 39

[Hannes Verschore [:h4writer]]

Comment on attachment 8761426 [details] [diff] [review]
(part 12b) - Use MOZ_MUST_USE even more in js/src/jit/

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

::: js/src/jit/RegisterAllocator.cpp
@@ +172,4 @@
>  
>                  while (!worklist.empty()) {
>                      IntegrityItem item = worklist.popCopy();
> +                    if (!checkIntegrity(item.block, *item.block->rbegin(), item.vreg, item.alloc, populateSafepoints))

Pre-existing nit:
can you cut this line into two to fit the 80 column width?

if (!checkIntegrity(item.block, *item.block->rbegin(), item.vreg, item.alloc,
                    populateSafepoints))
{
    return false;
}

Comment 40

[Nicholas Nethercote [inactive]]

> can you cut this line into two to fit the 80 column width?

I will cut it to fit the 100 column width :)

Comment 41

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dac921f07452ec836233b7ccc1f0b194b3d854
Bug 1267551 (part 12) - Use MOZ_MUST_USE even more in js/src/jit/. r=h4writer.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8dac921f074

Comment 43

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/src/vm/SavedFrame.h

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/src/vm/SavedStacks.h

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Stop trying to align method declarations in js/src/vm/SavedStacks.h

It just makes unrelated changes mess up blame when they fix indentation, or it
makes the indentation go bad to preserve blame. Neither is a good outcome...

Comment 46

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/UbiNode.h

Comment 47

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/UbiNodeCensus.h

Comment 48

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/UbiNodeDominatorTree.h

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/UbiNodePostOrder.h

Comment 50

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/UbiNodeShortestPaths.h

Comment 51

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/src/vm/Debugger.h

Comment 52

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/Debug.h

Comment 53

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push for my queue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d8a2e50a77

Comment 54

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

njn: I hope I am not stepping on your toes! I just figured that I could help speed up the transition with the parts of js I am familiar with (and I'm not going to get anything that requires deep thought done while waiting for my plane to board...)

Comment 55

[Nicholas Nethercote [inactive]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #54)
> njn: I hope I am not stepping on your toes! I just figured that I could help
> speed up the transition with the parts of js I am familiar with (and I'm not
> going to get anything that requires deep thought done while waiting for my
> plane to board...)

It's a big enough job that I'm happy to share it :)  Though it is a little odd having two people contributing patches in the same bug. This one probably should be closed -- I've dragged it out long enough -- and start a new one when I next work on this again. Would you mind closing it once you land your patches?

Comment 56

[Jim Blandy :jimb]

Comment on attachment 8762398 [details] [diff] [review]
Use MOZ_MUST_USE in js/public/Debug.h

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

::: js/public/Debug.h
@@ +281,5 @@
>  
>  
>  // For each Debugger that observed a debuggee involved in the given GC event,
>  // call its `onGarbageCollection` hook.
> +JS_PUBLIC_API(MOZ_MUST_USE bool)

You told me this works on all the try targets, but I think it would be better for MOZ_MUST_USE to appear outside the JS_PUBLIC_API call. This just looks really weird.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/991141e6f910
Use MOZ_MUST_USE in js/src/vm/SavedFrame.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb71788bdd54
Use MOZ_MUST_USE in js/src/vm/SavedStacks.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75159d1c1e9
Stop trying to align method declarations in js/src/vm/SavedStacks.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cf94fa7665
Use MOZ_MUST_USE in js/public/UbiNode.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4de4115e38
Use MOZ_MUST_USE in js/public/UbiNodeCensus.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6296c3d3ad
Use MOZ_MUST_USE in js/public/UbiNodeDominatorTree.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2129a04a13
Use MOZ_MUST_USE in js/public/UbiNodePostOrder.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddf8805e824
Use MOZ_MUST_USE in js/public/UbiNodeShortestPaths.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/53826630a9c1
Use MOZ_MUST_USE in js/src/vm/Debugger.h; r=jimb

Comment 58

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use MOZ_MUST_USE in js/public/Debug.h

Ok, fixed the ordering with JS_PUBLIC_API.

Comment 59

[Nicholas Nethercote [inactive]]

fitzgen: BTW, we should put this work on hold while jorendorff is working on bug 1277368, because mozilla::Result will hopefully replace all these bool return values, and mozilla::Result has MOZ_MUST_USE_TYPE on it.

Comment 60

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

SGTM, can't wait for the errormageddon :)

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/991141e6f910
https://hg.mozilla.org/mozilla-central/rev/eb71788bdd54
https://hg.mozilla.org/mozilla-central/rev/e75159d1c1e9
https://hg.mozilla.org/mozilla-central/rev/f6cf94fa7665
https://hg.mozilla.org/mozilla-central/rev/7c4de4115e38
https://hg.mozilla.org/mozilla-central/rev/6b6296c3d3ad
https://hg.mozilla.org/mozilla-central/rev/ee2129a04a13
https://hg.mozilla.org/mozilla-central/rev/7ddf8805e824
https://hg.mozilla.org/mozilla-central/rev/53826630a9c1