RFR 8217647: [backport] JFR: recordings on 32-bit systems unreadable

[Andrew Hughes]

On 16/06/2020 04:44, Denghui Dong wrote:
> Could I have a review of this backport?
> 
> Original bug:
>   https://bugs.openjdk.java.net/browse/JDK-8217647
> 
> Original fix from 11u:
>   http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/df5487678893
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8217647/webrev.00/
> 
> Backport reasons:
> 1. Fix the unreadable problem on 32-bit in 8u
> 2. Make subsequent backport more smoothly
> 
> The patch cannot apply to 8u cleanly, there are two rejection part and one build failure:
> 
>  a) Conflict in jfrRecorderService.cpp. Patch wants this:
> 
> -static intptr_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
> -  const intptr_t prev_cp_offset = cw.previous_checkpoint_offset();
> -  const intptr_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
> +static int64_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
> +  const int64_t prev_cp_offset = cw.previous_checkpoint_offset();
> +  const int64_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
>    cw.reserve(sizeof(u4));
>    cw.write<u8>(EVENT_CHECKPOINT);
>    cw.write(JfrTicks::now());
> -  cw.write<jlong>((jlong)0);
> +  cw.write((int64_t)0);
>    cw.write(prev_cp_relative_offset); // write previous checkpoint offset delta
>    cw.write<bool>(false); // flushpoint
> -  cw.write<u4>((u4)1); // nof types in this checkpoint
> -  cw.write<u8>(type_id);
> -  const intptr_t number_of_elements_offset = cw.current_offset();
> +  cw.write((u4)1); // nof types in this checkpoint
> +  cw.write(type_id);
> +  const int64_t number_of_elements_offset = cw.current_offset();
>    cw.reserve(sizeof(u4));
>    return number_of_elements_offset;
>  }
> 
>  ... but the 8u code is:
> 
> static intptr_t write_checkpoint_event_prologue(JfrChunkWriter& cw, u8 type_id) {
>   const intptr_t prev_cp_offset = cw.previous_checkpoint_offset();
>   const intptr_t prev_cp_relative_offset = 0 == prev_cp_offset ? 0 : prev_cp_offset - cw.current_offset();
>   cw.reserve(sizeof(u4));
>   cw.write<u8>(EVENT_CHECKPOINT);
>   cw.write(JfrTicks::now());
>   cw.write<jlong>((jlong)0);
>   cw.write<jlong>((jlong)prev_cp_relative_offset); // write previous checkpoint offset delta
>   cw.write<bool>(false); // flushpoint
>   cw.write<u4>((u4)1); // nof types in this checkpoint
>   cw.write<u8>(type_id);
>   const intptr_t number_of_elements_offset = cw.current_offset();
>   cw.reserve(sizeof(u4));
>   return number_of_elements_offset;
> }
> 
>  ... I did a manual adjustment: 
>     cw.write<jlong>((jlong)prev_cp_relative_offset); => cw.write(prev_cp_relative_offset);
> 
>  b) Conflict in jfrWriterHost.inline.hpp. Patch wants this:
> 
>  template <typename BE, typename IE, typename WriterPolicyImpl>
>  inline void WriterHost<BE, IE, WriterPolicyImpl>::write(double value) {
> -  be_write(*(uintptr_t*)&(value));
> +  be_write(*(u8*)&(value));
>  }
> 
>  ... but the 8u code is:
> 
> template <typename BE, typename IE, typename WriterPolicyImpl>
> inline void WriterHost<BE, IE, WriterPolicyImpl>::write(double value) {
>   be_write(*(u8*)&(value));
> }
> 
>  ... so I skipped it.
> 
>  c) Build failure. Patch wants this in jfrRepository.cpp:
> 
> +            log_info(jfr) ( // For user, should not be "jfr, system"
> +              "Unable to recover JFR data");
> +            break;
> 
>  ... but 8u didn't support unified logging, so I added "if (LogJFR) tty->print_cr("Unable to recover JFR data");"
> 
> 
>  d) Other Problem: the original patch(http://hg.openjdk.java.net/jdk/jdk/rev/0abec72a3ac2) also modified jfrChunkRotation.hpp、jfrChunkRotation.cpp and jfrJniMethod.cpp, but the backport patch for 11u ignored them(http://mail.openjdk.java.net/pipermail/jdk-updates-dev/2019-April/000921.html)
> 
> Thanks,
> Denghui Dong
> 

Thanks for the comprehensive analysis. It's really helpful in reviewing
this.

The changes in the patch look fine. I see you also omitted the changes
not in 11u. Was this intentional? I compared with the 11u version, so I
didn't notice this in reviewing the patch itself.

Incidentally, we haven't been building JFR on x86 so far, because it
crashed when I tried to build it:

/builddir/build/BUILD/java-1.8.0-openjdk-1.8.0.262.b02-0.2.ea.el7.i386/openjdk/build/jdk8.build-debug/images/j2sdk-image/bin/javac
-d . /builddir/build/SOURCES/TestCryptoLevel.java
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc:  SuppressErrorAt=/os.cpp:1281
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error
(/builddir/build/BUILD/java-1.8.0-openjdk-1.8.0.262.b02-0.2.ea.el7.i386/openjdk/hotspot/src/share/vm/runtime/os.cpp:1281),
pid=93916, tid=0xf6431b40
#  assert(SerializePageShiftCount == count) failed: thread size changed,
fix SerializePageShiftCount constant

TestCryptoLevel.java is a simple Java test we run on the just-built JDK
to check unlimited crypto is in use:

https://src.fedoraproject.org/rpms/java-1.8.0-openjdk/blob/master/f/TestCryptoLevel.java

I assumed from this that x86 wasn't supported, but if it should be
working, maybe we should look into this more.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222