Hi Boris,
On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
>
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
The AArch64 error was simply a missing semi-colon. With that corrected
AArch64 now builds and runs as expected (i.e. it fails the PMem support
test with an UnsupportedOperationException).
The second error is happening because the calling method
MacroAssembler::cache_wb has not been guarded with #ifdef _LP64 (the
same applies for MacroAssembler::cache_wbsync). Note that cache line
writeback via Unsafe.writeBackMemory is only expected to work on Intel
x86_64 so these two methods only get called from x86_64-specific code
(x86_64.ad and stuGenerator_x86_64.cpp).
So, the solution to this specific problem is to add #ifdef _LP64 around
the declaration and implementation of these two methods. At the same
time it would be helpful to remove the redundant #ifdef _LP64/#endif
that I for some strange reason inserted around the definitions, but not
the declarations, of clflushopt and clwb (that didn't help when I was
trying to work out what was going wrong).
However, a related problem also needs fixing. The Java code for method
Unsafe.writebackMemory only proceeds when the data cache line writeback
unit size (value of field UnsafeConstants.DATA_CACHE_LINE_FLUSH_SIZE) is
non-zero. Otherwise it throws an exception. On x86_32 that field /must/
be zero. The native methods which Unsafe calls out to and the intrinsics
which replace the native calls are not implemented on x86_32.
The field from which the value of the Java constant is derived is
currently initialised using CPU-specific information in
vm_version_x86.cpp as follows
if (os::supports_map_sync()) {
// publish data cache line flush size to generic field, otherwise
// let if default to zero thereby disabling writeback
_data_cache_line_flush_size =
_cpuid_info.std_cpuid1_ebx.bits.clflush_size * 8;
}
i.e. writeback is enabled on x86 when the operating is known to be
capable of supporting MAP_SYNC. os_linux.cpp returns true for that call,
irrespective of whether this is 32 or 64 bit linux. The rationale is
that any Linux is capable of supporting map_sync (by contrast Windows,
Solaris, AIX etc currently return false). So, the above assignment also
needs to be guarded by #ifdef _LP64 in order to ensure that writeback is
never attempted on x86_32.
Thank you for spotting these errors. I will add the relevant fixes to
the next patch and add you as a reviewer.
regards,
Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
