Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1696803002:
Add a variable indicating the target as Android M or later. (Closed)
|
Created:
4 years, 10 months ago by simonb (inactive) Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a variable indicating the target as Android M or later.
The system linker in Android M and later unpacks packed relocations
directly. This means that we can unconditionally enable relocation
packing for any APK that is explicitly designed to only run on such
Android releases, irrespective of whether or not the chromium or
other customized linker is used.
Invoker indicates this by setting requires_sdk_api_level_23 to true.
Patch Set 1 #
Total comments: 6
Messages
Total messages: 12 (3 generated)
The CQ bit was checked by simonb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1696803002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1696803002/1
simonb@chromium.org changed reviewers: + agrieve@chromium.org, torne@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1343: "Loading library from the apk requires use of the Chromium linker.") It's okay to load library from APK if it's sdk 23+ also, right? https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1346: if (defined(invoker.requires_sdk_api_level_23) && Can we just make this setting work like the other one? i.e. just allow the defining APK to specify whether it wants relocation packing, and then assert that the linker will support it? If we forcibly enable it here there won't be an easy way to build without it to compare.
On 2016/02/16 15:46:56, Torne wrote: > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.gni > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... > build/config/android/rules.gni:1343: "Loading library from the apk requires use > of the Chromium linker.") > It's okay to load library from APK if it's sdk 23+ also, right? > > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... > build/config/android/rules.gni:1346: if > (defined(invoker.requires_sdk_api_level_23) && > Can we just make this setting work like the other one? i.e. just allow the > defining APK to specify whether it wants relocation packing, and then assert > that the linker will support it? > > If we forcibly enable it here there won't be an easy way to build without it to > compare. lgtm with Torne's comments adressed. You'll also need to rebase due to https://codereview.chromium.org/1690603003/ I believe.
https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1343: "Loading library from the apk requires use of the Chromium linker.") On 2016/02/16 at 15:46:56, Torne wrote: > It's okay to load library from APK if it's sdk 23+ also, right? Yes, but at present only if the chromium linker is being used (requires path shuffling to construct a full path_to_apk!/path_within_apk.so string, passed to android_dlopen_ext). This differs from relocation unpacking, which the Android M and later system linker handles transparently and no matter how/where the library is loaded. I'm planning other changes to separate load from apk out from the chromium linker. For now this change targets only relocation packing. https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1346: if (defined(invoker.requires_sdk_api_level_23) && On 2016/02/16 at 15:46:56, Torne wrote: > Can we just make this setting work like the other one? i.e. just allow the defining APK to specify whether it wants relocation packing, and then assert that the linker will support it? The assert condition that the linker supports unpacking is "uses chromium linker OR apk's target android version is M or later". There isn't any easy way in gn to check the second of these -- the apk's target version could be a string like "M", or a string like "23", or an integer like 23, or something else entirely. If there were a way to check for apk target version later than or equal to android M that would be better used rather than a special flag. I don't believe there is one, though.
https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1343: "Loading library from the apk requires use of the Chromium linker.") On 2016/02/16 16:54:08, simonb wrote: > On 2016/02/16 at 15:46:56, Torne wrote: > > It's okay to load library from APK if it's sdk 23+ also, right? > > Yes, but at present only if the chromium linker is being used (requires path > shuffling to construct a full path_to_apk!/path_within_apk.so string, passed to > android_dlopen_ext). This differs from relocation unpacking, which the Android M > and later system linker handles transparently and no matter how/where the > library is loaded. > > I'm planning other changes to separate load from apk out from the chromium > linker. For now this change targets only relocation packing. OK. https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1346: if (defined(invoker.requires_sdk_api_level_23) && On 2016/02/16 16:54:08, simonb wrote: > On 2016/02/16 at 15:46:56, Torne wrote: > > Can we just make this setting work like the other one? i.e. just allow the > defining APK to specify whether it wants relocation packing, and then assert > that the linker will support it? > > The assert condition that the linker supports unpacking is "uses chromium linker > OR apk's target android version is M or later". There isn't any easy way in gn > to check the second of these -- the apk's target version could be a string like > "M", or a string like "23", or an integer like 23, or something else entirely. > > If there were a way to check for apk target version later than or equal to > android M that would be better used rather than a special flag. I don't believe > there is one, though. No, I mean, can we just make the *logic* work the same way: let the person defining the target set enable_relocation_packing = true, and then assert that if they did that that either they're using the chromium linker or they also set requires_sdk_api_level_23. Currently this behaves completely differently to the other one: it's forced on for api 23, and there's also no error if you try to enable it in a situation where it doesn't work.
On 2016/02/16 17:58:49, Torne wrote: > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.gni > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... > build/config/android/rules.gni:1343: "Loading library from the apk requires use > of the Chromium linker.") > On 2016/02/16 16:54:08, simonb wrote: > > On 2016/02/16 at 15:46:56, Torne wrote: > > > It's okay to load library from APK if it's sdk 23+ also, right? > > > > Yes, but at present only if the chromium linker is being used (requires path > > shuffling to construct a full path_to_apk!/path_within_apk.so string, passed > to > > android_dlopen_ext). This differs from relocation unpacking, which the Android > M > > and later system linker handles transparently and no matter how/where the > > library is loaded. > > > > I'm planning other changes to separate load from apk out from the chromium > > linker. For now this change targets only relocation packing. > > OK. > > https://codereview.chromium.org/1696803002/diff/1/build/config/android/rules.... > build/config/android/rules.gni:1346: if > (defined(invoker.requires_sdk_api_level_23) && > On 2016/02/16 16:54:08, simonb wrote: > > On 2016/02/16 at 15:46:56, Torne wrote: > > > Can we just make this setting work like the other one? i.e. just allow the > > defining APK to specify whether it wants relocation packing, and then assert > > that the linker will support it? > > > > The assert condition that the linker supports unpacking is "uses chromium > linker > > OR apk's target android version is M or later". There isn't any easy way in gn > > to check the second of these -- the apk's target version could be a string > like > > "M", or a string like "23", or an integer like 23, or something else entirely. > > > > If there were a way to check for apk target version later than or equal to > > android M that would be better used rather than a special flag. I don't > believe > > there is one, though. > > No, I mean, can we just make the *logic* work the same way: let the person > defining the target set enable_relocation_packing = true, and then assert that > if they did that that either they're using the chromium linker or they also set > requires_sdk_api_level_23. > > Currently this behaves completely differently to the other one: it's forced on > for api 23, and there's also no error if you try to enable it in a situation > where it doesn't work. torne - should we close this?
Message was sent while issue was closed.
Closed it, this isn't needed any more, we implemented a slightly different solution to enable relocation packing. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
