picotool icon indicating copy to clipboard operation
picotool copied to clipboard

Make the new Picotool compile on FreeBSD again

Open arg08 opened this issue 1 year ago • 6 comments

There are two minor issues preventing the new Picotool building on FreeBSD out-of-the-box:

  1. portable_endian.h seems to be out-of date or just plain wrong - I've seen the same thing needing to be patched in other projects ported to FreeBSD. The macros be16toh() etc are already defined in the native sys/endian.h (and note that these definitions are in a section already ifdefed for FreeBSD/NetBSD). I don't have a NetBSD system to check if it's spurious there also.

This patch fixes it:

diff --git a/elf/portable_endian.h b/elf/portable_endian.h
index 4286f91..42dc4cd 100644
--- a/elf/portable_endian.h
+++ b/elf/portable_endian.h
@@ -56,6 +56,7 @@
 
 #      include <sys/endian.h>
 
+#if !defined(__FreeBSD__)
 #      define be16toh(x) betoh16(x)
 #      define le16toh(x) letoh16(x)
 
@@ -64,6 +65,7 @@
 
 #      define be64toh(x) betoh64(x)
 #      define le64toh(x) letoh64(x)
+#endif
 
 #elif defined(__WINDOWS__)

  1. FreeBSD's C++ environment currently doesn't supply a <cuchar> include. I haven't researched why picotool is wanting this, but it's already excluded for __APPLE__, so it seems reasonably safe to exclude it for FreeBSD as well:
diff --git a/main.cpp b/main.cpp
index 4f5b2fe..4b3619b 100644
--- a/main.cpp
+++ b/main.cpp
@@ -14,7 +14,7 @@
 #include <csignal>
 #include <cstdio>
 #include <regex>
-#ifndef __APPLE__
+#if !defined(__APPLE__) && !defined(__FreeBSD__)
 #include <cuchar>
 #endif
 #include <cwchar>

With these two patches, the resulting Picotool appears to build and work satisfactorily.

arg08 avatar Aug 29 '24 21:08 arg08

See #118 ?

lurch avatar Aug 29 '24 21:08 lurch

Ah OK, someone beat me to it. I checked the other issues but not the open pull requests.

That PR is only fixing the portable_endian.h issue, not <cuchar> . Maybe he is using a newer FreeBSD that now has <cuchar>.

arg08 avatar Aug 29 '24 22:08 arg08

I'm happy to take these patches, but as I mentioned on that PR

Would a better fix be to treat FreeBSD the same as OpenBSD, as suggested by this comment?

Could you check if that fix works too - it's slightly neater than what you and #118 are suggesting, but if it doesn't work then I'm happy to merge #118

will-v-pi avatar Aug 30 '24 14:08 will-v-pi

Well, it wasn't entirely clear which of those alternatives you had in mind - there were several, and their starting point isn't necessarily the same upstream portable_endian.h as you currently have.

I tried this one:

diff --git a/elf/portable_endian.h b/elf/portable_endian.h
index 4286f91..14fdbdc 100644
--- a/elf/portable_endian.h
+++ b/elf/portable_endian.h
@@ -43,7 +43,7 @@
 #      define __LITTLE_ENDIAN LITTLE_ENDIAN
 #      define __PDP_ENDIAN    PDP_ENDIAN
 
-#elif defined(__OpenBSD__)
+#elif defined(__OpenBSD__) || defined(__FreeBSD__)
 
 #      include <endian.h>
 
@@ -52,7 +52,7 @@
 #      define __LITTLE_ENDIAN LITTLE_ENDIAN
 #      define __PDP_ENDIAN    PDP_ENDIAN
 
-#elif defined(__NetBSD__) || defined(__FreeBSD__) || defined(__DragonFly__)
+#elif defined(__NetBSD__) || defined(__DragonFly__)
 
 #      include <sys/endian.h>
 

and that worked, but gave compiler warnings (already defined) for those four #defines in the OpenBSD section. So I'm not keen on this one.

Also linked from that thread was this variation which is more verbose but looks better formed:

diff --git a/elf/portable_endian.h b/elf/portable_endian.h
index 4286f91..347dd50 100644
--- a/elf/portable_endian.h
+++ b/elf/portable_endian.h
@@ -43,27 +43,22 @@
 #      define __LITTLE_ENDIAN LITTLE_ENDIAN
 #      define __PDP_ENDIAN    PDP_ENDIAN
 
-#elif defined(__OpenBSD__)
-
-#      include <endian.h>
-
-#      define __BYTE_ORDER    BYTE_ORDER
-#      define __BIG_ENDIAN    BIG_ENDIAN
-#      define __LITTLE_ENDIAN LITTLE_ENDIAN
-#      define __PDP_ENDIAN    PDP_ENDIAN
-
-#elif defined(__NetBSD__) || defined(__FreeBSD__) || defined(__DragonFly__)
+#elif defined(__OpenBSD__) || defined(__NetBSD__) || defined(__FreeBSD__) || defined(__DragonFly__)
 
 #      include <sys/endian.h>
 
+#ifndef be16toh
 #      define be16toh(x) betoh16(x)
+#endif
+#ifndef le16toh
 #      define le16toh(x) letoh16(x)
-
+#endif
+#ifndef be32toh
 #      define be32toh(x) betoh32(x)
+#endif
+#ifndef le32toh
 #      define le32toh(x) letoh32(x)
-
-#      define be64toh(x) betoh64(x)
-#      define le64toh(x) letoh64(x)
+#endif
 
 #elif defined(__WINDOWS__)
 

This one compiles cleanly.

So pick your poison.....

arg08 avatar Aug 30 '24 19:08 arg08

I meant the first variation, but if that's giving compiler warnings then the second variation (with ##ifdefs around each #define) looks like the best choice - could you submit a PR with that second variation for the portable_endian.h patch, and your patch for cuchar, and then I'll get that merged?

will-v-pi avatar Sep 02 '24 10:09 will-v-pi

PR#133

arg08 avatar Sep 02 '24 20:09 arg08