RFR: 8309811: BytecodePrinter cannot handle unlinked classes

[Coleen Phillimore]

On Fri, 23 Jun 2023 05:18:36 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> `BytecodePrinter` assumes that it's printing a method from a linked class (e.g., it assumes that the two bytes following an `invokestatic` bytecode have been rewritten to be a `ConstantPoolCache` index.)
> 
> This works fine for `-XX:+TraceBytecodes`, which prints only code from linked classes. However, it doesn't work `Method::print_codes_on()` for a method from an unlinked class.
> 
> The old code in `BytecodePrinter` uses a very convoluted logic to go from the rewritten index back to the `cp_index`. It's also difficult to understand the type of the indices from the badly named variables like `i` and `orig_i`.
> 
> I had to rewrite all the logic for fetching the indices. Now they are in this form:
> 
> 
>     case Bytecodes::_putfield:
>     case Bytecodes::_getfield:
>       {
>         int cp_index;
>         if (is_linked()) {
>           int cpcache_index = get_native_index_u2();
>           cp_index = cpcache()->entry_at(cpcache_index)->constant_pool_index();
>         } else {
>           cp_index = get_Java_index_u2();
>         }
>         print_field_or_method(cp_index, st);
>       }
> 
> 
> The indices have names like `cp_index`, `cpcache_index`, `indy_index` so we know exactly what we are dealing with.
> 
> Other changes:
> - Added test cases to cover both linked/unlinked classes for the bytecodes modified by this PR
> - Fixed the printing of strings to escape unprintable characters (E.g., "\u0001" is very common used by indified string concat)
> - Added a test case for `-XX:+TraceBytecodes`
> - Added a develop flag `-XX:TraceBytecodesStopAt=` so the test cases won't produce excessive output.

Changes requested by coleenp (Reviewer).

src/hotspot/share/interpreter/bytecodeTracer.cpp line 70:

> 68:   int       get_native_index_u4()    { int i=Bytes::get_native_u4(_next_pc); _next_pc+=4; return i; }
> 69:   int       get_Java_index_u2()      { int i=Bytes::get_Java_u2(_next_pc); _next_pc+=2; return i; }
> 70:   int       get_Java_index_u4()      { int i=Bytes::get_Java_u4(_next_pc); _next_pc+=4; return i; }

Since you've done the reformatting, can you add spaces around i=Bytes ? I don't really care if you add spaces around _next_pc+=x.  That looks ok to me.

src/hotspot/share/interpreter/bytecodeTracer.cpp line 372:

> 370:       {
> 371:         int cp_index;
> 372:         if (Bytecodes::uses_cp_cache(raw_code())) {

Should this be uses_cp_cache() and is_linked()?

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

PR Review: https://git.openjdk.org/jdk/pull/14623#pullrequestreview-1499246875
PR Review Comment: https://git.openjdk.org/jdk/pull/14623#discussion_r1242654046
PR Review Comment: https://git.openjdk.org/jdk/pull/14623#discussion_r1242673634