[DeclCXX] Remove unknown external linkage specifications

[ekatz]

Summary

Partial revert of r372681 "Support for DWARF-5 C++ language tags".

The change introduced new external linkage languages ("C++11" and "C++14") which not supported in C++.

It also changed the definition of the existing enum to use the DWARF constants. The problem is that "LinkageSpecDeclBits.Language" (the field that reserves this enum) is actually defined as 3 bits length (bitfield), which cannot contain the new DWARF constants. Defining the enum as integer literals is more appropriate for maintaining valid values.

[dblaikie]

"The change introduced new external linkage languages ("C++11" and "C++14") which not supported in C++."

Could you describe more what you mean by "not supported in C++"? These identifiers aren't part of the C++ standard/they're an implementation detail of the compiler.
Also, if this is breaking functionality, could you provide a test case demonstrating the failure/problem? (& if it isn't breaking anything - some more detail on why it's a problem (perhaps it makes the code more difficult to maintain, etc) would be helpful)

[ekatz]

With this change (current trunk) you can write code as follows:

extern "C++11" int x;

And it will pass compilation. No other compiler support it, nor it should, as there is no such thing as extern "C++11" nor extern "C++14".

[dblaikie]

In D69935#1737615, @ekatz wrote:

With this change (current trunk) you can write code as follows:

extern "C++11" int x;

And it will pass compilation. No other compiler support it, nor it should, as there is no such thing as extern "C++11" nor extern "C++14".

Ah, thanks - fair enough. This should probably include a test case to demonstrate that? (though I suppose it's sort of hard to imagine why such a test case would be useful but for this particular bug & adding a test case would only cover one possible spelling here)

Are all the changes proposed here necessary to fix that bug?

But yeah, in general, it looks like these constants really aren't suitable for C++11/14 data at all. They're about C V C++ linkage/mangling/etc & just happened to use the DWARF constants for some reason (mentioned in the comment above, though I don't find it super compelling).

[ekatz]

Yes, these changes regard to the same thing - the removal of the "Linkage-Spec LanguageIDs". As you described it nicely, it is for linkage only, meaning C vs C++.

[uabelho]

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

[SouraVX]

I created that patch, for purpose of emitting C++ language standards C++11, C++14 in the debug information to be available for consumer's[GDB,LLDB].
DW_TAG_lang_c_plus_plus_11 .. 14 --new tags for languages in DWARF5.
Sorry, I think, I missed this unintended additions in LinkageSpecDecl.

BTW, did you plan to completely revert this ??

[ekatz]

In D69935#1738311, @SouraVX wrote:

I created that patch, for purpose of emitting C++ language standards C++11, C++14 in the debug information to be available for consumer's[GDB,LLDB].
DW_TAG_lang_c_plus_plus_11 .. 14 --new tags for languages in DWARF5.
Sorry, I think, I missed this unintended additions in LinkageSpecDecl.

BTW, did you plan to completely revert this ??

This is only a partial revert. It reverts only the part of the linkage languages. The other parts changed are untouched. You have also added a test, which should still pass.

[SouraVX]

In D69935#1738563, @ekatz wrote:

In D69935#1738311, @SouraVX wrote:

I created that patch, for purpose of emitting C++ language standards C++11, C++14 in the debug information to be available for consumer's[GDB,LLDB].
DW_TAG_lang_c_plus_plus_11 .. 14 --new tags for languages in DWARF5.
Sorry, I think, I missed this unintended additions in LinkageSpecDecl.

BTW, did you plan to completely revert this ??

This is only a partial revert. It reverts only the part of the linkage languages. The other parts changed are untouched. You have also added a test, which should still pass.

Thank you! +1 from my side. I'm kinda still new here. So you can wait for acceptance from other reviewers.

[dblaikie]

In D69935#1738273, @uabelho wrote:

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

+1 to this & then it seems fine (probably without a committed test, but I wouldn't mind a copy/paste of clang's behavior before/after this patch posted to this review to demonstrate what is being fixed)

[ekatz]

No problem.
For the source code:

extern "C++11" int x;

In clang 9.0 the compilation fails with the message:

<source>:1:8: error: unknown linkage language
extern "C++11" int x;
       ^~~~~~~
1 error generated.

while on the trunk it just pass without any error.

[rsmith]

clang/include/clang/AST/DeclCXX.h

2762

Using DWARF encodings here does nothing for AST format stability (compared to using our own numbering system), and is clearly creating confusion. Please just number these sequentially and remove the mention of DWARF here. We don't have a stable AST format across major releases, so it's fine to change the numbers (and hence the AST format) on master. (We don't even have working stability across patch releases yet, but should change/fix that at some point.)

[ekatz]

Removed DWARF reference from LinkageSpecDecl::LanguageIDs.

[dblaikie]

In D69935#1739235, @dblaikie wrote:

In D69935#1738273, @uabelho wrote:

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

+1 to this & then it seems fine (probably without a committed test, but I wouldn't mind a copy/paste of clang's behavior before/after this patch posted to this review to demonstrate what is being fixed)

Waiting on this bit.

[ekatz]

In D69935#1741268, @dblaikie wrote:

In D69935#1739235, @dblaikie wrote:

+1 to this & then it seems fine (probably without a committed test, but I wouldn't mind a copy/paste of clang's behavior before/after this patch posted to this review to demonstrate what is being fixed)

Waiting on this bit.

I described an expected behavior in D69935#1739871.

[uabelho]

In D69935#1739235, @dblaikie wrote:

In D69935#1738273, @uabelho wrote:

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

+1 to this

Did you see the above?

[ekatz]

In D69935#1741995, @uabelho wrote:

In D69935#1739235, @dblaikie wrote:

In D69935#1738273, @uabelho wrote:

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

+1 to this

Did you see the above?

Sorry, I missed that (its from a separate commit). Added to the patch.

[dblaikie]

Looks good - thanks!

clang/include/clang/AST/DeclCXX.h

 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/iterator_range.h"
-#include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include "llvm/Support/TrailingObjects.h"
 #include <cassert>
 #include <cstddef>
 #include <iterator>
 #include <memory>
class LinkageSpecDecl : public Decl, public DeclContext {
   // This class stores some data in DeclContext::LinkageSpecDeclBits to save
   // some space. Use the provided accessors to access it.
 public:
   /// Represents the language in a linkage specification.
   ///
   /// The values are part of the serialization ABI for
   /// ASTs and cannot be changed without altering that ABI.  To help
   /// ensure a stable ABI for this, we choose the DW_LANG_ encodings
   /// from the dwarf standard.
   enum LanguageIDs {
-    lang_c = llvm::dwarf::DW_LANG_C,
-    lang_cxx = llvm::dwarf::DW_LANG_C_plus_plus,
-    lang_cxx_11 = llvm::dwarf::DW_LANG_C_plus_plus_11,
-    lang_cxx_14 = llvm::dwarf::DW_LANG_C_plus_plus_14
+    lang_c = /* DW_LANG_C */ 0x0002,
+    lang_cxx = /* DW_LANG_C_plus_plus */ 0x0004
   };
 
 private:
   /// The source location for the extern keyword.
   SourceLocation ExternLoc;
 
   /// The source location for the right brace (if valid).
   SourceLocation RBraceLoc;

clang/lib/AST/DeclPrinter.cpp

if (Policy.TerseOutput) {
       VisitDeclContext(D);
       Indent() << "}";
     }
   }
 }
 
 void DeclPrinter::VisitLinkageSpecDecl(LinkageSpecDecl *D) {
   const char *l;
-  switch (D->getLanguage()) {
-  case LinkageSpecDecl::lang_c:
+  if (D->getLanguage() == LinkageSpecDecl::lang_c)
     l = "C";
-    break;
-  case LinkageSpecDecl::lang_cxx_14:
-    l = "C++14";
-    break;
-  case LinkageSpecDecl::lang_cxx_11:
-    l = "C++11";
-    break;
-  case LinkageSpecDecl::lang_cxx:
+  else {
+    assert(D->getLanguage() == LinkageSpecDecl::lang_cxx &&
+           "unknown language in linkage specification");
     l = "C++";
-    break;
   }
 
   Out << "extern \"" << l << "\" ";
   if (D->hasBraces()) {
     Out << "{\n";
     VisitDeclContext(D);
     Indent() << "}";
   } else

clang/lib/AST/JSONNodeDumper.cpp

if (D->hasDefaultArgument())
     });
 }
 
 void JSONNodeDumper::VisitLinkageSpecDecl(const LinkageSpecDecl *LSD) {
   StringRef Lang;
   switch (LSD->getLanguage()) {
   case LinkageSpecDecl::lang_c: Lang = "C"; break;
   case LinkageSpecDecl::lang_cxx: Lang = "C++"; break;
-  case LinkageSpecDecl::lang_cxx_11:
-    Lang = "C++11";
-    break;
-  case LinkageSpecDecl::lang_cxx_14:
-    Lang = "C++14";
-    break;
   }
   JOS.attribute("language", Lang);
   attributeOnlyIfTrue("hasBraces", LSD->hasBraces());
 }
 
 void JSONNodeDumper::VisitAccessSpecDecl(const AccessSpecDecl *ASD) {
   JOS.attribute("access", createAccessSpecifier(ASD->getAccess()));
 }

clang/lib/AST/TextNodeDumper.cpp

 void TextNodeDumper::VisitLinkageSpecDecl(const LinkageSpecDecl *D) {
   switch (D->getLanguage()) {
   case LinkageSpecDecl::lang_c:
     OS << " C";
     break;
   case LinkageSpecDecl::lang_cxx:
     OS << " C++";
     break;
-  case LinkageSpecDecl::lang_cxx_11:
-    OS << " C++11";
-    break;
-  case LinkageSpecDecl::lang_cxx_14:
-    OS << " C++14";
-    break;
   }
 }
 
 void TextNodeDumper::VisitAccessSpecDecl(const AccessSpecDecl *D) {
   OS << ' ';
   dumpAccessSpecifier(D->getAccess());
 }
 

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::EmitObjCIvarInitializations(ObjCImplementationDecl *D) {
   D->addInstanceMethod(CTORMethod);
   CodeGenFunction(*this).GenerateObjCCtorDtorMethod(D, CTORMethod, true);
   D->setHasNonZeroConstructors(true);
 }
 
 // EmitLinkageSpec - Emit all declarations in a linkage spec.
 void CodeGenModule::EmitLinkageSpec(const LinkageSpecDecl *LSD) {
   if (LSD->getLanguage() != LinkageSpecDecl::lang_c &&
-      LSD->getLanguage() != LinkageSpecDecl::lang_cxx &&
-      LSD->getLanguage() != LinkageSpecDecl::lang_cxx_11 &&
-      LSD->getLanguage() != LinkageSpecDecl::lang_cxx_14) {
+      LSD->getLanguage() != LinkageSpecDecl::lang_cxx) {
     ErrorUnsupported(LSD, "linkage spec");
     return;
   }
 
   EmitDeclContext(LSD);
 }
 
 void CodeGenModule::EmitDeclContext(const DeclContext *DC) {

clang/lib/Sema/SemaDeclCXX.cpp

Decl *Sema::ActOnStartLinkageSpecification(Scope *S, SourceLocation ExternLoc,
   }
 
   StringRef Lang = Lit->getString();
   LinkageSpecDecl::LanguageIDs Language;
   if (Lang == "C")
     Language = LinkageSpecDecl::lang_c;
   else if (Lang == "C++")
     Language = LinkageSpecDecl::lang_cxx;
-  else if (Lang == "C++11")
-    Language = LinkageSpecDecl::lang_cxx_11;
-  else if (Lang == "C++14")
-    Language = LinkageSpecDecl::lang_cxx_14;
   else {
     Diag(LangStr->getExprLoc(), diag::err_language_linkage_spec_unknown)
       << LangStr->getSourceRange();
     return nullptr;
   }
 
   // FIXME: Add all the various semantics of linkage specifications
 

clang/lib/Sema/SemaModule.cpp

static void checkModuleImportContext(Sema &S, Module *M,
 
   if (auto *LSD = dyn_cast<LinkageSpecDecl>(DC)) {
     switch (LSD->getLanguage()) {
     case LinkageSpecDecl::lang_c:
       if (ExternCLoc.isInvalid())
         ExternCLoc = LSD->getBeginLoc();
       break;
     case LinkageSpecDecl::lang_cxx:
-    case LinkageSpecDecl::lang_cxx_11:
-    case LinkageSpecDecl::lang_cxx_14:
       break;
     }
     DC = LSD->getParent();
   }
 
   while (isa<LinkageSpecDecl>(DC) || isa<ExportDecl>(DC))
     DC = DC->getParent();