i#2154 Android64: Remove get_kernel_args() #7240
Conversation
Since the original 32-bit Android port was done bionic was changed [1] to pass argc, argv, and envp to library _init() functions so we can get rid of the old workaround. This fixes several tests which were failing with the error: ``` Internal Error: DynamoRIO debug check failure: failed to find envp @dynamorio/core/unix/os.c:801 envp != NULL ``` [1] https://android.googlesource.com/platform/bionic/+/554374693408cd7c74d0cae596fca7349661edea Issue: #2154
| @@ -787,19 +787,6 @@ static init_fn_t | |||
| INITIALIZER_ATTRIBUTES int | |||
| _init(int argc, char **argv, char **envp) | |||
| { | |||
| # ifdef ANDROID | |||
There was a problem hiding this comment.
Is it worth keeping this workaround for older Bionic versions? Could we somehow check the Bionic version?
There was a problem hiding this comment.
The behaviour was changed in 2016. Do we need to maintain compatibility back that far?
There was a problem hiding this comment.
Unless there's a simple way to keep backward compatibility, this seems fine to me. Maybe worth a note in release.dox that Bionic version < N is not supported now.
Let's wait for Derek's approval too.
There was a problem hiding this comment.
Could we perhaps fallback to not using the workaround when we detect envp == NULL (instead of failing). It's what we do in release build too right?
There was a problem hiding this comment.
The behaviour was changed in 2016. Do we need to maintain compatibility back that far?
For Linux, I would say yes, 2016 is not that long ago. Maybe we should make an explicit decision about Android and document it: what is the oldest version to support? 7.0 was in 2016 and 6.0 in 2015. What is the oldest version that Play supports? I think there are people still using very old versions out there (seeing 4.x in some cases) but seems reasonable to not put effort into things older than what Play supports? AI search summary says that's 6.0 which would say to support pre-2016; but https://developer.android.com/google/play/requirements/target-sdk implies that 10.0 is barely worth supporting.
But if you wanted to keep the get_kernel_args() how would you know when to call it? If old Android didn't pass defined values, are the parameter values here just random values (whatever was in the registers) and not a sentinel like null? Or is there an easy way to get the version?
| @@ -787,19 +787,6 @@ static init_fn_t | |||
| INITIALIZER_ATTRIBUTES int | |||
| _init(int argc, char **argv, char **envp) | |||
| { | |||
| # ifdef ANDROID | |||
There was a problem hiding this comment.
The behaviour was changed in 2016. Do we need to maintain compatibility back that far?
For Linux, I would say yes, 2016 is not that long ago. Maybe we should make an explicit decision about Android and document it: what is the oldest version to support? 7.0 was in 2016 and 6.0 in 2015. What is the oldest version that Play supports? I think there are people still using very old versions out there (seeing 4.x in some cases) but seems reasonable to not put effort into things older than what Play supports? AI search summary says that's 6.0 which would say to support pre-2016; but https://developer.android.com/google/play/requirements/target-sdk implies that 10.0 is barely worth supporting.
But if you wanted to keep the get_kernel_args() how would you know when to call it? If old Android didn't pass defined values, are the parameter values here just random values (whatever was in the registers) and not a sentinel like null? Or is there an easy way to get the version?
Since the original 32-bit Android port was done bionic was changed [1] to pass argc, argv, and envp to library _init() functions so we can get rid of the old workaround.
This fixes several tests which were failing with the error:
[1] https://android.googlesource.com/platform/bionic/+/554374693408cd7c74d0cae596fca7349661edea
Issue: #2154