[DeclCXX] Remove unknown external linkage specifications
ClosedPublic
Actions

Authored by ekatz on Nov 7 2019, 2:45 AM.

Details

Reviewers

SouraVX
aprantl
dblaikie
JDevlieghere
uabelho
rsmith

Commits

rGc63f1b160eb6: [DeclCXX] Remove unknown external linkage specifications

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.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

ekatz created this revision.Nov 7 2019, 2:45 AM

Herald added a subscriber: cfe-commits. · View Herald TranscriptNov 7 2019, 2:45 AM

"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)

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".

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).

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++.

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?

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 ??

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.

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.

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)

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 added inline comments.Nov 10 2019, 9:21 AM

clang/include/clang/AST/DeclCXX.h
2759–2760	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.)

Removed DWARF reference from LinkageSpecDecl::LanguageIDs.

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.

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.

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?

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?