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

Allow building on OpenBSD #1504

Merged
merged 3 commits into from
Apr 4, 2018
Merged

Conversation

ararslan
Copy link
Contributor

@ararslan ararslan commented Apr 2, 2018

With this change, OpenBLAS builds and all tests pass on OpenBSD 6.2 using Clang. Tested on x86-64 only, with and without DYNAMIC_ARCH=1.

The only quirk here is that Clang is the default compiler on OpenBSD 6.2, and yet I still had to use CC=clang when building, otherwise OpenBLAS uses GCC. The default GCC version on OpenBSD is very old, so we run into #875. Let me know if there's something in the Makefile that needs to be adjusted that I've overlooked.

cc @ibara, who may be interested in this.

With this change, OpenBLAS builds and all tests pass on OpenBSD 6.2
using Clang. Tested on x86-64 only, with and without DYNAMIC_ARCH=1.
@martin-frbg
Copy link
Collaborator

WIthout wanting to start a "political" discussion, do you expect OpenBSD to deviate significantly from the already supported FreeBSD ? Else I think your PR could be simplified to affect much fewer files...
Also, does OpenBSD install both gcc and clang side by side by default, and still default to clang (how?) ? I have already queued a PR to (finally) get rid of #875, but we would probably want to use a recent clang rather than an old gcc if given the choice.

@ararslan
Copy link
Contributor Author

ararslan commented Apr 2, 2018

do you expect OpenBSD to deviate significantly from the already supported FreeBSD ?

I'm not sure what you mean, they're different systems. Can you elaborate?

Also, does OpenBSD install both gcc and clang side by side by default, and still default to clang (how?) ?

Yes, OpenBSD 6.2, which I tested this on, ships with both Clang 4.0.0 and GCC 4.2.1. It uses Clang as the default compiler starting with OpenBSD 6.2, i.e. cc refers to clang, and previous versions defaulted to GCC. Despite Clang being default for my version, somehow without specifying CC=clang when invoking Make, OpenBLAS chooses to build with GCC.

@martin-frbg
Copy link
Collaborator

It just occured to me that many of your changes added OS_OPENBSD where there already is OS_FREEBSD, so if it would be sufficient to define OS_FREEBSD when the actual name is either FreeBSD or OpenBSD fewer files would need to be touched.
Regarding the CC issue, are you building with make or cmake ? I believe the former should pick up $CC from the environment (through Makefile.prebuild which invokes c_check with that setting), I am less sure about the latter.

@ararslan
Copy link
Contributor Author

ararslan commented Apr 2, 2018

so if it would be sufficient to define OS_FREEBSD when the actual name is either FreeBSD or OpenBSD fewer files would need to be touched.

Ah, I see. That may work, but I'm hesitant to say one way or another whether it's completely kosher, as I'm not familiar with all of the differences between the FreeBSD and OpenBSD kernels. It might be best to leave them separate for now and perhaps simplify things later if we find that they can be safely combined in full generality. Does that seem okay to you?

are you building with make or cmake ?

GNU Make, I didn't try using CMake.

I just checked the output of gmake -p and CC is indeed getting set automatically to cc, so I'm not sure why it's picking up gcc. ¯\_(ツ)_/¯

@ibara
Copy link

ibara commented Apr 2, 2018

It is probably OK to have OpenBSD defined as OS_FREEBSD for building. However, OpenBSD doesn't have a __FreeBSD__ cpp define, so at least the __OpenBSD__ cpp define would need to be added where needed, as would the Makefile additions be needed. It would be nice if OpenBLAS reported running on OpenBSD when actually running on OpenBSD (if OpenBLAS does in fact report OS at runtime, I've never used it).

Some OpenBSD architectures ship with both gcc-4.2.1 and clang-5.0.1 but some ship with only clang. Additionally, clang is actively tracked and updated on OpenBSD as new releases come out. On systems where both are shipped, cc(1) is a symlink to clang, and the $CC variable is set to cc for official package builds (you may have to set it yourself for manual builds), so it would only ever build with clang in the scenarios we care about, aka official package builds. In all cases, it would either be clang or a more modern gcc (of the 4.9.4 vintage right now, but will be upgraded in the future) from packages that would be building OpenBLAS, so I don't think that #875 would be a problem. Also to note, that eventually, systems that ship with both gcc-4.2.1 and clang will only ship with clang, so this is a non-issue.

@ararslan
Copy link
Contributor Author

ararslan commented Apr 2, 2018

Something that may be a little more robust would be to define something like

#if defined(__FreeBSD__) || defined(__OpenBSD__)
OS_BSD_COMMON
#endif

then use that in place of all current uses of OS_FREEBSD. That makes the code a little clearer as well (at least to me) since you know that what you're reading applies to multiple BSD systems rather than just FreeBSD. (I imagine that DragonFly would fit into all of the same conditionals as well.)

I can also just define OS_FREEBSD on OpenBSD, but I'm not sure how to structure the change in c_check. I guess in that file OpenBSD would be omitted and reported as FreeBSD?

@martin-frbg
Copy link
Collaborator

Thank you very much for the detailed explanation. I guess at that point it is not useful to go through extra hoops just to avoid changing one or two of the files.
The compiler mis-detection is a bit intriguing - what does $CC -E ctest.c return on that system (which is what c_check invokes, the COMPILER_whatever output should be above the OS_OPENBSD line) ? Maybe that file needs another patch, if "your" clang does not identify itself by defining __clang__ .

@ararslan
Copy link
Contributor Author

ararslan commented Apr 3, 2018

On this branch:

$ cc -E ctest.c
# 1 "ctest.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 317 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "ctest.c" 2
# 12 "ctest.c"
COMPILER_CLANG
# 44 "ctest.c"
COMPILER_GNU
# 64 "ctest.c"
OS_OPENBSD
# 105 "ctest.c"
ARCH_X86_64
# 137 "ctest.c"
BINARY_64

@ararslan
Copy link
Contributor Author

ararslan commented Apr 3, 2018

Ah, the Makefile is setting the compiler to gcc because $(origin CC) is default. See https://github.com/xianyi/OpenBLAS/blob/develop/Makefile.system#L14-L20.

@ararslan
Copy link
Contributor Author

ararslan commented Apr 3, 2018

Since (nearly) the exact same edits are needed for supporting DragonFly BSD, I've added a separate commit that includes the appropriate changes for DragonFly. I tested it with the default compiler, GCC, on its only supported architecture, x86-64.

@ibara
Copy link

ibara commented Apr 4, 2018

OpenBSD clang defines __clang__

@martin-frbg martin-frbg merged commit d636b41 into OpenMathLib:develop Apr 4, 2018
@ararslan ararslan deleted the aa/openbsd branch April 4, 2018 17:46
@ararslan
Copy link
Contributor Author

ararslan commented Apr 4, 2018

Thank you both, @ibara and @martin-frbg!

martin-frbg added a commit that referenced this pull request Feb 7, 2019
* Restore dropped patches in the non-TLS branch of memory.c

As discovered in #2002, the reintroduction of the "original" non-TLS version of memory.c as an alternate branch had inadvertently used ba1f91f rather than a8002e2 , thereby dropping the commits for #1450, #1468, #1501, #1504 and #1520.
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.

3 participants