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.

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

Port to FreeBSD #86

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Port to FreeBSD #86

wants to merge 43 commits into from

Conversation

vrza
Copy link
Contributor

@vrza vrza commented Jun 8, 2022

  • Ported to FreeBSD
  • Makefile now compatible with both GNU make and BSD make

Caveat: needs GNU binutils 2.38 which is not yet in the FreeBSD ports tree, and binutils.sh will not compile it cleanly as configuration fails to include headers /usr/local/include. Did not want to go down this rabbit hole for now, as perhaps arrival of binutils 2.38 in FreeBSD ports will solve it.

Closes #81

@vrza vrza mentioned this pull request Jun 8, 2022
@vrza vrza mentioned this pull request Jun 9, 2022
OpenBSD doesn't seem to allow ioctl(2) to write directly to
the data segment in constructor code. Passing a heap pointer
instead.
@taviso
Copy link
Owner

taviso commented Jun 10, 2022

Thanks so much for working on this - I think we can merge most of this!

I think must be alloca() defined somewhere, and if not we can just use #define alloca __builtin_alloca, but we can't just s/alloca/malloc/, because it will leak memory.

@vrza
Copy link
Contributor Author

vrza commented Jun 10, 2022

@taviso Yeah, good catch. While BSD does not have strndupa(3), it does have plain alloca(3), but it's declared in stdlib.h rather than alloca.h. Fixed.

@taviso
Copy link
Owner

taviso commented Jun 15, 2022

Sorry for the slow response here - I've been working on tests and the showme bug!
Now that I have some tests I'm less worried about big changes.

@vrza
Copy link
Contributor Author

vrza commented Jun 15, 2022

Great, having a regression test suite is super helpful.

Perhaps setting up an automated CI workflow is in order.

Not sure about a cheap way to run CI for FreeBSD, OpenBSD, Solaris etc. GitHub Actions is free (as in beer), integrates with the GitHub workflow out of the box, and supports Linux and MacOS runners out of the box, so might be a decent starting point. Maybe there's a way to spin up VMs in their CI runner hosts.

The regression test suite should be portable as well.

@@ -417,7 +419,7 @@ struct unixdirent * __unix_readdir(DIR *dirp)
return NULL;
}

sighandler_t __unix_signal(int signum, sighandler_t handler)
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like FreeBSD does have sig_t though, I think we should add something like this:

#ifdef __FreeBSD__ 
typedef sig_t sighandler_t;
#endif

Copy link
Contributor Author

@vrza vrza Jun 16, 2022

Choose a reason for hiding this comment

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

I don't think that it's a good idea to use GNU as a baseline and then add an exception for each system that's not GNU. Maintaining many platform-specific code paths with #ifdefs on our end could get out of hand quickly.

Rather we should leverage the POSIX standard in the first place, and in the second place we should leverage GNU's compatibility features. GNU libc knows about POSIX, and knows about BSD, and offers so-called Feature Test Macros for compatibility purposes. There's a good writeup about FTMs on on LWN, and more info in glibc documentation and in the feature_test_macros(7) man page.

Concretely, using -D_DEFAULT_SOURCE instead of -D_GNU_SOURCE would make BSD sig_t translate into GNU sighandler_t. Howeve I'm interested in the reasoning behind using _GNU_SOURCE in the first place?

However, while using _DEFAULT_SOURCE and sig_t would work with GNU/Linux and all BSDs, it would not work on e.g. System V, which happens to be the system that this version of 123 was originally built for! I strongly feel that if we're porting this old SVR4 Lotus 123 to other platforms we should observe POSIX and System V APIs first and foremost, and only reach for GNU and BSD APIs and tooling where it's necessary to do so. This might be a matter of personal opinion, but nevertheless a matter I feel we should have some alignment on.

Anyways, hope this explains why I choose to observe the POSIX standard whenever possible, and in this particular case. This particular case is in alignment with the C standard as well.

@@ -426,10 +428,10 @@ sighandler_t __unix_signal(int signum, sighandler_t handler)
[16] = SIGUSR1,
[17] = SIGUSR2,
[18] = SIGCHLD,
[19] = SIGPWR,
[19] = SIGCONT,
Copy link
Owner

Choose a reason for hiding this comment

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

The intention here is to map UNIX signal numbers to Linux numbers, and it thinks 19 is SIGPWR. If FreeBSD doesn't have it, I think it should be -1, maybe just

#ifdef SIGPWR
[19] = SIGPWR,
#else
[19] = -1,
#endif

Copy link
Contributor Author

@vrza vrza Jun 16, 2022

Choose a reason for hiding this comment

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

Hm, while I did quickly hack this by finding some supposedly portable list of magic numbers online, I'm not absolutely clear about the intention behind this lookup table, perhaps you could help clarify it.

SIGPWR seems to be a Linux-specific symbol, and many other symbols in this table are not defined in the POSIX standard, not sure if they are defined on System V either. Is the original code, or our extensions to it, registering a handler for signal 19?

Similarly, magic numbers in the table (SIGUSR1, SIGUSR2, SIGCHLD...) that are not specific to Linux were likely the same on System V, so it eludes me why they would need to be translated. I can comment out all of these and the program seems to run fine, so I'm confused.

Finally, how did we arrive at translating the magic number 19 to the value of SIGPWR? On my Linux system, for example, SIGPWR is 30.

@@ -23,6 +23,10 @@ extern int __unix_errno;
#define SI86FPHW 40
#define FP_387 3

#define TCGETS 0x5401
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, maybe we're just missing an include?

Copy link
Contributor Author

@vrza vrza Jun 16, 2022

Choose a reason for hiding this comment

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

No, I don't believe that BSD defines these ioctl(2) magic numbers. They are likely included on Linux for compatibility with legacy software, but according to the POSIX standard we should use tcgetattr(fd, argp) instead.

#include <stdio.h>
#include <stdint.h>
#include <alloca.h>
Copy link
Owner

Choose a reason for hiding this comment

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

I think maybe just wrap this in #ifdef __Linux__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it doesn't seem to be standardised, alloca is not specific to Linux, but rather a GNU extension to the C compiler, implemented at least by GCC and clang, possibly other compilers. A comment in FreeBSD stdlib.h sheds some light:

/*
 * The alloca() function can't be implemented in C, and on some
 * platforms it can't be implemented at all as a callable function.
 * The GNU C compiler provides a built-in alloca() which we can use.
 * On platforms where alloca() is not in libc, programs which use it
 * will fail to link when compiled with non-GNU compilers.
 */
#if __GNUC__ >= 2
#undef  alloca  /* some GNU bits try to get cute and define this on their own */
#define alloca(sz) __builtin_alloca(sz)
#endif

Similar to the discussion about sig_t, if we define _DEFAULT_SOURCE then glibc stdlib.h will know it should pull in alloca.h, so that's another place where using _DEFAULT_SOURCE can help wuith portability.

@@ -1,74 +1,79 @@
platform != uname -s
undefine != ./detect-os-symbol-list-file.sh undefine
Copy link
Owner

Choose a reason for hiding this comment

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

Seems okay, but maybe we can just do undefine undefine-$(shell uname).lst? I dunno if that is portable though!

Copy link
Contributor Author

@vrza vrza Jun 16, 2022

Choose a reason for hiding this comment

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

AFAIK syntax-wise that should be portable between GNU and BSD make , but on systems for which you don't have an explicit list file, the build will error out with file not found. The shell script provides a default if it doesn't have explicit rules about the host system.

@emaste
Copy link

emaste commented Sep 6, 2022

Not sure about a cheap way to run CI for FreeBSD, OpenBSD, Solaris etc.

FreeBSD CI can be done via Cirrus-CI, although that still leaves the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FreeBSD port
3 participants