clang: Remove __atomic libstdc++ hack

[urnathan]

Summary

This hack was added back in 2012 for libstdc++4.6. The last 4.6
release was 2013. The commit was 45bb8855e0ecc, Richard Smith, Thu
Oct 4 22:13:39 2012 +0000. If someone really wants to use clang with
such a libstdc++, well, they know where to get one :)

but that;s just MHO

[aaron.ballman]

I am fine removing this hack for such an old version of libstdc++, but it looks like there are two test failures that should be addressed:

https://reviews.llvm.org/harbormaster/unit/view/552317/
https://reviews.llvm.org/harbormaster/unit/view/552314/

One question I do have is whether we have a documented compatibility statement anywhere about what versions of libstdc++ (or libc++ for that matter) we support and whether that needs to be updated. I couldn't find one when I looked, but I may have looked in the wrong places.

Also, should we mention something in the release notes about no longer supporting this version of libstdc++?

[ldionne]

Not a libstdc++ or Clang maintainer, but I think it's reasonable to remove this.

[jwakely]

LGTM.

I don't even remember those namespaces existing, but I don't think it's reasonable to expect current clang to support ancient (and broken) headers.

[aaron.ballman]

In D100465#2695077, @aaron.ballman wrote:

One question I do have is whether we have a documented compatibility statement anywhere about what versions of libstdc++ (or libc++ for that matter) we support and whether that needs to be updated. I couldn't find one when I looked, but I may have looked in the wrong places.

Aha, I *knew* we had something somewhere. We should probably update this: https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst#libstdc-gnu

[jwakely]

Not sure if I should have "accepted" this, I'm not an approver.

[aaron.ballman]

In D100465#2695288, @jwakely wrote:

Not sure if I should have "accepted" this, I'm not an approver.

No worries! FWIW, the patch approval documentation is at https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted but basically, anyone can approve but code should only go in once there's consensus including people with technical expertise in the area of the code being changed.

I'll "request changes" so it's clear there's a bit more to be done here.

[urnathan]

Fix failing test -- I must have confused myself

[urnathan]

Added ReleaseNote about libstdc++ -- is that the best place to document such a requirement?

[rsmith]

From a process standpoint, I think the best thing would be to first send out a patch adding a release note saying we no longer support libstdc++ < 4.8.3 and updating the part of docs/Toolchain.rst that Aaron linked to, and send a brief note to cfe-dev drawing people's attention to that patch and asking for objections -- I don't think you need to wait for more than a couple of days for such objections, but as this is effectively a policy change we should probably give people a little time to respond. Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

clang/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp

6

[rsmith]

In D100465#2695691, @rsmith wrote:

Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

Actually, you know what, I don't really see the point in splitting up these two steps. So I think my suggestion reduces to: let's mail cfe-dev to draw attention to this and go ahead in a couple of days if we don't have sustained objections.

[aaron.ballman]

In D100465#2695705, @rsmith wrote:

In D100465#2695691, @rsmith wrote:

Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

Actually, you know what, I don't really see the point in splitting up these two steps. So I think my suggestion reduces to: let's mail cfe-dev to draw attention to this and go ahead in a couple of days if we don't have sustained objections.

+1, I think that should alleviate most concerns.

[urnathan]

Ok, hopefully right this time. I went with v4.9, as that's the version Richard mentioned in D100469, I just confused myself with 4.8.3. will email cfe-dev

[urnathan]

Change to 4.8.3 as oldest version

[urnathan]

Set 4.8.3 as oldest supported

[aaron.ballman]

LGTM assuming community doesn't find a reason to change the supported version for some reason.

clang/docs/ReleaseNotes.rst

 C Language Changes in Clang
 ---------------------------
 
 - ...
 
 C++ Language Changes in Clang
 -----------------------------
 
+- The oldest supported GNU libstdc++ is now 4.8.3 (released
+  2014-05-22).  Clang workarounds for bugs in earlier versions have
+  been removed.
+
 - ...
 
-C++1z Feature Support
+C++20 Feature Support
+^^^^^^^^^^^^^^^^^^^^^
+...
+
+C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 ...
 
 Objective-C Language Changes in Clang
 -------------------------------------
 
 OpenCL C Language Changes in Clang
 ----------------------------------

clang/lib/Sema/SemaDeclCXX.cpp

 /// Diagnose a mismatch in 'inline' qualifiers when a namespace is
 /// reopened.
 static void DiagnoseNamespaceInlineMismatch(Sema &S, SourceLocation KeywordLoc,
                                             SourceLocation Loc,
                                             IdentifierInfo *II, bool *IsInline,
                                             NamespaceDecl *PrevNS) {
   assert(*IsInline != PrevNS->isInline());
 
-  // HACK: Work around a bug in libstdc++4.6's <atomic>, where
-  // std::__atomic[0,1,2] are defined as non-inline namespaces, then reopened as
-  // inline namespaces, with the intention of bringing names into namespace std.
-  //
-  // We support this just well enough to get that case working; this is not
-  // sufficient to support reopening namespaces as inline in general.
-  if (*IsInline && II && II->getName().startswith("__atomic") &&
-      S.getSourceManager().isInSystemHeader(Loc)) {
-    // Mark all prior declarations of the namespace as inline.
-    for (NamespaceDecl *NS = PrevNS->getMostRecentDecl(); NS;
-         NS = NS->getPreviousDecl())
-      NS->setInline(*IsInline);
-    // Patch up the lookup table for the containing namespace. This isn't really
-    // correct, but it's good enough for this particular case.
-    for (auto *I : PrevNS->decls())
-      if (auto *ND = dyn_cast<NamedDecl>(I))
-        PrevNS->getParent()->makeDeclVisibleInContext(ND);
-    return;
-  }
-
   if (PrevNS->isInline())
     // The user probably just forgot the 'inline', so suggest that it
     // be added back.
     S.Diag(Loc, diag::warn_inline_namespace_reopened_noninline)
       << FixItHint::CreateInsertion(KeywordLoc, "inline ");
   else
     S.Diag(Loc, diag::err_inline_namespace_mismatch);
 

clang/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp

 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
 
 // libstdc++ 4.6.x contains a bug where it defines std::__atomic[0,1,2] as a
 // non-inline namespace, then selects one of those namespaces and reopens it
 // as inline, as a strange way of providing something like a using-directive.
 // Clang has an egregious hack to work around the problem, by allowing a
 // namespace to be converted from non-inline to inline in this one specific
 // case.
 
+// the last 4.6 release was 2013, so the hack is removed.  This checks __atomic
+// is not special.
 #ifdef BE_THE_HEADER
 
 #pragma clang system_header
 
 namespace std {
-  namespace __atomic0 {
-    typedef int foobar;
-  }
-  namespace __atomic1 {
-    typedef void foobar;
-  }
+namespace __atomic0 { // expected-note {{previous definition}}
+} // namespace __atomic0
+} // namespace __atomic1
 
-  inline namespace __atomic0 {}
-}
+inline namespace __atomic0 {} // expected-error {{cannot be reopened as inline}}
+} // namespace std
 
 #else
 
 #define BE_THE_HEADER
 #include "libstdcxx_atomic_ns_hack.cpp"
 
-std::foobar fb;
-
-using T = void; // expected-note {{here}}
-using T = std::foobar; // expected-error {{different types ('std::foobar' (aka 'int') vs 'void')}}
+std::foobar fb; // expected-error {{no type named 'foobar' in namespace}}
 
 #endif