Not a review, just briefly jumping in because I noticed one thing to correct.
> On May 29, 2019, at 9:12 AM, Andrew Haley <a...@redhat.com> wrote: > > On 5/29/19 11:26 AM, Nick Gasson wrote: > > 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… +1 on all that. >> 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 think the solution here is to use placement new, but explicitly qualified to actually get there, e.g. ::new (reg_map) RegisterMap(map); Using unqualified new will do the lookup in RegisterMap, find some declarations in StackObj (but not the one we want) and fail. This is a pretty common pattern. >> /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. Yeah, I sometimes think we should disable -Wshift-negative-value.
