Skip to content

Added zend_get_opcode_flags() function #1610

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 30, 2015
Merged

Added zend_get_opcode_flags() function #1610

merged 3 commits into from
Oct 30, 2015

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Oct 28, 2015

Added zend_get_opcode_flags() function to get information about opcode operands and extended_value meaning

bwoebi
bwoebi reviewed Oct 28, 2015
View reviewed changes
Zend/zend_vm_def.h
@@ -6475,7 +6475,7 @@ ZEND_VM_C_LABEL(fe_fetch_w_exit):
ZEND_VM_NEXT_OPCODE();
}

ZEND_VM_HANDLER(114, ZEND_ISSET_ISEMPTY_VAR, CONST|TMPVAR|CV, UNUSED)
ZEND_VM_HANDLER(114, ZEND_ISSET_ISEMPTY_VAR, CONST|TMPVAR|CV, UNUSED, VAR_FETCH)
Copy link
Member

Choose a reason for hiding this comment

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

This actually is ISSET|VAR_FETCH (both flags).

Copy link
Member Author

Choose a reason for hiding this comment

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

right. I'll implement this.

@bwoebi
Copy link
Member

bwoebi commented Oct 28, 2015

In general, I like the patch, I just wonder whether some names are not descriptive enough or ambiguous.

LINE => what type of line? opline?
FCALL => a function call? That it's a fast_call isn't very intuitive (when we rename this one to FAST_CALL, also rename FRET to FAST_RET
REL_LINE => relative opline offset? I guess as it's used for e.g. ZEND_JMPNZ. should be REL_OPLINE too.
ADDR is… the same than REL_LINE, just for operands? Why don't we just use REL_OPLINE for both?

Additionally, we loose the ability to get the number of opcodes here, previously accessible through sizeof(zend_vm_opcodes_map) / sizeof(char*) via some extern...
May you please add some #define?

I'm just not sure whether some special cases which are directly tied to a specific opcode need their own flags (e.g. FRET, FCALL), after all we still need direct opcode comparisons, as, e.g. ZEND_CATCH has a bit of a special result variable.

@dstogov
Copy link
Member Author

dstogov commented Oct 29, 2015

On Thu, Oct 29, 2015 at 12:36 AM, Bob Weinand notifications@github.com
wrote:

In general, I like the patch, I just wonder whether some names are not
descriptive enough or ambiguous

LINE => what type of line? opline?

Yeah - absolute opline number. I'll change this into OPLINE (or JMP_NUM)

FCALL => a function call? That it's a fast_call isn't very intuitive (when
we rename this one to FAST_CALL, also rename FRET to FAST_RET

Agree. I'll do this.

REL_LINE => relative opline offset? I guess as it's used for e.g.
ZEND_JMPNZ. should be REL_OPLINE too.

Agree, REL_OPLINE (or JMP_REL)

ADDR is… the same than REL_LINE, just for operands? Why don't we just use
REL_OPLINE for both?

ADDR may be "absolute" or "relative" dependent on arch (see
ZEND_USE_ABS_JMP_ADDR in zend_compile.h).
(I may rename this into JMP_ADDR).

Additionally, we loose the ability to get the number of opcodes here,
previously accessible through sizeof(zend_vm_opcodes_map) / sizeof(char*)
via some extern...
May you please add some #define?

Ok. I'll do.

I'm just not sure whether some special cases which are directly tied to a
specific opcode need their own flags (e.g. FRET, FCALL), after all we still
need direct opcode comparisons, as, e.g. ZEND_CATCH has a bit of a special
result variable.

We still have few bits reserved, and may try to use them.

Thanks. Dmitry.

—
Reply to this email directly or view it on GitHub
#1610 (comment).

@bwoebi
Copy link
Member

bwoebi commented Oct 29, 2015

I like the JMP_* identifiers :-)

Thanks!

@dstogov
Copy link
Member Author

dstogov commented Oct 29, 2015

On Thu, Oct 29, 2015 at 3:33 PM, Bob Weinand notifications@github.com
wrote:

I like the JMP_* identifiers :-)

OK. what exactly? JMP_ADDR, JMP_ABS, JMP_REL?

Thanks!

—
Reply to this email directly or view it on GitHub
#1610 (comment).

@bwoebi
Copy link
Member

bwoebi commented Oct 29, 2015

yeah, these are fine.

@dstogov
Copy link
Member Author

dstogov commented Oct 29, 2015

renamed.
I'm going to commit this into master tomorrow.

On Thu, Oct 29, 2015 at 5:53 PM, Bob Weinand notifications@github.com
wrote:

yeah, these are fine.

—
Reply to this email directly or view it on GitHub
#1610 (comment).

@dstogov
Added zend_get_opcode_flags() function to get information about opcod…
d83600a 
…e operands and extended_value meaning
@dstogov
Fixed few issues:
5c65fc2 
- Added ZEND_VM_LAST_OPCODE macro
- Use better names LINE->OPLINE, REL_LINE->REL_OPLINE, FCALL->FAST_CALL, FRET->FAST_RET
- Added ISSET flag to extended value of ZEND_ISSET_ISEMPTY_VAR opcode
@dstogov
Argument type renaming (ADDR->JMP_ADDR, OPLINE->JMP_ABS, REL_OPLINE->…
9ccb432 
…JMP_REL)
@php-pulls php-pulls merged commit 9ccb432 into php:master Oct 30, 2015
@dstogov dstogov deleted the opcode_flags branch October 14, 2019 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dstogov @bwoebi @php-pulls