RFR(XL): 8182299: Enable disabled clang warnings, build on OSX 10 + Xcode 8
Hohensee, Paul
hohensee at amazon.com
Wed Jun 28 22:50:33 UTC 2017
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.
More information about the hotspot-dev
mailing list