NullPointer during IR Conversion in R8 [162568140]

Fixed

Bug

Status Update

No update yet.

Description

ma...@gmail.com

created issue #1

Jul 31, 2020 05:58PM

Using various versions of R8 I can consistently reproduce this NullPointerException encountered while processing valid bytecode.

Building my project with AGP v4.0.1 (R8 v2.0.88) I get the following stack trace:
Caused by: java.lang.NullPointerException
at com.android.tools.r8.ir.optimize.g0.a(:52)
at com.android.tools.r8.ir.conversion.O.a(:486)
at com.android.tools.r8.ir.conversion.O.a(:417)
at com.android.tools.r8.ir.conversion.O.b(:53)
... 8 more

This error occurs during processing processing
Error in inputs/3.jar:kotlinx/coroutines/internal/MainDispatcherLoader.class at Lkotlinx/coroutines/internal/MainDispatcherLoader;loadMainDispatcher()Lkotlinx/coroutines/MainCoroutineDispatcher;

I have uploaded the compiled jars and a script for running r8 that will reproduce this issue.

https://drive.google.com/file/d/1uX9Ueb2W8TnQBw1v_JqFYUTllTtJqEXY/view?usp=sharing

I have reproduced this issue with various versions of r8 compiled from tags of

https://r8.googlesource.com/r8
1.6.67
2.0.88 (with AGP v4.0.1)
2.1.38
2.2.6-dev

Please contact me if you need any further information.

Comments

sg...@google.com <sg...@google.com> Aug 4, 2020 06:29AM

Assigned to sg...@google.com.

ze...@google.com <ze...@google.com> #2Aug 4, 2020 07:37AM

Are there any updates here?

sg...@google.com <sg...@google.com> #3Aug 4, 2020 07:38AM

I looked at it and I'm planning to remove the deduplicating lookup the callstack refers to. It should be no longer needed. How was the 80ms in comment#1 calculated? If it's straightforward I'll calculate it again before my change goes in.

sg...@google.com <sg...@google.com> #4Aug 4, 2020 07:42AM

I couldn't move the logging off thread but I was able to address an area of concern in the callstack:
com.google.protobuf.Descriptors$FileDescriptor.internalBuildGeneratedFileFrom(Descriptors.java)
com.android.build.gradle.internal.cxx.logging.CxxLogging.<clinit>(CxxLogging.java:82)
com.android.build.gradle.internal.cxx.logging.LoggingMessage.getDescriptor(LoggingMessage.java:110)
com.android.build.gradle.internal.cxx.logging.LoggingMessage.hashCode(LoggingMessage.java:489)
java.util.HashMap.hash(HashMap.java:340)
java.util.HashMap.containsKey(HashMap.java:592)
java.util.HashSet.contains(HashSet.java:204)

This logic was deduplicating messages (including chatty 'info' messages). This deduplication shouldn't be needed anymore so I removed it.

I don't know how the 80ms from

comment#1

was gathered so I can't say how much it has improved logging, but it should help quite a bit. Let me know if you can still measure a performance issue. The next step would be to reduce the 'info' that's logged.

sg...@google.com <sg...@google.com> #5Aug 4, 2020 12:52PM

Accepted by sg...@google.com.

The change mentioned in

comment#4

(internal Id I4e51a10d9ee2a81ca56c8e81c7890dfb0bf239d7) should be available in Canary 8 of Flamingo.

ap...@google.com <ap...@google.com> #6Aug 4, 2020 01:00PM

Project: r8
Branch: master

commit aad51464604c3e360b36cc1f4473e057248b439e
Author: Søren Gjesse <sgjesse@google.com>
Date: Tue Aug 04 14:58:24 2020

Add phi check when checking in value definition in service rewriter

Bug: 162568140
Change-Id: I193475d8fbff5e8b53775d38a159181b392c1183

M src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
M src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java

https://r8-review.googlesource.com/52665

ap...@google.com <ap...@google.com> #7Aug 4, 2020 01:48PM

Project: r8
Branch: 2.1

commit f1c7696a1e9ce978541d9c303a2ad2261ddb8061
Author: Søren Gjesse <sgjesse@google.com>
Date: Tue Aug 04 15:14:14 2020

Version 2.1.56

Cherry-pick: Add phi check when checking in value definition in service rewriter
CL:

https://r8-review.googlesource.com/c/r8/+/52665

Cherry-pick: Reland "Ensure no service loader rewriting if it crosses a feature split"
CL:

https://r8-review.googlesource.com/c/r8/+/52542

bug: 162568140
Change-Id: I05cb372595d3755e81690cb8caab06c01b4c9c0b

M src/main/java/com/android/tools/r8/FeatureSplit.java
M src/main/java/com/android/tools/r8/Version.java
M src/main/java/com/android/tools/r8/graph/AppServices.java
M src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
M src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java

https://r8-review.googlesource.com/52670

ma...@gmail.com <ma...@gmail.com> #8Aug 4, 2020 01:54PM

These jars were protected by some of the features of PreEmptive Protection
DashO. I will let the team know about your concern regarding the unaltered
StackMapTable.

Regards,

Matt Insko

On Tue, Aug 4, 2020 at 8:52 AM <buganizer-system@google.com> wrote:

- Show quoted text -

sg...@google.com <sg...@google.com> #9Aug 4, 2020 02:27PM

Marked as fixed.

Thank you for you reply Matt. I see now why I could not make sense of the rewriting :-).

I will mark this fixed as the NullPointerException issue was an actual but that was uncovered by the supplied reproduction.