Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix compilation on FreeBSD. #3625

Merged
merged 10 commits into from
Dec 22, 2023
Merged

Fix compilation on FreeBSD. #3625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e0b6ede
Fix compilation on FreeBSD.
alexdupre Dec 19, 2023
2b69cc2
Explicitly error out on unsupported platforms.
alexdupre Dec 21, 2023
71794d5
Make the MAP_NORESERVE change specific for FreeBSD.
alexdupre Dec 21, 2023
2327de8
Fix the sed expression to run on FreeBSD, too.
alexdupre Dec 21, 2023
b0882d2
Make the TestMain to run on all java versions on FreeBSD.
alexdupre Dec 21, 2023
b7be08b
Update the documentation for FreeBSD.
alexdupre Dec 21, 2023
4b4c21a
Improve documentation.
alexdupre Dec 21, 2023
88a104d
Restore the ability to skip leading whitespaces in a more portable way.
alexdupre Dec 21, 2023
4b0aec9
Add a note about failing tests.
alexdupre Dec 21, 2023
d097491
Reword the comment and remove unused vals.
alexdupre Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 13 additions & 4 deletions docs/user/setup.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,24 @@ installation of Arch Linux.
$ sudo dnf groupinstall "Development Tools"
$ sudo dnf install gc-devel zlib-devel # both optional

**FreeBSD 12.2 and later**
**FreeBSD 12.4 and later**
alexdupre marked this conversation as resolved.
Show resolved Hide resolved

*Note 1:* Only AMD64 and ARM64 architectures are supported.

*Note 2:* Sufficiently recent versions of llvm and zlib come with the
installation of FreeBSD.

.. code-block:: shell

$ pkg install llvm10
$ pkg install boehm-gc # optional

*Note:* A version of zlib that is sufficiently recent comes with the
installation of FreeBSD.
*Note 3:* Using the boehm GC with multi-threaded binaries doesn't work
out-of-the-box yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that a number of Tests, primarily unit-tests, are known to not execute, it may be worthwhile to add a "Note 4" which says something like:
"
A number of Tests, primarily unit-tests, are known to fail
on FreeBSD. It is believed that the code-under-test is correct and that the defect is in the Test itself.

Work is underway to fix these tests. See PR 3625 for a list of tests known to fail at that time."

I/we could create an Issue, with checkboxes, which lists
the known failing tests (Java 8) and substitute that URL into the text above.

That way, we are saying what we are doing and doing what
we say.

The last PR in the series which fixes these tests would delete this Note. I usually dislike creating work items which have to be deleted later, but this work is in flux.

Your thoughts?

*Note 4:* A number of tests, primarily unit-tests, are known to fail
or not terminate on FreeBSD. It is believed that the code-under-test
is correct and that the defect is in the test itself.
Work is underway to fix these tests.

**Nix/NixOS**

Expand Down
6 changes: 5 additions & 1 deletion nativelib/src/main/resources/scala-native/delimcc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#define ASM_JMPBUF_SIZE 192
#define JMPBUF_STACK_POINTER_OFFSET (104 / 8)
#elif defined(__x86_64__) && \
(defined(__linux__) || defined(__APPLE__)) // x86-64 linux and macOS
(defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__))
#define ASM_JMPBUF_SIZE 72
#define JMPBUF_STACK_POINTER_OFFSET (16 / 8)
#elif defined(__i386__) && \
Expand All @@ -25,6 +25,8 @@
#elif defined(__x86_64__) && defined(_WIN64) // x86-64 Windows
#define ASM_JMPBUF_SIZE 256
#define JMPBUF_STACK_POINTER_OFFSET (16 / 8)
#else
#error "Unsupported platform"
#endif

#ifdef DELIMCC_DEBUG
Expand Down Expand Up @@ -57,7 +59,9 @@
#define __externc extern
#define __noreturn __attribute__((noreturn))
#define __returnstwice __attribute__((returns_twice))
#ifndef __noinline
#define __noinline __attribute__((noinline))
#endif
// define the lh_jmp_buf in terms of `void*` elements to have natural alignment
typedef void *lh_jmp_buf[ASM_JMPBUF_SIZE / sizeof(void *)];
// Non-standard setjmp.
Expand Down
2 changes: 1 addition & 1 deletion nativelib/src/main/resources/scala-native/delimcc/setjmp_amd64.S
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

#if defined(__x86_64__) && (defined(__linux__) || defined(__APPLE__))
#if defined(__x86_64__) && (defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with this change.

I never tried running on 32 bit FreeBSD. If the intent is
to support and & test such (which I doubt SN should be doing in the absence of a champion/advocate). does the
corresponding setjmp_amd32.s need a corresponding change?


/* ----------------------------------------------------------------------------
Copyright (c) 2016, Microsoft Research, Daan Leijen
Expand Down
3 changes: 3 additions & 0 deletions nativelib/src/main/resources/scala-native/gc/shared/Safepoint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
#include <windows.h>
#else // Unix
#include <sys/mman.h>
#if defined(__FreeBSD__) && !defined(MAP_NORESERVE)
#define MAP_NORESERVE 0
#endif
#endif

#if defined(__APPLE__)
Expand Down
2 changes: 1 addition & 1 deletion scripts/scalafmt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
set -e

HERE="`dirname $0`"
VERSION=$(sed -nre "s#\s*version[^0-9]+([0-9.]+)#\1#p" $HERE/../.scalafmt.conf)
VERSION=$(sed -nre "s#[[:space:]]*version[^0-9]+([0-9.]+)#\1#p" $HERE/../.scalafmt.conf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to repeat myself. Has this been run manually on Windows?
We, you & I, are going to win no friends if we break SN development on
Windows.

CI runs on Ubuntu Linux. Unless we know that this has been successfully manually run on Windows, we should assume it broken.

As mentioned, I can run this on Windows if such would be hard for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the build env on windows. I've tried on mingw64 and it worked, I don't know what exact sed implementation uses SN on Windows. To be 100% sure it's better if you try yourself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I look at this more closely, I realize that I am a goat, the barnyard critter, not the
"Greatest of all time" kind.

This is obviously a bash script, which will not run on plain Windows to begin with. Arrgh! 🤦
It might be run on WSL (linux on Windows), but any sed there should handle the POSIX
[[:space:]]

Sorry for the false trigger.

COURSIER="$HERE/.coursier"
SCALAFMT="$HERE/.scalafmt-$VERSION"

Expand Down
66 changes: 17 additions & 49 deletions test-interface/src/main/scala/scala/scalanative/testinterface/TestMain.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,56 +28,26 @@ object TestMain {
|""".stripMargin
}

final val iPv4Loopback = "127.0.0.1"
final val iPv6Loopback = "::1"

private def getFreeBSDLoopbackAddr(): String = {
private def setFreeBSDWorkaround(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "not allowing IPv4 mapped IPv6 addresses," does not match my memory of what was working for me circa 2023-10-15. When I take a run, hopefully soon, at fixing
unit-tests for FreeBSD, I will have to refresh my memory.

I believe I was running most unit-tests with an unmodified
TestMain on IPv6. If that was and continues to be true, this section may need to be re-worked in another PR.

I believe this edit can proceed because my only evidence is two month old memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that you had to introduce iPv6Loopback and use it on FreeBSD (even if openjdk8 on FreeBSD has dual mode sockets) was exactly because IPv4 mapped IPv6 addresses are forbidden, so using 127.0.0.1 on an AF_INET6 socket wasn't working.
If you enable them, then on Java8 there isn't anything else to do, but on java 11+ the server will continue to listen only on tcp4, unless java.net.preferIPv4Stack is explicitely set to false on sbt. Manually adjusting both settings seems much more complex than automatically push SN to use an IPv4 socket.

/* Standard out-of-the-box FreeBSD differs from Linux & macOS in
* not allowing IPv4 mapped IPv6 addresses, such as :FFFF:127.0.0.1
* or ::ffff:7f00:1. These can be enabled by setting the line
* ipv6_ipv4mapping="YES" in /etc/rc.conf (and rebooting).
* not allowing IPv4-mapped IPv6 addresses, such as :FFFF:127.0.0.1
* or ::ffff:7f00:1.
*
* FreeBSD TestMain initial connections should succeed on both IPv4
* and IPv6 systems without requiring arcane and non-standard system
* configuration. This method checks the protocol that Java connect()
* is likely used and returns the corresponding loopback address.
* Tests which use IPv4 addresses, either through hard-coding or
* bind resolution, on IPv6 systems will still fail. This allows to
* run the vast majority of tests which do not have this characteristic.
* Another difference is that Java versions >= 11 on FreeBSD set
* java.net.preferIPv4Stack=true by default, so the sbt server
* listens only on a tcp4 socket.
*
* Networking is complex, especially on j-random systems: full of
* joy & lamentation.
* Even if IPv4-mapped IPv6 addresses can be enabled (via the
* net.inet6.ip6.v6only=0 sysctl and/or via the ipv6_ipv4mapping="YES"
* rc.conf variable) and sbt can be instructed to listen on an IPv6
* socket (via the java.net.preferIPv4Stack=false system property),
* the easiest way to make TestMain to work on most FreeBSD machines,
* with different Java versions, is to set
* java.net.preferIPv4Stack=true in Scala Native, before the first
* Java network call, in order to always use an AF_INET IPv4 socket.
*/

if (!LinktimeInfo.isFreeBSD) iPv4Loopback // should never happen
else {
// These are the effective imports
import scalanative.posix.sys.socket._
import scalanative.posix.netinet.in
import scalanative.posix.unistd

/* These are to get this code to compile on Windows.
* Including all of them is cargo cult programming. Someone
* more patient or more knowledgeable about Windows may
* be able to reduce the set.
*/
import scala.scalanative.windows._
import scala.scalanative.windows.WinSocketApi._
import scala.scalanative.windows.WinSocketApiExt._
import scala.scalanative.windows.WinSocketApiOps._
import scala.scalanative.windows.ErrorHandlingApi._

/* The keen observer will note that this scheme could be used
* for Linux and macOS. That change is not introduced at this time
* in order to preserve historical behavior.
*/
val sd = socket(AF_INET6, SOCK_STREAM, in.IPPROTO_TCP)
if (sd == -1) iPv4Loopback
else {
unistd.close(sd)
iPv6Loopback
}
}
System.setProperty("java.net.preferIPv4Stack", "true")
}

/** Main method of the test runner. */
Expand All @@ -100,11 +70,9 @@ object TestMain {
new RuntimeException().fillInStackTrace().ensuring(_ != null)
}

val serverAddr =
if (!LinktimeInfo.isFreeBSD) iPv4Loopback
else getFreeBSDLoopbackAddr()
if (LinktimeInfo.isFreeBSD) setFreeBSDWorkaround()
val serverPort = args(0).toInt
val clientSocket = new Socket(serverAddr, serverPort)
val clientSocket = new Socket("127.0.0.1", serverPort)
val nativeRPC = new NativeRPC(clientSocket)(ExecutionContext.global)
val bridge = new TestAdapterBridge(nativeRPC)

Expand Down