unify ssize_t definition

[mkatanbaf]

This PR removes the type tvm_ssize_t introduced in (PR11232) , and unifies the definition of ssize_t.
Initially, there was an issue with the Windows build in (PR10967) , since the ssize_t was not properly defined in rpc_channel_logger.h
our initial solution was to define the ssize_t in that specific file. but we got a redefinition error, since ssize_t was also defined in socket.h. We also tried to include socket.h in rpc_channel_logger.h but we got some errors on hexagon build. (PR11223)
Next attempt, we added another type, tvm_ssize_t, in c_runtime_api.h to resolve the redefinition issue (PR11232) and the plan was to propagate tvm_ssize_t to most of other ssize_t mentions to minimize confusion. The PR got merged, however, there are two issues with this solution. First, the c_runtime_api.h doesn't seem to be the appropriate place for the definition and required header files (here), and ssize_t is widely used in the codebase, so propagating the tvm_ssize_t requires changes all over the codebase.
This brings us to this current solution. It is basically back to our initial solution of defining ssize_t where it is needed (rpc_channel_logger.h ), and resolving the redefinition error by placing the definition in ssize.h file, and including it in both places. I believe this solution makes more sense because it doesn't add an unnecessary and potentially confusing type tvm_ssize_t and keeps the ssize_t definitions consistent.
@areusch, @mehrdadh, @manupa-arm

[leandron]

Hi, I understand this is a draft PR, but it would be good to have a description with the motivation and context for this change to be submitted.

[junrushao]

Hey thanks for this PR! Is the intention to remove tvm_ssize_t and replace with ssize_t instead?

[manupak]

Thanks @mkatanbaf !

I think this is because of #11232 (comment)

and this solution looks good (that it moves the introduction of the type to rpc related sources) but I agree it is better to mention this in the PR description
-- maybe its still better to keep tvm_ssize_t over overloading ssize_t

[junrushao]

Thanks @manupa-arm for the context! It makes sense to me now :-) Yeah I agree with you that it would be better to keep tvm_ssize_t instead.

[mkatanbaf]

Thanks @manupa-arm for the comment. As I explained in the PR description, I believe introducing tvm_ssize_t was not the right approach. I placed the ssize_t definition in a header file under src/support, and hopefully, that would address the redefinition concerns.

[manupak]

LGTM! I did not have a strong preference either way but have observed platform-specific redefinition of ssize_t too.

I ll let others have a look as well.

[areusch]

thanks for the fix @mkatanbaf !

[areusch]

regarding overloading ssize_t vs adding tvm_ssize_t, my main preference here is around when we do this in a public-facing header file (in that case I pushed for tvm_ssize_t to avoid stepping on system types outside tvm's purview. for an internal header in src/ i don't have a strong reason to care right now. if others do, i'm happy to go with their preference, though prefer such decisions to be rooted in more than just personal preference. this PR reverts to what we were using before, so i'm good with merging it and re-addressing later on if it becomes an issue.

[manupak]

I agree with @areusch that this is an internal header (will not bleed elsewhere) and should be the reasoning to support the change.

[manupak]

@mkatanbaf I ve noted the commit title is too long and commit body is empty. Whilst, we are still in the process of establishing commit message guidelines, I think it would be better to include a commit title and a body.

Would you be able to propose these before we merge / or push a commit that has these ?

[mkatanbaf]

Thanks @manupa-arm , I updated the commit title and body.

[manupak]

Thanks @mkatanbaf @areusch @junrushao1994 ! This is merged now!