Linkage Checker to verify method references to JRE classes #1607

suztomo · 2020-08-19T01:32:36Z

Fixes #1605 .

suztomo · 2020-08-19T04:05:26Z

dependencies/src/main/resources/linkage-checker-exclusion-default.xml

+  <LinkageError>
+    <Target>
+      <Class name="java.lang.invoke.MethodHandle" />
+    </Target>
+    <Source>
+      <Package name="com.oracle.svm" />
+    </Source>
+    <Reason>
+      GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
+      only exists in special JDK. These missing classes are false positives, because
+      the code is only invoked when running in a GraalVM.
+      https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
+    </Reason>
+  </LinkageError>
+  <LinkageError>
+    <Target>
+      <Class name="java.lang.invoke.MethodHandle" />
+    </Target>
+    <Source>
+      <Package name="com.oracle.graal" />
+    </Source>
+    <Reason>
+      GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
+      only exists in special JDK. These missing classes are false positives, because
+      the code is only invoked when running in a GraalVM.
+      https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
+    </Reason>
+  </LinkageError>
+  <LinkageError>
+    <Target>
+      <Class name="java.lang.invoke.MethodHandle" />
+    </Target>
+    <Source>
+      <Package name="org.graalvm" />
+    </Source>
+    <Reason>
+      GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
+      only exists in special JDK. These missing classes are false positives, because
+      the code is only invoked when running in a GraalVM.
+      https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
+    </Reason>
+  </LinkageError>
+  <LinkageError>
+    <Target>
+      <Class name="java.lang.invoke.MethodHandle" />
+    </Target>
+    <Source>
+      <Package name="com.oracle.truffle" />
+    </Source>
+    <Reason>
+      GraalVM-related libraries depend on Java Compiler Interface (JVMCI) that
+      only exists in special JDK. These missing classes are false positives, because
+      the code is only invoked when running in a GraalVM.
+      https://github.com/GoogleCloudPlatform/cloud-opensource-java/issues/929
+    </Reason>
+  </LinkageError>


Because now we check JDK classes, GraalVM-related exclusion is needed.

suztomo · 2020-08-19T04:05:39Z

dependencies/src/test/java/com/google/cloud/tools/opensource/classpath/LinkageCheckerTest.java

+  @Test
+  public void testFindLinkageProblems_referenceToNonexistentMethodForJava8() throws IOException {
+    // Protobuf-java 3.12.4 contained wrong byte code to reference non-existent method in JRE 1.8
+    // https://github.com/protocolbuffers/protobuf/issues/7827


This is Kristen's finding yesterday.

suztomo · 2020-08-19T04:07:38Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java

@@ -239,8 +239,14 @@ private boolean problemFilter(LinkageProblem linkageProblem) {
    String targetClassName = symbol.getClassBinaryName();
    String methodName = symbol.getName();

-    // Skip references to Java runtime class. For example, java.lang.String.
-    if (classDumper.isSystemClass(targetClassName)) {


This was hiding the protobuf's linkage error against Java 8 runtime protocolbuffers/protobuf#7827

elharo · 2020-08-19T12:27:47Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java

+      return Optional.empty();
+    }
+    // Skip references from Java runtime class. For example, java.lang.String.
+    // It is possible for wrongly configured Java compiler to generate bad byte code that references


I'm not sure lines 246-248 explain this code. It explains what we're not doing, not what we are. I suggest deleting them.

Removed this if-statement and moved the explanation to array class skipping.

elharo · 2020-08-19T12:29:18Z

dependencies/src/test/java/com/google/cloud/tools/opensource/classpath/LinkageCheckerTest.java

+
+  @Test
+  public void testFindLinkageProblems_referenceToNonexistentMethodForJava8() throws IOException {
+    // Protobuf-java 3.12.4 contained wrong byte code to reference non-existent method in JRE 1.8


protobuf-java 3.12.4 references a Java 11 method that does not exist in Java 8

elharo

This is a really interesting one. Some take-aways:

The compiler doesn't catch all linkage errors, even to classes in the JDK.
We should report errors that the project (here Kristen) can't fix so they can understand them and report the errors to the team that can fix them.

suztomo

@elharo PTAL.

suztomo · 2020-08-19T12:52:52Z

dependencies/src/test/java/com/google/cloud/tools/opensource/classpath/LinkageCheckerTest.java

+
+  @Test
+  public void testFindLinkageProblems_referenceToNonexistentMethodForJava8() throws IOException {
+    // Protobuf-java 3.12.4 contained wrong byte code to reference non-existent method in JRE 1.8


suztomo · 2020-08-19T12:53:27Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java

+      return Optional.empty();
+    }
+    // Skip references from Java runtime class. For example, java.lang.String.
+    // It is possible for wrongly configured Java compiler to generate bad byte code that references


Removed this if-statement and moved the explanation to array class skipping.

elharo

needs merge with master

elharo · 2020-08-19T18:22:59Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/ClassDumper.java

@@ -126,8 +126,7 @@ JavaClass loadJavaClass(String className) throws ClassNotFoundException {
  /** Returns true if {@code className} is available in the system class loader. */
  boolean isSystemClass(String className) {
    try {
-      if (className.startsWith("[")) {
-        // Array class
+      if (isArrayClass(className)) {


will we catch a reference to an array of a missing class?

Yes, but not from array class reference. When there's a reference to array of a class, there should be a reference to the class in the same class file.

elharo · 2020-08-19T18:23:58Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java

-    // Skip references to Java runtime class. For example, java.lang.String.
-    if (classDumper.isSystemClass(targetClassName)) {
+    if (ClassDumper.isArrayClass(targetClassName)) {
+      // Skip references to array class. However, we want to check other JDK-provided classes,


delete this comment. It describes what the code is not doing.

suztomo

@elharo PTAL.

suztomo · 2020-08-19T18:34:05Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/ClassDumper.java

@@ -126,8 +126,7 @@ JavaClass loadJavaClass(String className) throws ClassNotFoundException {
  /** Returns true if {@code className} is available in the system class loader. */
  boolean isSystemClass(String className) {
    try {
-      if (className.startsWith("[")) {
-        // Array class
+      if (isArrayClass(className)) {


Yes, but not from array class reference. When there's a reference to array of a class, there should be a reference to the class in the same class file.

suztomo · 2020-08-19T18:34:44Z

dependencies/src/main/java/com/google/cloud/tools/opensource/classpath/LinkageChecker.java

-    // Skip references to Java runtime class. For example, java.lang.String.
-    if (classDumper.isSystemClass(targetClassName)) {
+    if (ClassDumper.isArrayClass(targetClassName)) {
+      // Skip references to array class. However, we want to check other JDK-provided classes,


elharo

Merge to master please

suztomo added 2 commits August 18, 2020 21:12

Validate system class in target

ab53f2e

GraalVM references in exclusion rule

a8a1571

google-cla bot added the cla: yes label Aug 19, 2020

suztomo commented Aug 19, 2020

View reviewed changes

suztomo requested a review from elharo August 19, 2020 04:07

elharo reviewed Aug 19, 2020

View reviewed changes

Applied review

a36b452

suztomo commented Aug 19, 2020

View reviewed changes

elharo reviewed Aug 19, 2020

View reviewed changes

suztomo added 2 commits August 19, 2020 14:34

Removing irrelevant comment

f0f81dc

Merge remote-tracking branch 'origin/master' into protobuf-3.12.4

8199078

suztomo commented Aug 19, 2020

View reviewed changes

suztomo mentioned this pull request Aug 19, 2020

Can Linkage Checker detect the bad Java8-incompatible byte code #1605

Closed

elharo reviewed Aug 24, 2020

View reviewed changes

suztomo added 4 commits August 24, 2020 08:13

Merge branch 'master' into protobuf-3.12.4

8a9dca3

Merge branch 'master' into protobuf-3.12.4

baeaa93

Merge branch 'master' into protobuf-3.12.4

82a0837

Merge branch 'master' into protobuf-3.12.4

b494064

elharo approved these changes Aug 25, 2020

View reviewed changes

Merge branch 'master' into protobuf-3.12.4

12c0fe1

suztomo merged commit 354bfee into master Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linkage Checker to verify method references to JRE classes #1607

Linkage Checker to verify method references to JRE classes #1607

suztomo commented Aug 19, 2020

suztomo Aug 19, 2020

suztomo Aug 19, 2020

suztomo Aug 19, 2020

elharo Aug 19, 2020

suztomo Aug 19, 2020

elharo Aug 19, 2020

suztomo Aug 19, 2020

elharo left a comment

suztomo left a comment

suztomo Aug 19, 2020

suztomo Aug 19, 2020

elharo left a comment

elharo Aug 19, 2020

suztomo Aug 19, 2020

elharo Aug 19, 2020

suztomo Aug 19, 2020

suztomo left a comment

suztomo Aug 19, 2020

suztomo Aug 19, 2020

elharo left a comment

Linkage Checker to verify method references to JRE classes #1607

Linkage Checker to verify method references to JRE classes #1607

Conversation

suztomo commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

suztomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suztomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment