[clang][AST] fix ast-print of extern <lang> with >=2 declarators, fixed #93913
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang Author: Artem Yurchenko (temyurchenko) ChangesFull diff: https://github.com/llvm/llvm-project/pull/93913.diff 2 Files Affected:
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 0cf4e64f83b8d..2018ebea67bf9 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -633,7 +633,7 @@ static void printExplicitSpecifier(ExplicitSpecifier ES, llvm::raw_ostream &Out,
Out << Proto;
}
-static void MaybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,
+static void maybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,
QualType T,
llvm::raw_ostream &Out) {
StringRef prefix = T->isClassType() ? "class "
@@ -643,6 +643,22 @@ static void MaybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,
Out << prefix;
}
+/// Return the language of the linkage spec of `D`, if applicable.
+///
+/// \Return - "C" if `D` has been declared with unbraced `extern "C"`
+/// - "C++" if `D` has been declared with unbraced `extern "C++"`
+/// - nullptr in any other case
+static const char *tryGetUnbracedLinkageLanguage(const Decl *D) {
+ const auto *SD = dyn_cast<LinkageSpecDecl>(D->getDeclContext());
+ if (!SD || SD->hasBraces())
+ return nullptr;
+ if (SD->getLanguage() == LinkageSpecLanguageIDs::C)
+ return "C";
+ assert(SD->getLanguage() == LinkageSpecLanguageIDs::CXX &&
+ "unknown language in linkage specification");
+ return "C++";
+}
+
void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
if (!D->getDescribedFunctionTemplate() &&
!D->isFunctionTemplateSpecialization()) {
@@ -662,6 +678,8 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
CXXConversionDecl *ConversionDecl = dyn_cast<CXXConversionDecl>(D);
CXXDeductionGuideDecl *GuideDecl = dyn_cast<CXXDeductionGuideDecl>(D);
if (!Policy.SuppressSpecifiers) {
+ if (const char *Lang = tryGetUnbracedLinkageLanguage(D))
+ Out << "extern \"" << Lang << "\" ";
switch (D->getStorageClass()) {
case SC_None: break;
case SC_Extern: Out << "extern "; break;
@@ -807,7 +825,7 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
}
if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
!Policy.SuppressUnwrittenScope)
- MaybePrintTagKeywordIfSupressingScopes(Policy, AFT->getReturnType(),
+ maybePrintTagKeywordIfSupressingScopes(Policy, AFT->getReturnType(),
Out);
AFT->getReturnType().print(Out, Policy, Proto);
Proto.clear();
@@ -932,6 +950,8 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
: D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
if (!Policy.SuppressSpecifiers) {
+ if (const char *Lang = tryGetUnbracedLinkageLanguage(D))
+ Out << "extern \"" << Lang << "\" ";
StorageClass SC = D->getStorageClass();
if (SC != SC_None)
Out << VarDecl::getStorageClassSpecifierString(SC) << " ";
@@ -961,7 +981,7 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
!Policy.SuppressUnwrittenScope)
- MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);
+ maybePrintTagKeywordIfSupressingScopes(Policy, T, Out);
printDeclType(T, (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
D->getIdentifier())
@@ -1064,6 +1084,8 @@ void DeclPrinter::VisitNamespaceAliasDecl(NamespaceAliasDecl *D) {
void DeclPrinter::VisitEmptyDecl(EmptyDecl *D) {
prettyPrintAttributes(D);
+ if (const char *Lang = tryGetUnbracedLinkageLanguage(D))
+ Out << "extern \"" << Lang << "\";";
}
void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
@@ -1136,22 +1158,21 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
}
void DeclPrinter::VisitLinkageSpecDecl(LinkageSpecDecl *D) {
- const char *l;
+ if (!D->hasBraces()) {
+ VisitDeclContext(D);
+ return;
+ }
+ const char *L;
if (D->getLanguage() == LinkageSpecLanguageIDs::C)
- l = "C";
+ L = "C";
else {
assert(D->getLanguage() == LinkageSpecLanguageIDs::CXX &&
"unknown language in linkage specification");
- l = "C++";
+ L = "C++";
}
-
- Out << "extern \"" << l << "\" ";
- if (D->hasBraces()) {
- Out << "{\n";
- VisitDeclContext(D);
- Indent() << "}";
- } else
- Visit(*D->decls_begin());
+ Out << "extern \"" << L << "\" {\n";
+ VisitDeclContext(D);
+ Indent() << "}";
}
void DeclPrinter::printTemplateParameters(const TemplateParameterList *Params,
diff --git a/clang/test/AST/ast-print-language-linkage.cpp b/clang/test/AST/ast-print-language-linkage.cpp
new file mode 100644
index 0000000000000..7e4dc3f25f062
--- /dev/null
+++ b/clang/test/AST/ast-print-language-linkage.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -ast-print %s -o - | FileCheck %s
+
+// CHECK: extern "C" int printf(const char *, ...);
+extern "C" int printf(const char *...);
+
+// CHECK: extern "C++" int f(int);
+// CHECK-NEXT: extern "C++" int g(int);
+extern "C++" int f(int), g(int);
+
+// CHECK: extern "C" char a;
+// CHECK-NEXT: extern "C" char b;
+extern "C" char a, b;
+
+// CHECK: extern "C" {
+// CHECK-NEXT: void foo();
+// CHECK-NEXT: int x;
+// CHECK-NEXT: int y;
+// CHECK-NEXT: extern short z;
+// CHECK-NEXT: }
+extern "C" {
+ void foo(void);
+ int x, y;
+ extern short z;
+}
+
+// CHECK: extern "C" {
+// CHECK-NEXT: }
+extern "C" {}
+
+// CHECK: extern "C++";
+extern "C++";
|
temyurchenko
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This is a relanding of #93131. The first commit is the same, the second commit presents and fixes the issue from the linked discussion. cc @erichkeane, @AaronBallman, @gulfemsavrun. |
|
I see the 2nds commit doesn't add any tests! Please make it do so, else LGTM (plus might want to do a 'merge' commit to reset the CI to a more stable state). |
I've tried and I'm not quite sure how to do it. The issue is we need to test AST printing of «implicitly declared» functions, such as builtins and functions instrumented by lldb, such as Furthermore, all that the second commit does is it adds an assertion of an invariant and then fixes the places in the code where the invariant breaks. Is it fair to say that the codebase itself is the test for the invariant? If any codepath would break the invariant, the assertion would be triggered. Not on a release build though, if that's a problem. |
Hmm... I don't have a great idea. Is using it not enough to get it to be emitted? I see here we don't seem to cause them to be emitted: https://godbolt.org/z/nYzMca7Te Perhaps @AaronBallman has an idea. |
Yes, I get the same result. I also tried adding a test in |
temyurchenko
force-pushed
the
main
branch
2 times, most recently
from
9b18a5c to
d908fe1
Compare
…lvm#93131) Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon. Solution: for unbraced case we traverse all declarators via `VisitDeclContext`. Furthermore, in appropriate visitors we query for whether they are a part of the unbraced extern language linkage spec, and if so, print appropriately.
In a real-world case with functions that have many, many R_RISCV_CALL_PLT relocations due to asan and ubsan instrumentation, all these can be relaxed by an instruction and the net result is more than 65536 bytes of reduction in the output .text section that totals about 1.2MiB in final size. This changes InputSection to use a 32-bit field for bytesDropped. The RISCV relaxation keeps track in a 64-bit field and detects 32-bit overflow as it previously detected 16-bit overflow. It doesn't seem likely that 32-bit overflow will arise, but it's not inconceivable and it's cheap enough to detect it. This unfortunately increases the size of InputSection on 64-bit hosts by a word, but that seems hard to avoid. Reviewed By: MaskRay Differential Revision: https://reviews.llvm.org/D150722
clang/lib/AST/Decl.cpp
Outdated
| if (!SD->hasBraces()) | ||
| return true; |
There was a problem hiding this comment.
| if (!SD->hasBraces()) | |
| return true; | |
| return !SD->hasBraces(); |
| @@ -2380,7 +2380,7 @@ FunctionDecl *Sema::CreateBuiltin(IdentifierInfo *II, QualType Type, | |||
| } | |||
|
|
|||
| FunctionDecl *New = FunctionDecl::Create(Context, Parent, Loc, Loc, II, Type, | |||
| /*TInfo=*/nullptr, SC_Extern, | |||
| /*TInfo=*/nullptr, SC_None, | |||
There was a problem hiding this comment.
This change looks suspicious to me -- this is creating a builtin function, which should be modeled as an extern. In C++, you seem to be relying on the language linkage being sufficient for that, but C has no notion of language linkage and so I worry that the declaration for the builtin will be incorrectly handled in C.
There was a problem hiding this comment.
You are right, I also paid attention to that issue. However, when a storage class specifier is omitted for a function, it's storage class is implicitly supposed to be extern. Thus, it's fine.
Furthermore, the meaning of this field according to the documentation comments is not for the actual storage duration or linkage, but for the storage-class specifier as present in the source code. Since builtins aren't really present in the source code, it seems further reasonable to omit a specifier.
There was a problem hiding this comment.
You are right, I also paid attention to that issue. However, when a storage class specifier is omitted for a function, it's storage class is implicitly supposed to be extern. Thus, it's fine.
That's not actually correct -- the declaration of a function at block scope should definitely not be extern, it should have no linkage: https://godbolt.org/z/81fMaPaTq
Furthermore, the meaning of this field according to the documentation comments is not for the actual storage duration or linkage, but for the storage-class specifier as present in the source code. Since builtins aren't really present in the source code, it seems further reasonable to omit a specifier.
Builtins should behave as-if they were declared in a header file that was included in the TU, and so they typically would be marked as extern.
There was a problem hiding this comment.
That's not actually correct -- the declaration of a function at block scope should definitely not be extern, it should have no linkage: https://godbolt.org/z/81fMaPaTq
I may be wrong to rely on cppreference, but the following page tells that indeed a function at a block scope has implicit extern, which could also be made explicit: https://en.cppreference.com/w/c/language/storage_duration.
I also checked your example locally by compiling (via clang) main.c with an additional defs.c:
main.c:
int main(void) {
void f();
extern void g();
f();
g();
return 0;
}defs.c:
#include <stdio.h>
void f() {
printf("f\n");
}
void g() {
printf("g\n");
}The executable works as expected.
I will also take a look at the C99 standard draft.
There was a problem hiding this comment.
I will also take a look at the C99 standard draft.
Paragraph 6.2.2.5 says:
If the declaration of an identifier for a function has no storage-class specifier, its linkage
is determined exactly as if it were declared with the storage-class specifier extern. If
the declaration of an identifier for an object has file scope and no storage-class specifier,
its linkage is external.
Thus, seemingly confirming that a function declared at the block scope is declared with an implicit extern.
There was a problem hiding this comment.
Ah, you're right, it's only objects declared at local scope that have no linkage (the following paragraph), not functions.
There was a problem hiding this comment.
I think I've come around to being okay with these changes, thank you!
| @@ -6286,7 +6286,7 @@ static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext &Context, | |||
| FunctionDecl *OverloadDecl = FunctionDecl::Create( | |||
| Context, Parent, FDecl->getLocation(), FDecl->getLocation(), | |||
| FDecl->getIdentifier(), OverloadTy, | |||
| /*TInfo=*/nullptr, SC_Extern, Sema->getCurFPFeatures().isFPConstrained(), | |||
| /*TInfo=*/nullptr, SC_None, Sema->getCurFPFeatures().isFPConstrained(), | |||
There was a problem hiding this comment.
Same concern here as above.
clang/lib/AST/Decl.cpp
Outdated
| if (!DC) | ||
| return false; | ||
| if (const auto *SD = dyn_cast<LinkageSpecDecl>(DC)) | ||
| return !SD->hasBraces(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
| if (!DC) | |
| return false; | |
| if (const auto *SD = dyn_cast<LinkageSpecDecl>(DC)) | |
| return !SD->hasBraces(); | |
| return false; | |
| } | |
| if (const auto *SD = dyn_cast_if_present<LinkageSpecDecl>(DC)) | |
| return !SD->hasBraces(); | |
| return false; | |
| } |
Sorry for not noticing this earlier, but we can simplify even further this way.
| @@ -2380,7 +2380,7 @@ FunctionDecl *Sema::CreateBuiltin(IdentifierInfo *II, QualType Type, | |||
| } | |||
|
|
|||
| FunctionDecl *New = FunctionDecl::Create(Context, Parent, Loc, Loc, II, Type, | |||
| /*TInfo=*/nullptr, SC_Extern, | |||
| /*TInfo=*/nullptr, SC_None, | |||
There was a problem hiding this comment.
I think I've come around to being okay with these changes, thank you!
Quoting 9.11.8: > A declaration directly contained in a linkage-specification is > treated as if it contains the extern specifier (9.2.2) for the > purpose of determining the linkage of the declared name and whether > it is a definition. Such a declaration shall not specify a storage > class. So, the invariant is that function and variable declarations within an unbraced language linkage specification have `StorageClass` set to `SC_None`. Problem: in several places the invariant was broken. Solution: remove the explicit `SC_Extern` specification. Note, that my changes result in the `extern` being implicit in some functions that are not contained in a language linkage specification.
|
Do you need someone to land these changes on your behalf? |
Yeah, I don't have the rights :( |
@erichkeane, perhaps you can land these, given that the PR is approved? |
|
Hey! Looks like this commit broke some tests on the LLDB macOS buildbots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/6805/console In the tests |
|
I am part of the Fuchsia toolchain team. Our Clang toolchain CI builders crash and I suspect this change is the reason. I am verifying that. But here's the log: and the crash: |
|
I reverted these changes in 71ff749 so @temyurchenko can investigate the issues. |
|
Chrome's Windows debug builds also saw crashes due to this commit: https://crbug.com/350541784 |
No description provided.