search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

IL tests checking DART_CONFIGURATION at runtime (e.g., using isSimulator) fail on android #53774

Open sstrickl opened 1 year ago

sstrickl commented 1 year ago

There are new test failures on Reland "[vm/compiler] Change MemoryCopy to also take untagged addresses.".

The tests

vm/dart/pointer_as_typed_list_il_test RuntimeError (expected Pass)

are failing on configurations

vm-aot-android-release-arm64c
vm-aot-android-release-arm_x64

Example log:

--- Command "vm_compile_to_kernel" (took 01.000330s):
DART_CONFIGURATION=ReleaseAndroidARM64C /b/swarming/w/ir/pkg/vm/tool/gen_kernel --aot --platform=out/ReleaseAndroidARM64C/vm_platform_strong.dill -o /b/swarming/w/ir/out/ReleaseAndroidARM64C/generated_compilations/vm-aot-android-release-arm64c/runtime_tests_vm_dart_pointer_as_typed_list_il_test/out.dill /b/swarming/w/ir/runtime/tests/vm/dart/pointer_as_typed_list_il_test.dart -Dtest_runner.configuration=vm-aot-android-release-arm64c --packages=/b/swarming/w/ir/.dart_tool/package_config.json -Ddart.vm.product=false --sound-null-safety

exit code:
0

...

Executing adb -s HT7BG1A04039 shell export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/data/local/tmp/precompilation-testing/test;export TEST_COMPILATION_DIR=/data/local/tmp/precompilation-testing/test;/data/local/tmp/precompilation-testing/dart_precompiled_runtime --android-log-to-stderr --sound-null-safety -Dtest_runner.configuration=vm-aot-android-release-arm64c --ignore-unrecognized-flags --packages=/b/swarming/w/ir/.dart_tool/package_config.json /data/local/tmp/precompilation-testing/test/out.aotsnapshot ; echo AdbShellExitCode:  $?
Stdout:
AdbShellExitCode: 255
Stderr:
Unhandled exception:
Expected DART_CONFIGURATION to be defined
#0      isSimulator.<anonymous closure> (package:vm/testing/il_matchers.dart:571)
#1      isSimulator (package:vm/testing/il_matchers.dart:574)
#2      main (file:///b/swarming/w/ir/runtime/tests/vm/dart/pointer_as_typed_list_il_test.dart)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:295)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184)
ExitCode: 255
Time: 0:00:00.086481

--- Re-run this test:
python3 tools/test.py -n vm-aot-android-release-arm64c vm/dart/pointer_as_typed_list_il_test

So the checking part of the IL tests pass, but it fails when trying to run the actual program. As shown above, the DART_CONFIGURATION runtime variable isn't passed to tests running on Android. However, these tests are also compiled to kernel with -Dtest_runner.configuration, so perhaps we should use that as either the primary source of truth, falling back to DART_CONFIGURATION when not provided, or vice versa.

/cc @alexmarkov @mraleph

dcharkes commented 1 year ago

So, DART_CONFIGURATION is defined as the directory the build is performed in

https://github.com/dart-lang/sdk/blob/e4bb5b608b42b97012e968866866922ccf175eba/pkg/test_runner/lib/src/test_suite.dart#L61

And then test_runner.configuration is defined as the configuration name. The one that is passed into test.py as -n. (Which is not 1:1 with "builder"...)

https://github.com/dart-lang/sdk/blob/e4bb5b608b42b97012e968866866922ccf175eba/pkg/test_runner/lib/src/options.dart#L700

I think we should avoid special casing this test. Rather, we should ensure that both env vars are defined always in how all tests are run. Unless there's some reason why they aren't.

I believe some tests use artefacts from multiple build folders. E.g. with using a gensnapshot from the non-sim build folder and then running with the runtime from the sim build folder.

So that probably means the tests should solely rely on test_runner.configuration and that env var should be always set when all tests are run.

sstrickl commented 1 year ago

Only checking test_runner.configuration is fine by me, I can easily adjust https://dart-review.googlesource.com/c/sdk/+/33074 accordingly.

sstrickl commented 1 year ago

Turns out this won't work, because when the compare_il utility is run, it only gets DART_CONFIGURATION, not -Dtest_runner.configuration, whereas whether DART_CONFIGURATION is passed when running the compiled test itself varies on the target platform. So for now, going back to checking both.

dcharkes commented 1 year ago

Shouldn't we fix that in pkg/test_runner so that compare_il does get a -Dtest_runner.configuration?

sstrickl commented 1 year ago

Would like to fix the test ASAP, I'll not close the bug and look into this after landing the immediate fix.