Skip to content

debug-engine: fix a few type mismatches #4189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 16, 2025

Conversation

yamt
Copy link
Collaborator

@yamt yamt commented Apr 10, 2025

debug-engine: fix a few type mismatches

Note: on some targets, int32_t is a long.
for example, GCC shipped with the recent ESP-IDF has such a
configuration. [1]

[1] apache/nuttx#15755 (comment)

cf.
apache/nuttx#16022
https://docs.espressif.com/projects/esp-idf/en/stable/esp32/migration-guides/release-5.x/5.0/gcc.html#espressif-toolchain-changes

also, use strict prototypes as it's complained by the same version of GCC.

@lum1n0us
Copy link
Collaborator

Note: on some targets, int32_t is a long.

How about uint32? will it be a problem too ?

@yamt
Copy link
Collaborator Author

yamt commented Apr 12, 2025

Note: on some targets, int32_t is a long.

How about uint32? will it be a problem too ?

uint32 is an alias of uint32_t. the code this PR fixes is using uint32/int32.

theoretically, it's possible to make uint32 and uint32_t different. but i guess it causes far more issues than fixes.

@lum1n0us
Copy link
Collaborator

So, should we use unsigned int to replace uint32 in the function signatures in files located in core/iwasm/libraries/debug-engine/?

Plus, I noticed some int32 in function signatures have been replaced and some are not, including several static variables.

@yamt
Copy link
Collaborator Author

yamt commented Apr 13, 2025

So, should we use unsigned int to replace uint32 in the function signatures in files located in core/iwasm/libraries/debug-engine/?

why do you think so?

Plus, I noticed some int32 in function signatures have been replaced and some are not, including several static variables.

are you suggesting it's a problem? how?

@lum1n0us
Copy link
Collaborator

I suspect the issue arose when I saw the changes in core/iwasm/libraries/debug-engine/gdbserver.*. It appears to be an incomplete modification. Please help to me to understand the situation.

  • If treating "int32 like a long" is the problem, then by the same reasoning, "uint32" could also be problematic.
  • Plus, some int32 in function signatures have been replaced and some are not, including several static variables.
$ egrep -rnc "uint32|int32" core/iwasm/libraries/debug-engine
core/iwasm/libraries/debug-engine/packets.c:1
core/iwasm/libraries/debug-engine/debug_engine.cmake:0
core/iwasm/libraries/debug-engine/utils.c:3
core/iwasm/libraries/debug-engine/handler.c:20
core/iwasm/libraries/debug-engine/handler.h:1
core/iwasm/libraries/debug-engine/gdbserver.c:4
core/iwasm/libraries/debug-engine/debug_engine.h:15
core/iwasm/libraries/debug-engine/debug_engine.c:48
core/iwasm/libraries/debug-engine/gdbserver.h:2
core/iwasm/libraries/debug-engine/utils.h:5
core/iwasm/libraries/debug-engine/packets.h:0

@yamt
Copy link
Collaborator Author

yamt commented Apr 14, 2025

we should not assume int32 or int32_t is int because they can be long or even other types.

for example, os_socket_bind takes int *port. so i changed port to int.

@lum1n0us
Copy link
Collaborator

we should not assume int32 or int32_t is int because they can be long or even other types.

Totally agree. If treating "int32 like a long" is the problem, then by the same reasoning, "uint32" could also be problematic. Right?

@yamt
Copy link
Collaborator Author

yamt commented Apr 14, 2025

we should not assume int32 or int32_t is int because they can be long or even other types.

Totally agree. If treating "int32 like a long" is the problem, then by the same reasoning, "uint32" could also be problematic. Right?

yes.

@lum1n0us
Copy link
Collaborator

we should not assume int32 or int32_t is int because they can be long or even other types.

Totally agree. If treating "int32 like a long" is the problem, then by the same reasoning, "uint32" could also be problematic. Right?

yes.

Then, two problems:

  • should we use unsigned int to replace uint32 in the function signatures in files located in core/iwasm/libraries/debug-engine?
  • There are still int32 in function signatures in core/iwasm/libraries/debug-engine should we leave them as they are or use int to replace them as well?

@yamt
Copy link
Collaborator Author

yamt commented Apr 14, 2025

we should not assume int32 or int32_t is int because they can be long or even other types.

Totally agree. If treating "int32 like a long" is the problem, then by the same reasoning, "uint32" could also be problematic. Right?

yes.

Then, two problems:

* should we use unsigned int to replace uint32 in the function signatures in files located in core/iwasm/libraries/debug-engine?

* There are still `int32` in function signatures in _core/iwasm/libraries/debug-engine_ should we leave them as they are or use `int` to replace them as well?

i don't see any reasons to replace them all.
they are not problems as far as they are used consistently.

@lum1n0us
Copy link
Collaborator

Certainly. Just out of curiosity, if the next contributor intends to add a new function in core/iwasm/libraries/debug-engine/gdbserver.h, should they use int/unsigned int or int32/uint32? What's the general guideline here?

@yamt
Copy link
Collaborator Author

yamt commented Apr 14, 2025

Certainly. Just out of curiosity, if the next contributor intends to add a new function in core/iwasm/libraries/debug-engine/gdbserver.h, should they use int/unsigned int or int32/uint32? What's the general guideline here?

nothing specific to debug-engine.
the common problematic pattern is int32 * vs int *.

Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

yamt added 2 commits April 15, 2025 12:28
complained by GCC -Wstrict-prototypes
@yamt yamt force-pushed the debug-engine-type-mismatches branch from 72fc24d to 151e123 Compare April 15, 2025 03:28
Copy link

@xiaoxiang781216 xiaoxiang781216 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xiaoxiang781216
Copy link

@no1wudi could you merge this patch to unblock ci on nuttx side?

Copy link
Collaborator

@TianlongLiang TianlongLiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lum1n0us lum1n0us merged commit 0ba6532 into bytecodealliance:main Apr 16, 2025
403 checks passed
lum1n0us pushed a commit to lum1n0us/wasm-micro-runtime that referenced this pull request Apr 16, 2025
- use strict prototypes complained by GCC `-Wstrict-prototypes`
- use `int*` instead of `int32*`

Note: on some targets, int32_t is a long.
for example, GCC shipped with the recent ESP-IDF has such a
configuration.

- apache/nuttx#15755 (comment)
- apache/nuttx#16022
- https://docs.espressif.com/projects/esp-idf/en/stable/esp32/migration-guides/release-5.x/5.0/gcc.html#espressif-toolchain-changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants