Atomics.isLockFree returns true for the size '8'

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This is on Mac OS X 64-bit x86_64.  Found by v8 test suite.

  build-debug$ dist/bin/js
  js> Atomics.isLockFree(8)
  true

The spec says isLockFree() may only return true if its argument is the value of BYTES_PER_ELEMENT for some integer TypedArray.
(Assignee)

Comment 1

2 years ago
Spec: https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.isLockFree

We already have test cases, they need to be adapted.
(Assignee)

Comment 2

2 years ago
Created attachment 8717390 [details] [diff] [review]
isLockFree(8) should be false
Attachment #8717390 - Flags: review?(bbouvier)
Comment on attachment 8717390 [details] [diff] [review]
isLockFree(8) should be false

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

Thanks!

::: js/src/jit-test/tests/atomics/basic-tests.js
@@ +376,5 @@
>  }
>  
> +// isLockFree(n) may return true only if there is an integer array
> +// on which atomic operations is allowed whose byte size is n,
> +// ie, it must return false for n=8.

If this changes in the code, one might oversee the comment and forget this, plus this says what the code is asserting, so code should be law, here.

::: js/src/jit/AtomicOperations.h
@@ +156,5 @@
> +    // Lock-freedom for 8 bytes is determined by the platform's
> +    // isLockfree8().  However, the spec stipulates that isLockFree(8)
> +    // is true only if there is an integer array that admits atomic
> +    // operations whose BYTES_PER_ELEMENT=8; at the moment (February
> +    // 2016) there are no such arrays.

I'd avoid referencing the current date at several places in the code, because there's nothing that prevents us from changing the code below and forget about the comment here. Keeping the original comment sounds fine, but letting it to your judgement.

::: js/src/jit/CodeGenerator.cpp
@@ +9818,5 @@
>      Register value = ToRegister(lir->value());
>      Register output = ToRegister(lir->output());
>  
> +    // Keep this in sync with isLockfree() in jit/AtomicOperations.h.
> +    MOZ_ASSERT(!AtomicOperations::isLockfree(8));

Could it be a static_assert?

@@ +9826,3 @@
>      masm.branch32(Assembler::Equal, value, Imm32(4), &Ldone);
>      masm.branch32(Assembler::Equal, value, Imm32(2), &Ldone);
>      masm.branch32(Assembler::Equal, value, Imm32(1), &Ldone);

Haha, could just test whether n & (n - 1) & 7 is 0, but that might not be faster, and I digress.
Attachment #8717390 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8717390 [details] [diff] [review]
> isLockFree(8) should be false
> 
> Review of attachment 8717390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/jit-test/tests/atomics/basic-tests.js
> @@ +376,5 @@
> >  }
> >  
> > +// isLockFree(n) may return true only if there is an integer array
> > +// on which atomic operations is allowed whose byte size is n,
> > +// ie, it must return false for n=8.
> 
> If this changes in the code, one might oversee the comment and forget this,
> plus this says what the code is asserting, so code should be law, here.
> 
> ::: js/src/jit/AtomicOperations.h
> @@ +156,5 @@
> > +    // Lock-freedom for 8 bytes is determined by the platform's
> > +    // isLockfree8().  However, the spec stipulates that isLockFree(8)
> > +    // is true only if there is an integer array that admits atomic
> > +    // operations whose BYTES_PER_ELEMENT=8; at the moment (February
> > +    // 2016) there are no such arrays.
> 
> I'd avoid referencing the current date at several places in the code,
> because there's nothing that prevents us from changing the code below and
> forget about the comment here. Keeping the original comment sounds fine, but
> letting it to your judgement.

On these points we disagree.  Code expresses mechanism, comments (and tests) express intent, which is something different.  And I don't know how many times I've run across a comment that says "currently, ..." and cursed because there's no indication of when "currently" was.  (Yeah, "hg blame".  But still.)

(That said I'm prone to over-commenting.)

> ::: js/src/jit/CodeGenerator.cpp
> @@ +9818,5 @@
> >      Register value = ToRegister(lir->value());
> >      Register output = ToRegister(lir->output());
> >  
> > +    // Keep this in sync with isLockfree() in jit/AtomicOperations.h.
> > +    MOZ_ASSERT(!AtomicOperations::isLockfree(8));
> 
> Could it be a static_assert?

Maybe with enough contortions, but the win from that is basically zero.

> @@ +9826,3 @@
> >      masm.branch32(Assembler::Equal, value, Imm32(4), &Ldone);
> >      masm.branch32(Assembler::Equal, value, Imm32(2), &Ldone);
> >      masm.branch32(Assembler::Equal, value, Imm32(1), &Ldone);
> 
> Haha, could just test whether n & (n - 1) & 7 is 0, but that might not be
> faster, and I digress.

Plus, the normal case is that the argument to isLockFree is a known constant and constant folding just folds the value of the call to true or false.

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d5f50452275
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.