Skip to content

Commit 48b6d9f

Browse files
john-h-kastnerjohn-h-kastner
authored
Fix function rewriting with r-paren between prototype and definition (#394)
A right parentheses appearing between a function prototype and the beginning of its definition caused anything falling between the parenthesis and the actual end of the prototype to be deleted. This problem is fixed by using clang provided source locations instead of locating the closing parenthesis by iterating of the source code characters. Fixes #392.
1 parent ad9b01e commit 48b6d9f

File tree

5 files changed

+201
-11
lines changed

5 files changed

+201
-11
lines changed

‎clang/include/clang/3C/RewriteUtils.h

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ class FunctionDeclReplacement
9797
}
9898
if (!TSInfo || TypeLoc.isNull())
9999
return SourceRange(Decl->getBeginLoc(),
100-
getFunctionDeclarationEnd(Decl, SM));
100+
getFunctionDeclRParen(Decl, SM));
101101

102102
// Function pointer are funky, and require special handling to rewrite the
103103
// return type.
@@ -118,11 +118,38 @@ class FunctionDeclReplacement
118118

119119
// If rewriting Parameters, stop at the right parenthesis of the parameters.
120120
// Otherwise, stop after the return type.
121-
// Note: getFunctionDeclarationEnd is used instead of getRParenLoc so that
122-
// itypes are deleted correctly when --remove-itypes is used.
123-
SourceLocation End = RewriteParams
124-
? getFunctionDeclarationEnd(Decl, SM)
125-
: Decl->getReturnTypeSourceRange().getEnd();
121+
SourceLocation End;
122+
if (RewriteParams) {
123+
// When there are no bounds or itypes on a function, the declaration ends
124+
// at the right paren of the declaration parameter list.
125+
End = TypeLoc.getRParenLoc();
126+
127+
// If there's a bounds expression, this comes after the right paren of the
128+
// function declaration parameter list.
129+
if (auto *BoundsE = Decl->getBoundsExpr()) {
130+
SourceLocation BoundsEnd = BoundsE->getEndLoc();
131+
if (BoundsEnd.isValid())
132+
End = BoundsEnd;
133+
}
134+
135+
// If there's an itype, this also comes after the right paren. In the case
136+
// that there is both a bounds expression and an itype, we need check
137+
// which is later in the file and use that as the declaration end.
138+
if (auto *InteropE = Decl->getInteropTypeExpr()) {
139+
SourceLocation InteropEnd = InteropE->getEndLoc();
140+
if (InteropEnd.isValid() &&
141+
(!End.isValid() || SM.isBeforeInTranslationUnit(End, InteropEnd)))
142+
End = InteropEnd;
143+
}
144+
145+
// SourceLocations are weird and turn up invalid for reasons I don't
146+
// understand. Fallback to extracting r paren location from source
147+
// character buffer.
148+
if (!End.isValid())
149+
End = getFunctionDeclRParen(Decl, SM);
150+
} else {
151+
End = Decl->getReturnTypeSourceRange().getEnd();
152+
}
126153

127154
assert("Invalid FunctionDeclReplacement SourceRange!" && Begin.isValid() &&
128155
End.isValid());

‎clang/include/clang/3C/Utils.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ bool isStructOrUnionType(clang::VarDecl *VD);
133133
std::string tyToStr(const clang::Type *T);
134134

135135
// Get the end source location of the end of the provided function.
136-
clang::SourceLocation getFunctionDeclarationEnd(clang::FunctionDecl *FD,
137-
clang::SourceManager &S);
136+
clang::SourceLocation getFunctionDeclRParen(clang::FunctionDecl *FD,
137+
clang::SourceManager &S);
138138

139139
// Remove auxillary casts from the provided expression.
140140
clang::Expr *removeAuxillaryCasts(clang::Expr *SrcExpr);

‎clang/lib/3C/DeclRewriter.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -713,8 +713,8 @@ FunctionDeclBuilder::buildDeclVar(PVConstraint *IntCV, PVConstraint *ExtCV,
713713
IType = "";
714714
} else {
715715
Type = ExtCV->getOriginalTy() + " ";
716-
IType =
717-
getExistingIType(ExtCV) + ABRewriter.getBoundsString(ExtCV, Decl, false);
716+
IType = getExistingIType(ExtCV);
717+
IType += ABRewriter.getBoundsString(ExtCV, Decl, !IType.empty());
718718
}
719719
}
720720

‎clang/lib/3C/Utils.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,16 @@ FunctionDecl *getDefinition(FunctionDecl *FD) {
5656
return nullptr;
5757
}
5858

59-
SourceLocation getFunctionDeclarationEnd(FunctionDecl *FD, SourceManager &S) {
59+
// Get the source location for the right paren of a function declaration
60+
// using the source character data buffer. Because this uses the character
61+
// buffer directly, it sees character data prior to preprocessing. This
62+
// means characters that are in comments, macros or otherwise not part of the
63+
// final preprocessed source code are seen and can cause this function to give
64+
// an incorrect result. This should only be used as a fall back for when the
65+
// clang library function FunctionTypeLoc::getRParenLoc cannot be called due to
66+
// a null FunctionTypeLoc or for when the function returns an invalid source
67+
// location.
68+
SourceLocation getFunctionDeclRParen(FunctionDecl *FD, SourceManager &S) {
6069
const FunctionDecl *OFd = nullptr;
6170

6271
if (FD->hasBody(OFd) && OFd == FD) {

‎clang/test/3C/functionDeclEnd.c

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
// RUN: 3c -addcr -alltypes %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
2+
// RUN: 3c -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s
3+
// RUN: 3c -addcr -alltypes %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
4+
// RUN: 3c -alltypes -output-postfix=checked %s
5+
// RUN: 3c -alltypes %S/functionDeclEnd.checked.c | count 0
6+
// RUN: rm %S/functionDeclEnd.checked.c
7+
8+
9+
// Tests for issue 392. When rewriting function prototypes sometimes code
10+
// falling between the start of the definition and the end of the prototype
11+
// could be deleted.
12+
// NOTE: Tests in this file assume that code in the branch of the preprocessor
13+
// directive that is not taken will not be rewritten. It might be desirable to
14+
// eventually rewrite in both branches. See issue 374.
15+
16+
#define FOO
17+
18+
#ifdef FOO
19+
void test0(int *a)
20+
// CHECK: void test0(_Ptr<int> a)
21+
#else
22+
void test0(int *a)
23+
#endif
24+
// CHECK: #else
25+
// CHECK: void test0(int *a)
26+
// CHECK: #endif
27+
{
28+
// CHECK: _Checked {
29+
return;
30+
}
31+
32+
#ifdef FOO
33+
int *test1()
34+
// CHECK: _Ptr<int> test1(void)
35+
#else
36+
int *test1()
37+
#endif
38+
// CHECK: #else
39+
// CHECK: int *test1()
40+
// CHECK: #endif
41+
{
42+
// CHECK: _Checked {
43+
return 0;
44+
}
45+
46+
#ifdef FOO
47+
int *test2()
48+
// CHECK: int *test2(void) : itype(_Ptr<int>)
49+
#else
50+
int *test2()
51+
#endif
52+
// CHECK: #else
53+
// CHECK: int *test2()
54+
// CHECK: #endif
55+
{
56+
// CHECK: {
57+
int *a = 1;
58+
return a;
59+
}
60+
61+
62+
// These test for rewriting with existing itype and bounds expression are
63+
// particularly important because they break the simplest fix where
64+
// getRParenLoc is always used.
65+
66+
#ifdef FOO
67+
int *test3(int *a, int l) : itype(_Array_ptr<int>) count(l)
68+
// CHECK: int *test3(_Ptr<int> a, int l) : itype(_Array_ptr<int>) count(l)
69+
#else
70+
int *test3(int *a, int l) : itype(_Array_ptr<int>) count(l)
71+
#endif
72+
// CHECK: #else
73+
// CHECK: int *test3(int *a, int l) : itype(_Array_ptr<int>) count(l)
74+
// CHECK: #endif
75+
{
76+
// CHECK: {
77+
int *b = 1;
78+
return b;
79+
}
80+
81+
#ifdef FOO
82+
int *test4(int *a) : itype(_Ptr<int>)
83+
// CHECK: int *test4(_Ptr<int> a) : itype(_Ptr<int>)
84+
#else
85+
int *test4(int *a) : itype(_Ptr<int>)
86+
#endif
87+
// CHECK: #else
88+
// CHECK: int *test4(int *a) : itype(_Ptr<int>)
89+
// CHECK: #endif
90+
{
91+
// CHECK: {
92+
int *b = 1;
93+
return b;
94+
}
95+
96+
#ifdef FOO
97+
_Array_ptr<int> test5(int *a, int l) : count(l)
98+
// CHECK: _Array_ptr<int> test5(_Ptr<int> a, int l) : count(l)
99+
#else
100+
_Array_ptr<int> test5(int *a, int l) : count(l)
101+
#endif
102+
// CHECK: #else
103+
// CHECK: _Array_ptr<int> test5(int *a, int l) : count(l)
104+
// CHECK: #endif
105+
{
106+
// CHECK: _Checked {
107+
return 0;
108+
}
109+
110+
void test6(int *a)
111+
// A comment ( with parentheses ) that shouldn't be deleted
112+
// CHECK: void test6(_Ptr<int> a)
113+
// CHECK: // A comment ( with parentheses ) that shouldn't be deleted
114+
{
115+
*a = 1;
116+
}
117+
118+
#ifdef FOO
119+
int *test7(int *a) : count(10)
120+
//CHECK_NOALL: int *test7(int *a : itype(_Ptr<int>)) : count(10)
121+
//CHECK_ALL: int *test7(_Array_ptr<int> a) : count(10)
122+
#else
123+
int *test7(int *a) : count(10)
124+
#endif
125+
;
126+
//CHECK: #else
127+
//CHECK: int *test7(int *a) : count(10)
128+
//CHECK: #endif
129+
//CHECK: ;
130+
131+
int *test7(int *a) : count(10) {
132+
//CHECK_ALL: int *test7(_Array_ptr<int> a) : count(10) _Checked {
133+
//CHECK_NOALL: int *test7(int *a : itype(_Ptr<int>)) : count(10) {
134+
return a;
135+
}
136+
137+
// This test cast checks that the itype is overwritten even when it appears
138+
// after the count. Unfortunately, the count and itype are reversed on
139+
// rewriting. This isn't great since it's an unnecessary change to the code,
140+
// but it is still valid.
141+
#ifdef FOO
142+
int *test8(int *a, int l) : count(l) itype(_Array_ptr<int>)
143+
// CHECK: int *test8(_Ptr<int> a, int l) : itype(_Array_ptr<int>) count(l)
144+
#else
145+
int *test8(int *a, int l) : count(l) itype(_Array_ptr<int>)
146+
#endif
147+
// CHECK: #else
148+
// CHECK: int *test8(int *a, int l) : count(l) itype(_Array_ptr<int>)
149+
// CHECK: #endif
150+
{
151+
// CHECK: {
152+
int *b = 1;
153+
return b;
154+
}

0 commit comments

Comments
 (0)