[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable
rjmccall added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:10286
+ // these variables must be a compile time constant.
+ VDecl->getType().getAddressSpace() == LangAS::opencl_constant)
CheckForConstantInitializer(Init, DclT);
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > Should this rule apply even in C++ mode? I can't remember if there
> > > > > > are any OpenCL/C++ hybrids.
> > > > > No. This rule (static local variable needs to be initialised with
> > > > > compile-time constant) only applies to C and OpenCL.
> > > > > In C++ static local variable can be initialised with non-compile-time
> > > > > constant, in which case Clang will emit atomic instructions to make
> > > > > sure it is only initialised once.
> > > > >
> > > > > Currently OpenCL 2.2 defines OpenCL C++ but clang does not support it.
> > > > Yes, I understand that C++ generally allows static locals to be lazily
> > > > initialized, and that that rule would (probably) still apply to
> > > > ordinary static locals in OpenCL C++. However, I would expect that
> > > > OpenCL C++ rule is that __constant local variables still need to be
> > > > statically initialized, because there's no plausible way in the OpenCL
> > > > implementation model to actually put lazily-initialized variables in
> > > > the constant address space. Assuming that's correct, then I would
> > > > recommend reworking this whole chain of conditions to:
> > > >
> > > > // Don't check the initializer if the declaration is malformed.
> > > > if (VDecl->isInvalidDecl()) {
> > > > // do nothing
> > > >
> > > > // OpenCL __constant locals must be constant-initialized, even in
> > > > OpenCL C++.
> > > > } else if (VDecl->getType().getAddressSpace() ==
> > > > LangAS::opencl_constant) {
> > > > CheckForConstantInitializer(Init, DclT);
> > > >
> > > > // Otherwise, C++ does not restrict the initializer.
> > > > } else if (getLangOpts().CPlusPlus) {
> > > > // do nothing
> > > >
> > > > // C99 6.7.8p4: All the expressions in an initializer for an object
> > > > that has
> > > > // static storage duration shall be constant expressions or string
> > > > literals.
> > > > } else if (VDecl->getStorageClass() == SC_Static) {
> > > > CheckForConstantInitializer(Init, DclT);
> > > >
> > > > // C89 is stricter than C99 for aggregate initializers.
> > > > // C89 6.5.7p3: All the expressions [...] in an initializer list
> > > > // for an object that has aggregate or union type shall be
> > > > // constant expressions.
> > > > } else if (!getLangOpts().C99 && VDecl->getType()->isAggregateType()
> > > > && isa(Init)) {
> > > > Expr *Culprit;
> > > > if (!Init->isConstantInitializer(Context, false, &Culprit)) {
> > > > ...
> > > > }
> > > > }
> > > Agree that even OpenCL C++ is unable to lazy initialise function-scope
> > > var in constant addr space. Will do.
> > I think the original way would be much simpler to read and understand
> > though.
> >
> > To be honest I wouldn't complicate things now for the feature we don't
> > support. I feel OpenCL C++ should be represented as a separate LangOpt
> > since there are some places that will require special handling due to
> > deviation from C++. I would rather extend things later in more systematic
> > way.
> I will delete the comment about OpenCL C++ when committing.
I disagree. Simple chains like this are almost always superior to building up
complex logical conditions: the priorities between conditions are clearer (such
as the interaction between __constant and C++ here), each condition can be
conveniently documented without having to add comments to the middle of an
expression, and there's less need to build up (A && !B) conditions just to make
sure that cases are routed to the right place. If the body of a clause is
complex, it's usually a good idea to extract it into a separate function
anyway, as has already been done here with CheckForConstantInitializer.
Deleting the comment about OpenCL C++ seems silly. The comment is correct and
explains why the clauses need to be ordered the way they are, and someone
implementing OpenCL C++ support later will not think to add it back.
Please trust me that you would not want to use a different LangOpt for OpenCL
C++. OpenCL C++ may feel a little different from normal C++ to a user, but for
the compiler its deviations are tiny compared to the number of ways in which
C++ deviates from C. The major C++ features that really cut across the
frontend are all still there: templates, references, type members, function
overloading, operator overloading, totally different lookup rules, etc.
https://reviews.llvm.org/D32977
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cg
[PATCH] D30946: [ScopePrinting] Added support to print full scopes of types and declarations.
schroedersi updated this revision to Diff 98887.
schroedersi added a comment.
- Ran clang-format (current trunk version) on the changes (except on some test
files)
- Adapted patch to current trunk version
https://reviews.llvm.org/D30946
Files:
include/clang/AST/PrettyPrinter.h
include/clang/AST/TemplateName.h
lib/AST/Decl.cpp
lib/AST/DeclPrinter.cpp
lib/AST/DeclarationName.cpp
lib/AST/NestedNameSpecifier.cpp
lib/AST/TemplateName.cpp
lib/AST/TypePrinter.cpp
lib/CodeGen/CGDebugInfo.cpp
lib/Tooling/Core/QualTypeNames.cpp
test/CXX/class.access/p6.cpp
test/Index/comment-cplus-decls.cpp
unittests/AST/AbsoluteScopeTest.cpp
unittests/AST/CMakeLists.txt
unittests/AST/NamedDeclPrinterTest.cpp
unittests/AST/TypePrinterTest.cpp
Index: unittests/AST/TypePrinterTest.cpp
===
--- /dev/null
+++ unittests/AST/TypePrinterTest.cpp
@@ -0,0 +1,464 @@
+//===- unittests/AST/TypePrinterTest.cpp -- TypePrinter printer tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file contains tests for TypePrinter::print(...).
+//
+// These tests have a coding convention:
+// * variable whose type to be printed is named 'A' unless it should have some
+// special name
+// * additional helper classes/namespaces/... are 'Z', 'Y', 'X' and so on.
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallString.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ast_matchers;
+using namespace tooling;
+namespace {
+
+class PrintMatch : public MatchFinder::MatchCallback {
+ SmallString<1024> Printed;
+ unsigned NumFoundDecls;
+ bool SuppressUnwrittenScope;
+ ScopePrintingKind::ScopePrintingKind Scope;
+
+public:
+ explicit PrintMatch(bool suppressUnwrittenScope,
+ ScopePrintingKind::ScopePrintingKind scope)
+ : NumFoundDecls(0), SuppressUnwrittenScope(suppressUnwrittenScope),
+Scope(scope) {}
+
+ void run(const MatchFinder::MatchResult &Result) override {
+const ValueDecl *VD = Result.Nodes.getNodeAs("id");
+if (!VD)
+ return;
+NumFoundDecls++;
+if (NumFoundDecls > 1)
+ return;
+
+llvm::raw_svector_ostream Out(Printed);
+PrintingPolicy Policy = Result.Context->getPrintingPolicy();
+Policy.SuppressUnwrittenScope = SuppressUnwrittenScope;
+Policy.Scope = Scope;
+QualType Type = VD->getType();
+Type.print(Out, Policy);
+ }
+
+ StringRef getPrinted() const { return Printed; }
+
+ unsigned getNumFoundDecls() const { return NumFoundDecls; }
+};
+
+::testing::AssertionResult
+PrintedTypeMatches(StringRef Code, const std::vector &Args,
+ bool SuppressUnwrittenScope,
+ const DeclarationMatcher &NodeMatch,
+ StringRef ExpectedPrinted, StringRef FileName,
+ ScopePrintingKind::ScopePrintingKind Scope) {
+ PrintMatch Printer(SuppressUnwrittenScope, Scope);
+ MatchFinder Finder;
+ Finder.addMatcher(NodeMatch, &Printer);
+ std::unique_ptr Factory =
+ newFrontendActionFactory(&Finder);
+
+ if (!runToolOnCodeWithArgs(Factory->create(), Code, Args, FileName))
+return testing::AssertionFailure()
+ << "Parsing error in \"" << Code.str() << "\"";
+
+ if (Printer.getNumFoundDecls() == 0)
+return testing::AssertionFailure()
+ << "Matcher didn't find any value declarations";
+
+ if (Printer.getNumFoundDecls() > 1)
+return testing::AssertionFailure()
+ << "Matcher should match only one value declaration "
+ "(found "
+ << Printer.getNumFoundDecls() << ")";
+
+ if (Printer.getPrinted() != ExpectedPrinted)
+return ::testing::AssertionFailure()
+ << "Expected \"" << ExpectedPrinted.str()
+ << "\", "
+ "got \""
+ << Printer.getPrinted().str() << "\"";
+
+ return ::testing::AssertionSuccess();
+}
+
+::testing::AssertionResult
+PrintedTypeCXX11MatchesWrapper(StringRef Code, StringRef DeclName,
+ StringRef ExpectedPrinted,
+ ScopePrintingKind::ScopePrintingKind Scope) {
+ std::vector Args(1, "-std=c++11");
+ return PrintedTypeMatches(Code, Args,
+/*SuppressUnwrittenScope*/ true,
+valueDecl(hasName(DeclName)).bind("id"),
+ExpectedPrinted, "input.cc", Scope);
+}
+
+::testing::AssertionResult
+PrintedTypeCXX11Matches(StringRef Code, StringRef DeclName,
+StringRef ExpectedPrintedAllScopes,
+
[PATCH] D30946: [ScopePrinting] Added support to print full scopes of types and declarations.
schroedersi added a comment.
In https://reviews.llvm.org/D30946#740567, @bkramer wrote:
> Also the mutable state in PrintingPolicy is really really ugly, is there no
> better way for this? :(
Thanks for your comment :-)
I assume with mutable state you mean `PrintingPolicy::TemporarySuppressScope`?
I do not really like it either. But other states of the policy are also mutated
during the printing process (e.g. SuppressScope, SuppressTagKeyword and
SuppressSpecifiers inside the RAII helpers).
A little background: It is necessary that scope printing can be temporarily
suppressed without changing the actual scope printing kind setting. For
example, in the case of an ElaboratedType, it is necessary that the underlying
type is printed without the outer scope (because it has already been printed
based on how it was written in the source and based on the scope printing kind
setting). However, the internal type should be printed according to the scope
printing kind setting. Another example are types inside a nested name
specifier. Again, the outer scope can not be printed. However, inner scopes
(e.g. for template arguments) must be printed according to the scope printing
kind setting.
For this temporal suppression, the information about whether a scope has
already been suppressed and thus the scope printing kind setting must be used,
must be shared over all member functions involved in the printing. These member
functions are the TypePrinter member functions and
`TemplateName::print`, `NestedNameSpecifier::print`, and
`NamedDecl::printQualifiedName`. Currently, the only object shared across all
these member functions is the PrintingPolicy. That is why I added the state to
the PrintingPolicy.
As an alternative to the current solution, the above-mentioned member functions
could each be extended by a "PrintingContext" argument, which then contains the
variable states.
And a bit of clarification: Without the patch, even with `SuppressScope = true`
it is not possible to suppress all scopes, and inner types are printed with a
scope most of the time (which seems to lead to the desired effect for most
users). For example, printing the type of `A` in `namespace N{class B{};
template class C{}; C<::N::B> A;}` with `SuppressScope = true`
results in `C< ::N::B>` which still includes scopes. That is, there is already
some kind of temporary suppress scope functionality, but it is called
SuppressScope. The patch only expands this functionality to full scopes (see
summary above), makes the behavior more consistent, and ensures that the
settings do what - at least I - would expect from their name and documentation.
I hope this explains my decision. Either way, I look forward to further
comments :-).
https://reviews.llvm.org/D30946
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33053: [PowerPC] Implement vec_xxpermdi builtin.
nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.
Add a test case like the one that currently crashes (see inline comment). Also,
please do the following:
- Put a note in the description (and the commit message) with a link to the PR
this fixes
- Put a note in the description that there is subsequent back end work to
identify the correct shuffles (i.e. we currently won't emit an XXPERMDI when
parameters are not vectors of doubleword elements)
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7997
-def err_shufflevector_non_vector : Error<
- "first two arguments to __builtin_shufflevector must be vectors">;
It isn't appropriate to remove a target-independent diagnostic and replace it
with a target-specific one (even if it's just in the name). I think it makes
sense to update both `err_shufflevector_non_vector` and
`err_shufflevector_incompatible_vector` to take an argument for the name of the
builtin, but not to replace it with a target-specific one.
I would recommend renaming it to something like:
`err_shufflevector_non_vector` -> `err_vec_builtin_non_vector`
`err_shufflevector_incompatible_vector` -> `err_vec_builtin_incompatible_vector`
and add the parameter.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8005
+def err_vsx_builtin_nonconstant_argument3 : Error<
+ "third argument to %0 must be a constant integer between 0-3">;
We shouldn't diagnose out-of-range values.
Comment at: lib/CodeGen/CGBuiltin.cpp:8407
// Create a shuffle mask of (1, 0)
- Constant *ShuffleElts[2] = { ConstantInt::get(Int32Ty, 1),
- ConstantInt::get(Int32Ty, 0)
- };
+ Constant *ShuffleElts[2] = {ConstantInt::get(Int32Ty, 1),
+ ConstantInt::get(Int32Ty, 0)};
This change is unrelated and inconsequential. Please revert it.
Comment at: lib/CodeGen/CGBuiltin.cpp:8425
+const int64_t MaxIndex = 3;
+int64_t Index = clamp(ArgCI->getSExtValue(), 0, MaxIndex);
+Ops[0] = Builder.CreateBitCast(Ops[0], llvm::VectorType::get(Int64Ty, 2));
We shouldn't clamp this. The correct semantics should be to mask out everything
but the low order two bits (i.e. `&0x3`). But I think even that is unnecessary
since you should just use the low order two bits from the input - see below.
Comment at: lib/CodeGen/CGBuiltin.cpp:8433
+if (getTarget().isLittleEndian()) {
+ switch (Index) {
+ case 0:
The switch is overkill. You should just implement this in an obvious way (i.e.
the same way as described in the ISA).
For big endian:
`ElemIdx0 = (Index & 2;) >> 1`
`ElemIdx1 = 2 + (Index & 1)`
For little endian:
`ElemIdx0 = (~Index & 1) + 2;`
`ElemIdx1 = ~Index & 2 >> 1;`
(of course, please verify the expressions).
Comment at: lib/CodeGen/CGBuiltin.cpp:8482
+Builder.CreateShuffleVector(Ops[0], Ops[1], ShuffleMask);
+return ShuffleCall;
+ }
This is not going to work. You can try it with this simple test case that it
crashes with:
```
#include
vector int test(vector int a, vector int b) {
return vec_xxpermdi(a, b, 0);
}
```
The problem is that the return type of the call expression may be different
from the return type of the `shufflevector`.
You'll probably need something like this:
```
QualType BIRetType = E->getType();
auto RetTy = ConvertType(BIRetType);
return Builder.CreateBitCast(ShuffleCall, RetTy);
```
Comment at: lib/Sema/SemaChecking.cpp:3900
+// vector short vec_xxsldwi(vector short, vector short, int);
+bool Sema::SemaBuiltinVSX(CallExpr *TheCall, unsigned NumArgs) {
+ if (TheCall->getNumArgs() < NumArgs)
I assume that we won't even need this at all if we're not diagnosing the range
of the third argument.
Comment at: lib/Sema/SemaChecking.cpp:3915
+ llvm::APSInt Value;
+ bool IsICE = TheCall->getArg(2)->isIntegerConstantExpr(Value, Context);
+ if (!(IsICE && Value >= 0 && Value <= 3))
No, we don't want to diagnose this. Values larger than 3 should just use the
low-order two bits. This matches GCC behaviour.
https://reviews.llvm.org/D33053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33010: Make google-build-using-namespace skip std::.*literals
marejde updated this revision to Diff 98893.
marejde added a comment.
Removed use of getQualifiedNameAsString().
https://reviews.llvm.org/D33010
Files:
clang-tidy/google/UsingNamespaceDirectiveCheck.cpp
clang-tidy/google/UsingNamespaceDirectiveCheck.h
test/clang-tidy/google-namespaces.cpp
Index: test/clang-tidy/google-namespaces.cpp
===
--- test/clang-tidy/google-namespaces.cpp
+++ test/clang-tidy/google-namespaces.cpp
@@ -6,3 +6,31 @@
// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use
using-declarations instead [google-build-using-namespace]
using spce::core; // no-warning
+
+namespace std {
+inline namespace literals {
+inline namespace chrono_literals {
+}
+inline namespace complex_literals {
+}
+inline namespace string_literals {
+}
+}
+}
+
+using namespace std::chrono_literals;// no-warning
+using namespace std::complex_literals; // no-warning
+using namespace std::literals; // no-warning
+using namespace std::literals::chrono_literals; // no-warning
+using namespace std::literals::complex_literals; // no-warning
+using namespace std::literals::string_literals; // no-warning
+using namespace std::string_literals;// no-warning
+
+namespace literals {}
+namespace foo::literals {}
+
+using namespace literals;
+// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use
using-declarations instead [google-build-using-namespace]
+
+using namespace foo::literals;
+// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use
using-declarations instead [google-build-using-namespace]
Index: clang-tidy/google/UsingNamespaceDirectiveCheck.h
===
--- clang-tidy/google/UsingNamespaceDirectiveCheck.h
+++ clang-tidy/google/UsingNamespaceDirectiveCheck.h
@@ -38,6 +38,9 @@
: ClangTidyCheck(Name, Context) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ bool isStdLiteralsNamespace(const NamespaceDecl *NS);
};
} // namespace build
Index: clang-tidy/google/UsingNamespaceDirectiveCheck.cpp
===
--- clang-tidy/google/UsingNamespaceDirectiveCheck.cpp
+++ clang-tidy/google/UsingNamespaceDirectiveCheck.cpp
@@ -34,12 +34,32 @@
if (U->isImplicit() || !Loc.isValid())
return;
+ // Do not warn if namespace is a std namespace with user-defined literals.
The
+ // user-defined literals can only be used with a using directive.
+ if (isStdLiteralsNamespace(U->getNominatedNamespace()))
+return;
+
diag(Loc, "do not use namespace using-directives; "
"use using-declarations instead");
// TODO: We could suggest a list of using directives replacing the using
// namespace directive.
}
+bool UsingNamespaceDirectiveCheck::isStdLiteralsNamespace(
+const NamespaceDecl *NS) {
+ if (!NS->getName().endswith("literals"))
+return false;
+
+ const auto *Parent = dyn_cast_or_null(NS->getParent());
+ if (!Parent)
+return false;
+
+ if (Parent->isStdNamespace())
+return true;
+
+ return Parent->getName() == "literals" && Parent->getParent() &&
+ Parent->getParent()->isStdNamespace();
+}
} // namespace build
} // namespace google
} // namespace tidy
Index: test/clang-tidy/google-namespaces.cpp
===
--- test/clang-tidy/google-namespaces.cpp
+++ test/clang-tidy/google-namespaces.cpp
@@ -6,3 +6,31 @@
// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
using spce::core; // no-warning
+
+namespace std {
+inline namespace literals {
+inline namespace chrono_literals {
+}
+inline namespace complex_literals {
+}
+inline namespace string_literals {
+}
+}
+}
+
+using namespace std::chrono_literals;// no-warning
+using namespace std::complex_literals; // no-warning
+using namespace std::literals; // no-warning
+using namespace std::literals::chrono_literals; // no-warning
+using namespace std::literals::complex_literals; // no-warning
+using namespace std::literals::string_literals; // no-warning
+using namespace std::string_literals;// no-warning
+
+namespace literals {}
+namespace foo::literals {}
+
+using namespace literals;
+// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
+
+using namespace foo::literals;
+// CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
Index: clang-tidy/google/UsingNamespaceDirectiveCheck.h
=
[PATCH] D33049: [libcxx] Support for Objective-C++ tests
dexonsmith added inline comments. Comment at: utils/libcxx/compiler.py:110-114 +if input_is_text: +if is_objcxx: +cmd += ['-x', 'objective-c++', '-fobjc-arc'] +else: +cmd += ['-x', 'c++'] Do we need/want a way to choose between ARC and no-ARC? https://reviews.llvm.org/D33049 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30909: [Analyzer] Finish taint propagation to derived symbols of tainted regions
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:656-659
+ // If the SVal is a LazyCompoundVal it might only cover sub-region of a given
+ // symbol. For example, the LCV might represent a field in an uninitialized
+ // struct. In this case, the LCV encapsulates a conjured symbol and reference
+ // to the sub-region representing the struct field.
NoQ wrote:
> vlad.tsyrklevich wrote:
> > NoQ wrote:
> > > I still feel bad about producing API with very tricky pre-conditions. The
> > > LCV may have various forms - some may have empty store with no symbols at
> > > all, and others may be full of direct bindings that make the symbol
> > > essentially irrelevant. However, because the taint API is designed to be
> > > defensive to cases when taint cannot be added, and it sounds like a good
> > > thing, i guess we've taken the right approach here :)
> > >
> > > I suggest commenting this more thoroughly though, something like:
> > >
> > > > If the SVal represents a structure, try to mass-taint all values within
> > > > the structure. For now it only works efficiently on lazy compound
> > > > values that were conjured during a conservative evaluation of a
> > > > function - either as return values of functions that return structures
> > > > or arrays by value, or as values of structures or arrays passed into
> > > > the function by reference, directly or through pointer aliasing. Such
> > > > lazy compound values are characterized by having exactly one binding in
> > > > their captured store within their parent region, which is a conjured
> > > > symbol default-bound to the base region of the parent region.
> > >
> > > Then inside `if (Sym)`:
> > >
> > > > If the parent region is a base region, we add taint to the whole
> > > > conjured symbol.
> > >
> > > > Otherwise, when the value represents a record-typed field within the
> > > > conjured structure, so we add partial taint for that symbol to that
> > > > field.
> > The pre-conditions for using the API are actually a bit simpler than what's
> > exposed here. I made it explicit to make the logic for tainting LCVs
> > explicit, but the following simpler logic works:
> > ```
> > if (auto LCV = V.getAs()) {
> > if (Optional binding =
> > getStateManager().StoreMgr->getDefaultBinding(*LCV)) {
> > if (SymbolRef Sym = binding->getAsSymbol()) {
> > return addPartialTaint(Sym, LCV->getRegion(), Kind);
> > }
> > }
> > }
> > ```
> >
> > This works because `addPartialTaint()` actually already performs the
> > 'getRegion() == getRegion()->getBaseRegion()` check already and taints the
> > parent symbol if the region is over the base region already. I chose to
> > replicate it here to make the logic more explicit, but now that I've
> > written this down the overhead of duplicating the logic seems unnecessary.
> > Do you agree?
> > The pre-conditions for using the API are actually a bit simpler than what's
> > exposed here.
>
> I'm talking about the situation when we add the partial taint to the
> default-bound symbol but it has no effect because there's a direct binding in
> the lazy compound value on top of it. The user should ideally understand why
> it doesn't work that way.
>
> > I chose to replicate it here to make the logic more explicit, but now that
> > I've written this down the overhead of duplicating the logic seems
> > unnecessary. Do you agree?
>
> The variant without the check looks easier to read, and it is kind of obvious
> that full taint is a special case of partial taint, so i'm for removing the
> check.
> when we add the partial taint to the default-bound symbol but it has no
> effect because there's a direct binding in the lazy compound value on top of
> it
Ah, so you're talking about the case where the LCV encompasses a value with
both direct & default bindings, e.g. `int foo[1024]; foo[123] = rand();
taint(foo);`? In that case we will miss tainting the `rand` SymbolConjured. I
suppose we could scan the region store for matching entries? In practice I
think we're really only interested in tainting default bindings anyway for some
unknown network/user input anyway.
Anyways, your comment makes more sense now. I've added it.
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:788
+ (R == I->first || R->isSubRegionOf(I->first)))
+return true;
+}
NoQ wrote:
> I actually have no idea why does this function accumulate things in the
> `Tainted` variable, instead of returning :)
It made a little more stylistic sense before this change, but not so much now.
I've updated them to return immediately.
https://reviews.llvm.org/D30909
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30909: [Analyzer] Finish taint propagation to derived symbols of tainted regions
vlad.tsyrklevich updated this revision to Diff 98905.
vlad.tsyrklevich added a comment.
Some stylistic & comment updates.
https://reviews.llvm.org/D30909
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
include/clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h
lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
lib/StaticAnalyzer/Core/ProgramState.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/taint-generic.c
Index: test/Analysis/taint-generic.c
===
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -192,20 +192,41 @@
void testStructArray() {
struct {
-char buf[16];
-struct {
- int length;
-} st[1];
- } tainted;
+int length;
+ } tainted[4];
- char buffer[16];
+ char dstbuf[16], srcbuf[16];
int sock;
sock = socket(AF_INET, SOCK_STREAM, 0);
- read(sock, &tainted.buf[0], sizeof(tainted.buf));
- read(sock, &tainted.st[0], sizeof(tainted.st));
- // FIXME: tainted.st[0].length should be marked tainted
- __builtin_memcpy(buffer, tainted.buf, tainted.st[0].length); // no-warning
+ __builtin_memset(srcbuf, 0, sizeof(srcbuf));
+
+ read(sock, &tainted[0], sizeof(tainted));
+ __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // expected-warning {{Untrusted data is used to specify the buffer size}}
+
+ __builtin_memset(&tainted, 0, sizeof(tainted));
+ read(sock, &tainted, sizeof(tainted));
+ __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // expected-warning {{Untrusted data is used to specify the buffer size}}
+
+ __builtin_memset(&tainted, 0, sizeof(tainted));
+ // If we taint element 1, we should not raise an alert on taint for element 0 or element 2
+ read(sock, &tainted[1], sizeof(tainted));
+ __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // no-warning
+ __builtin_memcpy(dstbuf, srcbuf, tainted[2].length); // no-warning
+}
+
+void testUnion() {
+ union {
+int x;
+char y[4];
+ } tainted;
+
+ char buffer[4];
+
+ int sock = socket(AF_INET, SOCK_STREAM, 0);
+ read(sock, &tainted.y, sizeof(tainted.y));
+ // FIXME: overlapping regions aren't detected by isTainted yet
+ __builtin_memcpy(buffer, tainted.y, tainted.x);
}
int testDivByZero() {
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -496,7 +496,10 @@
Optional getDefaultBinding(Store S, const MemRegion *R) override {
RegionBindingsRef B = getRegionBindings(S);
-return B.getDefaultBinding(R);
+// Default bindings are always applied over a base region so look up the
+// base region's default binding, otherwise the lookup will fail when R
+// is at an offset from R->getBaseRegion().
+return B.getDefaultBinding(R->getBaseRegion());
}
SVal getBinding(RegionBindingsConstRef B, Loc L, QualType T = QualType());
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -644,15 +644,33 @@
if (const Expr *E = dyn_cast_or_null(S))
S = E->IgnoreParens();
- SymbolRef Sym = getSVal(S, LCtx).getAsSymbol();
+ return addTaint(getSVal(S, LCtx), Kind);
+}
+
+ProgramStateRef ProgramState::addTaint(SVal V,
+ TaintTagType Kind) const {
+ SymbolRef Sym = V.getAsSymbol();
if (Sym)
return addTaint(Sym, Kind);
- const MemRegion *R = getSVal(S, LCtx).getAsRegion();
- addTaint(R, Kind);
+ // If the SVal represents a structure, try to mass-taint all values within the
+ // structure. For now it only works efficiently on lazy compound values that
+ // were conjured during a conservative evaluation of a function - either as
+ // return values of functions that return structures or arrays by value, or as
+ // values of structures or arrays passed into the function by reference,
+ // directly or through pointer aliasing. Such lazy compound values are
+ // characterized by having exactly one binding in their captured store within
+ // their parent region, which is a conjured symbol default-bound to the base
+ // region of the parent region.
+ if (auto LCV = V.getAs()) {
+if (Optional binding = getStateManager().StoreMgr->getDefaultBinding(*LCV)) {
+ if (SymbolRef Sym = binding->getAsSymbol())
+return addPartialTaint(Sym, LCV->getRegion(), Kind);
+}
+ }
- // Cannot add taint, so just return the state.
- return this;
+ const MemRegion *R = V.getAsRegion();
+ return addTaint(R, Kind);
}
ProgramStateRef ProgramState::addTaint(const MemRegion *R,
@@ -674,6 +692,28 @@
return NewState;
}
+ProgramStateRef ProgramState::addPartialTaint(SymbolRef ParentSym,
+ const SubRegion *SubRegion,
+
