Anna Zaks [Thu, 7 Mar 2013 03:02:36 +0000 (03:02 +0000)]
[analyzer] Warn on passing a reference to null pointer as an argument in a call
Warn about null pointer dereference earlier when a reference to a null pointer is
passed in a call. The idea is that even though the standard might allow this, reporting
the issue earlier is better for diagnostics (the error is reported closer to the place where
the pointer was set to NULL). This also simplifies analyzer’s diagnostic logic, which has
to track “where the null came from”. As a consequence, some of our null pointer
warning suppression mechanisms started triggering more often.
TODO: Change the name of the file and class to reflect the new check.
Add a hasExternalStorageAsWritten helper. No functionality change.
It is possible that some of the current uses of
"getStorageClassAsWritten() == SC_Extern" should use this but I don't know
enough about SC_PrivateExtern to change and test them.
Jordan Rose [Thu, 7 Mar 2013 01:23:25 +0000 (01:23 +0000)]
[analyzer] Check for returning null references in ReturnUndefChecker.
Officially in the C++ standard, a null reference cannot exist. However,
it's still very easy to create one:
int &getNullRef() {
int *p = 0;
return *p;
}
We already check that binds to reference regions don't create null references.
This patch checks that we don't create null references by returning, either.
Anna Zaks [Wed, 6 Mar 2013 20:25:59 +0000 (20:25 +0000)]
[analyzer] IDC: Add config option; perform the idc check on first “null node” rather than last “non-null”.
The second modification does not lead to any visible result, but, theoretically, is what we should
have been looking at to begin with since we are checking if the node was assumed to be null in
an inlined function.
[PCH] When pre-validating the headers from the PCH, only validate non-system headers.
Stat'ing all the headers from the PCH to make sure they are up-to-date takes significant time.
In a particular source file (whose PCH file included Cocoa.h) from total -fsyntax-only time
12% was just stat calls. Change pre-validation to only check non-system headers.
There are some notable disadvantages:
-If a system header, that is not include-guarded, changes after the PCH was created, we will not
find it in the header info table and we will #import it, effectively #importing it twice, thus
we will emit some error due to a multiple definition and after that the "header was modified" error will likely
be emitted, for example something like:
NSDictionary.h:12:1: error: duplicate interface definition for class 'NSDictionary'
@interface NSDictionary : NSObject <NSCopying, NSMutableCopying, NSSecureCoding, NSFastEnumeration>
^
NSDictionary.h:12:12: note: previous definition is here
@interface NSDictionary : NSObject <NSCopying, NSMutableCopying, NSSecureCoding, NSFastEnumeration>
^
fatal error: file 'NSDictionary.h' has been modified since the precompiled header was built
Though we get the "header was modified" error, this is a bit confusing.
-Theoretically it is possible that such a system header will cause no errors but it will just cause an
unfortunate semantic change, though I find this rather unlikely.
The advantages:
-Reduces compilation time when using a huge PCH like the Cocoa ones
-System headers change very infrequent and when they do, users/build systems should be able to know that
re-building from scratch is needed.
[PCH] For HeaderFileInfoTrait, hash the key using size & time of the file.
Previously the hash would be the filename portion of the path, which could be
different for a filename with different case or a symbolic link with a different
name completely.
This did not actually create any issue so far because by validating all headers
in the PCH we created uniqued FileEntries based on inodes, so an #include of
a symbolic link (refering to a file from the PCH) would end up with a FileEntry
with filename same as the one recorded in the PCH.
Edwin Vane [Wed, 6 Mar 2013 17:02:57 +0000 (17:02 +0000)]
New ASTMatchers and enhancement to hasOverloadedOperatorName
Added two new narrowing matchers:
* hasMethod: aplies a matcher to a CXXRecordDecl's methods until a match is made
or there are no more methods.
* hasCanonicalType: applies a matcher to a QualType's canonicalType.
Enhanced hasOverloadedOperatorName to work on CXXMethodDecl as well as
CXXOperatorCallExpr.
Richard Smith [Wed, 6 Mar 2013 01:37:38 +0000 (01:37 +0000)]
PR15390: If a function returns a pointer to a function, that function type
can't have default arguments even though it's a parameter-declaration-clause in
a function declaration.
Jordan Rose [Tue, 5 Mar 2013 23:54:55 +0000 (23:54 +0000)]
Preprocessor: don't keep comments under -traditional-cpp.
This patch is designed for minimal intrusion into normal preprocessing
and compilation; under -E -traditional-cpp, the lexer will still
generate tok::comment nodes since it is preserving all whitespace, but
the output printer will then throw it away.
Jordan Rose [Tue, 5 Mar 2013 22:51:04 +0000 (22:51 +0000)]
Preprocessor: don't consider // to be a line comment in -E -std=c89 mode.
It's beneficial when compiling to treat // as the start of a line
comment even in -std=c89 mode, since it's not valid C code (with a few
rare exceptions) and is usually intended as such. We emit a pedantic
warning and then continue on as if line comments were enabled.
This has been our behavior for quite some time.
However, people use the preprocessor for things besides C source files.
In today's prompting example, the input contains (unquoted) URLs, which
contain // but should still be preserved.
This change instructs the lexer to treat // as a plain token if Clang is
in C90 mode and generating preprocessed output rather than actually compiling.
Lang Hames [Tue, 5 Mar 2013 20:27:24 +0000 (20:27 +0000)]
Use ASTContext::getDeclAlign(<Field Decl>) to get the alignment of the first
field to be memcpy'd, rather instead of ASTContext::getTypeAlign(<Field Type>).
For packed structs the alignment of a field may be less than the alignment of
the field's type.
Richard Smith [Tue, 5 Mar 2013 08:30:04 +0000 (08:30 +0000)]
Don't emit calls to virtual [[noreturn]] functions as noreturn; overrides of a
[[noreturn]] function are not required to also be [[noreturn]]. We still emit
calls to virtual __attribute__((noreturn)) functions as noreturn; unlike GCC,
we do require overriders to also be noreturn for that attribute.
Jordan Rose [Mon, 4 Mar 2013 23:21:32 +0000 (23:21 +0000)]
[analyzer] Don't let cf_audited_transfer override CFRetain semantics.
We weren't treating a cf_audited_transfer CFRetain as returning +1 because
its name doesn't contain "Create" or "Copy". Oops! Fortunately, the
standard definitions of these functions are not marked audited.
Dmitri Gribenko [Mon, 4 Mar 2013 23:06:15 +0000 (23:06 +0000)]
Comment parsing: refactor handling of command markers in AST
* Use the term 'command marker', because the semantics of 'backslash' and 'at'
commands are the same. (Talking about 'at commands' makes them look like a
special entity.)
* Sink the flag down into bitfields, reducing the size of AST nodes.
* Change the flag into an enum for clarity. Boolean function parameters are
not very clear.
Bob Wilson [Mon, 4 Mar 2013 22:37:43 +0000 (22:37 +0000)]
Fix confused use of llvm::StringSwitch for armv7r architecture.
svn 170909 added support for cortex-r5 but in this case it was done
incorrectly. The last argument to StringSwitch.Cases() is the replacement
value, so by adding "cortex-r5" it changed the default cpu for armv7r to
cortex-r5 instead of cortex-r4.
[PCH] In HeaderFileInfoTrait::EqualKey(), use FileManager::getFile() to compare two filenames, instead of llvm::sys::fs::equivalent().
llvm::sys::fs::equivalent() does 2 stat calls every time it's called. Use FileManager::getFile() to take advantage
of the stat caching that FileManager is providing.
Check for warnings in a bunch of the linker invocations, and add one
with both -static-libgcc and -static on the commandline.
Fix a warning in the latter case due to a backwards short circuiting ||
operator in the driver. No real functionality changed here, just allows
the driver to properly consume -static-libgcc when -static is also
specified.
David Chisnall [Sun, 3 Mar 2013 16:02:42 +0000 (16:02 +0000)]
Improve C11 atomics support:
- Generate atomicrmw operations in most of the cases when it's sensible to do
so.
- Don't crash in several common cases (and hopefully don't crash in more of
them).
- Add some better tests.
We now generate significantly better code for things like:
_Atomic(int) x;
...
x++;
On MIPS, this now generates a 4-instruction ll/sc loop, where previously it
generated about 30 instructions in two nested loops. On x86-64, we generate a
single lock incl, instead of a lock cmpxchgl loop (one instruction instead of
ten).
Inlining brought a few "null pointer use" false positives, which occur because
the callee defensively checks if a pointer is NULL, whereas the caller knows
that the pointer cannot be NULL in the context of the given call.
This is a first attempt to silence these warnings by tracking the symbolic value
along the execution path in the BugReporter. The new visitor finds the node
in which the symbol was first constrained to NULL. If the node belongs to
a function on the active stack, the warning is reported, otherwise, it is
suppressed.
There are several areas for follow up work, for example:
- How do we differentiate the cases where the first check is followed by
another one, which does happen on the active stack?
Also, this only silences a fraction of null pointer use warnings. For example, it
does not do anything for the cases where NULL was assigned inside a callee.
Jordan Rose [Sat, 2 Mar 2013 00:49:47 +0000 (00:49 +0000)]
CMake: -Wno-nested-anon-types for Clang.
In LLVM, -pedantic is not set unless LLVM_ENABLE_PEDANTIC is set.
However, Clang's CMakeLists.txt unilaterally adds -pedantic to the run
line, so we need to disable -Wnested-anon-types explicitly.
Jordan Rose [Fri, 1 Mar 2013 23:03:17 +0000 (23:03 +0000)]
[analyzer] Special-case bitfields when finding sub-region bindings.
Previously we were assuming that we'd never ask for the sub-region bindings
of a bitfield, since a bitfield cannot have subregions. However,
unification of code paths has made that assumption invalid. While we could
take advantage of this by just checking for the single possible binding,
it's probably better to do the right thing, so that if/when we someday
support unions we'll do the right thing there, too.
This fixes a handful of false positives in analyzing LLVM.
comment parsing. Keep the original command format
in AST for source fidelity and use it in diagnostics
to refer to the original format. // rdar://13066276
Jordan Rose [Fri, 1 Mar 2013 19:45:10 +0000 (19:45 +0000)]
[analyzer] Suppress paths involving a reference whose rvalue is null.
Most map types have an operator[] that inserts a new element if the key
isn't found, then returns a reference to the value slot so that you can
assign into it. However, if the value type is a pointer, it will be
initialized to null. This is usually no problem.
However, if the user /knows/ the map contains a value for a particular key,
they may just use it immediately:
// From ClangSACheckersEmitter.cpp
recordGroupMap[group]->Checkers
In this case the analyzer reports a null dereference on the path where the
key is not in the map, even though the user knows that path is impossible
here. They could silence the warning by adding an assertion, but that means
splitting up the expression and introducing a local variable. (Note that
the analyzer has no way of knowing that recordGroupMap[group] will return
the same reference if called twice in a row!)
We already have logic that says a null dereference has a high chance of
being a false positive if the null came from an inlined function. This
patch simply extends that to references whose rvalues are null as well,
silencing several false positives in LLVM.
Daniel Jasper [Fri, 1 Mar 2013 16:48:32 +0000 (16:48 +0000)]
Normal indent for last element of builder-type call.
In builder type call, we indent to the laster function calls.
However, for the last element of such a call, we don't need to do
so, as that normally just wastes space and does not increase
readability.
John McCall [Fri, 1 Mar 2013 09:20:14 +0000 (09:20 +0000)]
Perform the receiver-expression transformations regardless of
whether we already have a method. Fixes a bug where we were
failing to properly contextually convert a message receiver
during template instantiation.
As a side-effect, we now actually perform correct method lookup
after adjusting a message-send to integral or non-ObjC pointer
types (legal outside of ARC).
Anna Zaks [Fri, 1 Mar 2013 06:38:16 +0000 (06:38 +0000)]
[analyzer] Reword FAQ
Reword the FAQ to stress more that the assert should be used only in case
the developer is sure that the issue is a false positive.
[PCH] Enhance InputFile to also include whether the file is out-of-date.
Previously we would return null for an out-of-date file. This inhibited ASTReader::ReadSLocEntry
from creating a FileID to recover gracefully in such a case.
projects / clang / log