Fix support for NetBSD and enable native code on NetBSD/aarch64 by kit-ty-kate · Pull Request #11097 · ocaml/ocaml

kit-ty-kate · 2022-03-07T19:42:37Z

Same as #11092 and #11096 but with some fixes to make it actually build.

Was failing with:

gcc -c -O2 -fno-strict-aliasing -fwrapv -pthread -Wall -Werror -fno-common -fexcess-precision=standard -ffunction-sections -g -fPIC  -D_FILE_OFFSET_BITS=64  -DCAMLDLLIMPORT=  -o unix.bpic.o unix.c
unix.c: In function 'caml_thread_setname':
unix.c:452:3: error: too few arguments to function 'pthread_setname_np'
  452 |   pthread_setname_np(self, name);
      |   ^~~~~~~~~~~~~~~~~~
In file included from unix.c:52:
/usr/include/pthread.h:160:5: note: declared here
  160 | int pthread_setname_np(pthread_t, const char *, void *);
      |     ^~~~~~~~~~~~~~~~~~
unix.c: In function 'caml_thread_setname':
unix.c:452:3: error: too few arguments to function 'pthread_setname_np'
  452 |   pthread_setname_np(self, name);
      |   ^~~~~~~~~~~~~~~~~~
In file included from unix.c:52:
/usr/include/pthread.h:160:5: note: declared here
  160 | int pthread_setname_np(pthread_t, const char *, void *);
      |     ^~~~~~~~~~~~~~~~~~
unix.c: In function 'caml_thread_setname':
unix.c: In function 'caml_thread_setname':
unix.c:452:3: error: too few arguments to function 'pthread_setname_np'
  452 |   pthread_setname_np(self, name);
      |   ^~~~~~~~~~~~~~~~~~
In file included from unix.c:52:
/usr/include/pthread.h:160:5: note: declared here
  160 | int pthread_setname_np(pthread_t, const char *, void *);
      |     ^~~~~~~~~~~~~~~~~~
unix.c:452:3: error: too few arguments to function 'pthread_setname_np'
  452 |   pthread_setname_np(self, name);
      |   ^~~~~~~~~~~~~~~~~~
In file included from unix.c:52:
/usr/include/pthread.h:160:5: note: declared here
  160 | int pthread_setname_np(pthread_t, const char *, void *);
      |     ^~~~~~~~~~~~~~~~~~
gmake[2]: *** [Makefile:423: unix.b.o] Error 1

and

gcc -c -O2 -fno-strict-aliasing -fwrapv -pthread -Wall -Werror -fno-common -fexcess-precision=standard -ffunction-sections -g  -D_FILE_OFFSET_BITS=64  -DCAMLDLLIMPORT= -DNATIVE_CODE -DTARGET_arm64 -DMODEL_default -DSYS_netbsd  -o sak.o sak.c
In file included from /usr/include/ctype.h:100,
                 from sak.c:29:
sak.c: In function 'add_stdlib_prefix':
sak.c:126:26: error: array subscript has type 'char' [-Werror=char-subscripts]
  126 |       *name = toupper_os(*name);
      |                          ^
sak.c:126:15: note: in expansion of macro 'toupper_os'
  126 |       *name = toupper_os(*name);
      |               ^~~~~~~~~~
cc1: all warnings being treated as errors
gmake[2]: *** [Makefile:313: sak.o] Error 1

All the manual pages necessary to understand those fixes have been added as comments in the code where they are needed.

avsm · 2022-03-08T11:11:04Z

runtime/sak.c

 #define strncmp_os strncmp
+#ifdef __NetBSD__
+/* NOTE: See CAVEATS section in https://man.netbsd.org/ctype.3 */
+#define toupper_os(x) toupper((unsigned int)x)


Given this from that manpage:

Some implementations of libc, such as glibc as of 2018, attempt to avoid the worst of the undefined behavior by defining the functions to work for all integer inputs representable by either unsigned char or char, and suppress the warning. However, this is not an excuse for avoiding con- version to unsigned char: if EOF coincides with any such value, as it does when it is -1 on platforms with signed char, programs that pass char will still necessarily confuse the classification and mapping of EOF with the classification and mapping of some non-EOF inputs.

Should we just unconditionally cast to (unsigned int) on all platforms, including glibc?

oooops, I meant to use unsigned char not unsigned int. Fixed.

Thanks; looking at the glibc man page for toupper, it looks appropriate to always cast toupper_os()'s argument to unsigned char:

The standards require that the argument c for these functions is either EOF or a value that is representable in the type unsigned char. If the argument c is of type char, it must be cast to unsigned char, as in the following example: char c; ... res = toupper((unsigned char) c); This is necessary because char may be the equivalent signed char, in which case a byte where the top bit is set would be sign extended when converting to int, yielding a value that is outside the range of unsigned char.

Faire enough. Done

avsm

The NetBSD fixes look good to me. If I could get another approval on the toupper cast before merging, that'd be nice...

Octachron · 2022-08-26T15:01:11Z

runtime/sak.c

-#define toupper_os toupper
+/* NOTE: See CAVEATS section in https://man.netbsd.org/ctype.3 */
+/* and NOTE section in https://man7.org/linux/man-pages/man3/toupper.3.html */
+#define toupper_os(x) toupper((unsigned char)x)


Am I reading correctly that this change is mainly to avoid a compiler warning on NetBSD?

Yes. It is triggered by -Werror=char-subscripts

Then the toupper_os change seems fine to me (and this is the only change that will survive rebasing).

Octachron · 2022-08-26T15:08:49Z

This looks fine to me, I will approve and merge once rebased.

Octachron · 2022-08-26T15:17:55Z

You may want to edit the commit message of the last commit.

Changes

dra27 · 2022-10-18T12:50:17Z

I was just cherry-picking #11639 to 5.0 and it unexpectedly conflicted because this PR isn't there.

I think this PR is intended for 5.0 - is it OK for me to cherry-pick it with #11639?

Octachron · 2022-10-18T13:06:34Z

The PR should have been cherry-picked to 5.0 indeed. So it is fine to cherry-pick it along with #11639 .

Fix support for NetBSD and enable native code on NetBSD/aarch64 (cherry picked from commit ef07bd8)

dra27 · 2022-10-18T13:47:41Z

Done: f35d5d6

Support for it was added in ocaml#11097

Support for it was added in #11097

Support for it was added in ocaml#11097

kit-ty-kate force-pushed the netbsd-arm64 branch from 4b250c9 to 6ade4fb Compare March 7, 2022 19:49

avsm reviewed Mar 8, 2022

View reviewed changes

kit-ty-kate force-pushed the netbsd-arm64 branch from 6ade4fb to d9875a7 Compare March 8, 2022 11:14

avsm approved these changes Mar 16, 2022

View reviewed changes

dra27 added this to the 5.0.0 milestone Jun 6, 2022

dra27 self-requested a review June 6, 2022 08:44

Octachron reviewed Aug 26, 2022

View reviewed changes

Fix support for NetBSD

284244a

kit-ty-kate force-pushed the netbsd-arm64 branch from 75623c9 to 72a4f98 Compare August 26, 2022 15:15

Octachron reviewed Aug 26, 2022

View reviewed changes

Changes Outdated Show resolved Hide resolved

Enable native code on NetBSD/aarch64

f1fde11

kit-ty-kate force-pushed the netbsd-arm64 branch from 72a4f98 to 6ffb182 Compare August 26, 2022 15:23

Always cast the argument of toupper(3) to unsigned char

f54b3ac

kit-ty-kate force-pushed the netbsd-arm64 branch from 6ffb182 to f54b3ac Compare August 26, 2022 15:30

Octachron approved these changes Aug 26, 2022

View reviewed changes

Octachron merged commit ef07bd8 into ocaml:trunk Aug 29, 2022

kit-ty-kate deleted the netbsd-arm64 branch August 29, 2022 11:35

dra27 pushed a commit that referenced this pull request Oct 18, 2022

Merge pull request #11097 from kit-ty-kate/netbsd-arm64

f35d5d6

Fix support for NetBSD and enable native code on NetBSD/aarch64 (cherry picked from commit ef07bd8)

Octachron mentioned this pull request Nov 25, 2022

Packages for OCaml 5.0.0 beta2 ocaml/opam-repository#22566

Merged

kit-ty-kate added a commit to kit-ty-kate/ocaml that referenced this pull request Jul 17, 2023

Add NetBSD/arm64 to the Tier 2 support list in the README

048501b

Support for it was added in ocaml#11097

kit-ty-kate mentioned this pull request Jul 17, 2023

Add NetBSD/arm64 to the Tier 2 support list in the README #12384

Merged

xavierleroy pushed a commit that referenced this pull request Aug 31, 2023

Add NetBSD/arm64 to the Tier 2 support list in the README (#12384)

15ef93c

Support for it was added in #11097

dra27 pushed a commit to dra27/ocaml that referenced this pull request Sep 21, 2023

Add NetBSD/arm64 to the Tier 2 support list in the README (PR#12384)

74a1201

Support for it was added in ocaml#11097

Conversation

kit-ty-kate commented Mar 7, 2022

Uh oh!

avsm Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

avsm Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

avsm left a comment

Choose a reason for hiding this comment

Uh oh!

Octachron Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

Octachron Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

Octachron commented Aug 26, 2022

Uh oh!

Octachron commented Aug 26, 2022

Uh oh!

Uh oh!

dra27 commented Oct 18, 2022

Uh oh!

Octachron commented Oct 18, 2022

Uh oh!

dra27 commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants