RFR(XL): 8182299: Enable disabled clang warnings, build on OSX 10 + Xcode 8
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu Jul 6 13:27:56 UTC 2017
Paul,
> http://cr.openjdk.java.net/~phh/8182299/webrev_hotspot.02/
Compiler changes look good.
Just a small refactoring suggestion while you on it:
src/cpu/x86/vm/macroAssembler_x86.cpp:
0x%016lx can be replaced with INTPTR_FORMAT.
Thanks for taking care of the cleanup and welcome back! :-)
Best regards,
Vladimir Ivanov
>
> Jesper says he’s ready to push the closed changes. Any other comments on the open ones?
>
> Thanks,
>
> Paul
>
> On 7/3/17, 12:58 AM, "Erik Helin" <erik.helin at oracle.com> wrote:
>
> On 06/30/2017 06:30 PM, Hohensee, Paul wrote:
> > Thanks for picking up the closed work, Jesper, and thanks for the review, Erik. It’s good to be back working on server Java. (
> >
> > Yes, I added the G1RootClosures virtual destructor because of the warning.
> >
> > In heapRegionType.cpp, I just followed along with the existing funky indentation and made the default clause also have a return because all the other clauses do. Clang would indeed be ok with a default clause with just a break. Also, I’ve used the same idiom in other files, so I’d change those too if I change these. Which would you prefer? I’m not invested in any particular format.
>
> I don't know about the other files, but given the indentation in
> heapRegionType.cpp, I would prefer:
>
> switch (_tag) {
> // ... bunch of case statements
> case ArchiveTag: return "ARC";
> default: break;
> }
> ShouldNotReachHere();
> return NULL; // just to keep some compilers happy
> }
>
> My reason for this is mainly that all the other case statements are on
> one line (`case` followed by `return`), so I would prefer to the
> `default` case to also be on one line (`default` followed by `break`).
>
> However, for in HeapRegionType::is_valid, I think your changes follows
> the existing style in that method:
>
> 37 case ArchiveTag:
> 38 return true;
> 39 default:
> 40 return false;
> 41 }
>
> I feel like this is quickly approaching bikeshed territory, but the
> above is at least my preference :)
>
> Thanks,
> Erik
>
> > Thanks,
> >
> > Paul
> >
> > On 6/30/17, 3:00 AM, "jesper.wilhelmsson at oracle.com" <jesper.wilhelmsson at oracle.com> wrote:
> >
> > 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