Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions #115

Closed
wants to merge 1 commit into from

Conversation

arun-joseph
Copy link
Member

@arun-joseph arun-joseph commented Feb 19, 2020

With new bytecode format introduced in Webkit 608.1, the unlinked and linked bytecodes were replaced by narrow (1-byte operand) and wide (4-byte operand) bytecodes. These were extended to narrow, wide16 and wide32 bytecodes in WebKit 609.1. In narrow instructions, each argument of the opcode has a fixed size of 1-byte. The same applies for wide 16 and wide32 with 2-byte and 4-byte, respectively.

In the Low Level Intepreter (LLInt), each opcode has a corresponding ID assigned for narrow, wide16 and wide32 implementation, and the variable numOpcodeIDs is used to denote the total number of opcodes. The narrow opcode IDs are mapped from 0 to (numOpcodeIDs - 1). The next numOpcodeIDs opcode IDs are mapped to wide16 opcodes, and similarly, the next numOpcodeIDs to wide32 opcodes. The same can be found in LowLevelInterpreter.cpp and also in the autogenerated file Bytecodes.h.

Bug: When getOpcodeWide(id) is called in LLIntData.h, the value (id - numOpcodesIDs) is returned.

Fix: It's modified to (id + numOpcodesIDs) in getOpcodeWide16() and (id + numOpcodesIDs*2) in getOpcodeWide32().


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions

Reviewers

  • Guru Hb (ghb - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/115/head:pull/115
$ git checkout pull/115

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 19, 2020

👋 Welcome back ajoseph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr Ready for review label Feb 19, 2020
@mlbridge
Copy link

mlbridge bot commented Feb 19, 2020

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Feb 19, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good. Testing looks good.

@openjdk
Copy link

openjdk bot commented Feb 19, 2020

@arun-joseph This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions

Reviewed-by: ghb, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 48ddd809f60b009d02971320afe709da701b1b22.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@guruhb, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Feb 19, 2020
@arun-joseph
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 20, 2020

@arun-joseph
Your change (at version eed2043) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor Ready to sponsor label Feb 20, 2020
@guruhb
Copy link
Contributor

guruhb commented Feb 20, 2020

/sponsor

@openjdk openjdk bot closed this Feb 20, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Feb 20, 2020
@openjdk
Copy link

openjdk bot commented Feb 20, 2020

@guruhb @arun-joseph The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.

Pushed as commit d8e7f85.

@mlbridge
Copy link

mlbridge bot commented Feb 20, 2020

Mailing list message from Guru Hb on openjfx-dev:

Changeset: d8e7f85
Author: Arun Joseph <ajoseph at openjdk.org>
Committer: Guru Hb <ghb at openjdk.org>
Date: 2020-02-20 05:33:04 +0000
URL: https://git.openjdk.java.net/jfx/commit/d8e7f850

8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions

Reviewed-by: ghb, kcr

! modules/javafx.web/src/main/native/Source/JavaScriptCore/llint/LLIntData.h

@arun-joseph arun-joseph deleted the 8239454 branch February 20, 2020 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants