Closed Bug 1186724 Opened 9 years ago Closed 9 years ago

Make BytecodeEmitter::emit2 take a uint8_t instead of a jsbtyecode

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: efaust, Assigned: mrrrgn, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 1 obsolete file)

As near as I can tell, we never have jsbytecode running around, and we almost certainly don't have it laying around for the second operand value. Instead, we should be clear that we expect an arbitrary byte there. This will also alleviate the needs for tons of spurious casting all over the emitter.
This should be a good first bug :)

Required code changes are following:
  * Change the type of the second argument (op1) of BytecodeEmitter::emit2 [1] to uint8_t
  * Rewrite all BytecodeEmitter::emit2 callsites to follow the change (remove cast to jsbytecode, or add cast to uint8_t)

You'll need basic knowledge of C++, and be able to build and test Firefox [2] or SpiderMonkey [3].

A skilled first-time contributor should be able to finish this in a month.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.

[1] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/js/src/frontend/BytecodeEmitter.cpp#l247
(note that I linked to specific revision of the file to point the method, it may not be the latest one)
[2] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Mentor: arai.unmht
Whiteboard: [good first bug][lang=c++]
I'm going to give this a shot.
Assignee: nobody → winter2718
Comment on attachment 8645393 [details] [diff] [review]
spidermonkey-jsbytecode-to-uint8_t.diff

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

Thank you for your patch :D
It's almost perfect, with following minor fixes.

https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.cpp#4348
>     jsbytecode offset = 1;
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.cpp#4509
>        if (offset != 1 && !emit2(JSOP_PICK, offset - 1))

I think there's no reason to keep the type of `offset` as `jsbytecode` now, please make it `uint8_t` as well.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +260,2 @@
>      code[0] = jsbytecode(op);
>      code[1] = op1;

Type of `code` should remain `jsbytecode*`, as other methods do so and next line does cast.
Instead, it would be better to add cast to `code[1] = op1;` line too.

@@ +2921,5 @@
> +        if (!emit2(JSOP_PICK, 4))       	    // OBJ N N+1 KEY THIS
> +            return false;
> +        if (!emit2(JSOP_PICK, 4))       	    // N N+1 KEY THIS OBJ
> +            return false;
> +        if (!emit2(JSOP_PICK, 3))       	    // N KEY THIS OBJ N+1

Please replace HTABs in above lines with SPACEs ;)
Attachment #8645393 - Flags: review?(arai.unmht) → feedback+
Got it, thank you for the review! :)
Attachment #8645393 - Attachment is obsolete: true
Attachment #8645401 - Flags: review?(arai.unmht)
Comment on attachment 8645401 [details] [diff] [review]
spidermonkey-jsbytecode-to-uint8_t.diff

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2943,5 @@
>              return emit1(JSOP_ZERO);
>          if (ival == 1)
>              return emit1(JSOP_ONE);
>          if ((int)(int8_t)ival == ival)
> +            return emit2(JSOP_INT8, (int8_t)ival);

This doesn't cause warnings about signedness in GCC or something?

`uint8_t(int8_t(ival))` would be ok.

In general we prefer C++-style function-like casts to C-style casts.
Comment on attachment 8645401 [details] [diff] [review]
spidermonkey-jsbytecode-to-uint8_t.diff

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

Thank you again :)
r+ with those cast fixes that jorendorff pointed out.
Attachment #8645401 - Flags: review?(arai.unmht) → review+
https://hg.mozilla.org/mozilla-central/rev/a2be047f4e05
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: