876156 - No 'atomic' header on OpenBSD/clang

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Landry Breuil (:gaston)]

For some reason it seems bug 732043 broke my builds on OpenBSD/clang, as found out in https://bugzilla.mozilla.org/show_bug.cgi?id=847344#c28.

The #define goo around line 29 in Atomics.h sets MOZ_HAVE_CXX11_ATOMICS on my configuration, but for some reason the <atomic> header doesnt exist in the llvm/clang package, probably because of the way clang 3.2 is built/packaged/configured on OpenBSD.

Comment 1

[Landry Breuil (:gaston)]

Sidenote: our libstdc++ is the one from gcc 4.2 (yeah...) and a full failing log can be found here : http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/319/steps/build/logs/stdio

Dunno if the problem lies in __has_include or the other (__cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)) check.

Comment 2

[Mike Hommey [:glandium]]

So the problem essentially is that your clang is supposed to support C++11 but your libstdc++ is too old for that. clang should probably reject that, and you should probably use libc++ in openbsd.

Comment 3

[Landry Breuil (:gaston)]

libc++ hasnt been ported yet to OpenBSD, so far we can only use this old libstdc++. So not an option.

Adding && !(defined(__OpenBSD__)) to the check Atomics.h might be acceptable ? I'd rather have a proper check for libstdc++ version or libc++ presence....

Comment 4

[Landry Breuil (:gaston)]

Note that it seems that __has_include(<atomic>) will always succeed, since there's one in stl_wrappers.......
All three checks independently succeed here.

Comment 5

[Landry Breuil (:gaston)]

add defined(_LIBCPP_VERSION) maybe ? (from http://clang-developers.42468.n3.nabble.com/Best-way-to-detect-libc-at-compile-time-td4025578.html)

i dont have a libc++ host here to check here...

Comment 6

[Jan Beich]

Attachment: skip stl_wrappers with __has_include_next

clang can also use more recent libstdc++ which has <atomic>. Only BSDs
are stuck with 4.2, not pure but an amalgamation of GPLv2 backports
and self-made fixes.

Ironically, OS X's libstdc++-4.2 is broken enough to gloss over the
issue with -fvisibility=hidden.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 754962 [details] [diff] [review]
skip stl_wrappers with __has_include_next

That's going to add warnings.

Comment 8

[Jan Beich]

Can you show an example or push to Try?

Comment 9

[Landry Breuil (:gaston)]

https://tbpl.mozilla.org/?tree=Try&rev=349830703274

Just for curiosity, on freebsd are you using libc++ for ports too, or not yet and fallback to the old libstdc++ 4.2.1 (and thus, fail as we do ?) ? Does libc++ define _LIBCPP_VERSION ?

Comment 10

[Landry Breuil (:gaston)]

The try run didnt show any real breakage, but still i'm curious to know what warnings it could add.

Comment 11

[Landry Breuil (:gaston)]

Related to bug 873649 ? same fix possible ?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Landry Breuil (:gaston) from comment #11)
> Related to bug 873649 ? same fix possible ?

I wouldn't have any problem with a !defined(__OpenBSD) or similar.

A better fix would do #include <ciso646> and check for _LIBCPP_VERSION; maybe there's some libstdc++ macro we could use for checking for proper support on clang/linux, too...

Comment 13

[Mike Hommey [:glandium]]

(In reply to Landry Breuil (:gaston) from comment #10)
> The try run didnt show any real breakage, but still i'm curious to know what
> warnings it could add.

Try won't show anything unless you make it use clang. Which will then complain because #include_next <atomic> is not supposed to happen in a file that is not atomic. (Although the warning message is slightly different, i don't remember what exactly)

Comment 14

[Landry Breuil (:gaston)]

Attachment: Make sure we dont use <atomic> on openbsd

So, first alternative with a big hammer : dont try to use <atomic> on OpenBSD at all.

Comment 15

[Landry Breuil (:gaston)]

Attachment: Make sure libc++ is used with clang to enable <atomic> use

And second alternative, include ciso646 & check for _LIBCPP_VERSION. This widens the scope, but then will fail if a recent libstdc++ is used, which it's supposed to work (i think). But since it's disabled on linux, and libstc++ is old on macos, this shouldnt affect tier1 platforms ?

Either way should work for me.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 756564 [details] [diff] [review]
Make sure we dont use <atomic> on openbsd

Review of attachment 756564 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Atomics.h
@@ +31,5 @@
>      * due to the loose typing of the __sync_* family of functions done by
>      * GCC.  We do not have a particularly good way to detect this sort of
>      * case at this point, so just assume that if we're on a Linux system,
>      * we can't use the system's <atomic>.
> +    * OpenBSD uses an old libstdc++ 4.2.1 and thus doesnt have <atomic>.

Please add a blank line of comment before this line.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 756567 [details] [diff] [review]
Make sure libc++ is used with clang to enable <atomic> use

Review of attachment 756567 [details] [diff] [review]:
-----------------------------------------------------------------

This would be better, but I don't want to get into the "our headers account for five different C++ libraries with seven compilers on nine systems" #ifdef mess.

Comment 18

[Landry Breuil (:gaston)]

Landed the !defined(__OpenBSD__) version with a comment adding a free empty line : https://hg.mozilla.org/integration/mozilla-inbound/rev/08ed531fed70

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08ed531fed70

Comment 20

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=349830703274
> 
> Just for curiosity, on freebsd are you using libc++ for ports too, or not
> yet and fallback to the old libstdc++ 4.2.1 (and thus, fail as we do ?) ?

clang on freebsd still uses libstdc++-4.2 by default but it can be made
to use either libc++ since 9.1 or newer libstdc++ from lang/gcc* ports.

> Does libc++ define _LIBCPP_VERSION ?

Yes and libstdc++ has smth similar.

$ fgrep -r _LIBCPP_VERSION /usr/include
/usr/include/c++/v1/__config:#define _LIBCPP_VERSION 1101

$ fgrep -r __GLIBCXX__ /usr/include
/usr/include/c++/4.2/bits/c++config.h:#define __GLIBCXX__ 20070831

(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Landry Breuil (:gaston) from comment #10)
> > The try run didnt show any real breakage, but still i'm curious to know what
> > warnings it could add.
> 
> Try won't show anything unless you make it use clang.

Try on OS X always uses clang given gcc 4.2 is no longer supported.

> Which will then complain because #include_next <atomic> is not
> supposed to happen in a file that is not atomic. (Although the
> warning message is slightly different, i don't remember what
> exactly)

OK but trying the patch locally on clang 3.3 I don't see any
warnings pertaining to Atomics.h using either libstdc++ or libc++,
gcc_hidden.h or -fvisibility.