Page MenuHomePhabricator

[DeclCXX] Remove unknown external linkage specifications
ClosedPublic

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

Details

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

Event Timeline

ekatz created this revision.Nov 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)

ekatz added a comment.Nov 7 2019, 1:09 PM

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

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 added a comment.Nov 7 2019, 10:20 PM

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

ekatz added a comment.Nov 8 2019, 3:44 AM

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.

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.

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 added a comment.Nov 9 2019, 8:56 AM

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

ekatz updated this revision to Diff 228755.EditedNov 11 2019, 12:22 PM

Removed DWARF reference from LinkageSpecDecl::LanguageIDs.

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 added a comment.Nov 12 2019, 3:18 AM

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

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 updated this revision to Diff 228860.Nov 12 2019, 4:24 AM

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 accepted this revision.Nov 12 2019, 12:51 PM

Looks good - thanks!

This revision is now accepted and ready to land.Nov 12 2019, 12:51 PM
This revision was automatically updated to reflect the committed changes.