(Re-)enable building on illumos (SmartOS, OmniOS, ...) and Solaris #10063
Conversation
|
I have added a commit to enable stack overflow detection and naked pointers checker on these platforms. These are not needed to enable building, so can be removed from PR if issues are found during review. |
|
@jperkin I would welcome a review. |
|
Unfortunately it does not work on my system: SunOS solaris 5.11 OpenSXCE2014.05__Illumos20140505 sun4u sparc SUNW,Sun-Blade-2500 Solaris |
|
@ksromanov I forgot to mention this patch is only for x86_64 builds. The error you are getting could be related to the following define in floats.c (see a similar report here: #7256) #define _XOPEN_SOURCE 700I don't know what the fix would be here. It does not look like a sparc-related issue, more like having a rather old toolchain. |
|
@tleedjarv , could you please rebase your branch? I have a patch for Solaris/SPARC, which is not published. Unfortunately it is quite old and has merge conflict with current trunk. May be we can make a joint patch, which works with Solaris/SPARC and Illumos/(SPARC/x86). |
tleedjarv
force-pushed
the
solaris-build
branch
from
75d0666
to
48d1bf7
Compare
|
Rebased.
@ksromanov Happy to do a joint patch! |
|
@tleedjarv , Tõivo, would you please check that my changes (see PR to your fork) do not break anything on your system. I checked the patch - it works on both
for 64-bit target. Due to alignment issues with multicore-related It also seem not to break regular |
|
Very good, Konstantin. It also works for me on Shall I merge your commits to this PR? |
Yes, please. |
Done. Konstantin, could you propose an update to the Changes file, to include what you have done. |
|
Could you please replace with And, please, regenerate configure - I didn't commit the last version of it! |
tleedjarv
changed the title
|
Tõivo,
on my systems (T5 and SunBlade 2500 Silver, see above). 32-bit builds do not work, but it is
So, right now our work is done. Happy New Year! |
|
@ksromanov It seems that the removal of one include in I restored the include for Windows. Konstantin, can you check if the build is still working for you. |
Unfortunately, Tõivo, it got broken on T5/Oracle Solaris (on SunBlade/Illumos everything works great). I'll find a solution, though I don't know how much time it takes. Anyway, this single include can be removed manually on our systems. So, let's leave it as is for now, while I am searching for the solution. There are couple small PRs by David A. which alter |
The include is now protected by
No problem at all. I will continue maintaining and rebasing this PR for the foreseeable future. |
This hack with |
|
Tõivo, I have some update:
Could you please a. Accept my PR - tleedjarv#2 (and please test it). "Remove caml/osdeps.h include from yacc." by me and c. regenerate
|
tleedjarv
force-pushed
the
solaris-build
branch
3 times, most recently
from
d66e125
to
7490b89
Compare
|
@ksromanov Done and done. Works good on my system. I had to regenerate configure because your version had some unrelated differences in there (different autoconf version?). I don't have any special compilation instructions to add. I use (On another note, rebasing and changing commits added my name to your commits. I hope it's ok, but I guess it's possible clean it up as well.) |
|
Thanks a lot! I checked the result on our systems (T5 and SunBlade - everything works for 64-bit). Unfortunately, I found one redundant commit - "Regenerate configure" ef2f0d1 Would you please remove it to clean a bit our commit history?
Yes, of course! (actually, I don't care about the authorship of these commits at all) |
There was a problem hiding this comment.
@ksromanov asked me if I could have a look at this patch. It looks okay to me, with most of the changes reasonable-looking and in configure.ac. The changes to the actual runtime code are minimal, and mostly look fine. The special-casing of __SUNPRO_C to silence _XOPEN_SOURCE definition is a bit unpleasant, but it's a sensible choice if we do want to make the users of those systems happy.
I am not knowledgeable enough in this part of the system to give an approval stamp myself, but I left review notes in the parts of the patch that I think are worth looking at for an expert. (Maybe count this as half-an-approval; easy to review, only one half remaining!)
| @@ -1491,10 +1514,11 @@ AC_CHECK_HEADER([sys/mman.h], | |||
| AC_CHECK_FUNC([pwrite], [AC_DEFINE([HAS_PWRITE])]) | |||
|
|
|||
| ## -fdebug-prefix-map support by the C compiler | |||
| AS_CASE([$CC,$host], | |||
| AS_CASE([$ocaml_cv_cc_vendor,$host], | |||
There was a problem hiding this comment.
Review note: this change affects all architectures, but actually it only matters when the cc_vendor is xlc and sunc, so it is actually rather safe.
There was a problem hiding this comment.
xlc and sunc are the oldest compilers of all 5 supported.
runtime/floats.c
Outdated
| #define _XOPEN_SOURCE 700 | ||
| #endif |
There was a problem hiding this comment.
It is strange that this (and the other disabling of XOPEN_SOURCE like that) would be necessary. I looked around the web and it looks rather similar to this issue. I think it would be nice to have a comment in the source as to why __SUNPRO_C is handled specially here.
There was a problem hiding this comment.
Unfortunately this is a problem of old standard library of Sun PRO C. I do not know a better way to fix it. We can add a comment about it.
There was a problem hiding this comment.
Thanks Xavier Leroy I found out that it is not necessary and can be substituted with -D_XPG6 compiler option. Will remove.
|
Many thanks @tleedjarv and @ksromanov for all the work both of you have
done on this PR, for your rigorous and enthousiast attitude.
Let's tryto be efficient to get this merged promptly, so that you guys
do not have to maintain it for too long.
Regarding the commit history: would it please be possible to make sure
that configure is re-generated each time configure.ac is modified? It's
better to keep both in sync for each commit because it makes bisection
easier.
And regarding the -O4 flag, am I understanding correctly that it's the
only way to get funcitons inlined? It seems a pity because the build
system makes it possible to override such flags and users arelikely to
indeed do it, e.g. for debugging purposes.
|
|
@shindere thank you very much for reviewing this PR!
Yes, exactly. Unfortunately, otherwise From my point of view, in about 10 years we will forget about Should we update
|
There was a problem hiding this comment.
I need to better understand several of the proposed changes, see my questions below.
Also: did you test with GCC or Clang as the C compiler? Performance of bytecode programs will suck with the Sun Pro compiler, assuming it does not implement the GNU extensions to C. That's why we strongly recommend GCC or Clang.
otherlibs/unix/link.c
Outdated
| #if !defined(__SUNPRO_C) | ||
| #define _XOPEN_SOURCE 700 | ||
| #endif |
There was a problem hiding this comment.
I need to understand the problem better. _XOPEN_SOURCE 700 corresponds to the Single Unix Specification version 4. https://docs.oracle.com/cd/E88353_01/html/E37853/xpg6-7.html says that SUSv4 is supported provided the c99 option is given to the compiler, and then _XOPEN_SOURCE is set to 700 (so, re-defining it should be a no-op).
- Why are things not working for you as described in Oracle's docs?
- If you compile with gcc but with the same set of standard include files,
__SUNPRO_Cwill be false and_XOPEN_SOURCEwill be defined to700. Does this cause the same errors?
There was a problem hiding this comment.
On
SunOS sundev13 5.11 11.3 sun4v sparc sun4v Solaris with Sun PRO C I get an error
"/usr/include/sys/feature_tests.h", line 354: #error: "Compiler or options invalid for pre-UNIX 03 X/Open applications and pre-2001 POSIX applications"another way to fix it is to define -D_XPG6="" for Solaris/Sun PRO C, which can be done in configure! Thanks a lot, @xavierleroy , we need to remove this change to link.c! See tleedjarv#3
So,
- Perhaps it is because
_XPG6was not defined. Oracle docs say (see your link)
"A default Oracle Solaris installation might require additional steps in order to be fully SUSv4 compliant."
apparently we do not have these steps fulfilled. The best workaround seems to define _XPG6.
- Unfortunately we do not have
gcconSPARCsystems in BB. I triedclang, and yes, it does not like this place (float.c and link.c have the same_XOPEN_SOURCEdefine):
clang -c -O2 -fno-strict-aliasing -fwrapv -Wall -Wdeclaration-after-statement -Werror -fno-common -g -D_FILE_OFFSET_BITS=64 -D_REENTRANT -DCAML_NAME_SPACE -DCAMLDLLIMPORT= -std=c99 -o floats.b.o floats.c
floats.c:21:9: error: '_XOPEN_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
#define _XOPEN_SOURCE 700
^
<built-in>:311:9: note: previous definition is here
#define _XOPEN_SOURCE 600
^
In file included from floats.c:26:
In file included from /usr/include/math.h:17:
In file included from /usr/include/iso/math_iso.h:12:
/usr/include/sys/feature_tests.h:354:2: error: "Compiler or options invalid for pre-UNIX 03 X/Open applications and pre-2001 POSIX applications"
#error "Compiler or options invalid for pre-UNIX 03 X/Open applications \
^
2 errors generated.defining _XPG6 solves only the second error:
clang -c -O2 -fno-strict-aliasing -fwrapv -Wall -Wdeclaration-after-statement -Werror -fno-common -g -D_FILE_OFFSET_BITS=64 -D_REENTRANT -DCAML_NAME_SPACE -DCAMLDLLIMPORT= -D_XPG6="" -std=c99 -o floats.b.o floats.c
floats.c:21:9: error: '_XOPEN_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
#define _XOPEN_SOURCE 700
^
<built-in>:311:9: note: previous definition is here
#define _XOPEN_SOURCE 600
^
1 error generated.
|
Konstantin Romanov (2021/01/05 08:36 -0800):
@shindere thank you very much for reviewing this PR!
My pleasure.
> And regarding the -O4 flag, am I understanding correctly that it's the only way to get funcitons inlined?
Yes, exactly. Unfortunately, otherwise `Sun PRO C` makes them external, these functions reference other functions, which are not linked into `ocamlyacc` and `Solaris` linker ends up with _unresolved externals_. This is a problem on `Solaris`/`Sun PRO C` side and I don't want to modify/spoil `ocamlyacc` code to workaround this problem.
From my point of view, in about 10 years we will forget about `SPARC`
target, so it is better to leave this in `configure.ac`. Hopefully
most of the code changes are needed for `Illumos/x86-64`, which will
live much longer than `Solaris/SPARC` target.
What do you think, @xavierleroy ?
> Regarding the commit history: would it please be possible to make sure that configure is re-generated each time configure.ac is modified? It's better to keep both in sync for each commit because it makes bisection easier.
1. May be we just squash these commits?
2. Otherwise @tleedjarv or myself can just rewrite the commits,
running `autogen` in between. Unfortunately we have different versions
of `autoconf`, so it should be a single person.
I'd say to squash those commits which indeed go together semantically.
For those where it odes not make semantic sense to squash them, then I'd
say it's better to keep them separate from each other than to
artificially squash them jsut to save re-generating configure.
Perhpas it's good ot see who has the most recent version of autoconf and
this person can do the squashing?
|
I would prefer to squash everything in the end, but this can be done by a "squash and merge" on Github, no need to change your source repository. |
Before this commit, the filter-locations.sh script used in two backtrace tests was using the GNU -o command-line option to grep. This commit rewrites the script so that it does not rely on this option any longer. This requires to rewrite the referencefiles for the two tests in question.
|
I can confirm I also see the core dump. I did a little bit of debugging and from what I can tell, |
|
Thanks! I'd appreciate @xavierleroy's input here.
|
|
I could not reproduce the crash with the |
|
Xavier Leroy (2021/01/14 06:01 -0800):
I could not reproduce the crash with the
`tests/runtime-errors/stackoverflow.ml` test, using the same OmniOS VM
that @shindere uses.
My bad, but I did tell you the repo was in a weird state.
You can now.
```
ci@ocaml-omnios:~/ocaml/testsuite/tests/runtime-errors$ ~/ocaml/ocamltest/ocamltest.opt stackoverflow.ml
```
should be the only thing needed to get you going.
|
|
All right, I see the segfault now. I had to install gdb (because mdb is no fun), from sources (because I couldn't find a package for it, that's sad). Turns out that the I've exhausted the time I can spend on this PR, so I will not debug any further. Please turn stack overflow detection off in the configure script. |
Stack overflow detection is not working properly on Solaris.
|
Thank you Xavier, I really appreciate your taking the time. |
|
Thank you! After 108 messages and 23 commits, it's time to merge this PR. I'll preserve the commit history because it's pretty clean. Thanks to all who participated. |
|
Thank you very much! @Octachron are you planning to cherry-pick the final set of commits into 4.12 branch? |
|
My opinion: 4.12.0 is in beta already, and I feel this set of changes is too big to be added at the last moment. |
|
At the same time, for non-illumos or openSolaris operating systems, this PR does not change anything, isn't it? It seems to me that having an experimental support for illumos and openSolaris available in the wild would garner more scrutiny than letting the patch linger on trunk for one release cycle. |
|
Florian Angeletti (2021/01/15 04:16 -0800):
At the same time, for non-illumos or openSolaris operating systems,
this PR does not change anything, isn't it?
Well it breaks a test on Windows platforms, see the discussion on
#10155...
It seems to me that having an experimental support for illumos and
openSolaris available in the wild would garner more scrutiny than
letting the patch linger on trunk for one release cycle.
It makes sense, indeed. Especially if we manage to fix the
`inline_traversal_test.ml` test on Wiindows.
|
|
(copied over from #10155, because the issue is better discussed here) I tried to understand what could be the cause of the Windows issue, here is a summary. Some of the tests in I think that a sensible fix would be to remove the script and have a reference file that is specific to the new backtrace format, and does not work with the older backtrace format. This makes exchanging testsuite changes between OCaml versions more cumbersome (in particular when rebasing fixes to older branches), but to me this is less painful than thinking about making |
|
Many thanks, @gasche, for having taken the time to investingate
theproblem with the backtrace tests!
Indeed, if we could get rid of the post-processing completely and just
check the output as it is produced by the programs, that would simplify
things. I found it surprising that the post-processing script was
removing information that the programs themselves computed. It would
really feel more accurate to check the program outputs directly.
I am willing to implement this (next week!) if there is an agreement
that this is the way to go.
|
|
After more investigation: my guess that the script was introduced to bridge over backtrace-printing differences was wrong. It was actually introduced long before in 28dc832 , when support for inlining in backtraces was introduced. (I suppose that the point was to remain robust in the reference file to different choices of inlining, typically between clambda and flambda.) |
|
Gabriel Scherer (2021/01/15 06:57 -0800):
After more investigation: my guess that the script was introduced to
bridge over backtrace-printing differences was wrong. It was actually
introduced long before in 28dc832 ,
when support for inlining in backtraces was introduced. (I suppose
that the point was to remain robust in the reference file to different
choices of inlining, typically between clambda and flambda.)
If what's needed is to have several reference files (lambda vs.
flambda, bytecode vs. native, degree of optimization...), that can be
achieved with ocamltest, too.
I, personnally, don't know what exactly the test want to test. It'd be
helpful to get input from their authors.
|
|
I proposed a PR (#10157) to remove the Note: getting input from test authors is nice, but right now our CI is broken, so I think we should try to fix it or revert the change. |
Re-enable building on x86-64 illumos (SmartOS, OmniOS, ...) and Solaris. (cherry picked from commit 42b0efb)
|
@ksromanov : we are currently testing this patch in the second beta for 4.12.0 . If everything goes well it should be part of the 4.12.0 release. |
|
@Octachron Thanks a lot! |
|
Xavier Leroy (2021/01/10 10:17 -0800):
This is probably OK. I wonder whether we should use `$CC -c` (or
something similar) as the default value for `as` and `aspp`. This
would work most of the time. Going through the C compiler driver to
invoke the assembler adds a bit of run-time but simplifies
configuration quite a bit.
Nice idea! Thanks! See #10176 which proposes an implementaiton.
|
strchr() is standard and declared in <string.h>. index() is legacy and declared in <strings.h>, which we don't include. The discrepancy was found on Solaris as part of ocaml#10063.
This PR replaces the now unmaintained #9087.
There are still several active OS distributions based on OpenSolaris and illumos. SmartOS, OmniOS and OpenIndiana are the ones best known. OCaml works well on these platforms. See also #2024 (comment) and #2024 (comment)
OCaml is also included in pkgsrc which is available on these platforms.
In this PR, build configuration is provided only for 64-bit and GCC.
Explanation to changes in systhreads sources
The issue with
_POSIX_PTHREAD_SEMANTICSis that even though it is defined in otherlibs/systhreads/st_posix.h, it is too late because otherlibs/systhreads/st_stubs.c includes caml/signals.h before st_posix.h. Due to this, there is no point in defining_POSIX_PTHREAD_SEMANTICSin st_posix.h.There are two solutions to this. First, rely on autoconf setting
PTHREAD_CFLAGSand include it in the Makefile. Alternatively, add the definition in st_stubs.c.Defining
_POSIX_PTHREAD_SEMANTICSin st_stubs.c makes compiling it independent of autoconf and Makefile, if that is what is desired. On the other hand, the definition may be seen as the responsibility of the build system. In this PR, both of these solutions are present for review (and may remain as is, since they don't conflict with each other).