From: <jik...@li...> - 2013-10-04 16:37:00
|
details: http://hg.code.sourceforge.net/p/jikesrvm/code/rev/5f4f4a3aa080 changeset: 10691:5f4f4a3aa080 user: Erik Brangs <eri...@gm...> date: Fri Oct 04 14:51:23 2013 +0200 description: RVM-1057 : Change uses of assertions to conform to the coding style and add exceptions for cases that the Checkstyle assertion plugin will not handle. A limitation of the plugin is that it does not track method calls. For example, it cannot recognize that a private method is always called inside of blocks guarded with VM.VerifyAssertions. details: http://hg.code.sourceforge.net/p/jikesrvm/code/rev/04fe401e7f2b changeset: 10692:04fe401e7f2b user: Erik Brangs <eri...@gm...> date: Fri Oct 04 15:53:34 2013 +0200 description: RVM-1057 : Move String concatenation out of the VM._assert(..) call for cases where the concatenation is harmless. The concatenation is harmless if the assertion will fail every time this point in the code is reached (i.e. VM.NOT_REACHED is the first argument to VM._assert(..)). details: http://hg.code.sourceforge.net/p/jikesrvm/code/rev/9608ddfc8848 changeset: 10693:9608ddfc8848 user: Erik Brangs <eri...@gm...> date: Fri Oct 04 15:55:06 2013 +0200 description: RVM-1057 : Move String concatenation out of the VM._assert(..) call for cases where the concatenation could reduce performance in builds with enabled assertions. For those cases, build the message only when the assertion will fail. details: http://hg.code.sourceforge.net/p/jikesrvm/code/rev/4e308b124fe1 changeset: 10694:4e308b124fe1 user: Erik Brangs <eri...@gm...> date: Fri Oct 04 18:11:22 2013 +0200 description: RVM-1057 : Add a Checkstyle plugin that checks our assertion coding style. diffstat: MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/Assert.java | 4 + MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/ScanThread.java | 6 + build.xml | 8 +- build/checkstyle/rvm-checks.xml | 4 + build/checkstyle/text-output-without-file.xsl | 28 + build/checkstylePlugin.xml | 79 ++ rvm/src/org/jikesrvm/classloader/Atom.java | 25 +- rvm/src/org/jikesrvm/classloader/RVMClass.java | 9 +- rvm/src/org/jikesrvm/compilers/baseline/TemplateCompilerFramework.java | 17 +- rvm/src/org/jikesrvm/compilers/baseline/ppc/BaselineCompilerImpl.java | 7 +- rvm/src/org/jikesrvm/compilers/opt/ExpressionFolding.java | 12 +- rvm/src/org/jikesrvm/compilers/opt/Simplifier.java | 22 +- rvm/src/org/jikesrvm/compilers/opt/StaticFieldReader.java | 19 +- rvm/src/org/jikesrvm/compilers/opt/bc2ir/BC2IR.java | 14 +- rvm/src/org/jikesrvm/compilers/opt/escape/SimpleEscape.java | 28 +- rvm/src/org/jikesrvm/compilers/opt/inlining/InlineTools.java | 8 +- rvm/src/org/jikesrvm/compilers/opt/ir/Instruction.java | 9 + rvm/src/org/jikesrvm/compilers/opt/lir2mir/ppc/BURS_Helpers.java | 12 +- rvm/src/org/jikesrvm/compilers/opt/mir2mc/ia32/AssemblerBase.java | 10 +- rvm/src/org/jikesrvm/compilers/opt/regalloc/ia32/CallingConvention.java | 3 +- rvm/src/org/jikesrvm/compilers/opt/runtimesupport/OptGCMap.java | 3 +- rvm/src/org/jikesrvm/compilers/opt/ssa/EnterSSA.java | 3 +- rvm/src/org/jikesrvm/compilers/opt/ssa/GlobalCSE.java | 6 +- rvm/src/org/jikesrvm/compilers/opt/ssa/LICM.java | 3 +- rvm/src/org/jikesrvm/jni/JNIEnvironment.java | 4 +- rvm/src/org/jikesrvm/jni/ia32/JNICompiler.java | 9 +- rvm/src/org/jikesrvm/osr/ExecutionState.java | 4 +- tools/checkstyle-assertion-plugin/checkstyle_packages.xml | 9 + tools/checkstyle-assertion-plugin/src/org/jikesrvm/tools/checkstyle/JikesRVMAssertionStyle.java | 310 ++++++++++ tools/checkstyle-assertion-plugin/test-file/AssertionStyleTest.expected | 16 + tools/checkstyle-assertion-plugin/test-file/AssertionStyleTest.java | 188 ++++++ 31 files changed, 797 insertions(+), 82 deletions(-) diffs (truncated from 1295 to 300 lines): diff --git a/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/Assert.java b/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/Assert.java --- a/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/Assert.java +++ b/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/Assert.java @@ -51,7 +51,9 @@ public final void _assert(boolean cond) { if (!org.mmtk.vm.VM.VERIFY_ASSERTIONS) VM.sysFail("All assertions must be guarded by VM.VERIFY_ASSERTIONS: please check the failing assertion"); + //CHECKSTYLE:OFF - Checkstyle assertion plugin would warn otherwise VM._assert(cond); + //CHECKSTYLE:ON } @Override @@ -60,7 +62,9 @@ if (!org.mmtk.vm.VM.VERIFY_ASSERTIONS) VM.sysFail("All assertions must be guarded by VM.VERIFY_ASSERTIONS: please check the failing assertion"); if (!cond) VM.sysWriteln(message); + //CHECKSTYLE:OFF - Checkstyle assertion plugin would warn otherwise VM._assert(cond); + //CHECKSTYLE:ON } @Override diff --git a/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/ScanThread.java b/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/ScanThread.java --- a/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/ScanThread.java +++ b/MMTk/ext/vm/jikesrvm/org/jikesrvm/mm/mmtk/ScanThread.java @@ -615,6 +615,11 @@ * but we can't move the native stack. */ private void assertImmovableInCurrentCollection() { + // This method is guarded by VM.VerifyAssertions. Our Checkstyle assertion + // plugin does not recognize this because it does not track calls. Therefore, + // switch off Checkstyle for this method. + + //CHECKSTYLE:OFF VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread.getStack()))); VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread))); VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread.getStack()))); @@ -624,6 +629,7 @@ VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread.getContextRegisters().gprs))); VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread.getExceptionRegisters()))); VM._assert(trace.willNotMoveInCurrentCollection(ObjectReference.fromObject(thread.getExceptionRegisters().gprs))); + //CHECKSTYLE:ON } /** diff --git a/build.xml b/build.xml --- a/build.xml +++ b/build.xml @@ -18,6 +18,7 @@ <property name="jikesrvm.dir" location="${basedir}"/> <import file="build/base.xml"/> <import file="build/tasks.xml"/> + <import file="build/checkstylePlugin.xml"/> <property name="config.mmtk" value="default"/> <property name="config.file" location="${jikesrvm.dir}/build/configs/${config.name}.properties"/> @@ -2199,8 +2200,9 @@ </if> </target> - <target name="checkstyle" depends="prepare-source,prepare-config-source,choose-classlib,ensure-checkstyle"> - <taskdef resource="checkstyletask.properties" classpath="${checkstyle.jar}"/> + <target name="checkstyle" depends="prepare-source,prepare-config-source,choose-classlib,ensure-checkstyle, + test-checkstyle-plugin"> + <taskdef resource="checkstyletask.properties" classpath="${checkstyle.jar}:${plugin.jar}"/> <mkdir dir="${build.dir}/checkstyle"/> <checkstyle config="build/checkstyle/rvm-checks.xml" failureProperty="checkstyle.failed" failOnViolation="false"> @@ -2215,7 +2217,7 @@ <fileset dir="libraryInterface/Common" includes="**/*.java"/> <fileset dir="libraryInterface/GNUClasspath/EPL" includes="**/*.java"/> <fileset dir="libraryInterface/Harmony/EPL" includes="**/*.java"/> - <fileset dir="tools" includes="**/*.java"/> + <fileset dir="tools" includes="**/src/**/*.java"/> </checkstyle> <xslt in="${build.dir}/checkstyle/report.xml" diff --git a/build/checkstyle/rvm-checks.xml b/build/checkstyle/rvm-checks.xml --- a/build/checkstyle/rvm-checks.xml +++ b/build/checkstyle/rvm-checks.xml @@ -146,6 +146,10 @@ <!--<module name="TodoComment"/>--> <module name="UpperEll"/> + <!-- Jikes RVM assertion style --> + <!-- See http://www.jikesrvm.org/Coding+Conventions --> + <module name="JikesRVMAssertionStyle"/> + </module> </module> diff --git a/build/checkstyle/text-output-without-file.xsl b/build/checkstyle/text-output-without-file.xsl new file mode 100644 --- /dev/null +++ b/build/checkstyle/text-output-without-file.xsl @@ -0,0 +1,28 @@ +<?xml version="1.0"?> +<!-- + ~ This file is part of the Jikes RVM project (http://jikesrvm.org). + ~ + ~ This file is licensed to You under the Eclipse Public License (EPL); + ~ You may not use this file except in compliance with the License. You + ~ may obtain a copy of the License at + ~ + ~ http://www.opensource.org/licenses/eclipse-1.0.php + ~ + ~ See the COPYRIGHT.txt file distributed with this work for information + ~ regarding copyright ownership. + --> +<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> +<xsl:output method="text"/> + +<xsl:template match="checkstyle"> +Checkstyle Results for Checkstyle assertion plugin test case +============================================================<xsl:for-each select="file[error]"> + +<xsl:for-each select="error"> + at line <xsl:value-of select="@line"/>,<xsl:value-of select="@column"/> : <xsl:value-of select="@message"/> +</xsl:for-each> +</xsl:for-each> + +Found <xsl:value-of select="count(file/error)"/> errors in <xsl:value-of select="count(file[error])"/> of <xsl:value-of select="count(file)"/> files. +</xsl:template> +</xsl:stylesheet> diff --git a/build/checkstylePlugin.xml b/build/checkstylePlugin.xml new file mode 100644 --- /dev/null +++ b/build/checkstylePlugin.xml @@ -0,0 +1,79 @@ +<!-- + ~ This file is part of the Jikes RVM project (http://jikesrvm.org). + ~ + ~ This file is licensed to You under the Eclipse Public License (EPL); + ~ You may not use this file except in compliance with the License. You + ~ may obtain a copy of the License at + ~ + ~ http://www.opensource.org/licenses/eclipse-1.0.php + ~ + ~ See the COPYRIGHT.txt file distributed with this work for information + ~ regarding copyright ownership. + --> +<project name="checkstylePlugin"> + + <!-- **************************************************************************** --> + <!-- * * --> + <!-- * Build the Checkstyle plugin * --> + <!-- * * --> + <!-- **************************************************************************** --> + + <property name="plugin.dir" location="${build.dir}/plugins"/> + <property name="plugin.classes" location="${plugin.dir}/classes"/> + <property name="plugin.jar.dir" location="${plugin.dir}/plugin-jar"/> + <property name="plugin.java.version" value="1.6"/> + <property name="plugin.basedir" value="${jikesrvm.dir}/tools/checkstyle-assertion-plugin"/> + <property name="plugin.src" value="${plugin.basedir}/src"/> + <property name="plugin.jar" value="${plugin.jar.dir}/jikes-rvm-assertion-style-checkstyle-plugin.jar"/> + <property name="plugin.package-config" value="${plugin.basedir}/checkstyle_packages.xml"/> + <property name="plugin.test.output.xml" value="${plugin.dir}/checkstyle-report-for-testfile.xml"/> + <property name="plugin.test.output.html" value="${plugin.dir}/checkstyle-report-for-testfile.html"/> + <property name="plugin.test.output.txt" value="${plugin.dir}/checkstyle-report-for-testfile.txt"/> + <property name="plugin.test.expected" value="${plugin.basedir}/test-file/AssertionStyleTest.expected"/> + + + <target name="compile-checkstyle-plugin"> + <mkdir dir="${plugin.classes}"/> + <javac srcdir="${plugin.src}" destdir="${plugin.classes}" debug="true" + classpath="${checkstyle.jar}" source="${plugin.java.version}" target="${plugin.java.version}" includeantruntime="false"/> + + <!-- Configure packages so that we can use a simple name instead of the fully-qualified one --> + <copy file="${plugin.package-config}" todir="${plugin.classes}"/> + + <jar destfile="${plugin.jar}" basedir="${plugin.classes}"/> + </target> + + <target name="test-checkstyle-plugin" depends="compile-checkstyle-plugin"> + <taskdef resource="checkstyletask.properties" classpath="${checkstyle.jar}:${plugin.jar}"/> + + <checkstyle config="build/checkstyle/rvm-checks.xml" failOnViolation="false"> + <formatter type="xml" tofile="${plugin.test.output.xml}"/> + <fileset dir="tools" includes="**/AssertionStyleTest.java"/> + </checkstyle> + + <xslt in="${plugin.test.output.xml}" + out="${plugin.test.output.html}" + style="${checkstyle.dir}/contrib/checkstyle-simple.xsl"/> + <xslt in="${plugin.test.output.xml}" + out="${plugin.test.output.txt}" + style="build/checkstyle/text-output-without-file.xsl"/> + + <checksum file="${plugin.test.expected}" property="plugin.test.expected.checksum"/> + <checksum file="${plugin.test.output.txt}" property="plugin.test.output.checksum"/> + + <!-- Intended for debugging + <echo message="Checksum for expected is ${plugin.test.expected.checksum}"/> + <echo message="Checksum for output is ${plugin.test.output.checksum}"/> + --> + + <checksum file="${plugin.test.output.txt}" property="${plugin.test.expected.checksum}" verifyproperty="plugin.test.ok"/> + + <!-- Intended for debugging + <loadfile property="checkstyle-plugin-test.output" srcFile="${plugin.test.output.txt}"/> + <echo message="${checkstyle-plugin-test.output}"/> + --> + + <fail unless="${plugin.test.ok}" message="Tests for the Checkstyle Jikes RVM assertion style plugin failed (checksum mismatch)!"/> + </target> + +</project> diff --git a/rvm/src/org/jikesrvm/classloader/Atom.java b/rvm/src/org/jikesrvm/classloader/Atom.java --- a/rvm/src/org/jikesrvm/classloader/Atom.java +++ b/rvm/src/org/jikesrvm/classloader/Atom.java @@ -492,12 +492,9 @@ return TypeReference.findOrCreate(cl, findOrCreate(val, i, val.length - i, toUnicodeStringInternal())); default: if (VM.VerifyAssertions) { - VM._assert(VM.NOT_REACHED, - "Need a valid method descriptor; got \"" + - this + - "\"; can't parse the character '" + - ((char)val[i]) + - "'"); + String msg = "Need a valid method descriptor; got \"" + this + + "\"; can't parse the character '" + ((char)val[i]) + "'"; + VM._assert(VM.NOT_REACHED, msg); } return null; // NOTREACHED } @@ -578,13 +575,9 @@ default: if (VM.VerifyAssertions) { - VM._assert(VM.NOT_REACHED, - "The class descriptor \"" + - this + - "\" contains the illegal" + - " character '" + - ((char)val[i]) + - "'"); + String msg = "The class descriptor \"" + this + "\" contains the illegal" + + " character '" + ((char)val[i]) + "'"; + VM._assert(VM.NOT_REACHED, msg); } } } @@ -837,8 +830,12 @@ @Pure public String annotationClassToAnnotationInterface() { if (VM.VerifyAssertions) { + boolean isClassAnnotation = val[0] == 'L' && val[val.length - 1] == ';'; VM._assert(val.length > 0); - VM._assert(val[0] == 'L' && val[val.length - 1] == ';', toString()); + if (!isClassAnnotation) { + String msg = toString(); + VM._assert(isClassAnnotation, msg); + } } return StringUtilities.asciiBytesToString(val, 1, val.length - 4).replace('/', '.'); } diff --git a/rvm/src/org/jikesrvm/classloader/RVMClass.java b/rvm/src/org/jikesrvm/classloader/RVMClass.java --- a/rvm/src/org/jikesrvm/classloader/RVMClass.java +++ b/rvm/src/org/jikesrvm/classloader/RVMClass.java @@ -732,7 +732,14 @@ */ @Pure public RVMMethod[] getConstructorMethods() { - if (VM.VerifyAssertions) VM._assert(isResolved(), "Error class " + this + " is not resolved but " + state); + if (VM.VerifyAssertions) { + boolean isResolved = isResolved(); + if (!isResolved) { + String msg = "Error class " + this + " is not resolved but " + state; + VM._assert(VM.NOT_REACHED, msg); + } + } + return constructorMethods; } diff --git a/rvm/src/org/jikesrvm/compilers/baseline/TemplateCompilerFramework.java b/rvm/src/org/jikesrvm/compilers/baseline/TemplateCompilerFramework.java --- a/rvm/src/org/jikesrvm/compilers/baseline/TemplateCompilerFramework.java +++ b/rvm/src/org/jikesrvm/compilers/baseline/TemplateCompilerFramework.java @@ -154,8 +154,11 @@ // uninterruptible // TODO: remove logically uninterruptible annotations (see RVM-115). if (VM.VerifyAssertions && method.hasLogicallyUninterruptibleAnnotation()) { - VM._assert(isUninterruptible, "LogicallyUninterruptible but not Uninterruptible method: ", - method.toString()); + if (!isUninterruptible) { + String msg = "LogicallyUninterruptible but not Uninterruptible method: " + + method.toString(); + VM._assert(VM.NOT_REACHED, msg); + } } } @@ -1620,13 +1623,9 @@ if (VM.VerifyUnint && !isInterruptible) forbiddenBytecode("anewarray ", arrayRef, bcodes.index()); if (VM.VerifyAssertions && elementTypeRef.isUnboxedType()) { - VM._assert(VM.NOT_REACHED, |