This is an archive of the discontinued LLVM Phabricator instance.

Undef stdatomic.h macro definitions that are defining functions provided in libc++ <atomic>
AbandonedPublic

Authored by mehdi_amini on Nov 7 2016, 4:23 PM.

Details

Summary

This solved compiling:

#include <stdatomic.h>
#include <atomic>

Event Timeline

mehdi_amini updated this revision to Diff 77116.Nov 7 2016, 4:23 PM
mehdi_amini retitled this revision from to Undef stdatomic.h macro definitions that are defining functions provided in libc++ <atomic>.
mehdi_amini updated this object.
mehdi_amini added a reviewer: rsmith.
mehdi_amini added a subscriber: cfe-commits.
rsmith edited edge metadata.

Hmm, this won't help when building libc++ as a module, and we don't have a wrapper header to hold these undefs since <stdatomic.h> is not part of c++.

So either that combination of includes gives a broken <atomic> or a broken <stdatomic.h>, or we do something nonstandard like reimplementing the latter in terms of the former in libc++.

Eric, Marshall, what do you think? How / how much should we support this non-c++ header?

mclow.lists edited edge metadata.Nov 16 2016, 9:52 AM

I'll do some research.

Ok - looking just at kill_dependency, this was added to the C standard in C11. It's required to be a macro.
atomic_is_lock_free appears to be a function, as does atomic_load. Haven't checked the rest.

I see that Bionic (sometimes) defines atomic_is_lock_free as a macro.

More info - The following code:

#include <stdatomic.h>
int main () {}

fails to compile on either gcc 6.2 (locally), gcc 7 head (online compiler) or MSVC (online compiler).

EricWF edited edge metadata.Dec 5 2016, 2:10 AM

Yeah this seems like a configuration that simply can't be supported. I'm not sure if this patch is a great idea.

More info - The following code:

#include <stdatomic.h>
int main () {}

fails to compile on either gcc 6.2 (locally), gcc 7 head (online compiler) or MSVC (online compiler).

Interesting, that lead me to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932 which describes the issue I believe.

One of the example is that we should guarantee ABI compatibility between C and C++ for such code:

#ifdef __cplusplus
#include <atomic>
using namespace std;
#else
#include <stdatomic.h>
#endif

struct s {
  atomic_int i;
};

Do you know if we're providing this guarantee today?

Yeah this seems like a configuration that simply can't be supported. I'm not sure if this patch is a great idea.

OK, let's abandon this then!

mehdi_amini abandoned this revision.Dec 5 2016, 12:40 PM

Thanks for the information! Closing this.

This is the wrong approach.

C and C++ compatibility is far more important than taking the easy way out.

By doing this, you're potentially breaking the ABI for all software that relies on atomic operations...