-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang][sema] Add nonnull attribute to builtin format functions #158626
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.
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
Changes from 1 commit
d61c6c7
61a4853
08ed7a3
91720ce
00c1357
e9ea00e
05e5dd1
d5e83cf
a10c9bd
e91468c
017b747
57369b9
16edd1e
4afad52
c3d823b
966f783
e93fc0e
f82de74
80f5cd1
5562e1b
8db64dc
709670f
627a143
3f8dbff
302f6eb
65e4f65
3f4ada4
a117067
6a2e170
0524613
e7c4900
877d2c7
1e24046
f0ad34c
5520a39
a758ab5
d6d2fc1
933e3c0
ea7220e
677d7b7
d35163c
bfad1ff
e36867f
3688b4c
e7e73ea
b19f48d
ece3772
041e95e
ea0314f
ceb5b96
219a9e1
bcfa088
b273900
554b8eb
68ddca9
4fe7831
ff3f2c4
f3eb75c
262fe7c
bc17738
5d2e25b
a64bc67
27bead9
2af9c02
b30a978
685d9ae
835c4a3
4290177
2f30802
aa2ae7f
5952c8b
90034da
2601491
d379289
9cf9194
cee38f5
59af088
17f4507
2c28313
49159cb
f6a00bd
01b778d
d7c9d6d
89194fd
2c1dbcf
aa4ce63
59263e1
e9c7b84
d1808f9
74d888c
8cd84a1
3eadbcd
22f2e6f
3ec22c1
d0c28e9
0f55c2b
83ac489
bb80f4e
d9e0f8e
0c46e1c
28e8f17
73f0118
ff90e4a
ae74d3c
97b5583
022ab46
44b8ce8
5d88cad
4c45ce9
f6ccdf4
912e48c
95e9a57
d080d89
14b51cc
0a82489
a1da620
e99ebfe
4879986
3558eca
668bb1e
bd32533
965bf6f
ef9e43a
1228e8c
87c86cc
0dc7fc3
a6942c5
4ceb3ef
95ec0c8
70b728c
54208cb
d6ae47b
11ac6f4
03e5e9d
02f24de
0e85a8e
54f720d
10d2535
88add45
d2b9c8c
ae0394e
dcc1118
aa57e5e
b61e39c
dfbccc7
d8dfc56
a497a30
51048fd
85cb7d7
6fc8943
fc42385
49b8a46
1f60d73
014a967
90d3c7c
7743c9c
431c849
d0ae7cd
a205298
02300ee
c10b0e2
1c1cb1a
71f0f14
15efcf2
2e2f7a3
0cc32d1
e7ece97
5e26e52
eca5529
0f35995
b0fc07e
add15a5
97ba293
f012e58
fe1cd8e
450f102
f537a0c
54c8c76
4afd2b1
97d5698
29e373a
356ccaf
9189e9e
f81c494
c0a8c79
99a4723
b592af4
3b9c31e
bede9bd
26fd7e1
fee4af5
5422d29
5b27e46
75a7e53
a71e16f
25c7909
1815787
df1d4f6
f670e56
25f1604
18d1feb
89f90e2
cb08dde
c30a394
089fc91
d961d6e
7ef86b8
2db15a9
6be5d43
dbe8b3c
c2dd800
0f7ac6b
5de73b0
b512d06
9ce52bc
f3edf41
1246b0a
80bcf26
6bac20d
945d9d3
d8d6e31
289373c
82d22b1
d214d98
9f1e2f8
0d6f461
bd2411c
663d208
f8bab4c
b75a5f2
fae34cd
433588e
aa36932
e857fe6
25c8bdb
7835665
08e465c
e3d514b
6c88292
f95ff06
6fef046
4726073
508c4be
0f8c577
3b1872e
c75d2a7
33d9ab2
0763b1c
e04ef6b
f3564d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -392,6 +392,10 @@ class Context { | |||||||||||||
bool performsCallback(unsigned ID, | ||||||||||||||
llvm::SmallVectorImpl<int> &Encoding) const; | ||||||||||||||
|
||||||||||||||
/// Return true if this builtin has parameters at fixed positions | ||||||||||||||
/// that must be non-null. | ||||||||||||||
bool IsNonNull(unsigned ID, llvm::SmallVectorImpl<int> &Indxs) const; | ||||||||||||||
|
/// Return true if this builtin has parameters at fixed positions | |
/// that must be non-null. | |
bool IsNonNull(unsigned ID, llvm::SmallVectorImpl<int> &Indxs) const; | |
/// Return true if this builtin has parameters that must be non-null. | |
/// The parameter indices are appended into 'Indxs'. | |
bool isNonNull(unsigned ID, SmallVectorImpl<int> &Indxs) const; |
The other functions here seem to use camelCase, so this one should too for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should the SmallVector
take unsigned
s instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should!
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,6 +293,34 @@ bool Builtin::Context::isScanfLike(unsigned ID, unsigned &FormatIdx, | |
return isLike(ID, FormatIdx, HasVAListArg, "sS"); | ||
} | ||
|
||
bool Builtin::Context::IsNonNull(unsigned ID, | ||
llvm::SmallVectorImpl<int> &Indxs) const { | ||
|
||
const char *CalleePos = ::strchr(getAttributesString(ID), 'N'); | ||
if (!CalleePos) | ||
return false; | ||
|
||
++CalleePos; | ||
assert(*CalleePos == '<' && | ||
"Callback callee specifier must be followed by a '<'"); | ||
++CalleePos; | ||
|
||
char *EndPos; | ||
int CalleeIdx = ::strtol(CalleePos, &EndPos, 10); | ||
assert(CalleeIdx >= 0 && "Callee index is supposed to be positive!"); | ||
Indxs.push_back(CalleeIdx); | ||
|
||
while (*EndPos == ',') { | ||
const char *PayloadPos = EndPos + 1; | ||
|
||
int PayloadIdx = ::strtol(PayloadPos, &EndPos, 10); | ||
Indxs.push_back(PayloadIdx); | ||
} | ||
|
||
assert(*EndPos == '>' && "Callback callee specifier must end with a '>'"); | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is copy-pasted from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable — I’ll refactor the parsing logic into a small helper function (so both places can share it) and update the local variable names to better reflect their meaning in this context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the parsing into a static helper
We keep them separate because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose if preexisting function already use |
||
} | ||
|
||
bool Builtin::Context::performsCallback(unsigned ID, | ||
SmallVectorImpl<int> &Encoding) const { | ||
const char *CalleePos = ::strchr(getAttributesString(ID), 'C'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17097,6 +17097,15 @@ void Sema::AddKnownFunctionAttributes(FunctionDecl *FD) { | |
} | ||
} | ||
|
||
SmallVector<int, 4> Indxs; | ||
if (Context.BuiltinInfo.IsNonNull(BuiltinID, Indxs) && | ||
!FD->hasAttr<NonNullAttr>()) { | ||
llvm::SmallVector<ParamIdx, 4> ParamIndxs; | ||
for (int I : Indxs) | ||
ParamIndxs.push_back(ParamIdx(I + 1, FD)); | ||
FD->addAttr(NonNullAttr::CreateImplicit(Context, ParamIndxs.data(), | ||
ParamIndxs.size())); | ||
Comment on lines
17144
to
17151
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it’d make more sense to create the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I forgot it requires a decl, in that case that’s fine. |
||
} | ||
if (Context.BuiltinInfo.isReturnsTwice(BuiltinID) && | ||
!FD->hasAttr<ReturnsTwiceAttr>()) | ||
FD->addAttr(ReturnsTwiceAttr::CreateImplicit(Context, | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,74 @@ | ||||||||
// RUN: %clang_cc1 -fsyntax-only -verify -Wnonnull -Wno-format-security %s | ||||||||
|
||||||||
#include <stdarg.h> | ||||||||
#include <stddef.h> | ||||||||
|
#include <stdarg.h> | |
#include <stddef.h> | |
typedef __SIZE_TYPE__ size_t; |
Clang tests should generally never include stdlib headers. If you need definitions from those headers, define them yourself (for size_t
in particular, you can do typedef __SIZE_TYPE__ size_t
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads-up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should just work, but can you test C23 nullptr
and GNU __null
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since __null
is a GCC C++ extension, should I add a small .cpp
test file just for that, or include the printf
/scanf
tests in it as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just adding some C++ tests for __null
would be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should document that this option takes arguments, similarly to
C
below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same 'N' as 86? if not, we should be documenting them separately/make it clear that they're not (ie, that the 'N' below is a placeholder). Additionally, this probably needs to be together if so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No in this case it’s a literal
N
, and yeah, that’s a bit unfortunate; it’d probably be clearer if we quoted everything that’s supposed to be a literal character as opposed to a variable/placeholder, but maybe that should be a separate NFC patch?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK if it is a separate NFC patch, but I think we need to do it BEFORE using
N
to mean something, else said NFC patch gets pretty annoying (and confusing for a while if that doesn't land). I'm open to whatever, but I'd like to see that patch merged before this one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that’s a good point; I don’t feel too strongly about this, so I suppose, @bozicrHT, feel free to make a separate patch for that or to integrate that into this one, whichever is easier for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and created a separate NFC PR #160080 for quoting the literal attribute letters.