WIP: Initial changes to support macOS build by gushromp · Pull Request #35 · arnetheduck/nlvm

gushromp · 2022-06-12T16:23:48Z

What works:

Building most things on x86_64
Building itself (make compare)
Using default GC and making debug builds (AFAIK, arc/orc don't work on Linux yet either?)
Some release builds (release build of nlvm self-build works fine)

What doesn't:

M1/arm64 yet, though I hope this is just because of ABI incompatibilities in the 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):
```
proc ensureInitialized() =
  if formatters.len == 0: # <- segfault happens here
    formatters = @[OutputFormatter(defaultConsoleFormatter())]
```
I'm still trying to investigate and hopefully fix this issue.

Notes:

I've temporarily changed the Nim submodule URL to my own fork, to make this branch work. If/when we merge this PR, we should revert it :)
If you run into a segfault or weird behavior/crashes, it's probably because I was too lazy to go through the entire posix module and change every imported struct definition by hand. It should probably be possible to repurpose the detect tool to do this boring work automatically for us.
For some reason, the Mac variant of POSIX constants isn't using the autogenerated values and thus I opted to temporarily define most of those in the ll bitcode. But it would probably be more elegant/performant to remove it from there and just import macosx_consts.nim in the posix module instead.
I hate POSIX.

arnetheduck

this is great to see!

I left a few minor comments - in general, I think to get this merged, the steps would be:

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
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)
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 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.

arnetheduck · 2022-06-13T15:45:24Z

nlvm/nim.cfg

-gcc.linkerexe:"g++"
-gcc.options.always:"-std=c++14"
+gcc.linkerexe="g++"
+gcc.options.always="-std=c++14"


with the change to compile, should be able to remove this

arnetheduck · 2022-06-13T15:47:44Z

llvm/llvm.nim

+
+static:
+  if not (defined(linux) or defined(macosx)):
+    echo "Unsupported platform. Exiting."


there's no need to quit in general, even if there's no explicit support for a platform - ie if it breaks, it'll break but maybe it won't

arnetheduck · 2022-06-13T15:57:45Z

make-llvm.sh


 shift 4
-cmake -GNinja -DLLVM_USE_LINKER=gold LLVM_INCLUDE_BENCHMARKS=OFF "$@" ..
+cmake -GNinja "$@" ..


should continue to use gold on linux and disable benchmarks everywhere

Yeah sorry I made ADDITIONAL_CMAKE_FLAGS variable on the top and forgot to add it back here. As for deleting LLVM_INCLUDE_BENCHMARKS, I did so because it's already passed from the Makefile to the script.

arnetheduck · 2022-06-13T15:58:05Z

nlvm/llgen.nim

 proc callErrno(g: LLGen, prefix: string): llvm.ValueRef =
  # on linux errno is a function, so we call it here. not at all portable.
-
+  


gushromp · 2022-06-14T12:59:29Z

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.

arnetheduck · 2022-06-19T06:13:24Z

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.

arnetheduck · 2022-06-19T06:15:34Z

Also, oddly, the test suite / github actions did not run for this PR - wonder what's up with that..

arnetheduck · 2022-06-19T06:32:53Z

ok, looks like 523e173 wasn't enough to make CI run - maybe a rebase would help?

arnetheduck · 2022-06-19T07:01:44Z

actually, it ran after all, and passes - nice - we'll probably need to differentiate skipped-tests between platforms

arnetheduck · 2022-07-03T07:10:54Z

llvm/llvm.nim

-{.passL: "-Wl,--as-needed".}
-{.passL: gorge(LLVMOut & "bin/llvm-config --ldflags").}
-{.passL: gorge(LLVMOut & "bin/llvm-config --system-libs").}
+{.compile("wrapper.cc", "-std=c++14").}


and with a small upstream fix, this is not needed either: 4355022

gushromp · 2022-07-04T10:21:41Z

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

arnetheduck · 2022-10-26T08:02:12Z

hey @gushromp - what's the status of this?

gushromp · 2022-11-15T16:28:44Z

@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 :)

Initial changes to support macOS build

ecc7647

gushromp changed the title ~~Initial changes to support macOS build~~ WIP: Initial changes to support macOS build Jun 12, 2022

arnetheduck reviewed Jun 13, 2022

View reviewed changes

arnetheduck closed this Jun 19, 2022

arnetheduck reopened this Jun 19, 2022

arnetheduck reviewed Jul 3, 2022

View reviewed changes

		proc callErrno(g: LLGen, prefix: string): llvm.ValueRef =
		# on linux errno is a function, so we call it here. not at all portable.

Conversation

gushromp commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnetheduck left a comment

Choose a reason for hiding this comment

Uh oh!

arnetheduck Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

arnetheduck Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

arnetheduck Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

gushromp Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

arnetheduck Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

gushromp commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnetheduck commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnetheduck commented Jun 19, 2022

Uh oh!

arnetheduck commented Jun 19, 2022

Uh oh!

arnetheduck commented Jun 19, 2022

Uh oh!

arnetheduck Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

gushromp commented Jul 4, 2022

Uh oh!

arnetheduck commented Oct 26, 2022

Uh oh!

gushromp commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gushromp commented Jun 12, 2022 •

edited

Loading

gushromp commented Jun 14, 2022 •

edited

Loading

arnetheduck commented Jun 19, 2022 •

edited

Loading