Skip to content

Commit

Permalink
Reland "Rework the printing of attributes (#87281)"
Browse files Browse the repository at this point in the history 
Original commit message:
"

Commit 46f3ade introduced a notion
of printing the attributes on the left to improve the printing of attributes
attached to variable declarations. The intent was to produce more GCC compatible
code because clang tends to print the attributes on the right hand side which is
not accepted by gcc.

This approach has increased the complexity in tablegen and the attrubutes
themselves as now the are supposed to know where they could appear. That lead to
mishandling of the `override` keyword which is modelled as an attribute in
clang.

This patch takes an inspiration from the existing approach and tries to keep the
position of the attributes as they were written. To do so we use simpler
heuristic which checks if the source locations of the attribute precedes the
declaration. If so, it is considered to be printed before the declaration.

Fixes #87151
"

The reason for the bot breakage is that attributes coming from ApiNotes are not
marked implicit even though they do not have source locations. This caused an
assert to trigger. This patch forces attributes with no source location
information to be printed on the left. That change is consistent to the overall
intent of the change to increase the chances for attributes to compile across
toolchains and at the same time the produced code to be as close as possible to
the one written by the user.
  • Loading branch information
@vgvassilev
vgvassilev committed Apr 9, 2024
1 parent dbb9749 commit 9391ff8
Show file tree
Hide file tree
Showing 19 changed files with 102 additions and 240 deletions.
14 changes: 1 addition & 13 deletions clang/include/clang/Basic/Attr.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,13 +324,10 @@ class Spelling<string name, string variety, int version = 1> {
}

class GNU<string name> : Spelling<name, "GNU">;
class Declspec<string name> : Spelling<name, "Declspec"> {
bit PrintOnLeft = 1;
}
class Declspec<string name> : Spelling<name, "Declspec">;
class Microsoft<string name> : Spelling<name, "Microsoft">;
class CXX11<string namespace, string name, int version = 1>
: Spelling<name, "CXX11", version> {
bit CanPrintOnLeft = 0;
string Namespace = namespace;
}
class C23<string namespace, string name, int version = 1>
Expand Down Expand Up @@ -596,12 +593,6 @@ class AttrSubjectMatcherAggregateRule<AttrSubject subject> {
def SubjectMatcherForNamed : AttrSubjectMatcherAggregateRule<Named>;

class Attr {
// Specifies that when printed, this attribute is meaningful on the
// 'left side' of the declaration.
bit CanPrintOnLeft = 1;
// Specifies that when printed, this attribute is required to be printed on
// the 'left side' of the declaration.
bit PrintOnLeft = 0;
// The various ways in which an attribute can be spelled in source
list<Spelling> Spellings;
// The things to which an attribute can appertain
Expand Down Expand Up @@ -937,7 +928,6 @@ def AVRSignal : InheritableAttr, TargetSpecificAttr<TargetAVR> {
}

def AsmLabel : InheritableAttr {
let CanPrintOnLeft = 0;
let Spellings = [CustomKeyword<"asm">, CustomKeyword<"__asm__">];
let Args = [
// Label specifies the mangled name for the decl.
Expand Down Expand Up @@ -1534,7 +1524,6 @@ def AllocSize : InheritableAttr {
}

def EnableIf : InheritableAttr {
let CanPrintOnLeft = 0;
// Does not have a [[]] spelling because this attribute requires the ability
// to parse function arguments but the attribute is not written in the type
// position.
Expand Down Expand Up @@ -3171,7 +3160,6 @@ def Unavailable : InheritableAttr {
}

def DiagnoseIf : InheritableAttr {
let CanPrintOnLeft = 0;
// Does not have a [[]] spelling because this attribute requires the ability
// to parse function arguments but the attribute is not written in the type
// position.
Expand Down
10 changes: 0 additions & 10 deletions clang/include/clang/Basic/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,6 @@ clang_tablegen(AttrList.inc -gen-clang-attr-list
SOURCE Attr.td
TARGET ClangAttrList)

clang_tablegen(AttrLeftSideCanPrintList.inc -gen-clang-attr-can-print-left-list
-I ${CMAKE_CURRENT_SOURCE_DIR}/../../
SOURCE Attr.td
TARGET ClangAttrCanPrintLeftList)

clang_tablegen(AttrLeftSideMustPrintList.inc -gen-clang-attr-must-print-left-list
-I ${CMAKE_CURRENT_SOURCE_DIR}/../../
SOURCE Attr.td
TARGET ClangAttrMustPrintLeftList)

clang_tablegen(AttrSubMatchRulesList.inc -gen-clang-attr-subject-match-rule-list
-I ${CMAKE_CURRENT_SOURCE_DIR}/../../
SOURCE Attr.td
Expand Down
179 changes: 47 additions & 132 deletions clang/lib/AST/DeclPrinter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/PrettyPrinter.h"
#include "clang/Basic/Module.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/Support/raw_ostream.h"
using namespace clang;

Expand Down Expand Up @@ -49,18 +50,6 @@ namespace {

void PrintObjCTypeParams(ObjCTypeParamList *Params);

enum class AttrPrintLoc {
None = 0,
Left = 1,
Right = 2,
Any = Left | Right,

LLVM_MARK_AS_BITMASK_ENUM(/*DefaultValue=*/Any)
};

void prettyPrintAttributes(Decl *D, raw_ostream &out,
AttrPrintLoc loc = AttrPrintLoc::Any);

public:
DeclPrinter(raw_ostream &Out, const PrintingPolicy &Policy,
const ASTContext &Context, unsigned Indentation = 0,
Expand Down Expand Up @@ -129,11 +118,10 @@ namespace {
const TemplateParameterList *Params);
void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> Args,
const TemplateParameterList *Params);

inline void prettyPrintAttributes(Decl *D) {
prettyPrintAttributes(D, Out);
}

enum class AttrPosAsWritten { Default = 0, Left, Right };
void
prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos = AttrPosAsWritten::Default);
void prettyPrintPragmas(Decl *D);
void printDeclType(QualType T, StringRef DeclName, bool Pack = false);
};
Expand Down Expand Up @@ -250,87 +238,48 @@ raw_ostream& DeclPrinter::Indent(unsigned Indentation) {
return Out;
}

// For CLANG_ATTR_LIST_CanPrintOnLeft macro.
#include "clang/Basic/AttrLeftSideCanPrintList.inc"
static DeclPrinter::AttrPosAsWritten getPosAsWritten(const Attr *A,
const Decl *D) {
SourceLocation ALoc = A->getLoc();
SourceLocation DLoc = D->getLocation();
const ASTContext &C = D->getASTContext();
if (ALoc.isInvalid() || DLoc.isInvalid())
return DeclPrinter::AttrPosAsWritten::Left;

// For CLANG_ATTR_LIST_PrintOnLeft macro.
#include "clang/Basic/AttrLeftSideMustPrintList.inc"
if (C.getSourceManager().isBeforeInTranslationUnit(ALoc, DLoc))
return DeclPrinter::AttrPosAsWritten::Left;

static bool canPrintOnLeftSide(attr::Kind kind) {
#ifdef CLANG_ATTR_LIST_CanPrintOnLeft
switch (kind) {
CLANG_ATTR_LIST_CanPrintOnLeft
return true;
default:
return false;
}
#else
return false;
#endif
}

static bool canPrintOnLeftSide(const Attr *A) {
if (A->isStandardAttributeSyntax())
return false;

return canPrintOnLeftSide(A->getKind());
return DeclPrinter::AttrPosAsWritten::Right;
}

static bool mustPrintOnLeftSide(attr::Kind kind) {
#ifdef CLANG_ATTR_LIST_PrintOnLeft
switch (kind) {
CLANG_ATTR_LIST_PrintOnLeft
return true;
default:
return false;
}
#else
return false;
#endif
}

static bool mustPrintOnLeftSide(const Attr *A) {
if (A->isDeclspecAttribute())
return true;

return mustPrintOnLeftSide(A->getKind());
}

void DeclPrinter::prettyPrintAttributes(Decl *D, llvm::raw_ostream &Out,
AttrPrintLoc Loc) {
void DeclPrinter::prettyPrintAttributes(const Decl *D,
AttrPosAsWritten Pos /*=Default*/) {
if (Policy.PolishForDeclaration)
return;

if (D->hasAttrs()) {
AttrVec &Attrs = D->getAttrs();
const AttrVec &Attrs = D->getAttrs();
for (auto *A : Attrs) {
if (A->isInherited() || A->isImplicit())
continue;

AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
if (mustPrintOnLeftSide(A)) {
// If we must always print on left side (e.g. declspec), then mark as
// so.
AttrLoc = AttrPrintLoc::Left;
} else if (canPrintOnLeftSide(A)) {
// For functions with body defined we print the attributes on the left
// side so that GCC accept our dumps as well.
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
FD && FD->isThisDeclarationADefinition())
// In case Decl is a function with a body, then attrs should be print
// on the left side.
AttrLoc = AttrPrintLoc::Left;

// In case it is a variable declaration with a ctor, then allow
// printing on the left side for readbility.
else if (const VarDecl *VD = dyn_cast<VarDecl>(D);
VD && VD->getInit() &&
VD->getInitStyle() == VarDecl::CallInit)
AttrLoc = AttrPrintLoc::Left;
switch (A->getKind()) {
#define ATTR(X)
#define PRAGMA_SPELLING_ATTR(X) case attr::X:
#include "clang/Basic/AttrList.inc"
break;
default:
AttrPosAsWritten APos = getPosAsWritten(A, D);
assert(APos != AttrPosAsWritten::Default &&
"Default not a valid for an attribute location");
if (Pos == AttrPosAsWritten::Default || Pos == APos) {
if (Pos != AttrPosAsWritten::Left)
Out << ' ';
A->printPretty(Out, Policy);
if (Pos == AttrPosAsWritten::Left)
Out << ' ';
}
break;
}
// Only print the side matches the user requested.
if ((Loc & AttrLoc) != AttrPrintLoc::None)
A->printPretty(Out, Policy);
}
}
}
Expand Down Expand Up @@ -691,8 +640,10 @@ static void MaybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,

void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
if (!D->getDescribedFunctionTemplate() &&
!D->isFunctionTemplateSpecialization())
!D->isFunctionTemplateSpecialization()) {
prettyPrintPragmas(D);
prettyPrintAttributes(D, AttrPosAsWritten::Left);
}

if (D->isFunctionTemplateSpecialization())
Out << "template<> ";
Expand All @@ -702,22 +653,6 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
printTemplateParameters(D->getTemplateParameterList(I));
}

std::string LeftsideAttrs;
llvm::raw_string_ostream LSAS(LeftsideAttrs);

prettyPrintAttributes(D, LSAS, AttrPrintLoc::Left);

// prettyPrintAttributes print a space on left side of the attribute.
if (LeftsideAttrs[0] == ' ') {
// Skip the space prettyPrintAttributes generated.
LeftsideAttrs.erase(0, LeftsideAttrs.find_first_not_of(' '));

// Add a single space between the attribute and the Decl name.
LSAS << ' ';
}

Out << LeftsideAttrs;

CXXConstructorDecl *CDecl = dyn_cast<CXXConstructorDecl>(D);
CXXConversionDecl *ConversionDecl = dyn_cast<CXXConversionDecl>(D);
CXXDeductionGuideDecl *GuideDecl = dyn_cast<CXXDeductionGuideDecl>(D);
Expand Down Expand Up @@ -883,7 +818,7 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
Ty.print(Out, Policy, Proto);
}

prettyPrintAttributes(D, Out, AttrPrintLoc::Right);
prettyPrintAttributes(D, AttrPosAsWritten::Right);

if (D->isPureVirtual())
Out << " = 0";
Expand Down Expand Up @@ -976,27 +911,12 @@ void DeclPrinter::VisitLabelDecl(LabelDecl *D) {
void DeclPrinter::VisitVarDecl(VarDecl *D) {
prettyPrintPragmas(D);

prettyPrintAttributes(D, AttrPosAsWritten::Left);

if (const auto *Param = dyn_cast<ParmVarDecl>(D);
Param && Param->isExplicitObjectParameter())
Out << "this ";

std::string LeftSide;
llvm::raw_string_ostream LeftSideStream(LeftSide);

// Print attributes that should be placed on the left, such as __declspec.
prettyPrintAttributes(D, LeftSideStream, AttrPrintLoc::Left);

// prettyPrintAttributes print a space on left side of the attribute.
if (LeftSide[0] == ' ') {
// Skip the space prettyPrintAttributes generated.
LeftSide.erase(0, LeftSide.find_first_not_of(' '));

// Add a single space between the attribute and the Decl name.
LeftSideStream << ' ';
}

Out << LeftSide;

QualType T = D->getTypeSourceInfo()
? D->getTypeSourceInfo()->getType()
: D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
Expand Down Expand Up @@ -1029,21 +949,16 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
}
}

StringRef Name;

Name = (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
D->getIdentifier())
? D->getIdentifier()->deuglifiedName()
: D->getName();

if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
!Policy.SuppressUnwrittenScope)
MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);
printDeclType(T, Name);

// Print the attributes that should be placed right before the end of the
// decl.
prettyPrintAttributes(D, Out, AttrPrintLoc::Right);
printDeclType(T, (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
D->getIdentifier())
? D->getIdentifier()->deuglifiedName()
: D->getName());

prettyPrintAttributes(D, AttrPosAsWritten::Right);

Expr *Init = D->getInit();
if (!Policy.SuppressInitializers && Init) {
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/AST/StmtPrinter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,11 @@ void StmtPrinter::VisitLabelStmt(LabelStmt *Node) {
}

void StmtPrinter::VisitAttributedStmt(AttributedStmt *Node) {
for (const auto *Attr : Node->getAttrs()) {
llvm::ArrayRef<const Attr *> Attrs = Node->getAttrs();
for (const auto *Attr : Attrs) {
Attr->printPretty(OS, Policy);
if (Attr != Attrs.back())
OS << ' ';
}

PrintStmt(Node->getSubStmt(), 0);
Expand Down
16 changes: 8 additions & 8 deletions clang/test/APINotes/retain-count-convention.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@

#import <SimpleKit/SimpleKit.h>

// CHECK: void *getCFOwnedToUnowned(void) __attribute__((cf_returns_not_retained));
// CHECK: void *getCFUnownedToOwned(void) __attribute__((cf_returns_retained));
// CHECK: void *getCFOwnedToNone(void) __attribute__((cf_unknown_transfer));
// CHECK: id getObjCOwnedToUnowned(void) __attribute__((ns_returns_not_retained));
// CHECK: id getObjCUnownedToOwned(void) __attribute__((ns_returns_retained));
// CHECK: int indirectGetCFOwnedToUnowned(void * _Nullable *out __attribute__((cf_returns_not_retained)));
// CHECK: int indirectGetCFUnownedToOwned(void * _Nullable *out __attribute__((cf_returns_retained)));
// CHECK: __attribute__((cf_returns_not_retained)) void *getCFOwnedToUnowned(void);
// CHECK: __attribute__((cf_returns_retained)) void *getCFUnownedToOwned(void);
// CHECK: __attribute__((cf_unknown_transfer)) void *getCFOwnedToNone(void);
// CHECK: __attribute__((ns_returns_not_retained)) id getObjCOwnedToUnowned(void);
// CHECK: __attribute__((ns_returns_retained)) id getObjCUnownedToOwned(void);
// CHECK: int indirectGetCFOwnedToUnowned(__attribute__((cf_returns_not_retained)) void * _Nullable *out);
// CHECK: int indirectGetCFUnownedToOwned(__attribute__((cf_returns_retained)) void * _Nullable *out);
// CHECK: int indirectGetCFOwnedToNone(void * _Nullable *out);
// CHECK: int indirectGetCFNoneToOwned(void **out __attribute__((cf_returns_not_retained)));
// CHECK: int indirectGetCFNoneToOwned(__attribute__((cf_returns_not_retained)) void **out);

// CHECK-LABEL: @interface MethodTest
// CHECK: - (id)getOwnedToUnowned __attribute__((ns_returns_not_retained));
Expand Down
4 changes: 2 additions & 2 deletions clang/test/APINotes/versioned.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#import <VersionedKit/VersionedKit.h>

// CHECK-UNVERSIONED: void moveToPointDUMP(double x, double y) __attribute__((swift_name("moveTo(x:y:)")));
// CHECK-VERSIONED: void moveToPointDUMP(double x, double y) __attribute__((swift_name("moveTo(a:b:)")));
// CHECK-VERSIONED:__attribute__((swift_name("moveTo(a:b:)"))) void moveToPointDUMP(double x, double y);

// CHECK-DUMP-LABEL: Dumping moveToPointDUMP
// CHECK-VERSIONED-DUMP: SwiftVersionedAdditionAttr {{.+}} Implicit 3.0 IsReplacedByActive{{$}}
Expand Down Expand Up @@ -65,7 +65,7 @@

// CHECK-DUMP-NOT: Dumping

// CHECK-UNVERSIONED: void acceptClosure(void (^block)(void) __attribute__((noescape)));
// CHECK-UNVERSIONED: void acceptClosure(__attribute__((noescape)) void (^block)(void));
// CHECK-VERSIONED: void acceptClosure(void (^block)(void));

// CHECK-UNVERSIONED: void privateFunc(void) __attribute__((swift_private));
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ast-print-method-decl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ struct DefMethodsWithoutBody {
// CHECK-NEXT: DefMethodsWithoutBody() = default;
~DefMethodsWithoutBody() = default;

// CHECK-NEXT: __attribute__((alias("X"))) void m1();
// CHECK-NEXT: void m1() __attribute__((alias("X")));
void m1() __attribute__((alias("X")));

// CHECK-NEXT: };
Expand Down

0 comments on commit 9391ff8

Please sign in to comment.