RFR(XL): 8182299: Enable disabled clang warnings, build on OSX 10 + Xcode 8

[jesper.wilhelmsson at oracle.com]

Hi Paul,

Thanks for doing this change! In general everything looks really good, there are a lot of really nice cleanups here. I just have two minor questions/nits:

* In hotspot/cpu/x86/vm/nativeInst_x86.hpp it seems the expression already have parenthesis around the & operations and the change here is "only" cleaning up the layout of the code which is not a bad thing in it self, but you move the logical operators to the beginning of each line which is a quite different style than the rest of the code in the same function where the operators are at the end of the line.

* In hotspot/share/vm/opto/graphKit.cpp you moved the #ifdef ASSERT so that Action_none and Action_make_not_compilable are available also when ASSERT is not defined. I don't see this mentioned in your description of the change. Was this change intentional?

Thanks,
/Jesper


> On 27 Jun 2017, at 21:34, Hohensee, Paul <hohensee at amazon.com> wrote:
> 
> An attempt at better formatting.
> 
> https://bugs.openjdk.java.net/browse/JDK-8182299
> http://cr.openjdk.java.net/~jwilhelm/8182299/webrev_jdk.00/
> http://cr.openjdk.java.net/~jwilhelm/8182299/webrev_hotspot.00/
> 
> Jesper has been kind enough to host the webrevs while I get my cr.openjdk.net account set up, and to be the sponsor.
> 
> This rfe a combination of enabling disabled clang warnings and getting jdk10 to build on OSX 10 and Xcode 8. At least one enabled warning (delete-non-virtual-dtor) detected what seems to me a real potential bug, with the rest enforcing good code hygiene.
> 
> These changes are only in OpenJDK, so I’m looking for a volunteer to make the closed changes.
> 
> Thanks,
> 
> Paul
> 
> 
> Here are the jdk file-specific details:
> 
> java_md_macosx.c splashscreen_sys.m
> 
>        Removed objc_registerThreadWithCollector() since it's obsolete and of questionable value in any case.
> 
> NSApplicationAWT.m
> 
>       Use the correct NSEventMask rather than NSUInteger.
> 
> jdhuff.c jdphuff.c
> 
>       Shifting a negative signed value is undefined.
> 
> Here are the hotspot notes:
> 
> Here are the lists of files affected by enabling a given warning:
> 
> switch: all of these are lack of a default clause
> 
>        c1_LIRAssembler_x86.cpp c1_LIRGenerator_x86.cpp c1_LinearScan_x86.hpp
>        jniFastGetField_x86_64.cpp assembler.cpp c1_Canonicalizer.cpp
>        c1_GraphBuilder.cpp c1_Instruction.cpp c1_LIR.cpp c1_LIRGenerator.cpp
>        c1_LinearScan.cpp c1_ValueStack.hpp c1_ValueType.cpp
>        bcEscapeAnalyzer.cpp ciArray.cpp ciEnv.cpp ciInstance.cpp ciMethod.cpp
>        ciMethodBlocks.cpp ciMethodData.cpp ciTypeFlow.cpp
>        compiledMethod.cpp dependencies.cpp nmethod.cpp compileTask.hpp
>        heapRegionType.cpp abstractInterpreter.cpp bytecodes.cpp
>        invocationCounter.cpp linkResolver.cpp rewriter.cpp jvmciCompilerToVM.cpp
>        jvmciEnv.cpp universe.cpp cpCache.cpp generateOopMap.cpp
>        method.cpp methodData.cpp compile.cpp connode.cpp gcm.cpp graphKit.cpp
>        ifnode.cpp library_call.cpp memnode.cpp parse1.cpp
>        parse2.cpp phaseX.cpp superword.cpp type.cpp vectornode.cpp
>        jvmtiClassFileReconstituter.cpp jvmtiEnter.xsl jvmtiEventController.cpp
>        jvmtiImpl.cpp jvmtiRedefineClasses.cpp methodComparator.cpp methodHandles.cpp
>        advancedThresholdPolicy.cpp reflection.cpp relocator.cpp sharedRuntime.cpp
>        simpleThresholdPolicy.cpp writeableFlags.cpp globalDefinitions.hpp
> 
> delete-non-virtual-dtor: these may be real latent bugs due to possible failure to execute destructor(s)
> 
>        decoder_aix.hpp decoder_machO.hpp classLoader.hpp g1RootClosures.hpp
>        jvmtiImpl.hpp perfData.hpp decoder.hpp decoder_elf.hpp
> 
> dynamic-class-memaccess: obscure use of memcpy
> 
>        method.cpp
> 
> empty-body: ‘;’ isn’t good enough for clang, it prefers {}
> 
>        objectMonitor.cpp mallocSiteTable.cpp
> 
> format: matches printf format strings against arguments. debug output will be affected by
>               incorrect code changes to these.
> 
>        macroAssembler_x86.cpp os_bsd.cpp os_bsd_x86.cpp ciMethodData.cpp javaClasses.cpp
>        debugInfo.cpp logFileOutput.cpp constantPool.cpp jvmtiEnter.xsl jvmtiRedefineClasses.cpp
>        safepoint.cpp thread.cpp
> 
> logical-op-parentheses: can be tricky to get correct. There are a few very long-winded predicates.
> 
>        nativeInst_x86.hpp archDesc.cpp output_c.cpp output_h.cpp c1_GraphBuilder.cpp
>        c1_LIRGenerator.cpp c1_LinearScan.cpp bcEscapeAnalyzer.cpp ciMethod.cpp
>        stackMapTableFormat.hpp compressedStream.cpp dependencies.cpp heapRegion.cpp
>        ptrQueue.cpp psPromotionManager.cpp jvmciCompilerToVM.cpp cfgnode.cpp
>        chaitin.cpp compile.cpp compile.hpp escape.cpp graphKit.cpp lcm.cpp
>        loopTransform.cpp loopnode.cpp loopopts.cpp macro.cpp memnode.cpp
>        output.cpp parse1.cpp parseHelper.cpp reg_split.cpp superword.cpp
>        superword.hpp jniCheck.cpp jvmtiEventController.cpp arguments.cpp
>        javaCalls.cpp sharedRuntime.cpp
> 
> parentheses
> 
>        adlparse.cpp
> 
> parentheses-equality
> 
>        output_c.cpp javaAssertions.cpp gcm.cpp
> 
> File-specific details:
> 
>        GensrcAdlc.gmk CompileJvm.gmk
>                Left tautological-compare in place to allow null 'this' pointer checks in methods
>                intended to be called from a debugger.
> 
>        CompileGTest.gmk
>                Just an enhanced comment.
> 
>        MacosxDebuggerLocal.m
>               PT_ATTACH has been replaced by PT_ATTACHEXC
> 
>        ciMethodData.cp
>               " 0x%" FORMAT64_MODIFIER "x"  reduces to  "0x%llx", whereas
>               " " INTPTRNZ_FORMAT  reduces to  "0x%lx", which latter is what clang want.
> 
>        generateOopMap.cpp
>               Refactored duplicate code in print_current_state().
> 
>        binaryTreeDictionary.cpp/hpp, hashtable.cpp/hpp
>                These provoked “instantiation of variable <static class variable> required here,
>                but no definition is available”.
> 
>        globalDefinitions_gcc.hpp
>                Define FORMAT64_MODIFIER properly for Apple, needed by os.cpp.
> 
>        globalDefinitions.hpp
>               Add INTPTRNZ_FORMAT, needed by ciMethodData.cpp.