Open gushromp opened 2 years ago
get the non-mac changes merged in a separate PR - ie there's a number of nice fixes and improvements here already, like the handling of the C++ flags, versions etc that prepare us for multi-platform support
I'll be glad to do so, but please help me understand the scope of the changes you'd like to see in a separate PR - since most of those are when defined(linux)/when defined(macosx)
etc. Does that mean you'd like me to leave the conditional compilation/shell script stuff for linux only in that other PR?
add mac to CI in a matrix build - to begin with, mac should not result in a hard fail (it should continue to show a green checkmark even if mac fails, but it's good to run it at least, to get an idea)
Will do!
negotiate a fix for the constant issue - the important part is that it's autogenerated - then it can live either in upstream or in the .ll file, either is fine
The posix constants fall back to posix_other_consts.nim
on Mac by default whereas for Linux it uses the output of detect
. I'm not sure why this is the case, maybe @Araq can shed some light on this? Either way I'm more than happy to use detect
for both Arm64/X86_64 on Mac and use its output for these constants so we can remove them from the bitcode :)
The other thing to look out for is ABI sizes - this is a general concern: if the size of an object is unknown or wrong, it'll result in hard-to-track issues - maybe that's what your crash is about: some stack allocation that is to small because a type is poorly described.
They're hard to track down in general - compiling with -d:nimAbiCheck (or something similar) can sometimes help.
Yeah this has been the most painful part of getting this to work :) I didn't know about the checkAbi
flag, thanks for letting me know, I'll try it out.
the scope of the changes
really, anything that "already works" but provides infrastructure for multi-platform support - for example the flag changes to compile
, llvm version handling, etc - the aim would be to isolate "infrastructure" work from actual platform support so that in case something goes wrong, the revert set is smaller. That said, perhaps after fixing some of the comments, this set is too small to be meaningful - it's mostly intended as a tool to keep the diff tight as it evolves.
Also, oddly, the test suite / github actions did not run for this PR - wonder what's up with that..
ok, looks like 523e1736c09a74e0c03d7b63ad5607547cb88232 wasn't enough to make CI run - maybe a rebase would help?
actually, it ran after all, and passes - nice - we'll probably need to differentiate skipped-tests between platforms
Hey, sorry for the delay, I've been quite busy with work and life in general :) Hopefully I'll get to making the necessary changes by the end of this week
hey @gushromp - what's the status of this?
@arnetheduck
Sorry, life's been hectic recently and I haven't had the time to see this through :( I probably won't have time to finish it in the forseeable future either, so hopefully someone else can take over.
Most of the stuff was working as-is, and I think the things that didn't work were because of ABI inconsistencies. I was planning to switch to the generated POSIX constants/types (which Araq confirmed is probably the way to go on Discord) - that would help especially with AARCH64 but also X86_64 where it currently fails at runtime.
If I'm able to continue at some point I'll let you know :)
What works:
make compare
)nlvm
self-build works fine)What doesn't:
posix
module which I'll try to iron out. In the meantime you can run in x86_64 via Rosetta just fine.Release builds crash with strange segfault on some things I've tested (i.e.
unittest
when it tries to initialize itself):I'm still trying to investigate and hopefully fix this issue.
Notes:
Nim
submodule URL to my own fork, to make this branch work. If/when we merge this PR, we should revert it :)posix
module and change every imported struct definition by hand. It should probably be possible to repurpose thedetect
tool to do this boring work automatically for us.macosx_consts.nim
in theposix
module instead.