RFR: 8312401: SymbolTable::do_add_if_needed hang when requesting length exceeds max_symbol_length [v3]

[Ioi Lam]

On Tue, 25 Jul 2023 03:57:52 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> src/hotspot/share/classfile/symbolTable.cpp line 340:
>> 
>>> 338: 
>>> 339: Symbol* SymbolTable::new_symbol(const char* name, int len) {
>>> 340:   assert(len <= Symbol::max_length(), "sanity");
>> 
>> There are other calls to `java_lang_Throwable::detail_message()` that will overflow `Symbol::max_length()`, so adding this assert will just change the hang into a crash, neither is desirable.
>> 
>> If we want to properly fix `java_lang_Throwable::detail_message()` in a future RFE, it's better to temporarily change `java_lang_Throwable::detail_message()` in this PR to truncate the string (instead of hanging). That's a better alternative than crashing or hanging.
>
>> There are other calls to `java_lang_Throwable::detail_message()` that will overflow `Symbol::max_length()`, so adding this assert will just change the hang into a crash, neither is desirable.
>> 
>> If we want to properly fix `java_lang_Throwable::detail_message()` in a future RFE, it's better to temporarily change `java_lang_Throwable::detail_message()` in this PR to truncate the string (instead of hanging). That's a better alternative than crashing or hanging.
> 
> Thanks for the thoughts. We have't seen issues in other call paths. It's better to fix the other callers properly with JDK-8312616 instead of using a workaround (truncate) with an undefined behavior.
> 
> Failing with assert provides more useful information than hang, however most end users only run on release binary (so most users would not see asserts). As no hang has been reported in other call paths, I think the new asserts would not be triggered in those cases with debug binaries. I can also revert the new asserts in this PR if there's a concern.

OK, sounds good to me. I think the current PR is fine.

But the name of the JBS issue should be changed, since this PR doesn't fix the fundamental cause of the hanging.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14938#discussion_r1273033561