Diagnose use of ATOMIC_VAR_INIT

[aaron.ballman]

Summary

The ATOMIC_VAR_INIT was deprecated in C17 (C17 7.31.8p2), largely because it is fundamentally broken. It has already been proposed to be removed from C2x (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2390.pdf). There is no replacement for the macro because the macro is unnecessary for implementations to get initialization correct (plain assignment suffices).

This patch adds a diagnostic when expanding the ATOMIC_VAR_INIT macro that comes from stdatomic.h in all C modes. This will help users to recognize that use of this macro is strongly discouraged.

[rsmith]

lib/Lex/PPMacroExpansion.cpp

523

This assumes there is a local directive for the macro. There need not be (it could be imported from a module). We should presumably check to see if all active macro directives are from that header instead.

[aaron.ballman]

lib/Lex/PPMacroExpansion.cpp

523

I'm not familiar with how that's done -- is there some existing code doing something similar I should model off of? Also, what would the test case look like for that scenario?

[jfb]

Is atomic initialization now correct in all modes (including C++) without this macro? I don’t think we should diagnose until such a time because some code uses to macro to be portably correct. IIRC we only ended up fixing C++ in 20 with Nico’s paper (after Olivier and I failed repeatedly to do so.

[aaron.ballman]

In D67023#1653425, @jfb wrote:

Is atomic initialization now correct in all modes (including C++) without this macro?

My understanding is yes, but I am not an expert in atomics. However, the diagnostic is currently tied to C and hasn't been introduced for C++.

I don’t think we should diagnose until such a time because some code uses to macro to be portably correct.

I think that time is now, but more importantly, I think we should diagnose anything the standard deprecates because that code will not be portable for long and this is the way we indicate that to users. This is no different than any of our other deprecation diagnostics in that regard. Is there a reason to deviate with this specific macro?

IIRC we only ended up fixing C++ in 20 with Nico’s paper (after Olivier and I failed repeatedly to do so.

I don't believe this macro is (reasonably) implementable in C++ any more than it is in C, so I'm skeptical that *everything* was fixed with it. Do you know of implementations where this macro is required in order to properly handle initialization? Has SG1 had any discussions about deprecating it?

[jfb]

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

[aaron.ballman]

In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)? Also, this functionality is implemented by libc++ and libstdc++, so I'm not certain what you mean by "until p0883 is fully implemented" or why that paper would be implemented "even just for C". Are you suggesting to gate this deprecation for C functionality based on what C++ standard library implementations are doing?

I'm sorry if I'm being dense here, but I am still not seeing the issue you're concerned by. C has already deprecated this feature and is planning to remove it shortly. I am diagnosing that fact in C, which seems perfectly appropriate to do regardless of what C++ is doing in this space.

Users have a portable way to write their code already because ATOMIC_VAR_INIT does not do any special magic and no implementation requires a user to use it to achieve portable atomic initialization semantics. If they get the diagnostic (which only triggers in C mode and only if the macro was defined in stdatomic.h), they should remove the use of ATOMIC_VAR_INIT from their code, or disable the deprecation diagnostic. If neither of those options appeals to the user, they can write something along the lines of:

_Atomic(int) i = 
#if defined(__cplusplus)
ATOMIC_VAR_INIT(12);
#else
12;
#endif

(not that I think anyone will want to write that, but strict adherence to a broken part of both standards does not seem like something we want to encourage anyway -- this is deprecated functionality, so the whole point is to discourage its use.)

[jfb]

In D67023#1654704, @aaron.ballman wrote:

In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)?

I think it'll make it in as an NB comment.

Also, this functionality is implemented by libc++ and libstdc++, so I'm not certain what you mean by "until p0883 is fully implemented" or why that paper would be implemented "even just for C". Are you suggesting to gate this deprecation for C functionality based on what C++ standard library implementations are doing?

Yes, because atomics are already hard enough to use correctly between C and C++. Once we've fixed C++ I'd like the diagnostic to be the same for both (modulo which standard it's deprecated in). I think we should add the diagnostic for C and C++ at the same time, and we should make sure there's no libc++ nor clang work required before diagnosing.

I'm sorry if I'm being dense here, but I am still not seeing the issue you're concerned by. C has already deprecated this feature and is planning to remove it shortly. I am diagnosing that fact in C, which seems perfectly appropriate to do regardless of what C++ is doing in this space.

Users have a portable way to write their code already because ATOMIC_VAR_INIT does not do any special magic and no implementation requires a user to use it to achieve portable atomic initialization semantics. If they get the diagnostic (which only triggers in C mode and only if the macro was defined in stdatomic.h), they should remove the use of ATOMIC_VAR_INIT from their code, or disable the deprecation diagnostic. If neither of those options appeals to the user, they can write something along the lines of:

_Atomic(int) i = 
#if defined(__cplusplus)
ATOMIC_VAR_INIT(12);
#else
12;
#endif

(not that I think anyone will want to write that, but strict adherence to a broken part of both standards does not seem like something we want to encourage anyway -- this is deprecated functionality, so the whole point is to discourage its use.)

Is this correct for all implementations of _Atomic and std::atomic that clang supports, including non-lock-free types? We should make sure before diagnosing.

[aaron.ballman]

In D67023#1656453, @jfb wrote:

In D67023#1654704, @aaron.ballman wrote:

In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)?

I think it'll make it in as an NB comment.

Also, this functionality is implemented by libc++ and libstdc++, so I'm not certain what you mean by "until p0883 is fully implemented" or why that paper would be implemented "even just for C". Are you suggesting to gate this deprecation for C functionality based on what C++ standard library implementations are doing?

Yes, because atomics are already hard enough to use correctly between C and C++.

I don't see why that should preclude us from diagnosing C code. Not everyone shares C and C++ code, and C developers should be alerted to this deprecation independent of what C++ is doing.

Once we've fixed C++ I'd like the diagnostic to be the same for both (modulo which standard it's deprecated in). I think we should add the diagnostic for C and C++ at the same time, and we should make sure there's no libc++ nor clang work required before diagnosing.

Given that C has already made this decision, it seems like the tail wagging the dog to wait for C++ to maybe adopt the same thing as part of an NB comment.

I'm sorry if I'm being dense here, but I am still not seeing the issue you're concerned by. C has already deprecated this feature and is planning to remove it shortly. I am diagnosing that fact in C, which seems perfectly appropriate to do regardless of what C++ is doing in this space.

Users have a portable way to write their code already because ATOMIC_VAR_INIT does not do any special magic and no implementation requires a user to use it to achieve portable atomic initialization semantics. If they get the diagnostic (which only triggers in C mode and only if the macro was defined in stdatomic.h), they should remove the use of ATOMIC_VAR_INIT from their code, or disable the deprecation diagnostic. If neither of those options appeals to the user, they can write something along the lines of:

_Atomic(int) i = 
#if defined(__cplusplus)
ATOMIC_VAR_INIT(12);
#else
12;
#endif

(not that I think anyone will want to write that, but strict adherence to a broken part of both standards does not seem like something we want to encourage anyway -- this is deprecated functionality, so the whole point is to discourage its use.)

Is this correct for all implementations of _Atomic and std::atomic that clang supports, including non-lock-free types? We should make sure before diagnosing.

Let me put it another way: code that does not work with = 12 will similarly not work with = ATOMIC_VAR_INIT(12) because the two are identical (modulo a ParenExpr) after preprocessing. So yes, I believe this is "correct" for all implementations of _Atomic and std::atomic that Clang supports insomuch as it's bug-for-bug-or-lack-thereof compatible.

I feel like we're going in circles, so perhaps if I explained my problem a different way, it will resonate more. I am worried about C developers writing C code against a C compiler when the C committee has deprecated ATOMIC_VAR_INIT and want to diagnose this situation in C mode. What I hear you being concerned with is: what about C++ developers? I've pointed out that they have several options if they really want full portability to any compiler in C or C++ mode, but beyond that, I am not concerned about C++ developers. They have ATOMIC_VAR_INIT and will until such time as WG21 decides to deprecate it, at which point I would like to diagnose in C++ mode as well.

include/clang/Basic/DiagnosticLexKinds.td

 def warn_pp_macro_hides_keyword : Extension<
   "keyword is hidden by macro definition">, InGroup<KeywordAsMacro>;
 def warn_pp_macro_is_reserved_id : Warning<
   "macro name is a reserved identifier">, DefaultIgnore,
   InGroup<ReservedIdAsMacro>;
 def warn_pp_objc_macro_redef_ignored : Warning<
   "ignoring redefinition of Objective-C qualifier macro">,
   InGroup<DiagGroup<"objc-macro-redefinition">>;
+def warn_pp_macro_deprecated : Warning<"%0 is deprecated">,
+  InGroup<Deprecated>;
 
 def pp_invalid_string_literal : Warning<
   "invalid string literal, ignoring final '\\'">;
 def warn_pp_expr_overflow : Warning<
   "integer overflow in preprocessor expression">;
 def warn_pp_convert_to_positive : Warning<
   "%select{left|right}0 side of operator converted from negative value to "
   "unsigned: %1">;

lib/Lex/PPMacroExpansion.cpp

     ++NumFnMacroExpanded;
   } else {
     ++NumMacroExpanded;
   }
 
   // Notice that this macro has been used.
   markMacroAsUsed(MI);
 
+  // C17 7.31.8p2
+  // If the macro is named ATOMIC_VAR_INIT and it came from the stdatomic.h
+  // header file, diagnose it as being deprecated. This macro does not exist
+  // in C++, so do not trigger a deprecation warning in that language mode.
+  if (!getLangOpts().CPlusPlus &&
+      Identifier.getIdentifierInfo()->isStr("ATOMIC_VAR_INIT") &&
+      llvm::sys::path::filename(getSourceManager().getFilename(
+          M.getLocalDirective()->getLocation())) == "stdatomic.h") {
+    Diag(Identifier, diag::warn_pp_macro_deprecated)
+        << Identifier.getIdentifierInfo();
+  }
+
   // Remember where the token is expanded.
   SourceLocation ExpandLoc = Identifier.getLocation();
   SourceRange ExpansionRange(ExpandLoc, ExpansionEnd);
 
   if (Callbacks) {
     if (InMacroArgs) {
       // We can have macro expansion inside a conditional directive while
       // reading the function macro arguments. To ensure, in that case, that

test/Preprocessor/Inputs/atomic_var_init/stdatomic.h

+#define ATOMIC_VAR_INIT(x) (x)

test/Preprocessor/atomic_var_init.c

+// RUN: %clang_cc1 %s -fsyntax-only -verify -Wdeprecated -isystem Inputs/atomic_var_init
+
+// Do not diagnose if the macro is not defined in stdatomic.h because it could
+// be a user-defined macro instead then.
+#define ATOMIC_VAR_INIT(x) (x)
+_Atomic int a = ATOMIC_VAR_INIT(12);
+#undef ATOMIC_VAR_INIT
+
+// Diagnose use of the macro from stdatomic.h
+#include <stdatomic.h>
+_Atomic int i = ATOMIC_VAR_INIT(12); // expected-warning {{'ATOMIC_VAR_INIT' is deprecated}}
+
+#define FOO(x) ATOMIC_VAR_INIT(x)
+_Atomic int j = FOO(12); // expected-warning {{'ATOMIC_VAR_INIT' is deprecated}}
+
+// Do not diagnose use in conditional inclusion directives.
+#ifdef ATOMIC_VAR_INIT
+#elif defined(ATOMIC_VAR_INIT)
+#endif
+