Fix compilation on macOS#189
Conversation
|
This is great! Thank you @waterlens for working on this 🎉 I ran these commands on my Mac (8-core M2, 2022, Monterey 12.5). I was able to compile a few examples and benchmarks with reasonable speedups. As far as I can tell right now, everything seems to be working! I've also confirmed that existing compilation (on x86 Ubuntu 20.04) still seems to work fine. (Something to keep in mind: we'll eventually need to do some more thorough testing on MacOS. The runtime system and scheduler were designed with x86-TSO in mind, and it's possible that there are some unexpected data races lurking under the weaker arm memory model.) @MatthewFluet, if you have a chance, I'd love to hear your thoughts on this PR. Perhaps all of the MacOS-specific elements should live in |
|
Glad to know that it works. Yeah, it's interesting to see if the runtime can function with a weaker memory model. Thank you for reminding me about the directory for platform-specfic code. I think we could place the code in |
|
Yes, I think By the way, just curious, where does this implementation of |
|
As far as I know, the only standard open-source implementation is at glibc. My implementation refers to both glibc and musl. But I just realized this may involve GPL license related issues. Instead of re-implementing GNU extension for other platforms, I think it would be preferable to switch from drand48_r to another random number generator. What do you think about it? |
|
A suggestion is to add a (trivial) implementation of dance whenever the current thread's I see the Finally, why the additional function declarations in |
|
The forward declarations are for functions surrounded by |
|
I played with this a little and it seems like the only forward declaration that I need is diff --git a/include/c-main.h b/include/c-main.h
index 698656c2c..6826e5829 100644
--- a/include/c-main.h
+++ b/include/c-main.h
@@ -14,10 +14,6 @@
#include "c-common.h"
void Parallel_run (void);
-int32_t Proc_processorNumber (GC_state s);
-void Proc_waitForInitialization (GC_state s);
-void HH_EBR_enterQuiescentState(GC_state s);
-void relayerLoop(GC_state s);With the above change everything still seems to work properly. If I remove the Perhaps this is due to a difference between gcc and clang? I'm guessing here, but it seems like |
|
Confirmed: on So, it does appear to be a difference between gcc and clang. I think previously we were just lucky that gcc decided not to complain. TLDR: I think the forward declaration of diff --git a/include/c-main.h b/include/c-main.h
index 698656c2c..6826e5829 100644
--- a/include/c-main.h
+++ b/include/c-main.h
@@ -14,10 +14,6 @@
#include "c-common.h"
void Parallel_run (void);
-int32_t Proc_processorNumber (GC_state s);
-void Proc_waitForInitialization (GC_state s);
-void HH_EBR_enterQuiescentState(GC_state s);
-void relayerLoop(GC_state s); |
|
The inconsistency looks weird. If I understand correctly, the only file included in the generated C source is /* c-main.h */
#define MLTON_GC_INTERNAL_TYPES
#define MLTON_GC_INTERNAL_BASIS
#include "platform.h"
/* platform.h */
#include "gc.h"
/* gc.h */
#include "gc/processor.h"
/* gc/processor.h */
#if (defined (MLTON_GC_INTERNAL_FUNCS))
int32_t Proc_processorNumber (GC_state s);
void Proc_waitForInitialization (GC_state s);
void Proc_signalInitialization (GC_state s);
bool Proc_isInitialized (GC_state s);
#endif /* (defined (MLTON_GC_INTERNAL_FUNCS)) */It looks like the declarations of called functions are not visible when mpl is invoking the C compiler because the reason I mentioned above. And that seems to explain why I got the following errors I tried it in a clean docker image and got the similar errors. |
|
I'm also now getting the undeclared function errors on Linux with gcc. I certainly wasn't a couple of weeks ago when I submitted #185 and #188. Something that has changed in that time is that I upgraded my machines from Fedora 39 (gcc 13.3.1) to Fedora 40 (gcc 14.1.1); and from https://gcc.gnu.org/gcc-14/porting_to.html#implicit-function-declaration, we see that it is now an error to call a function that has not been declared. I think that the right thing to do would be to properly expose those functions in |
|
Aha! Nice catch Matthew. I was having trouble reproducing, but after updating |
This is a relic of an old lock-based synchronization strategy for hierarchical heaps which was deprecated a few years ago (deprecated since 2019; see e.g. e0404a7). IIRC, the deprecated strategy associated a rwlock with each hierarchical heap, and used a randomized backoff algorithm to reduce contention. The randomization came from `s->tlsObjects.drand48_data` and `drand48_r(...)` which are no longer used. One benefit of removing these now is that #189 is easier. The best portability is no code at all.
@waterlens I looked into this and noticed that
I agree with Matthew. This can be done in the appropriate |
|
I prefer to merge the changes in #191 first, then I will edit this PR, removing the |
|
Sounds good! Just merged #191. |
waterlens
force-pushed
the
fix-macos-compilation
branch
from
1c34035 to
ec5d271
Compare
|
I edited some files to change the guard from looking at Or maybe these guards should just be removed? It will look like a bit of inconsistent for me because most functions in other files are put in such guards. |
|
Looking it over, the usage of these guards throughout the runtime codebase is definitely inconsistent. I think it's about time we do some cleanup of the runtime system, but for this PR I say: let's not worry about it. Putting these under I'll do some final testing but then I think we're probably good to merge. |
3 participants
I followed the instructions in #181.
The required
rand48_rseries functions were re-implemented as it is a GNU extension and Linux-only.The call to
getrusage(RUSAGE_THREAD, ...)is also replaced on this platform.