Fix exit code on BSD/Mac

[xvxx]

Continuing the conversation from 3f5325a613435b9ac478a9bfc4b002537d5e5084...

Currently on master, the exit code for EXECUTE - AND STORE EXIT CODE IN is always 0. I did some digging and it looks like BSD has a different WEXITSTATUS() macro than Linux:

OpenBSD's implementation of WEXITSTATUS uses the address-of operator (unary &) on its argument, effectively requiring that its argument have storage. You are calling it with the return value of a function, which doesn't have storage, so the compiler complains. - https://stackoverflow.com/a/13674801

Here's BSD/Mac's:

https://github.com/apple/darwin-xnu/blob/0a798f6738bc1db01281fc08ae024145e84df927/bsd/sys/wait.h#L144

Which uses the _W_INT macro:

https://github.com/apple/darwin-xnu/blob/0a798f6738bc1db01281fc08ae024145e84df927/bsd/sys/wait.h#L128

And glibc's, which doesn't:

https://github.com/bminor/glibc/blob/7628a1b05adb1e4c6857b87c6f8b71a1d0b1d72c/posix/sys/wait.h#L54

https://github.com/bminor/glibc/blob/7628a1b05adb1e4c6857b87c6f8b71a1d0b1d72c/bits/waitstatus.h#L28

I tried a few things but I suspect the most portable would just be to overwrite _W_INT if it exists to do what we want. Sounds like only really old code expects wait() to return a union on BSDs, anyway.

I also added the explicit (int) cast which my Linux machines needed, for some reason.

[Lartu]

It seems that this is failing in Travis (and thus it may fail under some OSs). This is quite the conundrum. Maybe we should implement our own WEXITSTATUS?

[Lartu]

I tried a few things but I suspect the most portable would just be to overwrite _W_INT if it exists to do what we want. Sounds like only really old code expects wait() to return a union on BSDs, anyway.

I agree with this.

I've been doing some research and it seems that the value system() returns is very implementation dependant. Maybe we should just save if it is 0 or non-0?

[xvxx]

It seems that this is failing in Travis (and thus it may fail under some OSs)

I only promised to fix BSD/Mac in the PR title!

My other idea was to just skip the macro and use its functionality directly, which passes the build and works on my test machines but I don't know how portable it is.

[Lartu]

Well, your solution seems quite pragmatic, haha! I hope it works under Windows but anyway I'm merging your PR. Thank you very much for your hard work, dvkt!

[Lartu]

My other idea was to just skip the macro and use its functionality directly, which passes the build and works on my test machines but I don't know how portable it is.

We'll see this in due time. If necessary, we can just add some platform-dependent macros (one for windows, one for BSD, one for Linux) and that will be the end of it. Until then, let's just assume it works. Thank you very much for this contribution, again! :smile:

[xvxx]

Okay thanks! I agree with that, either adding platform macros or our own system() like how there's an exec(). In there we could do something like:

auto status = system(cmd);
int code = WEXITSTATUS(status);

And that should work everywhere, but doing that inline in ldpl.cpp was a bit tricky so I went this other route tanglin' with the macro.

[Lartu]

You can always define a function in ldpl_lib.cpp and get away with it, haha. But anyway your solution is great and if it works that'll be the end of it. :smile: