Douglas Gregor [Tue, 19 Mar 2013 00:28:20 +0000 (00:28 +0000)]
<rdar://problem/13363214> Eliminate race condition between module rebuild and the global module index.
The global module index was querying the file manager for each of the
module files it knows about at load time, to prune out any out-of-date
information. The file manager would then cache the results of the
stat() falls used to find that module file.
Later, the same translation unit could end up trying to import one of the
module files that had previously been ignored by the module cache, but
after some other Clang instance rebuilt the module file to bring it
up-to-date. The stale stat() results in the file manager would
trigger a second rebuild of the already-up-to-date module, causing
failures down the line.
The global module index now lazily resolves its module file references
to actual AST reader module files only after the module file has been
loaded, eliminating the stat-caching race. Moreover, the AST reader
can communicate to its caller that a module file is missing (rather
than simply being out-of-date), allowing us to simplify the
module-loading logic and allowing the compiler to recover if a
dependent module file ends up getting deleted.
Richard Smith [Tue, 19 Mar 2013 00:01:12 +0000 (00:01 +0000)]
PR15383: When -fsanitize=float-cast-overflow checks a float-to-int conversion,
it wasn't taking into account that the float should be truncated *before* the
range check happens. Thus (unsigned)-0.99 and (unsigned char)255.9 have defined
behavior and should not be trapped.
Jordan Rose [Mon, 18 Mar 2013 23:34:37 +0000 (23:34 +0000)]
[analyzer] Do part of the work to find shortest bug paths up front.
Splitting the graph trimming and the path-finding (r177216) already
recovered quite a bit of performance lost to increased suppression.
We can still do better by also performing the reverse BFS up front
(needed for shortest-path-finding) and only walking the shortest path
for each report. This does mean we have to walk back up the path and
invalidate all the BFS numbers if the report turns out to be invalid,
but it's probably still faster than redoing the full BFS every time.
More performance work for <rdar://problem/13433687>
Richard Smith [Mon, 18 Mar 2013 22:52:47 +0000 (22:52 +0000)]
Add missing diagnostic for a nested-name-specifier on a free-standing type definition. Bump some related diagnostics from warning to extension in C++, since they're errors there. Add some missing checks for function specifiers on non-function declarations.
Reed Kotler [Mon, 18 Mar 2013 22:18:00 +0000 (22:18 +0000)]
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
Richard Smith [Mon, 18 Mar 2013 21:44:56 +0000 (21:44 +0000)]
C++11 status: mark 'extended integral types' as N/A, since we don't support
any, and add a note that we cannot treat __int128 as an extended integral type.
Objective-C modern translator. Don't put line info
into the pre-preprocessed file to be passed to
modern translator when compiling in no debug mode.
// rdar://13138170
David Blaikie [Sun, 17 Mar 2013 20:29:22 +0000 (20:29 +0000)]
Generalize a few debug info test cases
Checking for the annotation comment rather than the metadata values makes these
tests resilient to a coming refactor that will pull these fields out into a
separate metadata node.
Jordan Rose [Sat, 16 Mar 2013 02:14:06 +0000 (02:14 +0000)]
[analyzer] Model trivial copy/move assignment operators with a bind as well.
r175234 allowed the analyzer to model trivial copy/move constructors as
an aggregate bind. This commit extends that to trivial assignment
operators as well. Like the last commit, one of the motivating factors here
is not warning when the right-hand object is partially-initialized, which
can have legitimate uses.
Jordan Rose [Sat, 16 Mar 2013 01:07:58 +0000 (01:07 +0000)]
[analyzer] Separate graph trimming from creating the single-path graph.
When we generate a path diagnostic for a bug report, we have to take the
full ExplodedGraph and limit it down to a single path. We do this in two
steps: "trimming", which limits the graph to all the paths that lead to
this particular bug, and "creating the report graph", which finds the
shortest path in the trimmed path to any error node.
With BugReporterVisitor false positive suppression, this becomes more
expensive: it's possible for some paths through the trimmed graph to be
invalid (i.e. likely false positives) but others to be valid. Therefore
we have to run the visitors over each path in the graph until we find one
that is valid, or until we've ruled them all out. This can become quite
expensive.
This commit separates out graph trimming from creating the report graph,
performing the first only once per bug equivalence class and the second
once per bug report. It also cleans up that portion of the code by
introducing some wrapper classes.
This seems to recover most of the performance regression described in my
last commit.
Jordan Rose [Sat, 16 Mar 2013 01:07:47 +0000 (01:07 +0000)]
[analyzer] Don't repeat a bug equivalence class if every report is invalid.
I removed this check in the recursion->iteration commit, but forgot that
generatePathDiagnostic may be called multiple times if there are multiple
PathDiagnosticConsumers.
Manman Ren [Sat, 16 Mar 2013 00:11:09 +0000 (00:11 +0000)]
Exploit this-return of a callsite in a this-return function.
For constructors/desctructors that return 'this', if there exists a callsite
that returns 'this' and is immediately before the return instruction, make
sure we are using the return value from the callsite.
We don't need to keep 'this' alive through the callsite. It also enables
optimizations in the backend, such as tail call optimization.
Richard Trieu [Fri, 15 Mar 2013 23:55:09 +0000 (23:55 +0000)]
Improve template diffing handling of default integer values.
When the template argument is both default and value dependent, the expression
retrieved for the default argument cannot be evaluated, thus never matching
any argument value. To get the proper value, get the template argument
from the desugared template specialization. Also, output the original
expression to provide more information about the argument mismatch.
Anna Zaks [Fri, 15 Mar 2013 23:34:29 +0000 (23:34 +0000)]
[analyzer] Use isLiveRegion to determine when SymbolRegionValue is dead.
Fixes a FIXME, improves dead symbol collection, suppresses a false positive,
which resulted from reusing the same symbol twice for simulation of 2 calls to the same function.
Fixing this lead to 2 possible false negatives in CString checker. Since the checker is still alpha and
the solution will not require revert of this commit, move the tests to a FIXME section.
Ted Kremenek [Fri, 15 Mar 2013 23:09:37 +0000 (23:09 +0000)]
Fix buffer underrun (invalid read) triggered during diagnostic rendering. The test would overflow when computing '0 - 1'.
I don't have a good testcase for this that does not depend on system headers.
It did not trigger with preprocessed output, and I had trouble reducing the example.
Douglas Gregor [Fri, 15 Mar 2013 22:15:07 +0000 (22:15 +0000)]
<rdar://problem/13426257> Introduce SDKSettings.plist as an input file dependency for PCH/modules.
When we're building a precompiled header or module against an SDK on
Darwin, there will be a file SDKSettings.plist in the sysroot. Since
stat()'ing every system header on which a module or PCH file depends
is performance suicide, we instead stat() just SDKSettings.plist. This
hack works well on Darwin; it's unclear how we want to handle this on
other platforms. If there is a canonical file, we should use it; if
not, we either have to take the performance hit of stat()'ing system
headers repeatedly or roll the dice by not checking anything.
Jordan Rose [Fri, 15 Mar 2013 21:41:55 +0000 (21:41 +0000)]
[analyzer] Make GRBugReporter::generatePathDiagnostic iterative, not recursive.
The previous generatePathDiagnostic() was intended to be tail-recursive,
restarting and trying again if a report was marked invalid. However:
(1) this leaked all the cloned visitors, which weren't being deleted, and
(2) this wasn't actually tail-recursive because some local variables had
non-trivial destructors.
This was causing us to overflow the stack on inputs with large numbers of
reports in the same equivalence class, such as sqlite3.c. Being iterative
at least prevents us from blowing out the stack, but doesn't solve the
performance issue: suppressing thousands (yes, thousands) of paths in the
same equivalence class is expensive. I'm looking into that now.
Jordan Rose [Fri, 15 Mar 2013 21:41:53 +0000 (21:41 +0000)]
[analyzer] Collect stats on the max # of bug reports in an equivalence class.
We discovered that sqlite3.c currently has 2600 reports in a single
equivalence class; it would be good to know if this is a recent
development or what.
(For the curious, the different reports in an equivalence class represent
the same bug found along different paths. When we're suppressing false
positives, we need to go through /every/ path to make sure there isn't a
valid path to a bug. This is a flaw in our after-the-fact suppression,
made worse by the fact that that function isn't particularly optimized.)
Richard Trieu [Fri, 15 Mar 2013 20:35:18 +0000 (20:35 +0000)]
Refactor template diffing to store an enum that records which type of
difference is stored inside a DiffNode. This should not change any
diagnostic messages.
Adrian Prantl [Fri, 15 Mar 2013 17:09:05 +0000 (17:09 +0000)]
Force column info only for direct inlined functions. This should strike
the balance between expected behavior and compatibility with the gdb
testsuite.
(GDB gets confused if we break an expression into multiple debug
stmts so we enable this behavior only for inlined functions. For the
full experience people can still use -gcolumn-info.)
Sylvestre Ledru [Fri, 15 Mar 2013 16:22:43 +0000 (16:22 +0000)]
Take in account the triplet 'powerpc-linux-gnuspe' for PowerPC SPE. Done for the port of Debian on this arch. More information on: http://wiki.debian.org/PowerPCSPEPort Patch by Roland Stigge
Nico Weber [Fri, 15 Mar 2013 15:02:37 +0000 (15:02 +0000)]
Remove a pointless assertion.
FindNodeOrInsertPos() is called 10 lines earlier already, and the function
early-returns there if the result is != 0. InsertPos isn't recomputed after
that check, so this assert is always trivially true. (And it has nothing to
do with if T is canonical or not.)
Richard Smith [Fri, 15 Mar 2013 00:41:52 +0000 (00:41 +0000)]
PR15290: 'this' is not permitted in the declaration of a friend function,
therefore references to members should not be transformed into implicit uses of
'this'. Patch by Ismail Pazarbasi!
Don't try to typo-correct 'super' in an objc method.
This created 2 issues:
1) Performance issue, since typo-correction with PCH/modules is rather expensive.
2) Correctness issue, since if it managed to "correct" 'super' then bogus compiler errors would
be emitted, like this:
3.m:8:3: error: unknown type name 'super'; did you mean 'super1'?
super.x = 0;
^~~~~
super1
t3.m:5:13: note: 'super1' declared here
typedef int super1;
^
t3.m:8:8: error: expected identifier or '('
super.x = 0;
^
Anna Zaks [Thu, 14 Mar 2013 22:31:56 +0000 (22:31 +0000)]
[analyzer] Change the way in which IDC Visitor decides to kick in and make sure it attaches in the given edge case
In the test case below, the value V is not constrained to 0 in ErrorNode but it is in node N.
So we used to fail to register the Suppression visitor.
We also need to change the way we determine that the Visitor should kick in because the node N belongs to
the ExplodedGraph and might not be on the BugReporter path that the visitor sees. Instead of trying to match the node,
turn on the visitor when we see the last node in which the symbol is ‘0’.
Adrian Prantl [Thu, 14 Mar 2013 17:53:33 +0000 (17:53 +0000)]
Allocate stack storage for .block_descriptor and captured self at -O0.
This way the register allocator will not optimize away the debug info
for captured variables.
Manuel Klimek [Thu, 14 Mar 2013 16:33:21 +0000 (16:33 +0000)]
Implements memoization for ancestor matching.
This yields a log(#ast_nodes) worst-case improvement with matchers like
stmt(unless(hasAncestor(...))).
Also made the order of visitation for ancestor matches BFS, as the most
common use cases (for example finding the closest enclosing function
definition) rely on that.
Chandler Carruth [Thu, 14 Mar 2013 11:17:20 +0000 (11:17 +0000)]
Fix an unused variable warning from Clang by sinking a dyn_cast into an
isa and a cast inside the assert. The efficiency concern isn't really
important here. The code should likely be cleaned up a bit more,
especially getting a message into the assert.
John McCall [Thu, 14 Mar 2013 05:13:41 +0000 (05:13 +0000)]
Flag that friend function definitions are "late parsed" so that
template instantiation will still consider them to be definitions
if we instantiate the containing class before we get around
to parsing the friend.
This seems like a legitimate use of "late template parsed" to me,
but I'd appreciate it if someone responsible for the MS feature
would look over this.
This file already appears to access AST nodes directly, which
is arguably not kosher in the parser, but the performance of this
path matters enough that perpetuating the sin is justifiable.
Probably we ought to reconsider this policy for very simple
manipulations like this.
The reason this entire thing is necessary is that
function template instantiation plays some very gross games
in order to not associate an instantiated function template
with the class it came from unless it's a definition, and
the reason *that's* necessary is that the AST currently
cannot represent the instantiation history of individual
function template declarations, but instead tracks it in
common for the entire function template. That probably
prevents us from correctly reporting ill-formed calls to
ambiguously instantiated friend function templates.
Rafael Espindola [Thu, 14 Mar 2013 03:07:35 +0000 (03:07 +0000)]
Avoid computing the linkage too early. Don't invalidate it.
Before this patch we would compute the linkage lazily and cache it. When the
AST was modified in ways that could change the value, we would invalidate the
cache.
That was fairly brittle, since any code could ask for the a linkage before
the correct value was available.
We should change the API to one where the linkage is computed explicitly and
trying to get it when it is not available asserts.
This patch is a first step in that direction. We still compute the linkage
lazily, but instead of invalidating a cache, we assert that the AST
modifications didn't change the result.
projects / clang / log