Incorrect LIR type names in Ion logs

RESOLVED FIXED in Firefox 52

Status

()

P5
trivial
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: terpri, Assigned: terpri)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The definition of typeChars in js/src/jit/LIR.cpp is out of sync with the LDefinition::Type enum, resulting in incorrect Ion log output.
(Assignee)

Comment 1

2 years ago
Created attachment 8808910 [details] [diff] [review]
Update LIR type names for debugging output

This is one possible fix; I replaced typeChars with a function, and gcc/clang should emit warnings (from -Wswitch) if someone forgets to update LIR.cpp.
Comment on attachment 8808910 [details] [diff] [review]
Update LIR type names for debugging output

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

Looks good to me, thank you for the patch! I see that you are new to bugzilla but you might know the release process thanks to other people from Igalia :)

Just in case, to summarize:
- I've approved the patch (r+ for review+). Next time, you can take a look at who's touched the code you've modified with `hg blame`, and ask them for review by setting the flag to `r?` in bugzilla.
- Now somebody needs to land the patch. Ideally it should through our CI; we have testing CI called "try"; I'll push to try for you today. You can request commit access level 1 to push to try by yourselves, according to https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
- Then when we see that all the tests are passing, somebody can push to the integration CI (mozilla-inbound). By adding the `checkin-needed` keyword to the list of keywords of this bug, a sheriff would push it on your behalf, if there's a try push link and all the tests are passing. Otherwise, somebody with commit access level 3 needs to push this.

I've added `r=bbouvier` to the commit message, so it shows that somebody approved this change.

Here's the try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=779cfd636d1a9922ac3a56c3c499347b61afbc78
Attachment #8808910 - Flags: review+
Looks mostly green (the errors are not related to your patch). Setting checkin-needed so that a sheriff lands it on your behalf. Thanks again for the patch!
Keywords: checkin-needed
Assignee: nobody → robin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 4

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/592187676a06
Update LIR type names for debugging output. r=bbouvier
Keywords: checkin-needed
Priority: -- → P5

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/592187676a06
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.