Re: r335084 - Append new attributes to the end of an AttributeList.
Hi, multiple comments in the code indicate that the attribute order was surprising and probably has lead to bugs, and will lead to bugs in the future. The order had to be explicitly reversed to avoid those. This alone for me this seems a good reason to have the attributes in the order in which they appear in the source. It would be nice it you could send a reproducer. At a glance, your case does not seem related since the format strings are function arguments, not attributes. Michael 2018-06-23 17:17 GMT-05:00 David Jones : > This commit seems to have some substantial downsides... for example, it now > flags calls like this as warnings: > http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-tools/src/msgl-check.c?id=05ecefa943f339019b7127aa92cbb09f6fd49ed3#n478 > > Previously, the reverse order meant that the plural format string was > examined, but now it is only the singular string. Since the singular string > doesn't include a substitution, the unused format variable warning fires > after this revision. > > I don't see a strong argument for why one order is more correct than the > other; however, given that this is in conflict with real code found in the > wild, I think this needs to be addressed. > > Since the semantics of the ordering of multiple format args seems somewhat > ill-defined, it seems to me like reverting may be the best near-term choice. > I can imagine other ways to fix the diagnostic, but the only behaviour that > I would believe to be expected by existing code is the old one, so a change > like this one probably needs to be more carefully vetted before being > (re-)committed. Could you give more details about your concerns? > Please let me know your plan. (If I don't hear back in a day or so, I'll go > ahead and revert for you as the safe default course of action.) On a weekend? ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48521: [analyzer] Highlight container object destruction in MallocChecker
rnkovacs updated this revision to Diff 152615.
rnkovacs marked 7 inline comments as done.
rnkovacs retitled this revision from "[analyzer] Highlight STL object
destruction in MallocChecker" to "[analyzer] Highlight container object
destruction in MallocChecker".
rnkovacs added a comment.
Thanks for the comments!
I'll run this on some projects and see if any assertions fail.
https://reviews.llvm.org/D48521
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -26,7 +26,7 @@
{
std::string s;
c = s.c_str();
- }
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
consume(c); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
@@ -36,7 +36,7 @@
{
std::wstring ws;
w = ws.c_str();
- }
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
consume(w); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
@@ -46,7 +46,7 @@
{
std::u16string s16;
c16 = s16.c_str();
- }
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
consume(c16); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
@@ -56,7 +56,7 @@
{
std::u32string s32;
c32 = s32.c_str();
- }
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
consume(c32); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -481,8 +481,13 @@
inline bool isReleased(const RefState *S, const RefState *SPrev,
const Stmt *Stmt) {
// Did not track -> released. Other state (allocated) -> released.
- return (Stmt && (isa(Stmt) || isa(Stmt)) &&
- (S && S->isReleased()) && (!SPrev || !SPrev->isReleased()));
+ // The statement associated with the release might be missing.
+ bool IsReleased = (S && S->isReleased()) &&
+(!SPrev || !SPrev->isReleased());
+ assert(!IsReleased ||
+ (Stmt && (isa(Stmt) || isa(Stmt))) ||
+ (!Stmt && S->getAllocationFamily() == AF_InternalBuffer));
+ return IsReleased;
}
inline bool isRelinquished(const RefState *S, const RefState *SPrev,
@@ -2851,8 +2856,17 @@
std::shared_ptr MallocChecker::MallocBugVisitor::VisitNode(
const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
BugReport &BR) {
+
+ ProgramStateRef state = N->getState();
+ ProgramStateRef statePrev = PrevN->getState();
+
+ const RefState *RS = state->get(Sym);
+ const RefState *RSPrev = statePrev->get(Sym);
+
const Stmt *S = PathDiagnosticLocation::getStmt(N);
- if (!S)
+ // When dealing with containers, we sometimes want to give a note
+ // even if the statement is missing.
+ if (!S && (!RS || RS->getAllocationFamily() != AF_InternalBuffer))
return nullptr;
const LocationContext *CurrentLC = N->getLocationContext();
@@ -2877,14 +2891,6 @@
}
}
- ProgramStateRef state = N->getState();
- ProgramStateRef statePrev = PrevN->getState();
-
- const RefState *RS = state->get(Sym);
- const RefState *RSPrev = statePrev->get(Sym);
- if (!RS)
-return nullptr;
-
// FIXME: We will eventually need to handle non-statement-based events
// (__attribute__((cleanup))).
@@ -2897,7 +2903,22 @@
StackHint = new StackHintGeneratorForSymbol(Sym,
"Returned allocated memory");
} else if (isReleased(RS, RSPrev, S)) {
- Msg = "Memory is released";
+ const auto Family = RS->getAllocationFamily();
+ switch(Family) {
+case AF_Alloca:
+case AF_Malloc:
+case AF_CXXNew:
+case AF_CXXNewArray:
+case AF_IfNameIndex:
+ Msg = "Memory is released";
+ break;
+case AF_InternalBuffer:
+ Msg = "Internal buffer is released because the object was destroyed";
+ break;
+case AF_None:
+default:
+ llvm_unreachable("Unhandled allocation family!");
+ }
StackHint = new StackHintGeneratorForSymbol(Sym,
"Returning; memory was released");
@@ -2968,8 +2989,19 @@
assert(StackHint);
// Generate the extra diagnostic.
- PathDiagnosticLocation Pos(S, BRC.getSo
[PATCH] D48521: [analyzer] Highlight container object destruction in MallocChecker
xazax.hun accepted this revision. xazax.hun added a comment. LGTM, given that the assert does not fire for the projects you tested on. https://reviews.llvm.org/D48521 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48460: [analyzer] Fix invalidation on C++ const methods.
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang https://reviews.llvm.org/D48460 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48522: [analyzer] Highlight c_str() call in DanglingInternalBuffer checker
rnkovacs updated this revision to Diff 152616.
rnkovacs marked 4 inline comments as done.
rnkovacs added a comment.
Thanks! Addressed comments.
https://reviews.llvm.org/D48522
Files:
lib/StaticAnalyzer/Checkers/AllocationState.h
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -25,17 +25,29 @@
const char *c;
{
std::string s;
-c = s.c_str();
+c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_char2() {
+ const char *c;
+ {
+std::string s;
+c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
} // expected-note {{Internal buffer is released because the object was destroyed}}
+ std::string s;
+ const char *c2 = s.c_str();
consume(c); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
void deref_after_scope_wchar_t() {
const wchar_t *w;
{
std::wstring ws;
-w = ws.c_str();
+w = ws.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
} // expected-note {{Internal buffer is released because the object was destroyed}}
consume(w); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
@@ -45,7 +57,7 @@
const char16_t *c16;
{
std::u16string s16;
-c16 = s16.c_str();
+c16 = s16.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
} // expected-note {{Internal buffer is released because the object was destroyed}}
consume(c16); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
@@ -55,7 +67,7 @@
const char32_t *c32;
{
std::u32string s32;
-c32 = s32.c_str();
+c32 = s32.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
} // expected-note {{Internal buffer is released because the object was destroyed}}
consume(c32); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1994,6 +1994,11 @@
R->markInteresting(Sym);
R->addRange(Range);
R->addVisitor(llvm::make_unique(Sym));
+
+const RefState *RS = C.getState()->get(Sym);
+if (RS->getAllocationFamily() == AF_InternalBuffer)
+ R->addVisitor(allocation_state::getDanglingBufferBRVisitor(Sym));
+
C.emitReport(std::move(R));
}
}
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,13 +24,52 @@
using namespace clang;
using namespace ento;
+// FIXME: c_str() may be called on a string object many times, so it should
+// have a list of symbols associated with it.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+
namespace {
class DanglingInternalBufferChecker : public Checker {
CallDescription CStrFn;
public:
+ class DanglingBufferBRVisitor
+ : public BugReporterVisitorImpl {
+// Tracked pointer to a buffer.
+SymbolRef Sym;
+
+ public:
+DanglingBufferBRVisitor(SymbolRef Sym) : Sym(Sym) {}
+
+static void *getTag() {
+ static int Tag = 0;
+ return &Tag;
+}
+
+void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.AddPointer(getTag());
+}
+
+std::shared_ptr VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+
+// FIXME: Scan the map once in the visitor's constructor and do a direct
+// lookup by region.
+bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
+ RawPtrMapTy Map = State->get();
+ for (const auto Entry : Map) {
+if (Entry.second == Sym)
+ return true;
+ }
+ return false;
+}
+
+ };
+
DanglingInternalBufferChecker() : CStrFn("c_str") {}
/// Record the connection between the sym
[PATCH] D48522: [analyzer] Highlight c_str() call in DanglingInternalBuffer checker
xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:41 +// Tracked pointer to a buffer. +SymbolRef Sym; + I am fine with this as is, but I prefer self documenting code in general. Naming this variable PtrToBuf or something like that would conway the same information and render the comment redundant. https://reviews.llvm.org/D48522 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44143: [clang-tidy] Create properly seeded random generator check
boga95 marked 12 inline comments as done.
boga95 added a comment.
I think I resolved all of the comments. Do I forget anything?
Comment at: clang-tidy/cert/CERTTidyModule.cpp:44
"cert-dcl54-cpp");
-CheckFactories.registerCheck(
-"cert-dcl58-cpp");
+
CheckFactories.registerCheck("cert-dcl58-cpp");
CheckFactories.registerCheck(
aaron.ballman wrote:
> This change looks unrelated to the patch.
Clang format did it when I apply it to the whole file.
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:74
+ callExpr(has(implicitCastExpr(has(
+ declRefExpr(hasDeclaration(namedDecl(hasName("srand"
+ .bind("srand"),
aaron.ballman wrote:
> I think that in C mode, this is fine, but in C++ mode it should register
> `::std::srand`.
It is not match for ##::std::srand##, just for ##::srand##. I found some
examples, but I think they don't work neither.
Comment at: docs/clang-tidy/checks/cert-msc51-cpp.rst:29
+ }
+
+Options
Eugene.Zelenko wrote:
> Is there guideline documentation available online? If so, please add link.
> See other checks documentation as example.
There is a guideline [[
http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html | here
]].
https://reviews.llvm.org/D44143
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.
a_sidorin added a comment.
Hello Balázs,
Please clang-format the tests and delete injected-class-name-decl-1. Don't see
any other issues.
Comment at: lib/AST/ASTImporter.cpp:2132
+if (!DCXX->isInjectedClassName()) {
+ // In a record describing a template the type should be a
+ // InjectedClassNameType (see Sema::CheckClassTemplate). Update the
Nit: "an InjectedClassNameType".
Comment at: test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp:16
+} // namespace google
+namespace a {
+template class g;
martong wrote:
> balazske wrote:
> > a.sidorin wrote:
> > > This looks like raw creduce output. Is there a way to simplify this or
> > > make human-readable? Do we really need nested namespaces, unused decls
> > > and other stuff not removed by creduce? I know that creduce is bad at
> > > reducing templates because we often meet similar output internally. But
> > > it is usually not a problem to resolve some redundancy manually to assist
> > > creduce. In this case, we can start by removing `k` and `n`.
> > > We can leave this code as-is only if removing declarations or simplifying
> > > templates affects import order causing the bug to disappear. But even in
> > > this case we have to humanize the test.
> > Probably remove this test? There was some bug in a previous version of the
> > fix that was triggered by this test. Before that fix (and on current
> > master) this test does not fail so it is not possible to simplify it.
> I vote on deleting this test then. We already have another clear and simple
> test.
I think we should delete this test. As I see, it passes even in upstream clang,
so it doesn't really make sense to keep it.
Repository:
rC Clang
https://reviews.llvm.org/D47450
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47534: [ASTImporter] Add new tests about templated-described swing
a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.
Hi Gabor,
Tests are always welcome. This patch is OK and can be committed after fixing
nits without additional approval.
Comment at: unittests/AST/ASTImporterTest.cpp:1097
+ struct X;
+ template
+ struct X {};
Broken indentation.
Comment at: unittests/AST/ASTImporterTest.cpp:1098
+ template
+ struct X {};
+};
T0 * (missed space)
Repository:
rC Clang
https://reviews.llvm.org/D47534
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions
a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.
Hi Gabor,
The patch LG, thank you for addressing my questions. Just some stylish nits.
Comment at: unittests/AST/ASTImporterTest.cpp:2021
+
+TEST_P(ImportFriendFunctions,
+ DISABLED_ImportFriendFunctionRedeclChainDefWithClass_ImportTheProto) {
Could you add comments why these tests are disabled?
Comment at: unittests/AST/ASTImporterTest.cpp:2217
+
+ //Check that the function template instantiation is NOT the child of the TU
+ auto Pattern = translationUnitDecl(
Please add a space after '//' and a dot in the end. Same below.
Comment at: unittests/AST/ASTImporterTest.cpp:2328
+ Lang_CXX11, "input0.cc");
+ auto *FromD = FirstDeclMatcher().match(
+ FromTU, functionDecl(hasName("foo")));
In this patch, sometimes '*' is used for auto, sometimes not. Could we make it
consistent?
Repository:
rC Clang
https://reviews.llvm.org/D47532
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47367: [ASTImporter] Add ms compatibility to tests
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thank you! Could you describe the refactoring in the commit message? Comment at: unittests/AST/ASTImporterTest.cpp:212 + /// The verification is done by running VerificationMatcher against a + /// specified + /// AST node inside of one of given files. Looks like clang-format broke this comment in some places. Repository: rC Clang https://reviews.llvm.org/D47367 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47459: [ASTImporter] Eliminated some unittest warnings.
a_sidorin added a comment.
Hello Balázs,
The patch is mostly LG now, thank you!
Comment at: unittests/AST/ASTImporterTest.cpp:495
" struct s { int x; long y; unsigned z; }; "
- " (struct s){ 42, 0L, 1U }; }",
+ " (void) (struct s){ 42, 0L, 1U }; }",
Lang_CXX, "", Lang_CXX, Verifier,
Redundant space after cast.
Comment at: unittests/AST/ASTImporterTest.cpp:541
+ Lang_C, "", Lang_C, Verifier,
+ /*functionDecl(hasBody(compoundStmt(
has(labelStmt(hasDeclaration(labelDecl(hasName("loop"),
Commented code should be removed.
Comment at: unittests/AST/ASTImporterTest.cpp:802
Lang_CXX, "", Lang_CXX, Verifier,
- functionTemplateDecl(has(functionDecl(
- has(compoundStmt(has(cxxDependentScopeMemberExpr(;
+ functionTemplateDecl(has(functionDecl(has(compoundStmt(has(
+ cStyleCastExpr(has(
Sometimes we turn to usehasDescendant(), sometimes we add a cast matcher. Can
we make it consistent?
Repository:
rC Clang
https://reviews.llvm.org/D47459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48462: [X86] Update handling in CGBuiltin to be tolerant of out of range immediates.
craig.topper closed this revision. craig.topper added a comment. Committed in r335308, https://reviews.llvm.org/D48462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.
ikudrin created this revision.
ikudrin added reviewers: rsmith, rjmccall.
ikudrin added a project: clang.
Herald added subscribers: JDevlieghere, aprantl.
At the moment. when generating UBSan diagnostics for these cases, we rely on
the corresponding debug information, which might be absent, and, anyway,
not that accurate.
Repository:
rC Clang
https://reviews.llvm.org/D48531
Files:
lib/CodeGen/CGClass.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/ubsan-ctor-srcloc.cpp
Index: test/CodeGenCXX/ubsan-ctor-srcloc.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-ctor-srcloc.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux -emit-llvm -fsanitize=alignment -fblocks %s -o %t.ll
+// RUN: FileCheck -check-prefix=ZEROINIT < %t.ll %s
+// RUN: FileCheck -check-prefix=SRCLOC < %t.ll %s
+
+// ZEROINIT-NOT: @{{.+}} = private unnamed_addr global {{.+}} zeroinitializer
+struct A {
+ A(int);
+ int k;
+};
+
+struct B : A {
+ B();
+ B(const B &);
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 12 }
+ using A::A;
+ void f() const;
+};
+
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 10 }
+B::B() : A(1) {}
+
+void foo() {
+ B b(2);
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 5 }
+ ^{b.f();}();
+}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2362,7 +2362,8 @@
void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
bool ForVirtualBase, bool Delegating,
Address This, CallArgList &Args,
- AggValueSlot::Overlap_t Overlap);
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc);
/// Emit assumption load for all bases. Requires to be be called only on
/// most-derived class and not under construction of the object.
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -2031,7 +2031,7 @@
/*ParamsToSkip*/ 0, Order);
EmitCXXConstructorCall(D, Type, ForVirtualBase, Delegating, This, Args,
- Overlap);
+ Overlap, E->getExprLoc());
}
static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
@@ -2064,14 +2064,14 @@
bool Delegating,
Address This,
CallArgList &Args,
- AggValueSlot::Overlap_t Overlap) {
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc) {
const CXXRecordDecl *ClassDecl = D->getParent();
// C++11 [class.mfct.non-static]p2:
// If a non-static member function of a class X is called for an object that
// is not of type X, or of a type derived from X, the behavior is undefined.
- // FIXME: Provide a source location here.
- EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, SourceLocation(),
+ EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc,
This.getPointer(), getContext().getRecordType(ClassDecl));
if (D->isTrivial() && D->isDefaultConstructor()) {
@@ -2180,7 +2180,8 @@
}
EmitCXXConstructorCall(D, Ctor_Base, ForVirtualBase, /*Delegating*/false,
- This, Args, AggValueSlot::MayOverlap);
+ This, Args, AggValueSlot::MayOverlap,
+ E->getLocation());
}
void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
@@ -2277,7 +2278,7 @@
/*ParamsToSkip*/ 1);
EmitCXXConstructorCall(D, Ctor_Complete, false, false, This, Args,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, E->getExprLoc());
}
void
@@ -2313,7 +2314,7 @@
EmitCXXConstructorCall(Ctor, CtorType, /*ForVirtualBase=*/false,
/*Delegating=*/true, This, DelegateArgs,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, Loc);
}
namespace {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D48531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:1954
+ llvm::APInt getFixedPointMin(QualType Ty) const;
+ llvm::APInt getFixedPointOne(QualType Ty) const;
Are these opaque bit-patterns? I think there should be a type which abstracts
over constant fixed-point values, something `APSInt`-like that also carries the
signedness and scale. For now that type can live in Clang; if LLVM wants to
add intrinsic support for fixed-point, it'll be easy enough to move it there.
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding()
&&
+Ty->isUnsignedFixedPointType()) {
+ unsigned NumBits = CGF.getContext().getTypeSize(Ty);
Can you explain the padding thing? Why is padding uniformly present or absent
on all unsigned fixed point types on a target, and never on signed types? Is
this "low bits set" thing endian-specific?
Comment at: lib/CodeGen/CGExprScalar.cpp:1801
+ return Builder.CreateLShr(
+ EmitScalarConversion(Val, SrcTy, DestTy, CE->getExprLoc()), scale);
+ }
Please abstract functions for doing this kind of arithmetic instead of inlining
them into scalar emission. Feel free to make a new CGFixedPoint.h header and
corresponding implementation file.
Also, it looks like you're assuming that the output type is the same size as
the input type. I don't think that's actually a language restriction, right?
Repository:
rC Clang
https://reviews.llvm.org/D48456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL335445: [CodeGen] Provide source locations for UBSan type
checks when emitting… (authored by ikudrin, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48531?vs=152625&id=152626#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48531
Files:
cfe/trunk/lib/CodeGen/CGClass.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
Index: cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux -emit-llvm -fsanitize=alignment -fblocks %s -o %t.ll
+// RUN: FileCheck -check-prefix=ZEROINIT < %t.ll %s
+// RUN: FileCheck -check-prefix=SRCLOC < %t.ll %s
+// ZEROINIT-NOT: @{{.+}} = private unnamed_addr global {{.+}} zeroinitializer
+
+struct A {
+ A(int);
+ int k;
+};
+
+struct B : A {
+ B();
+ B(const B &);
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 12 }
+ using A::A;
+ void f() const;
+};
+
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 10 }
+B::B() : A(1) {}
+
+void foo() {
+ B b(2);
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32 [[@LINE+1]], i32 5 }
+ ^{b.f();}();
+}
Index: cfe/trunk/lib/CodeGen/CGClass.cpp
===
--- cfe/trunk/lib/CodeGen/CGClass.cpp
+++ cfe/trunk/lib/CodeGen/CGClass.cpp
@@ -2031,7 +2031,7 @@
/*ParamsToSkip*/ 0, Order);
EmitCXXConstructorCall(D, Type, ForVirtualBase, Delegating, This, Args,
- Overlap);
+ Overlap, E->getExprLoc());
}
static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
@@ -2064,14 +2064,14 @@
bool Delegating,
Address This,
CallArgList &Args,
- AggValueSlot::Overlap_t Overlap) {
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc) {
const CXXRecordDecl *ClassDecl = D->getParent();
// C++11 [class.mfct.non-static]p2:
// If a non-static member function of a class X is called for an object that
// is not of type X, or of a type derived from X, the behavior is undefined.
- // FIXME: Provide a source location here.
- EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, SourceLocation(),
+ EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc,
This.getPointer(), getContext().getRecordType(ClassDecl));
if (D->isTrivial() && D->isDefaultConstructor()) {
@@ -2180,7 +2180,8 @@
}
EmitCXXConstructorCall(D, Ctor_Base, ForVirtualBase, /*Delegating*/false,
- This, Args, AggValueSlot::MayOverlap);
+ This, Args, AggValueSlot::MayOverlap,
+ E->getLocation());
}
void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
@@ -2277,7 +2278,7 @@
/*ParamsToSkip*/ 1);
EmitCXXConstructorCall(D, Ctor_Complete, false, false, This, Args,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, E->getExprLoc());
}
void
@@ -2313,7 +2314,7 @@
EmitCXXConstructorCall(Ctor, CtorType, /*ForVirtualBase=*/false,
/*Delegating=*/true, This, DelegateArgs,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, Loc);
}
namespace {
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.h
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.h
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.h
@@ -2362,7 +2362,8 @@
void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
bool ForVirtualBase, bool Delegating,
Address This, CallArgList &Args,
- AggValueSlot::Overlap_t Overlap);
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc);
/// Emit assumption load for all bases. Requires to be be called only on
/// most-derived class and not under construction of the object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r335445 - [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.
Author: ikudrin
Date: Sun Jun 24 22:48:04 2018
New Revision: 335445
URL: http://llvm.org/viewvc/llvm-project?rev=335445&view=rev
Log:
[CodeGen] Provide source locations for UBSan type checks when emitting
constructor calls.
Differential Revision: https://reviews.llvm.org/D48531
Added:
cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
Modified:
cfe/trunk/lib/CodeGen/CGClass.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=335445&r1=335444&r2=335445&view=diff
==
--- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGClass.cpp Sun Jun 24 22:48:04 2018
@@ -2031,7 +2031,7 @@ void CodeGenFunction::EmitCXXConstructor
/*ParamsToSkip*/ 0, Order);
EmitCXXConstructorCall(D, Type, ForVirtualBase, Delegating, This, Args,
- Overlap);
+ Overlap, E->getExprLoc());
}
static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
@@ -2064,14 +2064,14 @@ void CodeGenFunction::EmitCXXConstructor
bool Delegating,
Address This,
CallArgList &Args,
- AggValueSlot::Overlap_t Overlap) {
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc) {
const CXXRecordDecl *ClassDecl = D->getParent();
// C++11 [class.mfct.non-static]p2:
// If a non-static member function of a class X is called for an object
that
// is not of type X, or of a type derived from X, the behavior is
undefined.
- // FIXME: Provide a source location here.
- EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, SourceLocation(),
+ EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc,
This.getPointer(), getContext().getRecordType(ClassDecl));
if (D->isTrivial() && D->isDefaultConstructor()) {
@@ -2180,7 +2180,8 @@ void CodeGenFunction::EmitInheritedCXXCo
}
EmitCXXConstructorCall(D, Ctor_Base, ForVirtualBase, /*Delegating*/false,
- This, Args, AggValueSlot::MayOverlap);
+ This, Args, AggValueSlot::MayOverlap,
+ E->getLocation());
}
void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
@@ -2277,7 +2278,7 @@ CodeGenFunction::EmitSynthesizedCXXCopyC
/*ParamsToSkip*/ 1);
EmitCXXConstructorCall(D, Ctor_Complete, false, false, This, Args,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, E->getExprLoc());
}
void
@@ -2313,7 +2314,7 @@ CodeGenFunction::EmitDelegateCXXConstruc
EmitCXXConstructorCall(Ctor, CtorType, /*ForVirtualBase=*/false,
/*Delegating=*/true, This, DelegateArgs,
- AggValueSlot::MayOverlap);
+ AggValueSlot::MayOverlap, Loc);
}
namespace {
Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=335445&r1=335444&r2=335445&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Sun Jun 24 22:48:04 2018
@@ -2362,7 +2362,8 @@ public:
void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
bool ForVirtualBase, bool Delegating,
Address This, CallArgList &Args,
- AggValueSlot::Overlap_t Overlap);
+ AggValueSlot::Overlap_t Overlap,
+ SourceLocation Loc);
/// Emit assumption load for all bases. Requires to be be called only on
/// most-derived class and not under construction of the object.
Added: cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp?rev=335445&view=auto
==
--- cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/ubsan-ctor-srcloc.cpp Sun Jun 24 22:48:04 2018
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux -emit-llvm
-fsanitize=alignment -fblocks %s -o %t.ll
+// RUN: FileCheck -check-prefix=ZEROINIT < %t.ll %s
+// RUN: FileCheck -check-prefix=SRCLOC < %t.ll %s
+// ZEROINIT-NOT: @{{.+}} = private unnamed_addr global {{.+}} zeroinitializer
+
+struct A {
+ A(int);
+ int k;
+};
+
+struct B : A {
+ B();
+ B(const B &);
+// SRCLOC-DAG: @{{.+}} = private unnamed_addr global {{.+}} @.src, i32
[[@
