[rpc] Implemented rpc logging #11232
Conversation
|
@mehrdadh, is there any specific Hexagon test I should run? the CI tests are passed. |
|
@mkatanbaf Nothing on your end, I'm gonna test it locally and get back to you. |
|
I tested hexagon_launcher and looks good. |
mkatanbaf
force-pushed
the
rpc_logger
branch
2 times, most recently
from
a2e7a45 to
86f58a8
Compare
|
@mkatanbaf i think this fix makes sense to me, but that we should propagate tvm_ssize_t to most of the other ssize_t mentions in the codebase. in particular, in |
|
@areusch I believe the conclusion was to propagate the tvm_ssize_t in a separate PR, to keep this one mergeable. |
|
okay that's fine by me so long as that's our intent |
* Implemented rpc logging * fixing windows build issue * trigger Co-authored-by: Mohamad <[email protected]>
* Implemented rpc logging * fixing windows build issue * trigger Co-authored-by: Mohamad <[email protected]>
* Implemented rpc logging * fixing windows build issue * trigger Co-authored-by: Mohamad <[email protected]>
| @@ -76,6 +76,18 @@ extern "C" { | |||
| #endif | |||
| #include <stddef.h> | |||
| #include <stdint.h> | |||
| #include <stdio.h> | |||
| #include <sys/types.h> | |||
There was a problem hiding this comment.
@mkatanbaf @mehrdadh @areusch why is this header needed ?
Is it just because to use the type ssize_t ?
There was a problem hiding this comment.
Also do we want this added to c_runtime_api ?
(As this is affecting bare metal deployments)
There was a problem hiding this comment.
ah, I forgot about that. I believe this was needed on Windows to define tvm_ssize_t properly. @mkatanbaf could you confirm that? If so, perhaps we can place this #include behind #ifdef _MSC_VER
There was a problem hiding this comment.
@manupa-arm @areusch The <stdio.h> is needed to define ssize_t (not on windows, for windows we directly used int64_t and int32_t instead of ssize_t). The <sys/type.h> was needed on hexagon.
We discussed the proper place to add this definition and landed on the c_runtime_api. @manupa-arm please lmk if you have another suggestion.
There was a problem hiding this comment.
From a naive look at the PR, It seems it is being used in rpc related sources (that uses PosixWrite and already assumes the presense of the OS). How about somewhere that is specific to rpc ?
There was a problem hiding this comment.
@manupa-arm thanks. there was an issue with ssize_t being defined in other parts of the code which caused a redefinition error. our plan was to unify the different definitions in a subsequent PR soon. I'll check if I can find a better place for the definition in order to minimize the effect on bare metal deployments.
There was a problem hiding this comment.
perhaps we can move this to e.g. tvm/support/types.h or something of that nature.
… modification to auto-build Android apps and upload artifacts (#11241) * Update gradle version in android_rpc app * Support latest gradle, bump versions, replace ndk build script with gradle tasks * [android_rpc] Fix linter errors, disable weird ones * [android_deploy] Support latest gradle, bump versions, fix linter errors, disable some of them * [android_camera] Support latest gradle, bump versions, rewrite readme * [android_camera] Fix linter errors * Fix sanity check errors * Add Android jobs for Github Actions * Add python requirements for TVM and android_camera, use preinstalled NDK * Revert to build with make * Add minrpc include (PR #11232) * Remove relative paths
This adds logging to rpc communications, by adding a wrapper around "RPCChannel". The wrapper, "RPCChannelLogging", has a "NanoRPCListener" that only listens to ongoing communication and log them in a human readable way. "NanoRPCListener" uses a "MinRPCSniffer" to decode the rpc commands. "MinRPCSniffer" is based on "MinRPCServer" and shares the same code to decode the packets, but replaces the execution and return handling with no-operation functions. The log is disabled by default for backward compatibility, and a test case ("test_rpc_simple_wlog") is added to the "test_runtime_rpc.py" with logging enabled.