Closed Bug 1572870 Opened 5 years ago Closed 5 years ago

Use bytecode interface classes BytecodeLocation and BytecodeIterator to encapsulate bytecode manipulation in VerifyGlobalNames

Categories

(Core :: JavaScript Engine, enhancement, P2)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: asorholm, Assigned: asorholm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1572870 - Use bytecode interface classes BytecodeLocation and BytecodeIterator to encapsulate bytecode manipulation in VerifyGlobalNames. r=tcampbell,jandem
47 bytes, text/x-phabricator-request
Details | Review

Currently in VerifyGlobalNames, bytecode is manipulated directly with the use of jsbytecode* and pcToOffset. The goal of this bug is to encapsulate that manipulation with the bytecode interface classes BytecodeLocation and BytecodeIterator.

Blocks: 1478034
Summary: Use bytecode interface classes `BytecodeLocation` and `BytecodeIterator` to encapsulate bytecode manipulation in VerifyGlobalNames → Use bytecode interface classes BytecodeLocation and BytecodeIterator to encapsulate bytecode manipulation in VerifyGlobalNames
Assignee: nobody → asorholm
Status: UNCONFIRMED → NEW
Ever confirmed: true

Replacing jsbytecode and pcToOffset with BytecodeLocation and BytecodeIterator.

Priority: -- → P2
Status: NEW → ASSIGNED
Keywords: checkin-needed

Landing failed:

On Fri, October 11, 2019, 3:25 AM GMT+3, by apavel@mozilla.com.
Revisions: D48112 diff 174490 ← D41326 diff 175479 ← D41485 diff 166153
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpNahp62 js/src/vm/BytecodeLocation.h Hunk #1 FAILED at 69. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation.h.rej js/src/vm/BytecodeLocation-inl.h Hunk #2 FAILED at 23. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation-inl.h.rej js/src/jit/BytecodeAnalysis.cpp Hunk #1 FAILED at 7. 1 out of 10 hunks FAILED -- saving rejects to file js/src/jit/BytecodeAnalysis.cpp.rej abort: patch command failed: exited with status 256

Keywords: checkin-needed
Flags: needinfo?(asorholm)

(In reply to Andreea Pavel [:apavel] from comment #3)

Landing failed:

On Fri, October 11, 2019, 3:25 AM GMT+3, by apavel@mozilla.com.
Revisions: D48112 diff 174490 ← D41326 diff 175479 ← D41485 diff 166153
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpNahp62 js/src/vm/BytecodeLocation.h Hunk #1 FAILED at 69. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation.h.rej js/src/vm/BytecodeLocation-inl.h Hunk #2 FAILED at 23. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation-inl.h.rej js/src/jit/BytecodeAnalysis.cpp Hunk #1 FAILED at 7. 1 out of 10 hunks FAILED -- saving rejects to file js/src/jit/BytecodeAnalysis.cpp.rej abort: patch command failed: exited with status 256

I rebased onto the current tip of mozilla-central (https://hg.mozilla.org/mozilla-central/rev/6660bc0d1b23ae45dc3c952b6e21c0335e9739e9). I built the shell and ran some test cases and everything passed. I'm going to set checkin-needed again, but if you'd like me to create a new try build before checking this in, let me know, I'd be glad to do so. Thanks!

Flags: needinfo?(asorholm)
Keywords: checkin-needed
Blocks: 1572504
No longer blocks: 1572504
Depends on: 1572504

Tried to land but failed again with message:

On Sat, October 12, 2019, 1:55 AM GMT+3, by nbeleuzu@mozilla.com.
Revisions: D48112 diff 174490 ← D41326 diff 175479 ← D41485 diff 176015
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpfo8VBv js/src/vm/BytecodeLocation.h Hunk #1 FAILED at 69. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation.h.rej js/src/vm/BytecodeLocation-inl.h Hunk #2 FAILED at 23. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation-inl.h.rej js/src/jit/BytecodeAnalysis.cpp Hunk #1 FAILED at 7. 1 out of 10 hunks FAILED -- saving rejects to file js/src/jit/BytecodeAnalysis.cpp.rej abort: patch command failed: exited with status 256

Flags: needinfo?(asorholm)
Keywords: checkin-needed

Adam: The problem is that you're trying to land the stack, but seem to have only rebased the top-most commit (which raises the question of how / why this is a stack)

Here's how I reproduced locally:

matthew@xtower:~/unified2$ hg up autoland --clean
3858 files updated, 0 files merged, 274 files removed, 0 files unresolved
(leaving bookmark D48479)
matthew@xtower:~/unified2$ moz-phab patch https://phabricator.services.mozilla.com/D48112 --apply-to . 
Revision D48112 has child commits.  Would you like to patch the full stack?. (YES/No/Always)? y
Patching revisions: D48112 D41326 D41485
Checked out .
Bookmark set to D41485
D48112 applied
patching file js/src/vm/BytecodeLocation.h
Hunk #1 FAILED at 68
1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation.h.rej
patching file js/src/vm/BytecodeLocation-inl.h
Hunk #2 FAILED at 22
1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation-inl.h.rej
patching file js/src/jit/BytecodeAnalysis.cpp
Hunk #1 FAILED at 6
1 out of 10 hunks FAILED -- saving rejects to file js/src/jit/BytecodeAnalysis.cpp.rej
abort: patch failed to apply
Patch failed to apply

So, you'll want to re-jigger the stack, or at least rebase and re-upload all the commits, not just the top most; for lando to work correctly, you should be able to use moz-phab patch to apply the commits to whatever autoland is near when you're submitting.

(In reply to Narcis Beleuzu [:NarcisB] from comment #5)

Tried to land but failed again with message:

On Sat, October 12, 2019, 1:55 AM GMT+3, by nbeleuzu@mozilla.com.
Revisions: D48112 diff 174490 ← D41326 diff 175479 ← D41485 diff 176015
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpfo8VBv js/src/vm/BytecodeLocation.h Hunk #1 FAILED at 69. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation.h.rej js/src/vm/BytecodeLocation-inl.h Hunk #2 FAILED at 23. 1 out of 3 hunks FAILED -- saving rejects to file js/src/vm/BytecodeLocation-inl.h.rej js/src/jit/BytecodeAnalysis.cpp Hunk #1 FAILED at 7. 1 out of 10 hunks FAILED -- saving rejects to file js/src/jit/BytecodeAnalysis.cpp.rej abort: patch command failed: exited with status 256

Oops! I'll take a look at this and see if I can address it in a way that I don't just repeat the same mistake. :-P

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)

So, you'll want to re-jigger the stack, or at least rebase and re-upload all the commits, not just the top most; for lando to work correctly, you should be able to use moz-phab patch to apply the commits to whatever autoland is near when you're submitting.

Thanks for the explanation :) So moz-phab patch the method for locally checking that my fix worked?

Flags: needinfo?(asorholm)

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)

I rebased the patches and updated their phabricator revisions. moz-phab patch ran successfully, here's the output:

[~/src/mozilla-central]$ moz-phab patch https://phabricator.services.mozilla.com/D48112 --apply-to .                                                                        
Installing arc
Cloning into '/home/ash/.mozbuild/moz-phab/arcanist'...
remote: Enumerating objects: 1129, done.
remote: Counting objects: 100% (1129/1129), done.
remote: Compressing objects: 100% (876/876), done.
remote: Total 1129 (delta 191), reused 818 (delta 154), pack-reused 0
Receiving objects: 100% (1129/1129), 762.83 KiB | 0 bytes/s, done.
Resolving deltas: 100% (191/191), done.
Checking connectivity... done.
Cloning into '/home/ash/.mozbuild/moz-phab/libphutil'...
remote: Enumerating objects: 784, done.
remote: Counting objects: 100% (784/784), done.
remote: Compressing objects: 100% (678/678), done.
remote: Total 784 (delta 73), reused 398 (delta 44), pack-reused 0
Receiving objects: 100% (784/784), 1.63 MiB | 0 bytes/s, done.
Resolving deltas: 100% (73/73), done.
Checking connectivity... done.
moz-phab is now distributed via PyPI. Please run `moz-phab self-update`.
Revision D48112 has child commits.  Would you like to patch the full stack?. (YES/No/Always)? y
Patching revisions: D48112 D41326 D41485
Checked out .
Bookmark set to D41485
D48112 applied
D41326 applied
D41485 applied

Should I flag checkin-needed now, or are there some other ways to check my fix before setting checkin-needed so I can be sure I'm not checking in the same bad patch again?

I think that's about as good as it gets for pre-flight checks; I'd double check that your try build is still good, but then go for it.

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #9)

I think that's about as good as it gets for pre-flight checks; I'd double check that your try build is still good, but then go for it.

Sounds good. I just rebased onto the current tip of m-c and reran moz-phab patch..., which passed. Here's the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9387ef971f49436fc71db4bf7103c4f674e56d57

Keywords: checkin-needed

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/723659cc5b85
Use bytecode interface classes BytecodeLocation and BytecodeIterator to encapsulate bytecode manipulation in VerifyGlobalNames. r=jandem

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/723659cc5b85

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: