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

Thu Jun 29 16:10:02 UTC 2017

Thanks for changing this!
Looks good to me.
/Jesper

> On 29 Jun 2017, at 15:31, Hohensee, Paul <hohensee at amazon.com> wrote:
> 
> I now have access to cr.openjdk.java.net, so the latest webrevs are at
> 
> http://cr.openjdk.java.net/~phh/8182299/webrev_jdk.00/
> http://cr.openjdk.java.net/~phh/8182299/webrev_hotspot.00/
> 
> On 6/28/17, 3:50 PM, "hotspot-dev on behalf of Hohensee, Paul" <hotspot-dev-bounces at openjdk.java.net on behalf of hohensee at amazon.com> wrote:
> 
>    Thanks for the review, Jesper.
> 
>    New webrev sent, has only a change to nativeInst_x86.cpp.
> 
>    In nativeInst_x86.cpp, I formatted the expression so I could easily understand it, but you’re right, not everyone does it that way (maybe only me!), so I’ve changed it to
> 
>      if (((ubyte_at(0) & NativeTstRegMem::instruction_rex_prefix_mask) == NativeTstRegMem::instruction_rex_prefix &&
>           ubyte_at(1) == NativeTstRegMem::instruction_code_memXregl &&
>           (ubyte_at(2) & NativeTstRegMem::modrm_mask) == NativeTstRegMem::modrm_reg) ||
>          (ubyte_at(0) == NativeTstRegMem::instruction_code_memXregl &&
>           (ubyte_at(1) & NativeTstRegMem::modrm_mask) == NativeTstRegMem::modrm_reg)) {
> 
>    In graphKit.cpp, the old code was
> 
>    #ifdef ASSERT
>      case Deoptimization::Action_none:
>      case Deoptimization::Action_make_not_compilable:
>        break;
>      default:
>        fatal("unknown action %d: %s", action, Deoptimization::trap_action_name(action));
>        break;
>    #endif
> 
>    In the non-ASSERT case, the compiler complained about the lack of Action_none, Action_make_not_compilable and default. If the warning had been turned off, the result would have been ‘break;’ for all three. In the ASSERT case, Action_none and Action_make_not_compilable result in ‘break;’, and in the default case ‘fatal(); break;’
> 
>    The new code is
> 
>      case Deoptimization::Action_none:
>      case Deoptimization::Action_make_not_compilable:
>        break;
>      default:
>    #ifdef ASSERT
>        fatal("unknown action %d: %s", action, Deoptimization::trap_action_name(action));
>    #endif
>        break;
> 
>    The compiler doesn’t complain about Action_none, Action_make_not_compilable or default anymore. In the non-ASSERT case, the result is ‘break;’ for all three, same as for the old code. In the ASSERT case, Action_none and Action_make_not_compilable result in ‘break;’, and in the default case ‘fatal(); break;’, again same as for the old code.
> 
>    Thanks,
> 
>    Paul
> 
>    On 6/28/17, 10:36 AM, "jesper.wilhelmsson at oracle.com" <jesper.wilhelmsson at oracle.com> wrote:
> 
>        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.
> 
> 
> 
> 
> 