1154858 - A test case where Atomics.add does not perform an addition.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jukka Jylänki]

Attachment: atomics_add_test.tar.gz

Attached is an odd test case where calling Atomics.add() on a 32-bit location in the SAB does not perform the increment, but seems to be a no-op.

Unzip and open iospt.html in the browser.

Observed: The code prints out lines like

__shared_count::__add_shared(): Value at heap address 13812 before increase is 0. Incrementing this by one...
__shared_count::__add_shared(): After atomic increment, value at heap address 13812 is now : 0, expected it to be 1

and eventually fails execution to an exception being thrown.

Interestingly, disabling asm.js by setting the pref javascript.options.asmjs;false , the test case starts to work and rerunning the page gives the expected output:

__shared_count::__add_shared(): Value at heap address 13812 before increase is 0. Incrementing this by one...
__shared_count::__add_shared(): After atomic increment, value at heap address 13812 is now : 1, expected it to be 1

...

and the page does not throw an exception, but prints as the last message:

"The thread should print 'Hello from thread'"

The interesting bit occurs in the following function:

function __ZNSt3__114__shared_count12__add_sharedEv(i1) {
 i1 = i1 | 0;
 var i2 = 0;
 i2 = i1 + 4 | 0;
 i1 = HEAP32[i2 >> 2] | 0;
 _emscripten_asm_const_int(7, i2 | 0, i1 | 0) | 0;
 Atomics_add(HEAP32, i2 >> 2, 1);
 _emscripten_asm_const_int(8, i2 | 0, HEAP32[i2 >> 2] | 0, i1 + 1 | 0) | 0;
 return;
}

Where it looks like Atomics_add() performs a no-op, even though it should correctly map to Atomics.add() as far as I can track.

Lars, can you try the test case to see if you can reproduce the issue?

Comment 1

[Lars T Hansen [:lth]]

Is the platform truly x86 (ie, 32-bit) or is it really x86-64?  I ask because the code generation is largely platform-specific for this stuff.

Comment 2

[Lars T Hansen [:lth]]

I can reproduce this in the stock Nightly build for 64-bit MacOS X from 2015-04-15.  I'll investigate further.

Comment 3

[Lars T Hansen [:lth]]

This is a bug that is somehow connected with executing an atomic add for effect (ie without using its return value), I can reproduce it in a smaller test case.  I implemented code for this optimization recently (within the last month) and must have made a mistake.

If you need to work around this temporarily, capture the return value from Atomics.add and make sure the captured value is used, perhaps by storing it in a heap slot would work.

Comment 4

[Lars T Hansen [:lth]]

The underlying cause is that the patch that landed from bug 1147916 to support the for-effect operations is wrong: the new operations that take scale+index parameters ignore those parameters, hence the object code is wrong.  (One cannot see this problem from the code generator dump since the spew calls for the incorrect primitives are actually correct.  Consider this a vote for a proper disassembler.)

Comment 5

[Lars T Hansen [:lth]]

Attachment: Bug fix and test case

Comment 6

[Jukka Jylänki]

Applied the patch locally and verified it to work. Absolutely stellar and fast work Lars!

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8593298 [details] [diff] [review]
Bug fix and test case

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

Looks good.

Comment 8

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c9bd26d5ef6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c9bd26d5ef6