These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
The CI on my PR https://github.com/llvm/llvm-project/pull/138895 is
failing with errors like
`clang/lib/Analysis/UnsafeBufferUsage.cpp:971:45: error: reference to
'PointerType' is ambiguous`
This patch should resolve it by removing `using namespace llvm`
The -Wunsafe-buffer-usage analysis currently warns when a const sized
array is safely accessed, with an index that is bound by the remainder
operator. The false alarm is now suppressed.
Example:
int circular_buffer(int array[10], uint idx) {
return array[idx %10]; // Safe operation
}
rdar://148443453
---------
Co-authored-by: MalavikaSamak <malavika2@apple.com>
This changes exposes a low-level helper that is used to implement
`forEachArgumentWithParamType` but can also be used without matchers,
e.g. if performance is a concern.
Commit f5ee10538b introduced a copy of the
implementation of the `forEachArgumentWithParamType` matcher that was
needed for optimizing performance of `-Wunsafe-buffer-usage`.
This change shares the code between the two so that we do not repeat
ourselves and any bugfixes or changes will be picked up by both
implementations in the future.
DenseSet, SmallPtrSet, SmallSet, SetVector, and StringSet recently
gained C++23-style insert_range. This patch replaces:
Dest.insert(Src.begin(), Src.end());
with:
Dest.insert_range(Src);
This patch does not touch custom begin like succ_begin for now.
The Clang disgnostic `-Wunsafe-buffer-usage` was adding up to +15%
compilation time when used. Profiling showed that most of the overhead
comes from the use of ASTMatchers.
This change replaces the ASTMatcher infrastructure with simple matching
functions and keeps the functionality unchanged. It reduces the overhead
added by `-Wunsafe-buffer-usage` by 87.8%, leaving a negligible
additional compilation time of 1.7% when the diagnostic is used.
**Old version without -Wunsafe-buffer-usage:**
```
$ hyperfine -i -w 1 --runs 5 '/tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20'
Benchmark 1: /tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20
Time (mean ± σ): 231.035 s ± 3.210 s [User: 229.134 s, System: 1.704 s]
Range (min … max): 228.751 s … 236.682 s 5 runs
```
**Old version with -Wunsafe-buffer-usage:**
```
$ hyperfine -i -w 1 --runs 10 '/tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage'
Benchmark 1: /tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage
Time (mean ± σ): 263.840 s ± 0.854 s [User: 262.043 s, System: 1.575 s]
Range (min … max): 262.442 s … 265.142 s 10 runs
```
**New version with -Wunsafe-buffer-usage:**
```
$ hyperfine -i -w 1 --runs 10 '/tmp/new_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage'
Benchmark 1: /tmp/new_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage
Time (mean ± σ): 235.169 s ± 1.408 s [User: 233.406 s, System: 1.561 s]
Range (min … max): 232.221 s … 236.792 s 10 runs
```
We can take advantage of the attribute `alloc_size`. For example,
```
void * malloc(size_t size) __attribute__((alloc_size(1)));
std::span<char>{(char *)malloc(x), x}; // this is safe
```
rdar://136634730
`MeasureTokenLength` may return an unsigned 0 representing failure in
obtaining length of a token. The analysis now gives up on such cases.
Otherwise, there might be issues caused by unsigned integer "overflow".
Do not warn when a constant sized array is indexed with an expression
that contains bitwise and operation
involving constants and it always results in a bound safe access.
(rdar://136684050)
---------
Co-authored-by: MalavikaSamak <malavika2@apple.com>
EvaluateAsConstExpr() can return an lvalue which is not compatible
with a subsequent getInt() call. Instead, use EvaluateAsInt() which
will use all techniques availble to get an int result compatible
with the subsequent getInt() call.
Do not warn when constant sized array is indexed by expressions that
evaluate to a const value. For instance, sizeof(T) expression value can
be evaluated at compile time and if an array is indexed by such an
expression, it's bounds can be validated.
(rdar://140320289)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
Do not warn about unsafe buffer access, when multi-dimensional constant
arrays are accessed and their indices are within the bounds of the
buffer. Warning in such cases would be a false positive. Such a
suppression already exists for 1-d
arrays and it is now extended to multi-dimensional arrays.
(rdar://137926311)
(rdar://140320139)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
Do not warn when two parameter constructor receives pointer address from
a std::addressof method and the span size is set to 1.
(rdar://139298119)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
Do not warn when a string literal is indexed and the idex value is
within the bounds of the length of the string.
(rdar://139106996)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
CXXCtorInitializers are not statements , but they point to an
initializer expression which is. When visiting a FunctionDecl, also
walk through any constructor initializers and run the warning
checks/matchers against their initializer expressions. This catches
warnings for initializing fields and calling other constructors, such
as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
Field initializers can be found by traversing CXXDefaultInitExprs. This
catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization is not fully covered where a field
specifies an initializer and it's not overridden in the aggregate initialization,
such as in:
struct AggregateViaValueInit {
UnsafeMembers f1;
// FIXME: A construction of this class does initialize the field
// through this initializer, so it should warn. Ideally it should
// also point to where the site of the construction is in
// testAggregateViaValueInit().
UnsafeMembers f2{3};
};
void testAggregateViaValueInit() {
auto A = AggregateViaValueInit();
};
There are 3 tests for different types of aggregate initialization with
FIXMEs documenting this future work.
One attempt to fix this involved returning true from
MatchDescendantVisitor::shouldVisitImplicitCode(), however, it breaks expectations
for field in-class initializers by moving the SourceLocation, possibly
to inside the implicit ctor instead of on the line where the field
initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
Issue #80482
Emit a warning when the raw pointer retrieved from std::vector and
std::array instances are cast to a larger type. Such a cast followed by
a field dereference to the resulting pointer could cause an OOB access.
This is similar to the existing span::data warning.
(rdar://136704278)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
For `snprintf(a, sizeof a, ...)`, the first two arguments form a safe
pattern if `a` is a constant array. In such a case, this commit will
suppress the warning.
(rdar://117182250)
The commit d7dd2c468f crashes for such
an example:
```
void printf() { printf(); }
```
Because it assumes `printf` must have arguments. This commit fixes
this issue.
(rdar://117182250)
[-Wunsafe-buffer-usage] Add warn on unsafe calls to libc functions
Warning about calls to libc functions involving buffer access. Warned
functions are hardcoded by names.
(rdar://117182250)
`QualType::isConstantArrayType()` checks canonical type. So a following
cast should be applied to canonical type as well:
```
if (Ty->isConstantArrayType())
cast<ConstantArrayType>(Ty.getCanonicalType()); // cast<ConstantArrayType>(Ty) is incorrect
```
Extend the unsafe_buffer_usage attribute, so they can also be added to
struct fields. This will cause the compiler to warn about the unsafe
field at their access sites.
Co-authored-by: MalavikaSamak <malavika2@apple.com>
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling a
CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size), but
that is no longer the case. We want the Reporter to generate different
warnings (in the -Wunsafe-buffer-usage-in-container subgroup) for the
span contructor. And we will want it to report more warnings for other
std-container-specific gadgets in the future. To handle this we allow
the gadget to control if the warning is general (it calls
handleUnsafeBufferUsage()) or is a std-container-specific warning (it
calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
Issue #80482
UPCAddressofArraySubscriptGadget::getClaimedVarUseSites should skip
parentheses when accessing the DeclRefExpr, otherwise a crash happens
with parenthesized references.
In PR #79382, I need to add a new type that derives from
ConstantArrayType. This means that ConstantArrayType can no longer use
`llvm::TrailingObjects` to store the trailing optional Expr*.
This change refactors ConstantArrayType to store a 60-bit integer and
4-bits for the integer size in bytes. This replaces the APInt field
previously in the type but preserves enough information to recreate it
where needed.
To reduce the number of places where the APInt is re-constructed I've
also added some helper methods to the ConstantArrayType to allow some
common use cases that operate on either the stored small integer or the
APInt as appropriate.
Resolves#85124.
Without the fix gcc warned like
../../clang/lib/Analysis/UnsafeBufferUsage.cpp:2203:26: warning: unused variable 'CArrTy' [-Wunused-variable]
2203 | } else if (const auto *CArrTy = Ctx.getAsConstantArrayType(
| ^~~~~~
Example:
int * const my_var = my_initializer;
Currently when transforming my_var to std::span the fixits:
- replace "int * const my_var = " with "std::span<int> const my_var {"
- add ", SIZE}" after "my_initializer" where SIZE is either inferred or
a placeholder
This patch makes that behavior less intrusive by not modifying variable
cv-qualifiers and initialization syntax.
The new behavior is:
- replace "int *" with "std::span<int>"
- add "{" before "my_initializer"
- add ", SIZE}" after "my_initializer"
This is an improvement on its own - since we don't touch the identifier,
we automatically can handle macros in them.
It also simplifies future work on initializer fixits.
Example:
int arr[10];
int * ptr = arr;
If ptr is unsafe and we transform it to std::span then the fixit we'd
currently provide transforms the code to:
std::span<int> ptr{arr, 10};
That's suboptimal as that repeats the size of the array in the code.
The idiomatic transformation should rely on the span constructor
that takes just the array argument and relies on template
parameter autodeduction to set the span size.
The transformed code should look like:
std::span<int> ptr = arr;
Note that it just should not change the initializer at all and that also
works for other forms of initialization like:
int * ptr {arr};
becoming:
std::span<int> ptr{arr};
This patch changes the initializer handling to the desired (empty)
fixit.
Introducing CArrayToPtrAssignment gadget and implementing fixits for some cases
of array being assigned to pointer.
Key observations:
- const size array can be assigned to std::span and bounds are propagated
- const size array can't be on LHS of assignment
This means array to pointer assignment has no strategy implications.
Fixits are implemented for cases where one of the variables in the assignment is
safe. For assignment of a safe array to unsafe pointer we know that the RHS will
never be transformed since it's safe and can immediately emit the optimal fixit.
Similarly for assignment of unsafe array to safe pointer.
(Obviously this is not and can't be future-proof in regards to what
variables we consider unsafe and that is fine.)
Fixits for assignment from unsafe array to unsafe pointer (from Array to Span
strategy) are not implemented in this patch as that needs to be properly designed
first - we might possibly implement optimal fixits for partially transformed
cases, put both variables in a single fixit group or do something else.
[-Wunsafe-buffer-usage] Ignore safe array subscripts
Don't emit warnings for array subscripts on constant size arrays where the index is constant and within bounds.
Example:
int arr[10];
arr[5] = 0; //safe, no warning
This patch recognizes only array indices that are integer literals - it doesn't understand more complex expressions (arithmetic on constants, etc.).
-Warray-bounds implemented in Sema::CheckArrayAccess() already solves a similar
(opposite) problem, handles complex expressions and is battle-tested.
Adding -Wunsafe-buffer-usage diagnostics to Sema is a non-starter as we need to emit
both the warnings and fixits and the performance impact of the fixit machine is
unacceptable for Sema.
CheckArrayAccess() as is doesn't distinguish between "safe" and "unknown" array
accesses. It also mixes the analysis that decides if an index is out of bounds
with crafting the diagnostics.
A refactor of CheckArrayAccess() might serve both the original purpose
and help us avoid false-positive with -Wunsafe-buffer-usage on constant
size arrrays.
Currently we ignore calls on function pointers (unlike direct calls of
functions and class methods). This patch adds support for function pointers as
well.
The change is to simply replace use of forEachArgumentWithParam matcher in UPC
gadget with forEachArgumentWithParamType.
from the documentation of forEachArgumentWithParamType:
/// Matches all arguments and their respective types for a \c CallExpr or
/// \c CXXConstructExpr. It is very similar to \c forEachArgumentWithParam but
/// it works on calls through function pointers as well.
Currently the matcher also uses hasPointerType() which checks that the
canonical type of an argument is pointer and won't match on arrays decayed to
pointer. Replacing hasPointerType() with isAnyPointerType() which allows
implicit casts allows for the arrays to be matched as well and this way we get
fixits for array arguments to function pointer calls too.
Covers cases where DeclRefExpr referring to a const-size array decays to a
pointer and is used "as a pointer" (e. g. passed to a pointer type
parameter).
Since std::array<T, N> doesn't implicitly convert to pointer to its element
type T* the cast needs to be done explicitly as part of the fixit
when we retrofit std::array to code that previously worked with constant
size array. std::array::data() method is used for the explicit
cast.
In terms of the fixit machine this covers the UPC(DRE) case for Array fixit strategy.
The emitted fixit inserts call to std::array::data() method similarly to
analogous fixit for Span strategy.
Array subscript on a const size array is not bounds-checked. The idiomatic
replacement is std::array which is bounds-safe in hardened mode of libc++.
This commit extends the fixit-producing machine to consider std::array as a
transformation target type and teaches it to handle the array subscript on const
size arrays with a trivial (empty) fixit.
