Apply patches when used in radare2 #569

satk0 · 2024-10-02T18:12:56Z

I've made a PR that fixes all the issues we found in r2 as I said I would do in issue #559

Based on the following commits:

saghul

Thank you! I left a couple of comments.

saghul · 2024-10-02T18:36:56Z

cutils.c

@@ -1129,7 +1129,7 @@ void rqsort(void *base, size_t nmemb, size_t size, cmp_f cmp, void *opaque)

 /*---- Portable time functions ----*/

-#if defined(_MSC_VER)
+#if defined(_MSC_VER) || __MINGW32__


Why is this necessary? Is it to support the old mingw32 aka not mingw-w64?

Probably yea, that's @trufae's commit, here:
radareorg/radare2@86529d7
and sheesh its failing all the mingw checks, idk why :/

Mingw32 is really old and no longer maintained AFAIK, mingw-w64 (which also has a 32bit version, despite the name) has been the de facto MinGW for a while I think.

@trufae ?

Can either of you confirm which version of MinGW you're using?

My fear is we'd break mingw32 support since we don't run tests on it.

That's mingw-w64 which we already support, I don't think those changes are necessary.

Any chance you can try to build radare without them?

quickjs.c

saghul · 2024-10-02T18:52:20Z

quickjs.c

@@ -34561,7 +34561,7 @@ static JSString *JS_ReadString(BCReaderState *s)
    }
 #ifdef DUMP_READ_OBJECT
    if (check_dump_flag(s->ctx->rt, DUMP_READ_OBJECT)) {
-        bc_read_trace(s, "");  // hex dump and indentation
+        bc_read_trace(s, "%s", ""); // hex dump and indentation


Ha, nice! Can you please remove the silencer flag I added to CMake then?

satk0 · 2024-10-02T19:55:09Z

Done, maybe this time mingw-w64 will pass, I added a check for only mingw32, excluding w64 🙏

satk0 · 2024-10-02T19:56:51Z

Btw, thanks a lot for being so responsive, it is really motivating!

saghul · 2024-10-03T05:40:54Z

libbf.c

@@ -1710,6 +1710,11 @@ static int __bf_div(bf_t *r, const bf_t *a, const bf_t *b, limb_t prec,
        slimb_t d;

        na = n + nb;
+
+        if (na >= (SIZE_MAX / sizeof(limb_t)) - 1) {


This fails on Windows, since IIRC the limb bits are fewer there.

D:/a/quickjs/quickjs/libbf.c:1714:16: error: comparison is always false due to limited range of data type [-Werror=type-limits]

Huh, that's rough, will try to play with it more

saghul · 2024-10-03T05:41:42Z

cutils.c

@@ -1129,7 +1129,7 @@ void rqsort(void *base, size_t nmemb, size_t size, cmp_f cmp, void *opaque)

 /*---- Portable time functions ----*/

-#if defined(_MSC_VER)
+#if defined(_MSC_VER) || (__MINGW32__ && !defined(__MINGW64__))


Come to think of it, we could use this everywhere on windows, perhaps that's a simpler thing, let's try it please.

Yea I need to find a macro that distinguish mingw32 from Mingw-w64

I was thinking on the opposite direction, as in, to use this implementation also in MinGW, if possible.

Ooh, allright, I could try

Btw can you plz give me any hint about the limb bits on windows?

See

quickjs/libbf.h

Line 34 in 1eb9608

#if INTPTR_MAX >= INT64_MAX && !defined(_WIN32) && !defined(__TINYC__)

the size of limb_t is platform dependent, so perhaps an extra if-def is necessary to check.

Thanks a lot, I will try this today as well as your __MINGW32__ idea

satk0 · 2024-10-04T21:19:14Z

Done, letme know if that's what you expected. Sorry for pushing it so late 😓

saghul · 2024-10-04T23:37:29Z

libbf.c

@@ -1710,6 +1710,11 @@ static int __bf_div(bf_t *r, const bf_t *a, const bf_t *b, limb_t prec,
        slimb_t d;

        na = n + nb;
+
+        if (na >= (SIZE_MAX / sizeof(limb_t)) - 1) && !defined(_WIN32) {


I think this check should be surrounded by some ifdef sizeof (limb_t) > sizeof(uint64_t) instead

W8, I haven't closed the brackets here, that's why its all failing, sorry. I will fix it

saghul · 2024-10-04T23:39:16Z

cutils.c

@@ -1129,7 +1129,7 @@ void rqsort(void *base, size_t nmemb, size_t size, cmp_f cmp, void *opaque)

 /*---- Portable time functions ----*/

-#if defined(_MSC_VER)
+#if defined(_MSC_VER) || __MINGW32__


Can either of you confirm which version of MinGW you're using?

My fear is we'd break mingw32 support since we don't run tests on it.

satk0 · 2024-10-05T20:24:01Z

Chck now, sry for this obv mistake I've made

satk0 · 2024-10-06T10:16:51Z

Allright, some checks are still failing, I think that changing this comparison back to a simple !defined(_WIN32) would solve it.

saghul · 2024-10-06T10:50:33Z

Allright, some checks are still failing, I think that changing this comparison back to a simple !defined(_WIN32) would solve it.

I think so too!

satk0 · 2024-10-06T13:27:53Z

Ok, letsee how it goes ❗

saghul · 2024-10-06T14:19:08Z

Ah I thought you meant something else.

I think what you want to check for then is if LIMB_LOG2_BITS is 6 or not.

satk0 · 2024-10-06T17:48:03Z

Oh, ok so I'd check for if LIMB_LOG2_BITS == 6 instead of _WIN32

Thanks for the quick review btw 😃

satk0 · 2024-10-06T17:53:03Z

Another thing is that I should check if defined(__MINGW32__) instead of only if __MINGW32__

saghul · 2024-10-06T18:13:04Z

Another thing is that I should check if defined(__MINGW32__) instead of only if __MINGW32__

I'd rather see it gone. I think it's not necessary since both us and radars are using mingw-w64.

satk0 · 2024-10-06T18:41:11Z

Ok, So I contacted pancake on it and he said this:

So yea, we use mingw32 in r2, I just asked pancake on mingw version, not mingw32 (I wasn't precise enough). So he answered with the mingw-w64 one XD.

But nevertheless, I'll go with your suggestion to omit this and see what happens with mingw32 on ubuntu24, thanks!

saghul · 2024-10-06T19:01:28Z

Sounds good!

satk0 · 2024-10-06T19:41:18Z

OK done, hopefully nothing will break 🙏

saghul · 2024-10-07T07:08:12Z

Looks good! Let's see if the CI is green!

satk0 · 2024-10-07T08:51:32Z

Hmm, seems I have to put this limb thing inside #if LIMB_LOG2_BITS == 6 cuz C is still calculating it cuz operator precedence 😖

satk0 · 2024-10-07T12:38:50Z

Ok done, chck it now

saghul · 2024-10-07T20:34:37Z

Squash - rebased them and landed! #582

saghul · 2024-10-07T20:34:56Z

Thanks for persevering! ❤️

saghul reviewed Oct 2, 2024

View reviewed changes

saghul reviewed Oct 3, 2024

View reviewed changes

satk0 force-pushed the r2-patches branch from 75d450d to 3907eff Compare October 4, 2024 21:18

saghul reviewed Oct 4, 2024

View reviewed changes

satk0 force-pushed the r2-patches branch from 511c672 to e30ab3e Compare October 5, 2024 20:22

satk0 force-pushed the r2-patches branch from 0ca98f0 to e1baab2 Compare October 6, 2024 19:39

saghul approved these changes Oct 7, 2024

View reviewed changes

satk0 added 7 commits October 7, 2024 14:37

Follow declaration of variables before for loop

c3f4303

Fix gcc warnings

f23a84c

Fix out-of-bound write in libbf.c

0a7ef6a

Make msvc compile when atomics experimental feature is not set

a69a498

Don't include pthread on wasi|enscripten

a6ead4f

Fix mingw32 build

fc907f7

Changes according to review

4f64454

satk0 added 7 commits October 7, 2024 14:37

Fix detection of mingw32

4cc548d

Fix different limb_t size on win, potential fix to mingw

fd8e5b8

Don't include sys/time.h for mingw32

9eddd97

Fix limb_t

556895c

Use #if not def _WIN32

1b66138

Get rid of unnecessary __MINGW32__, fix limb

818c2ed

Use #if instead of if

a25eca2

satk0 force-pushed the r2-patches branch from e1baab2 to a25eca2 Compare October 7, 2024 12:37

saghul closed this Oct 7, 2024

Apply patches when used in radare2 #569

Apply patches when used in radare2 #569

Conversation

satk0 commented Oct 2, 2024 • edited Loading

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satk0 commented Oct 2, 2024

satk0 commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satk0 Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satk0 commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satk0 commented Oct 5, 2024

satk0 commented Oct 6, 2024 • edited Loading

saghul commented Oct 6, 2024

satk0 commented Oct 6, 2024

saghul commented Oct 6, 2024

satk0 commented Oct 6, 2024 • edited Loading

satk0 commented Oct 6, 2024 • edited Loading

saghul commented Oct 6, 2024

satk0 commented Oct 6, 2024

saghul commented Oct 6, 2024

satk0 commented Oct 6, 2024 • edited Loading

saghul commented Oct 7, 2024

satk0 commented Oct 7, 2024

satk0 commented Oct 7, 2024

saghul commented Oct 7, 2024

saghul commented Oct 7, 2024

satk0 commented Oct 2, 2024 •

edited

Loading

satk0 Oct 3, 2024 •

edited

Loading

satk0 commented Oct 6, 2024 •

edited

Loading

satk0 commented Oct 6, 2024 •

edited

Loading

satk0 commented Oct 6, 2024 •

edited

Loading

satk0 commented Oct 6, 2024 •

edited

Loading