-
Notifications
You must be signed in to change notification settings - Fork 258
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
copy ASAN library to app install directory #540
Comments
Yes, please! And it should also generate wrap.sh script. |
I was uncertain about that. I was thinking maybe it would be better to let the user do that so they can add any of their own behaviors, but I suppose as long as we allow them to do that as well then there's no problem with us providing a sensible default. Probably will add an |
This actually ended up being a lot easier than I expected: https://android-review.googlesource.com/#/c/platform/ndk/+/618842 Too late for beta 1, but it will be in beta 2. Note that I haven't looked into the wrap.sh generation yet, but this handles the more involved part of the work. |
Will be in beta 1 after all since we're still waiting for gradle fixes. |
If the app uses any sanitizers, we can install the library to the out directory to allow them to use wrap.sh rather than root their devices. This only works for ndk-build due to the nature of CMake toolchain files. Test: Removed the sanitizer runtime libraries from my test devices Test: ./run_tests.py --rebuild --filter asan-smoke Bug: android/ndk#540 Change-Id: I2944c468a1b34161d792e689c68ad2c391bdd409 (cherry picked from commit d7e5e76ed836ed0c4093d5558d64fabecdfdb1eb)
Test: ./checkbuild.py && ./run_tests.py Bug: android/ndk#540 Change-Id: I36990c3f905611e2c56a5f89c1cee4bd54c8bdb1 (cherry picked from commit 87bda31b284539a2e1e4455fcfbbf6aa4e706168)
I used the wrong variable to check the sanitizer flags, so it was always using ASAN, causing the asan wrap.sh to be installed unless the user specified their own. Fix the problem and add a test case to make sure this is only installed if it's requested. Also clean up some debug logging that was accidentally left in. Test: ./run_tests.py --rebuild Bug: android/ndk#540 Change-Id: I0e6d4b0222a4aeb1b491f6869c338011b74e2e8f (cherry picked from commit 38b57e4af408b0df9288f110dd6cffa0edef30ba)
Deal with the `-fsanitize=address,undefined` syntax, as well as cases like `-fsanitize=address -fno-sanitize=address`. Test: ./run_tests.py --rebuild Test: Manually checked a few cases with more complicated flags Test: nose2 build Bug: android/ndk#540 Change-Id: Ib3e740cce53f5007e1d1218898c4c15a19b94922 (cherry picked from commit 4d958275a1026b6d1f3e9c6dd0ff45b6c6d57b8d)
The Python tests were passing because we only tested the individual functions, not the program as a whole. The run_tests.py tests were passing because errors from $(shell) do not cause the build to fail; ndk-build just ended up parsing the error message and not finding any sanitizers. The asan-smoke test passed because A) I hadn't run `./run_tests.py --clean-device` to purge the old tests, so the libraries were still on the device and B) running the tests in the r17 branch had led to asan_device_setup running again, installing the libraries to the system partition once again. Test: nose2 build Test: ./run_tests.py --clean-device # After purging asan /system libs Bug: android/ndk#540 Change-Id: I85a30cc198be4a0f39548271f56112835805ca76
The docs at https://github.com/google/sanitizers/wiki/AddressSanitizerOnAndroid are still indicating the need for a rooted phone. Could we get updated info and tips on that page about how to setup asan with gradle and cmake? |
I'd be happy to update that page or accept a pull request, but I don't know much about Gradle. |
@yabinc is probably our wrap.sh + gradle expert... |
Here is the part of doc in simpleperf showing how to add wrap.sh, https://android.googlesource.com/platform/prebuilts/simpleperf/+/master/doc/#prepare-an-android-application. |
Now that Android has wrap.sh, we need to ship the ASAN runtime (or whatever sanitizer runtime) with the other shared libraries that are a part of the app. ndk-build should install this in the out directory with the rest of the shared libs.
cc @eugenis
The text was updated successfully, but these errors were encountered: