Page MenuHomePhabricator

[libcxx] Make stdatomic.h work when included from a C source file
ClosedPublic

Authored by ngg on Sep 24 2022, 8:52 AM.

Details

Summary

If a C source file includes the libc++ stdatomic.h, compilation will
break because (a) the C++ standard check will fail (which is expected),
and (b) _LIBCPP_COMPILER_CLANG_BASED won't be defined because the
logic defining it in __config is guarded by a __cplusplus check, so
we'll end up with a blank header. Move the detection logic outside of
the __cplusplus check to make the second check pass even in a C context
when you're using Clang. Note that _LIBCPP_STD_VER is not defined when
in C mode, hence stdatomic.h needs to check if in C++ mode before using
that macro to avoid a warning.

In an ideal world, a C source file wouldn't be including the libc++
header directory in its search path, so we'd never have this issue.
Unfortunately, certain build environments make this hard to guarantee,
and in this case it's easy to tweak this header to make it work in a C
context, so I'm hoping this is acceptable.

Fixes https://github.com/llvm/llvm-project/issues/57710.

Note: I couldn't update the https://reviews.llvm.org/D131435 revision, that's why I've created a new one.

Diff Detail

Event Timeline

ngg created this revision.Sep 24 2022, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2022, 8:52 AM
ngg requested review of this revision.Sep 24 2022, 8:52 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 24 2022, 8:52 AM
ldionne requested changes to this revision.Sep 26 2022, 6:29 AM
ldionne added a subscriber: smeenai.

Thanks for picking this up. I think you probably needed to "Commandeer" D131435 in order to update it. But this will do, I'll ask @smeenai to close D131435.

Let's try to land this quickly, I realize only now that this is the source of some failures I've been seeing internally as well, so this is likely hitting a bunch of people and we should cherry-pick back to LLVM 15.

libcxx/include/stdatomic.h
124

Why is this change required?

This revision now requires changes to proceed.Sep 26 2022, 6:29 AM
ngg added inline comments.Sep 26 2022, 12:30 PM
libcxx/include/stdatomic.h
124

I've tried to describe it in the commit message: "Note that _LIBCPP_STD_VER is not defined when in C mode, hence stdatomic.h needs to check if in C++ mode before using that macro to avoid a warning."

_LIBCPP_STD_VER is defined only when in C++ mode, this triggers a warning "treating undefined as 0", and the include_as_c.sh.cpp test even fails because it uses -Werror

ldionne accepted this revision.Sep 26 2022, 4:14 PM
ldionne added inline comments.
libcxx/include/stdatomic.h
124

Oh, of course, this makes sense. Thanks and sorry for the dumbish question.

This revision is now accepted and ready to land.Sep 26 2022, 4:14 PM
ngg accepted this revision.Sep 26 2022, 10:17 PM

@ldionne can you please merge this? I don't have push permissions. Thanks!

@ldionne can you please merge this? I don't have push permissions. Thanks!

Sure, can you please provide Author Name <email@domain> for attribution please?

ngg added a comment.Sep 27 2022, 7:27 AM

Sure, can you please provide Author Name <email@domain> for attribution please?

Gergely Nagy <ngg@ngg.hu>

This revision was automatically updated to reflect the committed changes.