Convert all source code to C89 #81
Conversation
|
Personally I think this is really cool, but I feel like it'll cause some more political issues now that there are more contributors on board. I've only skimmed the diffs, I'll have to take a closer look at what exactly changed to make any judgements. Would be great to support C89, given that many memory-restricted systems do operate on ancient C, and that's kind of the target hardware of this project. |
|
What about creating a new branch ? |
| #include <ws2tcpip.h> | ||
| #else | ||
| #include <sys/socket.h> | ||
| #include <arpa/inet.h> |
There was a problem hiding this comment.
Android's arpa/inet.h file is not compatible with c89 because one of the other headers it imports contains an inline keyword, which produces this compilation error in any c89 code intended to support Android that attempts to import arpa/inet.h:
swab.h:21:8: error: unknown type name 'inline'
21 | static inline __attribute__((__const__)) __u32 __fswahw32(__u32 val) {
More information:
-
https://android.googlesource.com/platform/bionic/+/master/libc/kernel/#bionic-kernel-header-files
- "The ‘clean headers’ only contain type and macro definitions, with the exception of a couple static inline functions used for performance reason"
Minimal reproducible example using the unmodifed current official stable release of the Android NDK (there also exist unofficial/custom builds of the Android NDK, which I also tested one of and also reproduced the problem there, but it's probably helpful to show that the problem is reproducible with the official Android NDK and a very minimal reproducible example code snippet)
expand to view MRE of Android's arpa/inet.h being incompatible with c89
tacokoneko@CORSAIR /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin $ echo "#include <arpa/inet.h>" > test.c
tacokoneko@CORSAIR /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin $ ./aarch64-linux-android35-clang -c test.c
tacokoneko@CORSAIR /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin $ ./aarch64-linux-android35-clang -c test.c -std=gnu89
tacokoneko@CORSAIR /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin $ ./aarch64-linux-android35-clang -c test.c -std=c89
In file included from test.c:1:
In file included from /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/arpa/inet.h:34:
In file included from /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/netinet/in.h:37:
In file included from /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/in.h:233:
In file included from /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/aarch64-linux-android/asm/byteorder.h:12:
In file included from /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/byteorder/little_endian.h:17:
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:21:8: error: unknown type name 'inline'
21 | static inline __attribute__((__const__)) __u32 __fswahw32(__u32 val) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:21:47: error: expected ';' after top level declarator
21 | static inline __attribute__((__const__)) __u32 __fswahw32(__u32 val) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:24:8: error: unknown type name 'inline'
24 | static inline __attribute__((__const__)) __u32 __fswahb32(__u32 val) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:24:47: error: expected ';' after top level declarator
24 | static inline __attribute__((__const__)) __u32 __fswahb32(__u32 val) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:44:8: error: unknown type name 'inline'
44 | static inline __u32 __swahw32p(const __u32 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:44:20: error: expected ';' after top level declarator
44 | static inline __u32 __swahw32p(const __u32 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:47:8: error: unknown type name 'inline'
47 | static inline __u32 __swahb32p(const __u32 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:47:20: error: expected ';' after top level declarator
47 | static inline __u32 __swahb32p(const __u32 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:50:8: error: unknown type name 'inline'
50 | static inline void __swab16s(__u16 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:59:8: error: unknown type name 'inline'
59 | static inline void __swahw32s(__u32 * p) {
| ^
/mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/linux/swab.h:62:8: error: unknown type name 'inline'
62 | static inline void __swahb32s(__u32 * p) {
| ^
11 errors generated.
tacokoneko@CORSAIR /mnt/Storage/Files/Application_Files/android-sdk/ndk/28.2.13676358/toolchains/llvm/prebuilt/linux-x86_64/bin $
It is compatible with gnu89, though, which supports the inline keyword.
The current main branch of bareiron is fully compatible with Android without requiring any modifications at all, most likely by chance without having been officially tested (if anyone is unsure how that can be, they can ask me for more details regarding the entrypoint and build process), so this would unfortunately break the de facto working Android support currently in the main branch.
If preserving the de facto working Android support currently in the main branch is not a priority for the project goals of upstream (p2r3/bareiron), then this might not matter.
There was a problem hiding this comment.
A high-profile project I found, libgit2, when exposed to this problem, took the path of making the C standard default to c99 if an Android target is detected and default to c89 (also known as c90, which is the exact same language) instead if an Android target is not detected:
however, that might be a bit too overengineered and CMake-related of a solution for a small build.sh project like bareiron, which doesn't currently have any need to start developing a convoluted tree of conditional build settings related to Android.
There was a problem hiding this comment.
Correct me if I'm wrong, but since C is backwards-compatible, it sounds like this is just a matter of changing a compiler flag when targeting Android. That doesn't sound like that deep of an issue.
Regardless, wouldn't building for Android require changing a bit more than just one compiler flag anyway?
Still, thanks for pointing this out.
Edit: Would the handling in globals.h not work for this?
https://github.com/techflashYT/bareiron/blob/0bda9549ad316bf80e6a845f6ef99c86da88ed2b/include/tools.h#L8-L24
There was a problem hiding this comment.
Correct me if I'm wrong, but since C is backwards-compatible, it sounds like this is just a matter of changing a compiler flag when targeting Android. That doesn't sound like that deep of an issue.
Correct, it's not a deep issue. The only thing that really "breaks the default" in this PR is the line in build.sh that sets -std=c89. Manually editing build.sh and removing the -std=c89 string before compilation targeting Android is the only thing required for Android users to manually work around the problem - but if it's introduced as the default in the main branch, it does mean that all potential Android users would need to realize that's what they need to do to work around the error!
Edit: Would the handling in globals.h not work for this?
https://github.com/techflashYT/bareiron/blob/0bda9549ad316bf80e6a845f6ef99c86da88ed2b/include/tools.h#L8-L24
Unfortunately, I can confirm through testing that something about the current tools.h preprocessor condition tree you linked is not sufficient to prevent this problem on Android currently (but adjusting some of its conditions might be able to fix the problem, or might not, I'm not sure at the moment). If you would like, when I have some time I could investigate that and try to see if adjusting those conditions can resolve the problem.
Regardless, wouldn't building for Android require changing a bit more than just one compiler flag anyway?
I've already been testing the main branch unmodified on Android, and it works fine. It's only this PR which introduces the first Android-related problem I've noticed in the project so far, and it's quite a minor problem.
If you're unsure about how I'm able to use bareiron arguably "unmodified" on Android, and would like to see a video recording of myself performing my zero-to-done build steps in real life from a 3rd-person camera angle (which would make everything very clear very quickly and also familiarize everyone here with what I'm talking about if that's a priority), then I could do that, it just might take a while to record and edit the video.
There was a problem hiding this comment.
That's fine, I trust you - you seem to know what you're talking about.
The main reason I wouldn't want to make the -std=c89 flag opt-in is because it'd make it more confusing for developers. If we're trying to support ancient C, then we likely want all future testing and development to assume the C89 standard, lest someone write incompatible code.
Worst case scenario, if no technical solution is found, I don't think it'd be too bad to check for Android as a build target in build.sh, or even just mention it in the README.
There was a problem hiding this comment.
The main reason I wouldn't want to make the -std=c89 flag opt-in is because it'd make it more confusing for developers. If we're trying to support ancient C, then we likely want all future testing and development to assume the C89 standard, lest someone write incompatible code.
That's a really good idea, especially for the upstream repository, I agree, and if there's no other way to fix this, then it's fine to leave it as-is, and most likely if the project finds popularity on Android, someone will make a fork or guide for using it there anyway, like would be done for other embedded systems that aren't officially supported, and people will not have too much trouble getting it to work.
I think the # define inline /* nothing */ code you linked should in fact be able to function as a workaround for this. I think it's just that there is something incorrect (or at least insufficient for Android) about the way it's written in this PR.
I've troubleshooted it for a little while, and I believe the problem is that the tools.h, or at least some part of its contents such as the # define inline /* nothing */, would need to be moved to before all instances of #include <arpa/inet.h>, rather than after as they currently are.
Here is a quick and messy patch I made which does this - however, unfortunately, using this workaround line-for-line as I've shown places local includes above system includes in several files, which you, as an experienced C programmer, are aware would not be an ideal situation and which could even sometimes lead to errors on some other, non-Android platforms or if certain changes happen at some point in the headers, so to keep all local includes imported below system includes in the .c files, it would probably be necessary to reorganize this potential solution to be implemented differently, maybe by reorganizing the headers so that arpa/inet.h is only explicitly imported in one header, with the preprocessor conditional tree for #define inline right above it, and then that header imported everywhere else that arpa/inet.h is needed.
So to summarize, here are the two quick and dirty patches that are each their own complete workaround to make this PR work on Android without errors (keeping in mind that the main branch currently works without errors or modifications)
- Option 1:
diff --git a/src/globals.c b/src/globals.c
index 731e7fa..ba3b952 100644
--- a/src/globals.c
+++ b/src/globals.c
@@ -1,5 +1,6 @@
#include <stdio.h>
#include <stdint.h>
+#include "tools.h"
#ifdef _WIN32
#include <winsock2.h>
#include <ws2tcpip.h>
diff --git a/src/main.c b/src/main.c
index 6f2e824..7010241 100644
--- a/src/main.c
+++ b/src/main.c
@@ -8,6 +8,7 @@
#define CLOCK_REALTIME 0
#endif
+#include "tools.h"
#ifdef ESP_PLATFORM
#include "freertos/FreeRTOS.h"
#include "freertos/task.h"
@@ -32,7 +33,6 @@
#endif
#include "globals.h"
-#include "tools.h"
#include "varnum.h"
#include "packets.h"
#include "worldgen.h"
diff --git a/src/packets.c b/src/packets.c
index 63e6469..45f0756 100644
--- a/src/packets.c
+++ b/src/packets.c
@@ -1,6 +1,7 @@
#include <stdio.h>
#include <string.h>
+#include "tools.h"
#ifdef ESP_PLATFORM
#include "lwip/sockets.h"
#include "lwip/netdb.h"
@@ -16,7 +17,6 @@
#endif
#include "globals.h"
-#include "tools.h"
#include "varnum.h"
#include "registries.h"
#include "worldgen.h"
diff --git a/src/tools.c b/src/tools.c
index fa45f2c..79fdee1 100644
--- a/src/tools.c
+++ b/src/tools.c
@@ -2,6 +2,7 @@
#include <string.h>
#include <errno.h>
+#include "tools.h"
#ifdef ESP_PLATFORM
#include "lwip/sockets.h"
#include "lwip/netdb.h"
@@ -29,7 +30,6 @@
#include "globals.h"
#include "varnum.h"
#include "procedures.h"
-#include "tools.h"
diff --git a/src/varnum.c b/src/varnum.c
index 2a51cde..7360cf2 100644
--- a/src/varnum.c
+++ b/src/varnum.c
@@ -1,4 +1,5 @@
#include <stdint.h>
+#include "tools.h"
#ifdef _WIN32
#include <winsock2.h>
#include <ws2tcpip.h>
@@ -9,7 +10,6 @@
#include "varnum.h"
#include "globals.h"
-#include "tools.h"
int32_t readVarInt (int client_fd) {
int32_t value = 0;- Option 2:
diff --git a/build.sh b/build.sh
index 486a363..2fad5db 100755
--- a/build.sh
+++ b/build.sh
@@ -41,5 +41,5 @@ for arg in "$@"; do
done
rm -f "bareiron$exe"
-$compiler src/*.c -std=c89 -O3 -Iinclude -o "bareiron$exe" $windows_linker
+$compiler src/*.c -O3 -Iinclude -o "bareiron$exe" $windows_linker
"./bareiron$exe"- Option 3: do nothing and allow Android users to figure out one of the two above workarounds on their own (this is fine since upstream has no obligation to officially support Android, and not really inconvenient for most Android users either)
There was a problem hiding this comment.
When I check what others say, it turns out that the "objectively correct" include order may be more dependent on what the specific project is than I previously thought, with many pointing out that there is conflicting information present in various places according to the code style conventions of various organizations, for example, how the include order recommended by some of the answers in this thread:
https://stackoverflow.com/questions/2762568/c-c-include-header-file-order
conflicts with the include order recommended by Google's code style guide:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
however, I would say that keeping the convention "consistent within the project" is probably an important goal for general readability and maintainability, and your project currently includes system headers above local headers everywhere as far as I noticed, so you shouldn't feel the need to mix up your header include order convention or rethink it entirely just to support Android.
There was a problem hiding this comment.
Regardless, wouldn't building for Android require changing a bit more than just one compiler flag anyway?
I am sorry if this is TMI and not interesting to you, but I am not really a person who makes wild claims without backing them up, so in case you want to see what it looks like, I have now finished recording a full length uncut video of myself clean-installing bareiron's main branch on Android two different ways that arguably do not actually edit any bareiron code.
https://www.youtube.com/watch?v=S5vvBz47GGA
I have also written a commented plaintext form of almost all shell commands typed in the video in the description, which should be almost as clear, but if any step of either of the two methods seems unclear, the uncut video should make it very clear. I rehearsed the video a little bit but not really that much, and I did make some mistakes in the video, but rather than cut out my mistakes, I decided to leave in everything showing what I messed up and exactly what I did to troubleshoot and fix my mistakes, because those parts could definitely be helpful if anyone ever tries to do the same thing and gets stuck on the same common accidents that happened to me.
also, if you still want to set the main branch to c89 by default eventually then please don't interpret this as an argument against that, if that happens then I think most people can figure out how to manually set the C standard.
|
What's the status on this? You said on Discord you were planning to address the warnings from compiling with |
Yes, sorry, I do indeed plan to rebase on the latest code and continue this. I've just been busy with some IRL stuff recently. I hope I can get it finished some time this week. |
|
I think I've almost got the old version I was working on finished (sorry for such the long wait), then I'll just need to rebase onto the latest code. |
|
Git screwed it up and closed it when I force pushed, sorry 😅 |
|
Also note that this should largely correct much of the big discussion above, since it makes targetting C89 optional, and not the default, for platforms whose internal headers aren't C89-compliant. I also know that the Nintendo GameCube/Wii and Switch homebrew toolchains (devkitPPC/libogc and devkitA64/libnx) are also vulnerable to this off the top of my head, so it's definetly a problem that extends beyond Android. As a general rule of thumb the actual bareiron code would need to be kept C89 compliant, but outside per-platform code may or may not work in a strictly-C89 setup, so trying to force all platforms to compile like that would cause more problems than it's worth. This way, platforms that only support C89 can still work, whilst newer platforms that may rely on newer features aren't negatively affected. |
| fi | ||
| ;; | ||
| --strict-c89) | ||
| c_std="-std=c89 -pedantic -Werror=declaration-after-statement" |
There was a problem hiding this comment.
In order to resolve the concern that p2r3 had about the default for most people needing to be C89 so that all future code is verified to be C89-compatible, if that's desired, I could suggest adding these arguments to the invocation of cosmocc that's currently in the GitHub Actions workflow of this repository, so that the official cosmocc target that's tested in GitHub Actions here would be able to fail the CI and print relevant errors in PRs that would accidentally add C89-incompatible code,
but that's just my suggestion, not sure what p2r3 and other contributors would think of it. That idea would also be the most useful if GitHub Actions in this repo were eventually configured to test all PRs before merging and not only after merging, which it currently isn't, not sure if that's something p2r3 would think should be enabled or not.
bareiron/.github/workflows/build.yml
Lines 3 to 7 in b2aed79
There was a problem hiding this comment.
The problem with this is that, since C89 doesn't have inline, it'll technically produce less efficient code. I hack around this with a bunch of #ifdef's to determine if the platform has any way to do inline functions, and if not, just stub out the keyword. But, for real "production-grade" builds, one would probably want to have inline function support (for however much difference it makes, probably not that much). I'm not against test-building it in strict C89 mode to ensure that nothing is catastrophically broken, but I'd probably advise against making it the default for final builds.
There was a problem hiding this comment.
Ahh ok, I see, well in that case the solution is probably for there to be two builds in CI, an unoptimized C89-mode build (to test for C89 compatibility in CI) and an optimized C99+-mode build (to build any release binaries).
The project's code is very small and fast to compile in GitHub Actions, so two builds wouldn't bloat the CI time very much.
4 participants
Being in C89 has a few extra limitations, but allows theoretically compiling on basically every platform in existance.
Actually becoming supported on such platforms will require more changes later - this just converts all the source code as a sort of "stage 1".
I've confirmed this to compile in Linux with
gcc -std=c89. It is possible that I have missed something. I don't have an ESP32, so I can't easily test those codepaths to make sure that I haven't broken something. I could order one, but it'll still take a while to get here, and I wanted to get this going as soon as possible.