Fix Debugger timeout on 32 bit devices. #9869
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context #9573 Not sure why, but we are getting an odd problem were the `timeout` value we pass to mono for the debugger is not long enough. This is causing the debugger to timeout. What we see is `timeout=3` rather than `timeout=1740583188`. Lets upgrade to use `std:format` to see if that helps as it with automatically take care of the conversion for us.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Sorry opened the previous PR from a fork, have to reopen it. |
|
@akoeplinger any chance you can post the code you posted on the other PR? I had to close that one. |
|
@dellis1972 the one from #9868 (comment)? basically all of the code is visible, I just pasted |
|
@akoeplinger was able to reproduce the wrong conversion issue with %d on a sample C++ armv7 app running on an x86 emulator so this should indeed fix it.
|
|
never mind, I figured I could just copy and paste 😃 |
Fixes: https://github.com/dotnet/android/issues/9573
Context: 8e1c0e6e2f4c41c9a24904bb1cea943357d78ac4
A customer reports that they are unable to attach a debugger to
32-bit Android apps:
> am start -a "android.intent.action.MAIN" -c "android.intent.category.LAUNCHER" -n "com.companyname.exemploandroid/crc64358cda76bdc6f75f.MainActivity"
> Starting: Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=com.companyname.exemploandroid/crc64358cda76bdc6f75f.MainActivity }
…
[monodroid-debug] Trying to initialize the debugger with options: --debugger-agent=transport=dt_socket,loglevel=0,address=127.0.0.1:8805,server=y,embedding=1,timeout=8
…
[mono] Listening on 127.0.0.1:8805 (timeout=8 ms)...
[mono] debugger-agent: Timed out waiting to connect.
Of particular note from the above log messages is the `timeout=8`
value, which is considerably lower than it needs to be, which is a
[value in *milliseconds*][0]. This low value is responsible for the
subsequent `debugger-agent: Timed out` log message.
The `timeout=` value was introduced in commit 8e1c0e6e, which used
`%d` to convert the `timeout_time` value; [for context][1]:
struct RuntimeOptions {
int64_t timeout_time = 0;
// …
};
// …
char *debug_arg = utils.monodroid_strdup_printf (
"--debugger-agent=transport=dt_socket,loglevel=%d,address=%s:%d,%sembedding=1,timeout=%d",
loglevel,
options.host,
options.sdb_port,
options.server ? "server=y," : "",
options.timeout_time
);
`options` is a `RuntimeOptions`, and `options.timeout_time` is thus a
`int64_t`, but it's being `printf`d via `%d`.
`%d` is supposed to be an *int*; from [**printf**(3)][2]:
> **d**, **i**
> The *int* argument is converted to signed decimal notation.
which means using `%d` for an `int64_t` will only work properly on
ILP64 targets. Android, notably, is an ILP32 (32-bit) or
LP64 (64-bit) target, *never* ILP64; it's a wonder this worked
*anywhere*, with any degree of reliability.
The fix is to realize that it doesn't even make sense to be
forwarding `RuntimeOptions::timeout_time` in this manner in the first
place: `RuntimeOptions::timeout_time` is compared against
`time(NULL)`, i.e. it's a [Unix time value][3] (*seconds* since
1970-01-01 UTC), not a value in milliseconds at all!
Replacing the use of `options.timeout_time` with 30000 allows for
a value that is reasonable for the target domain, *and* works
properly and consistently on both ILP32 (32-bit) and LP64 (64-bit)
Android targets.
[0]: https://github.com/dotnet/runtime/blob/9022f3a9b63b56234726606bc5547378b2d08f6b/src/mono/mono/component/debugger-agent.c#L581
[1]: https://github.com/dotnet/android/blob/8e1c0e6e2f4c41c9a24904bb1cea943357d78ac4/src/monodroid/jni/monodroid-glue-internal.hh#L98-L106
[2]: https://linux.die.net/man/3/printf
[3]: https://en.wikipedia.org/wiki/Unix_time |
jonpryor
reviewed
Mar 4, 2025
| @@ -504,15 +504,10 @@ MonodroidRuntime::mono_runtime_init ([[maybe_unused]] JNIEnv *env, [[maybe_unuse | |||
| { | |||
| #if defined (DEBUG) | |||
| RuntimeOptions options{}; | |||
| int64_t cur_time; | |||
There was a problem hiding this comment.
We don't want to remove this check either. We want just the change to the --debugger-agent string.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #9573
Context: 8e1c0e6
A customer reports that they are unable to attach a debugger to
32-bit Android apps:
Of particular note from the above log messages is the
timeout=8value, which is considerably lower than it needs to be, which is a
value in milliseconds. This low value is responsible for the
subsequent
debugger-agent: Timed outlog message.The
timeout=value was introduced in commit 8e1c0e6, which used%dto convert thetimeout_timevalue; for context:optionsis aRuntimeOptions, andoptions.timeout_timeis thus aint64_t, but it's beingprintfd via%d.%dis supposed to be an int; from printf(3):which means using
%dfor anint64_twill only work properly onILP64 targets. Android, notably, is an ILP32 (32-bit) or
LP64 (64-bit) target, never ILP64; it's a wonder this worked
anywhere, with any degree of reliability.
The fix is to realize that it doesn't even make sense to be
forwarding
RuntimeOptions::timeout_timein this manner in the firstplace:
RuntimeOptions::timeout_timeis compared againsttime(NULL), i.e. it's a Unix time value (seconds since1970-01-01 UTC), not a value in milliseconds at all!
Replacing the use of
options.timeout_timewith 30000 allows fora value that is reasonable for the target domain, and works
properly and consistently on both ILP32 (32-bit) and LP64 (64-bit)
Android targets.