RFR(XL): 8182299: Enable disabled clang warnings, build on OSX 10 + Xcode 8
jesper.wilhelmsson at oracle.com
jesper.wilhelmsson at oracle.com
Fri Jun 30 10:00:50 UTC 2017
I ran the change through JPRT so at least it builds and runs on all platforms we support there.
I'm currently fixing our closed code to build on Mac with this change.
/Jesper
> On 30 Jun 2017, at 10:35, Erik Helin <erik.helin at oracle.com> wrote:
>
> Hi Paul,
>
> thanks for contributing! Please see my comments regarding the GC changes below.
>
> On 06/29/2017 03:31 PM, Hohensee, Paul 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/
>
> gc/g1/g1RootClosures.cpp:
> - // Closures to process raw oops in the root set.
> + virtual ~G1RootClosures() {}
> +
> +// Closures to process raw oops in the root set.
>
> I assume this is added because there is some warning about having only pure virtual methods but not having a virtual destructor. None of the classes inheriting from G1RootClosures needs a destructor, nor does G1RootClosures itself (it is just an "interface"). So there is no problem with correctness here :)
>
> However, I like to have lots of warnings enabled, so for me it is fine to an empty virtual destructor here just to please the compiler.
>
> gc/g1/heapRegion.cpp: looks correct
>
> gc/g1/heapRegionType.cpp:
> The indentation seems a bit funky to me here :) Also, have you compiled this on some other platforms? I think the last return is outside of the switch just to, as the comment says, "keep some compilers happy".
>
> Would clang be ok with having an empty default clause with just a break? And then fall through to the return outside of the switch?
>
> gc/g1/ptrQueue.cpp: looks correct
>
> gc/parallel/psPromotionManager.cpp: looks correct
>
> Thanks,
> Erik
>
>> 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.
>>
>>
>>
>>
>>
More information about the hotspot-dev
mailing list