969436 - IonMonkey: SafepointReader might miss interpret register masks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently, on x86, we define

  typedef uint8_t PackedRegisterMask;

because only 8 register can be used at max, and we do not need variable length encoding nor decoding when we have only one byte to mask all registers available.

This cause the SafepointWriter to write safepoint using only one bytes(*) (in WriteRegisterMask), but to read them from the SafepointReader constructor using a compact unsigned word as a variable length unsigned.

This means that if "edi" is allocated(*), then all our safepoints are miss interpreted.

We should modify the way we read the safepoints, based on the size of PackedRegisterMask.

(*) Note, we also need to fix WriteRegisterMask to check for " == 1" instead of " == 8", to make this optimization doable on x86, and thus save one byte per safepoint everytime edi is allocated.  If we did not had this extra bug this would have been a sec-critical issue.

Comment 1

[Manish Goregaokar [:manishearth]]

Assigning to benny as per request in IRC.

Nicolas, can you help him get started on this?

Comment 2

[Ben Hackett]

Hi Nicolas. Is it okay that I work on this even though my computer is 64 bit? If it is okay, then would you be able to help me get started? This is my first contribution. 
Thanks.

Comment 3

[Nicolas B. Pierron [:nbp]]

Sorry for the late reply.

(In reply to Ben Hackett from comment #2)
> Hi Nicolas. Is it okay that I work on this even though my computer is 64
> bit? If it is okay, then would you be able to help me get started? This is
> my first contribution. 
> Thanks.

It would be best if you could test it by cross-compiling the shell to x86.  In which case you should be able to test the modifications that you are making.  You can follow what is documented on [1] to cross-compile the JS shell.

[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation#Specifying_compilers_and_compiler_flags

Comment 4

[Ben Hackett]

No worries. Okay, awesome, thanks. 

Hopefully its okay if I just work on this on my weekends - starting to realize that during the week I don't have too much spare time.

Comment 5

[Ben Hackett]

Sorry I got off to a slow start. Okay, so:
I don't understand the reasoning or criteria for deciding what method to use to write bytes (using CompactBufferWriter). I would have thought that it would be something like this pseudo-code:
if (sizeof(PackedRegisterMask) <= 8) then call writeByte
else if (sizeof(PackedRegisterMask) <= 16) then call writeFixedUint16_t
else if (sizeof(PackedRegisterMask) <= 32) then call writeFixedUint32_t
else call writeUnsigned
...Or something like that.. I guess I just don't understand the connection between when exactly you need variable length or fixed length and what the size of PackedRegisterMask is.

Also, I should be using the same criteria for both the reading and the writing, correct?

Thanks a lot!

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #5)
> I don't understand the reasoning or criteria for deciding what method to use
> to write bytes (using CompactBufferWriter). I would have thought that it
> would be something like this pseudo-code:
> if (sizeof(PackedRegisterMask) <= 8) then call writeByte
> else if (sizeof(PackedRegisterMask) <= 16) then call writeFixedUint16_t
> else if (sizeof(PackedRegisterMask) <= 32) then call writeFixedUint32_t
> else call writeUnsigned

The goal is to minimize the space use to encode the allocated registers mask.
writeByte will write 8 bits (1 byte), writeUnsigned will write up to 5 bytes cluster of 7 bits (with one bit reserved to extend the number on another byte).  In practice, if we do writeUnisgned would be more optimized after 1 byte.

So the current switch is correct, but the condition is wrong, because "sizeof" returns the size in bytes, and not bits.

> ...Or something like that.. I guess I just don't understand the connection
> between when exactly you need variable length or fixed length and what the
> size of PackedRegisterMask is.

We need fixed length to avoid writing 2 bytes, when we allocate the register number 7.  as writeUnisgned cannot encode it on one byte.

> Also, I should be using the same criteria for both the reading and the
> writing, correct?

Correct.

Comment 7

[Ben Hackett]

Ah okay. Makes sense now.

So in terms of testing, how do I go about that? Any specific framework I should use? Should I be testing for performance gains? Do I need to write new tests?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #7)
> So in terms of testing, how do I go about that? Any specific framework I
> should use? Should I be testing for performance gains? Do I need to write
> new tests?

Cross-compile for x86, and if the test suite runs fine, then all is probably ok.  If you have any doubt that the function is used add a MOZ_ASSERT(false) to see if this is well triggered in the test suite.

Note that we encode most of the time, but we only decode it a few times.

Comment 9

[Ben Hackett]

I think I have fixed the bug, but I can't get the test suite to run. When I type "../tests/jstests.py ./js" I get a whole bunch of stuff, near the end is: 

File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied

I'm actually not even cross compiling for 32 bit yet, and this is with the old unmodified working code, I'm just trying to get the test suite running.

By the way, are there any times you are regularly on IRC? Just curious.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #9)
> I think I have fixed the bug, but I can't get the test suite to run. When I
> type "../tests/jstests.py ./js" I get a whole bunch of stuff, near the end
> is: 
> 
> File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
>     raise child_exception
> OSError: [Errno 13] Permission denied

Is the JS shell running at all, when you run "./js" ?  In recent builds, output directory now contains a prefix which is js/src, in which case the binary is stored in js/src/js.

Also we have 2 tests suite, we have the one under tests, and under jit-test.

> By the way, are there any times you are regularly on IRC? Just curious.

I am always on IRC, my nickname is "nbp"

Comment 11

[Ben Hackett]

You were right, I had to go down to js/src/js. But now I'm getting this problem:
"
ben@ozymandias src$ ../../../tests/jstests.py ./js
[ 526|   0|   0| 469]  14% =====>                                     |   8.1s
REGRESSION - js1_5/extensions/toLocaleFormat-02.js
[6224|   1|   0| 475] 100% ==========================================>|  84.4s
REGRESSIONS
    js1_5/extensions/toLocaleFormat-02.js
FAIL
"
Basically I guess what I'm trying to do is just establish normal behaviour before (a) cross compiling and (b) making the code changes. 

Thanks

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #11)
> REGRESSIONS
>     js1_5/extensions/toLocaleFormat-02.js

> Basically I guess what I'm trying to do is just establish normal behaviour

I know we used to have some bugs related to the time zone, can you try running the test again with the following environment variable:

  TZ=America/Los_Angeles

You might also want to run the jit-tests, as they are more relevant when we are working on the Jits.

Comment 13

[Ben Hackett]

Didn't have much time to do anything this morning but I re-ran that test suite. You mean a bash environment variable right? I still got the same result.
I'll run the jit test suite tonight. If that test suite passes should I just forget about this one?

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #13)
> Didn't have much time to do anything this morning but I re-ran that test
> suite. You mean a bash environment variable right? I still got the same
> result.

You need to export it, or add it as prefix of the command you are running.

export TZ=America/Los_Angeles

Comment 15

[Ben Hackett]

Did the export, got the same problem. 

In other news, I ran the jit tests and everything passed. But when I tried to cross compile (and this is all without changing the code yet), I enter...
PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig CC="gcc -m32" CXX="g++ -m32" AR=ar ../configure --target=i686-pc-linux
and get...
...
checking whether the C compiler (gcc -m32  ) works... no
configure: error: installation or configuration problem: C compiler cannot create executables.
------ config.log ------
configure:2032: c++ -c   conftest.c 1>&5
configure:2052: checking for i686-pc-linux-gcc
configure:2086: checking for gcc
configure:2199: checking whether the C compiler (gcc -m32  ) works
configure:2215: gcc -m32 -o conftest    conftest.c  1>&5
/usr/bin/ld: cannot find crt1.o: No such file or directory
/usr/bin/ld: cannot find crti.o: No such file or directory
...

Another option might be my laptop. I've got 32 bit ubuntu on there, but the CPU is 64 bit. Will that work? One drawback is that it has only 2GB of RAM. I ran tests with unmodified code and modified code. It looks like my code breaks pretty much every test there is... Attached is the file with most of the ouput. 

Any advice for how to quickly figure out what I am doing wrong? On my little laptop that runs 32 bit ubuntu it probably takes about 2 hours for the build and testing. Its painfully slow. So I can't afford to get the bug fix wrong too many times.

Thanks.

Comment 16

[Ben Hackett]

Attachment: March 14th test results (32 bit ubuntu)

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

When I cross-compile to x86 on Ubuntu, I usually use the following configure:

CC="gcc -m32 -march=pentiumpro" CXX="g++ -m32 -march=pentiumpro" AR=ar /home/ben/code/moz/inbound/js/src/configure --target=i686-pc-linux ...

There is apparently no need to define the PKG_CONFIG_LIBDIR env variable. Could you try that? Otherwise, it might be some missing package needed for cross-compilation but I can't find out the name right now.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

h4writer is saying on IRC that g++-multilib and gcc-multilib are the prerequisite packages for x86 cross compilation.

Comment 19

[Ben Hackett]

Thank you Benjamin! g++-multilib did the trick. Now I've got a much much much faster build time to work with. 

Still have no idea why I'm breaking all these tests, but that is a separate story..

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hackett from comment #19)
> Still have no idea why I'm breaking all these tests, but that is a separate
> story..

Unless you want to investigate these tests, I'll suggest that you assume the state of these tests before and keep doing your modifications and check that you are not changing the state of these tests.

I often do that when I disable features such as the internationalization.

Comment 21

[Ben Hackett]

Sorry I am just plain stumped, I can't figure out why this is not working.

The first attachment from a while ago contains all of the many bugs that are coming up.

The second attachment (March 22nd) contains the 2 source files I have modified.

In CompactBuffer.h, the only changes are:
I added line 14
I added lines 86-89

In Safepoints.cpp, the only changes are:
I modified line 46
I modified lines 342, 345, 351, 352, 354, 358

Thanks for any insight.

Comment 22

[Ben Hackett]

Attachment: CompactBuffer.h

In CompactBuffer.h, the only changes are:
I added line 14
I added lines 86-89

Comment 23

[Ben Hackett]

Attachment: Safepoints.cpp

In Safepoints.cpp, the only changes are:
I modified line 46
I modified lines 342, 345, 351, 352, 354, 358

Comment 24

[Nicolas B. Pierron [:nbp]]

Plain files are hard to review, I highly recommend you to follow the instructions which are listed here[1,2].

A part from these, the CompactBuffer.h file should not depend on the Register.h header.  Otherwise the sizeof fix is correct, and the readBasedOnRegisterMask should moved to the Safepoint.cpp file and should just be written in a similar way as writeRegisterMask is.

> +    osiCallPointOffset_ = stream_.readBasedOnRegisterMask();

Your modification does not work because, you changed a read which is not reading a register mask.
The SafepointWriter::encode function encode what the SnapshotReader constructor decodes.

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Comment 25

[Ben Hackett]

Attachment: SafepointReader.patch

In Safepoints.cpp,
I modified line 46,
I added lines 52-59,
I modified lines 354, 360, 361, 363, 367

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8395457 [details] [diff] [review]
SafepointReader.patch

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

This looks good, just a few style comments before being able to push it.

::: js/src/jit/Safepoints.cpp
@@ +51,5 @@
>  
> +static int32_t 
> +ReadRegisterMask(CompactBufferReader &stream)
> +{
> +    if (sizeof(PackedRegisterMask) == 1) 

style-nit: remove the trailing white spaces of your patch.

@@ +54,5 @@
> +{
> +    if (sizeof(PackedRegisterMask) == 1) 
> +        return stream.readByte();
> +    else 
> +        return stream.readUnsigned();

style-nit: This else is useless, as the previous return statement will continue the execution.

Comment 27

[Ben Hackett]

Attachment: SafepointReader.patch

I should have noticed the useless return. Is that okay now?

Thanks Nicolas.

Comment 28

[Ben Hackett]

useless else***. I blame monday on that.

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8396090 [details] [diff] [review]
SafepointReader.patch

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

Perfect :)

Comment 30

[Carsten Book [:Tomcat]]

landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/6a72a2510405

Comment 31

[Carsten Book [:Tomcat]]

and had to backed out for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=36716727&tree=Mozilla-Inbound

Comment 32

[Nicolas B. Pierron [:nbp]]

Thanks Tomcat, and sorry for this issue, I missed it during my review.

Comment 33

[Ben Hackett]

What's up? I broke the code? How do I fix and test this? Or just let me know what to do if I'm asking the wrong questions. No rush either, I understand you are busy this week.

Comment 34

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Ben Hackett from comment #33)
> What's up? I broke the code? How do I fix and test this? Or just let me know
> what to do if I'm asking the wrong questions. No rush either, I understand
> you are busy this week.

If you look at [0], you'll see there is a compilation error:
/jit/Safepoints.cpp:362:51: error: no member named 'ReadRegisterMask' in 'js::jit::CompactBufferReader'

You might want to check that you've implemented this on all platforms.

[0] https://tbpl.mozilla.org/php/getParsedLog.php?id=36716727&tree=Mozilla-Inbound#error0

Comment 35

[Ben Hackett]

Attachment: SafepointReader.patch

Oh wow, I see now. I screwed up. Sorry Nicolas. Sorry Tomcat.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f01dc3be26a7

Comment 37

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

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