Ben Hamilton [Mon, 5 Nov 2018 16:59:33 +0000 (16:59 +0000)]
[Format] Add debugging to ObjC language guesser
Summary:
To handle diagnosing bugs where ObjCHeaderStyleGuesser guesses
wrong, this diff adds a bit more debug logging to the Objective-C
language guesser.
Benjamin Kramer [Mon, 5 Nov 2018 12:46:02 +0000 (12:46 +0000)]
Reapply "Fix regression in behavior of clang -x c++-header -fmodule-name=XXX"
This reverts commit r345963. We have a path forward now.
Original commit message:
The driver accidentally stopped passing the input filenames on to -cc1
in this mode due to confusion over what action was being requested.
This change also fixes a couple of crashes I encountered when passing
multiple files to such a -cc1 invocation.
Kristof Umann [Mon, 5 Nov 2018 03:50:37 +0000 (03:50 +0000)]
[analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered
One of the reasons why AnalyzerOptions is so chaotic is that options can be
retrieved from the command line whenever and wherever. This allowed for some
options to be forgotten for a looooooong time. Have you ever heard of
"region-store-small-struct-limit"? In order to prevent this in the future, I'm
proposing to restrict AnalyzerOptions' interface so that only checker options
can be retrieved without special getters. I would like to make every option be
accessible only through a getter, but checkers from plugins are a thing, so I'll
have to figure something out for that.
This also forces developers who'd like to add a new option to register it
properly in the .def file.
This is done by
* making the third checker pointer parameter non-optional, and checked by an
assert to be non-null.
* I added new, but private non-checkers option initializers, meant only for
internal use,
* Renamed these methods accordingly (mind the consistent name for once with
getBooleanOption!):
- getOptionAsString -> getCheckerStringOption,
- getOptionAsInteger -> getCheckerIntegerOption
* The 3 functions meant for initializing data members (with the not very
descriptive getBooleanOption, getOptionAsString and getOptionAsUInt names)
were renamed to be overloads of the getAndInitOption function name.
* All options were in some way retrieved via getCheckerOption. I removed it, and
moved the logic to getStringOption and getCheckerStringOption. This did cause
some code duplication, but that's the only way I could do it, now that checker
and non-checker options are separated. Note that the non-checker version
inserts the new option to the ConfigTable with the default value, but the
checker version only attempts to find already existing entries. This is how
it always worked, but this is clunky and I might end reworking that too, so we
can eventually get a ConfigTable that contains the entire configuration of the
analyzer.
Kristof Umann [Mon, 5 Nov 2018 02:37:29 +0000 (02:37 +0000)]
Ensure the correct order of evaluation in part 2. of PlistMacroExpansion
Windows buildbots break with the previous commit '[analyzer][PlistMacroExpansion]
Part 2.: Retrieving the macro name and primitive expansion'. This patch attempts
to solve this issue.
Kristof Umann [Sun, 4 Nov 2018 13:59:44 +0000 (13:59 +0000)]
[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
This patch adds a couple new functions to acquire the macro's name, and also
expands it, although it doesn't expand the arguments, as seen from the test files
Brian Gesiak [Sat, 3 Nov 2018 22:35:17 +0000 (22:35 +0000)]
[coroutines] Fix fallthrough warning on try/catch
Summary:
The test case added in this diff would incorrectly warn that control
flow may fall through without returning. Here's a standalone example:
https://godbolt.org/z/dCwXEi
The same program, but using `return` instead of `co_return`, does not
produce a warning: https://godbolt.org/z/mVldqQ
The issue was in how Clang analysis would structure its representation
of the control-flow graph. Specifically, when constructing the CFG,
`CFGBuilder::Visit` had special handling of a `ReturnStmt`, in which it
would place object destructors in the same CFG block as a `return` statement,
immediately after it. Doing so would allow the logic in
`lib/Sema/AnalysisBasedWarning.cpp` `CheckFallThrough` to work properly in the
program that used `return`, correctly determining that no "plain edges" preceded
the exit block of the function.
Because a `co_return` statement would not enjoy the same treatment when
it was being built into the control-flow graph, object destructors
would not be placed in the same CFG block as the `co_return`, thus
resulting in a "plain edge" preceding the exit block of the function,
and so the warning logic would be triggered.
Add special casing for `co_return` to Clang analysis, thereby
remedying the mistaken warning.
Douglas Yung [Sat, 3 Nov 2018 08:51:27 +0000 (08:51 +0000)]
Fix test on Windows.
This test checks the entire output of a help option, the problem
is that on Windows, the line break occurs in a different place
causing the CHECK to fail because it is not expecting a line break.
Takuto Ikuta [Sat, 3 Nov 2018 06:45:00 +0000 (06:45 +0000)]
Add /Zc:DllexportInlines option to clang-cl
Summary:
This CL adds /Zc:DllexportInlines flag to clang-cl.
When Zc:DllexportInlines- is specified, inline class member function is not exported if the function does not have local static variables.
By not exporting inline function, code for those functions are not generated and that reduces both compile time and obj size. Also this flag does not import inline functions from dllimported class if the function does not have local static variables.
On my 24C48T windows10 machine, build performance of chrome target in chromium repository is like below.
These stats are come with 'target_cpu="x86" enable_nacl = false is_component_build=true dcheck_always_on=true` build config and applied
* https://chromium-review.googlesource.com/c/chromium/src/+/1212379
* https://chromium-review.googlesource.com/c/v8/v8/+/1186017
| config | build time | speedup | build dir size |
| with patch, PCH on, debug | 1h10m0s | x1.13 | 35.6GB |
| without patch, PCH on, debug | 1h19m17s | | 49.0GB |
| with patch, PCH off, debug | 1h15m45s | x1.16 | 33.7GB |
| without patch, PCH off, debug | 1h28m10s | | 52.3GB |
| with patch, PCH on, release | 1h13m13s | x1.22 | 26.2GB |
| without patch, PCH on, release | 1h29m57s | | 37.5GB |
| with patch, PCH off, release | 1h23m38s | x1.32 | 23.7GB |
| without patch, PCH off, release | 1h50m50s | | 38.7GB |
This patch reduced obj size and the number of exported symbols largely, that improved link time too.
e.g. link time stats of blink_core.dll become like below
| | cold disk cache | warm disk cache |
| with patch, PCH on, debug | 71s | 30s |
| without patch, PCH on, debug | 111s | 48s |
This patch's implementation is based on Nico Weber's patch. I modified to support static local variable, added tests and took stats.
Bug: https://bugs.llvm.org/show_bug.cgi?id=33628
Reviewers: hans, thakis, rnk, javed.absar
Reviewed By: hans
Subscribers: kristof.beyls, smeenai, dschuff, probinson, cfe-commits, eraman
Richard Smith [Sat, 3 Nov 2018 02:23:33 +0000 (02:23 +0000)]
Revert r345562: "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type"
This exposes a (known) CodeGen bug: it can't cope with emitting lvalue
expressions that denote non-odr-used but usable-in-constant-expression
variables. See PR39528 for a testcase.
Petr Hosek [Fri, 2 Nov 2018 23:49:36 +0000 (23:49 +0000)]
[CMake][Fuchsia] Set -fuse-ld=lld explicitly for Linux runtimes
We set lld as the default linker on non-Darwin platforms, but we still
need to set -fuse-ld=lld explicitly in to support cross-compiling Linux
runtimes on Darwin.
Kristof Umann [Fri, 2 Nov 2018 19:48:56 +0000 (19:48 +0000)]
Attempt to fix 'logical operation on address of string constant'
Caused a lot of warnings for Windows:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/21178/steps/build/logs/warnings%20%2867%29
Kristof Umann [Fri, 2 Nov 2018 16:02:10 +0000 (16:02 +0000)]
[analyzer] Put llvm.Conventions back in alpha
Interestingly, this many year old (when I last looked I remember 2010ish)
checker was committed without any tests, so I thought I'd implement them, but I
was shocked to see how I barely managed to get it working. The code is severely
outdated, I'm not even sure it has ever been used, so I'd propose to move it
back into alpha, and possibly even remove it.
Kristof Umann [Fri, 2 Nov 2018 15:50:44 +0000 (15:50 +0000)]
[analyzer][NFC] Collect all -analyzer-config options in a .def file
I'm in the process of refactoring AnalyzerOptions. The main motivation behind
here is to emit warnings if an invalid -analyzer-config option is given from the
command line, and be able to list them all.
In this patch, I'm moving all analyzer options to a def file, and move 2 enums
to global namespace.
Kristof Umann [Fri, 2 Nov 2018 15:48:10 +0000 (15:48 +0000)]
[analyzer][NFC] Fix some incorrect uses of -analyzer-config options
I'm in the process of refactoring AnalyzerOptions. The main motivation behind
here is to emit warnings if an invalid -analyzer-config option is given from
the command line, and be able to list them all.
In this patch, I found some flags that should've been used as checker options,
or have absolutely no mention of in AnalyzerOptions, or are nonexistent.
- NonLocalizedStringChecker now uses its "AggressiveReport" flag as a checker
option
- lib/StaticAnalyzer/Frontend/ModelInjector.cpp now accesses the "model-path"
option through a getter in AnalyzerOptions
- -analyzer-config path-diagnostics-alternate=false is not a thing, I removed it,
- lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and
lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually
only contain an option getter. I deleted them, and fixed RetainCountChecker
to get it's "leak-diagnostics-reference-allocation" option as a checker option,
- "region-store-small-struct-limit" has a proper getter now.
Alexey Bataev [Fri, 2 Nov 2018 14:54:07 +0000 (14:54 +0000)]
[OPENMP][NVPTX]Improve emission of the globalized variables for
target/teams/distribute regions.
Target/teams/distribute regions exist for all the time the kernel is
executed. Thus, if the variable is declared in their context and then
escape it, we can allocate global memory statically instead of
allocating it dynamically.
Patch captures all the globalized variables in target/teams/distribute
contexts, merges them into the records, one per each target region.
Those records are then joined into the union, one per compilation unit
(to save the global memory). Those units are organized into
2 x dimensional arrays, where the first dimension is
the number of blocks per SM and the second one is the number of SMs.
Runtime functions manage this global memory space between the executing
teams.
Tim Northover [Fri, 2 Nov 2018 13:14:11 +0000 (13:14 +0000)]
Reapply Logging: make os_log buffer size an integer constant expression.
The size of an os_log buffer is known at any stage of compilation, so making it
a constant expression means that the common idiom of declaring a buffer for it
won't result in a VLA. That allows the compiler to skip saving and restoring
the stack pointer around such buffers.
This also moves the OSLog and other FormatString helpers from
libclangAnalysis to libclangAST to avoid a circular dependency.
Patrick Lyster [Fri, 2 Nov 2018 12:18:11 +0000 (12:18 +0000)]
Add support for 'atomic_default_mem_order' clause on 'requires' directive. Also renamed test files relating to 'requires'. Differntial review: https://reviews.llvm.org/D53513
Ilya Biryukov [Fri, 2 Nov 2018 10:50:26 +0000 (10:50 +0000)]
Revert "Fix regression in behavior of clang -x c++-header -fmodule-name=XXX"
This reverts commit r345803 and r345915 (a follow-up fix to r345803).
Reason: r345803 blocks our internal integrate because of the new
warnings showing up in too many places. The fix is actually correct,
we will reland it after figuring out how to integrate properly.
[CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC.
Failed assertion is
> Assertion failed: ((ND->isUsed(false) || !isa<VarDecl>(ND) || !E->getLocation().isValid()) && "Should not use decl without marking it used!"), function EmitDeclRefLValue, file llvm-project/clang/lib/CodeGen/CGExpr.cpp, line 2437.
`EmitDeclRefLValue` mentions
> // A DeclRefExpr for a reference initialized by a constant expression can
> // appear without being odr-used. Directly emit the constant initializer.
The fix is to use the similar approach for non-references as for references. It
is achieved by trying to emit a constant before we attempt to load non-odr-used
variable as LValue.
Reid Kleckner [Thu, 1 Nov 2018 19:54:45 +0000 (19:54 +0000)]
Fix clang -Wimplicit-fallthrough warnings across llvm, NFC
This patch should not introduce any behavior changes. It consists of
mostly one of two changes:
1. Replacing fall through comments with the LLVM_FALLTHROUGH macro
2. Inserting 'break' before falling through into a case block consisting
of only 'break'.
We were already using this warning with GCC, but its warning behaves
slightly differently. In this patch, the following differences are
relevant:
1. GCC recognizes comments that say "fall through" as annotations, clang
doesn't
2. GCC doesn't warn on "case N: foo(); default: break;", clang does
3. GCC doesn't warn when the case contains a switch, but falls through
the outer case.
I will enable the warning separately in a follow-up patch so that it can
be cleanly reverted if necessary.
Aaron Ballman [Thu, 1 Nov 2018 18:57:38 +0000 (18:57 +0000)]
Output "rule" information in SARIF exports.
SARIF allows you to export descriptions about rules that are present in the SARIF log. Expose the help text table generated into Checkers.inc as the rule's "full description" and export all of the rules present in the analysis output. This information is useful for analysis result viewers like CodeSonar.
Reid Kleckner [Thu, 1 Nov 2018 18:53:02 +0000 (18:53 +0000)]
Add LLVM_FALLTHROUGH annotation after switch
This silences a -Wimplicit-fallthrough warning from clang. GCC does not
appear to warn when the case body ends in a switch.
This is a somewhat surprising but intended fallthrough that I pulled out
from my mechanical patch. The code intends to handle 'Yi' and related
constraints as the 'x' constraint.
Tim Northover [Thu, 1 Nov 2018 18:04:49 +0000 (18:04 +0000)]
Reapply Logging: make os_log buffer size an integer constant expression.
The size of an os_log buffer is known at any stage of compilation, so making it
a constant expression means that the common idiom of declaring a buffer for it
won't result in a VLA. That allows the compiler to skip saving and restoring
the stack pointer around such buffers.
This also moves the OSLog helpers from libclangAnalysis to libclangAST
to avoid a circular dependency.
Reid Kleckner [Thu, 1 Nov 2018 17:51:48 +0000 (17:51 +0000)]
Replace two fallthrough annotations after covered switch with unreachable
Both preceding switches handle all possible enumerators, so the
fallthrough is actually unreachable. This strengthens that to an
assertion.
The first instance had a comment from 2010 indicating that fallthrough
was possible, but that was back when we had a unary operator for
offsetof. Now it is its own expression kind, so the annotation was
stale.
Tim Northover [Thu, 1 Nov 2018 16:15:24 +0000 (16:15 +0000)]
Revert "Logging: make os_log buffer size an integer constant expression.
This also reverts a couple of follow-up commits trying to fix the
dependency issues. Latest revision added a cyclic dependency that can't
just be patched up in 5 minutes.
Erich Keane [Thu, 1 Nov 2018 15:11:41 +0000 (15:11 +0000)]
CPU-Dispatch- Fix type of a member function, prevent deferrals
The member type creation for a cpu-dispatch function was not correctly
including the 'this' parameter, so ensure that the type is properly
determined. Also, disable defer in the cases of emitting the functoins,
as it can end up resulting in the wrong version being emitted.
Tim Northover [Thu, 1 Nov 2018 13:49:54 +0000 (13:49 +0000)]
Logging: make os_log buffer size an integer constant expression.
The size of an os_log buffer is known at any stage of compilation, so making it
a constant expression means that the common idiom of declaring a buffer for it
won't result in a VLA. That allows the compiler to skip saving and restoring
the stack pointer around such buffers.
Erich Keane [Thu, 1 Nov 2018 12:50:37 +0000 (12:50 +0000)]
CPU-Dispatch-- Fix conflict between 'generic' and 'pentium'
When a dispatch function was being emitted that had both a generic and a
pentium configuration listed, we would assert. This is because neither
configuration has any 'features' associated with it so they were both
considered the 'default' version. 'pentium' lacks any features because
we implement it in terms of __builtin_cpu_supports (instead of Intel
proprietary checks), which is unable to decern between the two.
The fix for this is to omit the 'generic' version from the dispatcher if
both are present. This permits existing code to compile, and still will
choose the 'best' version available (since 'pentium' is technically
better than 'generic').
Li Jia He [Thu, 1 Nov 2018 02:32:49 +0000 (02:32 +0000)]
[Clang][PowerPC] Support constraint 'wi' in asm
From the gcc manual, we can see that the specific limit of wi inline asm is “FP or VSX register to hold 64-bit integers for VSX insns or NO_REGS”. The link is https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Machine-Constraints.html#Machine-Constraints. We should accept this constraint.
Artem Dergachev [Thu, 1 Nov 2018 00:43:35 +0000 (00:43 +0000)]
[analyzer] pr39348: MallocChecker: Realize that sized delete isn't custom delete.
MallocChecker no longer thinks that operator delete() that accepts the size of
the object to delete (available since C++14 or under -fsized-deallocation)
is some weird user-defined operator. Instead, it handles it like normal delete.
Additionally, it exposes a regression in NewDelete-intersections.mm's
testStandardPlacementNewAfterDelete() test, where the diagnostic is delayed
from before the call of placement new into the code of placement new
in the header. This happens because the check for pass-into-function-after-free
for placement arguments is located in checkNewAllocator(), which happens after
the allocator is inlined, which is too late. Move this use-after-free check
into checkPreCall instead, where it works automagically because the guard
that prevents it from working is useless and can be removed as well.
This commit causes regressions under -analyzer-config
c++-allocator-inlining=false but this option is essentially unsupported
because the respective feature has been enabled by default quite a while ago.
Shafik Yaghmour [Wed, 31 Oct 2018 21:53:15 +0000 (21:53 +0000)]
Revert "[ASTImporter][Structural Eq] Check for isBeingDefined"
This reverts commit r345760
because it caused an assertion in the lldb test suite. This is the log from the build bot: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12003/
Richard Smith [Wed, 31 Oct 2018 20:39:26 +0000 (20:39 +0000)]
Part of PR39508: Emit an @llvm.invariant.start after storing to
__tls_guard.
__tls_guard can only ever transition from 0 to 1, and only once. This
permits LLVM to remove repeated checks for TLS initialization and
repeated initialization code in cases like:
int g();
thread_local int n = g();
int a = n + n;
where we could not prove that __tls_guard was still 'true' when checking
it for the second reference to 'n' in the initializer of 'a'.
[clang-format] tweaked another case of lambda formatting
Summary:
This is done in order to improve cases where the lambda's body is moved too far to the right. Consider the following snippet with column limit set to 79:
The tool favors extra indentation for the lambda body and so the code incurs extra wrapping and adjacent calls are indented to a different level. I find this behavior annoying and I'd like the tool to favor new lines and, thus, use the extra width.
The fix, reduced, brings the following formatting.
George Karpenkov [Wed, 31 Oct 2018 17:38:29 +0000 (17:38 +0000)]
[analyzer] RetainCountChecker: for now, do not trust the summaries of inlined code
Trusting summaries of inlined code would require a more thorough work,
as the current approach was causing too many false positives, as the new
example in test. The culprit lies in the fact that we currently escape
all variables written into a field (but not passed off to unknown
functions!), which can result in inconsistent behavior.
Kristof Umann [Wed, 31 Oct 2018 14:54:27 +0000 (14:54 +0000)]
[analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag
This is the first part of the implementation of the inclusion of macro
expansions into the plist output. It adds a new flag that adds a new
"macro_expansions" entry to each report that has PathDiagnosticPieces that were
expanded from a macro. While there's an entry for each macro expansion, both
the name of the macro and what it expands to is missing, and will be implemented
in followup patches.
Hans Wennborg [Wed, 31 Oct 2018 10:34:46 +0000 (10:34 +0000)]
Follow-up to r345699: Call CheckStaticLocalForDllExport later for templates
Calling it too early might cause dllimport to get inherited onto the
VarDecl before the initializer got attached. See the test case for an
example where this broke things.
Hans Wennborg [Wed, 31 Oct 2018 08:38:48 +0000 (08:38 +0000)]
[clang-cl] Inherit dllexport to static locals also in template instantiations (PR39496)
In the course of D51340, @takuto.ikuta discovered that Clang fails to put
dllexport/import attributes on static locals during template instantiation.
For regular functions, this happens in Sema::FinalizeDeclaration(), however for
template instantiations we need to do something in or around
TemplateDeclInstantiator::VisitVarDecl(). This patch does that, and extracts
the code to a utility function.
Bill Wendling [Wed, 31 Oct 2018 03:48:47 +0000 (03:48 +0000)]
Create ConstantExpr class
A ConstantExpr class represents a full expression that's in a context where a
constant expression is required. This class reflects the path the evaluator
took to reach the expression rather than the syntactic context in which the
expression occurs.
In the future, the class will be expanded to cache the result of the evaluated
expression so that it's not needlessly re-evaluated
Reid Kleckner [Tue, 30 Oct 2018 23:58:41 +0000 (23:58 +0000)]
[Win64] Handle passing i128 by value
For arguments, pass it indirectly, since the ABI doc says pretty clearly
that arguments larger than 8 bytes are passed indirectly. This makes
va_list handling easier, anyway.
When returning, GCC returns in XMM0, and we match them.
This is the second half of Implicit Integer Conversion Sanitizer.
It completes the first half, and finally makes the sanitizer
fully functional! Only the bitfield handling is missing.
Summary:
C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while balancing between getting drowned in everything being convertible,
and nothing being convertible. As usual, this comes with a price:
```
void consume(unsigned int val);
void test(int val) {
consume(val);
// The 'val' is `signed int`, but `consume()` takes `unsigned int`.
// If val is negative, then consume() will be operating on a large
// unsigned value, and you may or may not have a bug.
// But yes, sometimes this is intentional.
// Making the conversion explicit silences the sanitizer.
consume((unsigned int)val);
}
```
Yes, there is a `-Wsign-conversion`` diagnostic group, but first, it is kinda
noisy, since it warns on everything (unlike sanitizers, warning on an
actual issues), and second, likely there are cases where it does **not** warn.
The actual detection is pretty easy. We just need to check each of the values
whether it is negative, and equality-compare the results of those comparisons.
The unsigned value is obviously non-negative. Zero is non-negative too.
https://godbolt.org/g/w93oj2
We do not have to emit the check *always*, there are obvious situations
where we can avoid emitting it, since it would **always** get optimized-out.
But i do think the tautological IR (`icmp ult %x, 0`, which is always false)
should be emitted, and the middle-end should cleanup it.
This sanitizer is in the `-fsanitize=implicit-conversion` group,
and is a logical continuation of D48958 `-fsanitize=implicit-integer-truncation`.
As for the ordering, i'we opted to emit the check **after**
`-fsanitize=implicit-integer-truncation`. At least on these simple 16 test cases,
this results in 1 of the 12 emitted checks being optimized away,
as compared to 0 checks being optimized away if the order is reversed.
This is a clang part.
The compiler-rt part is D50251.
Erik Pilkington [Tue, 30 Oct 2018 20:31:30 +0000 (20:31 +0000)]
NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects)
We haven't supported compiling ObjC1 for a long time (and never will again), so
there isn't any reason to keep these separate. This patch replaces
LangOpts::ObjC1 and LangOpts::ObjC2 with LangOpts::ObjC.
projects / clang / log