Refactor native debugger #4604

tushar3q34 · 2024-09-02T22:35:25Z

SQUASH ME

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the rizin book with the relevant information (if needed)

Detailed description

Refactored debug_native.c into OS and arch specific files. Each file contains only the functions that are necessary and with minimal amounts of #ifdef .

Test plan

Closing issues

closes #4581

tushar3q34 · 2024-09-02T23:11:12Z

For the build fail in AppVeyor, it runs Visual Studio 2017 and I think it does not support #warning preprocessor command. The error from logs is : fatal error C1021: invalid preprocessor command 'warning'. I think preprocessor command like pragma should work for outdated compilers. Should I change it or let it be ?

wargio

Awesome job! Please create a new folder called native and move these new files under it.
This is to ensure we know it is related to the native debugger.

Also change the uppercase names into lowercase (i.e. librz/debug/p/DragonFly.c to librz/debug/p/dragonfly.c

wargio · 2024-09-03T01:22:25Z

For the build fail in AppVeyor, it runs Visual Studio 2017 and I think it does not support #warning preprocessor command. The error from logs is : fatal error C1021: invalid preprocessor command 'warning'. I think preprocessor command like pragma should work for outdated compilers. Should I change it or let it be ?

We never had this issue before, so you probably have changed some code-paths.

Also librz/debug/p/debug_native.c needs to do #include "dragonfly.c" only if the OS where rizin is building is dragonfly, we cannot have all of these included.

We can also do the opposite and have a big #ifdef #endif at the beginning/end of each new .c files and just #include... them in the debug_native.c

tushar3q34 · 2024-09-03T02:14:50Z

Awesome job! Please create a new folder called native and move these new files under it. This is to ensure we know it is related to the native debugger.

Also change the uppercase names into lowercase (i.e. librz/debug/p/DragonFly.c to librz/debug/p/dragonfly.c

Thanks. I went through the code and logs once and found the following issues :

linux_x86_64.c file included native/reg.c file which had a function redefined causing errors.
The __WINDOWS__ macro had a typo in one location which is probably why it did not build in AppVeyor.

I will go through everything once more and look for more errors. I will also implement the changes suggested by you and will make a new commit in a few hours.

tushar3q34 · 2024-09-03T11:27:49Z

Hello @wargio I corrected the formatting. Now its passing all except this one test case and my code passed for this particular test last time. I did not change any file that should affect native debugger.
Can you test the code on that test again ? Also I cannot see any errors in the console or log of the test so I'm not sure where to do the corrections if the code requires so.

wargio · 2024-09-03T11:41:20Z

@brightprogrammer FYI

wargio · 2024-09-03T11:42:18Z

Hello @wargio I corrected the formatting. Now its passing all except this one test case and my code passed for this particular test last time. I did not change any file that should affect native debugger. Can you test the code on that test again ? Also I cannot see any errors in the console or log of the test so I'm not sure where to do the corrections if the code requires so.

The error in the macos CI is unrelated to this PR. sometimes they fails. we have not yet discovered why.

wargio

LGTM

XVilka

I don't have any objections since it's purely code moving, but this code should be seriously (but carefully, very carefully) cleaned up.

XVilka · 2024-09-03T13:56:30Z

librz/debug/p/native/android_arm.c

+// SPDX-License-Identifier: LGPL-3.0-only
+
+#include <errno.h>
+#if !defined(__HAIKU__) && !defined(__sun)


It's a bit out of place, but could be done in a separate PR.

XVilka · 2024-09-03T13:57:35Z

librz/debug/p/native/android_arm.c

+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS 0x20
+#endif
+	snprintf(code, sizeof(code),


We should definitely rewrite/fix this part. And it doesn't belong to debugger, IMHO.

the task was to split this, so we can refactor for each platform so we can be sure to avoid breaking stuff and remove code that should definitely not be there.

XVilka · 2024-09-03T13:57:48Z

librz/debug/p/native/android_arm.c

+	};
+	int num = rz_syscall_get_num(dbg->analysis->syscall, "munmap");
+
+	snprintf(code, sizeof(code),


XVilka · 2024-09-03T13:59:10Z

librz/debug/p/native/dragonfly.c

+RZ_API RZ_OWN RzList /*<RzDebugPid *>*/ *rz_debug_native_threads(RzDebug *dbg, int pid) {
+	RzList *list = rz_list_new();
+	if (!list) {
+		eprintf("No list?\n");


Could be removed probably.

XVilka · 2024-09-03T14:03:06Z

@tushar3q34 do you want to try this issue? #3072 - it would help cleaning up and refactoring the native debugger for BSD, since it will ensure there are no regressions and allow us test debugger automatically on these platforms.

XVilka · 2024-09-03T14:55:56Z

Hmm, I remember it was fixed before, not sure why it happens again:

librz/util/meson.build:176:19: ERROR: Tried to tied to mix a host machine library ("softfloat") with a build machine target "rz_util_native" This is not possible in a cross build.

cc @DMaroo @wargio

tushar3q34 · 2024-09-03T19:03:47Z

@tushar3q34 do you want to try this issue? #3072 - it would help cleaning up and refactoring the native debugger for BSD, since it will ensure there are no regressions and allow us test debugger automatically on these platforms.

Yeah sure I will look into the issue and try my best to resolve it. As for this PR, what else am I supposed to do ?

XVilka

Good we waited for the Android buids to be fixed. This change breaks the build:

/usr/local/lib/android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/linux-x86_64/bin/x86_64-linux-android33-clang -Ilibrz/debug/librz_debug.a.p -I. -I.. -Ilibrz -I../librz -Ilibrz/include -I../librz/include -I../librz/bin/format/elf -I../librz/bin/format/dmp -I../librz/bin/format/mdmp -I../librz/bin/format/pe -I../subprojects/rzgdb/include -I../subprojects/rzgdb/include/gdbclient -I../subprojects/rzgdb/include/gdbserver -Isubprojects/rzwinkd -I../subprojects/rzwinkd -Ilibrz/util/sdb/src -I../librz/util/sdb/src -I../librz/bin/format -I../librz/arch/isa -I../librz/arch/isa_gnu -Ilibrz/arch -I../librz/arch -I../librz/type/parser -I../subprojects/rzqnx/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O3 -DRZ_PLUGIN_INCORE=1 -DSUPPORTS_PCRE2_JIT -D_GNU_SOURCE --std=gnu99 -Werror=sizeof-pointer-memaccess -fPIC -MD -MQ librz/debug/librz_debug.a.p/p_debug_native.c.o -MF librz/debug/librz_debug.a.p/p_debug_native.c.o.d -o librz/debug/librz_debug.a.p/p_debug_native.c.o -c ../librz/debug/p/debug_native.c
In file included from ../librz/debug/p/debug_native.c:33:
In file included from ../librz/debug/p/native/android_x86_64.c:44:
../librz/debug/p/native/reg.c:7:14: error: redefinition of 'rz_debug_native_reg_profile'
    7 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
../librz/debug/p/native/android_x86_64.c:40:14: note: previous definition is here
   40 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
In file included from ../librz/debug/p/debug_native.c:33:
In file included from ../librz/debug/p/native/android_x86_64.c:44:
../librz/debug/p/native/reg.c:68:2: warning: Unsupported Kernel [-W#warnings]
   68 | #warning Unsupported Kernel
      |  ^
1 warning and 1 error generated.
[2247/2625] Compiling C object librz/debug/librz_debug.a.p/p_debug_gdb.c.o
[2248/2625] Compiling C object librz/debug/librz_debug.a.p/p_native_procfs.c.o
[2249/2625] Compiling C object librz/debug/librz_debug.a.p/p_debug_dmp.c.o
[2250/2625] Compiling C object librz/debug/librz_debug.a.p/p_native_linux_linux_debug.c.o
../librz/debug/p/native/linux/linux_debug.c:1183:2: warning: Android X86 does not support DRX [-W#warnings]
 1183 | #warning Android X86 does not support DRX
      |  ^
1 warning generated.

/usr/local/lib/android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/linux-x86_64/bin/armv7a-linux-androideabi33-clang -Ilibrz/debug/librz_debug.a.p -I. -I.. -Ilibrz -I../librz -Ilibrz/include -I../librz/include -I../librz/bin/format/elf -I../librz/bin/format/dmp -I../librz/bin/format/mdmp -I../librz/bin/format/pe -I../subprojects/rzgdb/include -I../subprojects/rzgdb/include/gdbclient -I../subprojects/rzgdb/include/gdbserver -Isubprojects/rzwinkd -I../subprojects/rzwinkd -Ilibrz/util/sdb/src -I../librz/util/sdb/src -I../librz/bin/format -I../librz/arch/isa -I../librz/arch/isa_gnu -Ilibrz/arch -I../librz/arch -I../librz/type/parser -I../subprojects/rzqnx/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O3 -DRZ_PLUGIN_INCORE=1 -DSUPPORTS_PCRE2_JIT -D_GNU_SOURCE --std=gnu99 -Werror=sizeof-pointer-memaccess -fPIC -MD -MQ librz/debug/librz_debug.a.p/p_debug_native.c.o -MF librz/debug/librz_debug.a.p/p_debug_native.c.o.d -o librz/debug/librz_debug.a.p/p_debug_native.c.o -c ../librz/debug/p/debug_native.c
In file included from ../librz/debug/p/debug_native.c:35:
In file included from ../librz/debug/p/native/android_arm.c:44:
../librz/debug/p/native/reg.c:7:14: error: redefinition of 'rz_debug_native_reg_profile'
    7 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
../librz/debug/p/native/android_arm.c:40:14: note: previous definition is here
   40 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
In file included from ../librz/debug/p/debug_native.c:35:
In file included from ../librz/debug/p/native/android_arm.c:44:
../librz/debug/p/native/reg.c:68:2: warning: Unsupported Kernel [-W#warnings]
   68 | #warning Unsupported Kernel
      |  ^
1 warning and 1 error generated.
[2250/2625] Compiling C object librz/debug/librz_debug.a.p/p_native_linux_linux_debug.c.o
../librz/debug/p/native/linux/linux_debug.c:1146:2: warning: print_fpu not implemented for this platform [-W#warnings]
 1146 | #warning print_fpu not implemented for this platform
      |  ^
../librz/debug/p/native/linux/linux_debug.c:1250:2: warning: getfpregs not implemented for this platform [-W#warnings]
 1250 | #warning getfpregs not implemented for this platform
      |  ^
../librz/debug/p/native/linux/linux_debug.c:1151:7: warning: variable 'showfpu' set but not used [-Wunused-but-set-variable]
 1151 |         bool showfpu = false;
      |              ^
../librz/debug/p/native/linux/linux_debug.c:1057:13: warning: unused function 'print_fpu' [-Wunused-function]
 1057 | static void print_fpu(void *f) {
      |             ^~~~~~~~~
4 warnings generated.

/usr/local/lib/android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android33-clang -Ilibrz/debug/librz_debug.a.p -I. -I.. -Ilibrz -I../librz -Ilibrz/include -I../librz/include -I../librz/bin/format/elf -I../librz/bin/format/dmp -I../librz/bin/format/mdmp -I../librz/bin/format/pe -I../subprojects/rzgdb/include -I../subprojects/rzgdb/include/gdbclient -I../subprojects/rzgdb/include/gdbserver -Isubprojects/rzwinkd -I../subprojects/rzwinkd -Ilibrz/util/sdb/src -I../librz/util/sdb/src -I../librz/bin/format -I../librz/arch/isa -I../librz/arch/isa_gnu -Ilibrz/arch -I../librz/arch -I../librz/type/parser -I../subprojects/rzqnx/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O3 -DRZ_PLUGIN_INCORE=1 -DSUPPORTS_PCRE2_JIT -D_GNU_SOURCE --std=gnu99 -Werror=sizeof-pointer-memaccess -fPIC -MD -MQ librz/debug/librz_debug.a.p/p_debug_native.c.o -MF librz/debug/librz_debug.a.p/p_debug_native.c.o.d -o librz/debug/librz_debug.a.p/p_debug_native.c.o -c ../librz/debug/p/debug_native.c
In file included from ../librz/debug/p/debug_native.c:37:
In file included from ../librz/debug/p/native/android_arm64.c:44:
../librz/debug/p/native/reg.c:7:14: error: redefinition of 'rz_debug_native_reg_profile'
    7 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
../librz/debug/p/native/android_arm64.c:40:14: note: previous definition is here
   40 | static char *rz_debug_native_reg_profile(RzDebug *dbg) {
      |              ^
In file included from ../librz/debug/p/debug_native.c:37:
In file included from ../librz/debug/p/native/android_arm64.c:44:
../librz/debug/p/native/reg.c:68:2: warning: Unsupported Kernel [-W#warnings]
   68 | #warning Unsupported Kernel
      |  ^
1 warning and 1 error generated.
[2247/2625] Compiling C object librz/debug/librz_debug.a.p/p_debug_qnx.c.o
[2248/2625] Compiling C object librz/debug/librz_debug.a.p/p_debug_dmp.c.o
[2249/2625] Compiling C object librz/debug/librz_debug.a.p/p_native_procfs.c.o
[2250/2625] Compiling C object librz/debug/librz_debug.a.p/p_debug_winkd.c.o
[2251/2625] Compiling C object librz/debug/librz_debug.a.p/p_native_linux_linux_debug.c.o
../librz/debug/p/native/linux/linux_debug.c:1146:2: warning: print_fpu not implemented for this platform [-W#warnings]
 1146 | #warning print_fpu not implemented for this platform
      |  ^
../librz/debug/p/native/linux/linux_debug.c:1250:2: warning: getfpregs not implemented for this platform [-W#warnings]
 1250 | #warning getfpregs not implemented for this platform
      |  ^
../librz/debug/p/native/linux/linux_debug.c:1151:7: warning: variable 'showfpu' set but not used [-Wunused-but-set-variable]
 1151 |         bool showfpu = false;
      |              ^
../librz/debug/p/native/linux/linux_debug.c:1057:13: warning: unused function 'print_fpu' [-Wunused-function]
 1057 | static void print_fpu(void *f) {
      |             ^~~~~~~~~
4 warnings generated.

tushar3q34 · 2024-09-04T15:14:16Z

Yeah the same issue as linux builds. I will commit the changes asap.

Refactor debug_native.c

57e063e

tushar3q34 requested review from yossizap and wargio as code owners September 2, 2024 22:35

github-actions bot added the RzDebug label Sep 2, 2024

wargio requested changes Sep 3, 2024

View reviewed changes

Required changes and bug fixes

37ff747

tushar3q34 requested review from ret2libc, thestr4ng3r, XVilka and kazarmy as code owners September 3, 2024 05:27

github-actions bot added RzBin RzAnalysis API RzCore RzCrypto RzType ELF PE Mach-O Windows ARM PPC RzEgg RzUtil java RzHash RzAsm-Disassembler labels Sep 3, 2024

github-actions bot removed Am29k AVR LH5801 LUA RX V810 labels Sep 3, 2024

wargio approved these changes Sep 3, 2024

View reviewed changes

XVilka approved these changes Sep 3, 2024

View reviewed changes

XVilka added the merge-when-green label Sep 3, 2024

This comment was marked as resolved.

Sign in to view

DMaroo mentioned this pull request Sep 4, 2024

Add wrap for native SoftFloat subproject and use the native dep #4605

Closed

5 tasks

This comment was marked as resolved.

Sign in to view

wargio mentioned this pull request Sep 4, 2024

Fix softfloat build issue when cross compiling #4606

Closed

5 tasks

DMaroo mentioned this pull request Sep 4, 2024

Add wrap for native SoftFloat subproject and use the native dep #4607

Merged

5 tasks

Merge branch 'dev' into dist-refactor-native-debug

8e34a24

XVilka removed the merge-when-green label Sep 4, 2024

XVilka requested changes Sep 4, 2024

View reviewed changes

Android Build Fixes

2b729c6

XVilka approved these changes Sep 4, 2024

View reviewed changes

XVilka merged commit 1057cf6 into rizinorg:dev Sep 4, 2024
51 checks passed

wargio mentioned this pull request Sep 5, 2024

Refactor native debugger #4581

Open

tushar3q34 deleted the dist-refactor-native-debug branch September 12, 2024 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor native debugger #4604

Refactor native debugger #4604

tushar3q34 commented Sep 2, 2024 •

edited by XVilka

Loading

tushar3q34 commented Sep 2, 2024 •

edited

Loading

wargio left a comment

wargio commented Sep 3, 2024

tushar3q34 commented Sep 3, 2024 •

edited

Loading

tushar3q34 commented Sep 3, 2024

wargio commented Sep 3, 2024

wargio commented Sep 3, 2024

wargio left a comment

XVilka left a comment

XVilka Sep 3, 2024

XVilka Sep 3, 2024

wargio Sep 4, 2024

XVilka Sep 3, 2024

XVilka Sep 3, 2024

XVilka commented Sep 3, 2024

XVilka commented Sep 3, 2024

tushar3q34 commented Sep 3, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

XVilka left a comment •

edited

Loading

tushar3q34 commented Sep 4, 2024 •

edited

Loading

Refactor native debugger #4604

Refactor native debugger #4604

Conversation

tushar3q34 commented Sep 2, 2024 • edited by XVilka Loading

SQUASH ME

tushar3q34 commented Sep 2, 2024 • edited Loading

wargio left a comment

Choose a reason for hiding this comment

wargio commented Sep 3, 2024

tushar3q34 commented Sep 3, 2024 • edited Loading

tushar3q34 commented Sep 3, 2024

wargio commented Sep 3, 2024

wargio commented Sep 3, 2024

wargio left a comment

Choose a reason for hiding this comment

XVilka left a comment

Choose a reason for hiding this comment

XVilka Sep 3, 2024

Choose a reason for hiding this comment

XVilka Sep 3, 2024

Choose a reason for hiding this comment

wargio Sep 4, 2024

Choose a reason for hiding this comment

XVilka Sep 3, 2024

Choose a reason for hiding this comment

XVilka Sep 3, 2024

Choose a reason for hiding this comment

XVilka commented Sep 3, 2024

XVilka commented Sep 3, 2024

tushar3q34 commented Sep 3, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

XVilka left a comment • edited Loading

Choose a reason for hiding this comment

tushar3q34 commented Sep 4, 2024 • edited Loading

tushar3q34 commented Sep 2, 2024 •

edited by XVilka

Loading

tushar3q34 commented Sep 2, 2024 •

edited

Loading

tushar3q34 commented Sep 3, 2024 •

edited

Loading

XVilka left a comment •

edited

Loading

tushar3q34 commented Sep 4, 2024 •

edited

Loading