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

replace custom opcode dumper with O+ dump #7227

Merged
merged 8 commits into from
Jul 13, 2021

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Jul 10, 2021

This replaces custom opcode dumper with Optimizer API.

It also changes the default behaviour of printing oplines while stepping; we'll now only print instructions if quietness is off (quietness is by default on).

This results in the loss of the ability to send oplog to specific file, this feature must be untested and is highly likely unused - the only real use case for the oplog file was superseded by the oplog list (coverage) feature.

@krakjoe krakjoe force-pushed the phpdbg-optimizer-dump branch 3 times, most recently from 8aff7ff to 5e5eaca Compare July 10, 2021 14:30
sapi/phpdbg/phpdbg_opcode.c Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jul 12, 2021

What's a bit unfortunate here is that the opcodes will be dumped to stderr, while the rest of phpdbg output is on stdout. May be problematic for interleaved output.

Maybe we should pass FILE * to zend_dump_op_line, so we can use stdout for phpdbg?

@krakjoe
Copy link
Member Author

krakjoe commented Jul 12, 2021

I'm not sure; Removing the oplog setting from phpdbg - which used to set FILE* output - seemed okay because you can redirect stderr to a file, and mostly get the same functionality.

The oplog can be quite loud, and it's probably actually desirable to have the ability to redirect away from normal output.

We'd have to pass a FILE* to every dumping function ... the diff for that is a bit far reaching for my liking ...

... cut ...

@krakjoe
Copy link
Member Author

krakjoe commented Jul 12, 2021

bit far reaching for my liking ...

Do we really want to do this ?

Zend/Optimizer/zend_dump.c Outdated Show resolved Hide resolved
Zend/Optimizer/zend_dump.c Outdated Show resolved Hide resolved
sapi/phpdbg/phpdbg.h Show resolved Hide resolved
sapi/phpdbg/phpdbg_opcode.c Outdated Show resolved Hide resolved
sapi/phpdbg/phpdbg_opcode.h Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jul 12, 2021

bit far reaching for my liking ...

Do we really want to do this ?

I don't particularly care. If you think dumping to stderr from phpdbg is fine, then let's stick with it.

sapi/phpdbg/phpdbg.h Outdated Show resolved Hide resolved
@krakjoe krakjoe merged commit 60fbd6d into php:master Jul 13, 2021
@krakjoe krakjoe deleted the phpdbg-optimizer-dump branch July 13, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants