Skip to content

[clang][AST] fix ast-print of extern <lang> with >=2 declarators, fixed #93913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 1, 2024
Merged

[clang][AST] fix ast-print of extern <lang> with >=2 declarators, fixed #93913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
410c7ba
[clang][AST] fix ast-print of `extern <lang>` with >=2 declarators (#…
temyurchenko May 30, 2024
b9fa7fe
[clang][AST] enforce the unbraced `extern <lang>` invariant
temyurchenko Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions clang/lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,13 +576,16 @@ template <typename T> static bool isFirstInExternCContext(T *D) {
return First->isInExternCContext();
}

static bool isSingleLineLanguageLinkage(const Decl &D) {
if (const auto *SD = dyn_cast<LinkageSpecDecl>(D.getDeclContext()))
if (!SD->hasBraces())
return true;
static bool isUnbracedLanguageLinkage(const DeclContext *DC) {
if (const auto *SD = dyn_cast_if_present<LinkageSpecDecl>(DC))
return !SD->hasBraces();
return false;
}

static bool hasUnbracedLanguageLinkage(const Decl &D) {
return isUnbracedLanguageLinkage(D.getDeclContext());
}

static bool isDeclaredInModuleInterfaceOrPartition(const NamedDecl *D) {
if (auto *M = D->getOwningModule())
return M->isInterfaceOrPartition();
Expand Down Expand Up @@ -644,7 +647,7 @@ LinkageComputer::getLVForNamespaceScopeDecl(const NamedDecl *D,

if (Var->getStorageClass() != SC_Extern &&
Var->getStorageClass() != SC_PrivateExtern &&
!isSingleLineLanguageLinkage(*Var))
!hasUnbracedLanguageLinkage(*Var))
return LinkageInfo::internal();
}

Expand Down Expand Up @@ -2133,6 +2136,12 @@ VarDecl::VarDecl(Kind DK, ASTContext &C, DeclContext *DC,
"ParmVarDeclBitfields too large!");
static_assert(sizeof(NonParmVarDeclBitfields) <= sizeof(unsigned),
"NonParmVarDeclBitfields too large!");

// The unbraced `extern "C"` invariant is that the storage class
// specifier is omitted in the source code, i.e. SC_None (but is,
// implicitly, `extern`).
assert(!isUnbracedLanguageLinkage(DC) || SC == SC_None);

AllBits = 0;
VarDeclBits.SClass = SC;
// Everything else is implicitly initialized to false.
Expand Down Expand Up @@ -2316,7 +2325,7 @@ VarDecl::isThisDeclarationADefinition(ASTContext &C) const {
// A declaration directly contained in a linkage-specification is treated
// as if it contains the extern specifier for the purpose of determining
// the linkage of the declared name and whether it is a definition.
if (isSingleLineLanguageLinkage(*this))
if (hasUnbracedLanguageLinkage(*this))
return DeclarationOnly;

// C99 6.9.2p2:
Expand Down Expand Up @@ -3041,6 +3050,12 @@ FunctionDecl::FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC,
DeclContext(DK), redeclarable_base(C), Body(), ODRHash(0),
EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
assert(T.isNull() || T->isFunctionType());

// The unbraced `extern "C"` invariant is that the storage class
// specifier is omitted in the source code, i.e. SC_None (but is,
// implicitly, `extern`).
assert(!isUnbracedLanguageLinkage(DC) || S == SC_None);

FunctionDeclBits.SClass = S;
FunctionDeclBits.IsInline = isInlineSpecified;
FunctionDeclBits.IsInlineSpecified = isInlineSpecified;
Expand Down
55 changes: 41 additions & 14 deletions clang/lib/AST/DeclPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand All @@ -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()) {
Expand All @@ -662,6 +678,11 @@ 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)) {
// the "extern" specifier is implicit
assert(D->getStorageClass() == SC_None);
Out << "extern \"" << Lang << "\" ";
}
switch (D->getStorageClass()) {
case SC_None: break;
case SC_Extern: Out << "extern "; break;
Expand Down Expand Up @@ -807,7 +828,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();
Expand Down Expand Up @@ -932,6 +953,11 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
: D->getASTContext().getUnqualifiedObjCPointerType(D->getType());

if (!Policy.SuppressSpecifiers) {
if (const char *Lang = tryGetUnbracedLinkageLanguage(D)) {
// the "extern" specifier is implicit
assert(D->getStorageClass() == SC_None);
Out << "extern \"" << Lang << "\" ";
}
StorageClass SC = D->getStorageClass();
if (SC != SC_None)
Out << VarDecl::getStorageClassSpecifierString(SC) << " ";
Expand Down Expand Up @@ -961,7 +987,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())
Expand Down Expand Up @@ -1064,6 +1090,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) {
Expand Down Expand Up @@ -1136,22 +1164,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,
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@temyurchenko temyurchenko Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, it's only objects declared at local scope that have no linkage (the following paragraph), not functions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've come around to being okay with these changes, thank you!

getCurFPFeatures().isFPConstrained(),
false, Type->isFunctionProtoType());
New->setImplicit();
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern here as above.

false,
/*hasPrototype=*/true);
SmallVector<ParmVarDecl*, 16> Params;
Expand Down
31 changes: 31 additions & 0 deletions clang/test/AST/ast-print-language-linkage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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++";
2 changes: 1 addition & 1 deletion lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ class CodeComplete : public CodeCompleteConsumer {
else
ToInsert += "(";
raw_string_ostream OS(Description);
F->print(OS, m_desc_policy, false);
F->print(OS, m_desc_policy);
OS.flush();
} else if (const VarDecl *V = dyn_cast<VarDecl>(D)) {
Description = V->getType().getAsString(m_desc_policy);
Expand Down
3 changes: 2 additions & 1 deletion lldb/source/Plugins/ExpressionParser/Clang/NameSearchContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ clang::NamedDecl *NameSearchContext::AddFunDecl(const CompilerType &type,

clang::FunctionDecl *func_decl = FunctionDecl::Create(
ast, context, SourceLocation(), SourceLocation(), decl_name, qual_type,
nullptr, SC_Extern, /*UsesFPIntrin=*/false, isInlineSpecified, hasWrittenPrototype,
nullptr, SC_None, /*UsesFPIntrin=*/false, isInlineSpecified,
hasWrittenPrototype,
isConstexprSpecified ? ConstexprSpecKind::Constexpr
: ConstexprSpecKind::Unspecified);

Expand Down
Loading