Enhance `zend_dump_op_array` to Properly Represent Non-Printable Characters (GH-15680) #15730

WangYihang · 2024-09-03T18:10:39Z

This pull request addresses issue GH-15680 which proposed an enhancement for zend_dump_op_array to properly represent non-printable characters in strings.

This is useful for debugging purposes, as it allows developers to see the actual content of strings that contain non-printable characters.

I have also added testcases for this change, and all tests related to this pull request were passed.

For example, for the following php code:

<?php
fwrite($buffer, "\x00\x01\xff--\n");

Current Output:

0040 INIT_FCALL 2 112 string("fwrite")
0041 SEND_VAR CV6($buffer) 1
0042 SEND_VAL string("--
") 2                                     <------ new line (\n) is directly printed
0043 DO_ICALL
Expected Output:

With this change, the output might looks like:

0040 INIT_FCALL 2 112 string("fwrite")
0041 SEND_VAR CV6($buffer) 1
0042 SEND_VAL string("\x00\x01\xff--\n") 2           <------ properly escaped with readable representations
0043 DO_ICALL

Related Issues/Pull requests:

…acters (phpGH-15680) This change enhances `zend_dump_op_array` to properly represent non-printable characters in strings. This is useful for debugging purposes, as it allows developers to see the actual content of strings that contain non-printable characters.

ext/standard/string.c

dstogov

See the optimisation suggestion.
The rest is fine.

ext/standard/string.c

dstogov

I don't see big problems, but I also don't see a real need for this.
@iluuu1994 what do you think?

dstogov · 2024-09-04T06:37:25Z

ext/standard/charrepr.h

+    const unsigned char len;
+} char_repr_t;
+
+static const char_repr_t char_reprs[256] = {


What is the reason to separate this array into a header?
It's going to be duplicated every time the header is included.

The reason I separated this array into a header file was primarily for modularity and reusability.

The current file (string.c) is already quite long, and combining these contents with the C file might make it harder to read and maintain.

However, if you believe this separation is unnecessary, I can certainly move this part back into string.c.

Was this code copied from somewhere? There's no header/license that would indicate so.

ext/standard/string.c

WangYihang · 2024-09-04T13:47:19Z

I don't see big problems, but I also don't see a real need for this. @iluuu1994 what do you think?

Thank you for your feedback.

While I understand that this enhancement might not seem immediately necessary, I believe it could bring several significant benefits:

Improved Developer Experience: Introducing a dedicated function or enhancing the existing one would provide developers with a more reliable way to handle non-printable characters, reducing the risk of errors in edge cases and improving code readability and maintainability. This need has been raised by several developers concerning string representation in PHP. Similar work has already been merged ([1] [2]), which focused on quoting a few specific characters like newlines. My pull request builds on that foundation by extending the functionality to cover all non-printable characters, making it a logical next step that should be considered for inclusion.
Consistency Across Languages: Many modern programming languages, such as Python (repr), Ruby (inspect), and JavaScript (JSON.stringify), offer similar functionality. Having a comparable feature in PHP would align it more closely with these languages, providing a familiar toolset for developers who work across multiple environments.
Prevention of Display Issues: During debugging, dumping non-printable control characters can disrupt the terminal's display, making the output difficult to interpret. A function that safely represents these characters would prevent such issues, improving debugging efficiency.
Easier Debugging with var_dump: Currently, PHP's var_dump function does not quote strings, which can make it challenging for developers to distinguish between visible and non-visible characters during debugging. This can lead to confusion and increased difficulty in identifying issues within the code. A standardized way to represent strings, including all non-printable characters, would alleviate this problem. Which could be left for the future work (If you guys agreed that it worthwhile, I can help to implement that).
Future-Proofing: Even if the need isn’t pressing at this moment, implementing this feature could support future debugging and logging functionalities, helping to keep PHP robust and user-friendly as it evolves.

I believe this change, while relatively minor, could add substantial value by addressing these points. I’m open to further refining the implementation based on your feedback.

What are your thoughts on this? @iluuu1994

dstogov · 2024-09-09T07:32:47Z

@iluuu1994 @nielsdos @arnaud-lb please make a decision.

iluuu1994

I don't object to changing this. LGTM otherwise.

iluuu1994 · 2024-09-09T09:24:24Z

ext/standard/charrepr.h

+    const unsigned char len;
+} char_repr_t;
+
+static const char_repr_t char_reprs[256] = {


Was this code copied from somewhere? There's no header/license that would indicate so.

iluuu1994 · 2024-09-09T09:25:21Z

ext/standard/charrepr.h

+	{"\\\"", 2},
+	{"#", 1},
+	{"$", 1},
+	{"%%", 1},


This looks fishy. %% only needs to be escaped in printf. Given the impl uses memcpy rather than strcpy, it shouldn't lead to a bug though.

Was this code copied from somewhere? There's no header/license that would indicate so.

Actually, I just generated this header file by a few lines of ad-hoc Python code with a few manual tweaks.

This looks fishy. %% only needs to be escaped in printf. Given the impl uses memcpy rather than strcpy, it shouldn't lead to a bug though.

Ahhhh, yes, I will fix it soon.

@iluuu1994 Fixed in 32340ba.

32340ba#diff-9dc13ac25ae79e9c7bd257341e953577aae1b2cb7562fb9acd64ce06c1baa71eL63-R63

iluuu1994 · 2024-09-09T09:31:45Z

ext/standard/string.c

@@ -3863,6 +3866,38 @@ PHPAPI zend_string *php_addcslashes_str(const char *str, size_t len, const char
 }
 /* }}} */

+/* {{{ php_repr_str */
+PHPAPI zend_string *php_repr_str(const char *str, size_t len) {


Ah, and I would also improve the naming. I don't find this particularly descriptive.

Any suggestions about the naming? basically this function take a string as input, returns a zend_string which converts all non-printable chars to a human readable format (e.g. chr(256) will be convert to "\xff").

The repr part in the function name was inspired by the python repr function which means REPResenting the STRing.

repr returns a string containing a printable representation of an object in Python.

arnaud-lb

I like this change. This looks good to me, but I think we could reuse some existing code.

arnaud-lb · 2024-09-09T14:24:26Z

ext/standard/string.c

@@ -3863,6 +3866,38 @@ PHPAPI zend_string *php_addcslashes_str(const char *str, size_t len, const char
 }
 /* }}} */

+/* {{{ php_repr_str */
+PHPAPI zend_string *php_repr_str(const char *str, size_t len) {


I think this function has the same output as smart_str_append_escaped() (minus the enclosing double quotes). Is there anything blocking us from using it?

arnaud-lb · 2024-09-09T14:35:06Z

Zend/Optimizer/zend_dump.c

-			zend_string *escaped_string = php_addcslashes(Z_STR_P(zv), "\"\\", 2);
-
-			fprintf(stderr, " string(\"%s\")", ZSTR_VAL(escaped_string));
-
+			zend_string *escaped_string = php_repr_str(Z_STR_P(zv)->val, Z_STR_P(zv)->len);
+			fprintf(stderr, " string(%s)", ZSTR_VAL(escaped_string));


We can achieve a similar result with php_addcslashes(Z_STR_P(zv), "\x00..\x1f\\\\\x7f..\xff", 10) (with the difference that it uses an octal representation), or the same result with smart_str_append_escaped().

…nt Non-Printable Characters in String Literals Replaces phpGH-15730 as that PR became stale. But instead of introducing a new helper, reuse smart_str_append_escaped(), this also removes the dependency on ext/standard.

WangYihang requested review from dstogov, iluuu1994 and bukka as code owners September 3, 2024 18:10

github-actions bot added Extension: opcache Extension: standard Category: Optimizer labels Sep 3, 2024

WangYihang mentioned this pull request Sep 3, 2024

Enhance zend_dump_op_array to Properly Represent Non-Printable Characters in String Literals #15680

Closed

dstogov reviewed Sep 3, 2024

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

dstogov reviewed Sep 3, 2024

View reviewed changes

nielsdos reviewed Sep 3, 2024

View reviewed changes

ext/standard/string.c Outdated Show resolved Hide resolved

WangYihang added 3 commits September 4, 2024 04:29

Improve performance of php_repr_str by 32.56%

5aa31c8

Fix invalid condition for zend_string_truncate in php_repr_str

2dc8acc

Fix char_reprs array initialization in charrepr.h

a4f53c8

WangYihang force-pushed the master branch from 7d2ccfc to a4f53c8 Compare September 3, 2024 20:58

dstogov reviewed Sep 4, 2024

View reviewed changes

chore: Optimize string representation in php_repr_str

90fe73f

iluuu1994 approved these changes Sep 9, 2024

View reviewed changes

iluuu1994 reviewed Sep 9, 2024

View reviewed changes

refactor charrepr.h to fix array initialization and update macro name

32340ba

arnaud-lb reviewed Sep 9, 2024

View reviewed changes

Merge branch 'php:master' into master

238a730

github-actions bot added the ABI break label Oct 21, 2024

nielsdos mentioned this pull request Dec 26, 2024

Implement GH-15680: Enhance zend_dump_op_array to Properly Represent Non-Printable Characters in String Literals #17277

Closed

nielsdos closed this in 55afe8b Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance `zend_dump_op_array` to Properly Represent Non-Printable Characters (GH-15680) #15730

Enhance `zend_dump_op_array` to Properly Represent Non-Printable Characters (GH-15680) #15730

WangYihang commented Sep 3, 2024 •

edited

Loading

dstogov left a comment

dstogov left a comment

dstogov Sep 4, 2024

WangYihang Sep 4, 2024

iluuu1994 Sep 9, 2024

WangYihang commented Sep 4, 2024

dstogov commented Sep 9, 2024

iluuu1994 left a comment

iluuu1994 Sep 9, 2024

iluuu1994 Sep 9, 2024

WangYihang Sep 9, 2024 •

edited

Loading

WangYihang Sep 9, 2024

WangYihang Sep 9, 2024

iluuu1994 Sep 9, 2024

WangYihang Sep 9, 2024 •

edited

Loading

arnaud-lb left a comment

arnaud-lb Sep 9, 2024

arnaud-lb Sep 9, 2024 •

edited

Loading

Enhance zend_dump_op_array to Properly Represent Non-Printable Characters (GH-15680) #15730

Enhance zend_dump_op_array to Properly Represent Non-Printable Characters (GH-15680) #15730

Conversation

WangYihang commented Sep 3, 2024 • edited Loading

dstogov left a comment

Choose a reason for hiding this comment

dstogov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WangYihang commented Sep 4, 2024

dstogov commented Sep 9, 2024

iluuu1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WangYihang Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WangYihang Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

arnaud-lb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaud-lb Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Enhance `zend_dump_op_array` to Properly Represent Non-Printable Characters (GH-15680) #15730

Enhance `zend_dump_op_array` to Properly Represent Non-Printable Characters (GH-15680) #15730

WangYihang commented Sep 3, 2024 •

edited

Loading

WangYihang Sep 9, 2024 •

edited

Loading

WangYihang Sep 9, 2024 •

edited

Loading

arnaud-lb Sep 9, 2024 •

edited

Loading