Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

D8 dex merger fails when a synthetic class is placed on a different shard than its container class #16368

Closed
mauriciogg opened this issue Oct 1, 2022 · 11 comments
Labels
P1 I'll work on this now. (Assignee required) team-Android Issues for Android team type: bug

Comments

@mauriciogg
Copy link
Contributor

mauriciogg commented Oct 1, 2022

Description of the bug:

a desugared lambda is packed in a different shard than the one
from its original class D8 will fail with the following message:

Attempt at compiling intermediate artifact without its context
e.g.

I'm encountering an issue when compiling my app with the following classes:
org/chromium/base/MemoryPressureListener ``org/chromium/base/MemoryPressureListener$$ExternalSyntheticLambda0

bazel fails with the following message

Error in bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/mirroring-sample-app/dexsplits/apk/9.shard.zip:org/chromium/base/MemoryPressureListener$$ExternalSyntheticLambda0.class.dex:
Attempt at compiling intermediate artifact without its context
Merge failed: Compilation failed to complete, origin: bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/mirroring-sample-app/dexsplits/apk/9.shard.zip:org/chromium/base/MemoryPressureListener$$ExternalSyntheticLambda0.class.dex
Exception in thread "main" com.android.tools.r8.CompilationFailedException: Merge failed: Compilation failed to complete, origin: bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/mirroring-sample-app/dexsplits/apk/9.shard.zip:org/chromium/base/MemoryPressureListener$$ExternalSyntheticLambda0.class.dex
at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:389)
Target //apps/hermosa/hermosa-services/realtime/mirroring-sample-app:apk failed to build

Here the container class is in shard #8 and the lambda class is in shard #9

Running with d8 dexmerger and native multidex.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Its hard to provide a small reproducible example since it needs to be an app large enough to exceed the size of a dex file and the sharding has to result in a lambda class ending up in a different shard than the container class.

Which operating system are you running Bazel on?

Linux/Mac

What is the output of bazel info release?

Internal build merged with master at commit b6803a2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@nkoroste
Copy link
Contributor

nkoroste commented Oct 3, 2022

@ahumesky

@Bencodes
Copy link
Contributor

Bencodes commented Oct 4, 2022

We are also seeing this in our repo using the latest rolling release (6.0.0-pre.20220922.1)

@ahumesky ahumesky added P1 I'll work on this now. (Assignee required) and removed untriaged labels Oct 25, 2022
@lukaciko
Copy link

@ahumesky can this issue be added to Bazel 6.1. release blockers? As you noted #16369 does not completely solve it. We are hesitant to upgrade to 6.0 even with that fix since we might run into the same issue with another of synthetic classes at any point.

@ahumesky
Copy link
Contributor

Yes I think we can get the fix in for 6.1.

But a question for those of you running into this issue -- we're not seeing this internally, so I'm trying to figure out what differences might be causing this with bazel. Is anyone using the min_sdk_version attribute of android_binary (related: #10556 (comment)), or perhaps modified the hard coded dexing API level?

@pswaminathan
Copy link

We are not setting either of those. We are setting min API level in the manifest using the uses-sdk tag. For an application that exhibits it, it is happening consistently. However, triggering it doesn't seem to be related to changing build rules. i.e. We have experienced with an application in which the change that triggered this behavior is purely a code change. It's a little hard for me to create a repro, given that it depends on internal code.

Also—I don't see this added to 6.1 release blockers. Is that still a possibility?

@ahumesky
Copy link
Contributor

Thanks -- turns out the reason we weren't seeing this internally isn't related to API level exactly, but internally we have other code paths that avoid this problem. We're removing these codepath soon so that the code matches internally and externally, so we'll want the fix for this as well.

It's been pretty tricky to get the fix for this working in all cases, and has required reworking the PR for this a fair bit (#16369 (comment)). I'm rerunning all our tests again to see if anything still fails with the updates to the PR, so I'm a little hesitant now to block 6.1.0 on this if there are more cases that need to be addressed. If we can't get the fix in for 6.1.0 we cherrypick this into 6.2.0.

@ahumesky
Copy link
Contributor

ahumesky commented Mar 2, 2023

After working through #16369 and talking with the D8 team, they propose (copied from #16369):

  1. Update CompatDexBuilder to use the D8 DexFilePerClassFile consumer instead of DexIndexed (this should be a no op for the build as the current build there is already doing one dex per class file, but it uses another D8 API such that it can get the synthetic info).

  2. Update the D8 DexFilePerClassFile accept function to also receive the "synthetic context" information if the compiled class is a compiler synthesized class.

  3. Update CompatDexBuilder to write the synthetic context information to some build meta info for later access by the DexFileSplitter. Something like this is already done for the desugar dependencies which are written to a proto in META-INF/desugar_deps.

  4. Update DexFileSplitter to use the meta-inf of synthetic classes to group with the right contexts.

The D8 team would handle 1 and 2, and the bazel team and/or community could do 3 and 4. This avoids coupling to D8 implementation details and having to parse dex files and hopefully reduces the code complexity a bit.

copybara-service bot pushed a commit that referenced this issue May 8, 2023
This completes steps 2 and 3 in b/241351268#comment9
towards resolving #16368

PiperOrigin-RevId: 530238344
Change-Id: I569bf8e3bfa81f7005f3e9b79338d5a5b868e339
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
This completes steps 2 and 3 in b/241351268#comment9
towards resolving bazelbuild#16368

PiperOrigin-RevId: 530238344
Change-Id: I569bf8e3bfa81f7005f3e9b79338d5a5b868e339
@ahumesky
Copy link
Contributor

The fix for this should be ready to submit tomorrow, and then I'll cherrypick it into the Bazel 6.3 release.

ahumesky pushed a commit to ahumesky/bazel that referenced this issue Jun 29, 2023
This completes steps 2 and 3 in b/241351268#comment9
towards resolving bazelbuild#16368

PiperOrigin-RevId: 530238344
Change-Id: I569bf8e3bfa81f7005f3e9b79338d5a5b868e339
ahumesky added a commit to ahumesky/bazel that referenced this issue Jun 29, 2023
… so that synthetic classes don't end up without their context classes in dex shards, which will causing merging to fail in DexFileMerger. Fixes bazelbuild#16368.

RELNOTES: None
PiperOrigin-RevId: 544134712
Change-Id: Ib29f6659f18dd71be96a7985bc25cfb44e719ae5
meteorcloudy pushed a commit that referenced this issue Jun 29, 2023
* Update D8/R8 dependency in bazel to 8.0.40

PiperOrigin-RevId: 520331100
Change-Id: Ia150c637747c980fc5cdfa07c80edd3a9e30e04b

* Add options converters specific to R8 to avoid depending on android_builder_lib which causes classpath conflicts with D8 classes.

RELNOTE: None
PiperOrigin-RevId: 526149555
Change-Id: Iacfce24cc3d2327ef63c47f3bfd76813beaefc7a

* Add android_gmaven_r8 to bazel's WORKSPACE file so that it overrides the version of R8 that might come from android_remote_tools.WORKSPACE in a released version of bazel used to run the test (which would be potentially out of date with respect to the source code). Also merge this with android_gmaven_r8_for_testing since they would be redundant.

RELNOTES: None.
PiperOrigin-RevId: 529830525
Change-Id: Ic5c18ce5beb7fb915b084dc19590a05b04675548

* Collect the contexts of D8 compiler-synthesized classes.

This completes steps 2 and 3 in b/241351268#comment9
towards resolving #16368

PiperOrigin-RevId: 530238344
Change-Id: I569bf8e3bfa81f7005f3e9b79338d5a5b868e339

* Group synthetic classes with their context classes in DexFileSplitter so that synthetic classes don't end up without their context classes in dex shards, which will causing merging to fail in DexFileMerger. Fixes #16368.

RELNOTES: None
PiperOrigin-RevId: 544134712
Change-Id: Ib29f6659f18dd71be96a7985bc25cfb44e719ae5

---------

Co-authored-by: Googler <noreply@google.com>
@ahumesky
Copy link
Contributor

There's still this same issue with the dex_shards attribute and DexMapper (which is still used for mobile-install "classic" while we open source the starlark replacement). I have an in progress change that applies same fix for DexFileSplitter to DexMapper, which we'll merge into Bazel 6.3 as well.

@ahumesky ahumesky reopened this Jun 30, 2023
ahumesky added a commit to ahumesky/bazel that referenced this issue Jul 6, 2023
…classes when sharding dexes, like DexFileSplitter in bazelbuild@9092303. Fixes bazelbuild#16368

RELNOTES: None
PiperOrigin-RevId: 546041575
Change-Id: I12fa8cff15b13534ca9a5682b7e9b43e6c860ef8
ahumesky added a commit to ahumesky/bazel that referenced this issue Jul 6, 2023
…classes when sharding dexes, like DexFileSplitter in bazelbuild@9092303. Fixes bazelbuild#16368

RELNOTES: None
PiperOrigin-RevId: 546041575
Change-Id: I12fa8cff15b13534ca9a5682b7e9b43e6c860ef8
iancha1992 pushed a commit that referenced this issue Jul 7, 2023
…context … (#18853)

* Teach DexMapper to not separate synthetic classes from their context classes when sharding dexes, like DexFileSplitter in 9092303. Fixes #16368

RELNOTES: None
PiperOrigin-RevId: 546041575
Change-Id: I12fa8cff15b13534ca9a5682b7e9b43e6c860ef8

* Use remap_paths to android_tools

This ensures that the `all_android_tools_deploy.jar` and `ImportDepsChecker_deploy.jar` artifacts end up in the root of the `tar` where `exports_files` is able to reference them.

Before this PR the tar had the following directory structure:
```
  ./
  ./BUILD
  ./WORKSPACE
  ./desugar_jdk_libs.jar
  ./version.txt
  ./src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ImportDepsChecker_deploy.jar
  ./src/tools/android/java/com/google/devtools/build/android/all_android_tools_deploy.jar
```

After:
```
  BUILD
  ImportDepsChecker_deploy.jar
  WORKSPACE
  all_android_tools_deploy.jar
  desugar_jdk_libs.jar
  version.txt
```

Closes #17187.

PiperOrigin-RevId: 501862387
Change-Id: Ida51fa3f0bd3b07d3106653e5292a90cac143b68

* Add rules_pkg to the list of test repositories set up for shell integration tests. Commit b7a79ff depends on this. This is a partial cherrypick of e9929af.

---------

Co-authored-by: Benjamin Lee <ben@ben.cm>
copybara-service bot pushed a commit that referenced this issue Jul 10, 2023
RELNOTES: None
PiperOrigin-RevId: 547016404
Change-Id: I7a00a076db4a6d4f5e9e631357fc30696e6038f6
iancha1992 added a commit that referenced this issue Jul 12, 2023
…ps://gith... (#18891)

* Update Android tools to 0.27.1 for fixes to DexMapper for #16368

* Update Android rules to 0.27.2 to include #18909

---------

Co-authored-by: keertk <keerthanakumar@google.com>
Co-authored-by: Ian (Hee) Cha <heec@google.com>
brettchabot added a commit to android/android-test that referenced this issue Aug 18, 2023
The main motivation is to pick up fix for
bazelbuild/bazel#16368,
which appears to be needed to increment version of androidx.window.
brettchabot added a commit to android/android-test that referenced this issue Aug 18, 2023
The main motivation is to pick up fix for bazelbuild/bazel#16368,
which appears to be needed to increment version of androidx.window.
oliviernotteghem pushed a commit to uber-common/bazel that referenced this issue Oct 6, 2023
… so that synthetic classes don't end up without their context classes in dex shards, which will causing merging to fail in DexFileMerger. Fixes bazelbuild#16368.

RELNOTES: None
PiperOrigin-RevId: 544134712
Change-Id: Ib29f6659f18dd71be96a7985bc25cfb44e719ae5
@nkoroste
Copy link
Contributor

nkoroste commented Oct 13, 2023

@ahumesky I think we're still seeing this issue on Linux only when filenames contain capital letters.

11:49:44 18:49:44 [Bazel] bazel-out/k8-opt-exec-ST-7a15de0b7d6a/bin/external/bazel_tools/tools/android/d8_dexmerger --input bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexsplits/apk/7.shard.zip --output bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexfiles/apk/7.shard.zip '--multidex=given_shard')
11:49:44 18:49:44 [Bazel] # Configuration: 064afe4bfa5b855024151ed67fa23ab997cd53f5f864cf3be12acef0225806a5
11:49:44 18:49:44 [Bazel] # Execution platform: @local_config_platform//:host
11:49:44 18:49:44 [Bazel] Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
11:49:44 18:49:44 [Bazel] Error in bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexsplits/apk/7.shard.zip:com/snap/ipc/buffers/IBufferMapper$-CC.class.dex:
11:49:44 18:49:44 [Bazel] Attempt at compiling intermediate artifact without its context
11:49:44 18:49:44 [Bazel] Merge failed: Compilation failed to complete, origin: bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexsplits/apk/7.shard.zip:com/snap/ipc/buffers/IBufferMapper$-CC.class.dex
11:49:44 18:49:44 [Bazel] Exception in thread "main" java.lang.RuntimeException: Merge failed: Compilation failed to complete, origin: bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexsplits/apk/7.shard.zip:com/snap/ipc/buffers/IBufferMapper$-CC.class.dex
11:49:44 18:49:44 [Bazel] at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:412)
11:49:44 18:49:44 [Bazel] Caused by: com.android.tools.r8.CompilationFailedException: Compilation failed to complete, origin: bazel-out/k8-fastbuild/bin/apps/hermosa/hermosa-services/realtime/dexsplits/apk/7.shard.zip:com/snap/ipc/buffers/IBufferMapper$-CC.class.dex
11:49:44 18:49:44 [Bazel] at Version.fakeStackEntry(Version_8.1.51.java:0)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.M.a(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:5)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.utils.R0.a(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:81)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.utils.R0.a(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:32)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.utils.R0.a(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:31)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.utils.R0.b(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:2)
11:49:44 18:49:44 [Bazel] at com.android.tools.r8.D8.run(R8_8.1.51_9b737621870a00577be8badac89ea7485627e0cae5c007fd3c57c275c1a604a9:6)
11:49:44 18:49:44 [Bazel] at com.google.devtools.build.android.r8.DexFileMerger.run(DexFileMerger.java:395)
11:49:44 18:49:44 [Bazel] at com.google.devtools.build.android.r8.DexFileMerger.main(DexFileMerger.java:409)
11:49:44 18:49:44 [Bazel] Caused by: com.android.tools.r8.utils.b: Attempt at compiling intermediate artifact without its context

@ricowind
Copy link

nkoroste@ is this a custom R8 build (I can't retrace)?

Is IBufferMapper.class in any of the shards?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Android Issues for Android team type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants