On 5/29/19 11:26 AM, Nick Gasson wrote:
> Please review this set of fixes to building hotspot with GCC 8.3 or
> Clang 7.0 on AArch64.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8224851
> Webrev: http://cr.openjdk.java.net/~ngasson/8224851/webrev.0/
>
> Ran jtreg with no new failures.
Oh dear. Experience has shown me (and probably you, too) that this is
one of the most error-prone parts of software development. Many
very serious failures have been caused by trying to shut up
compilers. These failures have even included major security breaches.
With that, let's press on with a heavy heart...
>
> This first one is with GCC 8.3:
>
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/frame_aarch64.cpp: In function
> 'void pf(long unsigned int, long unsigned int, long unsigned int, long
> unsigned int, long unsigned int)':
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/frame_aarch64.cpp:775:35: error:
> 'void* memcpy(void*, const void*, size_t)' writing to an object of type
> 'class RegisterMap' with no trivial copy-assignment; use copy-assignment or
> copy-initialization instead [-Werror=class-memaccess]
> memcpy(reg_map, &map, sizeof map);
> ^
>
> RegisterMap is non-POD because its base class AllocatedObj has virtual
> methods, so we need to use copy-assignment rather than memcpy. But
> because we're allocating reg_map with os::malloc we ought to use
> placement new to properly construct it first. But we can't do that
> because RegisterMap inherits from StackObj which disallows new. So I
> just put in some casts to void * to silence the warning.
We can't do that. It seems to me that this must be Undefined
Behaviour, and we must never attempt to cover up UB with casts.
> I can't see where `pf' and `internal_pf' are used - perhaps they can be
> called manually from the debugger?
They're debugger commands. That's why they have external C linkage.
> If no one uses them maybe they should be deleted?
Perhaps so. We have the debugger command pfl() which does all the
frame printing I find I ever need.
> The remaining warnings / errors are with Clang 7.0.
>
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/assembler_aarch64.hpp:279:22:
> error: & has lower precedence than ==; == will be evaluated first
> [-Werror,-Wparentheses]
> assert_cond(bits & mask == mask);
> ^~~~~~~~~~~~~~
>
> To preserve the behaviour we should write `bits & (mask == mask)' but I
> don't think this was what was intended so I changed it to
> `(bits & mask) == mask'.
That's right. It's supposed to check that the field being read from an
instruction has already been set.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/interp_masm_aarch64.hpp:44:25:
> error: redeclaration of using declaration
> using MacroAssembler::call_VM_leaf_base;
> ~~~~~~~~~~~~~~~~^
>
> This using declaration appears twice in a row.
OK.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/aarch64.ad:13866:80: error:
> invalid suffix 'D' on floating constant
> __ fcmps(as_FloatRegister(opnd_array(1)->reg(ra_,this,idx1)/* src1 */),
> 0.0D);
>
> ^
>
> The "d" or "D" suffix on floating point literals is not in C++98. I
> believe it comes from an extension to C to support decimal floating
> point? Anyway it's not necessary here.
OK.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/c1_LIRGenerator_aarch64.cpp:429:66:
> error: implicit conversion of NULL constant to 'bool'
> [-Werror,-Wnull-conversion]
> arithmetic_op_fpu(x->op(), reg, left.result(), right.result(), NULL);
> ~~~~~~~~~~~~~~~~~ ^~~~
> false
>
> The NULL here is for the `bool is_strictfp' argument to
> LIRGenerator::arithmetic_op_fpu. Changing it to `false' would preserve
> the existing behaviour but it seems like it should be `x->is_strictfp()'
> to match other platforms.
OK, use x->is_strictfp(). I don't think that it can possibly make any
difference on AArch64.
> As a consequence of this we need to handle the lir_{div,mul}_scriptfp
> opcodes in LIR_Assembler::arith_op, but these just fall through to the
> non-strictfp variants so there should be no behaviour change.
Yep.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp:1074:24:
> error: '&&' within '||' [-Werror,-Wlogical-op-parentheses]
> if (is_unordered && op->cond() == lir_cond_equal
> ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Added parenthesis to make this explicit. No behaviour change.
OK. I don't quite get what this is for, but I guess it shuts the
compiler up and is harmless.
> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/copy_linux_aarch64.s:162:32:
> error: index must be a multiple of 8 in range [0, 32760].
> prfm pldl1keep, [s, #-256]
> ^
>
> The immediate offset in `prfm' gets zero-extended to 64-bits and then
> left shifted three places so can only be unsigned. GNU as actually
> assembles [s, #-256] the same as [s]:
>
> 0: f9800000 prfm pldl1keep, [x0]
>
> Seems like GNU as ought to report an error here instead. I've replaced
> this with `prfum' which has a sign-extended offset.
OK.
> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:43:39:
> error: cannot initialize a parameter of type 'char *' with an lvalue of type
> 'unsigned long'
> return __sync_add_and_fetch(dest, add_value);
> ^~~~~~~~~
>
> If the add_and_fetch template is instantiated with I=integer and
> D=pointer then we need an explicit cast here.
Please don't. Please have a look at where this happens and fix the
types there, as appropriate. What do other ports do?
> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp:679:8:
> error: conflicting types for '_Copy_conjoint_jshorts_atomic'
> void _Copy_conjoint_jshorts_atomic(jshort* from, jshort* to, size_t count) {
> ^
>
> This family of functions is declared with const `from' pointer but in
> the definition it is non-const. Made it const everywhere.
OK.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp:177:13:
> error: using the result of an assignment as a condition without parentheses
> [-Werror,-Wparentheses]
> if (p = strchr(buf, ':')) {
> ~~^~~~~~~~~~~~~~~~~~
>
> Put parentheses around the argument. No functional change.
OK.
> /home/nicgas01/jdk/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp:2684:17:
> error: shifting a negative signed value is undefined
> [-Werror,-Wshift-negative-value]
> offset &= -1<<12;
> ~~^
Oh FFS. :-) This is perfectly legal in GCC and probably clang as well.
> Rewrote as ~((1<<12) - 1) which matches the masks used in the rest of
> the function.
Let's not. All we have to do to create a mask is is use -1u<<12. I
would challenge anyone to figure out without using a pencil what the
effect of &= ~(((1<<12)-1)<<12) might be.
> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp:103:20:
> error: variable 'esp' is uninitialized when used here
> [-Werror,-Wuninitialized]
> return (address) esp;
> ^~~
> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp:101:21:
> note: initialize the variable 'esp' to silence this warning
> register void *esp __asm__ (SPELL_REG_SP);
> ^
> = nullptr
>
> We could silence the -Wuninitialized warning for this function, but that
> doesn't help much because Clang then compiles
> os::current_stack_pointer() into:
>
> 0000000000000000 <_ZN2os21current_stack_pointerEv>:
> 0: d65f03c0 ret
>
> Whereas with GCC we get:
>
> 0000000000000250 <_ZN2os21current_stack_pointerEv>:
> 250: 910003e0 mov x0, sp
> 254: d65f03c0 ret
>
> Added some inline assembly to explicitly move sp into the result #ifdef
> __clang__. Same with _get_previous_fp().
This is all looking extremely fragile. We'd need to look at what the
usages of this code are and trace through.
--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
