8301065: Handle control characters in java_lang_String::print

[kevinjwalls]

Change to avoid printing raw control characters in java_lang_String::print.
Usually called in debug printing and error reporting. One usage from debug logging for modules.

Format as \x followed by two hex digits, e.g. \x0A

Small change, in a routine with few callers. Could make two calls to value->byte_at(), which I left in as it reads more clearly, and this is not called at time critical situations.

The error reporting and debug.cpp usages I can test manually, with some trial and error, trying to catch the register info containing relevant info. This same String printing routine is used for showing register contents or stack slot mappings, and for a String, or a class containing a String.

Before the change: (newlines and null embedded in String)

R11=0x000000011f74e568 is an oop: java.lang.String
{0x000000011f74e568} - klass: 'java/lang/String'
 - string: "this is my string
^@and also: this is more of my string
"
 - ---- fields (total size 3 words):
 - private 'hash' 'I' @12  0 (0x00000000)

With the change:

{0x000000011faf5bf0} - klass: 'Test'
 - ---- fields (total size 3 words):
 - private 'lastChar' 'C' @12    10 (0x000a)
 - private 'myString' 'Ljava/lang/String;' @16  "this is my string\x0A\x00and also: this is more of my string\x0A"{0x000000011faf5ca8} (0x23f5eb95)
R15=0x00007f928002a260 is a thread

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8301065: Handle control characters in java_lang_String::print

Reviewers

• Ioi Lam (@iklam - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12190/head:pull/12190
$ git checkout pull/12190

Update a local copy of the PR:
$ git checkout pull/12190
$ git pull https://git.openjdk.org/jdk.git pull/12190/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12190

View PR using the GUI difftool:
$ git pr show -t 12190

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12190.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kevinw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@kevinjwalls The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (d06e0989)
• 00: Full (e703a67b)

[bridgekeeper]

@kevinjwalls This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[kevinjwalls]

(I will get back to this...)

[bridgekeeper]

@kevinjwalls This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[openjdk]

@kevinjwalls This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8301065: Handle control characters in java_lang_String::print

Reviewed-by: iklam, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 37 new commits pushed to the master branch:

• e575924: 8305336: java.security.debug=sunpkcs11 breaks PKCS#11 configuration with slotListIndex
• caa841d: 8306029: ProblemList runtime/ErrorHandling/TestDwarf.java on linux
• 314bad3: 8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries
• 793da60: 8305403: Shenandoah evacuation workers may deadlock
• 2cc4bf1: 8305085: Suppress removal warning for finalize() from test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineFinalizer.java
• 5a78865: 8304930: Enable Link Time Optimization as an option for Visual C++
• 30a140b: 8304912: Use OperatingSystem enum in java.desktop module
• 1fd4006: 8305405: Compile_lock not needed in Universe::genesis()
• ebeee6d: 8305404: Compile_lock not needed for InstanceKlass::implementor()
• d2ce04b: 8301496: Replace NULL with nullptr in cpu/riscv
• ... and 27 more: https://git.openjdk.org/jdk/compare/d8af7a6014055295355a1242db6c2872299c6398...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dholmes-ora]

Escaping the control characters this way seems quite reasonable to me. I assume we never intentionally include them, but if tracking down a bug we might be printing a corrupt string and so this aids in showing the actual character content. Arguably in such a situation all non-printable characters should be escaped, so there could be a second RFE to handle that generalization.

Thanks.

[iklam]

Escaping the control characters this way seems quite reasonable to me. I assume we never intentionally include them, but if tracking down a bug we might be printing a corrupt string and so this aids in showing the actual character content.

We actually have some intentional use of non printable characters. For example, for string concatenation like this:

String x = ....;
String s = "abc" + x + "xyz";

We'll pass a parameter to the StringConcatFactory.makeConcatWithConstants() bootstrap method that looks like this:

"abc\u0001xyz"

[dholmes-ora]

We actually have some intentional use of non printable characters. For example, for string concatenation like this:

String x = ....;
String s = "abc" + x + "xyz";

We'll pass a parameter to the StringConcatFactory.makeConcatWithConstants() bootstrap method that looks like this:

"abc\u0001xyz"

That doesn't seem a valid thing to do as string literals could contain any character, including \u0001. ??

[iklam]

We actually have some intentional use of non printable characters. For example, for string concatenation like this:

String x = ....;
String s = "abc" + x + "xyz";

We'll pass a parameter to the StringConcatFactory.makeConcatWithConstants() bootstrap method that looks like this:

"abc\u0001xyz"

That doesn't seem a valid thing to do as string literals could contain any character, including \u0001. ??

String literals that contain special characters are passed as separate bootstrap arguments, and are marked by \u0002 in the first bootstrap argument to StringConcatFactory.makeConcatWithConstants(). You can see the implementation of StringConcatFactory for details.

[kevinjwalls]

Thanks Ioi and David for looking over this.

The additional StringConcatFactory mention is also educational!

[kevinjwalls]

/integrate

[openjdk]

Going to push as commit 41d6be4.
Since your change was applied there have been 144 commits pushed to the master branch:

• 4b23bef: 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again
• f32adaf: 8304836: Make MALLOC_MIN4 macro more robust
• 2763cf1: 8304896: Update to use jtreg 7.2
• b2ccc97: 8306444: Don't leak memory in PhaseChaitin::PhaseChaitin
• d980cb4: 8306658: GHA: MSVC installation could be optional since it might already be pre-installed
• 62acc88: 8306476: CDS ArchiveHeapTestClass.java test asserts when vm_exit is called on VM thread
• 7400aff: 8305252: make_method_handle_intrinsic may call java code under a lock
• f239695: 8305056: Avoid unaligned access in emit_intX methods if it's unsupported
• 136dad7: 8306566: Open source several clipboard AWT tests
• 30fa50d: 8305062: Refactor CardTable::resize_covered_region
• ... and 134 more: https://git.openjdk.org/jdk/compare/d8af7a6014055295355a1242db6c2872299c6398...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinjwalls Pushed as commit 41d6be4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.