[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

Ping! Can you take a look, please?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

[Petr Hosek via Phabricator via cfe-commits]

phosek marked an inline comment as done.
phosek added a comment.

In D59168#1424968 , @smeenai wrote:

> I don't think the duplicated triple is too huge of a deal. I think the layout 
> where the resource directory is moved inside the triple directory is a bit 
> nicer, but I also don't know how much work that change would be and if it's 
> worth it.


One downside is that we would be duplicating Clang resource headers for each 
target which may not be too big of a deal but something worth mentioning.




Comment at: clang/test/Driver/linux-per-target-runtime-dir.c:15
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
+// CHECK-PER-TARGET-RUNTIME: 
"-L{{.*}}{{/|}}..{{/|}}lib{{/|}}clang{{/|}}x86_64-linux-gnu"
 // CHECK-PER-TARGET-RUNTIME: 
"-L[[RESDIR]]{{/|}}x86_64-linux-gnu{{/|}}lib"

smeenai wrote:
> Idk if it's worth making this a bit more specific – `clang -###` should print 
> out its InstallerDir, so you could capture that in a FileCheck regex and use 
> it to check the exact path.
I did that in the previous version but that was breaking on Windows and haven't 
yet figured out how to make it work there.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59168/new/

https://reviews.llvm.org/D59168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56990: Bugfix for Replacement of tied operand of inline asm

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added a comment.

We found may tests failed about this issue.
I hope It can be committed.
Thank you very much!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56990/new/

https://reviews.llvm.org/D56990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58324: Enable esan for the cache frag support

[David CARLIER via Phabricator via cfe-commits]

devnexen abandoned this revision.
devnexen added a comment.

Esan removed


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58324/new/

https://reviews.llvm.org/D58324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r355743 - [8.0 Regression] Fix handling of `__builtin_constant_p` inside template arguments, enumerators, case statements, and the enable_if attribute.

[Hans Wennborg via cfe-commits]

Merged to release_80 in r355898.

On Fri, Mar 8, 2019 at 11:05 PM Eric Fiselier via cfe-commits
 wrote:
>
> Author: ericwf
> Date: Fri Mar  8 14:06:48 2019
> New Revision: 355743
>
> URL: http://llvm.org/viewvc/llvm-project?rev=355743&view=rev
> Log:
> [8.0 Regression] Fix handling of `__builtin_constant_p` inside template 
> arguments, enumerators, case statements, and the enable_if attribute.
>
> Summary:
> The following code is accepted by Clang 7 and prior but rejected by the 
> upcoming 8 release and in trunk [1]
>
> ```
> // error {{never produces a constant expression}}
> void foo(const char* s) __attribute__((enable_if(__builtin_constant_p(*s) == 
> false, "trap"))) {}
> void test() { foo("abc"); }
> ```
>
> Prior to Clang 8, the call to `__builtin_constant_p` was a constant 
> expression returning false. Currently, it's not a valid constant expression.
>
> The bug is caused because we failed to set `InConstantContext` when 
> attempting to evaluate unevaluated constant expressions.
>
> [1]  https://godbolt.org/z/ksAjmq
>
> Reviewers: rsmith, hans, sbenza
>
> Reviewed By: rsmith
>
> Subscribers: kristina, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D59038
>
> Modified:
> cfe/trunk/lib/AST/ExprConstant.cpp
> cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
> cfe/trunk/test/SemaCXX/enable_if.cpp
>
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=355743&r1=355742&r2=355743&view=diff
> ==
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Mar  8 14:06:48 2019
> @@ -11131,6 +11131,7 @@ bool Expr::EvaluateAsConstantExpr(EvalRe
>const ASTContext &Ctx) const {
>EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
>EvalInfo Info(Ctx, Result, EM);
> +  Info.InConstantContext = true;
>if (!::Evaluate(Result.Val, Info, this))
>  return false;
>
> @@ -11771,6 +11772,7 @@ bool Expr::EvaluateWithSubstitution(APVa
>  const Expr *This) const {
>Expr::EvalStatus Status;
>EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpressionUnevaluated);
> +  Info.InConstantContext = true;
>
>LValue ThisVal;
>const LValue *ThisPtr = nullptr;
> @@ -11854,6 +11856,7 @@ bool Expr::isPotentialConstantExprUneval
>
>EvalInfo Info(FD->getASTContext(), Status,
>  EvalInfo::EM_PotentialConstantExpressionUnevaluated);
> +  Info.InConstantContext = true;
>
>// Fabricate a call stack frame to give the arguments a plausible cover 
> story.
>ArrayRef Args;
>
> Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp?rev=355743&r1=355742&r2=355743&view=diff
> ==
> --- cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp (original)
> +++ cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp Fri Mar  8 14:06:48 
> 2019
> @@ -1135,3 +1135,27 @@ constexpr bool indirect_builtin_constant
>return __builtin_constant_p(*__s);
>  }
>  constexpr bool n = indirect_builtin_constant_p("a");
> +
> +__attribute__((enable_if(indirect_builtin_constant_p("a") == n, "OK")))
> +int test_in_enable_if() { return 0; }
> +int n2 = test_in_enable_if();
> +
> +template 
> +int test_in_template_param() { return 0; }
> +int n3 = test_in_template_param();
> +
> +void test_in_case(int n) {
> +  switch (n) {
> +case indirect_builtin_constant_p("abc"):
> +break;
> +  }
> +}
> +enum InEnum1 {
> +  ONE = indirect_builtin_constant_p("abc")
> +};
> +enum InEnum2 : int {
> +  TWO = indirect_builtin_constant_p("abc")
> +};
> +enum class InEnum3 {
> +  THREE = indirect_builtin_constant_p("abc")
> +};
>
> Modified: cfe/trunk/test/SemaCXX/enable_if.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enable_if.cpp?rev=355743&r1=355742&r2=355743&view=diff
> ==
> --- cfe/trunk/test/SemaCXX/enable_if.cpp (original)
> +++ cfe/trunk/test/SemaCXX/enable_if.cpp Fri Mar  8 14:06:48 2019
> @@ -514,3 +514,11 @@ namespace TypeOfFn {
>
>static_assert(is_same<__typeof__(foo)*, decltype(&foo)>::value, "");
>  }
> +
> +namespace InConstantContext {
> +void foo(const char *s) 
> __attribute__((enable_if(((void)__builtin_constant_p(*s), true), "trap"))) {}
> +
> +void test() {
> +  InConstantContext::foo("abc");
> +}
> +} // namespace InConstantContext
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
htt

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 190218.
hokein marked 4 inline comments as done.
hokein added a comment.
Herald added a reviewer: serge-sans-paille.

Address review comments, rewrite the tool with python.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58345/new/

https://reviews.llvm.org/D58345

Files:
  clangd/StdGen/StdGen.py
  clangd/StdGen/test.py
  clangd/StdSymbolMap.inc
  clangd/index/CanonicalIncludes.cpp

Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -107,53 +107,20 @@
 
 void addSystemHeadersMapping(CanonicalIncludes *Includes) {
   static const std::vector> SymbolMap = {
-  {"std::addressof", ""},
-  // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},
-  {"std::basic_fstream", ""},
-  {"std::basic_ifstream", ""},
-  {"std::basic_ofstream", ""},
   {"std::filebuf", ""},
-  {"std::fstream", ""},
-  {"std::ifstream", ""},
-  {"std::ofstream", ""},
   {"std::wfilebuf", ""},
-  {"std::wfstream", ""},
-  {"std::wifstream", ""},
-  {"std::wofstream", ""},
-  {"std::basic_ios", ""},
-  {"std::ios", ""},
-  {"std::wios", ""},
-  {"std::basic_iostream", ""},
-  {"std::iostream", ""},
-  {"std::wiostream", ""},
   {"std::basic_istream", ""},
   {"std::istream", ""},
   {"std::wistream", ""},
-  {"std::istreambuf_iterator", ""},
-  {"std::ostreambuf_iterator", ""},
   {"std::basic_ostream", ""},
   {"std::ostream", ""},
   {"std::wostream", ""},
-  {"std::basic_istringstream", ""},
-  {"std::basic_ostringstream", ""},
-  {"std::basic_stringbuf", ""},
-  {"std::basic_stringstream", ""},
-  {"std::istringstream", ""},
-  {"std::ostringstream", ""},
-  {"std::string", ""},
-  {"std::stringbuf", ""},
-  {"std::stringstream", ""},
-  {"std::wistringstream", ""},
-  {"std::wostringstream", ""},
-  {"std::wstringbuf", ""},
-  {"std::wstringstream", ""},
-  {"std::basic_streambuf", ""},
-  {"std::streambuf", ""},
-  {"std::wstreambuf", ""},
   {"std::uint_least16_t", ""}, //  redeclares these
   {"std::uint_least32_t", ""},
-  {"std::declval", ""},
+#define SYMBOL(Name, NameSpace, Header) { #NameSpace#Name, #Header },
+  #include "StdSymbolMap.inc"
+#undef SYMBOL
   };
   for (const auto &Pair : SymbolMap)
 Includes->addSymbolMapping(Pair.first, Pair.second);
Index: clangd/StdSymbolMap.inc
===
--- /dev/null
+++ clangd/StdSymbolMap.inc
@@ -0,0 +1,1229 @@
+//===-- StdGen'erated file --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
+//
+// Automatically generated file, do not edit.
+//===--===//
+
+SYMBOL(Assignable, std::, )
+SYMBOL(Boolean, std::, )
+SYMBOL(Common, std::, )
+SYMBOL(CommonReference, std::, )
+SYMBOL(Constructible, std::, )
+SYMBOL(ConvertibleTo, std::, )
+SYMBOL(CopyConstructible, std::, )
+SYMBOL(Copyable, std::, )
+SYMBOL(DefaultConstructible, std::, )
+SYMBOL(DerivedFrom, std::, )
+SYMBOL(Destructible, std::, )
+SYMBOL(EqualityComparable, std::, )
+SYMBOL(EqualityComparableWith, std::, )
+SYMBOL(FILE, std::, )
+SYMBOL(Integral, std::, )
+SYMBOL(Invocable, std::, )
+SYMBOL(Movable, std::, )
+SYMBOL(MoveConstructible, std::, )
+SYMBOL(Predicate, std::, )
+SYMBOL(Regular, std::, )
+SYMBOL(RegularInvocable, std::, )
+SYMBOL(Relation, std::, )
+SYMBOL(Same, std::, )
+SYMBOL(Semiregular, std::, )
+SYMBOL(SignedIntegral, std::, )
+SYMBOL(StrictTotallyOrdered, std::, )
+SYMBOL(StrictTotallyOrderedWith, std::, )
+SYMBOL(StrictWeakOrder, std::, )
+SYMBOL(Swappable, std::, )
+SYMBOL(SwappableWith, std::, )
+SYMBOL(UniformRandomBitGenerator, std::, )
+SYMBOL(UnsignedIntegral, std::, )
+SYMBOL(_Exit, std::, )
+SYMBOL(accumulate, std::, )
+SYMBOL(add_const, std::, )
+SYMBOL(add_const_t, std::, )
+SYMBOL(add_cv, std::, )
+SYMBOL(add_cv_t, std::, )
+SYMBOL(add_lvalue_reference, std::, )
+SYMBOL(add_lvalue_reference_t, std::, )
+SYMBOL(add_pointer, std::, )
+SYMBOL(add_pointer_t, std::, )
+SYMBOL(add_rvalue_reference, std::, )
+SYMBOL(add_rvalue_reference_t, std::, )
+SYMBOL(add_volatile, std::, )
+SYMBOL(add_volatile_t, std::, )
+SYMBOL(addressof, std::, )
+SYMBOL(adjacent_difference, std::, )
+SYMBOL(adjacent_find, std::, )
+SYMBOL(adopt_lock, std::, )
+SYMBOL(adopt_lock_

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

add @ioeric as a reviewer, since @sammccall is OOO.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58345/new/

https://reviews.llvm.org/D58345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59183: [clang-tidy] Expand cases covered by the abseil-duration-unnecessary-conversion check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp:31
 
+auto factory_matcher = cxxConstructExpr(hasArgument(
+0,

could you add a few comment briefly describing these matchers?



Comment at: test/clang-tidy/abseil-duration-unnecessary-conversion.cpp:48
 
+  d2 = absl::Hours(d1 / absl::Hours(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration 
conversions [abseil-duration-unnecessary-conversion]

An off-topic comment: do we have this code pattern in the codebase? From my 
understanding, the usage like this is really rare. 



Comment at: test/clang-tidy/abseil-duration-unnecessary-conversion.cpp:50
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration 
conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d1
+  d2 = absl::Minutes(d1 / absl::Minutes(1));

could you check the full statement `d2 = d1`?  the same to other places.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59183/new/

https://reviews.llvm.org/D59183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:266
+
+  /// Returns the AST statement representing OpenMP structured-block of this
+  /// OpenMP executable directive,

"the AST node"



Comment at: include/clang/AST/StmtOpenMP.h:2158
 
+  /// OpenMP flush construct is a stand-alone directive.
+  llvm::Optional getStructuredBlockImpl() const { return llvm::None; };

This comment (and other similar comments on getStructuredBlockImpl()) are 
implementation comments and should be written in the function body.



Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif

Why not add a default definition:

```
#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
```

(Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
difficult to read (when they are 700 lines long), and it is unclear what the 
important parts are.

WDYT about converting them to unit tests?  See 
`clang/unittests/AST/StmtPrinterTest.cpp` for an example.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional getStructuredBlock() const;
 };

Why not `Stmt *` as a return value?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 190223.
Szelethus marked 13 inline comments as done.
Szelethus added a comment.

Fixes according to reviewer comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57855/new/

https://reviews.llvm.org/D57855

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/disable-all-checks.c
  test/Analysis/invalid-checker-option.c
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -90,6 +90,26 @@
   .str();
 }
 
+/// Retrieves the type from a CmdOptionTypeEnum typed Record object. Note that
+/// the class itself has to be modified for adding a new option type in
+/// CheckerBase.td.
+static std::string getCheckerOptionType(const Record &R) {
+  if (BitsInit *BI = R.getValueAsBitsInit("Type")) {
+switch(getValueFromBitsInit(BI, R)) {
+case 0:
+  return "int";
+case 1:
+  return "string";
+case 2:
+  return "bool";
+}
+  }
+  PrintFatalError(R.getLoc(),
+  "unable to parse command line option type for "
+  + getCheckerFullName(&R));
+  return "";
+}
+
 static void printChecker(llvm::raw_ostream &OS, const Record &R) {
 OS << "CHECKER(" << "\"";
 OS.write_escaped(getCheckerFullName(&R)) << "\", ";
@@ -134,6 +154,45 @@
   OS << "#endif // GET_PACKAGES\n"
 "\n";
 
+  // Emit a package option.
+  //
+  // PACKAGE_OPTION(OPTIONTYPE, PACKAGENAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - PACKAGENAME: Name of the package.
+  //   - OPTIONNAME: Name of the option.
+  //   - DESCRIPTION
+  //   - DEFAULT: The default value for this option.
+  //
+  // The full option can be specified in the command like like this:
+  //   -analyzer-config PACKAGENAME:OPTIONNAME=VALUE
+  OS << "\n"
+"#ifdef GET_PACKAGE_OPTIONS\n";
+  for (const Record *Package : packages) {
+
+if (Package->isValueUnset("PackageOptions"))
+  continue;
+
+std::vector PackageOptions = Package
+   ->getValueAsListOfDefs("PackageOptions");
+for (Record *PackageOpt : PackageOptions) {
+  OS << "PACKAGE_OPTION(\"";
+  OS.write_escaped(getCheckerOptionType(*PackageOpt)) << "\", \"";
+  OS.write_escaped(getPackageFullName(Package)) << "\", ";
+  OS << '\"' << getStringValue(*PackageOpt, "CmdFlag") << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "Desc")) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "DefaultVal")) << "\"";
+  OS << ")\n";
+}
+  }
+  OS << "#endif // GET_PACKAGE_OPTIONS\n"
+"\n";
+
   // Emit checkers.
   //
   // CHECKER(FULLNAME, CLASS, HELPTEXT)
@@ -160,15 +219,15 @@
   //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
   OS << "\n"
 "#ifdef GET_CHECKER_DEPENDENCIES\n";
-  for (const Record *checker : checkers) {
-if (checker->isValueUnset("Dependencies"))
+  for (const Record *Checker : checkers) {
+if (Checker->isValueUnset("Dependencies"))
   continue;
 
 for (const Record *Dependency :
-checker->getValueAsListOfDefs("Dependencies")) {
+Checker->getValueAsListOfDefs("Dependencies")) {
   OS << "CHECKER_DEPENDENCY(";
   OS << '\"';
-  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS.write_escaped(getCheckerFullName(Checker)) << "\", ";
   OS << '\"';
   OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
   OS << ")\n";
@@ -176,5 +235,45 @@
   }
   OS << "\n"
 "#endif // GET_CHECKER_DEPENDENCIES\n";
+
+  // Emit a package option.
+  //
+  // CHECKER_OPTION(OPTIONTYPE, CHECKERNAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - CHECKERNAME: Name of the package.
+  //   -

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:156
+  return FullName == Rhs.FullName;
+}
+

baloghadamsoftware wrote:
> Are we able now to distinguish checkers of the same short names but in 
> different packages? In earlier versions the short name had to be different to 
> avoid conflicts.
Full names contain the packages as well, like 
`"alpha.cplusplus.IteratorModeling"`. We don't really care about short names 
(which refers to the checker's name, right?) in `CheckerRegistry`.



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:282
+  /// \c Checkers field. For convenience purposes.
+  llvm::StringMap PackageSizes;
 

baloghadamsoftware wrote:
> Cannot we store the size of the `Package` in the `Package` itself?
Yup, this is a mess, but I only renamed this field. I sat on this for a while, 
thinking about how `Packages` and `PackageSizes` could be merged, and figured 
that it just isn't worth the development time to refactor, and would just 
further bloat this patch.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:50
+  }
+};
+

baloghadamsoftware wrote:
> Could we maybe use `std::less`?
Nope, `CheckerOrPackageInfo` doesn't implement `operator<`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57855/new/

https://reviews.llvm.org/D57855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57893: [analyzer] Fix function macro crash

[Tibor Brunner via Phabricator via cfe-commits]

bruntib added a comment.

I rebased the patch on the current master.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57893/new/

https://reviews.llvm.org/D57893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57893: [analyzer] Fix function macro crash

[Tibor Brunner via Phabricator via cfe-commits]

bruntib updated this revision to Diff 190225.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57893/new/

https://reviews.llvm.org/D57893

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
 1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
 }
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+   
+ 
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Returning from 'foo'
+ message
+ Returning from 'foo'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col35
+   file0
+  
+  
+   line459
+   col35
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col35
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+  issue_context_kindfunction
+  issue_contextuseZeroApplier1
+  issue_hash_function_offset0
+  location
+  
+   line459
+   col35
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+458
+459
+   
+  
+  
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line468
+   

r355903 - [analyzer] Fix function macro crash

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Tue Mar 12 03:03:32 2019
New Revision: 355903

URL: http://llvm.org/viewvc/llvm-project?rev=355903&view=rev
Log:
[analyzer] Fix function macro crash

When there is a functor-like macro which is passed as parameter to another
"function" macro then its parameters are not listed at the place of expansion:

#define foo(x) int bar() { return x; }
#define hello(fvar) fvar(0)
hello(foo)
int main() { 1 / bar(); }

Expansion of hello(foo) asserted Clang, because it expected an l_paren token in
the 3rd line after "foo", since it is a function-like token.

Patch by Tibor Brunner!

Differential Revision: https://reviews.llvm.org/D57893

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=355903&r1=355902&r2=355903&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Tue Mar 12 03:03:32 
2019
@@ -900,7 +900,7 @@ static std::string getMacroNameAndPrintE
   // If this is a function-like macro, skip its arguments, as
   // getExpandedMacro() already printed them. If this is the case, let's
   // first jump to the '(' token.
-  if (MI->getNumParams() != 0)
+  if (std::next(It)->is(tok::l_paren))
 It = getMatchingRParen(++It, E);
   continue;
 }
@@ -928,7 +928,15 @@ static std::string getMacroNameAndPrintE
 
 getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
   Info.Args, AlreadyProcessedTokens);
-if (MI->getNumParams() != 0)
+// Peek the next token if it is a tok::l_paren. This way we can decide
+// if this is the application or just a reference to a function maxro
+// symbol:
+//
+// #define apply(f) ...
+// #define func(x) ...
+// apply(func)
+// apply(func(42))
+if (std::next(ArgIt)->is(tok::l_paren))
   ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
   }
   continue;
@@ -990,8 +998,16 @@ static MacroNameAndArgs getMacroNameAndA
 return { MacroName, MI, {} };
 
   RawLexer.LexFromRawLexer(TheTok);
-  assert(TheTok.is(tok::l_paren) &&
- "The token after the macro's identifier token should be '('!");
+  // When this is a token which expands to another macro function then its
+  // parentheses are not at its expansion locaiton. For example:
+  //
+  // #define foo(x) int bar() { return x; }
+  // #define apply_zero(f) f(0)
+  // apply_zero(foo)
+  //   ^
+  //   This is not a tok::l_paren, but foo is a function.
+  if (TheTok.isNot(tok::l_paren))
+return { MacroName, MI, {} };
 
   MacroArgMap Args;
 

Modified: 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist?rev=355903&r1=355902&r2=355903&view=diff
==
--- 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 (original)
+++ 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 Tue Mar 12 03:03:32 2019
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16

[PATCH] D57893: [analyzer] Fix function macro crash

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355903: [analyzer] Fix function macro crash (authored by 
Szelethus, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57893/new/

https://reviews.llvm.org/D57893

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
 1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
 }
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+   
+ 
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Returning from 'foo'
+ message
+ Returning from 'foo'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col35
+   file0
+  
+  
+   line459
+   col35
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col35
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+  issue_context_kindfunction
+  issue_contextuseZeroApplier1
+  issue_hash_function_offset0
+  location
+  
+   line459
+   col35
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+458
+459
+   
+  
+  
+  
+

[PATCH] D57890: [analyzer] Fix in self assignment checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.
Herald added subscribers: Charusso, jdoerfert, whisperity.

Yup, I agree.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57890/new/

https://reviews.llvm.org/D57890



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57855: [analyzer] Reimplement checker options

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:49
+  "NoDiagnoseCallsToSystemHeaders",
+  "",
+  "false">

Let's put at least a FIXME here that the documentation for this option was 
missing.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:907
+  "TrackNSCFStartParam",
+  "",
+  "false">

Same here for missing documentation/description for optstring.



Comment at: lib/Frontend/CompilerInvocation.cpp:425
   bool HasFailed = getStringOption(Config, Name, std::to_string(DefaultVal))
- .getAsInteger(10, OptionField);
+ .getAsInteger(0, OptionField);
   if (Diags && HasFailed)

What is this trying to do?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57855/new/

https://reviews.llvm.org/D57855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58065: [analyzer] Document the frontend library

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:85
+-fuse-ld=lld \
+../llvm
+

george.karpenkov wrote:
> Information on compiling LLVM IMO should not be here.
> Also, why Sphinx? Why X86? Why LLD and not gold?
> Information on compiling LLVM IMO should not be here.
> Also, why Sphinx? Why X86? Why LLD and not gold?

Analyser for development -- however, you build `release` instead of (at least) 
`RelWithDebInfo`.

`EXPORT_COMPILE_COMMANDS` is also totally unnecessary for an "analyser for 
development".

Perhaps individual recommended settings could be linked back to the 
configuration guide's sections? (Unfortunately that guide can't link to 
individual //options// sadly.)



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:94
+Other documents detail the difference between the *driver* and the *frontend*
+of clang far more precisely, but we'll touch on this briefly: When you input
+``clang`` into the command line, you invoke the driver. This compiler driver

When we talk about the project, use **C**lang. When talking about the binary, 
`clang` in monospace is good style.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:140
+
+Although we don't support running the analyzer without enabling the entire core
+package, it is possible, but might lead to crashes and incorrect reports.

`core` for emphasis.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58065/new/

https://reviews.llvm.org/D58065



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:329
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.

Insertion



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:340
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {

Does this mean that we no longer can give "1" and "0" for boolean checker 
options? Or was that //Tidy// who allowed such in the first place?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58573: [analyzer] Move UninitializedObject out of alpha

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In case no bug reports //against// the checker are reported on the mailing list 
or Bugzilla, I wholeheartedly agree with kicking the ball here.

As for the package, perhaps we could go into `optin.bugprone` or 
`optin.conventions`? (These are new package names...)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58573/new/

https://reviews.llvm.org/D58573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[libunwind] r355910 - Creating release candidate rc5 from release_800 branch

[Hans Wennborg via cfe-commits]

Author: hans
Date: Tue Mar 12 04:16:25 2019
New Revision: 355910

URL: http://llvm.org/viewvc/llvm-project?rev=355910&view=rev
Log:
Creating release candidate rc5 from release_800 branch

Added:
libunwind/tags/RELEASE_800/rc5/
  - copied from r355909, libunwind/branches/release_80/

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[libclc] r355910 - Creating release candidate rc5 from release_800 branch

[Hans Wennborg via cfe-commits]

Author: hans
Date: Tue Mar 12 04:16:25 2019
New Revision: 355910

URL: http://llvm.org/viewvc/llvm-project?rev=355910&view=rev
Log:
Creating release candidate rc5 from release_800 branch

Added:
libclc/tags/RELEASE_800/rc5/
  - copied from r355909, libclc/branches/release_80/

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355911 - Revert "[analyzer] Fix function macro crash"

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Tue Mar 12 04:22:30 2019
New Revision: 355911

URL: http://llvm.org/viewvc/llvm-project?rev=355911&view=rev
Log:
Revert "[analyzer] Fix function macro crash"

Buildbot breaks when LLVm is compiled with memory sanitizer.

WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0xa3d16d8 in getMacroNameAndPrintExpansion(blahblah)
 lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:903:11

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=355911&r1=355910&r2=355911&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Tue Mar 12 04:22:30 
2019
@@ -900,7 +900,7 @@ static std::string getMacroNameAndPrintE
   // If this is a function-like macro, skip its arguments, as
   // getExpandedMacro() already printed them. If this is the case, let's
   // first jump to the '(' token.
-  if (std::next(It)->is(tok::l_paren))
+  if (MI->getNumParams() != 0)
 It = getMatchingRParen(++It, E);
   continue;
 }
@@ -928,15 +928,7 @@ static std::string getMacroNameAndPrintE
 
 getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
   Info.Args, AlreadyProcessedTokens);
-// Peek the next token if it is a tok::l_paren. This way we can decide
-// if this is the application or just a reference to a function maxro
-// symbol:
-//
-// #define apply(f) ...
-// #define func(x) ...
-// apply(func)
-// apply(func(42))
-if (std::next(ArgIt)->is(tok::l_paren))
+if (MI->getNumParams() != 0)
   ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
   }
   continue;
@@ -998,16 +990,8 @@ static MacroNameAndArgs getMacroNameAndA
 return { MacroName, MI, {} };
 
   RawLexer.LexFromRawLexer(TheTok);
-  // When this is a token which expands to another macro function then its
-  // parentheses are not at its expansion locaiton. For example:
-  //
-  // #define foo(x) int bar() { return x; }
-  // #define apply_zero(f) f(0)
-  // apply_zero(foo)
-  //   ^
-  //   This is not a tok::l_paren, but foo is a function.
-  if (TheTok.isNot(tok::l_paren))
-return { MacroName, MI, {} };
+  assert(TheTok.is(tok::l_paren) &&
+ "The token after the macro's identifier token should be '('!");
 
   MacroArgMap Args;
 

Modified: 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist?rev=355911&r1=355910&r2=355911&view=diff
==
--- 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 (original)
+++ 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 Tue Mar 12 04:22:30 2019
@@ -5577,484 +5577,6 @@

   
   
-  
-   path
-   
-
- kindcontrol
- edges
-  
-   
-start
- 
-  
-   line459
-   col33
-   file0
-  
-  
-   line459
-   col33
-   file0
-  
- 
-end
- 
-  
-   line459
-   col37
-   file0
-  
-  
-   line459
-   col39
-   file0
-  
- 
-   
-  
-
-
- kindevent
- location
- 
-  line459
-  col37
-  file0
- 
- ranges
- 
-   
-
- line459
- col37
- file0
-
-
- line459
- col41
- file0
-
-   
- 
- depth0
- extended_message
- Calling 'foo'
- message
- Calling 'foo'
-
-
- kindevent
- location
- 
-  line458
-  col1
-  file0
- 
- depth1
- extended_message
- Entered call from 'useZeroApplier1'
- message
- Entered call from 'useZeroApplier1'
-
-
- kindevent
- location
- 
-  line458
-  col1
-  file0
- 
- ranges
- 
-   
-
- line458
- col1
- file0
-
-
- line458
- col16
- file0
-
-   
- 
- depth1
- extended_message
- Returning zero
- message
- Returning zero
-
-
- kindevent
- location
- 
-  line459
-  col37
-

[PATCH] D57464: Generalize method overloading on addr spaces to C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D57464#1421493 , @ebevhan wrote:

> Any more input on this?


I think not. :( But I am wondering if we could proceed for now in some general 
direction and then make any improvements later. Probably the biggest part of 
this patch is not going to change. Would this make sense?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57464/new/

https://reviews.llvm.org/D57464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:340
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {

whisperity wrote:
> Does this mean that we no longer can give "1" and "0" for boolean checker 
> options? Or was that //Tidy// who allowed such in the first place?
Well yea, "true" or "false" is the only input we accept (and have ever 
accepted) for boolean configs. I have nothing against adding 1 and 0 too, but 
let that be a topic of another revision.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Would `NoteTag`s be displayed in a dumped exploded graph?

In D58367#1405185 , @NoQ wrote:

> In D58367#1404722 , @Charusso wrote:
>
> > So with that, I would out-chain this patch from the MIG-patches because it 
> > is a lot more serious problem-set. I also would like to ask you to take it 
> > more serious, as all your mentioned benefits will rely on this simple and 
> > cool approach, so it has to be flexible as possible.
>
>
> I've been thinking for about a month about this until now, and i'm actually 
> very surprised that i see no obvious flexibility issues here. Stateful 
> visitors (eg., the ones that only highlight the *last* interesting event) can 
> be easily implemented by keeping via lambdas as a private state data in the 
> bug reporter. Mutually recursive systems of multiple visitors that add each 
> other dynamically during the visit (such as the `trackExpressionValue` that's 
> infamous for that exact reason) should be (and most likely could be) 
> untangled anyway, and once they're untangled (eg., by keeping just one 
> instance of the visitor while dynamically updating its tracking information), 
> the flexibility issue disappears; i'm almost happy that it would no longer be 
> possible to entangle the code that way. Dunno, this is weird - usually i 
> quickly come up with examples of why something wouldn't work and decide to 
> implement it anyway, but this time i'm surprisingly secure about implementing 
> it.




In D58367#1402847 , @NoQ wrote:

> In D58367#1402796 , @Szelethus wrote:
>
> > 2. I guess most of the visitors could be changed to this format, do you 
> > have plans to convert them? I'll happily land a hand, because it sounds 
> > like a big chore. I guess that would also test this implementation fairly 
> > well.
>
>
> I don't have an immediate plan but i'll definitely convert visitors when i 
> touch them and get annoyed. Also i believe that this new functionality is 
> much more useful for //core// visitors than for checker visitors, simply 
> because there's much more information to reverse-engineer in every visitor.


As I understand it, this solution could be used to entirely get rid of the 
current bugreporter visitor structure (at least for checkers), right? The 
discussion seems to conclude that this is just as general, far easier to 
understand, far easier to implement, and is basically better in every regard 
without a hit to performance? Because if so, I'm definitely against supporting 
two concurrent implementations of the same functionality -- in fact, we should 
even just forbid checkers to add custom visitors.

I can't say much about the rest of the discussion, seems like you two are far 
more knowledgable on this topic than me :^)

> it hurts me when i see all of them duplicated in the visitor as a 
> military-grade portion of spaghetti.

Made my day :D




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:386-398
+  // Manage memory for NoteTag objects.
+  class Factory {
+std::vector> Tags;
+
+  public:
+const NoteTag *makeNoteTag(Callback &&Cb) {
+  // We cannot use make_unique because we cannot access the private

Hmmm, did you consider the other memory management options we have in LLVM, 
like `BumpPtrAllocator`? Sounds a bit better for this use case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58367/new/

https://reviews.llvm.org/D58367



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355915 - [PR41007][OpenCL] Allow printf in C++ mode.

[Anastasia Stulova via cfe-commits]

Author: stulova
Date: Tue Mar 12 05:46:56 2019
New Revision: 355915

URL: http://llvm.org/viewvc/llvm-project?rev=355915&view=rev
Log:
[PR41007][OpenCL] Allow printf in C++ mode.

As for OpenCL C, we need to allow using printf and toolchain variadic
functions (prefixed by "__") in C++ mode.

Differential Revision: https://reviews.llvm.org/D59219


Modified:
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/SemaOpenCL/extensions.cl

Modified: cfe/trunk/lib/Sema/SemaType.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=355915&r1=355914&r2=355915&view=diff
==
--- cfe/trunk/lib/Sema/SemaType.cpp (original)
+++ cfe/trunk/lib/Sema/SemaType.cpp Tue Mar 12 05:46:56 2019
@@ -4585,7 +4585,7 @@ static TypeSourceInfo *GetFullTypeForDec
 if (FTI.isVariadic &&
 !(D.getIdentifier() &&
   ((D.getIdentifier()->getName() == "printf" &&
-LangOpts.OpenCLVersion >= 120) ||
+(LangOpts.OpenCLCPlusPlus || LangOpts.OpenCLVersion >= 120)) ||
D.getIdentifier()->getName().startswith("__" {
   S.Diag(D.getIdentifierLoc(), diag::err_opencl_variadic_function);
   D.setInvalidType(true);

Modified: cfe/trunk/test/SemaOpenCL/extensions.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaOpenCL/extensions.cl?rev=355915&r1=355914&r2=355915&view=diff
==
--- cfe/trunk/test/SemaOpenCL/extensions.cl (original)
+++ cfe/trunk/test/SemaOpenCL/extensions.cl Tue Mar 12 05:46:56 2019
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59219: [PR41007][OpenCL] Allow printf and toolchain reserved variadic functions in C++

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355915: [PR41007][OpenCL] Allow printf in C++ mode. 
(authored by stulova, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59219?vs=190108&id=190239#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59219/new/

https://reviews.llvm.org/D59219

Files:
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/SemaOpenCL/extensions.cl


Index: cfe/trunk/test/SemaOpenCL/extensions.cl
===
--- cfe/trunk/test/SemaOpenCL/extensions.cl
+++ cfe/trunk/test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -4585,7 +4585,7 @@
 if (FTI.isVariadic &&
 !(D.getIdentifier() &&
   ((D.getIdentifier()->getName() == "printf" &&
-LangOpts.OpenCLVersion >= 120) ||
+(LangOpts.OpenCLCPlusPlus || LangOpts.OpenCLVersion >= 120)) ||
D.getIdentifier()->getName().startswith("__" {
   S.Diag(D.getIdentifierLoc(), diag::err_opencl_variadic_function);
   D.setInvalidType(true);


Index: cfe/trunk/test/SemaOpenCL/extensions.cl
===
--- cfe/trunk/test/SemaOpenCL/extensions.cl
+++ cfe/trunk/test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ -finclude-default-header
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -4585,7 +4585,7 @@
 if (FTI.isVariadic &&
 !(D.getIdentifier() &&
   ((D.getIdentifier()->getName() == "printf" &&
-LangOpts.OpenCLVersion >= 120) ||
+(LangOpts.OpenCLCPlusPlus || LangOpts.OpenCLVersion >= 120)) ||
D.getIdentifier()->getName().startswith("__" {
   S.Diag(D.getIdentifierLoc(), diag::err_opencl_variadic_function);
   D.setInvalidType(true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

Is this the most efficient way to concatenate `StringRef`s?



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:443
+StringRef SuppliedChecker(Config.first().substr(0, Pos));
+StringRef SuppliedOption(Config.first().drop_front(Pos + 1));
 

What about `Config.first().split(":")`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57893: [analyzer] Fix function macro crash

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus reopened this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Ugh. Reverted the patch.

  FAIL: Clang :: Analysis/plist-macros-with-expansion.cpp (720 of 14281)
   TEST 'Clang :: Analysis/plist-macros-with-expansion.cpp' 
FAILED 
  Script:
  --
  : 'RUN: at line 1';   
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 
-internal-isystem 
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/9.0.0/include 
-nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=core 
-verify 
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
  : 'RUN: at line 3';   
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 
-internal-isystem 
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/9.0.0/include 
-nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=core 
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
 -analyzer-output=plist -o 
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
-analyzer-config expand-macros=true
  : 'RUN: at line 8';   cat 
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
 | diff -u -w -I "/" -I ".:" -I "version"
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 -
  : 'RUN: at line 13';   
/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/FileCheck 
--input-file=/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
 
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
  --
  Exit Code: 77
  
  Command Output (stderr):
  --
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:451:7:
 warning: expression result unused
  1 / value; // expected-warning{{Division by zero}}
  ~ ^ ~
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:27:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:40:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:60:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:80:12:
 warning: Dereference of null pointer (loaded from variable 'a')
DEREF(a) = 5; // expected-warning{{Dereference of null pointer}}
  ~  ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:99:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:116:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:136:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:163:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:172:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:181:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:195:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitiz

[PATCH] D57893: [analyzer] Fix function macro crash

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Let's investigate what's behind this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57893/new/

https://reviews.llvm.org/D57893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

baloghadamsoftware wrote:
> Is this the most efficient way to concatenate `StringRef`s?
> @baloghadamsoftware 
> Is this the most efficient way to concatenate `StringRef`s?

`Twine` is specifically designed for that, yes.
You can't evade the final "copy", although most likely there'll be a RVO taking 
place.

However, the *first* `Twine` could be initialised from the `FullName` variable, 
as opposed to empty node.
And you could use single characters as characters, as opposed to strings,  i.e. 
`(Twine(FullName) + ':' + OptionName).str()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57893: [analyzer] Fix function macro crash

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Some bots also break but emit a different message:

   TEST 'Clang :: Analysis/plist-macros-with-expansion.cpp' 
FAILED 
  Script:
  --
  : 'RUN: at line 1';   
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/clang -cc1 
-internal-isystem 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/lib/clang/9.0.0/include
 -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=core 
-verify 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
  : 'RUN: at line 3';   
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/clang -cc1 
-internal-isystem 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/lib/clang/9.0.0/include
 -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=core 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
 -analyzer-output=plist -o 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
-analyzer-config expand-macros=true
  : 'RUN: at line 8';   cat 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
 | diff -u -w -I "/" -I ".:" -I "version"
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 -
  : 'RUN: at line 13';   
/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/FileCheck 
--input-file=/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
 
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:451:7:
 warning: expression result unused
  1 / value; // expected-warning{{Division by zero}}
  ~ ^ ~
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:27:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:40:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:60:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:80:12:
 warning: Dereference of null pointer (loaded from variable 'a')
DEREF(a) = 5; // expected-warning{{Dereference of null pointer}}
  ~  ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:99:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:116:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:136:8:
 warning: Dereference of null pointer (loaded from variable 'ptr')
*ptr = 5; // expected-warning{{Dereference of null pointer}}
 ~~~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:163:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:172:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:181:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp:195:6:
 warning: Dereference of null pointer (loaded from variable 'a')
*a = 5; // expected-warning{{Dereference of null pointer}}
 ~ ^
  
/b/sanitizer-x86_64-l

[PATCH] D59195: [analyzer] Remove the default value arg from getChecker*Option

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware accepted this revision.
baloghadamsoftware added a comment.

This is straightforward.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59195/new/

https://reviews.llvm.org/D59195



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59253: [AIX][libcxx] AIX system headers need stdint.h and inttypes.h to be re-enterable when macro _STD_TYPES_T is defined

[Xing Xue via Phabricator via cfe-commits]

xingxue created this revision.
xingxue added reviewers: hubert.reinterpretcast, jasonliu, mclow.lists.
xingxue added a project: LLVM.
Herald added a reviewer: EricWF.
Herald added subscribers: libcxx-commits, cfe-commits, christof.
Herald added projects: clang, libc++.

AIX system headers need stdint.h and inttypes.h to be re-enterable when macro 
_STD_TYPES_T is defined so that limit macro definitions such as UINT32_MAX can 
be found.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59253

Files:
  clang/lib/Headers/inttypes.h
  clang/lib/Headers/stdint.h
  libcxx/include/inttypes.h
  libcxx/include/stdint.h


Index: libcxx/include/stdint.h
===
--- libcxx/include/stdint.h
+++ libcxx/include/stdint.h
@@ -8,7 +8,11 @@
 
//===--===//
 
 #ifndef _LIBCPP_STDINT_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define _LIBCPP_STDINT_H
+#endif // _STD_TYPES_T
 
 /*
 stdint.h synopsis
Index: libcxx/include/inttypes.h
===
--- libcxx/include/inttypes.h
+++ libcxx/include/inttypes.h
@@ -8,7 +8,11 @@
 
//===--===//
 
 #ifndef _LIBCPP_INTTYPES_H
+// AIX system headers need inttypes.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define _LIBCPP_INTTYPES_H
+#endif // _STD_TYPES_T
 
 /*
 inttypes.h synopsis
Index: clang/lib/Headers/stdint.h
===
--- clang/lib/Headers/stdint.h
+++ clang/lib/Headers/stdint.h
@@ -23,7 +23,11 @@
 
\*===--===*/
 
 #ifndef __CLANG_STDINT_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T) || !defined(__STDC_HOSTED__)
 #define __CLANG_STDINT_H
+#endif
 
 /* If we're hosted, fall back to the system's stdint.h, which might have
  * additional definitions.
Index: clang/lib/Headers/inttypes.h
===
--- clang/lib/Headers/inttypes.h
+++ clang/lib/Headers/inttypes.h
@@ -21,7 +21,11 @@
 
\*===--===*/
 
 #ifndef __CLANG_INTTYPES_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define __CLANG_INTTYPES_H
+#endif
 
 #if defined(_MSC_VER) && _MSC_VER < 1800
 #error MSVC does not have inttypes.h prior to Visual Studio 2013


Index: libcxx/include/stdint.h
===
--- libcxx/include/stdint.h
+++ libcxx/include/stdint.h
@@ -8,7 +8,11 @@
 //===--===//
 
 #ifndef _LIBCPP_STDINT_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define _LIBCPP_STDINT_H
+#endif // _STD_TYPES_T
 
 /*
 stdint.h synopsis
Index: libcxx/include/inttypes.h
===
--- libcxx/include/inttypes.h
+++ libcxx/include/inttypes.h
@@ -8,7 +8,11 @@
 //===--===//
 
 #ifndef _LIBCPP_INTTYPES_H
+// AIX system headers need inttypes.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define _LIBCPP_INTTYPES_H
+#endif // _STD_TYPES_T
 
 /*
 inttypes.h synopsis
Index: clang/lib/Headers/stdint.h
===
--- clang/lib/Headers/stdint.h
+++ clang/lib/Headers/stdint.h
@@ -23,7 +23,11 @@
 \*===--===*/
 
 #ifndef __CLANG_STDINT_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T) || !defined(__STDC_HOSTED__)
 #define __CLANG_STDINT_H
+#endif
 
 /* If we're hosted, fall back to the system's stdint.h, which might have
  * additional definitions.
Index: clang/lib/Headers/inttypes.h
===
--- clang/lib/Headers/inttypes.h
+++ clang/lib/Headers/inttypes.h
@@ -21,7 +21,11 @@
 \*===--===*/
 
 #ifndef __CLANG_INTTYPES_H
+// AIX system headers need stdint.h to be re-enterable if _STD_TYPES_T
+// is defined.
+#if !defined(_AIX) || !defined(_STD_TYPES_T)
 #define __CLANG_INTTYPES_H
+#endif
 
 #if defined(_MSC_VER) && _MSC_VER < 1800
 #error MSVC does not have inttypes.h prior to Visual Studio 201

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:332
+  AnOpts.Config.insert({(Twine() + CheckerFullName + ":" + OptionName).str(),
+DefaultValStr});
 }

`Twine(CheckerFullName) + ":" + OptionName`. However, since the constructor 
`Twine(const StringRef &Str)` is implicit, `CheckerFullName + ":" + OptionName` 
results the same and is more readable.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:354
+  AnOpts.Config.insert({(Twine() + FullName + ":" + OptionName).str(),
+DefaultValStr});
 }

Same as above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57922/new/

https://reviews.llvm.org/D57922



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

whisperity wrote:
> baloghadamsoftware wrote:
> > Is this the most efficient way to concatenate `StringRef`s?
> > @baloghadamsoftware 
> > Is this the most efficient way to concatenate `StringRef`s?
> 
> `Twine` is specifically designed for that, yes.
> You can't evade the final "copy", although most likely there'll be a RVO 
> taking place.
> 
> However, the *first* `Twine` could be initialised from the `FullName` 
> variable, as opposed to empty node.
> And you could use single characters as characters, as opposed to strings,  
> i.e. `(Twine(FullName) + ':' + OptionName).str()`.
The constructor `Twine(const StringRef &Str)` is implicit so `(FullName + ':' + 
OptionName).str()` results the same and is more readable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  ---*- python 
-*--===#
+#

I'd avoid abbreviation in the file name and the new directory name. `StdGen` 
sounds a lot like a library ;)

I'd suggest something like `std-include-mapping/generate.py`



Comment at: clangd/StdGen/StdGen.py:14
+
+Caveats and FIXMEs:
+  - only symbols directly in "std::namespace" are added

we should also mention what the tool's behaviors are for these cases.



Comment at: clangd/StdGen/StdGen.py:15
+Caveats and FIXMEs:
+  - only symbols directly in "std::namespace" are added
+  - symbols with multiple variants, or defined in multiple headers aren't added

nit: s/"std::namespace"/std:: namespace/?



Comment at: clangd/StdGen/StdGen.py:16
+  - only symbols directly in "std::namespace" are added
+  - symbols with multiple variants, or defined in multiple headers aren't added
+

it's not trivial what these are. could provide an example here. 



Comment at: clangd/StdGen/StdGen.py:18
+
+Usage:
+  1. Install BeautifulSoup dependency

Suggestion: it would be nice to provide the common commands for each step. E.g. 
`pip install `. 



Comment at: clangd/StdGen/StdGen.py:23
+  3. Run the command:
+   StdGen.py -cppreference-doc  > StdGen.h
+"""

what is the `` here? is it the downloaded zip, the 
unzipped (e.g. `cppreference-doc-20181028/`), or some specific html doc in the 
directory?



Comment at: clangd/StdGen/StdGen.py:23
+  3. Run the command:
+   StdGen.py -cppreference-doc  > StdGen.h
+"""

ioeric wrote:
> what is the `` here? is it the downloaded zip, the 
> unzipped (e.g. `cppreference-doc-20181028/`), or some specific html doc in 
> the directory?
Again, `StdGen` is not great name. I'd suggest `StdHeaderMapping`.

nit: s/.h/.inc/



Comment at: clangd/StdGen/StdGen.py:32
+
+STDGEN_CODE = """\
+//===-- StdGen'erated file --*- C++ 
-*-===//

nit: maybe `STDGEN_CODE_HEADER`?



Comment at: clangd/StdGen/StdGen.py:48
+def ParseSymbolPage(symbol_page_html):
+  """Parse the symbol page and retrieve the include header defined in this 
page.
+  Returns a list of headers.

could you provide a sample html snippet in the comment? it would make the code 
easier to understand. thanks!

(same for `ParseIndexPage`)



Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+exit("Path %s doesn't exist!" % cpp_reference_root)

why not check `exists(index_page_path)` instead?



Comment at: clangd/StdGen/StdGen.py:116
+for name, headers in sorted(symbols.items(), key=lambda t : t[0]):
+  # FIXME: support ambiguous symbols
+  if len(headers) > 1:

move this FIXME into the `if` body to make it clearer that this is for 
`len(headers) > 1`. 

nit: it's not very clear whether `ambiguous symbols` = `symbols with multiple 
headers`. Maybe be more specific?



Comment at: clangd/StdSymbolMap.inc:8
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.

can we include the information about the HTML archive (e.g. STL version) from 
which this table is generated? This would be useful for future maintenance.



Comment at: clangd/StdSymbolMap.inc:11
+//
+// Automatically generated file, do not edit.
+//===--===//

Add a pointer to the python script?



Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector>
   SystemHeaderMap = {

hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > ioeric wrote:
> > > > Can we remove the suffix header mapping now? Is it for the 
> > > > `std::chrono::` symbols? What are the reasons not to include them in 
> > > > this patch? 
> > > Ideally, we could remove it once we get a perfect symbol mapping but I'd 
> > > prefer to keep it (it serves as a fallback when we don't find symbols in 
> > > the symbol mapping), so that we could add new symbol mapping increasingly 
> > > without changing the current behavior.
> > > 
> > > Removing it should be done carefully without introducing regression, and 
> > > is outside the scope of this patch.
> > We do need fallback heuristics. We should conisder cases:
> >  - for known `std::` symbols mapped to one system header, we don't need a 
> > fallback
> >  -

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

To split this into multiple independent changes, we could start with storing 
the symbols for template specializations, but only showing the primary template 
in the results of workspaceSymbols.
This would enable other features (xrefs, etc), and we could re-add 
specializations to workspaceSymbols after we improve the presentation.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59083/new/

https://reviews.llvm.org/D59083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl created this revision.
Herald added subscribers: cfe-commits, jdoerfert, mgorny.
Herald added a project: clang.

This patch set introduces structure field layout randomization into the Clang
compiler. The Randstruct feature is a compile-time hardening technique that 
randomizes the field layout for designated structures of a code base. 
Admittedly, this is mostly useful for closed-source releases of code (since 
the randomization seed would be available for public and open source application
s). However, this patch set also enhances Clang’s feature parity with that 
of GCC which already has the Randstruct feature.

This patch set is a from-scratch reimplementation of the Randstruct feature 
that was originally ported to GCC. The patches for this implementation in GCC 
can be found here:

  https://www.openwall.com/lists/kernel-hardening/2017/04/06/14.

This feature identifies structures for randomization in two ways. The first 
method targets structures that are manually marked with the new 
“randomize_layout” attribute. The second is an optional feature that will 
automatically select and randomize structures that are found to consist entirely
of function pointers. This automatic selection feature can be extended to 
include other vulnerable structure types that are safe to randomize as they are
identified. You can also opt a specific structure out of this feature with the 
“no_randomize_layout” attribute. Automatic structure selection is enabled with 
the “-randstruct-auto” compiler flag. By default, Randstruct seeds on the empty
string, but a seed can be supplied with the “-randstruct-seed=” command line 
argument.

This entire patch set is the sum total of an undergraduate computer science 
capstone team’s effort.

Portland State University Clang Randstruct Capstone Team (Fall 2018-Winter 
2019):

Co-authored-by: Cole Nixon 
Co-authored-by: Connor Kuehl 
Co-authored-by: James Foster 
Co-authored-by: Jeff Takahashi 
Co-authored-by: Jordan Cantrell 
Co-authored-by: Nikk Forbus 
Co-authored-by: Tim Pugh 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59254

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/RandstructSeed.h
  clang/include/clang/AST/RecordFieldReorganizer.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/RecordFieldReorganizer.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -81,6 +81,7 @@
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function)
 // CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
+// CHECK-NEXT: NoRandomizeLayout (SubjectMatchRule_record)
 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
@@ -118,6 +119,7 @@
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: RandomizeLayout (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6966,6 +6966,12 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_CodeSeg:
 handleCodeSegAttr(S, D, AL);
 break;
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//

[PATCH] D59253: [AIX][libcxx] AIX system headers need stdint.h and inttypes.h to be re-enterable when macro _STD_TYPES_T is defined

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists requested changes to this revision.
mclow.lists added a comment.
This revision now requires changes to proceed.

I see no tests here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59253/new/

https://reviews.llvm.org/D59253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59255: NOLINT support for "clang-diagnostic-*".

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: alexfh, aaron.ballman.
Herald added a project: clang.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59255

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint.cpp


Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -31,6 +31,7 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' 
[clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -47,4 +48,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -138,7 +138,7 @@
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
-  StringRef getCheckName(unsigned DiagnosticID) const;
+  std::string getCheckName(unsigned DiagnosticID) const;
 
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -254,7 +254,11 @@
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+  std::string ClangWarningOption =
+  DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
+  if (!ClangWarningOption.empty())
+return "clang-diagnostic-" + ClangWarningOption;
   llvm::DenseMap::const_iterator I =
   CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
@@ -305,7 +309,7 @@
   Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
   // Allow disabling all the checks with "*".
   if (ChecksStr != "*") {
-StringRef CheckName = Context.getCheckName(DiagID);
+std::string CheckName = Context.getCheckName(DiagID);
 // Allow specifying a few check names, delimited with comma.
 SmallVector Checks;
 ChecksStr.split(Checks, ',', -1, false);
@@ -402,13 +406,7 @@
"A diagnostic note can only be appended to a message.");
   } else {
 finalizeLastError();
-StringRef WarningOption =
-Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
-Info.getID());
-std::string CheckName = !WarningOption.empty()
-? ("clang-diagnostic-" + WarningOption).str()
-: Context.getCheckName(Info.getID()).str();
-
+std::string CheckName = Context.getCheckName(Info.getID());
 if (CheckName.empty()) {
   // This is a compiler diagnostic without a warning option. Assign check
   // name based on its level.


Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -31,6 +31,7 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -47,4 +48,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -138,7 +138,7 @@
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
-  StringRef getCheckName(unsigned DiagnosticID) const;
+  std::string getCheckName(unsigned DiagnosticID) const;
 
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -254,7 +254,11 @@
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+  std::string ClangWarningOption =
+  DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(Diag

[PATCH] D59255: [clang-tidy] NOLINT support for "clang-diagnostic-*".

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59255/new/

https://reviews.llvm.org/D59255



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added inline comments.



Comment at: clang/lib/AST/DeclBase.cpp:1262
+  // The last one in the chain should have a null next!
+  PrevDecl->NextInContextAndBits.setPointer(nullptr);
+

Apologies that this is included. I've left a comment on 
`clang/lib/AST/RecordFieldReorganizer.cpp` describing our issues with this 
code. I also mention experimenting with fixing it in that comment as well, and 
in those experiments we've been able to remove this change from BuildDeclChain. 
We're just waiting for further advice before we change the code too rapidly.



Comment at: clang/lib/AST/RecordFieldReorganizer.cpp:58
+  Decl *First, *Last;
+  std::tie(First, Last) = DeclContext::BuildDeclChain(
+  NewFieldOrder, D->hasLoadedFieldsFromExternalStorage());

This part of the code where we rebuild the new DeclChain with the new order of 
fields is resulting in a Clang compiler that segfaults when building the Linux 
kernel.

Any advice with how we can safely manage the DeclContext field chain is greatly 
appreciated here.

In our experimentation (which isn't present in this diff here) we thought our 
best bet to was to save the LastDecl's next pointer before BuildDeclChain is 
called and then we set the NEW LastDecl's next pointer to it in an effort to 
preserve whatever context chaining is happening.

Truthfully, we don't fully understand the machinery of the DeclContext or why 
our assumptions about this linked list of fields are incorrect.

More troubleshooting information regarding this particular instance is on our 
github issue: https://github.com/clang-randstruct/llvm-project/issues/42







Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [RFC 00/12] Introduce struct layout randomization feature

[Connor Kuehl via cfe-commits]

Thank you for the heads up! I put the patchset on Phabricator and sent 
the RFC out to the cfe-dev mailing list.


On 3/9/19 1:59 AM, Roman Lebedev wrote:

You probably want to submit this patchset to phabricator.
It will get lost in mailing list.

On Sat, Mar 9, 2019 at 1:38 AM Connor Kuehl via cfe-commits
 wrote:

This patch set introduces structure field layout randomization into the Clang
compiler. The Randstruct feature is a compile-time hardening technique that
randomizes the field layout for designated structures of a code base.
Admittedly, this is mostly useful for closed-source releases of code (since
the randomization seed would be available for public and open source application
s). However, this patch set also enhances Clang’s feature parity with that
of GCC which already has the Randstruct feature.

This patch set is a from-scratch reimplementation of the Randstruct feature
that was originally ported to GCC. The patches for this implementation in GCC
can be found here:

 https://www.openwall.com/lists/kernel-hardening/2017/04/06/14.

This feature identifies structures for randomization in two ways. The first
method targets structures that are manually marked with the new
“randomize_layout” attribute. The second is an optional feature that will
automatically select and randomize structures that are found to consist entirely
of function pointers. This automatic selection feature can be extended to
include other vulnerable structure types that are safe to randomize as they are
identified. You can also opt a specific structure out of this feature with the
“no_randomize_layout” attribute. Automatic structure selection is enabled with
the “-randstruct-auto” compiler flag. By default, Randstruct seeds on the empty
string, but a seed can be supplied with the “-randstruct-seed=” command line
argument.

This entire patch set is the sum total of an undergraduate computer science
capstone team’s effort.

Portland State University Clang Randstruct Capstone Team (Fall 2018-Winter 
2019):

Co-authored-by: Cole Nixon 
Co-authored-by: Connor Kuehl 
Co-authored-by: James Foster 
Co-authored-by: Jeff Takahashi 
Co-authored-by: Jordan Cantrell 
Co-authored-by: Nikk Forbus 
Co-authored-by: Tim Pugh 

Connor Kuehl (12):
   Add documentation for randstruct attributes
   Add randomize_layout attribute and handler
   Add no_randomize_layout attribute and handler
   Add randomize_layout warning for unions
   Add warning for mutually exclusive attributes
   Add globals to store command line arguments in
   Add randstruct-seed compiler argument
   Add automatic structure selection compiler switch
   Implement record field randomization algorithms
   Fix: Set tail pointer to null in field list
   Forward declare RecordFieldReorganizer
   Wire up Randstruct; intercept and randomize

  clang/include/clang/AST/Decl.h|   1 +
  clang/include/clang/AST/DeclBase.h|   2 +
  clang/include/clang/AST/RandstructSeed.h  |   8 +
  .../clang/AST/RecordFieldReorganizer.h|  59 
  clang/include/clang/Basic/Attr.td |  14 +
  clang/include/clang/Basic/AttrDocs.td |  45 +++
  .../include/clang/Basic/DiagnosticASTKinds.td |   5 +
  clang/include/clang/Driver/CC1Options.td  |   2 +
  clang/include/clang/Driver/Options.td |   4 +
  clang/lib/AST/CMakeLists.txt  |   1 +
  clang/lib/AST/DeclBase.cpp|   3 +
  clang/lib/AST/RecordFieldReorganizer.cpp  | 257 ++
  clang/lib/AST/RecordLayoutBuilder.cpp |  20 ++
  clang/lib/Driver/ToolChains/Clang.cpp |  10 +
  clang/lib/Frontend/CompilerInvocation.cpp |   8 +
  clang/lib/Sema/SemaDeclAttr.cpp   |   6 +
  ...a-attribute-supported-attributes-list.test |   2 +
  17 files changed, 447 insertions(+)
  create mode 100644 clang/include/clang/AST/RandstructSeed.h
  create mode 100644 clang/include/clang/AST/RecordFieldReorganizer.h
  create mode 100644 clang/lib/AST/RecordFieldReorganizer.cpp

--
2.17.1

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev added a comment.

Ping!
Should I add more FE guys to review this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58675/new/

https://reviews.llvm.org/D58675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355927 - [CMake] Tell libc++ that we're using compiler-rt on Apple platforms

[Louis Dionne via cfe-commits]

Author: ldionne
Date: Tue Mar 12 08:32:00 2019
New Revision: 355927

URL: http://llvm.org/viewvc/llvm-project?rev=355927&view=rev
Log:
[CMake] Tell libc++ that we're using compiler-rt on Apple platforms

Reviewers: beanz, arphaman, EricWF

Subscribers: dberris, mgorny, jkorous, dexonsmith, jdoerfert, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D58921

Modified:
cfe/trunk/cmake/caches/Apple-stage2.cmake

Modified: cfe/trunk/cmake/caches/Apple-stage2.cmake
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/cmake/caches/Apple-stage2.cmake?rev=355927&r1=355926&r2=355927&view=diff
==
--- cfe/trunk/cmake/caches/Apple-stage2.cmake (original)
+++ cfe/trunk/cmake/caches/Apple-stage2.cmake Tue Mar 12 08:32:00 2019
@@ -38,6 +38,7 @@ set(CMAKE_BUILD_TYPE RelWithDebInfo CACH
 set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
 set(LIBCXX_INSTALL_HEADERS ON CACHE BOOL "")
 set(LIBCXX_INCLUDE_TESTS OFF CACHE BOOL "")
+set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(LLVM_LTO_VERSION_OFFSET 3000 CACHE STRING "")
 
 # Generating Xcode toolchains is useful for developers wanting to build and use


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58921: [CMake] Tell libc++ that we're using compiler-rt on Apple platforms

[Louis Dionne via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC355927: [CMake] Tell libc++ that we're using 
compiler-rt on Apple platforms (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58921?vs=189192&id=190268#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58921/new/

https://reviews.llvm.org/D58921

Files:
  cmake/caches/Apple-stage2.cmake


Index: cmake/caches/Apple-stage2.cmake
===
--- cmake/caches/Apple-stage2.cmake
+++ cmake/caches/Apple-stage2.cmake
@@ -38,6 +38,7 @@
 set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
 set(LIBCXX_INSTALL_HEADERS ON CACHE BOOL "")
 set(LIBCXX_INCLUDE_TESTS OFF CACHE BOOL "")
+set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(LLVM_LTO_VERSION_OFFSET 3000 CACHE STRING "")
 
 # Generating Xcode toolchains is useful for developers wanting to build and use


Index: cmake/caches/Apple-stage2.cmake
===
--- cmake/caches/Apple-stage2.cmake
+++ cmake/caches/Apple-stage2.cmake
@@ -38,6 +38,7 @@
 set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
 set(LIBCXX_INSTALL_HEADERS ON CACHE BOOL "")
 set(LIBCXX_INCLUDE_TESTS OFF CACHE BOOL "")
+set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(LLVM_LTO_VERSION_OFFSET 3000 CACHE STRING "")
 
 # Generating Xcode toolchains is useful for developers wanting to build and use
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D56370#1424177 , @nridge wrote:

> Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a 
> fromJSON() implementation
>
> (Does the fact that this didn't cause any test failures suggest that the 
> fromJSON() functions aren't exercised by any tests?)


Unfortunately we usually test LSP layer with lit tests, and since we don't have 
any lit tests for this use-case it wasn't caught. Could you add a lit test for 
overall use case? You can see examples inside clang-tools-extra/test/clangd/ 
folder.

In D56370#1424180 , @nridge wrote:

> Unfortunately, there is a further problem: the Theia client-side 
> implementation of type hierarchy has recently merged, and their code has 
> changed so that they do require `typeHierarchy/resolve` to be supported. They 
> ask for 1 level in the initial request, ignore any extra levels the server 
> might send, and ask for extra levels using `typeHierarchy/resolve` instead.
>
> What should we do here -- seek to change Theia, or implement 
> `typeHierachy/resolve` for supertypes after all?


Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 
inside theia's implementation of this feature, I believe these are all subject 
to change.
So let's leave it as it is, even if it would mean you will be able to get only 
one level of parents knowledge in theia for now. I believe it should be 
addressed in theia's implementation and proposal itself(which is acutally 
addressed by sam, 
https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

nridge wrote:
> kadircet wrote:
> > nridge wrote:
> > > kadircet wrote:
> > > > Can you revert this change?
> > > I tried, but clang-format automatically makes the change again.
> > > 
> > > Is there some particular clang-format configuration I should apply, so it 
> > > produces the same results? I don't currently have any configuration, i.e. 
> > > I think it's just reading the .clang-format file in the monorepo root.
> > That's what we use as well(and your formatted version is correct), but 
> > unfortunately sometimes people send out changes with wrong formatting and 
> > they are hard to notice during review. Generally we try not to touch those 
> > lines in irrelevant patches to keep blame clean.
> > 
> > If you are running clang-format directly you can instead try 
> > clang-format-diff to format only changed lines. But not that crucial.
> I'm not running clang-format directly, VSCode's clang-format extension is 
> doing so automatically upon every file-save. I have not found an option to 
> configure it to format only changed lines.
> 
> Would it help if I moved the formatting corrections to their own patch (and 
> made this patch depend on that)?
it is not that important, nvm.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

I left some comments, but it is difficult for me to review without 
understanding what the requirements for this hasher are, why some information 
is hashed in, and some is left out, could you clarify?  See detailed comments.

> This implementation is covered by lit tests using it through c-index-test in 
> upcoming patch.

This code looks very unit-testable.  It also handles many corner cases (what 
information is hashed and what is left out).  c-index-tests are integration 
tests that are not as good at that, and also they would be testing this code 
quite indirectly.




Comment at: clang/lib/Index/IndexRecordHasher.cpp:175
+// There's a balance between caching results and not growing the cache too
+// much. Measurements showed that avoiding caching all decls is beneficial
+// particularly when including all of Cocoa.

"caching all decls" => "caching hashes for all decls"



Comment at: clang/lib/Index/IndexRecordHasher.cpp:191
+
+  auto asCanon = [](QualType Ty) -> CanQualType {
+return CanQualType::CreateUnsafe(Ty);

I don't think that hiding a "CreateUnsafe" in an operation with a benign name 
"asCanon" is a good idea.  Why not inline this lambda everywhere, it looks like 
it is defined to only save typing a few characters.



Comment at: clang/lib/Index/IndexRecordHasher.cpp:207
+if(qVal)
+  Hash = hash_combine(Hash, qVal);
+

What about other qualifiers?

Why not use `Qualifiers::getAsOpaqueValue()` instead of manually converting a 
subset of qualifiers to unsigned?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:209
+
+// Hash in ObjC GC qualifiers?
+

Is this a FIXME or?..



Comment at: clang/lib/Index/IndexRecordHasher.cpp:215
+if (const PointerType *PT = dyn_cast(T)) {
+  Hash = hash_combine(Hash, '*');
+  CT = asCanon(PT->getPointeeType());

Why not hash in `T->getTypeClass()` uniformly for all types, instead of 
inventing a random sigil?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:219
+}
+if (const ReferenceType *RT = dyn_cast(T)) {
+  Hash = hash_combine(Hash, '&');

There is LValueReferenceType and RValueReferenceType, do we care about the 
difference?  If not, why not?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:254
+
+break;
+  }

What about all other types -- array type, function type, decltype(), ...?

What about attributes?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:277
+template 
+hash_code IndexRecordHasher::tryCache(const void *Ptr, T Obj) {
+  auto It = HashByPtr.find(Ptr);

I had to read the implementation of this function to understand what it does.  
How about renaming it to "cachedHash()" ?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:291
+
+hash_code IndexRecordHasher::hashImpl(const Decl *D) {
+  return DeclHashVisitor(*this).Visit(D);

hashImpl => cachedHashImpl



Comment at: clang/lib/Index/IndexRecordHasher.cpp:409
+
+  // Unhandled type.
+  return Hash;

So what is the failure mode for unhandled types, what is the effect on the 
whole system?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:476
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+// Fall through to hash the type.
+

I'd suggest to remove this comment.  It is more confusing than helpful.  It 
makes the code look like there's some processing (like a "break" in other 
cases), but at a close inspection it turns out to be a comment.  This kind of 
fallthrough is pretty common and I don't think it requires a comment.  (For 
example, hashImpl above has this kind of fallthough, but does not have comments 
like this.)



Comment at: clang/lib/Index/IndexRecordHasher.h:31
+/// Implements hashing of AST nodes suitable for the index.
+/// Caching all produced hashes.
+class IndexRecordHasher {

Could you add some explanation about the information that is being hashed?  
What is the underlying principle behind choosing to hash some aspect of the AST 
node (or to skip it).  For example, I see you're hashing names, but not, say 
source locations.  Or that QualTypes are first translated into canonical types.

What are the completeness constraints for this hasher?  What happens if we 
don't hash something that we should have?

"Caching all produced hashes" seems like an implementation comment.  Especially 
since the implementation chooses not to cache some of the hashes.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.o

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1496
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(

Sorry for not pointing this out before, but it would be great if you could move 
related tests into a new file, XRefsTests is getting out of hand.

Feel free to ignore if you don't feel like it.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1834
+
+TEST(TypeHierarchy, RecursiveHierarchy1) {
+  Annotations Source(R"cpp(

Could you also check for the response, since parent traversal for these cases 
are also disabled currently(due to dependent types issue).
So that we won't forget to fix any issues regarding these cases. 
I suppose, a check with only current symbol and empty parents and children 
should be enough. Same for other two cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  ---*- python 
-*--===#
+#

ioeric wrote:
> I'd avoid abbreviation in the file name and the new directory name. `StdGen` 
> sounds a lot like a library ;)
> 
> I'd suggest something like `std-include-mapping/generate.py`
Personally I'd vote `StdGen`, and it follows llvm's `TableGen`.  The name 
`std-include-mapping/generate.py` seems too verbose...



Comment at: clangd/StdGen/StdGen.py:32
+
+STDGEN_CODE = """\
+//===-- StdGen'erated file --*- C++ 
-*-===//

ioeric wrote:
> nit: maybe `STDGEN_CODE_HEADER`?
renamed to `STDGEN_CODE_PREFIX`.



Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+exit("Path %s doesn't exist!" % cpp_reference_root)

ioeric wrote:
> why not check `exists(index_page_path)` instead?
I think either way works.



Comment at: clangd/StdSymbolMap.inc:8
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.

ioeric wrote:
> can we include the information about the HTML archive (e.g. STL version) from 
> which this table is generated? This would be useful for future maintenance.
The version of HTML archive is the date information, but this information is 
only present in the `.zip` name, we lose it after we unzip the `zip` file


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58345/new/

https://reviews.llvm.org/D58345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 190275.
hokein marked 13 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58345/new/

https://reviews.llvm.org/D58345

Files:
  clangd/StdGen/StdGen.py
  clangd/StdGen/test.py
  clangd/StdSymbolMap.inc
  clangd/index/CanonicalIncludes.cpp

Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -107,53 +107,20 @@
 
 void addSystemHeadersMapping(CanonicalIncludes *Includes) {
   static const std::vector> SymbolMap = {
-  {"std::addressof", ""},
-  // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},
-  {"std::basic_fstream", ""},
-  {"std::basic_ifstream", ""},
-  {"std::basic_ofstream", ""},
   {"std::filebuf", ""},
-  {"std::fstream", ""},
-  {"std::ifstream", ""},
-  {"std::ofstream", ""},
   {"std::wfilebuf", ""},
-  {"std::wfstream", ""},
-  {"std::wifstream", ""},
-  {"std::wofstream", ""},
-  {"std::basic_ios", ""},
-  {"std::ios", ""},
-  {"std::wios", ""},
-  {"std::basic_iostream", ""},
-  {"std::iostream", ""},
-  {"std::wiostream", ""},
   {"std::basic_istream", ""},
   {"std::istream", ""},
   {"std::wistream", ""},
-  {"std::istreambuf_iterator", ""},
-  {"std::ostreambuf_iterator", ""},
   {"std::basic_ostream", ""},
   {"std::ostream", ""},
   {"std::wostream", ""},
-  {"std::basic_istringstream", ""},
-  {"std::basic_ostringstream", ""},
-  {"std::basic_stringbuf", ""},
-  {"std::basic_stringstream", ""},
-  {"std::istringstream", ""},
-  {"std::ostringstream", ""},
-  {"std::string", ""},
-  {"std::stringbuf", ""},
-  {"std::stringstream", ""},
-  {"std::wistringstream", ""},
-  {"std::wostringstream", ""},
-  {"std::wstringbuf", ""},
-  {"std::wstringstream", ""},
-  {"std::basic_streambuf", ""},
-  {"std::streambuf", ""},
-  {"std::wstreambuf", ""},
   {"std::uint_least16_t", ""}, //  redeclares these
   {"std::uint_least32_t", ""},
-  {"std::declval", ""},
+#define SYMBOL(Name, NameSpace, Header) { #NameSpace#Name, #Header },
+  #include "StdSymbolMap.inc"
+#undef SYMBOL
   };
   for (const auto &Pair : SymbolMap)
 Includes->addSymbolMapping(Pair.first, Pair.second);
Index: clangd/StdSymbolMap.inc
===
--- /dev/null
+++ clangd/StdSymbolMap.inc
@@ -0,0 +1,1224 @@
+//===-- StdGen.py generated file *- C++ -*-===//
+//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
+//
+// Automatically generated file, do not edit.
+//===--===//
+
+SYMBOL(Assignable, std::, )
+SYMBOL(Boolean, std::, )
+SYMBOL(Common, std::, )
+SYMBOL(CommonReference, std::, )
+SYMBOL(Constructible, std::, )
+SYMBOL(ConvertibleTo, std::, )
+SYMBOL(CopyConstructible, std::, )
+SYMBOL(Copyable, std::, )
+SYMBOL(DefaultConstructible, std::, )
+SYMBOL(DerivedFrom, std::, )
+SYMBOL(Destructible, std::, )
+SYMBOL(EqualityComparable, std::, )
+SYMBOL(EqualityComparableWith, std::, )
+SYMBOL(FILE, std::, )
+SYMBOL(Integral, std::, )
+SYMBOL(Invocable, std::, )
+SYMBOL(Movable, std::, )
+SYMBOL(MoveConstructible, std::, )
+SYMBOL(Predicate, std::, )
+SYMBOL(Regular, std::, )
+SYMBOL(RegularInvocable, std::, )
+SYMBOL(Relation, std::, )
+SYMBOL(Same, std::, )
+SYMBOL(Semiregular, std::, )
+SYMBOL(SignedIntegral, std::, )
+SYMBOL(StrictTotallyOrdered, std::, )
+SYMBOL(StrictTotallyOrderedWith, std::, )
+SYMBOL(StrictWeakOrder, std::, )
+SYMBOL(Swappable, std::, )
+SYMBOL(SwappableWith, std::, )
+SYMBOL(UniformRandomBitGenerator, std::, )
+SYMBOL(UnsignedIntegral, std::, )
+SYMBOL(_Exit, std::, )
+SYMBOL(accumulate, std::, )
+SYMBOL(add_const, std::, )
+SYMBOL(add_const_t, std::, )
+SYMBOL(add_cv, std::, )
+SYMBOL(add_cv_t, std::, )
+SYMBOL(add_lvalue_reference, std::, )
+SYMBOL(add_lvalue_reference_t, std::, )
+SYMBOL(add_pointer, std::, )
+SYMBOL(add_pointer_t, std::, )
+SYMBOL(add_rvalue_reference, std::, )
+SYMBOL(add_rvalue_reference_t, std::, )
+SYMBOL(add_volatile, std::, )
+SYMBOL(add_volatile_t, std::, )
+SYMBOL(addressof, std::, )
+SYMBOL(adjacent_difference, std::, )
+SYMBOL(adjacent_find, std::, )
+SYMBOL(adopt_lock, std::, )
+SYMBOL(adopt_lock_t, std::, )
+SYMBOL(advance, std::, )
+SYMBOL(align, std::, )
+SYMBOL(align_val_t, std::, )
+SYMBOL(aligned_alloc, std::, )
+SYMBOL(aligned_storage, std::, )
+SYMBOL(aligned_storage_t, std::, )
+SYMBOL(aligned_union, std::, )
+SYMBOL(aligned_union_t, std::, )
+SYMBOL(alignment_of, std::, )
+SYMBOL(alignment_of_v, std::, )
+SYMBOL(all_of, std::, )
+SYMBOL(alloca

[clang-tools-extra] r355934 - [clang-tidy] NOLINT support for "clang-diagnostic-*".

[Haojian Wu via cfe-commits]

Author: hokein
Date: Tue Mar 12 09:11:46 2019
New Revision: 355934

URL: http://llvm.org/viewvc/llvm-project?rev=355934&view=rev
Log:
[clang-tidy] NOLINT support for "clang-diagnostic-*".

Reviewers: alexfh, aaron.ballman

Reviewed By: alexfh

Subscribers: xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59255

Modified:
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/trunk/test/clang-tidy/nolint.cpp

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=355934&r1=355933&r2=355934&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Tue Mar 
12 09:11:46 2019
@@ -254,7 +254,11 @@ bool ClangTidyContext::treatAsError(Stri
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+  std::string ClangWarningOption =
+  DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
+  if (!ClangWarningOption.empty())
+return "clang-diagnostic-" + ClangWarningOption;
   llvm::DenseMap::const_iterator I =
   CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
@@ -305,7 +309,7 @@ static bool IsNOLINTFound(StringRef Noli
   Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
   // Allow disabling all the checks with "*".
   if (ChecksStr != "*") {
-StringRef CheckName = Context.getCheckName(DiagID);
+std::string CheckName = Context.getCheckName(DiagID);
 // Allow specifying a few check names, delimited with comma.
 SmallVector Checks;
 ChecksStr.split(Checks, ',', -1, false);
@@ -402,13 +406,7 @@ void ClangTidyDiagnosticConsumer::Handle
"A diagnostic note can only be appended to a message.");
   } else {
 finalizeLastError();
-StringRef WarningOption =
-Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
-Info.getID());
-std::string CheckName = !WarningOption.empty()
-? ("clang-diagnostic-" + WarningOption).str()
-: Context.getCheckName(Info.getID()).str();
-
+std::string CheckName = Context.getCheckName(Info.getID());
 if (CheckName.empty()) {
   // This is a compiler diagnostic without a warning option. Assign check
   // name based on its level.

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=355934&r1=355933&r2=355934&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h Tue Mar 12 
09:11:46 2019
@@ -138,7 +138,7 @@ public:
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
-  StringRef getCheckName(unsigned DiagnosticID) const;
+  std::string getCheckName(unsigned DiagnosticID) const;
 
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///

Modified: clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/nolint.cpp?rev=355934&r1=355933&r2=355934&view=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/nolint.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/nolint.cpp Tue Mar 12 09:11:46 2019
@@ -31,6 +31,7 @@ void f() {
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' 
[clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -47,4 +48,4 @@ MACRO_NOLINT
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59255: [clang-tidy] NOLINT support for "clang-diagnostic-*".

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355934: [clang-tidy] NOLINT support for 
"clang-diagnostic-*". (authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59255/new/

https://reviews.llvm.org/D59255

Files:
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/trunk/test/clang-tidy/nolint.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
@@ -31,6 +31,7 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' 
[clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -47,4 +48,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -254,7 +254,11 @@
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+  std::string ClangWarningOption =
+  DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
+  if (!ClangWarningOption.empty())
+return "clang-diagnostic-" + ClangWarningOption;
   llvm::DenseMap::const_iterator I =
   CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
@@ -305,7 +309,7 @@
   Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
   // Allow disabling all the checks with "*".
   if (ChecksStr != "*") {
-StringRef CheckName = Context.getCheckName(DiagID);
+std::string CheckName = Context.getCheckName(DiagID);
 // Allow specifying a few check names, delimited with comma.
 SmallVector Checks;
 ChecksStr.split(Checks, ',', -1, false);
@@ -402,13 +406,7 @@
"A diagnostic note can only be appended to a message.");
   } else {
 finalizeLastError();
-StringRef WarningOption =
-Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
-Info.getID());
-std::string CheckName = !WarningOption.empty()
-? ("clang-diagnostic-" + WarningOption).str()
-: Context.getCheckName(Info.getID()).str();
-
+std::string CheckName = Context.getCheckName(Info.getID());
 if (CheckName.empty()) {
   // This is a compiler diagnostic without a warning option. Assign check
   // name based on its level.
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -138,7 +138,7 @@
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
-  StringRef getCheckName(unsigned DiagnosticID) const;
+  std::string getCheckName(unsigned DiagnosticID) const;
 
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///


Index: clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
@@ -31,6 +31,7 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -47,4 +48,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -254,7 +254,11 @@
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+std::string ClangTidyContext::getCheckName(unsigned DiagnosticID

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  ---*- python 
-*--===#
+#

hokein wrote:
> ioeric wrote:
> > I'd avoid abbreviation in the file name and the new directory name. 
> > `StdGen` sounds a lot like a library ;)
> > 
> > I'd suggest something like `std-include-mapping/generate.py`
> Personally I'd vote `StdGen`, and it follows llvm's `TableGen`.  The name 
> `std-include-mapping/generate.py` seems too verbose...
Why do we want to follow `TableGen`?

Two reasons why I think a more informative name should be used. 1) `StdGen` 
sounds like a name for library and doesn't follow the naming convention for 
tool directory (e.g. `global-symbol-builder`) and 2) it's hard for readers 
without much context to understand what `StdGen` actually means.



Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+exit("Path %s doesn't exist!" % cpp_reference_root)

hokein wrote:
> ioeric wrote:
> > why not check `exists(index_page_path)` instead?
> I think either way works.
It seems to me that using `exists(index_page_path)` would be less likely to 
trigger an exception (better use experience). I don't see a good reason to 
check the root.



Comment at: clangd/StdSymbolMap.inc:8
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.

hokein wrote:
> ioeric wrote:
> > can we include the information about the HTML archive (e.g. STL version) 
> > from which this table is generated? This would be useful for future 
> > maintenance.
> The version of HTML archive is the date information, but this information is 
> only present in the `.zip` name, we lose it after we unzip the `zip` file
If we can't get this from the doc, we should ask users to explicitly provide 
this information. I think we would want some version info to relate the output 
to the source.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58345/new/

https://reviews.llvm.org/D58345



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Probably @rnk needs to look at this, i'll ping him today.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek created this revision.
phosek added a reviewer: echristo.
Herald added subscribers: cfe-commits, jdoerfert, fedor.sergeev, dberris, 
srhines.
Herald added a project: clang.
phosek edited the summary of this revision.

When compiler-rt is selected as the runtime library for Linux
use its crtbegin.o/crtend.o implementation rather than platform one.


Repository:
  rC Clang

https://reviews.llvm.org/D59264

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/linux-ld.c

Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -2,7 +2,7 @@
 // sysroot to make these tests independent of the host system.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux \
+// RUN: --target=i386-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-32 %s
@@ -51,16 +51,18 @@
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT %s
 // CHECK-LD-RT-NOT: warning:
+// CHECK-LD-RT: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT: "--eh-frame-hdr"
 // CHECK-LD-RT: "-m" "elf_x86_64"
 // CHECK-LD-RT: "-dynamic-linker"
-// CHECK-LD-RT: "{{.*}}/usr/lib/gcc/x86_64-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-x86_64.o"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../../../x86_64-unknown-linux/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../.."
@@ -69,19 +71,22 @@
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
 // CHECK-LD-RT: "-lc"
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-x86_64.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=i686-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT-I686 %s
 // CHECK-LD-RT-I686-NOT: warning:
+// CHECK-LD-RT-I686: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT-I686: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT-I686: "--eh-frame-hdr"
 // CHECK-LD-RT-I686: "-m" "elf_i386"
 // CHECK-LD-RT-I686: "-dynamic-linker"
-// CHECK-LD-RT-I686: "{{.*}}/usr/lib/gcc/i686-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-i386.o"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../../../i686-unknown-linux/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../.."
@@ -90,6 +95,7 @@
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
 // CHECK-LD-RT-I686: "-lc"
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-i386.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-androideabi \
@@ -110,7 +116,6 @@
 // RUN: --target=x86_64-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
-// RUN: --rtlib=libgcc \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-GCC %s
 // CHECK-LD-GCC-NOT: warning:
 // CHECK-LD-GCC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
@@ -291,7 +296,7 @@
 // RUN:   | FileCheck --check-prefix=CHECK-LD-64-STATIC %s
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m32 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m32 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-32 %s
@@ -308,7 +313,7 @@
 // CHECK-32-TO-32: "-L[[SYSROOT]]/usr/lib"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m64 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m64 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-64 %s
@@ -326,7 +331,7 @@
 // CHECK-32-TO-64: "-L[[SYSROOT]]/usr/lib"
 //
 // RUN: %cl

[PATCH] D59118: creduce script for clang crashes

[Amy Huang via Phabricator via cfe-commits]

akhuang updated this revision to Diff 190285.
akhuang marked 6 inline comments as done.
akhuang added a comment.

style edits


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118

Files:
  clang/utils/creduce-clang-crash.py

Index: clang/utils/creduce-clang-crash.py
===
--- /dev/null
+++ clang/utils/creduce-clang-crash.py
@@ -0,0 +1,118 @@
+#!/usr/bin/env python
+"""Calls C-Reduce to create a minimal reproducer for clang crashes.
+
+Requires C-Reduce and not (part of LLVM utils) to be installed.
+"""
+
+from argparse import ArgumentParser
+import os
+import re
+import stat
+import sys
+import subprocess
+import pipes
+from distutils.spawn import find_executable
+
+def create_test(build_script, llvm_not):
+  """
+  Create an interestingness test from the crash output.
+  Return as a string.
+  """
+  # Get clang call from build script
+  # Assumes the call is the last line of the script
+  with open(build_script) as f:
+cmd = f.readlines()[-1].rstrip('\n\r')
+
+  # Get crash output
+  p = subprocess.Popen(build_script,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
+
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
+  cmd))
+
+  # Add messages from crash output to the test
+  # If there is an Assertion failure, use that; otherwise use the
+  # last five stack trace functions
+  assertion_re = r'Assertion `([^\']+)\' failed'
+  assertion_match = re.search(assertion_re, crash_output)
+  if assertion_match:
+msg = assertion_match.group(1)
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+  else:
+stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
+matches = re.findall(stacktrace_re, crash_output)
+del matches[:-5]
+output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+
+  return output
+
+def main():
+  parser = ArgumentParser(description=__doc__)
+  parser.add_argument('build_script', type=str, nargs=1,
+  help='Name of the script that generates the crash.')
+  parser.add_argument('file_to_reduce', type=str, nargs=1,
+  help='Name of the file to be reduced.')
+  parser.add_argument('-o', '--output', dest='output', type=str,
+  help='Name of the output file for the reduction. Optional.')
+  parser.add_argument('--llvm-not', dest='llvm_not', type=str,
+  help="The path to the llvm-not executable. "
+  "Required if 'not' is not in PATH environment.");
+  parser.add_argument('--creduce', dest='creduce', type=str,
+  help="The path to the C-Reduce executable. "
+  "Required if 'creduce' is not in PATH environment.");
+  args = parser.parse_args()
+
+  build_script = os.path.abspath(args.build_script[0])
+  file_to_reduce = os.path.abspath(args.file_to_reduce[0])
+  llvm_not = (find_executable(args.llvm_not) if args.llvm_not else
+  find_executable('not'))
+  creduce = (find_executable(args.creduce) if args.creduce else
+ find_executable('creduce'))
+
+  if not os.path.isfile(build_script):
+print(("ERROR: input file '%s' does not exist") % build_script)
+return 1
+
+  if not os.path.isfile(file_to_reduce):
+print(("ERROR: input file '%s' does not exist") % file_to_reduce)
+return 1
+
+  if not llvm_not:
+parser.print_help()
+return 1
+
+  if not creduce:
+parser.print_help()
+return 1
+
+  # Write interestingness test to file
+  test_contents = create_test(build_script, llvm_not)
+  testname, _ = os.path.splitext(file_to_reduce)
+  testfile = testname + '.test.sh'
+  with open(testfile, 'w') as f:
+f.write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
+
+  # Confirm that the interestingness test passes
+  try:
+with open(os.devnull, 'w') as devnull:
+  subprocess.check_call(testfile, stdout=devnull)
+  except subprocess.CalledProcessError:
+print("For some reason the interestingness test does not return zero")
+return 1
+
+  # FIXME: try running clang preprocessor first
+
+  try:
+p = subprocess.Popen([creduce, testfile, file_to_reduce])
+p.communicate()
+  except KeyboardInterrupt:
+# Hack to kill C-Reduce because it jumps into its own pgid
+print('\n\nctrl-c detected, killed creduce')
+p.kill()
+
+if __name__ == '__main__':
+  sys.exit(main())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59219: [PR41007][OpenCL] Allow printf and toolchain reserved variadic functions in C++

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: cfe/trunk/test/SemaOpenCL/extensions.cl:31
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
 

Shoudn't we move this test to test/SemaOpenCLCXX?
Does `-finclude-default-header` include opencl-c.h? I think it's an overkill to 
test that OpenCL C++ allows printf. Ideally we should minimize number of times 
we parsing 11500+ lines header in LIT tests.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59219/new/

https://reviews.llvm.org/D59219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

I think that addresses all of the concerns people have put forward; given rnk's 
comment about one more round of fixes, this LGTM. Will check this in for you 
shortly.

Thanks again!




Comment at: clang/utils/creduce-clang-crash.py:1
+#!/usr/bin/env python
+# A tool that calls C-Reduce to create a minimal reproducer for clang crashes

thakis wrote:
> rnk wrote:
> > george.burgess.iv wrote:
> > > nit: please specify a python version here, since the world is steadily 
> > > making `python` == `python3` (if `pipes.quote` is working, I assume 
> > > you'll want `#!/usr/bin/env python2`)
> > Please don't do that, there is no python2.exe or python3.exe on Windows. 
> > `#!/usr/bin/env python` is the simplest thing that could work.
> There's no python2 on mac either. `#!/usr/bin/env python` is the correct 
> first line for executable python scripts.
TIL. Thank you both for the counterexamples :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: steveire, aaron.ballman.
lebedev.ri added a comment.

Thanks for taking a look, some replies.




Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional getStructuredBlock() const;
 };

gribozavr wrote:
> Why not `Stmt *` as a return value?
Good question.

Because usually checking a pointer for null is an exceptional
case, you //normally// have the AST node, you don't check
**every single** pointer for `null`, i don't think?

This is absolutely not the case here,  `null` is **not** an
exceptional failure state here, it's the expected return value
for stand-alone directives. With `Optional`, you explicitly
signal "hey, i'm not a plain pointer, pay attention!",
thus hopefully maybe preventing some issues.

I'm not sure we can more nicely solve this with an extra base class[es],
at least not without having only two direct subclasses of 
`OMPExecutableDirective`:
* `OMPNonstandaloneExecutableDirective`.
* `OMPStandaloneExecutableDirective`.
and two different `OMPOrderedDirective` classes (one standalone, one not).
But somehow i don't think there is any chance of that being accepted,
even though it would result in slightly cleaner AST.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

ABataev wrote:
> No need to insert it into each class, just add:
> ```
> Stmt * OMPExecutableDirective::getStructuredBlock() const {
>   if (!hasAssociatedStmt() || !getAssociatedStmt())
> return nullptr;
>   if (auto *LD = dyn_cast(this))
> return LD->getBody();
>   return getInnermostCapturedStmt()->getCapturedStmt();
> }
> ```
I absolutely can do that, you are sure that is the most future-proof state?
In particular, i want to re-point-out that if it's implemented like this,
in the base class, then the sub-class may(will) not even know about this 
function,
and thus 'forget' to update it, should it not be giving the correct answer for
that new specific OpenMP executable directive.

You are sure it's better to implement it in the `OMPExecutableDirective` itself?



Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif

gribozavr wrote:
> Why not add a default definition:
> 
> ```
> #  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
> ```
> 
> (Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)
Hm, i have never seen that chaining pattern in `.def` headers before,
so it could be surprising behavior.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

gribozavr wrote:
> I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
> difficult to read (when they are 700 lines long), and it is unclear what the 
> important parts are.
> 
> WDYT about converting them to unit tests?  See 
> `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> They are difficult to update.

The updating part is true, for all of the clang tests,
they unlike llvm tests have no update scripts, so it's a major pain.
Here, it's is actually reasonably simple:
1. prune old `// CHECK*` lines
2. run the run line
3. prepend each line of output with `// CHECK-NEXT: `
4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
5. scrub filepath prefix, again a simple string-replace
6. prune everything after `TranslationUnitDecl` and before first `FunctionDecl`
7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
8. append the final processed output to the test file. done.

It would not be too hard to write an update tool for this, but i've managed 
with a simple macro..

> They are fragile, difficult to read (when they are 700 lines long),
> and it is unclear what the important parts are.

This is kind-of intentional. I've tried to follow the llvm-proper approach of 
gigantic 
tests that are overly verbose, but quickly cement the state so **any** change 
**will**
be noticed. That has been a **very** successful approach for LLVM.

In fact, the lack of this ast-dumper test coverage was not helping
the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.

So no, i really don't want to //convert// the tests into a less verbose state.

I suppose i //could// //add// a more fine-grained tests, 
though readability is arguable. Here, it is obvious nothing is omitted,
and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
With more distilled tests, it's less obvious. (i'm not talking about 
`StmtPrinterTest`)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://revie

r355944 - Add a creduce script for clang crashes

[George Burgess IV via cfe-commits]

Author: gbiv
Date: Tue Mar 12 10:48:53 2019
New Revision: 355944

URL: http://llvm.org/viewvc/llvm-project?rev=355944&view=rev
Log:
Add a creduce script for clang crashes

This CL adds a script that calls C-Reduce on an input file and given the
clang crash script, which is used to generate an interestingness test
for C-Reduce.

Patch by Amy Huang!

Differential Revision: https://reviews.llvm.org/D59118

Added:
cfe/trunk/utils/creduce-clang-crash.py

Added: cfe/trunk/utils/creduce-clang-crash.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/creduce-clang-crash.py?rev=355944&view=auto
==
--- cfe/trunk/utils/creduce-clang-crash.py (added)
+++ cfe/trunk/utils/creduce-clang-crash.py Tue Mar 12 10:48:53 2019
@@ -0,0 +1,118 @@
+#!/usr/bin/env python
+"""Calls C-Reduce to create a minimal reproducer for clang crashes.
+
+Requires C-Reduce and not (part of LLVM utils) to be installed.
+"""
+
+from argparse import ArgumentParser
+import os
+import re
+import stat
+import sys
+import subprocess
+import pipes
+from distutils.spawn import find_executable
+
+def create_test(build_script, llvm_not):
+  """
+  Create an interestingness test from the crash output.
+  Return as a string.
+  """
+  # Get clang call from build script
+  # Assumes the call is the last line of the script
+  with open(build_script) as f:
+cmd = f.readlines()[-1].rstrip('\n\r')
+
+  # Get crash output
+  p = subprocess.Popen(build_script,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
+
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
+  cmd))
+
+  # Add messages from crash output to the test
+  # If there is an Assertion failure, use that; otherwise use the
+  # last five stack trace functions
+  assertion_re = r'Assertion `([^\']+)\' failed'
+  assertion_match = re.search(assertion_re, crash_output)
+  if assertion_match:
+msg = assertion_match.group(1)
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+  else:
+stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
+matches = re.findall(stacktrace_re, crash_output)
+del matches[:-5]
+output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+
+  return output
+
+def main():
+  parser = ArgumentParser(description=__doc__)
+  parser.add_argument('build_script', type=str, nargs=1,
+  help='Name of the script that generates the crash.')
+  parser.add_argument('file_to_reduce', type=str, nargs=1,
+  help='Name of the file to be reduced.')
+  parser.add_argument('-o', '--output', dest='output', type=str,
+  help='Name of the output file for the reduction. 
Optional.')
+  parser.add_argument('--llvm-not', dest='llvm_not', type=str,
+  help="The path to the llvm-not executable. "
+  "Required if 'not' is not in PATH environment.");
+  parser.add_argument('--creduce', dest='creduce', type=str,
+  help="The path to the C-Reduce executable. "
+  "Required if 'creduce' is not in PATH environment.");
+  args = parser.parse_args()
+
+  build_script = os.path.abspath(args.build_script[0])
+  file_to_reduce = os.path.abspath(args.file_to_reduce[0])
+  llvm_not = (find_executable(args.llvm_not) if args.llvm_not else
+  find_executable('not'))
+  creduce = (find_executable(args.creduce) if args.creduce else
+ find_executable('creduce'))
+
+  if not os.path.isfile(build_script):
+print(("ERROR: input file '%s' does not exist") % build_script)
+return 1
+
+  if not os.path.isfile(file_to_reduce):
+print(("ERROR: input file '%s' does not exist") % file_to_reduce)
+return 1
+
+  if not llvm_not:
+parser.print_help()
+return 1
+
+  if not creduce:
+parser.print_help()
+return 1
+
+  # Write interestingness test to file
+  test_contents = create_test(build_script, llvm_not)
+  testname, _ = os.path.splitext(file_to_reduce)
+  testfile = testname + '.test.sh'
+  with open(testfile, 'w') as f:
+f.write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
+
+  # Confirm that the interestingness test passes
+  try:
+with open(os.devnull, 'w') as devnull:
+  subprocess.check_call(testfile, stdout=devnull)
+  except subprocess.CalledProcessError:
+print("For some reason the interestingness test does not return zero")
+return 1
+
+  # FIXME: try running clang preprocessor first
+
+  try:
+p = subprocess.Popen([creduce, testfile, file_to_reduce])
+p.communicate()
+  except KeyboardInterrupt:
+# Hack to kill C-Reduce because it jumps into its own pgid
+print('\n\nctrl-c detected, killed creduce')
+p.kill()
+
+if __

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355944: Add a creduce script for clang crashes (authored by 
gbiv, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59118?vs=190285&id=190294#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118

Files:
  utils/creduce-clang-crash.py

Index: utils/creduce-clang-crash.py
===
--- utils/creduce-clang-crash.py
+++ utils/creduce-clang-crash.py
@@ -0,0 +1,118 @@
+#!/usr/bin/env python
+"""Calls C-Reduce to create a minimal reproducer for clang crashes.
+
+Requires C-Reduce and not (part of LLVM utils) to be installed.
+"""
+
+from argparse import ArgumentParser
+import os
+import re
+import stat
+import sys
+import subprocess
+import pipes
+from distutils.spawn import find_executable
+
+def create_test(build_script, llvm_not):
+  """
+  Create an interestingness test from the crash output.
+  Return as a string.
+  """
+  # Get clang call from build script
+  # Assumes the call is the last line of the script
+  with open(build_script) as f:
+cmd = f.readlines()[-1].rstrip('\n\r')
+
+  # Get crash output
+  p = subprocess.Popen(build_script,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
+
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
+  cmd))
+
+  # Add messages from crash output to the test
+  # If there is an Assertion failure, use that; otherwise use the
+  # last five stack trace functions
+  assertion_re = r'Assertion `([^\']+)\' failed'
+  assertion_match = re.search(assertion_re, crash_output)
+  if assertion_match:
+msg = assertion_match.group(1)
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+  else:
+stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
+matches = re.findall(stacktrace_re, crash_output)
+del matches[:-5]
+output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+
+  return output
+
+def main():
+  parser = ArgumentParser(description=__doc__)
+  parser.add_argument('build_script', type=str, nargs=1,
+  help='Name of the script that generates the crash.')
+  parser.add_argument('file_to_reduce', type=str, nargs=1,
+  help='Name of the file to be reduced.')
+  parser.add_argument('-o', '--output', dest='output', type=str,
+  help='Name of the output file for the reduction. Optional.')
+  parser.add_argument('--llvm-not', dest='llvm_not', type=str,
+  help="The path to the llvm-not executable. "
+  "Required if 'not' is not in PATH environment.");
+  parser.add_argument('--creduce', dest='creduce', type=str,
+  help="The path to the C-Reduce executable. "
+  "Required if 'creduce' is not in PATH environment.");
+  args = parser.parse_args()
+
+  build_script = os.path.abspath(args.build_script[0])
+  file_to_reduce = os.path.abspath(args.file_to_reduce[0])
+  llvm_not = (find_executable(args.llvm_not) if args.llvm_not else
+  find_executable('not'))
+  creduce = (find_executable(args.creduce) if args.creduce else
+ find_executable('creduce'))
+
+  if not os.path.isfile(build_script):
+print(("ERROR: input file '%s' does not exist") % build_script)
+return 1
+
+  if not os.path.isfile(file_to_reduce):
+print(("ERROR: input file '%s' does not exist") % file_to_reduce)
+return 1
+
+  if not llvm_not:
+parser.print_help()
+return 1
+
+  if not creduce:
+parser.print_help()
+return 1
+
+  # Write interestingness test to file
+  test_contents = create_test(build_script, llvm_not)
+  testname, _ = os.path.splitext(file_to_reduce)
+  testfile = testname + '.test.sh'
+  with open(testfile, 'w') as f:
+f.write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
+
+  # Confirm that the interestingness test passes
+  try:
+with open(os.devnull, 'w') as devnull:
+  subprocess.check_call(testfile, stdout=devnull)
+  except subprocess.CalledProcessError:
+print("For some reason the interestingness test does not return zero")
+return 1
+
+  # FIXME: try running clang preprocessor first
+
+  try:
+p = subprocess.Popen([creduce, testfile, file_to_reduce])
+p.communicate()
+  except KeyboardInterrupt:
+# Hack to kill C-Reduce because it jumps into its own pgid
+print('\n\nctrl-c detected, killed creduce')
+p.kill()
+
+if __name__ == '__main__':
+  sys.exit(main())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

lebedev.ri wrote:
> ABataev wrote:
> > No need to insert it into each class, just add:
> > ```
> > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > return nullptr;
> >   if (auto *LD = dyn_cast(this))
> > return LD->getBody();
> >   return getInnermostCapturedStmt()->getCapturedStmt();
> > }
> > ```
> I absolutely can do that, you are sure that is the most future-proof state?
> In particular, i want to re-point-out that if it's implemented like this,
> in the base class, then the sub-class may(will) not even know about this 
> function,
> and thus 'forget' to update it, should it not be giving the correct answer for
> that new specific OpenMP executable directive.
> 
> You are sure it's better to implement it in the `OMPExecutableDirective` 
> itself?
Yes, I'm sure. It is the universal solution and all future classes must be 
compatible with it. If they are not, then they are incorrect.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional getStructuredBlock() const;
 };

lebedev.ri wrote:
> gribozavr wrote:
> > Why not `Stmt *` as a return value?
> Good question.
> 
> Because usually checking a pointer for null is an exceptional
> case, you //normally// have the AST node, you don't check
> **every single** pointer for `null`, i don't think?
> 
> This is absolutely not the case here,  `null` is **not** an
> exceptional failure state here, it's the expected return value
> for stand-alone directives. With `Optional`, you explicitly
> signal "hey, i'm not a plain pointer, pay attention!",
> thus hopefully maybe preventing some issues.
> 
> I'm not sure we can more nicely solve this with an extra base class[es],
> at least not without having only two direct subclasses of 
> `OMPExecutableDirective`:
> * `OMPNonstandaloneExecutableDirective`.
> * `OMPStandaloneExecutableDirective`.
> and two different `OMPOrderedDirective` classes (one standalone, one not).
> But somehow i don't think there is any chance of that being accepted,
> even though it would result in slightly cleaner AST.
> Because usually checking a pointer for null is an exceptional
> case, you normally have the AST node, you don't check
> every single pointer for null, i don't think?

When deciding whether to check for null, we look at the function contract and 
check those pointers that are nullable.  For example, `IfStmt::getThen()` does 
not return NULL in valid ASTs, but `IfStmt::getElse()` does.

> This is absolutely not the case here,  null is not an
> exceptional failure state here, it's the expected return value
> for stand-alone directives.

Yes, I understand what optional is and how a new codebase could use it with 
pointers.  However, LLVM and Clang code has not been using optional to indicate 
nullability for pointers.  I could only find 136 occurrences of 
`Optional<.*\*>` in llvm-project.git.



Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif

lebedev.ri wrote:
> gribozavr wrote:
> > Why not add a default definition:
> > 
> > ```
> > #  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
> > ```
> > 
> > (Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)
> Hm, i have never seen that chaining pattern in `.def` headers before,
> so it could be surprising behavior.
It is used in Clang, for example, in 
`llvm/tools/clang/include/clang/AST/TypeNodes.def`, or 
`llvm/tools/clang/include/clang/AST/BuiltinTypes.def`.  I was surprised that 
this file does not use this pattern, to be honest.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

lebedev.ri wrote:
> gribozavr wrote:
> > I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
> > difficult to read (when they are 700 lines long), and it is unclear what 
> > the important parts are.
> > 
> > WDYT about converting them to unit tests?  See 
> > `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > They are difficult to update.
> 
> The updating part is true, for all of the clang tests,
> they unlike llvm tests have no update scripts, so it's a major pain.
> Here, it's is actually reasonably simple:
> 1. prune old `// CHECK*` lines
> 2. run the run line
> 3. prepend each line of output with `// CHECK-NEXT: `
> 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> 5. scrub filepath prefix, again a simple string-replace
> 6. prune everything after `TranslationUnitDecl` and before first 
> `FunctionDecl`
> 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> 8. append the final processed output to the test file. done.
> 
> It would not be too hard to write an update tool for this, but i've managed 
> with a simple macro..
> 
> > They are fragile, difficult to read (when they are 700 lines long),
> > and it is unclear what the important parts are.
> 
> This is kind-of intentional. I've tried to follow the llvm-proper approach of 
> gigantic 
> tests that are overly verbose, but quickly cement the state so **any** change 
> **will**
> be noticed. That has been a **very** successful approach for LLVM.
> 
> In fact, the lack of this ast-dumper test coverage was not helping
> the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> 
> So no, i really don't want to //convert// the tests into a less verbose state.
> 
> I suppose i //could// //add// a more fine-grained tests, 
> though readability is arguable. Here, it is obvious nothing is omitted,
> and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
> With more distilled tests, it's less obvious

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

ABataev wrote:
> lebedev.ri wrote:
> > ABataev wrote:
> > > No need to insert it into each class, just add:
> > > ```
> > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > return nullptr;
> > >   if (auto *LD = dyn_cast(this))
> > > return LD->getBody();
> > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > }
> > > ```
> > I absolutely can do that, you are sure that is the most future-proof state?
> > In particular, i want to re-point-out that if it's implemented like this,
> > in the base class, then the sub-class may(will) not even know about this 
> > function,
> > and thus 'forget' to update it, should it not be giving the correct answer 
> > for
> > that new specific OpenMP executable directive.
> > 
> > You are sure it's better to implement it in the `OMPExecutableDirective` 
> > itself?
> Yes, I'm sure. It is the universal solution and all future classes must be 
> compatible with it. If they are not, then they are incorrect.
Aha! Well, ok then.

Do you also suggest that `Optional<>` is too fancy?
Would it be better to do this instead?
```
bool isStandaloneDirective() const {
  return !hasAssociatedStmt() || !getAssociatedStmt();
}

// Requires: !isStandaloneDirective()
Stmt *OMPExecutableDirective::getStructuredBlock() const {
  assert(!isStandaloneDirective() && "Standalone Executable OpenMP directives 
don't have structured blocks.")
  if (auto *LD = dyn_cast(this))
return LD->getBody();
  return getInnermostCapturedStmt()->getCapturedStmt();
}
```
Hm, maybe that actually conveys more meaning..


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355950 - Modules: Add LangOptions::CacheGeneratedPCH

[Duncan P. N. Exon Smith via cfe-commits]

Author: dexonsmith
Date: Tue Mar 12 11:38:04 2019
New Revision: 355950

URL: http://llvm.org/viewvc/llvm-project?rev=355950&view=rev
Log:
Modules: Add LangOptions::CacheGeneratedPCH

Add an option to cache the generated PCH in the ModuleCache when
emitting it.  This protects clients that build PCHs and read them in the
same process, allowing them to avoid race conditions between parallel
jobs the same way that Clang's implicit module build system does.

rdar://problem/48740787

Modified:
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Frontend/FrontendActions.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/lib/Serialization/GeneratePCH.cpp
cfe/trunk/unittests/Frontend/FrontendActionTest.cpp

Modified: cfe/trunk/include/clang/Basic/LangOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=355950&r1=355949&r2=355950&view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.def Tue Mar 12 11:38:04 2019
@@ -154,6 +154,7 @@ BENIGN_ENUM_LANGOPT(CompilingModule, Com
 "compiling a module interface")
 BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
 BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a 
corresponding object file")
+BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
 COMPATIBLE_LANGOPT(ModulesDeclUse, 1, 0, "require declaration of module 
uses")
 BENIGN_LANGOPT(ModulesSearchAll  , 1, 1, "searching even non-imported modules 
to find unresolved references")
 COMPATIBLE_LANGOPT(ModulesStrictDeclUse, 1, 0, "requiring declaration of 
module uses and all headers to be in modules")

Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=355950&r1=355949&r2=355950&view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Tue Mar 12 11:38:04 2019
@@ -570,7 +570,8 @@ public:
   /// the module but currently is merely a random 32-bit number.
   ASTFileSignature WriteAST(Sema &SemaRef, const std::string &OutputFile,
 Module *WritingModule, StringRef isysroot,
-bool hasErrors = false);
+bool hasErrors = false,
+bool ShouldCacheASTInMemory = false);
 
   /// Emit a token.
   void AddToken(const Token &Tok, RecordDataImpl &Record);
@@ -974,6 +975,7 @@ class PCHGenerator : public SemaConsumer
   llvm::BitstreamWriter Stream;
   ASTWriter Writer;
   bool AllowASTWithErrors;
+  bool ShouldCacheASTInMemory;
 
 protected:
   ASTWriter &getWriter() { return Writer; }
@@ -985,7 +987,8 @@ public:
StringRef OutputFile, StringRef isysroot,
std::shared_ptr Buffer,
ArrayRef> Extensions,
-   bool AllowASTWithErrors = false, bool IncludeTimestamps = true);
+   bool AllowASTWithErrors = false, bool IncludeTimestamps = true,
+   bool ShouldCacheASTInMemory = false);
   ~PCHGenerator() override;
 
   void InitializeSema(Sema &S) override { SemaPtr = &S; }

Modified: cfe/trunk/lib/Frontend/FrontendActions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendActions.cpp?rev=355950&r1=355949&r2=355950&view=diff
==
--- cfe/trunk/lib/Frontend/FrontendActions.cpp (original)
+++ cfe/trunk/lib/Frontend/FrontendActions.cpp Tue Mar 12 11:38:04 2019
@@ -112,7 +112,7 @@ GeneratePCHAction::CreateASTConsumer(Com
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   FrontendOpts.ModuleFileExtensions,
   CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-  FrontendOpts.IncludeTimestamps));
+  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, InFile, OutputFile, std::move(OS), Buffer));
 
@@ -176,6 +176,8 @@ GenerateModuleAction::CreateASTConsumer(
   CI.getFrontendOpts().ModuleFileExtensions,
   /*AllowASTWithErrors=*/false,
   /*IncludeTimestamps=*/
+  +CI.getFrontendOpts().BuildingImplicitModule,
+  /*ShouldCacheASTInMemory=*/
   +CI.getFrontendOpts().BuildingImplicitModule));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, InFile, OutputFile, std::move(OS), Buffer));

Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=355950&r1=355949&r2=355950&vi

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed in r355950.  Thanks for the review.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59176/new/

https://reviews.llvm.org/D59176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355952 - [OPENMP 5.0]Initial support for 'allocator' clause.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Tue Mar 12 11:52:33 2019
New Revision: 355952

URL: http://llvm.org/viewvc/llvm-project?rev=355952&view=rev
Log:
[OPENMP 5.0]Initial support for 'allocator' clause.

Added parsing/sema analysis/serialization/deserialization for the
'allocator' clause of the 'allocate' directive.

Added:
cfe/trunk/test/OpenMP/allocate_allocator_ast_print.cpp
cfe/trunk/test/OpenMP/allocate_allocator_messages.cpp
Modified:
cfe/trunk/include/clang/AST/ASTNodeTraverser.h
cfe/trunk/include/clang/AST/DeclOpenMP.h
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Basic/OpenMPKinds.def
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/DeclOpenMP.cpp
cfe/trunk/lib/AST/DeclPrinter.cpp
cfe/trunk/lib/AST/OpenMPClause.cpp
cfe/trunk/lib/AST/StmtProfile.cpp
cfe/trunk/lib/Basic/OpenMPKinds.cpp
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/Parse/ParseOpenMP.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/lib/Sema/TreeTransform.h
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
cfe/trunk/test/PCH/chain-openmp-allocate.cpp
cfe/trunk/tools/libclang/CIndex.cpp

Modified: cfe/trunk/include/clang/AST/ASTNodeTraverser.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTNodeTraverser.h?rev=355952&r1=355951&r2=355952&view=diff
==
--- cfe/trunk/include/clang/AST/ASTNodeTraverser.h (original)
+++ cfe/trunk/include/clang/AST/ASTNodeTraverser.h Tue Mar 12 11:52:33 2019
@@ -396,6 +396,8 @@ public:
   void VisitOMPAllocateDecl(const OMPAllocateDecl *D) {
 for (const auto *E : D->varlists())
   Visit(E);
+for (const auto *C : D->clauselists())
+  Visit(C);
   }
 
   template 

Modified: cfe/trunk/include/clang/AST/DeclOpenMP.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclOpenMP.h?rev=355952&r1=355951&r2=355952&view=diff
==
--- cfe/trunk/include/clang/AST/DeclOpenMP.h (original)
+++ cfe/trunk/include/clang/AST/DeclOpenMP.h Tue Mar 12 11:52:33 2019
@@ -421,12 +421,21 @@ public:
 ///
 class OMPAllocateDecl final
 : public Decl,
-  private llvm::TrailingObjects {
+  private llvm::TrailingObjects {
   friend class ASTDeclReader;
   friend TrailingObjects;
 
   /// Number of variable within the allocate directive.
   unsigned NumVars = 0;
+  /// Number of clauses associated with the allocate directive.
+  unsigned NumClauses = 0;
+
+  size_t numTrailingObjects(OverloadToken) const {
+return NumVars;
+  }
+  size_t numTrailingObjects(OverloadToken) const {
+return NumClauses;
+  }
 
   virtual void anchor();
 
@@ -443,19 +452,41 @@ class OMPAllocateDecl final
 
   void setVars(ArrayRef VL);
 
+  /// Returns an array of immutable clauses associated with this directive.
+  ArrayRef getClauses() const {
+return llvm::makeArrayRef(getTrailingObjects(), NumClauses);
+  }
+
+  /// Returns an array of clauses associated with this directive.
+  MutableArrayRef getClauses() {
+return MutableArrayRef(getTrailingObjects(),
+NumClauses);
+  }
+
+  /// Sets an array of clauses to this requires declaration
+  void setClauses(ArrayRef CL);
+
 public:
   static OMPAllocateDecl *Create(ASTContext &C, DeclContext *DC,
- SourceLocation L, ArrayRef VL);
+ SourceLocation L, ArrayRef VL,
+ ArrayRef CL);
   static OMPAllocateDecl *CreateDeserialized(ASTContext &C, unsigned ID,
- unsigned N);
+ unsigned NVars, unsigned 
NClauses);
 
   typedef MutableArrayRef::iterator varlist_iterator;
   typedef ArrayRef::iterator varlist_const_iterator;
   typedef llvm::iterator_range varlist_range;
   typedef llvm::iterator_range varlist_const_range;
+  using clauselist_iterator = MutableArrayRef::iterator;
+  using clauselist_const_iterator = ArrayRef::iterator;
+  using clauselist_range = llvm::iterator_range;
+  using clauselist_const_range = 
llvm::iterator_range;
+
 
   unsigned varlist_size() const { return NumVars; }
   bool varlist_empty() const { return NumVars == 0; }
+  unsigned clauselist_size() const { return NumClauses; }
+  bool clauselist_empty() const { return NumClauses == 0; }
 
   varlist_range varlists() {
 return varlist_range(varlist_begin(), varlist_end());
@@ -468,6 +499,21 @@ public:
   varlist_const_iterator varlist_begin() const { return get

[PATCH] D59118: creduce script for clang crashes

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

In case anyone is interested, for the CHERI fork of LLVM/Clang I added a 
similar script that contains additional features such as inferring the crash 
message (so that you get the minimal reproducer for the issue that you are 
trying to reduce and not some obscure parser crash for invalid input), dealing 
with infinite loops, generating a test case with a mostly sensible `RUN: line`.
Furthermore, it will attempt to use bugpoint if it is a backend rather than a 
frontend crash since that is much much faster than creduce.
It should allows you to run `$LLVM_BUILDIR/bin/creduce_crash_testcase.py 
/tmp/clang-reproducers.sh` and then give a sensible minimal test case back

I can try to clean that up and remove the CHERI-specific compiler flags and 
llvm-lit substitutions.

For reference that script can be found here: 
https://github.com/CTSRD-CHERI/llvm-project/blob/master/clang/utils/creduce_crash_testcase.py


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
> > > difficult to read (when they are 700 lines long), and it is unclear what 
> > > the important parts are.
> > > 
> > > WDYT about converting them to unit tests?  See 
> > > `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > > They are difficult to update.
> > 
> > The updating part is true, for all of the clang tests,
> > they unlike llvm tests have no update scripts, so it's a major pain.
> > Here, it's is actually reasonably simple:
> > 1. prune old `// CHECK*` lines
> > 2. run the run line
> > 3. prepend each line of output with `// CHECK-NEXT: `
> > 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> > 5. scrub filepath prefix, again a simple string-replace
> > 6. prune everything after `TranslationUnitDecl` and before first 
> > `FunctionDecl`
> > 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> > 8. append the final processed output to the test file. done.
> > 
> > It would not be too hard to write an update tool for this, but i've managed 
> > with a simple macro..
> > 
> > > They are fragile, difficult to read (when they are 700 lines long),
> > > and it is unclear what the important parts are.
> > 
> > This is kind-of intentional. I've tried to follow the llvm-proper approach 
> > of gigantic 
> > tests that are overly verbose, but quickly cement the state so **any** 
> > change **will**
> > be noticed. That has been a **very** successful approach for LLVM.
> > 
> > In fact, the lack of this ast-dumper test coverage was not helping
> > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > 
> > So no, i really don't want to //convert// the tests into a less verbose 
> > state.
> > 
> > I suppose i //could// //add// a more fine-grained tests, 
> > though readability is arguable. Here, it is obvious nothing is omitted,
> > and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
> > With more distilled tests, it's less obvious. (i'm not talking about 
> > `StmtPrinterTest`)
> > Here, it's is actually reasonably simple:
> 
> Unfortunately, an 8-step process is anything but simple.
> 
> > This is kind-of intentional. I've tried to follow the llvm-proper approach 
> > of gigantic 
> > tests that are overly verbose, but quickly cement the state so any change 
> > will
> > be noticed.
> 
> That's pretty much a definition of a "fragile test".  The tests should check 
> what they intend to check.
> 
> > That has been a very successful approach for LLVM.
> 
> I don't think LLVM does this, the CHECK lines are for the most part manually 
> crafted.
> 
> > In fact, the lack of this ast-dumper test coverage was not helping
> > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> 
> If we need tests for AST dumper, we should write tests for it.  OpenMP tests 
> should not double for AST dumper tests.
> 
>> Here, it's is actually reasonably simple:
> Unfortunately, an 8-step process is anything but simple.

Hah, what i was saying is that it is entirely automatable,
all steps are identical each time.

> That's pretty much a definition of a "fragile test". The tests should check 
> what they intend to check.
> I don't think LLVM does this, the CHECK lines are for the most part manually 
> crafted.

No.
I //know// that 'most' of the new/updated tests are not manual,
from looking at the tests in commits, it's not an empty statement.

```
llvm/test/CodeGen$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll 
| xargs grep "update_llc" | wc -l; done | sort -r -n -k 2,2
X86 1427
PowerPC 125
RISCV 90
...
```
```
llvm/test/CodeGen$ find X86/ -iname *.ll | wc -l
3370
llvm/test/CodeGen$ find PowerPC/ -iname *.ll | wc -l
1019
llvm/test/CodeGen$ find RISCV/ -iname *.ll | wc -l
105
```
So a half of X86 backend tests, and pretty much all RISCV backend tests.
```
llvm/test$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | xargs 
grep "update_test" | wc -l; done | sort -r -n -k 2,2
Transforms 845
Analysis 17
CodeGen 6
Other 2
```
```
$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | wc -l; done | 
sort -r -n -k 2,2
Transforms 4620
Analysis 804
```
So sure, by the numbers, 'most' aren't auto-generated.
It's a question of legacy mainly.

I've both added llvm tests, and clang tests.
llvm tests are a breeze, just come up with sufficient IR,
and run the script [verify that the CHECK is what you thought it is,
or tune IR until it is], and you're done. With clang (codegen mostly),
one needs to first come up with the test, and then with the check lines,
and be really careful not to misspell `CHECK`, or else you're suddenly
not testin

r355960 - [OPENMP]Allow to redefine entry for the variables definitions.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Tue Mar 12 13:05:17 2019
New Revision: 355960

URL: http://llvm.org/viewvc/llvm-project?rev=355960&view=rev
Log:
[OPENMP]Allow to redefine entry for the variables definitions.

If the variable was declared and marked as declare target, a new offload
entry with size 0 is created. But if later a definition is created and
marked as declare target, this definition is not added to the entry set
and the definition remains not mapped to the target. Patch fixes this
problem allowing to redefine the size and linkage for
previously registered declaration.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/test/OpenMP/declare_target_codegen.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=355960&r1=355959&r2=355960&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Tue Mar 12 13:05:17 2019
@@ -3760,14 +3760,29 @@ void CGOpenMPRuntime::OffloadEntriesInfo
"Entry not initialized!");
 assert((!Entry.getAddress() || Entry.getAddress() == Addr) &&
"Resetting with the new address.");
-if (Entry.getAddress() && hasDeviceGlobalVarEntryInfo(VarName))
+if (Entry.getAddress() && hasDeviceGlobalVarEntryInfo(VarName)) {
+  if (Entry.getVarSize().isZero()) {
+Entry.setVarSize(VarSize);
+Entry.setLinkage(Linkage);
+  }
   return;
-Entry.setAddress(Addr);
+}
 Entry.setVarSize(VarSize);
 Entry.setLinkage(Linkage);
+Entry.setAddress(Addr);
   } else {
-if (hasDeviceGlobalVarEntryInfo(VarName))
+if (hasDeviceGlobalVarEntryInfo(VarName)) {
+  auto &Entry = OffloadEntriesDeviceGlobalVar[VarName];
+  assert(Entry.isValid() && Entry.getFlags() == Flags &&
+ "Entry not initialized!");
+  assert((!Entry.getAddress() || Entry.getAddress() == Addr) &&
+ "Resetting with the new address.");
+  if (Entry.getVarSize().isZero()) {
+Entry.setVarSize(VarSize);
+Entry.setLinkage(Linkage);
+  }
   return;
+}
 OffloadEntriesDeviceGlobalVar.try_emplace(
 VarName, OffloadingEntriesNum, Addr, VarSize, Flags, Linkage);
 ++OffloadingEntriesNum;

Modified: cfe/trunk/test/OpenMP/declare_target_codegen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/declare_target_codegen.cpp?rev=355960&r1=355959&r2=355960&view=diff
==
--- cfe/trunk/test/OpenMP/declare_target_codegen.cpp (original)
+++ cfe/trunk/test/OpenMP/declare_target_codegen.cpp Tue Mar 12 13:05:17 2019
@@ -17,6 +17,7 @@
 // CHECK-NOT: @{{hhh|ggg|fff|eee}} =
 // CHECK-DAG: @aaa = external global i32,
 // CHECK-DAG: @bbb = global i32 0,
+// CHECK-DAG: weak constant %struct.__tgt_offload_entry { i8* bitcast (i32* 
@bbb to i8*),
 // CHECK-DAG: @ccc = external global i32,
 // CHECK-DAG: @ddd = global i32 0,
 // CHECK-DAG: @hhh_decl_tgt_link_ptr = common global i32* null
@@ -31,24 +32,35 @@
 // CHECK-DAG: [[STAT:@.+stat]] = internal global %struct.S zeroinitializer,
 // CHECK-DAG: [[STAT_REF:@.+]] = internal constant %struct.S* [[STAT]]
 // CHECK-DAG: @out_decl_target = global i32 0,
-// CHECK-DAG: @llvm.used = appending global [6 x i8*] [i8* bitcast (void ()* 
@__omp_offloading__{{.+}}_globals_l[[@LINE+69]]_ctor to i8*), i8* bitcast (void 
()* @__omp_offloading__{{.+}}_stat_l[[@LINE+70]]_ctor to i8*),
+// CHECK-DAG: @llvm.used = appending global [6 x i8*] [i8* bitcast (void ()* 
@__omp_offloading__{{.+}}_globals_l[[@LINE+80]]_ctor to i8*), i8* bitcast (void 
()* @__omp_offloading__{{.+}}_stat_l[[@LINE+81]]_ctor to i8*),
 // CHECK-DAG: @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast 
(%struct.S** [[STAT_REF]] to i8*)],
 
 // CHECK-DAG: define {{.*}}i32 @{{.*}}{{foo|bar|baz2|baz3|FA|f_method}}{{.*}}()
 // CHECK-DAG: define {{.*}}void 
@{{.*}}TemplateClass{{.*}}(%class.TemplateClass* %{{.*}})
 // CHECK-DAG: define {{.*}}i32 
@{{.*}}TemplateClass{{.*}}f_method{{.*}}(%class.TemplateClass* %{{.*}})
-// CHECK-DAG: define {{.*}}void 
@__omp_offloading__{{.*}}_globals_l[[@LINE+63]]_ctor()
+// CHECK-DAG: define {{.*}}void 
@__omp_offloading__{{.*}}_globals_l[[@LINE+74]]_ctor()
 
 #ifndef HEADER
 #define HEADER
 
 #pragma omp declare target
+extern int bbb;
+#pragma omp end declare target
+#pragma omp declare target
+extern int bbb;
+#pragma omp end declare target
+
+#pragma omp declare target
 extern int aaa;
 int bbb = 0;
 extern int ccc;
 int ddd = 0;
 #pragma omp end declare target
 
+#pragma omp declare target
+extern int bbb;
+#pragma omp end declare target
+
 extern int eee;
 int fff = 0;
 extern int ggg;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

lebedev.ri wrote:
> ABataev wrote:
> > lebedev.ri wrote:
> > > ABataev wrote:
> > > > No need to insert it into each class, just add:
> > > > ```
> > > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > > return nullptr;
> > > >   if (auto *LD = dyn_cast(this))
> > > > return LD->getBody();
> > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > }
> > > > ```
> > > I absolutely can do that, you are sure that is the most future-proof 
> > > state?
> > > In particular, i want to re-point-out that if it's implemented like this,
> > > in the base class, then the sub-class may(will) not even know about this 
> > > function,
> > > and thus 'forget' to update it, should it not be giving the correct 
> > > answer for
> > > that new specific OpenMP executable directive.
> > > 
> > > You are sure it's better to implement it in the `OMPExecutableDirective` 
> > > itself?
> > Yes, I'm sure. It is the universal solution and all future classes must be 
> > compatible with it. If they are not, then they are incorrect.
> Aha! Well, ok then.
> 
> Do you also suggest that `Optional<>` is too fancy?
> Would it be better to do this instead?
> ```
> bool isStandaloneDirective() const {
>   return !hasAssociatedStmt() || !getAssociatedStmt();
> }
> 
> // Requires: !isStandaloneDirective()
> Stmt *OMPExecutableDirective::getStructuredBlock() const {
>   assert(!isStandaloneDirective() && "Standalone Executable OpenMP directives 
> don't have structured blocks.")
>   if (auto *LD = dyn_cast(this))
> return LD->getBody();
>   return getInnermostCapturedStmt()->getCapturedStmt();
> }
> ```
> Hm, maybe that actually conveys more meaning..
Great, that doesn't work, and highlights my concerns.
`target enter data` / `target exit data` / `target update` are stand-alone 
directives as per the spec,
but not as per that `isStandaloneDirective()` check ^.
https://godbolt.org/z/0tE93s

Is this a bug, or intentional?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59234: [CodeGen][ObjC] Remove the leading 'l' from symbols for protocol metadata and protocol list

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Using internal linkage instead of private makes sense to me.  Even if we wanted 
to use private linkage, it never made sense to be doing this with `\01l` 
instead of just setting the linkage properly.  Maybe this was implemented 
before LLVM directly supported private linkage, but that hasn't been a 
constraint in 10+ years.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59234/new/

https://reviews.llvm.org/D59234



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 190317.
jkorous added a comment.
Herald added a subscriber: jfb.

Based on Kadir comment I refactored the code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749

Files:
  clang/lib/Index/CMakeLists.txt
  clang/lib/Index/IndexRecordHasher.h
  clang/lib/Index/RecordHasher/CMakeLists.txt
  clang/lib/Index/RecordHasher/CachingHasher.cpp
  clang/lib/Index/RecordHasher/CachingHasher.h
  clang/lib/Index/RecordHasher/DeclHasher.cpp
  clang/lib/Index/RecordHasher/DeclHasher.h
  clang/lib/Index/RecordHasher/IndexRecordHasher.cpp

Index: clang/lib/Index/RecordHasher/IndexRecordHasher.cpp
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/IndexRecordHasher.cpp
@@ -0,0 +1,33 @@
+//===--- IndexRecordHasher.cpp - Hashing of FileIndexRecord -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "IndexRecordHasher.h"
+#include "FileIndexRecord.h"
+#include "RecordHasher/CachingHasher.h"
+
+namespace clang {
+namespace index {
+
+using llvm::hash_code;
+
+hash_code hashRecord(ASTContext &Ctx, const FileIndexRecord &Record) {
+
+  CachingHasher Hasher(Ctx);
+
+  hash_code Hash = InitialHash;
+  for (auto &Info : Record.getDeclOccurrencesSortedByOffset()) {
+Hash = hash_combine(Hash, Info.Roles, Info.Offset, Hasher.hash(Info.Dcl));
+for (auto &Rel : Info.Relations) {
+  Hash = hash_combine(Hash, Hasher.hash(Rel.RelatedSymbol));
+}
+  }
+  return Hash;
+}
+
+} // end namespace index
+} // end namespace clang
Index: clang/lib/Index/RecordHasher/DeclHasher.h
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/DeclHasher.h
@@ -0,0 +1,51 @@
+//===--- DeclHasher.h - Hashing of Decl nodes in AST *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
+#define LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclVisitor.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace index {
+
+class CachingHasher;
+
+/// Implements hashing for declaration nodes in AST.
+/// This is just a convenient way how to avoid writing a huge switch for various
+/// types derived from Decl. Uses CachingHasher for hashing of atomic entities.
+
+class DeclHasher : public ConstDeclVisitor {
+  CachingHasher &Hasher;
+
+public:
+  DeclHasher(CachingHasher &Hasher) : Hasher(Hasher) {}
+
+  llvm::hash_code VisitDecl(const Decl *D);
+  llvm::hash_code VisitNamedDecl(const NamedDecl *D);
+  llvm::hash_code VisitTagDecl(const TagDecl *D);
+  llvm::hash_code VisitClassTemplateSpecializationDecl(
+  const ClassTemplateSpecializationDecl *D);
+  llvm::hash_code VisitObjCContainerDecl(const ObjCContainerDecl *D);
+  llvm::hash_code VisitObjCImplDecl(const ObjCImplDecl *D);
+  llvm::hash_code VisitObjCCategoryDecl(const ObjCCategoryDecl *D);
+  llvm::hash_code VisitFunctionDecl(const FunctionDecl *D);
+  llvm::hash_code
+  VisitUnresolvedUsingTypenameDecl(const UnresolvedUsingTypenameDecl *D);
+  llvm::hash_code
+  VisitUnresolvedUsingValueDecl(const UnresolvedUsingValueDecl *D);
+  llvm::hash_code VisitDeclContext(const DeclContext *DC);
+  llvm::hash_code hashLoc(SourceLocation Loc, bool IncludeOffset);
+};
+
+} // end namespace index
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
Index: clang/lib/Index/RecordHasher/DeclHasher.cpp
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/DeclHasher.cpp
@@ -0,0 +1,140 @@
+//===--- DeclHasher.cpp - Hashing of Decl nodes in AST --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "RecordHasher/DeclHasher.h"
+#include "RecordHasher/CachingHasher.h"
+
+namespace clang {
+namespace index {
+
+using llvm::hash_code;
+
+hash_code DeclHasher::VisitDecl(const Decl *D) {
+  return VisitDeclContext(D->getDeclContext());
+}
+
+hash_code DeclHasher::VisitNamedDecl(const NamedDecl *D) {
+  hash_code Hash = VisitDecl(D);
+  if (auto *attr = D->getExternalSourceSym

r355964 - [Remarks] Add -foptimization-record-passes to filter remark emission

[Francis Visoiu Mistrih via cfe-commits]

Author: thegameg
Date: Tue Mar 12 13:28:50 2019
New Revision: 355964

URL: http://llvm.org/viewvc/llvm-project?rev=355964&view=rev
Log:
[Remarks] Add -foptimization-record-passes to filter remark emission

Currently we have -Rpass for filtering the remarks that are displayed as
diagnostics, but when using -fsave-optimization-record, there is no way
to filter the remarks while generating them.

This adds support for filtering remarks by passes using a regex.
Ex: `clang -fsave-optimization-record -foptimization-record-passes=inline`

will only emit the remarks coming from the pass `inline`.

This adds:

* `-fsave-optimization-record` to the driver
* `-opt-record-passes` to cc1
* `-lto-pass-remarks-filter` to the LTOCodeGenerator
* `--opt-remarks-passes` to lld
* `-pass-remarks-filter` to llc, opt, llvm-lto, llvm-lto2
* `-opt-remarks-passes` to gold-plugin

Differential Revision: https://reviews.llvm.org/D59268

Modified:
cfe/trunk/include/clang/Basic/CodeGenOptions.h
cfe/trunk/include/clang/Driver/CC1Options.td
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/CodeGen/opt-record-MIR.c
cfe/trunk/test/CodeGen/opt-record.c
cfe/trunk/test/Driver/darwin-ld.c
cfe/trunk/test/Driver/opt-record.c

Modified: cfe/trunk/include/clang/Basic/CodeGenOptions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/CodeGenOptions.h?rev=355964&r1=355963&r2=355964&view=diff
==
--- cfe/trunk/include/clang/Basic/CodeGenOptions.h (original)
+++ cfe/trunk/include/clang/Basic/CodeGenOptions.h Tue Mar 12 13:28:50 2019
@@ -238,6 +238,10 @@ public:
   /// records.
   std::string OptRecordFile;
 
+  /// The regex that filters the passes that should be saved to the 
optimization
+  /// records.
+  std::string OptRecordPasses;
+
   /// Regular expression to select optimizations for which we should enable
   /// optimization remarks. Transformation passes whose name matches this
   /// expression (and support this feature), will emit a diagnostic

Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=355964&r1=355963&r2=355964&view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Tue Mar 12 13:28:50 2019
@@ -603,6 +603,8 @@ def arcmt_migrate : Flag<["-"], "arcmt-m
 
 def opt_record_file : Separate<["-"], "opt-record-file">,
   HelpText<"File name to use for YAML optimization record output">;
+def opt_record_passes : Separate<["-"], "opt-record-passes">,
+  HelpText<"Only record remark information for passes whose names match the 
given regular expression">;
 
 def print_stats : Flag<["-"], "print-stats">,
   HelpText<"Print performance metrics and statistics">;

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=355964&r1=355963&r2=355964&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue Mar 12 13:28:50 2019
@@ -1715,6 +1715,10 @@ def fno_save_optimization_record : Flag<
 def foptimization_record_file_EQ : Joined<["-"], "foptimization-record-file=">,
   Group,
   HelpText<"Specify the file name of any generated YAML optimization record">;
+def foptimization_record_passes_EQ : Joined<["-"], 
"foptimization-record-passes=">,
+  Group,
+  HelpText<"Only include passes which match a specified regular expression in 
the generated optimization record (by default, include all passes)">;
+
 
 def ftest_coverage : Flag<["-"], "ftest-coverage">, Group;
 def fvectorize : Flag<["-"], "fvectorize">, Group,

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=355964&r1=355963&r2=355964&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue Mar 12 13:28:50 2019
@@ -1340,6 +1340,7 @@ static void runThinLTOBackend(ModuleSumm
   Conf.DebugPassManager = CGOpts.DebugPassManager;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
+  Conf.RemarksPasses = CGOpts.OptRecordPasses;
   Conf.DwoPath = CGOpts.SplitDwarfFile;
   switch (Action) {
   case Backend_EmitNothing:

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/Cod

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58749#1426270 , @gribozavr wrote:

> I left some comments, but it is difficult for me to review without 
> understanding what the requirements for this hasher are, why some information 
> is hashed in, and some is left out, could you clarify?  See detailed comments.


Will do.

>> This implementation is covered by lit tests using it through c-index-test in 
>> upcoming patch.
> 
> This code looks very unit-testable.  It also handles many corner cases (what 
> information is hashed and what is left out).  c-index-tests are integration 
> tests that are not as good at that, and also they would be testing this code 
> quite indirectly.

I basically didn't really like the idea of testing against hard-coded hash 
values in unittests as I consider it to be an implementation detail. I was 
thinking about integration tests that would work around this by both writing 
index and reading index and writing tests against that data. What do you think?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

lebedev.ri wrote:
> lebedev.ri wrote:
> > ABataev wrote:
> > > lebedev.ri wrote:
> > > > ABataev wrote:
> > > > > No need to insert it into each class, just add:
> > > > > ```
> > > > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > > > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > > > return nullptr;
> > > > >   if (auto *LD = dyn_cast(this))
> > > > > return LD->getBody();
> > > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > > }
> > > > > ```
> > > > I absolutely can do that, you are sure that is the most future-proof 
> > > > state?
> > > > In particular, i want to re-point-out that if it's implemented like 
> > > > this,
> > > > in the base class, then the sub-class may(will) not even know about 
> > > > this function,
> > > > and thus 'forget' to update it, should it not be giving the correct 
> > > > answer for
> > > > that new specific OpenMP executable directive.
> > > > 
> > > > You are sure it's better to implement it in the 
> > > > `OMPExecutableDirective` itself?
> > > Yes, I'm sure. It is the universal solution and all future classes must 
> > > be compatible with it. If they are not, then they are incorrect.
> > Aha! Well, ok then.
> > 
> > Do you also suggest that `Optional<>` is too fancy?
> > Would it be better to do this instead?
> > ```
> > bool isStandaloneDirective() const {
> >   return !hasAssociatedStmt() || !getAssociatedStmt();
> > }
> > 
> > // Requires: !isStandaloneDirective()
> > Stmt *OMPExecutableDirective::getStructuredBlock() const {
> >   assert(!isStandaloneDirective() && "Standalone Executable OpenMP 
> > directives don't have structured blocks.")
> >   if (auto *LD = dyn_cast(this))
> > return LD->getBody();
> >   return getInnermostCapturedStmt()->getCapturedStmt();
> > }
> > ```
> > Hm, maybe that actually conveys more meaning..
> Great, that doesn't work, and highlights my concerns.
> `target enter data` / `target exit data` / `target update` are stand-alone 
> directives as per the spec,
> but not as per that `isStandaloneDirective()` check ^.
> https://godbolt.org/z/0tE93s
> 
> Is this a bug, or intentional?
Well, this is an incompatibility caused by previous not-quite correct 
implementation. It was reworked already, but these incorrect children still 
remain, I just had no time to clean them out. You can fix this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional getStructuredBlockImpl() const {
+return const_cast(getInnermostCapturedStmt()->getCapturedStmt());

ABataev wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > ABataev wrote:
> > > > lebedev.ri wrote:
> > > > > ABataev wrote:
> > > > > > No need to insert it into each class, just add:
> > > > > > ```
> > > > > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > > > > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > > > > return nullptr;
> > > > > >   if (auto *LD = dyn_cast(this))
> > > > > > return LD->getBody();
> > > > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > > > }
> > > > > > ```
> > > > > I absolutely can do that, you are sure that is the most future-proof 
> > > > > state?
> > > > > In particular, i want to re-point-out that if it's implemented like 
> > > > > this,
> > > > > in the base class, then the sub-class may(will) not even know about 
> > > > > this function,
> > > > > and thus 'forget' to update it, should it not be giving the correct 
> > > > > answer for
> > > > > that new specific OpenMP executable directive.
> > > > > 
> > > > > You are sure it's better to implement it in the 
> > > > > `OMPExecutableDirective` itself?
> > > > Yes, I'm sure. It is the universal solution and all future classes must 
> > > > be compatible with it. If they are not, then they are incorrect.
> > > Aha! Well, ok then.
> > > 
> > > Do you also suggest that `Optional<>` is too fancy?
> > > Would it be better to do this instead?
> > > ```
> > > bool isStandaloneDirective() const {
> > >   return !hasAssociatedStmt() || !getAssociatedStmt();
> > > }
> > > 
> > > // Requires: !isStandaloneDirective()
> > > Stmt *OMPExecutableDirective::getStructuredBlock() const {
> > >   assert(!isStandaloneDirective() && "Standalone Executable OpenMP 
> > > directives don't have structured blocks.")
> > >   if (auto *LD = dyn_cast(this))
> > > return LD->getBody();
> > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > }
> > > ```
> > > Hm, maybe that actually conveys more meaning..
> > Great, that doesn't work, and highlights my concerns.
> > `target enter data` / `target exit data` / `target update` are stand-alone 
> > directives as per the spec,
> > but not as per that `isStandaloneDirective()` check ^.
> > https://godbolt.org/z/0tE93s
> > 
> > Is this a bug, or intentional?
> Well, this is an incompatibility caused by previous not-quite correct 
> implementation. It was reworked already, but these incorrect children still 
> remain, I just had no time to clean them out. You can fix this.
Okay, that is reassuring, thanks.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> I basically didn't really like the idea of testing against hard-coded hash 
> values in unittests as I consider it to be an implementation detail. I was 
> thinking about integration tests that would work around this by both writing 
> index and reading index and writing assertions against that data. What do you 
> think?

Instead of testing against hardcoded values maybe you can test for difference? 
Making sure hashes differ or stays same when appropriate changes are done to 
the contents?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> I basically didn't really like the idea of testing against hard-coded hash 
> values in unittests as I consider it to be an implementation detail.

Sorry, that's not what I was suggesting.  There are better ways to test 
hashing.  For example, write a piece of source code and check that the hash 
codes for two declarations that only differ in one aspect are same or different.

For example,

  void f(int &&);
  void f(int &);

and then assert that hashes for two decls are same or different depending on 
the desired specification.

> I was thinking about integration tests that would work around this by both 
> writing index and reading index and writing assertions against that data.

How is hashing used in this process?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355976 - Revert "[Remarks] Add -foptimization-record-passes to filter remark emission"

[Francis Visoiu Mistrih via cfe-commits]

Author: thegameg
Date: Tue Mar 12 13:54:18 2019
New Revision: 355976

URL: http://llvm.org/viewvc/llvm-project?rev=355976&view=rev
Log:
Revert "[Remarks] Add -foptimization-record-passes to filter remark emission"

This reverts commit 20fff32b7d1f1a1bd417b22aa9f26ededd97a3e5.

Modified:
cfe/trunk/include/clang/Basic/CodeGenOptions.h
cfe/trunk/include/clang/Driver/CC1Options.td
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/CodeGen/opt-record-MIR.c
cfe/trunk/test/CodeGen/opt-record.c
cfe/trunk/test/Driver/darwin-ld.c
cfe/trunk/test/Driver/opt-record.c

Modified: cfe/trunk/include/clang/Basic/CodeGenOptions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/CodeGenOptions.h?rev=355976&r1=355975&r2=355976&view=diff
==
--- cfe/trunk/include/clang/Basic/CodeGenOptions.h (original)
+++ cfe/trunk/include/clang/Basic/CodeGenOptions.h Tue Mar 12 13:54:18 2019
@@ -238,10 +238,6 @@ public:
   /// records.
   std::string OptRecordFile;
 
-  /// The regex that filters the passes that should be saved to the 
optimization
-  /// records.
-  std::string OptRecordPasses;
-
   /// Regular expression to select optimizations for which we should enable
   /// optimization remarks. Transformation passes whose name matches this
   /// expression (and support this feature), will emit a diagnostic

Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=355976&r1=355975&r2=355976&view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Tue Mar 12 13:54:18 2019
@@ -603,8 +603,6 @@ def arcmt_migrate : Flag<["-"], "arcmt-m
 
 def opt_record_file : Separate<["-"], "opt-record-file">,
   HelpText<"File name to use for YAML optimization record output">;
-def opt_record_passes : Separate<["-"], "opt-record-passes">,
-  HelpText<"Only record remark information for passes whose names match the 
given regular expression">;
 
 def print_stats : Flag<["-"], "print-stats">,
   HelpText<"Print performance metrics and statistics">;

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=355976&r1=355975&r2=355976&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue Mar 12 13:54:18 2019
@@ -1715,10 +1715,6 @@ def fno_save_optimization_record : Flag<
 def foptimization_record_file_EQ : Joined<["-"], "foptimization-record-file=">,
   Group,
   HelpText<"Specify the file name of any generated YAML optimization record">;
-def foptimization_record_passes_EQ : Joined<["-"], 
"foptimization-record-passes=">,
-  Group,
-  HelpText<"Only include passes which match a specified regular expression in 
the generated optimization record (by default, include all passes)">;
-
 
 def ftest_coverage : Flag<["-"], "ftest-coverage">, Group;
 def fvectorize : Flag<["-"], "fvectorize">, Group,

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=355976&r1=355975&r2=355976&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue Mar 12 13:54:18 2019
@@ -1340,7 +1340,6 @@ static void runThinLTOBackend(ModuleSumm
   Conf.DebugPassManager = CGOpts.DebugPassManager;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
-  Conf.RemarksPasses = CGOpts.OptRecordPasses;
   Conf.DwoPath = CGOpts.SplitDwarfFile;
   switch (Action) {
   case Backend_EmitNothing:

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenAction.cpp?rev=355976&r1=355975&r2=355976&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenAction.cpp Tue Mar 12 13:54:18 2019
@@ -19,7 +19,6 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/CodeGen/ModuleBuilder.h"
-#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/Preprocessor.h"
@@ -281,12 +280,6 @@ namespace clang {
 Ctx.setRemarkStreamer(llvm

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > I'm not a fan of ast-dump tests.  They are fragile, difficult to 
> > > > update, difficult to read (when they are 700 lines long), and it is 
> > > > unclear what the important parts are.
> > > > 
> > > > WDYT about converting them to unit tests?  See 
> > > > `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > > > They are difficult to update.
> > > 
> > > The updating part is true, for all of the clang tests,
> > > they unlike llvm tests have no update scripts, so it's a major pain.
> > > Here, it's is actually reasonably simple:
> > > 1. prune old `// CHECK*` lines
> > > 2. run the run line
> > > 3. prepend each line of output with `// CHECK-NEXT: `
> > > 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> > > 5. scrub filepath prefix, again a simple string-replace
> > > 6. prune everything after `TranslationUnitDecl` and before first 
> > > `FunctionDecl`
> > > 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> > > 8. append the final processed output to the test file. done.
> > > 
> > > It would not be too hard to write an update tool for this, but i've 
> > > managed with a simple macro..
> > > 
> > > > They are fragile, difficult to read (when they are 700 lines long),
> > > > and it is unclear what the important parts are.
> > > 
> > > This is kind-of intentional. I've tried to follow the llvm-proper 
> > > approach of gigantic 
> > > tests that are overly verbose, but quickly cement the state so **any** 
> > > change **will**
> > > be noticed. That has been a **very** successful approach for LLVM.
> > > 
> > > In fact, the lack of this ast-dumper test coverage was not helping
> > > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > > 
> > > So no, i really don't want to //convert// the tests into a less verbose 
> > > state.
> > > 
> > > I suppose i //could// //add// a more fine-grained tests, 
> > > though readability is arguable. Here, it is obvious nothing is omitted,
> > > and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for 
> > > it.
> > > With more distilled tests, it's less obvious. (i'm not talking about 
> > > `StmtPrinterTest`)
> > > Here, it's is actually reasonably simple:
> > 
> > Unfortunately, an 8-step process is anything but simple.
> > 
> > > This is kind-of intentional. I've tried to follow the llvm-proper 
> > > approach of gigantic 
> > > tests that are overly verbose, but quickly cement the state so any change 
> > > will
> > > be noticed.
> > 
> > That's pretty much a definition of a "fragile test".  The tests should 
> > check what they intend to check.
> > 
> > > That has been a very successful approach for LLVM.
> > 
> > I don't think LLVM does this, the CHECK lines are for the most part 
> > manually crafted.
> > 
> > > In fact, the lack of this ast-dumper test coverage was not helping
> > > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > 
> > If we need tests for AST dumper, we should write tests for it.  OpenMP 
> > tests should not double for AST dumper tests.
> > 
> >> Here, it's is actually reasonably simple:
> > Unfortunately, an 8-step process is anything but simple.
> 
> Hah, what i was saying is that it is entirely automatable,
> all steps are identical each time.
> 
> > That's pretty much a definition of a "fragile test". The tests should check 
> > what they intend to check.
> > I don't think LLVM does this, the CHECK lines are for the most part 
> > manually crafted.
> 
> No.
> I //know// that 'most' of the new/updated tests are not manual,
> from looking at the tests in commits, it's not an empty statement.
> 
> ```
> llvm/test/CodeGen$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname 
> \*.ll | xargs grep "update_llc" | wc -l; done | sort -r -n -k 2,2
> X86 1427
> PowerPC 125
> RISCV 90
> ...
> ```
> ```
> llvm/test/CodeGen$ find X86/ -iname *.ll | wc -l
> 3370
> llvm/test/CodeGen$ find PowerPC/ -iname *.ll | wc -l
> 1019
> llvm/test/CodeGen$ find RISCV/ -iname *.ll | wc -l
> 105
> ```
> So a half of X86 backend tests, and pretty much all RISCV backend tests.
> ```
> llvm/test$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | 
> xargs grep "update_test" | wc -l; done | sort -r -n -k 2,2
> Transforms 845
> Analysis 17
> CodeGen 6
> Other 2
> ```
> ```
> $ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | wc -l; done | 
> sort -r -n -k 2,2
> Transforms 4620
> Analysis 804
> ```
> So sure, by the numbers, 'most' aren't auto-generated.
> It's a question of legacy mainly.
> 
> I've both added llvm tests, and clang tests.
> llvm tests are a breeze, just come up with sufficient IR,
> and run the script [veri

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

This needs a test under `test/CodeGen` at least.




Comment at: clang/include/clang/AST/RecordFieldReorganizer.h:54
+  std::seed_seq Seq;
+  std::default_random_engine rng;
+};

I don't think we can use `default_random_engine` for this because the behaviour 
would need to be consistent between C++ standard library implementations, and 
the behaviour of `default_random_engine` is implementation defined. Similarly, 
I don't think that we can use `std::shuffle` (see note in 
https://en.cppreference.com/w/cpp/algorithm/random_shuffle ).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: lib/AST/RecordLayoutBuilder.cpp:2750-2753
+// It is possible that there were no fields or bases located after vbptr,
+// so the size was not adjusted before.
+if (Size < FieldStart)
+  Size = FieldStart;

I think this may be an interesting test case:
  struct __declspec(align(16)) NVBase {
int x, y;
virtual ~NVBase();
  };
  struct VBase { int z; };
  struct Foo : NVBase, virtual VBase {
  };

On reading the code more, I don't think this test uncovers any bugs, but it 
seems worth including to make sure we do the right thing w.r.t. alignment.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 190342.
jkorous marked 2 inline comments as done.
jkorous added a comment.

Addressed some of Dmitri's comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749

Files:
  clang/lib/Index/CMakeLists.txt
  clang/lib/Index/IndexRecordHasher.h
  clang/lib/Index/RecordHasher/CMakeLists.txt
  clang/lib/Index/RecordHasher/CachingHasher.cpp
  clang/lib/Index/RecordHasher/CachingHasher.h
  clang/lib/Index/RecordHasher/DeclHasher.cpp
  clang/lib/Index/RecordHasher/DeclHasher.h
  clang/lib/Index/RecordHasher/IndexRecordHasher.cpp

Index: clang/lib/Index/RecordHasher/IndexRecordHasher.cpp
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/IndexRecordHasher.cpp
@@ -0,0 +1,33 @@
+//===--- IndexRecordHasher.cpp - Hashing of FileIndexRecord -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "IndexRecordHasher.h"
+#include "FileIndexRecord.h"
+#include "RecordHasher/CachingHasher.h"
+
+namespace clang {
+namespace index {
+
+using llvm::hash_code;
+
+hash_code hashRecord(ASTContext &Ctx, const FileIndexRecord &Record) {
+
+  CachingHasher Hasher(Ctx);
+
+  hash_code Hash = InitialHash;
+  for (auto &Info : Record.getDeclOccurrencesSortedByOffset()) {
+Hash = hash_combine(Hash, Info.Roles, Info.Offset, Hasher.hash(Info.Dcl));
+for (auto &Rel : Info.Relations) {
+  Hash = hash_combine(Hash, Hasher.hash(Rel.RelatedSymbol));
+}
+  }
+  return Hash;
+}
+
+} // end namespace index
+} // end namespace clang
Index: clang/lib/Index/RecordHasher/DeclHasher.h
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/DeclHasher.h
@@ -0,0 +1,51 @@
+//===--- DeclHasher.h - Hashing of Decl nodes in AST *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
+#define LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclVisitor.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace index {
+
+class CachingHasher;
+
+/// Implements hashing for declaration nodes in AST.
+/// This is just a convenient way how to avoid writing a huge switch for various
+/// types derived from Decl. Uses CachingHasher for hashing of atomic entities.
+
+class DeclHasher : public ConstDeclVisitor {
+  CachingHasher &Hasher;
+
+public:
+  DeclHasher(CachingHasher &Hasher) : Hasher(Hasher) {}
+
+  llvm::hash_code VisitDecl(const Decl *D);
+  llvm::hash_code VisitNamedDecl(const NamedDecl *D);
+  llvm::hash_code VisitTagDecl(const TagDecl *D);
+  llvm::hash_code VisitClassTemplateSpecializationDecl(
+  const ClassTemplateSpecializationDecl *D);
+  llvm::hash_code VisitObjCContainerDecl(const ObjCContainerDecl *D);
+  llvm::hash_code VisitObjCImplDecl(const ObjCImplDecl *D);
+  llvm::hash_code VisitObjCCategoryDecl(const ObjCCategoryDecl *D);
+  llvm::hash_code VisitFunctionDecl(const FunctionDecl *D);
+  llvm::hash_code
+  VisitUnresolvedUsingTypenameDecl(const UnresolvedUsingTypenameDecl *D);
+  llvm::hash_code
+  VisitUnresolvedUsingValueDecl(const UnresolvedUsingValueDecl *D);
+  llvm::hash_code VisitDeclContext(const DeclContext *DC);
+  llvm::hash_code hashLoc(SourceLocation Loc, bool IncludeOffset);
+};
+
+} // end namespace index
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_INDEX_RECORDHASHER_DECLHASHER_H
Index: clang/lib/Index/RecordHasher/DeclHasher.cpp
===
--- /dev/null
+++ clang/lib/Index/RecordHasher/DeclHasher.cpp
@@ -0,0 +1,140 @@
+//===--- DeclHasher.cpp - Hashing of Decl nodes in AST --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "RecordHasher/DeclHasher.h"
+#include "RecordHasher/CachingHasher.h"
+
+namespace clang {
+namespace index {
+
+using llvm::hash_code;
+
+hash_code DeclHasher::VisitDecl(const Decl *D) {
+  return VisitDeclContext(D->getDeclContext());
+}
+
+hash_code DeclHasher::VisitNamedDecl(const NamedDecl *D) {
+  hash_code Hash = VisitDecl(D);
+  if (auto *attr = D->getExternalSourceSymbolAttr()) {
+Hash 

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 8 inline comments as done.
jkorous added a comment.

Addressed some comments, going to update the diff.




Comment at: clang/lib/Index/IndexRecordHasher.cpp:291
+
+hash_code IndexRecordHasher::hashImpl(const Decl *D) {
+  return DeclHashVisitor(*this).Visit(D);

gribozavr wrote:
> hashImpl => cachedHashImpl
Honestly, I am not sure this would be better. I added a comment about this set 
of methods in the header file. Wouldn't mind renaming them but think `hashImpl` 
is actually quite accurate.



Comment at: clang/lib/Index/IndexRecordHasher.cpp:409
+
+  // Unhandled type.
+  return Hash;

gribozavr wrote:
> So what is the failure mode for unhandled types, what is the effect on the 
> whole system?
Seems like just the `InitialHash` is returned at the moment.

I guess using llvm::Optional as a return type would be better. WDYT?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58749#1426769 , @kadircet

>




In D58749#1426778 , @gribozavr

>


I see what you mean now. That's a good idea. I'll add some unit tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58749/new/

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355984 - Reland "[Remarks] Add -foptimization-record-passes to filter remark emission"

[Francis Visoiu Mistrih via cfe-commits]

Author: thegameg
Date: Tue Mar 12 14:22:27 2019
New Revision: 355984

URL: http://llvm.org/viewvc/llvm-project?rev=355984&view=rev
Log:
Reland "[Remarks] Add -foptimization-record-passes to filter remark emission"

Currently we have -Rpass for filtering the remarks that are displayed as
diagnostics, but when using -fsave-optimization-record, there is no way
to filter the remarks while generating them.

This adds support for filtering remarks by passes using a regex.
Ex: `clang -fsave-optimization-record -foptimization-record-passes=inline`

will only emit the remarks coming from the pass `inline`.

This adds:

* `-fsave-optimization-record` to the driver
* `-opt-record-passes` to cc1
* `-lto-pass-remarks-filter` to the LTOCodeGenerator
* `--opt-remarks-passes` to lld
* `-pass-remarks-filter` to llc, opt, llvm-lto, llvm-lto2
* `-opt-remarks-passes` to gold-plugin

Differential Revision: https://reviews.llvm.org/D59268

Original llvm-svn: 355964

Modified:
cfe/trunk/include/clang/Basic/CodeGenOptions.h
cfe/trunk/include/clang/Driver/CC1Options.td
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/CodeGen/opt-record-MIR.c
cfe/trunk/test/CodeGen/opt-record.c
cfe/trunk/test/Driver/darwin-ld.c
cfe/trunk/test/Driver/opt-record.c

Modified: cfe/trunk/include/clang/Basic/CodeGenOptions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/CodeGenOptions.h?rev=355984&r1=355983&r2=355984&view=diff
==
--- cfe/trunk/include/clang/Basic/CodeGenOptions.h (original)
+++ cfe/trunk/include/clang/Basic/CodeGenOptions.h Tue Mar 12 14:22:27 2019
@@ -238,6 +238,10 @@ public:
   /// records.
   std::string OptRecordFile;
 
+  /// The regex that filters the passes that should be saved to the 
optimization
+  /// records.
+  std::string OptRecordPasses;
+
   /// Regular expression to select optimizations for which we should enable
   /// optimization remarks. Transformation passes whose name matches this
   /// expression (and support this feature), will emit a diagnostic

Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=355984&r1=355983&r2=355984&view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Tue Mar 12 14:22:27 2019
@@ -603,6 +603,8 @@ def arcmt_migrate : Flag<["-"], "arcmt-m
 
 def opt_record_file : Separate<["-"], "opt-record-file">,
   HelpText<"File name to use for YAML optimization record output">;
+def opt_record_passes : Separate<["-"], "opt-record-passes">,
+  HelpText<"Only record remark information for passes whose names match the 
given regular expression">;
 
 def print_stats : Flag<["-"], "print-stats">,
   HelpText<"Print performance metrics and statistics">;

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=355984&r1=355983&r2=355984&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue Mar 12 14:22:27 2019
@@ -1715,6 +1715,10 @@ def fno_save_optimization_record : Flag<
 def foptimization_record_file_EQ : Joined<["-"], "foptimization-record-file=">,
   Group,
   HelpText<"Specify the file name of any generated YAML optimization record">;
+def foptimization_record_passes_EQ : Joined<["-"], 
"foptimization-record-passes=">,
+  Group,
+  HelpText<"Only include passes which match a specified regular expression in 
the generated optimization record (by default, include all passes)">;
+
 
 def ftest_coverage : Flag<["-"], "ftest-coverage">, Group;
 def fvectorize : Flag<["-"], "fvectorize">, Group,

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=355984&r1=355983&r2=355984&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue Mar 12 14:22:27 2019
@@ -1340,6 +1340,7 @@ static void runThinLTOBackend(ModuleSumm
   Conf.DebugPassManager = CGOpts.DebugPassManager;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
+  Conf.RemarksPasses = CGOpts.OptRecordPasses;
   Conf.DwoPath = CGOpts.SplitDwarfFile;
   switch (Action) {
   case Backend_EmitNothing:

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/ll

r355987 - [NFC][clang][PCH][ObjC] Add some missing `VisitStmt(S);`

[Roman Lebedev via cfe-commits]

Author: lebedevri
Date: Tue Mar 12 14:31:00 2019
New Revision: 355987

URL: http://llvm.org/viewvc/llvm-project?rev=355987&view=rev
Log:
[NFC][clang][PCH][ObjC] Add some missing `VisitStmt(S);`

Summary:
These ObjC AST classes inherit from Stmt, but don't call `VisitStmt(S);`.
Some were founded with help of existing tests (with `NumStmtFields` bumped to 
`1`),
but some of them don't even have PCH test coverage. :/

Reviewers: arphaman, sammccall, smeenai, aprantl, rsmith, jordan_rose

Reviewed By: jordan_rose

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59197

Modified:
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

Modified: cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderStmt.cpp?rev=355987&r1=355986&r2=355987&view=diff
==
--- cfe/trunk/lib/Serialization/ASTReaderStmt.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderStmt.cpp Tue Mar 12 14:31:00 2019
@@ -1254,7 +1254,7 @@ void ASTStmtReader::VisitObjCAtFinallySt
 }
 
 void ASTStmtReader::VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setSubStmt(Record.readSubStmt());
   S->setAtLoc(ReadSourceLocation());
 }
@@ -1274,14 +1274,14 @@ void ASTStmtReader::VisitObjCAtTryStmt(O
 }
 
 void ASTStmtReader::VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setSynchExpr(Record.readSubStmt());
   S->setSynchBody(Record.readSubStmt());
   S->setAtSynchronizedLoc(ReadSourceLocation());
 }
 
 void ASTStmtReader::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setThrowExpr(Record.readSubStmt());
   S->setThrowLoc(ReadSourceLocation());
 }

Modified: cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriterStmt.cpp?rev=355987&r1=355986&r2=355987&view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriterStmt.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriterStmt.cpp Tue Mar 12 14:31:00 2019
@@ -1198,6 +1198,7 @@ void ASTStmtWriter::VisitObjCForCollecti
 }
 
 void ASTStmtWriter::VisitObjCAtCatchStmt(ObjCAtCatchStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getCatchBody());
   Record.AddDeclRef(S->getCatchParamDecl());
   Record.AddSourceLocation(S->getAtCatchLoc());
@@ -1206,18 +1207,21 @@ void ASTStmtWriter::VisitObjCAtCatchStmt
 }
 
 void ASTStmtWriter::VisitObjCAtFinallyStmt(ObjCAtFinallyStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getFinallyBody());
   Record.AddSourceLocation(S->getAtFinallyLoc());
   Code = serialization::STMT_OBJC_FINALLY;
 }
 
 void ASTStmtWriter::VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSubStmt());
   Record.AddSourceLocation(S->getAtLoc());
   Code = serialization::STMT_OBJC_AUTORELEASE_POOL;
 }
 
 void ASTStmtWriter::VisitObjCAtTryStmt(ObjCAtTryStmt *S) {
+  VisitStmt(S);
   Record.push_back(S->getNumCatchStmts());
   Record.push_back(S->getFinallyStmt() != nullptr);
   Record.AddStmt(S->getTryBody());
@@ -1230,6 +1234,7 @@ void ASTStmtWriter::VisitObjCAtTryStmt(O
 }
 
 void ASTStmtWriter::VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSynchExpr());
   Record.AddStmt(S->getSynchBody());
   Record.AddSourceLocation(S->getAtSynchronizedLoc());
@@ -1237,6 +1242,7 @@ void ASTStmtWriter::VisitObjCAtSynchroni
 }
 
 void ASTStmtWriter::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getThrowExpr());
   Record.AddSourceLocation(S->getThrowLoc());
   Code = serialization::STMT_OBJC_AT_THROW;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59197: [NFC][clang][PCH][ObjC] Add some missing `VisitStmt(S);`

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355987: [NFC][clang][PCH][ObjC] Add some missing 
`VisitStmt(S);` (authored by lebedevri, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59197/new/

https://reviews.llvm.org/D59197

Files:
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp


Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -1198,6 +1198,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtCatchStmt(ObjCAtCatchStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getCatchBody());
   Record.AddDeclRef(S->getCatchParamDecl());
   Record.AddSourceLocation(S->getAtCatchLoc());
@@ -1206,18 +1207,21 @@
 }
 
 void ASTStmtWriter::VisitObjCAtFinallyStmt(ObjCAtFinallyStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getFinallyBody());
   Record.AddSourceLocation(S->getAtFinallyLoc());
   Code = serialization::STMT_OBJC_FINALLY;
 }
 
 void ASTStmtWriter::VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSubStmt());
   Record.AddSourceLocation(S->getAtLoc());
   Code = serialization::STMT_OBJC_AUTORELEASE_POOL;
 }
 
 void ASTStmtWriter::VisitObjCAtTryStmt(ObjCAtTryStmt *S) {
+  VisitStmt(S);
   Record.push_back(S->getNumCatchStmts());
   Record.push_back(S->getFinallyStmt() != nullptr);
   Record.AddStmt(S->getTryBody());
@@ -1230,6 +1234,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSynchExpr());
   Record.AddStmt(S->getSynchBody());
   Record.AddSourceLocation(S->getAtSynchronizedLoc());
@@ -1237,6 +1242,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getThrowExpr());
   Record.AddSourceLocation(S->getThrowLoc());
   Code = serialization::STMT_OBJC_AT_THROW;
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -1254,7 +1254,7 @@
 }
 
 void ASTStmtReader::VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setSubStmt(Record.readSubStmt());
   S->setAtLoc(ReadSourceLocation());
 }
@@ -1274,14 +1274,14 @@
 }
 
 void ASTStmtReader::VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setSynchExpr(Record.readSubStmt());
   S->setSynchBody(Record.readSubStmt());
   S->setAtSynchronizedLoc(ReadSourceLocation());
 }
 
 void ASTStmtReader::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
-  VisitStmt(S);
+  VisitStmt(S); // FIXME: no test coverage.
   S->setThrowExpr(Record.readSubStmt());
   S->setThrowLoc(ReadSourceLocation());
 }


Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -1198,6 +1198,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtCatchStmt(ObjCAtCatchStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getCatchBody());
   Record.AddDeclRef(S->getCatchParamDecl());
   Record.AddSourceLocation(S->getAtCatchLoc());
@@ -1206,18 +1207,21 @@
 }
 
 void ASTStmtWriter::VisitObjCAtFinallyStmt(ObjCAtFinallyStmt *S) {
+  VisitStmt(S);
   Record.AddStmt(S->getFinallyBody());
   Record.AddSourceLocation(S->getAtFinallyLoc());
   Code = serialization::STMT_OBJC_FINALLY;
 }
 
 void ASTStmtWriter::VisitObjCAutoreleasePoolStmt(ObjCAutoreleasePoolStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSubStmt());
   Record.AddSourceLocation(S->getAtLoc());
   Code = serialization::STMT_OBJC_AUTORELEASE_POOL;
 }
 
 void ASTStmtWriter::VisitObjCAtTryStmt(ObjCAtTryStmt *S) {
+  VisitStmt(S);
   Record.push_back(S->getNumCatchStmts());
   Record.push_back(S->getFinallyStmt() != nullptr);
   Record.AddStmt(S->getTryBody());
@@ -1230,6 +1234,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getSynchExpr());
   Record.AddStmt(S->getSynchBody());
   Record.AddSourceLocation(S->getAtSynchronizedLoc());
@@ -1237,6 +1242,7 @@
 }
 
 void ASTStmtWriter::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
+  VisitStmt(S); // FIXME: no test coverage.
   Record.AddStmt(S->getThrowExpr());
   Record.AddSourceLocation(S->getThrowLoc());
   Code = serialization::STMT_OBJC_AT_THROW;
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -1254,7 +1254,7 @@
 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Sorry for the delay.  I will finish reviewing tomorrow.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:9
+/// \file
+/// \brief Utility class for listening for file system changes in a directory.
+//===--===//

This comment only repeats the doc comment for the class, I'd suggest to remove 
it here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

In D36836#1021863 , @chandlerc wrote:

> In D36836#931995 , @lebedev.ri wrote:
>
> > - Rebased
> > - As advised by @aaron.ballman, moved into it's own directory/module. 
> > Please review that, i'm not entirely sure i have done that fully correctly.
> >
> >   @chandlerc Hi! @aaron.ballman has suggested for me to try to talk to you 
> > about this. Is there some precedent for the licensing 'issue' at hand? Do 
> > you have any opinion? @dberlin did not react to the pings, so i'm not sure 
> > i personally can come up with anything better for `LICENSE.txt`
>
>
> Sadly, what you need here is legal advice for a good way to handle this, and 
> I'm not a lawyer and so I can't really give you that advice.
>
> To be clear, what you currently have isn't OK for several reasons, not least 
> of which what Aaron brought up that this is not in fact a license.
>
> > If there are no further ideas, i'll try to contact sonarsource.
>
> Unless Danny can volunteer his time, we finish with relicensing efforts


Hi @chandlerc !
I was just wondering what percentage of "finish" was meant here, and where at 
it's now?

> and can devote the foundation's lawyer's (sadly precious) time to this, I 
> think either you or sonarsource working to understand the best legal way to 
> contribute this would be the best way forward. Sorry that this is a tricky 
> situation. Happy to have a private discussion w/ your or sonarsources's 
> lawyer via my @ llvm.org email address if useful.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36836/new/

https://reviews.llvm.org/D36836



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r355989 - Add XCOFF triple object format type for AIX

[Jason Liu via cfe-commits]

Author: jasonliu
Date: Tue Mar 12 15:01:10 2019
New Revision: 355989

URL: http://llvm.org/viewvc/llvm-project?rev=355989&view=rev
Log:
Add XCOFF triple object format type for AIX

This patch adds an XCOFF triple object format type into LLVM.
This XCOFF triple object file type will be used later by object file and 
assembly generation for the AIX platform.

Differential Revision: https://reviews.llvm.org/D58930

Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=355989&r1=355988&r2=355989&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue Mar 12 15:01:10 2019
@@ -1447,6 +1447,9 @@ static const char* getSectionNameForBitc
   case Triple::Wasm:
   case Triple::UnknownObjectFormat:
 return ".llvmbc";
+  case Triple::XCOFF:
+llvm_unreachable("XCOFF is not yet implemented");
+break;
   }
   llvm_unreachable("Unimplemented ObjectFormatType");
 }
@@ -1460,6 +1463,9 @@ static const char* getSectionNameForComm
   case Triple::Wasm:
   case Triple::UnknownObjectFormat:
 return ".llvmcmd";
+  case Triple::XCOFF:
+llvm_unreachable("XCOFF is not yet implemented");
+break;
   }
   llvm_unreachable("Unimplemented ObjectFormatType");
 }

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=355989&r1=355988&r2=355989&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Mar 12 15:01:10 2019
@@ -4406,6 +4406,8 @@ CodeGenModule::GetAddrOfConstantCFString
   switch (Triple.getObjectFormat()) {
   case llvm::Triple::UnknownObjectFormat:
 llvm_unreachable("unknown file format");
+  case llvm::Triple::XCOFF:
+llvm_unreachable("XCOFF is not yet implemented");
   case llvm::Triple::COFF:
   case llvm::Triple::ELF:
   case llvm::Triple::Wasm:


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58930: Add XCOFF triple object format type for AIX

[Jason Liu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL355989: Add XCOFF triple object format type for AIX 
(authored by jasonliu, committed by ).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D58930?vs=189713&id=190346#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58930/new/

https://reviews.llvm.org/D58930

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  llvm/trunk/include/llvm/ADT/Triple.h
  llvm/trunk/include/llvm/MC/MCObjectFileInfo.h
  llvm/trunk/lib/MC/MCContext.cpp
  llvm/trunk/lib/MC/MCObjectFileInfo.cpp
  llvm/trunk/lib/MC/MCParser/AsmParser.cpp
  llvm/trunk/lib/Support/Triple.cpp
  llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/trunk/unittests/ADT/TripleTest.cpp

Index: llvm/trunk/include/llvm/ADT/Triple.h
===
--- llvm/trunk/include/llvm/ADT/Triple.h
+++ llvm/trunk/include/llvm/ADT/Triple.h
@@ -219,6 +219,7 @@
 ELF,
 MachO,
 Wasm,
+XCOFF,
   };
 
 private:
@@ -598,6 +599,11 @@
!isAndroid();
   }
 
+  /// Tests whether the OS is AIX.
+  bool isOSAIX() const {
+return getOS() == Triple::AIX;
+  }
+
   /// Tests whether the OS uses the ELF binary format.
   bool isOSBinFormatELF() const {
 return getObjectFormat() == Triple::ELF;
@@ -618,6 +624,11 @@
 return getObjectFormat() == Triple::Wasm;
   }
 
+  /// Tests whether the OS uses the XCOFF binary format.
+  bool isOSBinFormatXCOFF() const {
+return getObjectFormat() == Triple::XCOFF;
+  }
+
   /// Tests whether the target is the PS4 CPU
   bool isPS4CPU() const {
 return getArch() == Triple::x86_64 &&
Index: llvm/trunk/include/llvm/MC/MCObjectFileInfo.h
===
--- llvm/trunk/include/llvm/MC/MCObjectFileInfo.h
+++ llvm/trunk/include/llvm/MC/MCObjectFileInfo.h
@@ -380,7 +380,7 @@
 return EHFrameSection;
   }
 
-  enum Environment { IsMachO, IsELF, IsCOFF, IsWasm };
+  enum Environment { IsMachO, IsELF, IsCOFF, IsWasm, IsXCOFF };
   Environment getObjectFileType() const { return Env; }
 
   bool isPositionIndependent() const { return PositionIndependent; }
Index: llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
===
--- llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -5594,6 +5594,9 @@
   case MCObjectFileInfo::IsWasm:
 CurrentFormat = WASM;
 break;
+  case MCObjectFileInfo::IsXCOFF:
+llvm_unreachable("unexpected object format");
+break;
   }
 
   if (~Prefix->SupportedFormats & CurrentFormat) {
Index: llvm/trunk/lib/Support/Triple.cpp
===
--- llvm/trunk/lib/Support/Triple.cpp
+++ llvm/trunk/lib/Support/Triple.cpp
@@ -534,6 +534,9 @@
 
 static Triple::ObjectFormatType parseFormat(StringRef EnvironmentName) {
   return StringSwitch(EnvironmentName)
+// "xcoff" must come before "coff" because of the order-dependendent
+// pattern matching.
+.EndsWith("xcoff", Triple::XCOFF)
 .EndsWith("coff", Triple::COFF)
 .EndsWith("elf", Triple::ELF)
 .EndsWith("macho", Triple::MachO)
@@ -622,6 +625,7 @@
   case Triple::ELF: return "elf";
   case Triple::MachO: return "macho";
   case Triple::Wasm: return "wasm";
+  case Triple::XCOFF: return "xcoff";
   }
   llvm_unreachable("unknown object format type");
 }
@@ -686,6 +690,8 @@
   case Triple::ppc64:
 if (T.isOSDarwin())
   return Triple::MachO;
+else if (T.isOSAIX())
+  return Triple::XCOFF;
 return Triple::ELF;
 
   case Triple::wasm32:
Index: llvm/trunk/lib/MC/MCParser/AsmParser.cpp
===
--- llvm/trunk/lib/MC/MCParser/AsmParser.cpp
+++ llvm/trunk/lib/MC/MCParser/AsmParser.cpp
@@ -710,6 +710,9 @@
   case MCObjectFileInfo::IsWasm:
 PlatformParser.reset(createWasmAsmParser());
 break;
+  case MCObjectFileInfo::IsXCOFF:
+// TODO: Need to implement createXCOFFAsmParser for XCOFF format.
+break;
   }
 
   PlatformParser->Initialize(*this);
Index: llvm/trunk/lib/MC/MCContext.cpp
===
--- llvm/trunk/lib/MC/MCContext.cpp
+++ llvm/trunk/lib/MC/MCContext.cpp
@@ -161,6 +161,9 @@
   return new (Name, *this) MCSymbolMachO(Name, IsTemporary);
 case MCObjectFileInfo::IsWasm:
   return new (Name, *this) MCSymbolWasm(Name, IsTemporary);
+case MCObjectFileInfo::IsXCOFF:
+  // TODO: Need to implement class MCSymbolXCOFF.
+  break;
 }
   }
   return new (Name, *this) MCSymbol(MCSymbol::SymbolKindUnset, Name,
I

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 190351.
MyDeveloperDay added a comment.

run git clang-format


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59087/new/

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -98,7 +98,7 @@
 }
 
 TEST_F(FormatTestSelective, FormatsIfWithoutCompoundStatement) {
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   EXPECT_EQ("if (a) return;", format("if(a)\nreturn;", 7, 1));
   EXPECT_EQ("if (a) return; // comment",
 format("if(a)\nreturn; // comment", 20, 1));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -439,7 +439,8 @@
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
   AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
-  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   verifyFormat("if (a)\n"
"  // comment\n"
"  f();",
@@ -487,6 +488,41 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else\n"
+   "  g();\n",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
+
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
@@ -515,7 +551,8 @@
   AllowSimpleBracedStatements.ColumnLimit = 40;
   AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
@@ -563,7 +600,8 @@
"};",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  f();\n"
@@ -588,7 +626,8 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
   AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
 
@@ -625,7 +664,8 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
@@ -659,7 +699,7 @@
 TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(60);
   Style.AllowShortBlocksOnASingleLine = true;
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A