[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:22
+
+// Find a better way of detecting const-reference-template type
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.

You can write `TODO: ` or `FIXME: ` in front of this comment to make it well 
visible.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

This indeed looks a bit ugly. Is there no check that skips const-ref template 
params and handles `using`s / `typedef`s?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+
+// Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType &ParamType) {

`TODO: ` too.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType &ParamType) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}

I'm not sure if you can find a better way to find parameters of type 
`std::function` than this... Unless we find the rules that distinguish function 
types from others.
Why is `std::function` so different? How could we match `boost::function` and 
alike types?

Just setting the questions, I have no answers.

Anyway, I think that this might be left for later to be improved.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+AST_MATCHER(CXXMethodDecl, hasTemplateReturnType) {
+  // Don't put [[nodiscard]] int front functions return T
+  return (Node.getReturnType()->isTemplateTypeParmType() ||

int front ... -> in front of functions that return T.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:23
+via the return type. Unless the member functions are using mutable member
+variables or alteringing state via some external call (e.g. I/O).
+

Typo: alteringing



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:162
+using reference = value_type&;
+using const_reference = const value_type&;
+

Could you use similar test cases for `typedef`s, please?


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

https://reviews.llvm.org/D55433



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

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D55650#1329692 , @kadircet wrote:

> I think the description is misleading, you are saying that we were triggering 
> an assertion and you are fixing that. But in reality, we were not triggering 
> any assertions, this patch is adding the assertions right?


Ah, sorry for the confusion caused by the assertion I added. The description is 
correct, and this patch is trying to fix an assertion (not the assertion I 
added, it is the assertion in `llvm::Optional`, we are accessing an empty 
`Optional`).




Comment at: clangd/index/Background.cpp:385
 
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");

kadircet wrote:
> I don't think it is a good idea to assert and die just for one file, since we 
> are actually trying to index all the files. We should rather check these 
> exists and propagate the error in case of failure. You can have a look at 
> https://reviews.llvm.org/D55224, same lines.
> 
> Also with your changes in the endsourcefileaction this assertion is not 
> possible to be triggered, since you will fill them with "empty" versions.
`check these exists` seems a wired way to me, We should explicitly check the 
error here.

The assertion I added here is to guarantee that we have all the data when 
indexing a file successfully. 



Comment at: clangd/index/IndexAction.cpp:150
+assert(SymbolsCallback && "SymbolsCallback must be set.");
+SymbolsCallback(std::move(Symbols));
+if (RefsCallback)

kadircet wrote:
> I don't think it is a good idea to respond with an "empty" symbol slab rather 
> than not replying. They have different semantics. The former implies 
> everything went well, just couldn't find any symbols in the file, whereas the 
> latter implies there was an error.
> 
> So I believe we should keep this code as it was.
Sounds fair, I kept the old behavior.
but we have different behavior in `clangd-indexer` -- `clangd-indexer` will 
emit an empty index file when processing a problematic cpp file... a tradeoff 
is that we will reindex the problematic cpp file since we don't emit any index 
file, and the `backgroundindex `  thinks this file is not being indexed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650



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

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 178203.
hokein marked 5 inline comments as done.
hokein added a comment.

Keep the old behavior.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650

Files:
  clangd/index/Background.cpp
  unittests/clangd/BackgroundIndexTests.cpp


Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -66,6 +66,25 @@
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -381,6 +381,14 @@
   if (!Action->Execute())
 return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+return createStringError(inconvertibleErrorCode(),
+ "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
   Inputs.CompileCommand.Filename, Index.Symbols->size(),


Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -66,6 +66,25 @@
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -381,6 +381,14 @@
   if (!Action->Execute())
 return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+return createStringError(inconvertibleErrorCode(),
+ "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
   Inputs.CompileCommand.Filename, Index.Symbols->size(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55697: [analyzer] Assume that we always have a SubEngine available

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: NoQ, george.karpenkov.
xazax.hun added a project: clang.
Herald added subscribers: gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.

I was stumbled upon some checks whether a SubEngine is available. I was 
wondering why do we have those checks so I removed them and wondered if the 
tests would break. They did not break on my machine, so I submitted this patch.

Do yo think these should be removed? If no, how could we test those code paths 
because apparently they are untested at the moment.


Repository:
  rC Clang

https://reviews.llvm.org/D55697

Files:
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp

Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -347,11 +347,10 @@
 : StoreManager(mgr), Features(f),
   RBFactory(mgr.getAllocator()), CBFactory(mgr.getAllocator()),
   SmallStructLimit(0) {
-if (SubEngine *Eng = StateMgr.getOwningEngine()) {
-  AnalyzerOptions &Options = Eng->getAnalysisManager().options;
-  SmallStructLimit =
-Options.RegionStoreSmallStructLimit;
-}
+SubEngine *Eng = StateMgr.getOwningEngine();
+AnalyzerOptions &Options = Eng->getAnalysisManager().options;
+SmallStructLimit =
+  Options.RegionStoreSmallStructLimit;
   }
 
 
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -125,7 +125,7 @@
   ProgramStateRef newState = makeWithStore(Mgr.StoreMgr->Bind(getStore(),
  LV, V));
   const MemRegion *MR = LV.getAsRegion();
-  if (MR && Mgr.getOwningEngine() && notifyChanges)
+  if (MR && notifyChanges)
 return Mgr.getOwningEngine()->processRegionChange(newState, MR, LCtx);
 
   return newState;
@@ -138,9 +138,7 @@
   const MemRegion *R = loc.castAs().getRegion();
   const StoreRef &newStore = Mgr.StoreMgr->BindDefaultInitial(getStore(), R, V);
   ProgramStateRef new_state = makeWithStore(newStore);
-  return Mgr.getOwningEngine()
- ? Mgr.getOwningEngine()->processRegionChange(new_state, R, LCtx)
- : new_state;
+  return Mgr.getOwningEngine()->processRegionChange(new_state, R, LCtx);
 }
 
 ProgramStateRef
@@ -149,9 +147,7 @@
   const MemRegion *R = loc.castAs().getRegion();
   const StoreRef &newStore = Mgr.StoreMgr->BindDefaultZero(getStore(), R);
   ProgramStateRef new_state = makeWithStore(newStore);
-  return Mgr.getOwningEngine()
- ? Mgr.getOwningEngine()->processRegionChange(new_state, R, LCtx)
- : new_state;
+  return Mgr.getOwningEngine()->processRegionChange(new_state, R, LCtx);
 }
 
 typedef ArrayRef RegionList;
@@ -198,39 +194,32 @@
   ProgramStateManager &Mgr = getStateManager();
   SubEngine* Eng = Mgr.getOwningEngine();
 
-  InvalidatedSymbols Invalidated;
+  InvalidatedSymbols InvalidatedSyms;
   if (!IS)
-IS = &Invalidated;
+IS = &InvalidatedSyms;
 
   RegionAndSymbolInvalidationTraits ITraitsLocal;
   if (!ITraits)
 ITraits = &ITraitsLocal;
 
-  if (Eng) {
-StoreManager::InvalidatedRegions TopLevelInvalidated;
-StoreManager::InvalidatedRegions Invalidated;
-const StoreRef &newStore
-= Mgr.StoreMgr->invalidateRegions(getStore(), Values, E, Count, LCtx, Call,
-  *IS, *ITraits, &TopLevelInvalidated,
-  &Invalidated);
-
-ProgramStateRef newState = makeWithStore(newStore);
-
-if (CausedByPointerEscape) {
-  newState = Eng->notifyCheckersOfPointerEscape(newState, IS,
-TopLevelInvalidated,
-Call,
-*ITraits);
-}
+  StoreManager::InvalidatedRegions TopLevelInvalidated;
+  StoreManager::InvalidatedRegions Invalidated;
+  const StoreRef &newStore
+  = Mgr.StoreMgr->invalidateRegions(getStore(), Values, E, Count, LCtx, Call,
+*IS, *ITraits, &TopLevelInvalidated,
+&Invalidated);
+
+  ProgramStateRef newState = makeWithStore(newStore);
 
-return Eng->processRegionChanges(newState, IS, TopLevelInvalidated,
- Invalidated, LCtx, Call);
+  if (CausedByPointerEscape) {
+newState = Eng->notifyCheckersOfPointerEscape(newState, IS,
+  TopLevelInvalidated,
+  Call,
+  *ITraits);
   }
 
-  const StoreRef &newStore =
-  Mgr.StoreMgr->invalidate

[PATCH] D55697: [analyzer] Assume that we always have a SubEngine available

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

Szelethus added a comment.

We could also add an assert to getOwningEngine I guess.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55697



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

[PATCH] D55698: [MinGW] Produce a vtable and RTTI for dllexported classes without a key function

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: rnk, majnemer.

This matches what GCC does in these situations.

This fixes compiling Qt in debug mode. In release mode, references to the 
vtable of this particular class ends up optimized away, but in debug mode, the 
compiler creates references to the vtable, which is expected to be dllexported 
from a different DLL. Make sure the dllexported version actually ends up 
emitted.


Repository:
  rC Clang

https://reviews.llvm.org/D55698

Files:
  lib/Sema/SemaDeclCXX.cpp
  test/CodeGenCXX/dllexport-missing-key.cpp


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s |
+// FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5750,6 +5750,10 @@
 
   if (ClassExported)
 DelayedDllExportClasses.push_back(Class);
+
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);
 }
 
 /// Perform propagation of DLL attributes from a derived class to a


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s |
+// FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5750,6 +5750,10 @@
 
   if (ClassExported)
 DelayedDllExportClasses.push_back(Class);
+
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);
 }
 
 /// Perform propagation of DLL attributes from a derived class to a
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Ah you are right, the Optional.. Thanks!




Comment at: clangd/index/Background.cpp:384
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {

Then don't need to keep these checks anymore in the endsourcefileaction of 
indexingaction right?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650



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

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.



Comment at: clangd/index/Background.cpp:384
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {

kadircet wrote:
> Then don't need to keep these checks anymore in the endsourcefileaction of 
> indexingaction right?
We need indeed. We have other clients (`clangd-indexer`, and our internal 
indexer) using the `IndexingAction` -- we don't want symbols from broken TUs 
because they causes many troublesomes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650



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

[libunwind] r349140 - [AArch64][libunwind] Unwinding support for return address signing

[Luke Cheeseman via cfe-commits]

Author: lukecheeseman
Date: Fri Dec 14 03:30:12 2018
New Revision: 349140

URL: http://llvm.org/viewvc/llvm-project?rev=349140&view=rev
Log:
[AArch64][libunwind] Unwinding support for return address signing

- Follow up to revision r342895
- gcc would not build libunwind with the earlier patch as the autia1716
  instruction wasn't allowed to be assembled for pre armv8.3a targets
- The autia1716 instruction lives in the hint space encodings so is a valid
  instruction for all armv8a targets
- To work around this I have swapped out the autia1716 instruction for the hint
  instruction

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


Modified:
libunwind/trunk/include/libunwind.h
libunwind/trunk/src/DwarfInstructions.hpp
libunwind/trunk/src/DwarfParser.hpp
libunwind/trunk/src/Registers.hpp
libunwind/trunk/src/dwarf2.h

Modified: libunwind/trunk/include/libunwind.h
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/include/libunwind.h?rev=349140&r1=349139&r2=349140&view=diff
==
--- libunwind/trunk/include/libunwind.h (original)
+++ libunwind/trunk/include/libunwind.h Fri Dec 14 03:30:12 2018
@@ -57,6 +57,9 @@ enum {
   UNW_EINVAL= -6547, /* unsupported operation or bad value */
   UNW_EBADVERSION   = -6548, /* unwind info has unsupported version */
   UNW_ENOINFO   = -6549  /* no unwind info found */
+#if defined(_LIBUNWIND_TARGET_AARCH64) && !defined(_LIBUNWIND_IS_NATIVE_ONLY)
+  , UNW_ECROSSRASIGNING = -6550 /* cross unwind with return address signing */
+#endif
 };
 
 struct unw_context_t {
@@ -547,6 +550,8 @@ enum {
   UNW_ARM64_X31 = 31,
   UNW_ARM64_SP  = 31,
   // reserved block
+  UNW_ARM64_RA_SIGN_STATE = 34,
+  // reserved block
   UNW_ARM64_D0  = 64,
   UNW_ARM64_D1  = 65,
   UNW_ARM64_D2  = 66,

Modified: libunwind/trunk/src/DwarfInstructions.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfInstructions.hpp?rev=349140&r1=349139&r2=349140&view=diff
==
--- libunwind/trunk/src/DwarfInstructions.hpp (original)
+++ libunwind/trunk/src/DwarfInstructions.hpp Fri Dec 14 03:30:12 2018
@@ -198,6 +198,27 @@ int DwarfInstructions::stepWithDwa
   // restoring SP means setting it to CFA.
   newRegisters.setSP(cfa);
 
+#if defined(_LIBUNWIND_TARGET_AARCH64)
+  // If the target is aarch64 then the return address may have been signed
+  // using the v8.3 pointer authentication extensions. The original
+  // return address needs to be authenticated before the return address is
+  // restored. autia1716 is used instead of autia as autia1716 assembles
+  // to a NOP on pre-v8.3a architectures.
+  if (prolog.savedRegisters[UNW_ARM64_RA_SIGN_STATE].value) {
+#if !defined(_LIBUNWIND_IS_NATIVE_ONLY)
+return UNW_ECROSSRASIGNING;
+#else
+register unsigned long long x17 __asm("x17") = returnAddress;
+register unsigned long long x16 __asm("x16") = cfa;
+
+// This is the autia1716 instruction. The hint instruction is used here
+// as gcc does not assemble autia1716 for pre armv8.3a targets.
+asm("hint 0xc": "+r"(x17): "r"(x16));
+returnAddress = x17;
+#endif
+  }
+#endif
+
   // Return address is address after call site instruction, so setting IP 
to
   // that does simualates a return.
   newRegisters.setIP(returnAddress);

Modified: libunwind/trunk/src/DwarfParser.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=349140&r1=349139&r2=349140&view=diff
==
--- libunwind/trunk/src/DwarfParser.hpp (original)
+++ libunwind/trunk/src/DwarfParser.hpp Fri Dec 14 03:30:12 2018
@@ -666,6 +666,14 @@ bool CFI_Parser::parseInstructions(A
   _LIBUNWIND_TRACE_DWARF(
   "DW_CFA_GNU_negative_offset_extended(%" PRId64 ")\n", offset);
   break;
+
+#if defined(_LIBUNWIND_TARGET_AARCH64)
+case DW_CFA_AARCH64_negate_ra_state:
+  results->savedRegisters[UNW_ARM64_RA_SIGN_STATE].value ^= 0x1;
+  _LIBUNWIND_TRACE_DWARF("DW_CFA_AARCH64_negate_ra_state\n");
+  break;
+#endif
+
 default:
   operand = opcode & 0x3F;
   switch (opcode & 0xC0) {

Modified: libunwind/trunk/src/Registers.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Registers.hpp?rev=349140&r1=349139&r2=349140&view=diff
==
--- libunwind/trunk/src/Registers.hpp (original)
+++ libunwind/trunk/src/Registers.hpp Fri Dec 14 03:30:12 2018
@@ -1786,7 +1786,7 @@ private:
 uint64_t __lr;// Link register x30
 uint64_t __sp;// Stack pointer x31
 uint64_t __pc;// Program counter
-uint64_t padding; // 16-byte align
+uint64_t __ra_sign_state; // RA sign state register
   };
 
   GPRs_registers;
@@ -1822,6 

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: clangd/index/Background.cpp:384
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {

hokein wrote:
> kadircet wrote:
> > Then don't need to keep these checks anymore in the endsourcefileaction of 
> > indexingaction right?
> We need indeed. We have other clients (`clangd-indexer`, and our internal 
> indexer) using the `IndexingAction` -- we don't want symbols from broken TUs 
> because they causes many troublesomes.
Ah you are right.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650



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

[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl created this revision.
r.stahl added reviewers: NoQ, dcoughlin, george.karpenkov.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

The LocationE parameter of evalStore is documented as "The location expression 
that is stored to". When storing from an increment / decrement operator this 
was not satisfied. In user code this causes an inconsistency between the SVal 
and Stmt parameters of checkLocation.


Repository:
  rC Clang

https://reviews.llvm.org/D55701

Files:
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -21,16 +21,7 @@
 namespace ento {
 namespace {
 
-class CustomChecker : public Checker {
-public:
-  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
-BugReporter &BR) const {
-BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
-   "Custom diagnostic description",
-   PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
-  }
-};
-
+template 
 class TestAction : public ASTFrontendAction {
   class DiagConsumer : public PathDiagnosticConsumer {
 llvm::raw_ostream &Output;
@@ -59,22 +50,55 @@
 Compiler.getAnalyzerOpts()->CheckersControlList = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-  Registry.addChecker("custom.CustomChecker", "Description");
+  Registry.addChecker("custom.CustomChecker", "Description");
 });
 return std::move(AnalysisConsumer);
   }
 };
 
+template 
+bool runCheckerOnCode(const std::string &Code, std::string &Diags) {
+  llvm::raw_string_ostream OS(Diags);
+  return tooling::runToolOnCode(new TestAction(OS), Code);
+}
+template 
+bool runCheckerOnCode(const std::string &Code) {
+  std::string Diags;
+  return runCheckerOnCode(Code, Diags);
+}
+
+
+class CustomChecker : public Checker {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+BugReporter &BR) const {
+BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
+   "Custom diagnostic description",
+   PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+  }
+};
 
 TEST(RegisterCustomCheckers, RegisterChecker) {
   std::string Diags;
-  {
-llvm::raw_string_ostream OS(Diags);
-EXPECT_TRUE(tooling::runToolOnCode(new TestAction(OS), "void f() {;}"));
-  }
+  EXPECT_TRUE(runCheckerOnCode("void f() {;}", Diags));
   EXPECT_EQ(Diags, "custom.CustomChecker:Custom diagnostic description");
 }
 
+class LocIncDecChecker : public Checker {
+public:
+  void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
+ CheckerContext &C) const {
+auto UnaryOp = dyn_cast(S);
+if (UnaryOp && !IsLoad)
+  EXPECT_FALSE(UnaryOp->isIncrementOp());
+  }
+};
+
+TEST(RegisterCustomCheckers, CheckLocationIncDec) {
+  EXPECT_TRUE(
+  runCheckerOnCode("void f() { int *p; (*p)++; }"));
+}
+
 }
 }
 }
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -1052,7 +1052,7 @@
   // Perform the store, so that the uninitialized value detection happens.
   Bldr.takeNodes(*I);
   ExplodedNodeSet Dst3;
-  evalStore(Dst3, U, U, *I, state, loc, V2_untested);
+  evalStore(Dst3, U, Ex, *I, state, loc, V2_untested);
   Bldr.addNodes(Dst3);
 
   continue;
@@ -1120,7 +1120,7 @@
 // Perform the store.
 Bldr.takeNodes(*I);
 ExplodedNodeSet Dst3;
-evalStore(Dst3, U, U, *I, state, loc, Result);
+evalStore(Dst3, U, Ex, *I, state, loc, Result);
 Bldr.addNodes(Dst3);
   }
   Dst.insert(Dst2);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

cfe-dev thread with short discussion: 
http://lists.llvm.org/pipermail/cfe-dev/2018-April/057562.html


Repository:
  rC Clang

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

https://reviews.llvm.org/D55701



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.
Herald added subscribers: dkrupp, donat.nagy, Szelethus, mikhail.ramalho, 
baloghadamsoftware.

Please let me know if there is anything else that should be addressed.

Note that even though this diff is quite large, most is just renaming things to 
stay consistent.


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

https://reviews.llvm.org/D46421



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178217.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

Fix review comments

- minor changes to code comments
- add more unit tests for typedef'd template types


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,227 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#include 
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned& my_unsigned_reference;
+typedef const unsigned& my_unsigned_const_reference;
+
+class Foo
+{
+public:
+using size_type = unsigned;
+
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+   
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+
+BOOLEAN_FUNC;
+// CHECK-MESSAGES-NOT: warning:
+
+bool f24(size_type) const;
+  

[PATCH] D55702: [clangd] Fix memory leak in ClangdTests.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: kadircet.
Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, ilya-biryukov.

createInvocationFromCommandLine sets DisableFree to true by default,
which leads memory leak in clangd. The fix is to  use the 
`BuildCompilationInvocation`
to create CI with the correct options (DisableFree is false).

Fix https://bugs.llvm.org/show_bug.cgi?id=39991.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55702

Files:
  unittests/clangd/TestTU.cpp


Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -38,8 +38,10 @@
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, 
HeaderCode}});
   auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *createInvocationFromCommandLine(Cmd),
+  buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);


Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -38,8 +38,10 @@
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
   auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *createInvocationFromCommandLine(Cmd),
+  buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks for the review.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650



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

[clang-tools-extra] r349144 - [clangd] Fix an assertion failure in background index.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Dec 14 04:39:08 2018
New Revision: 349144

URL: http://llvm.org/viewvc/llvm-project?rev=349144&view=rev
Log:
[clangd] Fix an assertion failure in background index.

Summary:
When indexing a file which contains an uncompilable error, we will
trigger an assertion failure -- the IndexFileIn data is not set, but we
access them in the backgound index.

Reviewers: kadircet

Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=349144&r1=349143&r2=349144&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Fri Dec 14 04:39:08 2018
@@ -382,6 +382,14 @@ Error BackgroundIndex::index(tooling::Co
   if (!Action->Execute())
 return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+return createStringError(inconvertibleErrorCode(),
+ "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
   Inputs.CompileCommand.Filename, Index.Symbols->size(),

Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=349144&r1=349143&r2=349144&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Fri Dec 
14 04:39:08 2018
@@ -66,6 +66,25 @@ protected:
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.


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

[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349144: [clangd] Fix an assertion failure in background 
index. (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55650

Files:
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
@@ -66,6 +66,25 @@
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -382,6 +382,14 @@
   if (!Action->Execute())
 return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+return createStringError(inconvertibleErrorCode(),
+ "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
   Inputs.CompileCommand.Filename, Index.Symbols->size(),


Index: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
@@ -66,6 +66,25 @@
   BackgroundIndexTest() { preventThreadStarvationInTests(); }
 };
 
+TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.cc")] = "error file";
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+  [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+}
+
 TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -382,6 +382,14 @@
   if (!Action->Execute())
 return createStringError(inconvertibleErrorCode(), "Execute() failed");
   Action->EndSourceFile();
+  if (Clang->hasDiagnostics() &&
+  Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+return createStringError(inconvertibleErrorCode(),
+ "IndexingAction failed: has uncompilable errors");
+  }
+
+  assert(Index.Symbols && Index.Refs && Index.Sources
+ && "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
   Inputs.CompileCommand.Filename, Index.Symbols->size(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55702: [clangd] Fix memory leak in ClangdTests.

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

With this patch applied we don't see any issues on our end. Thanks for the help!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55702



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 7 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

curdeius wrote:
> This indeed looks a bit ugly. Is there no check that skips const-ref template 
> params and handles `using`s / `typedef`s?
I'm half hoping @JonasToth might guide me to something better ;-) 



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType &ParamType) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}

curdeius wrote:
> I'm not sure if you can find a better way to find parameters of type 
> `std::function` than this... Unless we find the rules that distinguish 
> function types from others.
> Why is `std::function` so different? How could we match `boost::function` and 
> alike types?
> 
> Just setting the questions, I have no answers.
> 
> Anyway, I think that this might be left for later to be improved.
I know, I didn't like it either... I was trying to exclude a lambda being 
passed in, just to limit the heuristic and thus reduce the SNR


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

https://reviews.llvm.org/D55433



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

[PATCH] D55698: [MinGW] Produce a vtable and RTTI for dllexported classes without a key function

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 178222.
mstorsjo added a comment.

Fixed wrapping of the RUN line in the test.


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

https://reviews.llvm.org/D55698

Files:
  lib/Sema/SemaDeclCXX.cpp
  test/CodeGenCXX/dllexport-missing-key.cpp


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s | 
FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5750,6 +5750,10 @@
 
   if (ClassExported)
 DelayedDllExportClasses.push_back(Class);
+
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);
 }
 
 /// Perform propagation of DLL attributes from a derived class to a


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s | FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5750,6 +5750,10 @@
 
   if (ClassExported)
 DelayedDllExportClasses.push_back(Class);
+
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);
 }
 
 /// Perform propagation of DLL attributes from a derived class to a
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55702: [clangd] Fix memory leak in ClangdTests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55702



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

[clang-tools-extra] r349145 - [clangd] Fix memory leak in ClangdTests.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Dec 14 05:19:38 2018
New Revision: 349145

URL: http://llvm.org/viewvc/llvm-project?rev=349145&view=rev
Log:
[clangd] Fix memory leak in ClangdTests.

Summary:
createInvocationFromCommandLine sets DisableFree to true by default,
which leads memory leak in clangd. The fix is to  use the 
`BuildCompilationInvocation`
to create CI with the correct options (DisableFree is false).

Fix https://bugs.llvm.org/show_bug.cgi?id=39991.

Reviewers: kadircet

Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

Modified:
clang-tools-extra/trunk/unittests/clangd/TestTU.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestTU.cpp?rev=349145&r1=349144&r2=349145&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TestTU.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestTU.cpp Fri Dec 14 05:19:38 2018
@@ -38,8 +38,10 @@ ParsedAST TestTU::build() const {
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, 
HeaderCode}});
   auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *createInvocationFromCommandLine(Cmd),
+  buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);


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

[PATCH] D55702: [clangd] Fix memory leak in ClangdTests.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349145: [clangd] Fix memory leak in ClangdTests. (authored 
by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55702

Files:
  clang-tools-extra/trunk/unittests/clangd/TestTU.cpp


Index: clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
@@ -38,8 +38,10 @@
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, 
HeaderCode}});
   auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *createInvocationFromCommandLine(Cmd),
+  buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);


Index: clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
@@ -38,8 +38,10 @@
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
   auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *createInvocationFromCommandLine(Cmd),
+  buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2667
+Because multiple push directives can be nested, if you're writing a macro that
+expands to ``_Pragma("clang attribute")`` it's good hygiene to add a label to
+your push/pop directives. A pop directive with a label will pop the innermost

The documentation reads like the label is optional, but the examples make it 
look required (because all examples have the label present). Can you clarify 
what happens when the label is omitted and keep an example that shows it?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:840-842
+def err_pragma_attribute_stack_mismatch_label : Error<
+  "'#pragma clang attribute pop %0' with no matching "
+  "'#pragma clang attribute push %0'">;

Can this be combined with the diagnostic above using `%select` (given that the 
wording is basically identical)?



Comment at: clang/include/clang/Sema/Sema.h:8504
+  void ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
+ IdentifierInfo *PushLabel);
 

Can you sprinkle some const-correctness around to make this `const 
IdentifierInfo *`?



Comment at: clang/lib/Sema/SemaAttr.cpp:648
 
-  for (const PragmaAttributeEntry &Entry :
-   PragmaAttributeStack.back().Entries) {
-if (!Entry.IsUsed) {
-  assert(Entry.Attribute && "Expected an attribute");
-  Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
-  << Entry.Attribute->getName();
-  Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+  // Dig back through the stack trying to find the group that corrisponds to
+  // PushLabel. Note that this works fine if no label is present, think of

corrisponds -> corresponds



Comment at: clang/lib/Sema/SemaAttr.cpp:659
+  Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
+  << Entry.Attribute->getName();
+  Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);

You can pass in `*Entry.Attribute` here instead of calling `getName()` and it 
will properly quote the results.



Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' 
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang 
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push 
NOT_MY_LABEL'}}

Should we really treat this as an error? It seems to me that this should be a 
warning because pop without a label could be viewed as "I don't care what I'm 
popping, just pop it". Still worth warning about, but maybe not worth stopping 
a build over.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

I feel like this should be diagnosed, perhaps even as an error. The user 
provided labels but then got the push and pop order wrong when explicitly 
saying what to pop. That seems more likely to be a logic error on the user's 
part.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55628



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Sorry for the delay. The changes are looking good to me, although I think it 
might be worth to split this up into two patch, one NFC with the renaming, and 
one that actually introduces the changes.

@martong could you also take a look?

First, we would also like to test this internally. just to make sure it works 
as intended. Could you rebase the path to current top of tree?

Thanks!




Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:350
+  bool VisitVarDecl(VarDecl *VD) {
+if (!Opts->naiveCTUEnabled())
+  return true;

Maybe we should also opt out for non-const VarDecls to avoid even attempting to 
import the initializer if we will not find it anyways.


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

https://reviews.llvm.org/D46421



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

[PATCH] D55654: [clang] [Driver] [NetBSD] Add -D_REENTRANT when using sanitizers

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

Please normalize the description to resemble a typical commit message.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55654



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

[PATCH] D55552: [Sema] Better static assert diagnostics for expressions involving temporaries.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/Stmt.h:847
+   const ASTContext *Context = nullptr,
+   bool PrintCanonicalTypes = false) const;
 

I'm pretty wary of long lists of parameters that include booleans with default 
values. Why is `PrintCanonicalTypes` not part of the printing policy? I think 
that's where we should keep all the knobs relating to how the printing happens.


Repository:
  rC Clang

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

https://reviews.llvm.org/D2



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

[PATCH] D53713: Add extension to always default-initialize nullptr_t.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: NoQ, george.karpenkov.
aaron.ballman added a comment.

Adding some reviewers for the static analyzer questions raised.

I think this is a reasonable approach, but are there situations where we should 
diagnose this as an extension? I can't think of one, but I figured the question 
should still be asked.


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

https://reviews.llvm.org/D53713



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

[clang-tools-extra] r349148 - [clangd] Use buildCompilerInvocation to simplify the HeadersTests, NFC.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Dec 14 05:49:00 2018
New Revision: 349148

URL: http://llvm.org/viewvc/llvm-project?rev=349148&view=rev
Log:
[clangd] Use buildCompilerInvocation to simplify the HeadersTests, NFC.

Modified:
clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp?rev=349148&r1=349147&r2=349148&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp Fri Dec 14 
05:49:00 2018
@@ -44,17 +44,11 @@ private:
 auto VFS = FS.getFileSystem();
 VFS->setCurrentWorkingDirectory(Cmd->Directory);
 
-std::vector Argv;
-for (const auto &S : Cmd->CommandLine)
-  Argv.push_back(S.c_str());
-auto CI = clang::createInvocationFromCommandLine(
-Argv,
-CompilerInstance::createDiagnostics(new DiagnosticOptions(),
-&IgnoreDiags, false),
-VFS);
+ParseInputs PI;
+PI.CompileCommand = *Cmd;
+PI.FS = VFS;
+auto CI = buildCompilerInvocation(PI);
 EXPECT_TRUE(static_cast(CI));
-CI->getFrontendOpts().DisableFree = false;
-
 // The diagnostic options must be set before creating a CompilerInstance.
 CI->getDiagnosticOpts().IgnoreWarnings = true;
 auto Clang = prepareCompilerInstance(


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

[clang-tools-extra] r349150 - clang-include-fixer.el: support remote files

[Philipp Stephani via cfe-commits]

Author: phst
Date: Fri Dec 14 05:56:05 2018
New Revision: 349150

URL: http://llvm.org/viewvc/llvm-project?rev=349150&view=rev
Log:
clang-include-fixer.el: support remote files

Summary: Support remote files (e.g., Tramp) in the Emacs integration for 
clang-include-fixer

Reviewers: klimek

Reviewed By: klimek

Subscribers: cfe-commits

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

Modified:
clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.el

Modified: clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.el
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.el?rev=349150&r1=349149&r2=349150&view=diff
==
--- clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.el (original)
+++ clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.el Fri Dec 
14 05:56:05 2018
@@ -93,8 +93,12 @@ temporary buffer, and CALLBACK is called
 buffer as only argument."
   (unless buffer-file-name
 (user-error "clang-include-fixer works only in buffers that visit a file"))
-  (let ((process (if (fboundp 'make-process)
- ;; Prefer using ‘make-process’ if available, because
+  (let ((process (if (and (fboundp 'make-process)
+  ;; ‘make-process’ doesn’t support remote files
+  ;; 
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28691).
+  (not (find-file-name-handler default-directory
+   'start-file-process)))
+ ;; Prefer using ‘make-process’ if possible, because
  ;; ‘start-process’ doesn’t allow us to separate the
  ;; standard error from the output.
  (clang-include-fixer--make-process callback args)
@@ -125,7 +129,7 @@ arguments.  Return the new process objec
   :stderr stderr)))
 
 (defun clang-include-fixer--start-process (callback args)
-  "Start a new clang-incude-fixer process using `start-process'.
+  "Start a new clang-incude-fixer process using `start-file-process'.
 CALLBACK is called after the process finishes successfully; it is
 called with a single argument, the buffer where standard output
 has been inserted.  ARGS is a list of additional command line
@@ -133,7 +137,7 @@ arguments.  Return the new process objec
   (let* ((stdin (current-buffer))
  (stdout (generate-new-buffer "*clang-include-fixer output*"))
  (process-connection-type nil)
- (process (apply #'start-process "clang-include-fixer" stdout
+ (process (apply #'start-file-process "clang-include-fixer" stdout
  (clang-include-fixer--command args
 (set-process-coding-system process 'utf-8-unix 'utf-8-unix)
 (set-process-query-on-exit-flag process nil)
@@ -156,7 +160,7 @@ file name; prepends ARGS directly in fro
 ,(format "-input=%s" clang-include-fixer-init-string)
 "-stdin"
 ,@args
-,(buffer-file-name)))
+,(clang-include-fixer--file-local-name buffer-file-name)))
 
 (defun clang-include-fixer--sentinel (stdin stdout stderr callback)
   "Return a process sentinel for clang-include-fixer processes.
@@ -446,5 +450,11 @@ non-nil.  Otherwise return nil."
 (defalias 'clang-include-fixer--format-message
   (if (fboundp 'format-message) 'format-message 'format))
 
+;; ‘file-local-name’ is new in Emacs 26.1.  Provide a fallback for older
+;; versions.
+(defalias 'clang-include-fixer--file-local-name
+  (if (fboundp 'file-local-name) #'file-local-name
+(lambda (file) (or (file-remote-p file 'localname) file
+
 (provide 'clang-include-fixer)
 ;;; clang-include-fixer.el ends here


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

[PATCH] D54672: clang-include-fixer.el: support remote files

[Philipp via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE349150: clang-include-fixer.el: support remote files 
(authored by phst, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54672?vs=174522&id=178227#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54672

Files:
  include-fixer/tool/clang-include-fixer.el


Index: include-fixer/tool/clang-include-fixer.el
===
--- include-fixer/tool/clang-include-fixer.el
+++ include-fixer/tool/clang-include-fixer.el
@@ -93,8 +93,12 @@
 buffer as only argument."
   (unless buffer-file-name
 (user-error "clang-include-fixer works only in buffers that visit a file"))
-  (let ((process (if (fboundp 'make-process)
- ;; Prefer using ‘make-process’ if available, because
+  (let ((process (if (and (fboundp 'make-process)
+  ;; ‘make-process’ doesn’t support remote files
+  ;; 
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28691).
+  (not (find-file-name-handler default-directory
+   'start-file-process)))
+ ;; Prefer using ‘make-process’ if possible, because
  ;; ‘start-process’ doesn’t allow us to separate the
  ;; standard error from the output.
  (clang-include-fixer--make-process callback args)
@@ -125,7 +129,7 @@
   :stderr stderr)))
 
 (defun clang-include-fixer--start-process (callback args)
-  "Start a new clang-incude-fixer process using `start-process'.
+  "Start a new clang-incude-fixer process using `start-file-process'.
 CALLBACK is called after the process finishes successfully; it is
 called with a single argument, the buffer where standard output
 has been inserted.  ARGS is a list of additional command line
@@ -133,7 +137,7 @@
   (let* ((stdin (current-buffer))
  (stdout (generate-new-buffer "*clang-include-fixer output*"))
  (process-connection-type nil)
- (process (apply #'start-process "clang-include-fixer" stdout
+ (process (apply #'start-file-process "clang-include-fixer" stdout
  (clang-include-fixer--command args
 (set-process-coding-system process 'utf-8-unix 'utf-8-unix)
 (set-process-query-on-exit-flag process nil)
@@ -156,7 +160,7 @@
 ,(format "-input=%s" clang-include-fixer-init-string)
 "-stdin"
 ,@args
-,(buffer-file-name)))
+,(clang-include-fixer--file-local-name buffer-file-name)))
 
 (defun clang-include-fixer--sentinel (stdin stdout stderr callback)
   "Return a process sentinel for clang-include-fixer processes.
@@ -446,5 +450,11 @@
 (defalias 'clang-include-fixer--format-message
   (if (fboundp 'format-message) 'format-message 'format))
 
+;; ‘file-local-name’ is new in Emacs 26.1.  Provide a fallback for older
+;; versions.
+(defalias 'clang-include-fixer--file-local-name
+  (if (fboundp 'file-local-name) #'file-local-name
+(lambda (file) (or (file-remote-p file 'localname) file
+
 (provide 'clang-include-fixer)
 ;;; clang-include-fixer.el ends here


Index: include-fixer/tool/clang-include-fixer.el
===
--- include-fixer/tool/clang-include-fixer.el
+++ include-fixer/tool/clang-include-fixer.el
@@ -93,8 +93,12 @@
 buffer as only argument."
   (unless buffer-file-name
 (user-error "clang-include-fixer works only in buffers that visit a file"))
-  (let ((process (if (fboundp 'make-process)
- ;; Prefer using ‘make-process’ if available, because
+  (let ((process (if (and (fboundp 'make-process)
+  ;; ‘make-process’ doesn’t support remote files
+  ;; (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28691).
+  (not (find-file-name-handler default-directory
+   'start-file-process)))
+ ;; Prefer using ‘make-process’ if possible, because
  ;; ‘start-process’ doesn’t allow us to separate the
  ;; standard error from the output.
  (clang-include-fixer--make-process callback args)
@@ -125,7 +129,7 @@
   :stderr stderr)))
 
 (defun clang-include-fixer--start-process (callback args)
-  "Start a new clang-incude-fixer process using `start-process'.
+  "Start a new clang-incude-fixer process using `start-file-process'.
 CALLBACK is called after the process finishes successfully; it is
 called with a single argument, the buffer where standard output
 has been inserted.  ARGS is a list of additional command line
@@ -133,7 +137,7 @@
   (let* ((stdin (current-buffer))
  (stdout (generate-new-buffer "*clang-inc

[PATCH] D55705: [dexp] Change FuzzyFind to also print scope of symbols

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: cfe-commits, arphaman, jkorous, ioeric, ilya-biryukov.

When there are multiple symbols in the result of a fuzzy find with the
same name, one has to perform an additional query to figure out which of those
symbols are coming from the "interesting" scope. This patch prints the scope in
fuzzy find results to get rid of the second symbol.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55705

Files:
  clangd/index/dex/dexp/Dexp.cpp


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -149,7 +149,8 @@
 outs() << formatv(OutputFormat, "Rank", "Symbol ID", "Symbol Name");
 size_t Rank = 0;
 Index->fuzzyFind(Request, [&](const Symbol &Sym) {
-  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name);
+  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(),
+Sym.Scope + Sym.Name);
 });
   }
 };


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -149,7 +149,8 @@
 outs() << formatv(OutputFormat, "Rank", "Symbol ID", "Symbol Name");
 size_t Rank = 0;
 Index->fuzzyFind(Request, [&](const Symbol &Sym) {
-  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name);
+  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(),
+Sym.Scope + Sym.Name);
 });
   }
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

>> In D55044#1329604 , @hokein wrote:
>>  In fact, the existing modernize-make-unique can be configured to support 
>> absl::make_unique, you'd just need to configure the check option 
>> MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).
> 
> See https://reviews.llvm.org/D52670#change-eVDYJhWSHWeW (the 
> getModuleOptions() part) on how to do that

Thanks! Yes, it is trivial to create an alias check, but if we want to support 
`WrapUnique`, I don't think this is a right way to go.

In D55044#1330102 , @axzhang wrote:

> In D55044#1329604 , @hokein wrote:
>
> > In fact, the existing `modernize-make-unique` can be configured to support 
> > `absl::make_unique`, you'd just need to configure the check option 
> > `MakeSmartPtrFunction` to `absl::make_unique` (this is what we do inside 
> > google).
> >
> > The biggest missing part of the `modernize-make-unique` is 
> > `absl::WrapUnique`, I think we should extend `MakeSmartPtrCheck` class 
> > (maybe add hooks) to support it.
>
>
> What is the best way to extend `MakeSmartPtrCheck`? The behavior I want to 
> achieve is that `absl::WrapUnique` is suggested when brace initialization is 
> used, but `absl::make_unique` is used in all other cases.


I think we'd need to add more interfaces to `MakeSmartPtrCheck` allowing 
subclasses to inject their logic to handle different initialization styles of 
`new` expression.

- `MakeSmartPtrCheck::replaceNew` is the interesting place
- we may split `MakeSmartPtrCheck::replaceNew` implementation into different 
methods (e.g. `handleCallInit`, `handleListInit`) which are corresponding to 
handle different `new` cases
- from my experience,  handling brace initialization is tricky, there are lots 
of corner cases, you can see the comments in `MakeSmartPtrCheck::replaceNew`


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

https://reviews.llvm.org/D55044



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

[clang-tools-extra] r349152 - [dexp] Change FuzzyFind to also print scope of symbols

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Dec 14 06:17:18 2018
New Revision: 349152

URL: http://llvm.org/viewvc/llvm-project?rev=349152&view=rev
Log:
[dexp] Change FuzzyFind to also print scope of symbols

Summary:
When there are multiple symbols in the result of a fuzzy find with the
same name, one has to perform an additional query to figure out which of those
symbols are coming from the "interesting" scope. This patch prints the scope in
fuzzy find results to get rid of the second symbol.

Reviewers: hokein

Subscribers: ilya-biryukov, ioeric, jkorous, arphaman, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp

Modified: clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp?rev=349152&r1=349151&r2=349152&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp Fri Dec 14 06:17:18 
2018
@@ -149,7 +149,8 @@ class FuzzyFind : public Command {
 outs() << formatv(OutputFormat, "Rank", "Symbol ID", "Symbol Name");
 size_t Rank = 0;
 Index->fuzzyFind(Request, [&](const Symbol &Sym) {
-  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name);
+  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(),
+Sym.Scope + Sym.Name);
 });
   }
 };


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

[PATCH] D55705: [dexp] Change FuzzyFind to also print scope of symbols

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349152: [dexp] Change FuzzyFind to also print scope of 
symbols (authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55705

Files:
  clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp


Index: clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
@@ -149,7 +149,8 @@
 outs() << formatv(OutputFormat, "Rank", "Symbol ID", "Symbol Name");
 size_t Rank = 0;
 Index->fuzzyFind(Request, [&](const Symbol &Sym) {
-  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name);
+  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(),
+Sym.Scope + Sym.Name);
 });
   }
 };


Index: clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
@@ -149,7 +149,8 @@
 outs() << formatv(OutputFormat, "Rank", "Symbol ID", "Symbol Name");
 size_t Rank = 0;
 Index->fuzzyFind(Request, [&](const Symbol &Sym) {
-  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(), Sym.Name);
+  outs() << formatv(OutputFormat, Rank++, Sym.ID.str(),
+Sym.Scope + Sym.Name);
 });
   }
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

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

dexonsmith added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' 
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang 
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push 
NOT_MY_LABEL'}}

aaron.ballman wrote:
> Should we really treat this as an error? It seems to me that this should be a 
> warning because pop without a label could be viewed as "I don't care what I'm 
> popping, just pop it". Still worth warning about, but maybe not worth 
> stopping a build over.
IMO this is most likely to be an implementation error on the part of a macro 
author, where the END macro is missing the label used in BEGIN.  This makes the 
macro pair unsafe to mix with other macros.  If the macro author doesn’t want 
safety, why use a label in the BEGIN macro at all?

I see you’re envisioning this being used directly by an end-user, which I 
suppose is plausible, but I think the same logic applies.  Why add a label to 
push if you don’t want to be precise about pop?



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

aaron.ballman wrote:
> I feel like this should be diagnosed, perhaps even as an error. The user 
> provided labels but then got the push and pop order wrong when explicitly 
> saying what to pop. That seems more likely to be a logic error on the user's 
> part.
On the contrary, the user is using two differently-named and independent macro 
pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are implemented 
with _Pragma(“clang attribute ...”) under the hood.  The point is to give the 
same result as two independent pragma pairs, whose regions do not need to be 
nested.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55628



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

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a subscriber: rsmith.
aaron.ballman added a comment.

I'd like @rsmith's opinion on whether this is a good churn or not. I think it's 
mostly reasonable, but it's also a lot of changes for identical behavior and in 
some cases the changes are a bit unfortunate.




Comment at: include/clang/ASTMatchers/ASTMatchers.h:4504
 FunctionDecl)) {
-  return Node.isThisDeclarationADefinition();
 }

I don't care for this refactoring -- the new code repeats the types from the 
function definition and is considerably harder to read.



Comment at: lib/AST/Decl.cpp:2378
+  assert(D);
+  if (auto *Def = D->getDefinition(Context))
+return Def;

Bad use of `auto`, though it is used above so maybe it should stay for local 
consistency.



Comment at: lib/Sema/SemaDecl.cpp:2849
+static std::pair
+getNoteDiagForInvalidRedeclaration(const ASTContext &Context, const T *Old,
+   const T *New) {

I'm not keen on the code duplication. :-(


Repository:
  rC Clang

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

https://reviews.llvm.org/D55658



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

[PATCH] D55382: Make -Wstring-plus-int warns even if when the result is not out of bounds

[Arnaud Bienner via Phabricator via cfe-commits]

ArnaudBienner added a comment.

Really sorry about breaking the build :( Thanks for reverting it.
Actually, I think we could fix that test by removing the " +1": AFAICT this 
test is checking that warnings are correctly emitted from clang, but not 
testing all the warnings. So the conversion from const char* to int is enough: 
no need to have an extra  +1 at the end.
I will update my patch to update the test accordingly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55382



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

r349155 - Implement -frecord-command-line (-frecord-gcc-switches)

[Scott Linder via cfe-commits]

Author: scott.linder
Date: Fri Dec 14 07:38:15 2018
New Revision: 349155

URL: http://llvm.org/viewvc/llvm-project?rev=349155&view=rev
Log:
Implement -frecord-command-line (-frecord-gcc-switches)

Implement options in clang to enable recording the driver command-line
in an ELF section.

Implement a new special named metadata, llvm.commandline, to support
frontends embedding their command-line options in IR/ASM/ELF.

This differs from the GCC implementation in some key ways:

* In GCC there is only one command-line possible per compilation-unit,
  in LLVM it mirrors llvm.ident and multiple are allowed.
* In GCC individual options are separated by NULL bytes, in LLVM entire
  command-lines are separated by NULL bytes. The advantage of the GCC
  approach is to clearly delineate options in the face of embedded
  spaces. The advantage of the LLVM approach is to support merging
  multiple command-lines unambiguously, while handling embedded spaces
  with escaping.

Differential Revision: https://reviews.llvm.org/D54487
Clang Differential Revision: https://reviews.llvm.org/D54489

Modified:
cfe/trunk/docs/ClangCommandLineReference.rst
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/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/Driver/clang_f_opts.c
cfe/trunk/test/Driver/debug-options.c

Modified: cfe/trunk/docs/ClangCommandLineReference.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangCommandLineReference.rst?rev=349155&r1=349154&r2=349155&view=diff
==
--- cfe/trunk/docs/ClangCommandLineReference.rst (original)
+++ cfe/trunk/docs/ClangCommandLineReference.rst Fri Dec 14 07:38:15 2018
@@ -792,6 +792,16 @@ Don't use blacklist file for sanitizers
 
 .. option:: -fparse-all-comments
 
+.. option:: -frecord-command-line, -frecord-gcc-switches, 
-fno-record-command-line, -fno-record-gcc-switches
+
+Generate a section named ".GCC.command.line" containing the clang driver
+command-line. After linking, the section may contain multiple command lines,
+which will be individually terminated by null bytes. Separate arguments within
+a command line are combined with spaces; spaces and backslashes within an
+argument are escaped with backslashes. This format differs from the format of
+the equivalent section produced by GCC with the -frecord-gcc-switches flag.
+This option is currently only supported on ELF targets.
+
 .. option:: -fsanitize-address-field-padding=
 
 Level of field padding for AddressSanitizer
@@ -2831,7 +2841,7 @@ Embed source text in DWARF debug section
 
 .. option:: -gpubnames, -gno-pubnames
 
-.. option:: -grecord-gcc-switches, -gno-record-gcc-switches
+.. option:: -grecord-command-line, -grecord-gcc-switches, 
-gno-record-command-line, -gno-record-gcc-switches
 
 .. option:: -gsplit-dwarf
 

Modified: cfe/trunk/include/clang/Basic/CodeGenOptions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/CodeGenOptions.h?rev=349155&r1=349154&r2=349155&view=diff
==
--- cfe/trunk/include/clang/Basic/CodeGenOptions.h (original)
+++ cfe/trunk/include/clang/Basic/CodeGenOptions.h Fri Dec 14 07:38:15 2018
@@ -148,6 +148,10 @@ public:
   /// non-empty.
   std::string DwarfDebugFlags;
 
+  /// The string containing the commandline for the llvm.commandline metadata,
+  /// if non-empty.
+  std::string RecordCommandLine;
+
   std::map DebugPrefixMap;
 
   /// The ABI to use for passing floating point arguments.

Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=349155&r1=349154&r2=349155&view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Fri Dec 14 07:38:15 2018
@@ -167,6 +167,8 @@ def fdebug_compilation_dir : Separate<["
   HelpText<"The compilation directory to embed in the debug info.">;
 def dwarf_debug_flags : Separate<["-"], "dwarf-debug-flags">,
   HelpText<"The string to embed in the Dwarf debug flags record.">;
+def record_command_line : Separate<["-"], "record-command-line">,
+  HelpText<"The string to embed in the .LLVM.command.line section.">;
 def compress_debug_sections : Flag<["-", "--"], "compress-debug-sections">,
 HelpText<"DWARF debug sections compression">;
 def compress_debug_sections_EQ : Joined<["-"], "compress-debug-sections=">,

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=349155&r1=349154&r2=349155&view=diff
==

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

[Scott Linder via Phabricator via cfe-commits]

scott.linder closed this revision.
scott.linder added a comment.

rL349155 


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

https://reviews.llvm.org/D54489



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Rafael,

This is a great patch and good improvement, thank you! I had a few minor 
comments.
(Also, I was thinking about what else could we import in the future, maybe 
definitions of C++11 enum classes would be also worth to be handled by CTU.)




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+  llvm::Expected
+  getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+   StringRef IndexName);

r.stahl wrote:
> xazax.hun wrote:
> > I wonder if we need these overloads. Maybe having only the templated 
> > version and having a static assert for the supported types is better? 
> > Asking the other reviewers as well.
> They are not required, but I thought they make the interface more clear and 
> help prevent implementation in headers.
I consider the new overload just great. Later, if we want we still can extend 
the interface with a template which would call the overloaded functions.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:117
+}
+static bool hasDefinition(const VarDecl *D, const VarDecl *&DefD) {
+  return D->getAnyInitializer(DefD) != nullptr;

The naming here is confusing for me, would be better to be `hasInit`, because 
there are cases when a VarDecl has an initializer but it is not a definition:
```
  struct A {
static const int a = 1 + 2; // VarDecl: has init, but not a definition
  };
  const int A::a; // VarDecl: definition
```
In the above case we probably want to import the initializer and we don't care 
about the definition.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:120
+}
+template  static bool hasDefinition(const T *D) {
+  const T *Unused;

`hasDefinitionOrInit` ?



Comment at: test/Analysis/Inputs/ctu-other.cpp:79
+};
+extern const S extS = {.a = 4};

Could we have another test case when `S::a` is static?



Comment at: test/Analysis/func-mapping-test.cpp:18
+struct S {
+  int a;
+};

Could we have one more test when we have a `static` member variable?


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

https://reviews.llvm.org/D46421



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

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' 
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang 
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push 
NOT_MY_LABEL'}}

dexonsmith wrote:
> aaron.ballman wrote:
> > Should we really treat this as an error? It seems to me that this should be 
> > a warning because pop without a label could be viewed as "I don't care what 
> > I'm popping, just pop it". Still worth warning about, but maybe not worth 
> > stopping a build over.
> IMO this is most likely to be an implementation error on the part of a macro 
> author, where the END macro is missing the label used in BEGIN.  This makes 
> the macro pair unsafe to mix with other macros.  If the macro author doesn’t 
> want safety, why use a label in the BEGIN macro at all?
> 
> I see you’re envisioning this being used directly by an end-user, which I 
> suppose is plausible, but I think the same logic applies.  Why add a label to 
> push if you don’t want to be precise about pop?
> Why add a label to push if you don’t want to be precise about pop?

Why is this important enough to fail everyone's build over it as opposed to 
warning users that they've done something that could be a bad code smell and 
let -Werror usage decide whether to fail the build or not? It seems like an 
extreme measure for something that has explicable "fallback" behavior.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

dexonsmith wrote:
> aaron.ballman wrote:
> > I feel like this should be diagnosed, perhaps even as an error. The user 
> > provided labels but then got the push and pop order wrong when explicitly 
> > saying what to pop. That seems more likely to be a logic error on the 
> > user's part.
> On the contrary, the user is using two differently-named and independent 
> macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are 
> implemented with _Pragma(“clang attribute ...”) under the hood.  The point is 
> to give the same result as two independent pragma pairs, whose regions do not 
> need to be nested.
> On the contrary, the user is using two differently-named and independent 
> macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) 

I don't think this is a safe assumption to make, and in this case, is false. 
There are no macros anywhere in this test case.

> The point is to give the same result as two independent pragma pairs, whose 
> regions do not need to be nested.

I don't find this to be intuitive behavior. These are stack manipulations with 
the names push and pop -- pretending that they're overlapping rather than a 
stack in the presence of labels is confusing. If I saw the code from this test 
case during a code review, I would flag it as being incorrect because the 
labels do not match -- I don't think I'd be the only one.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55628



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

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Just to expend on the instrumentation results: The measurement was done with 
all of Boost. I would take the estimation of the time wasted with
a grain of salt since this is just `num_iterations * 10ns` which is obviously a 
very rough estimation.
However on my machine I get that removing half of the iterations in 
`getTranslationUnitDecl` reduce the run-time of an fsyntax-only by a little 
more than 1%.

Extrapolating from this, this could be worth about 2% if we remove, say 80%, of 
the iterations in `getTranslationUnitDecl`.

I guess that the question is: Is this churn worth about 2% ? I would like to 
argue that yes since:

1. 2% is a lot for something that is morally a simple getter.
2. The methods of the expression classes must already take a ref to the 
`ASTContext` if they need it.
3. In the vast majority of cases the context is already easily available. It is 
true though that this requires propagating it in some functions.
4. This figure do not include some potential gains in the time needed to do the 
AST -> LLVM IR code generation.
5. I suspect (but have not done the measurement) that the gain is even greater 
during a typical compilation where more than one thread is used since possibly 
more iterations are going to cause an expensive LLC miss.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55658



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

[PATCH] D55707: Update to SARIF 11-28

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: NoQ, george.karpenkov.

This updates our SARIF support from 10-10 to 11-28. Functional changes include:

- The run.files property is now an array instead of a mapping.
- fileLocation objects now have a fileIndex property specifying the array index 
into run.files.
- The resource.rules property is now an array instead of a mapping.
- The result object was given a ruleIndex property that is an index into the 
resource.rules array.
- rule objects now have their "id" field filled out in addition to the name 
field.
- Updated the schema and spec version numbers to 11-28.

(Note, the SARIF viewer plugins for Visual Studio and VSCode have not caught up 
to 11-28 yet, but are expected to be updated.)


https://reviews.llvm.org/D55707

Files:
  lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  
test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
  
test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif

Index: test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
===
--- test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
+++ test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
@@ -1,9 +1,9 @@
 {
-  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-10-10";,
+  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28";,
   "runs": [
 {
-  "files": {
-"file:sarif-multi-diagnostic-test.c": {
+  "files": [  
+{
   "fileLocation": {
 "uri": "file:sarif-multi-diagnostic-test.c"
   },
@@ -13,34 +13,37 @@
 "resultFile"
   ]
 }
-  },
+  ],
   "resources": {
-"rules": {
-  "core.CallAndMessage": {
+"rules": [
+  {
 "fullDescription": {
-  "text": "Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)"
+  "text": "Mark tainted symbols as such."
 },
+"id": "debug.TaintTest",
 "name": {
-  "text": "core.CallAndMessage"
+  "text": "debug.TaintTest"
 }
-  },
-  "core.DivideZero": {
+  },
+  {
 "fullDescription": {
-  "text": "Check for division by zero"
+  "text": "Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)"
 },
+"id": "core.CallAndMessage",
 "name": {
-  "text": "core.DivideZero"
+  "text": "core.CallAndMessage"
 }
   },
-  "debug.TaintTest": {
+  {
 "fullDescription": {
-  "text": "Mark tainted symbols as such."
+  "text": "Check for division by zero"
 },
+"id": "core.DivideZero",
 "name": {
-  "text": "debug.TaintTest"
+  "text": "core.DivideZero"
 }
   }
-}
+]
   },
   "results": [
 {
@@ -57,6 +60,7 @@
 },
 "physicalLocation": {
   "fileLocation": {
+"fileIndex": 0,
 "uri": "file:sarif-multi-diagnostic-test.c"
   },
   "region": {
@@ -76,6 +80,7 @@
 },
 "physicalLocation": {
   "fileLocation": {
+"fileIndex": 0,
 "uri": "file:sarif-multi-diagnostic-test.c"
   },
   "region": {
@@ -96,6 +101,7 @@
 {
   "physicalLocation": {
 "fileLocation": {
+  "fileIndex": 0,
   "uri": "file:sarif-multi-diagnostic-test.c"
 },
 "region": {
@@ -110,7 +116,8 @@
   "message": {
 "text": "tainted"
   },
-  "ruleId": "debug.TaintTest"
+  "ruleId": "debug.TaintTest",
+  "ruleIndex": 0
 },
 {
   "codeFlows": [
@@ -126,6 +133,7 @@
 },
 "physicalLocation": {
   "fileLocation": {
+"fileIndex": 0,
 "uri": "file:sarif-multi-diagnostic-test.c"
   },
   "region": {
@@ -145,6 +153,7 @@
 },
 "physicalLocation": {
   "fileLocation": {
+ 

[PATCH] D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

@rsmith ping!

@thakis: You said, //"From a user point of view, I have no idea what "CTAD" 
means, and I sometimes work on a C++ compiler."// Did you mean "I think the 
command-line option should be named something more verbose than 
`-W[c++14-compat-]ctad`"? Or "I don't know what CTAD is and I would indeed like 
a diagnostic if I use it accidentally"? Or something else?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54565



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

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

@EricWF ping!


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47344



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

[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:47-49
+  "consider to cast the return value of %0 from type integer to type char, 
"
+  "possible loss of precision if an error has occurred or the end "
+  "of file has been reached");

hgabii wrote:
> aaron.ballman wrote:
> > This diagnostic confuses me, so perhaps you can explain the situation you 
> > want to diagnose a bit further.
> > 
> > FIO34-C is about situations where `sizeof(char) == sizeof(int)` and the 
> > call returns EOF/WEOF. In that case, there's no way to distinguish between 
> > an EOF/error and a valid character. Suggesting to explicitly cast to a 
> > character type doesn't add any safety to the code -- the user needs to 
> > insert calls to `feof()` or `ferror()` instead (to make it portable, 
> > anyway) and should store the character value in a sufficiently large 
> > integer type before doing the comparison against EOF.
> > 
> > Are you trying to catch a different kind of bug?
> Yes, I want to diagnose this problem. Do you think I need to add suggestions 
> into the warning message to use `feof()` or `ferror()`?
I think there are two different situations to think about:

1) If the target architecture has `sizeof(char) == sizeof(int)`, no amount of 
casting will help the user because the bit pattern for `EOF` is the same 
whether stored as an `int` or a `char` on that platform. There, the user really 
must use `feof()` and `ferror()`. This situation is theoretically even worse 
for the wide versions of the APIs because `sizeof(wint_t) == sizeof(wchar_t)` 
quite commonly, except that it would require a terrible choice in character 
sets for `wchar_t` by the implementation because Unicode makes guarantees about 
certain bit patterns not being a valid character (like 0x in UTF-16). I 
don't know that we have to worry about the wide-char versions at all in 
practice, but if you know of a platform where this is a legitimate concern, I'd 
love to hear about it.

2) If the target architecture does not have this 
identical-size-in-band-error-indicator problem, then it might make sense to 
diagnose the code for portability reasons or it might make sense to not 
diagnose at all because the user won't run into it. Might be worth making this 
an optional feature of the check.

Either way, I don't know that we'll be able to suggest fixes for the user in a 
diagnostic message of reasonable length. This might be a case where we want to 
tell the user about the danger and let the documentation tell them how to fix 
it. Maybe a diagnostic like: `"the value for %select{EOF|WEOF}0 is 
indistinguishable from a valid character with the same bit pattern; consider 
explicit checks for error or end-of-file indicators"`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D42682



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

[PATCH] D55363: [clangd] Expose FileStatus in LSP.

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In D55363#1329652 , @ilya-biryukov 
wrote:

> Sorry if I missed any important design discussions here, but wanted to clear 
> up what information we are trying to convey to the user with the status 
> messages?
>  E.g. why seeing "building preamble", "building file" or "queued" in the 
> status bar can be useful to the user? Those messages mention internals of 
> clangd, I'm not sure how someone unfamiliar with internals of clangd should 
> interpret this information.


I agree about not necessarily needing to expose clang internals, but I think 
the end goal of providing some feedback to the user (still thinking/done) is 
worthy.

If this is a clangd-specific event, shouldn't the name of the event reflect it? 
 As-is, it looks like an official LSP feature and can be confusing.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55363



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

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

[S. B. Tam via Phabricator via cfe-commits]

cpplearner updated this revision to Diff 178242.
cpplearner marked an inline comment as done.

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

https://reviews.llvm.org/D55413

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp


Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -356,6 +356,14 @@
 if (a != 13) return false;
 a &= 14;
 if (a != 12) return false;
+a += -1.2;
+if (a != 10) return false;
+a -= 3.1;
+if (a != 6) return false;
+a *= 2.2;
+if (a != 13) return false;
+if (&(a /= 1.5) != &a) return false;
+if (a != 8) return false;
 return true;
   }
   static_assert(test_int(), "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3424,19 +3424,31 @@
 if (!checkConst(SubobjType))
   return false;
 
-if (!SubobjType->isIntegerType() || !RHS.isInt()) {
+if (!SubobjType->isIntegerType()) {
   // We don't support compound assignment on integer-cast-to-pointer
   // values.
   Info.FFDiag(E);
   return false;
 }
 
-APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
-SubobjType, Value);
-if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
-  return false;
-Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
-return true;
+if (RHS.isInt()) {
+  APSInt LHS =
+  HandleIntToIntCast(Info, E, PromotedLHSType, SubobjType, Value);
+  if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
+return false;
+  Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
+  return true;
+} else if (RHS.isFloat()) {
+  APFloat FValue(0.0);
+  return HandleIntToFloatCast(Info, E, SubobjType, Value, PromotedLHSType,
+  FValue) &&
+ handleFloatFloatBinOp(Info, E, FValue, Opcode, RHS.getFloat()) &&
+ HandleFloatToIntCast(Info, E, PromotedLHSType, FValue, SubobjType,
+  Value);
+}
+
+Info.FFDiag(E);
+return false;
   }
   bool found(APFloat &Value, QualType SubobjType) {
 return checkConst(SubobjType) &&


Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -356,6 +356,14 @@
 if (a != 13) return false;
 a &= 14;
 if (a != 12) return false;
+a += -1.2;
+if (a != 10) return false;
+a -= 3.1;
+if (a != 6) return false;
+a *= 2.2;
+if (a != 13) return false;
+if (&(a /= 1.5) != &a) return false;
+if (a != 8) return false;
 return true;
   }
   static_assert(test_int(), "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3424,19 +3424,31 @@
 if (!checkConst(SubobjType))
   return false;
 
-if (!SubobjType->isIntegerType() || !RHS.isInt()) {
+if (!SubobjType->isIntegerType()) {
   // We don't support compound assignment on integer-cast-to-pointer
   // values.
   Info.FFDiag(E);
   return false;
 }
 
-APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
-SubobjType, Value);
-if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
-  return false;
-Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
-return true;
+if (RHS.isInt()) {
+  APSInt LHS =
+  HandleIntToIntCast(Info, E, PromotedLHSType, SubobjType, Value);
+  if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
+return false;
+  Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
+  return true;
+} else if (RHS.isFloat()) {
+  APFloat FValue(0.0);
+  return HandleIntToFloatCast(Info, E, SubobjType, Value, PromotedLHSType,
+  FValue) &&
+ handleFloatFloatBinOp(Info, E, FValue, Opcode, RHS.getFloat()) &&
+ HandleFloatToIntCast(Info, E, PromotedLHSType, FValue, SubobjType,
+  Value);
+}
+
+Info.FFDiag(E);
+return false;
   }
   bool found(APFloat &Value, QualType SubobjType) {
 return checkConst(SubobjType) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55710: add pragmas to control Software Pipelining optimisation

[Alexey Lapshin via Phabricator via cfe-commits]

alexey.lapshin created this revision.
alexey.lapshin added reviewers: bcahoon, kparzysz, aaron.ballman, arphaman, 
rsmith, marksl, yakush.
Herald added a subscriber: zzheng.

  [PIPELINER] add two pragmas to control Software Pipelining optimisation.
  
#pragma clang loop pipeline(disable)
  
Disable SWP optimization for the next loop.
“disable” is the only possible value.
  
#pragma clang loop pipeline_ii_count(number)
  
Set value of initiation interval for SWP
optimization to specified number value for
the next loop. Number is the positive value
greater than 0.
  
These pragmas could be used for debugging or reducing
compile time purposes. It is possible to disable SWP for
concrete loops to save compilation time or to find bugs
by not doing SWP to certain loops. It is possible to set
value of initiation interval to concrete number to save
compilation time by not doing extra pipeliner passes or
to check created schedule for specific initiation interval.
  
That is clang part of the fix


https://reviews.llvm.org/D55710

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticParseKinds.td
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  lib/Parse/ParsePragma.cpp
  lib/Sema/SemaStmtAttr.cpp
  test/CodeGenCXX/pragma-pipeline.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-pipeline.cpp
  test/Parser/pragma-unroll-and-jam.cpp

Index: test/Parser/pragma-unroll-and-jam.cpp
===
--- test/Parser/pragma-unroll-and-jam.cpp
+++ test/Parser/pragma-unroll-and-jam.cpp
@@ -67,7 +67,7 @@
   }
 
 // pragma clang unroll_and_jam is disabled for the moment
-/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop unroll_and_jam(4)
+/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_ii_count or distribute}} */ #pragma clang loop unroll_and_jam(4)
   for (int i = 0; i < Length; i++) {
 for (int j = 0; j < Length; j++) {
   List[i * Length + j] = Value;
Index: test/Parser/pragma-pipeline.cpp
===
--- /dev/null
+++ test/Parser/pragma-pipeline.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++11 -verify %s
+
+// Note that this puts the expected lines before the directives to work around
+// limitations in the -verify mode.
+
+void test(int *List, int Length, int Value) {
+  int i = 0;
+
+#pragma clang loop pipeline(disable) 
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+#pragma clang loop pipeline_ii_count(10) 
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+/* expected-error {{expected ')'}} */ #pragma clang loop pipeline(disable
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(enable)
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(error)
+/* expected-error {{missing argument; expected an integer value}} */ #pragma clang loop pipeline_ii_count()
+/* expected-error {{use of undeclared identifier 'error'}} */ #pragma clang loop pipeline_ii_count(error)
+/* expected-error {{expected '('}} */ #pragma clang loop pipeline_ii_count 1 2
+  for (int i = 0; i < Length; i++) {
+for (int j = 0; j < Length; j++) {
+  List[i * Length + j] = Value;
+}
+  }
+
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int j = Length;
+#pragma clang loop pipeline_ii_count(4) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int k = Length;
+
+#pragma clang loop pipeline(disable)
+#pragma clang loop pipeline_ii_count(4) /* expected-error {{incompatible directives 'pipeline(disable)' and 'pipeline_ii_count(4)'}} */
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected statement}} */ }
Index: test/Parser/pragma-loop.cpp
===
--- test/Parser/pragma-loop.cpp
+++ test/Parser/pragma-loop.cpp
@@ -147,7 +147,7 @@
 /* expected-error {{missing argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll()
 /* expected-error {{missing argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute()
 
-/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop
+/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipelin

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington marked an inline comment as done.
erik.pilkington added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' 
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang 
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push 
NOT_MY_LABEL'}}

aaron.ballman wrote:
> dexonsmith wrote:
> > aaron.ballman wrote:
> > > Should we really treat this as an error? It seems to me that this should 
> > > be a warning because pop without a label could be viewed as "I don't care 
> > > what I'm popping, just pop it". Still worth warning about, but maybe not 
> > > worth stopping a build over.
> > IMO this is most likely to be an implementation error on the part of a 
> > macro author, where the END macro is missing the label used in BEGIN.  This 
> > makes the macro pair unsafe to mix with other macros.  If the macro author 
> > doesn’t want safety, why use a label in the BEGIN macro at all?
> > 
> > I see you’re envisioning this being used directly by an end-user, which I 
> > suppose is plausible, but I think the same logic applies.  Why add a label 
> > to push if you don’t want to be precise about pop?
> > Why add a label to push if you don’t want to be precise about pop?
> 
> Why is this important enough to fail everyone's build over it as opposed to 
> warning users that they've done something that could be a bad code smell and 
> let -Werror usage decide whether to fail the build or not? It seems like an 
> extreme measure for something that has explicable "fallback" behavior.
My implicit assumption (which I should have been more clear about!) was that 
you'd only really ever write a label on a `push` in a BEGIN/END macro. In that 
world, you'd only ever see this case if 1) you're interacting with another 
macro that doesn't use the label convention, or 2) if you're interacting with 
manual push/pop code. In both of those cases, you'd end up popping the wrong 
attribute group and start applying attributes onto declarations that the 
programmer didn't intend.

I'm fine with downgrading this to a warning, but IMO an error seems more 
appropriate. If we wanted to force 1) or 2) through the compiler then we'd also 
need to downgrade `pop UNPUSHED_LABEL` to a warning, which doesn't seem like 
the end of the world either.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

aaron.ballman wrote:
> dexonsmith wrote:
> > aaron.ballman wrote:
> > > I feel like this should be diagnosed, perhaps even as an error. The user 
> > > provided labels but then got the push and pop order wrong when explicitly 
> > > saying what to pop. That seems more likely to be a logic error on the 
> > > user's part.
> > On the contrary, the user is using two differently-named and independent 
> > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are 
> > implemented with _Pragma(“clang attribute ...”) under the hood.  The point 
> > is to give the same result as two independent pragma pairs, whose regions 
> > do not need to be nested.
> > On the contrary, the user is using two differently-named and independent 
> > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) 
> 
> I don't think this is a safe assumption to make, and in this case, is false. 
> There are no macros anywhere in this test case.
> 
> > The point is to give the same result as two independent pragma pairs, whose 
> > regions do not need to be nested.
> 
> I don't find this to be intuitive behavior. These are stack manipulations 
> with the names push and pop -- pretending that they're overlapping rather 
> than a stack in the presence of labels is confusing. If I saw the code from 
> this test case during a code review, I would flag it as being incorrect 
> because the labels do not match -- I don't think I'd be the only one.
I think the labels only really makes sense if you're writing macros that hide 
the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), which for 
better or for worse people do write, and in fact was the intended use case for 
#pragma clang attribute. I think if we were to write this feature again, we'd 
forgo the stack entirly and require every `push` to have a label and be in its 
own namespace. But this is the best we can do now.

I don't really think that anyone should write a push label outside of a macro 
definition, since I agree that the semantics are a bit surprising when you're 
writing the #pragmas yourself without macros. I'll update this test case and 
the documentation to stress this point more. If you think this is going to be a 
potential pain point, maybe we can even warn on using a label outside of a 
macro definition. 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.ll

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

[S. B. Tam via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/ExprConstant.cpp:3453
   Value) &&
handleFloatFloatBinOp(Info, E, Value, Opcode, RHS.getFloat()) &&
HandleFloatToFloatCast(Info, E, PromotedLHSType, SubobjType, Value);

rjmccall wrote:
> rsmith wrote:
> > Does this work for the float += int case?
> IIRC, the RHS gets promoted to the computation type in Sema.
Yes, in the float += int case, the RHS is the result of ImplicitCastExpr of 
kind IntegralToFloating. And `test_float()` contains test for that case.


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

https://reviews.llvm.org/D55413



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

[PATCH] D55707: Update to SARIF 11-28

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:218
 const PathDiagnosticLocation &P = Piece->getLocation();
+const auto *CP = dyn_cast(Piece.get());
 Locations.push_back(createThreadFlowLocation(

Ignore this line, it is debugging cruft and will be removed.


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

https://reviews.llvm.org/D55707



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

[PATCH] D55712: [Driver][PS4] Do not implicitly link against asan or ubsan if -nostdlib or -nodefaultlibs on PS4.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau created this revision.
pgousseau added reviewers: filcab, probinson, rsmith, gbedwell, dmikulin.
Herald added a subscriber: cfe-commits.

NFC for targets other than PS4.

Respect -nostdlib and -nodefaultlibs when enabling asan or ubsan.


Repository:
  rC Clang

https://reviews.llvm.org/D55712

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/PS4CPU.cpp
  test/Driver/fsanitize.c
  test/Driver/sanitizer-ld.c


Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -673,6 +673,13 @@
 // CHECK-AUBSAN-PS4: "{{.*}}ld{{(.gold)?(.exe)?}}"
 // CHECK-AUBSAN-PS4: -lSceDbgAddressSanitizer_stub_weak
 
+// RUN: %clang -fsanitize=address,undefined %s -### -o %t.o 2>&1 \
+// RUN: -target x86_64-scei-ps4 -fuse-ld=ld \
+// RUN: -shared \
+// RUN: -nostdlib \
+// RUN:   | FileCheck --check-prefix=CHECK-NOLIB-PS4 %s
+// CHECK-NOLIB-PS4-NOT: SceDbgAddressSanitizer_stub_weak
+
 // RUN: %clang -fsanitize=efficiency-cache-frag %s -### -o %t.o 2>&1 \
 // RUN: -target x86_64-unknown-linux -fuse-ld=ld \
 // RUN:   | FileCheck --check-prefix=CHECK-ESAN-LINUX %s
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -735,6 +735,10 @@
 // CHECK-ASAN-PS4: --dependent-lib=libSceDbgAddressSanitizer_stub_weak.a
 // CHECK-ASAN-PS4-NOT: {{(\.(o|bc)"? |-l).*-lSceDbgAddressSanitizer_stub_weak}}
 // CHECK-ASAN-PS4: -lSceDbgAddressSanitizer_stub_weak
+// RUN: %clang -target x86_64-scei-ps4 -fsanitize=address -nostdlib %s -### 
2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-NOLIB-PS4
+// RUN: %clang -target x86_64-scei-ps4 -fsanitize=address -nodefaultlibs %s 
-### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-NOLIB-PS4
+// RUN: %clang -target x86_64-scei-ps4 -fsanitize=address -nodefaultlibs 
-nostdlib  %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-NOLIB-PS4
+// CHECK-ASAN-NOLIB-PS4-NOT: SceDbgAddressSanitizer_stub_weak
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address 
-fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-ASAN-MINIMAL
 // CHECK-ASAN-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' 
not allowed with '-fsanitize=address'
Index: lib/Driver/ToolChains/PS4CPU.cpp
===
--- lib/Driver/ToolChains/PS4CPU.cpp
+++ lib/Driver/ToolChains/PS4CPU.cpp
@@ -121,7 +121,8 @@
 assert(Output.isNothing() && "Invalid output.");
   }
 
-  AddPS4SanitizerArgs(ToolChain, CmdArgs);
+  if(!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
+AddPS4SanitizerArgs(ToolChain, CmdArgs);
 
   Args.AddAllArgs(CmdArgs, options::OPT_L);
   Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
@@ -190,7 +191,8 @@
 assert(Output.isNothing() && "Invalid output.");
   }
 
-  AddPS4SanitizerArgs(ToolChain, CmdArgs);
+  if(!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
+AddPS4SanitizerArgs(ToolChain, CmdArgs);
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
 const char *crt1 = nullptr;
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4049,7 +4049,8 @@
 ABICompatArg->render(Args, CmdArgs);
 
   // Add runtime flag for PS4 when PGO, coverage, or sanitizers are enabled.
-  if (RawTriple.isPS4CPU()) {
+  if (RawTriple.isPS4CPU() &&
+  !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
 PS4cpu::addProfileRTArgs(TC, Args, CmdArgs);
 PS4cpu::addSanitizerArgs(TC, CmdArgs);
   }


Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -673,6 +673,13 @@
 // CHECK-AUBSAN-PS4: "{{.*}}ld{{(.gold)?(.exe)?}}"
 // CHECK-AUBSAN-PS4: -lSceDbgAddressSanitizer_stub_weak
 
+// RUN: %clang -fsanitize=address,undefined %s -### -o %t.o 2>&1 \
+// RUN: -target x86_64-scei-ps4 -fuse-ld=ld \
+// RUN: -shared \
+// RUN: -nostdlib \
+// RUN:   | FileCheck --check-prefix=CHECK-NOLIB-PS4 %s
+// CHECK-NOLIB-PS4-NOT: SceDbgAddressSanitizer_stub_weak
+
 // RUN: %clang -fsanitize=efficiency-cache-frag %s -### -o %t.o 2>&1 \
 // RUN: -target x86_64-unknown-linux -fuse-ld=ld \
 // RUN:   | FileCheck --check-prefix=CHECK-ESAN-LINUX %s
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -735,6 +735,10 @@
 // CHECK-ASAN-PS4: --dependent-lib=libSceDbgAddressSanitizer_stub_weak.a
 // CHECK-ASAN-PS4-NOT: {{(\.(o|bc)"? |-l).*-lSceDbgAddressSanitizer_stub_weak}}
 // CHECK-ASAN-PS4: -lSceDbgAddressSanitizer_stub_weak
+// RUN: %clang -target x86_64-scei-ps4 -fsanitize=address -nostdlib 

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 178254.
erik.pilkington marked 7 inline comments as done.
erik.pilkington added a comment.

Address @aaron.ballman comments. Thanks!


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

https://reviews.llvm.org/D55628

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/test/Sema/pragma-attribute-label.c

Index: clang/test/Sema/pragma-attribute-label.c
===
--- /dev/null
+++ clang/test/Sema/pragma-attribute-label.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define ASSUME_MY_ANNOTATE_BEGIN _Pragma("clang attribute push MY_LABEL (__attribute__((annotate)), apply_to=function)")
+#define ASSUME_MY_ANNOTATE_END _Pragma("clang attribute pop MY_LABEL")
+
+#define ASSUME_MY_OTHER_ANNOTATE_BEGIN _Pragma("clang attribute push MY_OTHER_LABEL (__attribute__((annotate)), apply_to=function)")
+#define ASSUME_MY_OTHER_ANNOTATE_END _Pragma("clang attribute pop MY_OTHER_LABEL")
+
+
+ASSUME_MY_ANNOTATE_BEGIN // expected-error 2 {{'annotate' attribute}}
+
+int some_func(); // expected-note{{when applied to this declaration}}
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push NOT_MY_LABEL'}}
+
+ASSUME_MY_OTHER_ANNOTATE_BEGIN // expected-error 2 {{'annotate' attribute}}
+
+int some_other_func(); // expected-note 2 {{when applied to this declaration}}
+
+// Out of order!
+ASSUME_MY_ANNOTATE_END
+
+int some_other_other_func(); // expected-note 1 {{when applied to this declaration}}
+
+ASSUME_MY_OTHER_ANNOTATE_END
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -631,28 +631,45 @@
   {PragmaLoc, &Attribute, std::move(SubjectMatchRules), /*IsUsed=*/false});
 }
 
-void Sema::ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc) {
+void Sema::ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
+ const IdentifierInfo *PushLabel) {
   PragmaAttributeStack.emplace_back();
   PragmaAttributeStack.back().Loc = PragmaLoc;
+  PragmaAttributeStack.back().Label = PushLabel;
 }
 
-void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc) {
+void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc,
+   const IdentifierInfo *PushLabel) {
   if (PragmaAttributeStack.empty()) {
-Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch);
+Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch) << 1;
 return;
   }
 
-  for (const PragmaAttributeEntry &Entry :
-   PragmaAttributeStack.back().Entries) {
-if (!Entry.IsUsed) {
-  assert(Entry.Attribute && "Expected an attribute");
-  Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
-  << Entry.Attribute->getName();
-  Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+  // Dig back through the stack trying to find the group that corresponds to
+  // PushLabel. Note that this works fine if no label is present, think of
+  // unlabeled push/pops as having an implicit "nullptr" label.
+  for (size_t Index = PragmaAttributeStack.size(); Index;) {
+--Index;
+if (PragmaAttributeStack[Index].Label == PushLabel) {
+  for (const PragmaAttributeEntry &Entry :
+   PragmaAttributeStack[Index].Entries) {
+if (!Entry.IsUsed) {
+  assert(Entry.Attribute && "Expected an attribute");
+  Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
+  << *Entry.Attribute;
+  Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+}
+  }
+  PragmaAttributeStack.erase(PragmaAttributeStack.begin() + Index);
+  return;
 }
   }
 
-  PragmaAttributeStack.pop_back();
+  if (PushLabel)
+Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch)
+<< 0 << PushLabel->getName();
+  else
+Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch) << 1;
 }
 
 void Sema::AddPragmaAttributes(Scope *S, Decl *D) {
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -1139,6 +1139,7 @@
   enum ActionType { Push, Pop, Attribute };
   ParsedAttributes &Attributes;
   ActionType Action;
+  const IdentifierInfo *PushLabel = nullptr;
   ArrayRef Tokens;
 
   PragmaAttributeInfo(ParsedAttributes &Attributes) : Attributes(Attributes) {}
@@ -1393,7 +1394,7 @@
   auto *Info = static_cast(Tok.getAnnotationValue(

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2667
+Because multiple push directives can be nested, if you're writing a macro that
+expands to ``_Pragma("clang attribute")`` it's good hygiene to add a label to
+your push/pop directives. A pop directive with a label will pop the innermost

aaron.ballman wrote:
> The documentation reads like the label is optional, but the examples make it 
> look required (because all examples have the label present). Can you clarify 
> what happens when the label is omitted and keep an example that shows it?
Sure, I moved this to the end of the section, added an example, and left the 
manual `#pragma clang attribute` below intact to make it more clear.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:840-842
+def err_pragma_attribute_stack_mismatch_label : Error<
+  "'#pragma clang attribute pop %0' with no matching "
+  "'#pragma clang attribute push %0'">;

aaron.ballman wrote:
> Can this be combined with the diagnostic above using `%select` (given that 
> the wording is basically identical)?
Sure, good point.


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

https://reviews.llvm.org/D55628



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

[PATCH] D50563: Fixed frontend clang tests in windows read-only container

[Stella Stamenova via Phabricator via cfe-commits]

stella.stamenova requested changes to this revision.
stella.stamenova added inline comments.
This revision now requires changes to proceed.



Comment at: test/Frontend/output-failures.c:1
-// RUN: not %clang_cc1 -emit-llvm -o %S/doesnotexist/somename %s 2> %t
-// RUN: FileCheck -check-prefix=OUTPUTFAIL -input-file=%t %s
+// RUN: not %clang_cc1 -emit-llvm -o %ROOT%/doesnotexist/somename %s 2>&1 | 
FileCheck -check-prefix=OUTPUTFAIL %s
 

I don't think this is the right solution - more specifically, I don't think we 
should be blindly looking for a file in the root directory that we do not 
control and adding a new property in the configuration for it.

A better solution would be to use a %t file here (for example, %t.doesnotexist) 
and run your tests out of tree (see test_exec_root).


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

https://reviews.llvm.org/D50563



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: zturner, majnemer, compnerd, rnk.
Herald added subscribers: erik.pilkington, Anastasia.

All of the symbols demangle on llvm-undname and demangler.com.  This
address space qualifier is useful for when we want to use opencl C++ in
Windows mode.  Additionally, C++ address-space using functions will now
be usable on windows.


https://reviews.llvm.org/D55715

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenCXX/mangle-address-space.cpp
  test/CodeGenOpenCL/address-spaces-mangling.cl

Index: test/CodeGenOpenCL/address-spaces-mangling.cl
===
--- test/CodeGenOpenCL/address-spaces-mangling.cl
+++ test/CodeGenOpenCL/address-spaces-mangling.cl
@@ -3,6 +3,11 @@
 // RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no -triple %itanium_abi_triple -emit-llvm -o - | FileCheck -check-prefixes=NOASMANG,NOASMAN10 %s
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map -faddress-space-map-mangling=no -triple %itanium_abi_triple -emit-llvm -o - | FileCheck -check-prefixes=NOASMANG,NOASMAN20 %s
 
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map -faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no -triple x86_64-windows-pc -emit-llvm -o - | FileCheck -check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map -faddress-space-map-mangling=no -triple x86_64-windows-pc -emit-llvm -o - | FileCheck -check-prefixes=MS_NOASMANG,MS_NOASMAN20 %s
+
 // We check that the address spaces are mangled the same in both version of OpenCL
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL2.0 -emit-llvm -o - | FileCheck -check-prefix=OCL-20 %s
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=CL1.2 -emit-llvm -o - | FileCheck -check-prefix=OCL-12 %s
@@ -16,6 +21,10 @@
 // ASMANG20: @_Z2ffPU3AS4i
 // NOASMANG10: @_Z2ffPi
 // NOASMANG20: @_Z2ffPU9CLgenerici
+// MS_ASMANG10: @_Z2ffPi
+// MS_ASMANG20: @_Z2ffPU3AS4i
+// MS_NOASMANG10: @_Z2ffPi
+// MS_NOASMANG20: @_Z2ffPU9CLgenerici
 // OCL-20-DAG: @_Z2ffPU3AS4i
 // OCL-12-DAG: @_Z2ffPi
 
@@ -23,6 +32,8 @@
 void f(private int *arg) { }
 // ASMANG: @_Z1fPi
 // NOASMANG: @_Z1fPU9CLprivatei
+// MS_ASMANG: @_Z1fPi
+// MS_NOASMANG: @_Z1fPU9CLprivatei
 // OCL-20-DAG: @_Z1fPi
 // OCL-12-DAG: @_Z1fPi
 
@@ -30,6 +41,8 @@
 void f(global int *arg) { }
 // ASMANG: @_Z1fPU3AS1i
 // NOASMANG: @_Z1fPU8CLglobali
+// MS_ASMANG: @_Z1fPU3AS1i
+// MS_NOASMANG: @_Z1fPU8CLglobali
 // OCL-20-DAG: @_Z1fPU3AS1i
 // OCL-12-DAG: @_Z1fPU3AS1i
 
@@ -37,6 +50,8 @@
 void f(local int *arg) { }
 // ASMANG: @_Z1fPU3AS3i
 // NOASMANG: @_Z1fPU7CLlocali
+// MS_ASMANG: @_Z1fPU3AS3i
+// MS_NOASMANG: @_Z1fPU7CLlocali
 // OCL-20-DAG: @_Z1fPU3AS3i
 // OCL-12-DAG: @_Z1fPU3AS3i
 
@@ -44,6 +59,8 @@
 void f(constant int *arg) { }
 // ASMANG: @_Z1fPU3AS2i
 // NOASMANG: @_Z1fPU10CLconstanti
+// MS_ASMANG: @_Z1fPU3AS2i
+// MS_NOASMANG: @_Z1fPU10CLconstanti
 // OCL-20-DAG: @_Z1fPU3AS2i
 // OCL-12-DAG: @_Z1fPU3AS2i
 
@@ -52,5 +69,7 @@
 void f(generic int *arg) { }
 // ASMANG20: @_Z1fPU3AS4i
 // NOASMANG20: @_Z1fPU9CLgenerici
+// MS_ASMANG20: @_Z1fPU3AS4i
+// MS_NOASMANG20: @_Z1fPU9CLgenerici
 // OCL-20-DAG: @_Z1fPU3AS4i
 #endif
Index: test/CodeGenCXX/mangle-address-space.cpp
===
--- test/CodeGenCXX/mangle-address-space.cpp
+++ test/CodeGenCXX/mangle-address-space.cpp
@@ -1,15 +1,64 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s --check-prefixes=CHECK,CHECKNOOCL
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-pc -o - %s | FileCheck %s --check-prefixes=WIN,WINNOOCL
+// RUN: %clang_cc1 -cl-std=c++ -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s --check-prefixes=CHECK,CHECKOCL
+// RUN: %clang_cc1 -cl-std=c++ -emit-llvm -triple x86_64-windows-pc -o - %s | FileCheck %s --check-prefixes=WIN,WINOCL
 
-// CHECK-LABEL: define {{.*}}void @_Z2f0Pc
+// CHECKNOOCL-LABEL: define {{.*}}void @_Z2f0Pc
+// WINNOOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAD@Z"
+// CHECKOCL-LABEL: define {{.*}}void @_Z2f0PU9CLgenericc
+// WINOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAU?$AS_CLgeneric@$$CAD@__clang@@@Z" 
 void f0(char *p) { }
 // CHECK-LABEL: define {{.*}}void @_Z2f0PU3AS1c
+// WIN-LABEL: define {{.*}}void @"?f0@@YAXPEAU?$AS_@$00$$CAD@__clang@@@Z"
 void f0(char __attribute__((address_space(1))) *p) { }
 
 struct OpaqueType;
 typedef OpaqueType __attribute__((address_space(100))) * OpaqueTypePtr;
 
 // CHECK-LABEL: def

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

[John McCall via Phabricator via cfe-commits]

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

Thanks, that looks good to me.


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

https://reviews.llvm.org/D55413



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

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 178259.
riccibruno marked 3 inline comments as done.
riccibruno added a comment.

Addressed inline comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D55658

Files:
  include/clang/AST/ComparisonCategories.h
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/DeclTemplate.h
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  include/clang/Sema/SemaInternal.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTImporter.cpp
  lib/AST/ComparisonCategories.cpp
  lib/AST/Decl.cpp
  lib/AST/DeclBase.cpp
  lib/AST/DeclTemplate.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGObjCGNU.cpp
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/Index/IndexingContext.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  tools/libclang/CIndex.cpp
  tools/libclang/CXIndexDataConsumer.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3336,18 +3336,20 @@
   FromTU, varDecl(hasName("a"))); // Decl with definition
   ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl());
   ASSERT_TRUE(FromDWithInit->getInit());
-  ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition());
-  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+  ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition(
+  FromDWithInit->getASTContext()));
+  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition(
+  FromDWithDef->getASTContext()));
   ASSERT_FALSE(FromDWithDef->getInit());
 
   auto *ToD = FirstDeclMatcher().match(
   ToTU, varDecl(hasName("a"))); // Decl with init
   ASSERT_TRUE(ToD->getInit());
-  ASSERT_FALSE(ToD->getDefinition());
+  ASSERT_FALSE(ToD->getDefinition(ToD->getASTContext()));
 
   auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
   EXPECT_TRUE(ImportedD->getAnyInitializer());
-  EXPECT_TRUE(ImportedD->getDefinition());
+  EXPECT_TRUE(ImportedD->getDefinition(ImportedD->getASTContext()));
 }
 
 TEST_P(ImportVariables, InitAndDefinitionAreInTheFromContext) {
@@ -3367,18 +3369,20 @@
   FromTU, varDecl(hasName("a"))); // Decl with definition and with init.
   ASSERT_EQ(FromDDeclarationOnly, FromDWithDef->getPreviousDecl());
   ASSERT_FALSE(FromDDeclarationOnly->getInit());
-  ASSERT_FALSE(FromDDeclarationOnly->isThisDeclarationADefinition());
-  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+  ASSERT_FALSE(FromDDeclarationOnly->isThisDeclarationADefinition(
+  FromDDeclarationOnly->getASTContext()));
+  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition(
+  FromDWithDef->getASTContext()));
   ASSERT_TRUE(FromDWithDef->getInit());
 
   auto *ToD = FirstDeclMatcher().match(
   ToTU, varDecl(hasName("a")));
   ASSERT_FALSE(ToD->getInit());
-  ASSERT_FALSE(ToD->getDefinition());
+  ASSERT_FALSE(ToD->getDefinition(ToD->getASTContext()));
 
   auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
   EXPECT_TRUE(ImportedD->getAnyInitializer());
-  EXPECT_TRUE(ImportedD->getDefinition());
+  EXPECT_TRUE(ImportedD->getDefinition(ImportedD->getASTContext()));
 }
 
 struct DeclContextTest : ASTImporterTestBase {};
Index: tools/libclang/CXIndexDataConsumer.cpp
===
--- tools/libclang/CXIndexDataConsumer.cpp
+++ tools/libclang/CXIndexDataConsumer.cpp
@@ -623,7 +623,8 @@
 }
 
 bool CXIndexDataConsumer::handleVar(const VarDecl *D) {
-  DeclInfo DInfo(!D->isFirstDecl(), D->isThisDeclarationADefinition(),
+  DeclInfo DInfo(!D->isFirstDecl(),
+ D->isThisDeclarationADefinition(getASTContext()),
  /*isContainer=*/false);
   return handleDecl(D, D->getLocation(), getCursor(D), DInfo);
 }
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -6290,7 +6290,8 @@
   case Decl::VarTemplatePartialSpecialization:
   case Decl::Decomposition: {
 // Ask the variable if it has a definition.
-if (const VarDecl *Def = cast(D)->getDefinition())
+if (const VarDecl *Def =
+cast(D)->getDefinition(D->getASTContext()))
   return MakeCXCursor(Def, TU);
 return clang_getNullCursor();
   }
@@ -6312,7 +6313,8 @@
 
   case Decl::VarTemplate: {
 if (VarDecl *

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

It would be interesting to teach `llvm-undname` to demangle this more 
naturally.  Right for this mangling: `?f1@@YAXPEAU?$AS_@$00$$CBD@__clang@@@Z` I 
see this demangling: `void __cdecl f1(struct __clang::AS_<1, char const> *)`.  
That's an accurate representation of the underlying structure, but it would be 
cool if we had special cases for things in the `__clang` namespace, so that we 
could display this as `void __cdecl f1(char __attribute__((address_space(1))) 
const *) {}`
`


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

https://reviews.llvm.org/D55715



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: test/CodeGenCXX/mangle-address-space.cpp:7
+// CHECKNOOCL-LABEL: define {{.*}}void @_Z2f0Pc
+// WINNOOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAD@Z"
+// CHECKOCL-LABEL: define {{.*}}void @_Z2f0PU9CLgenericc

You know, now that we have a demangler, I wonder if we shouldn't add a RUN line 
like this to test that these names really do demangle properly:
`// RUN: %clang_cc1 %s ... -emit-bitcode -o - | llvm-nm -demangle - | FileCheck 
%s --check-prefix=DEMANGLED`



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:7
+// RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple x86_64-windows-pc -emit-llvm -o - | FileCheck 
-check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s

I would replace `-triple x86_64-windows-pc` here and everywhere with `-triple 
x86_64-windows-itanium` (or gnu instead of itanium) to test the Itanium 
mangling on Windows. I don't think `x86_64-windows-pc` parses into a real 
triple. After all, "pc" is parsed as the vendor, which is supposed to be second.



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:24
 // NOASMANG20: @_Z2ffPU9CLgenerici
+// MS_ASMANG10: @_Z2ffPi
+// MS_ASMANG20: @_Z2ffPU3AS4i

Why "MS_"? This isn't the MSVC environment, this is using the Itanium C++ ABI 
mangling. Maybe WIN_ instead would be better?


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

https://reviews.llvm.org/D55715



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 4 inline comments as done.
erichkeane added inline comments.



Comment at: test/CodeGenCXX/mangle-address-space.cpp:7
+// CHECKNOOCL-LABEL: define {{.*}}void @_Z2f0Pc
+// WINNOOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAD@Z"
+// CHECKOCL-LABEL: define {{.*}}void @_Z2f0PU9CLgenericc

rnk wrote:
> You know, now that we have a demangler, I wonder if we shouldn't add a RUN 
> line like this to test that these names really do demangle properly:
> `// RUN: %clang_cc1 %s ... -emit-bitcode -o - | llvm-nm -demangle - | 
> FileCheck %s --check-prefix=DEMANGLED`
I'm not sure how that would work, but I can take a look.



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:7
+// RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple x86_64-windows-pc -emit-llvm -o - | FileCheck 
-check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s

rnk wrote:
> I would replace `-triple x86_64-windows-pc` here and everywhere with `-triple 
> x86_64-windows-itanium` (or gnu instead of itanium) to test the Itanium 
> mangling on Windows. I don't think `x86_64-windows-pc` parses into a real 
> triple. After all, "pc" is parsed as the vendor, which is supposed to be 
> second.
Woops! I didn't mean to check this file in.  It has some other nasty problems 
that need to be fixed up, so I tested this feature in the mangle-address-space. 
 I'll still do this above.


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

https://reviews.llvm.org/D55715



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:7
+// RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple x86_64-windows-pc -emit-llvm -o - | FileCheck 
-check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s

erichkeane wrote:
> rnk wrote:
> > I would replace `-triple x86_64-windows-pc` here and everywhere with 
> > `-triple x86_64-windows-itanium` (or gnu instead of itanium) to test the 
> > Itanium mangling on Windows. I don't think `x86_64-windows-pc` parses into 
> > a real triple. After all, "pc" is parsed as the vendor, which is supposed 
> > to be second.
> Woops! I didn't mean to check this file in.  It has some other nasty problems 
> that need to be fixed up, so I tested this feature in the 
> mangle-address-space.  I'll still do this above.
@rnk I tried this, but it changes off of the MicrosoftMangler and switches back 
to the Itanium mangling.  Is there a better triple for testing microsoft 
mangling?


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

https://reviews.llvm.org/D55715



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[David Majnemer via Phabricator via cfe-commits]

majnemer added inline comments.



Comment at: lib/AST/MicrosoftMangle.cpp:1806-1836
+Extra.mangleSourceName("AS_");
+Extra.mangleIntegerLiteral(llvm::APSInt::getUnsigned(TargetAS),
+   /*IsBoolean*/ false);
+  } else {
+switch (AS) {
+default:
+  llvm_unreachable("Not a language specific address space");

Don't these need to be _AS_Foobar to avoid collisions with normal code?


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

https://reviews.llvm.org/D55715



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

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

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

dexonsmith added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' 
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang 
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push 
NOT_MY_LABEL'}}

erik.pilkington wrote:
> aaron.ballman wrote:
> > dexonsmith wrote:
> > > aaron.ballman wrote:
> > > > Should we really treat this as an error? It seems to me that this 
> > > > should be a warning because pop without a label could be viewed as "I 
> > > > don't care what I'm popping, just pop it". Still worth warning about, 
> > > > but maybe not worth stopping a build over.
> > > IMO this is most likely to be an implementation error on the part of a 
> > > macro author, where the END macro is missing the label used in BEGIN.  
> > > This makes the macro pair unsafe to mix with other macros.  If the macro 
> > > author doesn’t want safety, why use a label in the BEGIN macro at all?
> > > 
> > > I see you’re envisioning this being used directly by an end-user, which I 
> > > suppose is plausible, but I think the same logic applies.  Why add a 
> > > label to push if you don’t want to be precise about pop?
> > > Why add a label to push if you don’t want to be precise about pop?
> > 
> > Why is this important enough to fail everyone's build over it as opposed to 
> > warning users that they've done something that could be a bad code smell 
> > and let -Werror usage decide whether to fail the build or not? It seems 
> > like an extreme measure for something that has explicable "fallback" 
> > behavior.
> My implicit assumption (which I should have been more clear about!) was that 
> you'd only really ever write a label on a `push` in a BEGIN/END macro. In 
> that world, you'd only ever see this case if 1) you're interacting with 
> another macro that doesn't use the label convention, or 2) if you're 
> interacting with manual push/pop code. In both of those cases, you'd end up 
> popping the wrong attribute group and start applying attributes onto 
> declarations that the programmer didn't intend.
> 
> I'm fine with downgrading this to a warning, but IMO an error seems more 
> appropriate. If we wanted to force 1) or 2) through the compiler then we'd 
> also need to downgrade `pop UNPUSHED_LABEL` to a warning, which doesn't seem 
> like the end of the world either.
I'm not fine with this just being a warning.  The entire point is that the 
stacks are independent; having them suddenly mixed here would be super 
confusing.  @aaron.ballman, please see my other comment for a longer 
explanation.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

erik.pilkington wrote:
> aaron.ballman wrote:
> > dexonsmith wrote:
> > > aaron.ballman wrote:
> > > > I feel like this should be diagnosed, perhaps even as an error. The 
> > > > user provided labels but then got the push and pop order wrong when 
> > > > explicitly saying what to pop. That seems more likely to be a logic 
> > > > error on the user's part.
> > > On the contrary, the user is using two differently-named and independent 
> > > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are 
> > > implemented with _Pragma(“clang attribute ...”) under the hood.  The 
> > > point is to give the same result as two independent pragma pairs, whose 
> > > regions do not need to be nested.
> > > On the contrary, the user is using two differently-named and independent 
> > > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) 
> > 
> > I don't think this is a safe assumption to make, and in this case, is 
> > false. There are no macros anywhere in this test case.
> > 
> > > The point is to give the same result as two independent pragma pairs, 
> > > whose regions do not need to be nested.
> > 
> > I don't find this to be intuitive behavior. These are stack manipulations 
> > with the names push and pop -- pretending that they're overlapping rather 
> > than a stack in the presence of labels is confusing. If I saw the code from 
> > this test case during a code review, I would flag it as being incorrect 
> > because the labels do not match -- I don't think I'd be the only one.
> I think the labels only really makes sense if you're writing macros that hide 
> the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), which for 
> better or for worse people do write, and in fact was the intended use case 
> for #pragma clang attribute. I think if we were to write this feature again, 
> we'd forgo the stack entirly and require every `push` to have a label and be 
> in its own namespace. But this is the best we can do now.
> 
> I don't really think that anyone should write a push label outside of a macro 
> definition, since I agree that the semanti

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/AST/MicrosoftMangle.cpp:1806-1836
+Extra.mangleSourceName("AS_");
+Extra.mangleIntegerLiteral(llvm::APSInt::getUnsigned(TargetAS),
+   /*IsBoolean*/ false);
+  } else {
+switch (AS) {
+default:
+  llvm_unreachable("Not a language specific address space");

majnemer wrote:
> Don't these need to be _AS_Foobar to avoid collisions with normal code?
I think we're in __clang::, so it's OK? In any case, it's what's done for 
Itanium, IIUC. It's also not like we have to worry about conflicts with user 
macros.



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:7
+// RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple x86_64-windows-pc -emit-llvm -o - | FileCheck 
-check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s

erichkeane wrote:
> erichkeane wrote:
> > rnk wrote:
> > > I would replace `-triple x86_64-windows-pc` here and everywhere with 
> > > `-triple x86_64-windows-itanium` (or gnu instead of itanium) to test the 
> > > Itanium mangling on Windows. I don't think `x86_64-windows-pc` parses 
> > > into a real triple. After all, "pc" is parsed as the vendor, which is 
> > > supposed to be second.
> > Woops! I didn't mean to check this file in.  It has some other nasty 
> > problems that need to be fixed up, so I tested this feature in the 
> > mangle-address-space.  I'll still do this above.
> @rnk I tried this, but it changes off of the MicrosoftMangler and switches 
> back to the Itanium mangling.  Is there a better triple for testing microsoft 
> mangling?
Tacking on "-msvc" or "-itanium" or "-gnu" is the more explicit way to the C++ 
ABI. I guess I was confused, it looks like this file expects Itanium manglings 
from the -pc triple.


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

https://reviews.llvm.org/D55715



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D46421#1121080 , @r.stahl wrote:

> It seems like a good idea to not do that, since non-const values are not 
> used. It might become useful if we ever do some kind of straight line 
> execution from static initialization to main.
>  However for structs it is enough if one of their fields is declared const.


Aaand in C++ there's also the `mutable` keyword that can cancel the effect of 
the surrounding `const` keyword, at least for the purposes of precise memory 
contents modeling in `RegionStore`.

The idea looks great to me. It was far from obvious to me that importing 
variables manually was necessary, nice catch.

I definitely wish for a more direct test for this, i.e. "CTU analysis avoids 
that specific false positive due to the new functionality".


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

https://reviews.llvm.org/D46421



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

[PATCH] D55698: [MinGW] Produce a vtable and RTTI for dllexported classes without a key function

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/Sema/SemaDeclCXX.cpp:5754-5756
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);

This may be too early, you can get into situations like this if you start 
emitting the vtable (which will emit inline methods) before we get to the end 
of the outermost class. See this bug for example:
https://bugs.llvm.org/show_bug.cgi?id=40006

Maybe if you have a dllexported nested class with a virtual method that 
references the constructor of the outer class which has a late-parsed member 
initializer... you can get things to go wrong as in the bug above.

I think the fix will be to touch the vtable when we process delayed dllexported 
classes from the list just above this line.



Comment at: test/CodeGenCXX/dllexport-missing-key.cpp:20
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr

OK, good, I was going to ask if it became weak_odr, but looks like it all works.


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

https://reviews.llvm.org/D55698



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

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

dexonsmith wrote:
> erik.pilkington wrote:
> > aaron.ballman wrote:
> > > dexonsmith wrote:
> > > > aaron.ballman wrote:
> > > > > I feel like this should be diagnosed, perhaps even as an error. The 
> > > > > user provided labels but then got the push and pop order wrong when 
> > > > > explicitly saying what to pop. That seems more likely to be a logic 
> > > > > error on the user's part.
> > > > On the contrary, the user is using two differently-named and 
> > > > independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no 
> > > > idea they are implemented with _Pragma(“clang attribute ...”) under the 
> > > > hood.  The point is to give the same result as two independent pragma 
> > > > pairs, whose regions do not need to be nested.
> > > > On the contrary, the user is using two differently-named and 
> > > > independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) 
> > > 
> > > I don't think this is a safe assumption to make, and in this case, is 
> > > false. There are no macros anywhere in this test case.
> > > 
> > > > The point is to give the same result as two independent pragma pairs, 
> > > > whose regions do not need to be nested.
> > > 
> > > I don't find this to be intuitive behavior. These are stack manipulations 
> > > with the names push and pop -- pretending that they're overlapping rather 
> > > than a stack in the presence of labels is confusing. If I saw the code 
> > > from this test case during a code review, I would flag it as being 
> > > incorrect because the labels do not match -- I don't think I'd be the 
> > > only one.
> > I think the labels only really makes sense if you're writing macros that 
> > hide the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), 
> > which for better or for worse people do write, and in fact was the intended 
> > use case for #pragma clang attribute. I think if we were to write this 
> > feature again, we'd forgo the stack entirly and require every `push` to 
> > have a label and be in its own namespace. But this is the best we can do 
> > now.
> > 
> > I don't really think that anyone should write a push label outside of a 
> > macro definition, since I agree that the semantics are a bit surprising 
> > when you're writing the #pragmas yourself without macros. I'll update this 
> > test case and the documentation to stress this point more. If you think 
> > this is going to be a potential pain point, maybe we can even warn on using 
> > a label outside of a macro definition. 
> >> The point is to give the same result as two independent pragma pairs, 
> >> whose regions do not need to be nested.
> > I don't find this to be intuitive behavior. These are stack manipulations 
> > with the names push and pop -- pretending that they're overlapping rather 
> > than a stack in the presence of labels is confusing. If I saw the code from 
> > this test case during a code review, I would flag it as being incorrect 
> > because the labels do not match -- I don't think I'd be the only one.
> 
> As Erik says, the intention is that these are only used by macros.
> 
> Stepping back, the goal of this pragma (which we added in 
> https://reviews.llvm.org/D30009) is to avoid adding a new region-based pragma 
> every time we want to apply an attribute within a region.  Clang has a lot of 
> these pragmas, in order to support independent macros like this:
> ```
> #define A_BEGIN _Pragma("clang a push")
> #define A_END   _Pragma("clang a pop")
> #define B_BEGIN _Pragma("clang b push")
> #define B_END   _Pragma("clang b pop")
> #define C_BEGIN _Pragma("clang c push")
> #define C_END   _Pragma("clang c pop")
> ```
> 
> @arphaman came up with the idea of introduce "one pragma to rule them all", 
> `pragma clang attribute`, so that we didn't need to introduce a new pragma 
> each time we wanted an independent region:
> ```
> #define A_BEGIN _Pragma("clang attribute push(a)")
> #define A_END   _Pragma("clang attribute pop")
> #define B_BEGIN _Pragma("clang attribute push(b)")
> #define B_END   _Pragma("clang attribute pop")
> #define C_BEGIN _Pragma("clang attribute push(c)")
> #define C_END   _Pragma("clang attribute pop")
> ```
> 
> However, we've realized that there is a major design flaw: these macros are 
> not independent, because they're using the same stack.  @erik.pilkington has 
> come up with this solution using identifiers to namespace the attribute 
> stacks, allowing the macros to be independent (like they were before, when 
> each had a dedicated pragma):
> ```
> #define A_BEGIN _Pragma("clang attribute push A (a)")
> #define A_END   _Pragma("clang attribute pop  A")
> #define B_BEGIN _Pragma("clang attribute push B (b)")
> #define B_END   _Pragma("clang attribute pop  B")
> #define C_BEGIN _Pragma("clang attribute push C (c)")
> #define C_END   _Pragm

[PATCH] D55710: add pragmas to control Software Pipelining optimisation

[Alexey Lapshin via Phabricator via cfe-commits]

alexey.lapshin updated this revision to Diff 178264.
alexey.lapshin added a comment.

deleted small typo in CGLoopInfo.cpp


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

https://reviews.llvm.org/D55710

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticParseKinds.td
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  lib/Parse/ParsePragma.cpp
  lib/Sema/SemaStmtAttr.cpp
  test/CodeGenCXX/pragma-pipeline.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-pipeline.cpp
  test/Parser/pragma-unroll-and-jam.cpp

Index: test/Parser/pragma-unroll-and-jam.cpp
===
--- test/Parser/pragma-unroll-and-jam.cpp
+++ test/Parser/pragma-unroll-and-jam.cpp
@@ -67,7 +67,7 @@
   }
 
 // pragma clang unroll_and_jam is disabled for the moment
-/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop unroll_and_jam(4)
+/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_ii_count or distribute}} */ #pragma clang loop unroll_and_jam(4)
   for (int i = 0; i < Length; i++) {
 for (int j = 0; j < Length; j++) {
   List[i * Length + j] = Value;
Index: test/Parser/pragma-pipeline.cpp
===
--- /dev/null
+++ test/Parser/pragma-pipeline.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++11 -verify %s
+
+// Note that this puts the expected lines before the directives to work around
+// limitations in the -verify mode.
+
+void test(int *List, int Length, int Value) {
+  int i = 0;
+
+#pragma clang loop pipeline(disable) 
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+#pragma clang loop pipeline_ii_count(10) 
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+/* expected-error {{expected ')'}} */ #pragma clang loop pipeline(disable
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(enable)
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(error)
+/* expected-error {{missing argument; expected an integer value}} */ #pragma clang loop pipeline_ii_count()
+/* expected-error {{use of undeclared identifier 'error'}} */ #pragma clang loop pipeline_ii_count(error)
+/* expected-error {{expected '('}} */ #pragma clang loop pipeline_ii_count 1 2
+  for (int i = 0; i < Length; i++) {
+for (int j = 0; j < Length; j++) {
+  List[i * Length + j] = Value;
+}
+  }
+
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int j = Length;
+#pragma clang loop pipeline_ii_count(4) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int k = Length;
+
+#pragma clang loop pipeline(disable)
+#pragma clang loop pipeline_ii_count(4) /* expected-error {{incompatible directives 'pipeline(disable)' and 'pipeline_ii_count(4)'}} */
+  for (int i = 0; i < Length; i++) {
+  List[i] = Value;
+  }
+
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected statement}} */ }
Index: test/Parser/pragma-loop.cpp
===
--- test/Parser/pragma-loop.cpp
+++ test/Parser/pragma-loop.cpp
@@ -147,7 +147,7 @@
 /* expected-error {{missing argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll()
 /* expected-error {{missing argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute()
 
-/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop
+/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_ii_count or distribute}} */ #pragma clang loop
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword(enable)
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop vectorize(enable) badkeyword(4)
Index: test/CodeGenCXX/pragma-pipeline.cpp
===
--- /dev/null
+++ test/CodeGenCXX/pragma-pipeline.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple hexagon -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+void pipeline_disabled(int *List, int Length, int Value) {
+  // CHECK-LABEL: define {{.*}} @_Z17pipeline_disabled 
+#pragma clang loop pipeline(disable) 
+  for (int i = 0; i < Length; i++) {
+  // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
+  List[i] = Value;
+  }
+}
+
+void pipeline_not_disabled(

[PATCH] D55707: Update to SARIF 11-28

[Artem Dergachev via Phabricator via cfe-commits]

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

Yup, looks good, and i keep passively cheering for this project to be 
successful.




Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+if (const json::Object *Obj = File.getAsObject()) {

This sounds like `find_if` to me.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:143
+  // that an empty file lists always causes a file to be added.
+  if (Files.empty() || Index == Files.size())
+Files.push_back(createFile(FE));

I suspect that the LHS of `||` implies the RHS of `||` and is therefore 
redundant.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:289
+if (P.second) {
+  RuleMapping[RuleID] = Rules.size(); // Maps RuleID to an Array Index.
+  Rules.push_back(createRule(*D));

Aha, ok, so now instead of an alphabetical order we have the order in which 
reports of the respective checkers are squeezed into the consumer. Which is 
probably deterministic, so it's fine. I just enjoy talking to myself on 
phabricator sometimes.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:318
 void SarifDiagnostics::FlushDiagnosticsImpl(
 std::vector &Diags, FilesMade *) {
   // We currently overwrite the file if it already exists. However, it may be

I wonder why we didn't mark `Diags` as `const &` in this callback.


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

https://reviews.llvm.org/D55707



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

[PATCH] D55707: Update to SARIF 11-28

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+if (const json::Object *Obj = File.getAsObject()) {

NoQ wrote:
> This sounds like `find_if` to me.
The problem is: we need the `Index`. It felt a bit weird to have 
`llvm::find_if()` (a const operation) also mutating a locally-captured 
variable. WDYT?



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:143
+  // that an empty file lists always causes a file to be added.
+  if (Files.empty() || Index == Files.size())
+Files.push_back(createFile(FE));

NoQ wrote:
> I suspect that the LHS of `||` implies the RHS of `||` and is therefore 
> redundant.
Nope, this is needed for a boundary condition. In the case where `Files` is 
empty, we don't loop over anything in the range-based for loop, and so `Index` 
is zero, which means `Index == Files.size()`. On the other end of that 
boundary, if `Files` is nonempty but `FE` hasn't been added to it yet, you'll 
loop over the entire list of `Files` in the range-based for loop, which also 
triggers `Index == Files.size()`.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:289
+if (P.second) {
+  RuleMapping[RuleID] = Rules.size(); // Maps RuleID to an Array Index.
+  Rules.push_back(createRule(*D));

NoQ wrote:
> Aha, ok, so now instead of an alphabetical order we have the order in which 
> reports of the respective checkers are squeezed into the consumer. Which is 
> probably deterministic, so it's fine. I just enjoy talking to myself on 
> phabricator sometimes.
Yes, this should be deterministic -- good thought to check!



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:318
 void SarifDiagnostics::FlushDiagnosticsImpl(
 std::vector &Diags, FilesMade *) {
   // We currently overwrite the file if it already exists. However, it may be

NoQ wrote:
> I wonder why we didn't mark `Diags` as `const &` in this callback.
It would be a nice refactoring for someday. :-)


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

https://reviews.llvm.org/D55707



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

r349188 - Update our SARIF support from 10-10 to 11-28.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Fri Dec 14 12:34:23 2018
New Revision: 349188

URL: http://llvm.org/viewvc/llvm-project?rev=349188&view=rev
Log:
Update our SARIF support from 10-10 to 11-28.

Functional changes include:

* The run.files property is now an array instead of a mapping.
* fileLocation objects now have a fileIndex property specifying the array index 
into run.files.
* The resource.rules property is now an array instead of a mapping.
* The result object was given a ruleIndex property that is an index into the 
resource.rules array.
* rule objects now have their "id" field filled out in addition to the name 
field.
* Updated the schema and spec version numbers to 11-28.

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

cfe/trunk/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif

cfe/trunk/test/Analysis/diagnostics/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif

Modified: cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp?rev=349188&r1=349187&r2=349188&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp Fri Dec 14 12:34:23 
2018
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 
@@ -77,7 +78,7 @@ static std::string fileNameToURI(StringR
   if (Root.startswith("//")) {
 // There is an authority, so add it to the URI.
 Ret += Root.drop_front(2).str();
-  } else {
+  } else if (!Root.empty()) {
 // There is no authority, so end the component and add the root to the URI.
 Ret += Twine("/" + Root).str();
   }
@@ -118,12 +119,31 @@ static json::Object createFile(const Fil
 }
 
 static json::Object createFileLocation(const FileEntry &FE,
-   json::Object &Files) {
+   json::Array &Files) {
   std::string FileURI = fileNameToURI(getFileName(FE));
-  if (!Files.get(FileURI))
-Files[FileURI] = createFile(FE);
 
-  return json::Object{{"uri", FileURI}};
+  // See if the Files array contains this URI already. If it does not, create
+  // a new file object to add to the array. Calculate the index within the file
+  // location array so it can be stored in the JSON object.
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+if (const json::Object *Obj = File.getAsObject()) {
+  if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
+Optional URI = FileLoc->getString("uri");
+if (URI && URI->equals(FileURI))
+  break;
+  }
+}
+++Index;
+  }
+
+  // If we reached the end of the array, then add the file to the list of files
+  // we're tracking; Index then points to the last element of the array. Note
+  // that an empty file lists always causes a file to be added.
+  if (Files.empty() || Index == Files.size())
+Files.push_back(createFile(FE));
+
+  return json::Object{{"uri", FileURI}, {"fileIndex", Index}};
 }
 
 static json::Object createTextRegion(SourceRange R, const SourceManager &SM) {
@@ -136,7 +156,7 @@ static json::Object createTextRegion(Sou
 
 static json::Object createPhysicalLocation(SourceRange R, const FileEntry &FE,
const SourceManager &SMgr,
-   json::Object &Files) {
+   json::Array &Files) {
   return json::Object{{{"fileLocation", createFileLocation(FE, Files)},
{"region", createTextRegion(R, SMgr)}}};
 }
@@ -190,7 +210,7 @@ static Importance calculateImportance(co
 }
 
 static json::Object createThreadFlow(const PathPieces &Pieces,
- json::Object &Files) {
+ json::Array &Files) {
   const SourceManager &SMgr = Pieces.front()->getLocation().getManager();
   json::Array Locations;
   for (const auto &Piece : Pieces) {
@@ -206,7 +226,7 @@ static json::Object createThreadFlow(con
 }
 
 static json::Object createCodeFlow(const PathPieces &Pieces,
-   json::Object &Files) {
+   json::Array &Files) {
   return json::Object{
   {"threadFlows", json::Array{createThreadFlow(Pieces, Files)}}};
 }
@@ -218,11 +238,14 @@ static json::Object createTool() {
   {"version", getClangFullVersion()}};
 }
 
-static json::Object createResult(const PathDiagnostic &Diag,
- json::Object &Files) {
+static json::Object createResult(const PathDiagnostic &Diag, json::

[PATCH] D55707: Update to SARIF 11-28

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed in r349188.

(@NoQ -- if you'd like to see a switch to `llvm::find_if()`, I'm happy to 
handle it post-commit.)


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

https://reviews.llvm.org/D55707



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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:7725-7726
+return true;
+  // Look through vector types, since we do default argument promotion for
+  // those in OpenCL.
+  if (const ExtVectorType *VecTy = From->getAs())

Looking at `Sema::DefaultArgumentPromotion()`, it seems there is some special 
logic there for _Float16 vs half/fp16. Do we need to deal with that here as 
well?



Comment at: lib/Sema/SemaChecking.cpp:7727
+  // those in OpenCL.
+  if (const ExtVectorType *VecTy = From->getAs())
+From = VecTy->getElementType();

Should use `const auto *` here and below.


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

https://reviews.llvm.org/D51211



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

[PATCH] D55707: Update to SARIF 11-28

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Sure, np.




Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+if (const json::Object *Obj = File.getAsObject()) {

aaron.ballman wrote:
> NoQ wrote:
> > This sounds like `find_if` to me.
> The problem is: we need the `Index`. It felt a bit weird to have 
> `llvm::find_if()` (a const operation) also mutating a locally-captured 
> variable. WDYT?
The way i see it:

```
auto I = std::find_if(Files.begin(), Files.end(), [&](const json::Value &File {
  if (const json::Object *Obj = File.getAsObject())
if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
  Optional URI = FileLoc->getString("uri");
  return URI && URI->equals(FileURI);
}
  return false;
});

if (I == Files.end())
  Files.push_back(createFile(FE));

return std::distance(Files.begin(), I);
```

Or pre-compute the index before `push_back` if the container invalidates 
iterators upon `push_back`.



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

https://reviews.llvm.org/D55707



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

[PATCH] D53713: Add extension to always default-initialize nullptr_t.

[Artem Dergachev via Phabricator via cfe-commits]

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

I approve this for Static Analyzer. As long as this is the correct thing to do 
in the compiler, Static Analyzer should ideally behave similarly to the 
compiler. Exposing portability bugs that would show up on other compilers is 
generally less important than behaving similarly to Clang itself.


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

https://reviews.llvm.org/D53713



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

r349190 - [analyzer] MoveChecker: Improve invalidation policies.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Fri Dec 14 12:47:58 2018
New Revision: 349190

URL: http://llvm.org/viewvc/llvm-project?rev=349190&view=rev
Log:
[analyzer] MoveChecker: Improve invalidation policies.

If a moved-from object is passed into a conservatively evaluated function
by pointer or by reference, we assume that the function may reset its state.

Make sure it doesn't apply to const pointers and const references. Add a test
that demonstrates that it does apply to rvalue references.

Additionally, make sure that the object is invalidated when its contents change
for reasons other than invalidation caused by evaluating a call conservatively.
In particular, when the object's fields are manipulated directly, we should
assume that some sort of reset may be happening.

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
cfe/trunk/test/Analysis/use-after-move.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=349190&r1=349189&r2=349190&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Fri Dec 14 12:47:58 
2018
@@ -53,8 +53,8 @@ public:
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
  const InvalidatedSymbols *Invalidated,
- ArrayRef ExplicitRegions,
- ArrayRef Regions,
+ ArrayRef RequestedRegions,
+ ArrayRef InvalidatedRegions,
  const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream &Out, ProgramStateRef State,
   const char *NL, const char *Sep) const override;
@@ -525,19 +525,35 @@ void MoveChecker::checkDeadSymbols(Symbo
 
 ProgramStateRef MoveChecker::checkRegionChanges(
 ProgramStateRef State, const InvalidatedSymbols *Invalidated,
-ArrayRef ExplicitRegions,
-ArrayRef Regions, const LocationContext *LCtx,
-const CallEvent *Call) const {
-  // In case of an InstanceCall don't remove the ThisRegion from the GDM since
-  // it is handled in checkPreCall and checkPostCall.
-  const MemRegion *ThisRegion = nullptr;
-  if (const auto *IC = dyn_cast_or_null(Call)) {
-ThisRegion = IC->getCXXThisVal().getAsRegion();
-  }
-
-  for (const auto *Region : ExplicitRegions) {
-if (ThisRegion != Region)
-  State = removeFromState(State, Region);
+ArrayRef RequestedRegions,
+ArrayRef InvalidatedRegions,
+const LocationContext *LCtx, const CallEvent *Call) const {
+  if (Call) {
+// Relax invalidation upon function calls: only invalidate parameters
+// that are passed directly via non-const pointers or non-const references
+// or rvalue references.
+// In case of an InstanceCall don't invalidate the this-region since
+// it is fully handled in checkPreCall and checkPostCall.
+const MemRegion *ThisRegion = nullptr;
+if (const auto *IC = dyn_cast(Call))
+  ThisRegion = IC->getCXXThisVal().getAsRegion();
+
+// Requested ("explicit") regions are the regions passed into the call
+// directly, but not all of them end up being invalidated.
+// But when they do, they appear in the InvalidatedRegions array as well.
+for (const auto *Region : RequestedRegions) {
+  if (ThisRegion != Region) {
+if (llvm::find(InvalidatedRegions, Region) !=
+std::end(InvalidatedRegions)) {
+  State = removeFromState(State, Region);
+}
+  }
+}
+  } else {
+// For invalidations that aren't caused by calls, assume nothing. In
+// particular, direct write into an object's field invalidates the status.
+for (const auto *Region : InvalidatedRegions)
+  State = removeFromState(State, Region->getBaseRegion());
   }
 
   return State;

Modified: cfe/trunk/test/Analysis/use-after-move.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/use-after-move.cpp?rev=349190&r1=349189&r2=349190&view=diff
==
--- cfe/trunk/test/Analysis/use-after-move.cpp (original)
+++ cfe/trunk/test/Analysis/use-after-move.cpp Fri Dec 14 12:47:58 2018
@@ -116,6 +116,19 @@ public:
   bool empty() const;
   bool isEmpty() const;
   operator bool() const;
+
+  void testUpdateField() {
+A a;
+A b = std::move(a);
+a.i = 1;
+a.foo(); // no-warning
+  }
+  void testUpdateFieldDouble() {
+A a;
+A b = std::move(a);
+a.d = 1.0;
+a.foo(); // no-warning
+  }
 };
 
 int bignum();
@@ -594,24 +607,50 @@ void paramEvaluateOrderTest() {
   foobar(a.getI(), std::move(a)); //no-warning
 }
 
-void not_known(A &a);
-void not_known(A *a);
+void not_known_pass_by_ref(A &a);
+void not_known_pass_by_const_ref

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349190: [analyzer] MoveChecker: Improve invalidation 
policies. (authored by dergachev, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D55289

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -116,6 +116,19 @@
   bool empty() const;
   bool isEmpty() const;
   operator bool() const;
+
+  void testUpdateField() {
+A a;
+A b = std::move(a);
+a.i = 1;
+a.foo(); // no-warning
+  }
+  void testUpdateFieldDouble() {
+A a;
+A b = std::move(a);
+a.d = 1.0;
+a.foo(); // no-warning
+  }
 };
 
 int bignum();
@@ -594,24 +607,50 @@
   foobar(a.getI(), std::move(a)); //no-warning
 }
 
-void not_known(A &a);
-void not_known(A *a);
+void not_known_pass_by_ref(A &a);
+void not_known_pass_by_const_ref(const A &a);
+void not_known_pass_by_rvalue_ref(A &&a);
+void not_known_pass_by_ptr(A *a);
+void not_known_pass_by_const_ptr(const A *a);
 
 void regionAndPointerEscapeTest() {
   {
 A a;
 A b;
 b = std::move(a);
-not_known(a);
-a.foo(); //no-warning
+not_known_pass_by_ref(a);
+a.foo(); // no-warning
+  }
+  {
+A a;
+A b;
+b = std::move(a); // expected-note{{Object 'a' is moved}}
+not_known_pass_by_const_ref(a);
+a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
+ // expected-note@-1{{Method called on moved-from object 'a'}}
+  }
+  {
+A a;
+A b;
+b = std::move(a);
+not_known_pass_by_rvalue_ref(std::move(a));
+a.foo(); // no-warning
   }
   {
 A a;
 A b;
 b = std::move(a);
-not_known(&a);
+not_known_pass_by_ptr(&a);
 a.foo(); // no-warning
   }
+  {
+A a;
+A b;
+b = std::move(a); // expected-note{{Object 'a' is moved}}
+not_known_pass_by_const_ptr(&a);
+a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
+ // expected-note@-1{{Method called on moved-from object 'a'}}
+  }
 }
 
 // A declaration statement containing multiple declarations sequences the
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -53,8 +53,8 @@
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
  const InvalidatedSymbols *Invalidated,
- ArrayRef ExplicitRegions,
- ArrayRef Regions,
+ ArrayRef RequestedRegions,
+ ArrayRef InvalidatedRegions,
  const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream &Out, ProgramStateRef State,
   const char *NL, const char *Sep) const override;
@@ -525,19 +525,35 @@
 
 ProgramStateRef MoveChecker::checkRegionChanges(
 ProgramStateRef State, const InvalidatedSymbols *Invalidated,
-ArrayRef ExplicitRegions,
-ArrayRef Regions, const LocationContext *LCtx,
-const CallEvent *Call) const {
-  // In case of an InstanceCall don't remove the ThisRegion from the GDM since
-  // it is handled in checkPreCall and checkPostCall.
-  const MemRegion *ThisRegion = nullptr;
-  if (const auto *IC = dyn_cast_or_null(Call)) {
-ThisRegion = IC->getCXXThisVal().getAsRegion();
-  }
-
-  for (const auto *Region : ExplicitRegions) {
-if (ThisRegion != Region)
-  State = removeFromState(State, Region);
+ArrayRef RequestedRegions,
+ArrayRef InvalidatedRegions,
+const LocationContext *LCtx, const CallEvent *Call) const {
+  if (Call) {
+// Relax invalidation upon function calls: only invalidate parameters
+// that are passed directly via non-const pointers or non-const references
+// or rvalue references.
+// In case of an InstanceCall don't invalidate the this-region since
+// it is fully handled in checkPreCall and checkPostCall.
+const MemRegion *ThisRegion = nullptr;
+if (const auto *IC = dyn_cast(Call))
+  ThisRegion = IC->getCXXThisVal().getAsRegion();
+
+// Requested ("explicit") regions are the regions passed into the call
+// directly, but not all of them end up being invalidated.
+// But when they do, they appear in the InvalidatedRegions array as well.
+for (const auto *Region : RequestedRegions) {
+  if (ThisRegion != Region) {
+if (llvm::find(InvalidatedRegions, Region) !=
+std::end(InvalidatedRegions)) {
+  State = removeFromState(State, Region);
+}
+  }
+}
+  } else {
+// For invalidations that aren't caused by calls, assume nothing. In
+// particular, 

r349191 - [analyzer] MoveChecker Pt.6: Suppress the warning for the move-safe STL classes.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Fri Dec 14 12:52:57 2018
New Revision: 349191

URL: http://llvm.org/viewvc/llvm-project?rev=349191&view=rev
Log:
[analyzer] MoveChecker Pt.6: Suppress the warning for the move-safe STL classes.

Some C++ standard library classes provide additional guarantees about their
state after move. Suppress warnings on such classes until a more precise
behavior is implemented. Warnings for locals are not suppressed anyway
because it's still most likely a bug.

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h
cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp
cfe/trunk/test/Analysis/use-after-move.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=349191&r1=349190&r2=349191&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Fri Dec 14 12:52:57 
2018
@@ -20,6 +20,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/StringSet.h"
 
 using namespace clang;
 using namespace ento;
@@ -67,20 +68,43 @@ private:
 bool STL : 1; // Is this an object of a standard type?
   };
 
+  // Not all of these are entirely move-safe, but they do provide *some*
+  // guarantees, and it means that somebody is using them after move
+  // in a valid manner.
+  // TODO: We can still try to identify *unsafe* use after move, such as
+  // dereference of a moved-from smart pointer (which is guaranteed to be 
null).
+  const llvm::StringSet<> StandardMoveSafeClasses = {
+  "basic_filebuf",
+  "basic_ios",
+  "future",
+  "optional",
+  "packaged_task"
+  "promise",
+  "shared_future",
+  "shared_lock",
+  "shared_ptr",
+  "thread",
+  "unique_ptr",
+  "unique_lock",
+  "weak_ptr",
+  };
+
   // Obtains ObjectKind of an object. Because class declaration cannot always
   // be easily obtained from the memory region, it is supplied separately.
-  static ObjectKind classifyObject(const MemRegion *MR,
-   const CXXRecordDecl *RD);
+  ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) 
const;
 
   // Classifies the object and dumps a user-friendly description string to
   // the stream. Return value is equivalent to classifyObject.
-  static ObjectKind explainObject(llvm::raw_ostream &OS,
-  const MemRegion *MR, const CXXRecordDecl 
*RD);
+  ObjectKind explainObject(llvm::raw_ostream &OS,
+   const MemRegion *MR, const CXXRecordDecl *RD) const;
+
+  bool isStandardMoveSafeClass(const CXXRecordDecl *RD) const;
 
   class MovedBugVisitor : public BugReporterVisitor {
   public:
-MovedBugVisitor(const MemRegion *R, const CXXRecordDecl *RD)
-: Region(R), RD(RD), Found(false) {}
+MovedBugVisitor(const MoveChecker &Chk,
+const MemRegion *R, const CXXRecordDecl *RD)
+: Chk(Chk), Region(R), RD(RD), Found(false) {}
 
 void Profile(llvm::FoldingSetNodeID &ID) const override {
   static int X = 0;
@@ -97,6 +121,7 @@ private:
BugReport &BR) override;
 
   private:
+const MoveChecker &Chk;
 // The tracked region.
 const MemRegion *Region;
 // The class of the tracked object.
@@ -157,7 +182,7 @@ static const MemRegion *unwrapRValueRefe
 
 std::shared_ptr
 MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
-BugReporterContext &BRC, BugReport &) {
+BugReporterContext &BRC, BugReport 
&BR) {
   // We need only the last move of the reported object's region.
   // The visitor walks the ExplodedGraph backwards.
   if (Found)
@@ -182,7 +207,7 @@ MoveChecker::MovedBugVisitor::VisitNode(
   llvm::raw_svector_ostream OS(Str);
 
   OS << "Object";
-  ObjectKind OK = explainObject(OS, Region, RD);
+  ObjectKind OK = Chk.explainObject(OS, Region, RD);
   if (OK.STL)
 OS << " is left in a valid but unspecified state after move";
   else
@@ -251,7 +276,7 @@ ExplodedNode *MoveChecker::reportBug(con
 auto R =
 llvm::make_unique(*BT, OS.str(), N, LocUsedForUniqueing,
  
MoveNode->getLocationContext()->getDecl());
-R->addVisitor(llvm::make_unique(Region, RD));
+R->addVisitor(llvm::make_unique(*this, Region, RD));
 C.emitReport(std::move(R));
 return N;
   }
@@ -370,20 +395,30 @@ bool MoveChecker::isInMoveSafeContext(co
   return false;
 

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349191: [analyzer] MoveChecker Pt.6: Suppress the warning 
for the move-safe STL classes. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55307?vs=177048&id=178266#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T &a, T &b) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template  // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T *get() const;
+
+typename std::add_lvalue_reference::type operator*() const;
+T *operator->() const;
+  };
+}
+#endif
+
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &&move(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T &a, T &b) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T &t);
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &&a) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/Static

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: test/Analysis/Inputs/system-header-simulator-cxx.h:782
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {

a_sidorin wrote:
> Nit: our coding rules require a space before template lbrace.
Fxd.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55307



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

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

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

dexonsmith added inline comments.



Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+

aaron.ballman wrote:
> dexonsmith wrote:
> > erik.pilkington wrote:
> > > aaron.ballman wrote:
> > > > dexonsmith wrote:
> > > > > aaron.ballman wrote:
> > > > > > I feel like this should be diagnosed, perhaps even as an error. The 
> > > > > > user provided labels but then got the push and pop order wrong when 
> > > > > > explicitly saying what to pop. That seems more likely to be a logic 
> > > > > > error on the user's part.
> > > > > On the contrary, the user is using two differently-named and 
> > > > > independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no 
> > > > > idea they are implemented with _Pragma(“clang attribute ...”) under 
> > > > > the hood.  The point is to give the same result as two independent 
> > > > > pragma pairs, whose regions do not need to be nested.
> > > > > On the contrary, the user is using two differently-named and 
> > > > > independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) 
> > > > 
> > > > I don't think this is a safe assumption to make, and in this case, is 
> > > > false. There are no macros anywhere in this test case.
> > > > 
> > > > > The point is to give the same result as two independent pragma pairs, 
> > > > > whose regions do not need to be nested.
> > > > 
> > > > I don't find this to be intuitive behavior. These are stack 
> > > > manipulations with the names push and pop -- pretending that they're 
> > > > overlapping rather than a stack in the presence of labels is confusing. 
> > > > If I saw the code from this test case during a code review, I would 
> > > > flag it as being incorrect because the labels do not match -- I don't 
> > > > think I'd be the only one.
> > > I think the labels only really makes sense if you're writing macros that 
> > > hide the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), 
> > > which for better or for worse people do write, and in fact was the 
> > > intended use case for #pragma clang attribute. I think if we were to 
> > > write this feature again, we'd forgo the stack entirly and require every 
> > > `push` to have a label and be in its own namespace. But this is the best 
> > > we can do now.
> > > 
> > > I don't really think that anyone should write a push label outside of a 
> > > macro definition, since I agree that the semantics are a bit surprising 
> > > when you're writing the #pragmas yourself without macros. I'll update 
> > > this test case and the documentation to stress this point more. If you 
> > > think this is going to be a potential pain point, maybe we can even warn 
> > > on using a label outside of a macro definition. 
> > >> The point is to give the same result as two independent pragma pairs, 
> > >> whose regions do not need to be nested.
> > > I don't find this to be intuitive behavior. These are stack manipulations 
> > > with the names push and pop -- pretending that they're overlapping rather 
> > > than a stack in the presence of labels is confusing. If I saw the code 
> > > from this test case during a code review, I would flag it as being 
> > > incorrect because the labels do not match -- I don't think I'd be the 
> > > only one.
> > 
> > As Erik says, the intention is that these are only used by macros.
> > 
> > Stepping back, the goal of this pragma (which we added in 
> > https://reviews.llvm.org/D30009) is to avoid adding a new region-based 
> > pragma every time we want to apply an attribute within a region.  Clang has 
> > a lot of these pragmas, in order to support independent macros like this:
> > ```
> > #define A_BEGIN _Pragma("clang a push")
> > #define A_END   _Pragma("clang a pop")
> > #define B_BEGIN _Pragma("clang b push")
> > #define B_END   _Pragma("clang b pop")
> > #define C_BEGIN _Pragma("clang c push")
> > #define C_END   _Pragma("clang c pop")
> > ```
> > 
> > @arphaman came up with the idea of introduce "one pragma to rule them all", 
> > `pragma clang attribute`, so that we didn't need to introduce a new pragma 
> > each time we wanted an independent region:
> > ```
> > #define A_BEGIN _Pragma("clang attribute push(a)")
> > #define A_END   _Pragma("clang attribute pop")
> > #define B_BEGIN _Pragma("clang attribute push(b)")
> > #define B_END   _Pragma("clang attribute pop")
> > #define C_BEGIN _Pragma("clang attribute push(c)")
> > #define C_END   _Pragma("clang attribute pop")
> > ```
> > 
> > However, we've realized that there is a major design flaw: these macros are 
> > not independent, because they're using the same stack.  @erik.pilkington 
> > has come up with this solution using identifiers to namespace the attribute 
> > stacks, allowing the macros to be independent (like they were before, when 
> > each had a dedicated pragma):
> > ```
> > #define A_BEGIN _Pragma("clang attribute push A (a)")
> > #define A_END   _Pragma("clang attribute 

r349192 - [OPENMP][NVPTX]Improved interwarp copy function.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Fri Dec 14 13:00:58 2018
New Revision: 349192

URL: http://llvm.org/viewvc/llvm-project?rev=349192&view=rev
Log:
[OPENMP][NVPTX]Improved interwarp copy function.

Inlined runtime with the current implementation of the interwarp copy
function leads to the undefined behavior because of the not quite
correct implementation of the barriers. Start using generic
__kmpc_barier function instead of the custom made barriers.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/test/OpenMP/nvptx_target_parallel_reduction_codegen.cpp
cfe/trunk/test/OpenMP/nvptx_teams_reduction_codegen.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp?rev=349192&r1=349191&r2=349192&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp Fri Dec 14 13:00:58 2018
@@ -189,13 +189,6 @@ enum MachineConfiguration : unsigned {
   SharedMemorySize = 128,
 };
 
-enum NamedBarrier : unsigned {
-  /// Synchronize on this barrier #ID using a named barrier primitive.
-  /// Only the subset of active threads in a parallel region arrive at the
-  /// barrier.
-  NB_Parallel = 1,
-};
-
 static const ValueDecl *getPrivateItem(const Expr *RefExpr) {
   RefExpr = RefExpr->IgnoreParens();
   if (const auto *ASE = dyn_cast(RefExpr)) {
@@ -655,26 +648,9 @@ static void getNVPTXCTABarrier(CodeGenFu
   CGF.EmitRuntimeCall(F);
 }
 
-/// Get barrier #ID to synchronize selected (multiple of warp size) threads in
-/// a CTA.
-static void getNVPTXBarrier(CodeGenFunction &CGF, int ID,
-llvm::Value *NumThreads) {
-  CGBuilderTy &Bld = CGF.Builder;
-  llvm::Value *Args[] = {Bld.getInt32(ID), NumThreads};
-  llvm::Function *F = llvm::Intrinsic::getDeclaration(
-  &CGF.CGM.getModule(), llvm::Intrinsic::nvvm_barrier);
-  F->addFnAttr(llvm::Attribute::Convergent);
-  CGF.EmitRuntimeCall(F, Args);
-}
-
 /// Synchronize all GPU threads in a block.
 static void syncCTAThreads(CodeGenFunction &CGF) { getNVPTXCTABarrier(CGF); }
 
-/// Synchronize worker threads in a parallel region.
-static void syncParallelThreads(CodeGenFunction &CGF, llvm::Value *NumThreads) 
{
-  return getNVPTXBarrier(CGF, NB_Parallel, NumThreads);
-}
-
 /// Get the value of the thread_limit clause in the teams directive.
 /// For the 'generic' execution mode, the runtime encodes thread_limit in
 /// the launch parameters, always starting thread_limit+warpSize threads per
@@ -3272,14 +3248,10 @@ static llvm::Value *emitInterWarpCopyFun
 
   CGF.EmitBlock(MergeBB);
 
-  Address AddrNumWarpsArg = CGF.GetAddrOfLocalVar(&NumWarpsArg);
-  llvm::Value *NumWarpsVal = CGF.EmitLoadOfScalar(
-  AddrNumWarpsArg, /*Volatile=*/false, C.IntTy, Loc);
-
-  llvm::Value *NumActiveThreads = Bld.CreateNSWMul(
-  NumWarpsVal, getNVPTXWarpSize(CGF), "num_active_threads");
-  // named_barrier_sync(ParallelBarrierID, num_active_threads)
-  syncParallelThreads(CGF, NumActiveThreads);
+  // kmpc_barrier.
+  CGM.getOpenMPRuntime().emitBarrierCall(CGF, Loc, OMPD_unknown,
+ /*EmitChecks=*/false,
+ /*ForceSimpleCall=*/true);
 
   //
   // Warp 0 copies reduce element from transfer medium.
@@ -3288,6 +3260,10 @@ static llvm::Value *emitInterWarpCopyFun
   llvm::BasicBlock *W0ElseBB = CGF.createBasicBlock("else");
   llvm::BasicBlock *W0MergeBB = CGF.createBasicBlock("ifcont");
 
+  Address AddrNumWarpsArg = CGF.GetAddrOfLocalVar(&NumWarpsArg);
+  llvm::Value *NumWarpsVal = CGF.EmitLoadOfScalar(
+  AddrNumWarpsArg, /*Volatile=*/false, C.IntTy, Loc);
+
   // Up to 32 threads in warp 0 are active.
   llvm::Value *IsActiveThread =
   Bld.CreateICmpULT(ThreadID, NumWarpsVal, "is_active_thread");
@@ -3329,7 +3305,10 @@ static llvm::Value *emitInterWarpCopyFun
 
   // While warp 0 copies values from transfer medium, all other warps must
   // wait.
-  syncParallelThreads(CGF, NumActiveThreads);
+  // kmpc_barrier.
+  CGM.getOpenMPRuntime().emitBarrierCall(CGF, Loc, OMPD_unknown,
+ /*EmitChecks=*/false,
+ /*ForceSimpleCall=*/true);
   if (NumIters > 1) {
 Cnt = Bld.CreateNSWAdd(Cnt, llvm::ConstantInt::get(CGM.IntTy, 
/*V=*/1));
 CGF.EmitStoreOfScalar(Cnt, CntAddr, /*Volatile=*/false, C.IntTy);

Modified: cfe/trunk/test/OpenMP/nvptx_target_parallel_reduction_codegen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/nvptx_target_parallel_reduction_codegen.cpp?rev=349192&r1=349191&r2=349192&view=diff
==
--- cfe/trunk/test/Open

[PATCH] D40218: [Clang] Add __builtin_launder

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 178267.
EricWF added a comment.

Merging with upstream. Preparing to commit.


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

https://reviews.llvm.org/D40218

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtins.c
  test/CodeGenCXX/builtin-launder.cpp
  test/Preprocessor/feature_tests.c
  test/Sema/builtins.c
  test/SemaCXX/builtins.cpp

Index: test/SemaCXX/builtins.cpp
===
--- test/SemaCXX/builtins.cpp
+++ test/SemaCXX/builtins.cpp
@@ -53,3 +53,95 @@
 void synchronize_args() {
   __sync_synchronize(0); // expected-error {{too many arguments}}
 }
+
+namespace test_launder {
+#define TEST_TYPE(Ptr, Type) \
+  static_assert(__is_same(decltype(__builtin_launder(Ptr)), Type), "expected same type")
+
+struct Dummy {};
+
+using FnType = int(char);
+using MemFnType = int (Dummy::*)(char);
+using ConstMemFnType = int (Dummy::*)() const;
+
+void foo() {}
+
+void test_builtin_launder_diags(void *vp, const void *cvp, FnType *fnp,
+MemFnType mfp, ConstMemFnType cmfp, int (&Arr)[5]) {
+  __builtin_launder(vp);   // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cvp);  // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(fnp);  // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(mfp);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cmfp); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder(&fnp);
+  __builtin_launder(42);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(nullptr); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(foo); // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder(Arr);
+}
+
+void test_builtin_launder(char *p, const volatile int *ip, const float *&fp,
+  double *__restrict dp) {
+  int x;
+  __builtin_launder(x); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+
+  TEST_TYPE(p, char*);
+  TEST_TYPE(ip, const volatile int*);
+  TEST_TYPE(fp, const float*);
+  TEST_TYPE(dp, double *__restrict);
+
+  char *d = __builtin_launder(p);
+  const volatile int *id = __builtin_launder(ip);
+  int *id2 = __builtin_launder(ip); // expected-error {{cannot initialize a variable of type 'int *' with an rvalue of type 'const volatile int *'}}
+  const float* fd = __builtin_launder(fp);
+}
+
+void test_launder_return_type(const int (&ArrayRef)[101], int (&MArrRef)[42][13],
+  void (**&FuncPtrRef)()) {
+  TEST_TYPE(ArrayRef, const int *);
+  TEST_TYPE(MArrRef, int(*)[13]);
+  TEST_TYPE(FuncPtrRef, void (**)());
+}
+
+template 
+constexpr Tp *test_constexpr_launder(Tp *tp) {
+  return __builtin_launder(tp);
+}
+constexpr int const_int = 42;
+constexpr int const_int2 = 101;
+constexpr const int *const_ptr = test_constexpr_launder(&const_int);
+static_assert(&const_int == const_ptr, "");
+static_assert(const_ptr != test_constexpr_launder(&const_int2), "");
+
+void test_non_constexpr() {
+  constexpr int i = 42;// expected-note {{declared here}}
+  constexpr const int *ip = __builtin_launder(&i); // expected-error {{constexpr variable 'ip' must be initialized by a constant expression}}
+  // expected-note@-1 {{pointer to 'i' is not a constant expression}}
+}
+
+constexpr bool test_in_constexpr(const int &i) {
+  return (__builtin_launder(&i) == &i);
+}
+
+static_assert(test_in_constexpr(const_int), "");
+void f() {
+  constexpr int i = 42;
+  static_assert(test_in_constexpr(i), "");
+}
+
+struct Incomplete; // expected-note {{forward declaration}}
+struct IncompleteMember {
+  Incomplete &i;
+};
+void test_incomplete(Incomplete *i, IncompleteMember *im) {
+  // expected-error@+1 {{incomplete type 'test_launder::Incomplete' where a complete type is required}}
+  __builtin_launder(i);
+  __builtin_launder(&i); // OK
+  __builtin_launder(im); // OK
+}
+
+void test_noexcept(int *i) {
+  static_assert(noexcept(__builtin_launder(i)), "");
+}
+#undef TEST_TYPE
+} // end namespace test_launder
Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -258,6 +258,24 @@
 return buf;
 }
 
+typedef void (fn_t)(int);
+
+void test_builtin_launder(char *p, void *vp, const void *cvp,
+  const volatile int *ip, float *restrict fp,
+  fn_t *fn) {
+  __builtin_laund

[PATCH] D40218: [Clang] Add __builtin_launder

[Eric Fiselier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349195: [Clang] Add __builtin_launder (authored by EricWF, 
committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D40218

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtins.c
  test/CodeGenCXX/builtin-launder.cpp
  test/Preprocessor/feature_tests.c
  test/Sema/builtins.c
  test/SemaCXX/builtins.cpp

Index: test/SemaCXX/builtins.cpp
===
--- test/SemaCXX/builtins.cpp
+++ test/SemaCXX/builtins.cpp
@@ -53,3 +53,95 @@
 void synchronize_args() {
   __sync_synchronize(0); // expected-error {{too many arguments}}
 }
+
+namespace test_launder {
+#define TEST_TYPE(Ptr, Type) \
+  static_assert(__is_same(decltype(__builtin_launder(Ptr)), Type), "expected same type")
+
+struct Dummy {};
+
+using FnType = int(char);
+using MemFnType = int (Dummy::*)(char);
+using ConstMemFnType = int (Dummy::*)() const;
+
+void foo() {}
+
+void test_builtin_launder_diags(void *vp, const void *cvp, FnType *fnp,
+MemFnType mfp, ConstMemFnType cmfp, int (&Arr)[5]) {
+  __builtin_launder(vp);   // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cvp);  // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(fnp);  // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(mfp);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cmfp); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder(&fnp);
+  __builtin_launder(42);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(nullptr); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(foo); // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder(Arr);
+}
+
+void test_builtin_launder(char *p, const volatile int *ip, const float *&fp,
+  double *__restrict dp) {
+  int x;
+  __builtin_launder(x); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+
+  TEST_TYPE(p, char*);
+  TEST_TYPE(ip, const volatile int*);
+  TEST_TYPE(fp, const float*);
+  TEST_TYPE(dp, double *__restrict);
+
+  char *d = __builtin_launder(p);
+  const volatile int *id = __builtin_launder(ip);
+  int *id2 = __builtin_launder(ip); // expected-error {{cannot initialize a variable of type 'int *' with an rvalue of type 'const volatile int *'}}
+  const float* fd = __builtin_launder(fp);
+}
+
+void test_launder_return_type(const int (&ArrayRef)[101], int (&MArrRef)[42][13],
+  void (**&FuncPtrRef)()) {
+  TEST_TYPE(ArrayRef, const int *);
+  TEST_TYPE(MArrRef, int(*)[13]);
+  TEST_TYPE(FuncPtrRef, void (**)());
+}
+
+template 
+constexpr Tp *test_constexpr_launder(Tp *tp) {
+  return __builtin_launder(tp);
+}
+constexpr int const_int = 42;
+constexpr int const_int2 = 101;
+constexpr const int *const_ptr = test_constexpr_launder(&const_int);
+static_assert(&const_int == const_ptr, "");
+static_assert(const_ptr != test_constexpr_launder(&const_int2), "");
+
+void test_non_constexpr() {
+  constexpr int i = 42;// expected-note {{declared here}}
+  constexpr const int *ip = __builtin_launder(&i); // expected-error {{constexpr variable 'ip' must be initialized by a constant expression}}
+  // expected-note@-1 {{pointer to 'i' is not a constant expression}}
+}
+
+constexpr bool test_in_constexpr(const int &i) {
+  return (__builtin_launder(&i) == &i);
+}
+
+static_assert(test_in_constexpr(const_int), "");
+void f() {
+  constexpr int i = 42;
+  static_assert(test_in_constexpr(i), "");
+}
+
+struct Incomplete; // expected-note {{forward declaration}}
+struct IncompleteMember {
+  Incomplete &i;
+};
+void test_incomplete(Incomplete *i, IncompleteMember *im) {
+  // expected-error@+1 {{incomplete type 'test_launder::Incomplete' where a complete type is required}}
+  __builtin_launder(i);
+  __builtin_launder(&i); // OK
+  __builtin_launder(im); // OK
+}
+
+void test_noexcept(int *i) {
+  static_assert(noexcept(__builtin_launder(i)), "");
+}
+#undef TEST_TYPE
+} // end namespace test_launder
Index: test/CodeGen/builtins.c
===
--- test/CodeGen/builtins.c
+++ test/CodeGen/builtins.c
@@ -132,6 +132,8 @@
   R(extract_return_addr, (&N));
   P(signbit, (1.0));
 
+  R(launder, (&N));
+
   return 0;
 }
 
@@ -396,6 +398,15 @@
   return __builtin_readcyclecounter(

r349195 - [Clang] Add __builtin_launder

[Eric Fiselier via cfe-commits]

Author: ericwf
Date: Fri Dec 14 13:11:28 2018
New Revision: 349195

URL: http://llvm.org/viewvc/llvm-project?rev=349195&view=rev
Log:
[Clang] Add __builtin_launder

Summary:
This patch adds `__builtin_launder`, which is required to implement 
`std::launder`. Additionally GCC provides `__builtin_launder`, so thing brings 
Clang in-line with GCC.

I'm not exactly sure what magic `__builtin_launder` requires, but  based on 
previous discussions this patch applies a `@llvm.invariant.group.barrier`. As 
noted in previous discussions, this may not be enough to correctly handle 
vtables.

Reviewers: rnk, majnemer, rsmith

Reviewed By: rsmith

Subscribers: kristina, Romain-Geissler-1A, erichkeane, amharc, jroelofs, 
cfe-commits, Prazek

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

Added:
cfe/trunk/test/CodeGenCXX/builtin-launder.cpp
Modified:
cfe/trunk/include/clang/Basic/Builtins.def
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/CodeGen/builtins.c
cfe/trunk/test/Preprocessor/feature_tests.c
cfe/trunk/test/Sema/builtins.c
cfe/trunk/test/SemaCXX/builtins.cpp

Modified: cfe/trunk/include/clang/Basic/Builtins.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=349195&r1=349194&r2=349195&view=diff
==
--- cfe/trunk/include/clang/Basic/Builtins.def (original)
+++ cfe/trunk/include/clang/Basic/Builtins.def Fri Dec 14 13:11:28 2018
@@ -498,6 +498,7 @@ BUILTIN(__builtin_snprintf, "ic*zcC*.",
 BUILTIN(__builtin_vsprintf, "ic*cC*a", "nFP:1:")
 BUILTIN(__builtin_vsnprintf, "ic*zcC*a", "nFP:2:")
 BUILTIN(__builtin_thread_pointer, "v*", "nc")
+BUILTIN(__builtin_launder, "v*v*", "nt")
 
 // GCC exception builtins
 BUILTIN(__builtin_eh_return, "vzv*", "r") // FIXME: Takes intptr_t, not size_t!

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=349195&r1=349194&r2=349195&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Dec 14 13:11:28 
2018
@@ -9475,4 +9475,8 @@ def warn_noderef_on_non_pointer_or_array
 def warn_noderef_to_dereferenceable_pointer : Warning<
   "casting to dereferenceable pointer removes 'noderef' attribute">, 
InGroup;
 
+def err_builtin_launder_invalid_arg : Error<
+  "%select{non-pointer|function pointer|void pointer}0 argument to "
+  "'__builtin_launder' is not allowed">;
+
 } // end of sema component.

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=349195&r1=349194&r2=349195&view=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Dec 14 13:11:28 2018
@@ -6112,7 +6112,8 @@ bool PointerExprEvaluator::VisitBuiltinC
 
 return true;
   }
-
+  case Builtin::BI__builtin_launder:
+return evaluatePointer(E->getArg(0), Result);
   case Builtin::BIstrchr:
   case Builtin::BIwcschr:
   case Builtin::BImemchr:

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=349195&r1=349194&r2=349195&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Fri Dec 14 13:11:28 2018
@@ -25,6 +25,7 @@
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DataLayout.h"
@@ -1409,6 +1410,42 @@ static llvm::Value *dumpRecord(CodeGenFu
   return Res;
 }
 
+static bool
+TypeRequiresBuiltinLaunderImp(const ASTContext &Ctx, QualType Ty,
+  llvm::SmallPtrSetImpl &Seen) {
+  if (const auto *Arr = Ctx.getAsArrayType(Ty))
+Ty = Ctx.getBaseElementType(Arr);
+
+  const auto *Record = Ty->getAsCXXRecordDecl();
+  if (!Record)
+return false;
+
+  // We've already checked this type, or are in the process of checking it.
+  if (!Seen.insert(Record).second)
+return false;
+
+  assert(Record->hasDefinition() &&
+ "Incomplete types should already be diagnosed");
+
+  if (Record->isDynamicClass())
+return true;
+
+  for (FieldDecl *F : Record->fields()) {
+if (TypeRequiresBuiltinLaunderImp(Ctx, F->getType(), Seen))
+  return true;
+  }
+  return false;
+}
+
+/// Determine if the specified type requires laundering by checking if it is a
+/// dyn

[PATCH] D55707: Update to SARIF 11-28

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+if (const json::Object *Obj = File.getAsObject()) {

NoQ wrote:
> aaron.ballman wrote:
> > NoQ wrote:
> > > This sounds like `find_if` to me.
> > The problem is: we need the `Index`. It felt a bit weird to have 
> > `llvm::find_if()` (a const operation) also mutating a locally-captured 
> > variable. WDYT?
> The way i see it:
> 
> ```
> auto I = std::find_if(Files.begin(), Files.end(), [&](const json::Value &File 
> {
>   if (const json::Object *Obj = File.getAsObject())
> if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
>   Optional URI = FileLoc->getString("uri");
>   return URI && URI->equals(FileURI);
> }
>   return false;
> });
> 
> if (I == Files.end())
>   Files.push_back(createFile(FE));
> 
> return std::distance(Files.begin(), I);
> ```
> 
> Or pre-compute the index before `push_back` if the container invalidates 
> iterators upon `push_back`.
> 
I dig it! Implemented in r349197.


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

https://reviews.llvm.org/D55707



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

r349197 - Using llvm::find_if() instead of a range-based for loop; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Fri Dec 14 13:14:44 2018
New Revision: 349197

URL: http://llvm.org/viewvc/llvm-project?rev=349197&view=rev
Log:
Using llvm::find_if() instead of a range-based for loop; NFC.

This addresses post-commit review feedback from r349188.

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

Modified: cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp?rev=349197&r1=349196&r2=349197&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp Fri Dec 14 13:14:44 
2018
@@ -123,24 +123,21 @@ static json::Object createFileLocation(c
   std::string FileURI = fileNameToURI(getFileName(FE));
 
   // See if the Files array contains this URI already. If it does not, create
-  // a new file object to add to the array. Calculate the index within the file
-  // location array so it can be stored in the JSON object.
-  unsigned Index = 0;
-  for (const json::Value &File : Files) {
+  // a new file object to add to the array.
+  auto I = llvm::find_if(Files, [&](const json::Value &File) {
 if (const json::Object *Obj = File.getAsObject()) {
   if (const json::Object *FileLoc = Obj->getObject("fileLocation")) {
 Optional URI = FileLoc->getString("uri");
-if (URI && URI->equals(FileURI))
-  break;
+return URI && URI->equals(FileURI);
   }
 }
-++Index;
-  }
+return false;
+  });
 
-  // If we reached the end of the array, then add the file to the list of files
-  // we're tracking; Index then points to the last element of the array. Note
-  // that an empty file lists always causes a file to be added.
-  if (Files.empty() || Index == Files.size())
+  // Calculate the index within the file location array so it can be stored in
+  // the JSON object.
+  auto Index = static_cast(std::distance(Files.begin(), I));
+  if (I == Files.end())
 Files.push_back(createFile(FE));
 
   return json::Object{{"uri", FileURI}, {"fileIndex", Index}};


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

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 178272.
NoQ marked 4 inline comments as done.
NoQ added a comment.

Fxd.


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

https://reviews.llvm.org/D55387

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp

Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -89,6 +89,20 @@
   "weak_ptr",
   };
 
+  // Should we bother tracking the state of the object?
+  bool shouldBeTracked(ObjectKind OK) const {
+// In non-aggressive mode, only warn on use-after-move of local variables
+// (or local rvalue references) and of STL objects. The former is possible
+// because local variables (or local rvalue references) are not tempting
+// their user to re-use the storage. The latter is possible because STL
+// objects are known to end up in a valid but unspecified state after the
+// move and their state-reset methods are also known, which allows us to
+// predict precisely when use-after-move is invalid. In aggressive mode,
+// warn on any use-after-move because the user has intentionally asked us
+// to completely eliminate use-after-move in his code.
+return IsAggressive || OK.Local || OK.STL;
+  }
+
   // Obtains ObjectKind of an object. Because class declaration cannot always
   // be easily obtained from the memory region, it is supplied separately.
   ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const;
@@ -136,8 +150,20 @@
 
 private:
   mutable std::unique_ptr BT;
+
+  // Check if the given form of potential misuse of a given object
+  // should be reported. If so, get it reported. The callback from which
+  // this function was called should immediately return after the call
+  // because this function adds one or two transitions.
+  void modelUse(ProgramStateRef State, const MemRegion *Region,
+const CXXRecordDecl *RD, MisuseKind MK,
+CheckerContext &C) const;
+
+  // Returns the exploded node against which the report was emitted.
+  // The caller *must* add any further transitions against this node.
   ExplodedNode *reportBug(const MemRegion *Region, const CXXRecordDecl *RD,
   CheckerContext &C, MisuseKind MK) const;
+
   bool isInMoveSafeContext(const LocationContext *LC) const;
   bool isStateResetMethod(const CXXMethodDecl *MethodDec) const;
   bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const;
@@ -236,6 +262,25 @@
   return MoveNode;
 }
 
+void MoveChecker::modelUse(ProgramStateRef State, const MemRegion *Region,
+   const CXXRecordDecl *RD, MisuseKind MK,
+   CheckerContext &C) const {
+  assert(!C.isDifferent() && "No transitions should have been made by now");
+  const RegionState *RS = State->get(Region);
+
+  if (!RS || isAnyBaseRegionReported(State, Region)
+  || isInMoveSafeContext(C.getLocationContext())) {
+// Finalize changes made by the caller.
+C.addTransition(State);
+return;
+  }
+
+  ExplodedNode *N = reportBug(Region, RD, C, MK);
+
+  State = State->set(Region, RegionState::getReported());
+  C.addTransition(State, N);
+}
+
 ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
  const CXXRecordDecl *RD,
  CheckerContext &C,
@@ -294,12 +339,10 @@
   if (!MethodDecl)
 return;
 
-  const auto *ConstructorDecl = dyn_cast(MethodDecl);
-
-  const auto *CC = dyn_cast_or_null(&Call);
   // Check if an object became moved-from.
   // Object can become moved from after a call to move assignment operator or
   // move constructor .
+  const auto *ConstructorDecl = dyn_cast(MethodDecl);
   if (ConstructorDecl && !ConstructorDecl->isMoveConstructor())
 return;
 
@@ -310,23 +353,11 @@
   if (!ArgRegion)
 return;
 
-  // In non-aggressive mode, only warn on use-after-move of local variables (or
-  // local rvalue references) and of STL objects. The former is possible because
-  // local variables (or local rvalue references) are not tempting their user to
-  // re-use the storage. The latter is possible because STL objects are known
-  // to end up in a valid but unspecified state after the move and their
-  // state-reset methods are also known, which allows us to predict
-  // precisely when use-after-move is invalid.
-  // In aggressive mode, warn on any use-after-move because the user
-  // has intentionally asked us to completely eliminate use-after-move
-  // in his code.
-  ObjectKind OK = classifyObject(ArgRegion, MethodDecl->getParent());
-  if (!IsAggressive && !OK.Local && !OK.STL)
-return;
-
   // Skip moving the object to itself.
+  const auto *CC = dyn_cast_or_null(&Call);
   if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion)
 return;
+
   if (const auto *IC = dyn_cas

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:265
 
+void MoveChecker::checkUse(ProgramStateRef State, const MemRegion *Region,
+   const CXXRecordDecl *RD, MisuseKind MK,

a_sidorin wrote:
> I think that if the function is named "checkUse()", committing State changes 
> is not what is really expected from it. Should we rename it or change the 
> logic somehow? For example, return true if a report was emitted and add 
> transition in the caller?
Good point. Renamed to `modelUse()` because the `addTransition()` logic becomes 
more complicated in the next patch, so i didn't want to duplicate it on all 
those call sites.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:272
+  if (!RS || isAnyBaseRegionReported(State, Region)
+  || isInMoveSafeContext(C.getLocationContext())) {
+// Finalize changes made by the caller.

a_sidorin wrote:
> This formatting is different from what clang-format does.
This gets overwritten in the next patch anyway. But imho this is more fancy 
than what clang-format does.


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

https://reviews.llvm.org/D55387



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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 178273.
NoQ added a comment.

Rebase.


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

https://reviews.llvm.org/D55388

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue
+// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
+// RUN:  -analyzer-checker debug.ExprInspection
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
+// RUN:  -analyzer-checker debug.ExprInspection
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_warnIfReached();
+
 class B {
 public:
   B() = default;
@@ -810,7 +816,19 @@
 // expected-note@-4{{Object 'P' is moved}}
 // expected-note@-4{{Method called on moved-from object 'P'}}
 #endif
-*P += 1; // FIXME: Should warn that the pointer is null.
+
+// Because that well-defined state is null, dereference is still UB.
+// Note that in aggressive mode we already warned about 'P',
+// so no extra warning is generated.
+*P += 1;
+#ifndef AGGRESSIVE
+// expected-warning@-2{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+// expected-note@-14{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
+// expected-note@-4{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+#endif
+
+// The program should have crashed by now.
+clang_analyzer_warnIfReached(); // no-warning
   }
 };
 
@@ -827,3 +845,9 @@
   P.get(); // expected-warning{{Method called on moved-from object 'P'}}
// expected-note@-1{{Method called on moved-from object 'P'}}
 }
+
+void localUniquePtrWithArrow(std::unique_ptr P) {
+  std::unique_ptr Q = std::move(P); // expected-note{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
+  P->foo(); // expected-warning{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+// expected-note@-1{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
+}
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -61,19 +61,35 @@
   const char *NL, const char *Sep) const override;
 
 private:
-  enum MisuseKind { MK_FunCall, MK_Copy, MK_Move };
+  enum MisuseKind { MK_FunCall, MK_Copy, MK_Move, MK_Dereference };
+  enum StdObjectKind { SK_NonStd, SK_Unsafe, SK_Safe, SK_SmartPtr };
+
+  static bool misuseCausesCrash(MisuseKind MK) {
+return MK == MK_Dereference;
+  }
 
   struct ObjectKind {
-bool Local : 1; // Is this a local variable or a local rvalue reference?
-bool STL : 1; // Is this an object of a standard type?
+// Is this a local variable or a local rvalue reference?
+bool IsLocal : 1;
+// Is this an STL object? If so, of what kind?
+StdObjectKind StdKind : 2;
+  };
+
+  // STL smart pointers are automatically re-initialized to null when moved
+  // from. So we can't warn on many methods, but we can warn when it is
+  // dereferenced, which is UB even if the resulting lvalue never gets read.
+  const llvm::StringSet<> StdSmartPtrClasses = {
+  "shared_ptr",
+  "unique_ptr",
+  "weak_ptr",
   };
 
   // Not all of these are entirely move-safe, but they do provide *some*
   // guarantees, and it means that somebody is using them after move
   // in a valid manner.
-  // T

[PATCH] D55698: [MinGW] Produce a vtable and RTTI for dllexported classes without a key function

[Martin Storsjö via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaDeclCXX.cpp:5754-5756
+  if (ClassExported &&
+  Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+MarkVTableUsed(Class->getLocation(), Class, true);

rnk wrote:
> This may be too early, you can get into situations like this if you start 
> emitting the vtable (which will emit inline methods) before we get to the end 
> of the outermost class. See this bug for example:
> https://bugs.llvm.org/show_bug.cgi?id=40006
> 
> Maybe if you have a dllexported nested class with a virtual method that 
> references the constructor of the outer class which has a late-parsed member 
> initializer... you can get things to go wrong as in the bug above.
> 
> I think the fix will be to touch the vtable when we process delayed 
> dllexported classes from the list just above this line.
Ok, will upload a new version of the patch.


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

https://reviews.llvm.org/D55698



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

[PATCH] D55698: [MinGW] Produce a vtable and RTTI for dllexported classes without a key function

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 178275.
mstorsjo added a comment.

Moved the code to work on DelayedDllExportClasses instead, as suggested, which 
still works for the testcase. (I've yet to test that approach on a larger 
codebase though.)


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

https://reviews.llvm.org/D55698

Files:
  lib/Sema/SemaDeclCXX.cpp
  test/CodeGenCXX/dllexport-missing-key.cpp


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s | 
FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5493,6 +5493,9 @@
 // declaration.
 return;
 
+  if (S.Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+S.MarkVTableUsed(Class->getLocation(), Class, true);
+
   for (Decl *Member : Class->decls()) {
 // Defined static variables that are members of an exported base
 // class must be marked export too.


Index: test/CodeGenCXX/dllexport-missing-key.cpp
===
--- /dev/null
+++ test/CodeGenCXX/dllexport-missing-key.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++11 -o - %s | FileCheck --check-prefix=GNU %s
+
+class __declspec(dllexport) QAbstractLayoutStyleInfo {
+public:
+  QAbstractLayoutStyleInfo() : m_isWindow(false) {}
+  virtual ~QAbstractLayoutStyleInfo() {}
+
+  virtual bool hasChangedCore() const { return false; }
+
+  virtual void invalidate() {}
+
+  virtual double windowMargin(bool orientation) const = 0;
+
+  bool isWindow() const { return m_isWindow; }
+
+protected:
+  bool m_isWindow;
+};
+
+// GNU-DAG: @_ZTV24QAbstractLayoutStyleInfo = weak_odr dso_local dllexport
+// GNU-DAG: @_ZTS24QAbstractLayoutStyleInfo = linkonce_odr
+// GNU-DAG: @_ZTI24QAbstractLayoutStyleInfo = linkonce_odr
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -5493,6 +5493,9 @@
 // declaration.
 return;
 
+  if (S.Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+S.MarkVTableUsed(Class->getLocation(), Class, true);
+
   for (Decl *Member : Class->decls()) {
 // Defined static variables that are members of an exported base
 // class must be marked export too.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: lib/AST/MicrosoftMangle.cpp:1806-1836
+Extra.mangleSourceName("AS_");
+Extra.mangleIntegerLiteral(llvm::APSInt::getUnsigned(TargetAS),
+   /*IsBoolean*/ false);
+  } else {
+switch (AS) {
+default:
+  llvm_unreachable("Not a language specific address space");

rnk wrote:
> majnemer wrote:
> > Don't these need to be _AS_Foobar to avoid collisions with normal code?
> I think we're in __clang::, so it's OK? In any case, it's what's done for 
> Itanium, IIUC. It's also not like we have to worry about conflicts with user 
> macros.
Right, we're in __clang (thus cannot conflict), and cannot be replaced with 
macros or anything (since this is a mangling name), so I opted to save the char 
instead.  

However, I'll make it _AS_CU just cause.



Comment at: test/CodeGenOpenCL/address-spaces-mangling.cl:7
+// RUN: %clang_cc1 %s -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN10 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -ffake-address-space-map 
-faddress-space-map-mangling=yes -triple x86_64-windows-pc -emit-llvm -o - | 
FileCheck -check-prefixes=MS_ASMANG,MS_ASMAN20 %s
+// RUN: %clang_cc1 %s -ffake-address-space-map -faddress-space-map-mangling=no 
-triple x86_64-windows-pc -emit-llvm -o - | FileCheck 
-check-prefixes=MS_NOASMANG,MS_NOASMAN10 %s

rnk wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > rnk wrote:
> > > > I would replace `-triple x86_64-windows-pc` here and everywhere with 
> > > > `-triple x86_64-windows-itanium` (or gnu instead of itanium) to test 
> > > > the Itanium mangling on Windows. I don't think `x86_64-windows-pc` 
> > > > parses into a real triple. After all, "pc" is parsed as the vendor, 
> > > > which is supposed to be second.
> > > Woops! I didn't mean to check this file in.  It has some other nasty 
> > > problems that need to be fixed up, so I tested this feature in the 
> > > mangle-address-space.  I'll still do this above.
> > @rnk I tried this, but it changes off of the MicrosoftMangler and switches 
> > back to the Itanium mangling.  Is there a better triple for testing 
> > microsoft mangling?
> Tacking on "-msvc" or "-itanium" or "-gnu" is the more explicit way to the 
> C++ ABI. I guess I was confused, it looks like this file expects Itanium 
> manglings from the -pc triple.
Ah, yeah, I should have figured that out.  I was originally looking to do the 
opencl-c++ test in this file but it has some bigger problems that cause issues 
(so I never finished it).

Somehow it stayed in my workspace when I stashed.

I'll update this whole patch as soon as my build passes.


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

https://reviews.llvm.org/D55715



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

[PATCH] D55719: [OpenMP] parsing and sema support for 'close' map-type-modifier

[Ahsan Saghir via Phabricator via cfe-commits]

saghir created this revision.
saghir added reviewers: ABataev, kkwli0, Hahnfeld, RaviNarayanaswamy, mikerice, 
hfinkel, gtbercea.
saghir added a project: clang.
Herald added subscribers: cfe-commits, guansong.

A map clause with the close map-type-modifier is a hint to prefer that the 
variables are mapped using a copy into faster memory.

Reference: TR7 Section 2.15.8 and 2.22.7.1


Repository:
  rC Clang

https://reviews.llvm.org/D55719

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_data_ast_print.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_parallel_map_messages.cpp
  clang/test/OpenMP/target_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -454,7 +454,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always'}} expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always' or 'close'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
@@ -529,7 +529,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always'}} expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always' or 'close'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
Index: clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
+++ clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
@@ -163,7 +163,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always'}} expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always' or 'close'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
@@ -271,7 +271,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always'}} expected-error {{incorrect map type, expected one of 'to', 'from', 'tofrom', 'alloc', 'release', or 'delete'}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always' or 'close'}}

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 178282.
erichkeane added a comment.

Should catch me up on all comments except @zturner's llvm-undname feature 
request :)


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

https://reviews.llvm.org/D55715

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenCXX/mangle-address-space.cpp

Index: test/CodeGenCXX/mangle-address-space.cpp
===
--- test/CodeGenCXX/mangle-address-space.cpp
+++ test/CodeGenCXX/mangle-address-space.cpp
@@ -1,15 +1,64 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s --check-prefixes=CHECK,CHECKNOOCL
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-msvc -o - %s | FileCheck %s --check-prefixes=WIN,WINNOOCL
+// RUN: %clang_cc1 -cl-std=c++ -emit-llvm -triple %itanium_abi_triple -o - %s | FileCheck %s --check-prefixes=CHECK,CHECKOCL
+// RUN: %clang_cc1 -cl-std=c++ -emit-llvm -triple x86_64-windows-msvc -o - %s | FileCheck %s --check-prefixes=WIN,WINOCL
 
-// CHECK-LABEL: define {{.*}}void @_Z2f0Pc
+// CHECKNOOCL-LABEL: define {{.*}}void @_Z2f0Pc
+// WINNOOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAD@Z"
+// CHECKOCL-LABEL: define {{.*}}void @_Z2f0PU9CLgenericc
+// WINOCL-LABEL: define {{.*}}void @"?f0@@YAXPEAU?$_ASCLgeneric@$$CAD@__clang@@@Z" 
 void f0(char *p) { }
 // CHECK-LABEL: define {{.*}}void @_Z2f0PU3AS1c
+// WIN-LABEL: define {{.*}}void @"?f0@@YAXPEAU?$_AS@$00$$CAD@__clang@@@Z"
 void f0(char __attribute__((address_space(1))) *p) { }
 
 struct OpaqueType;
 typedef OpaqueType __attribute__((address_space(100))) * OpaqueTypePtr;
 
 // CHECK-LABEL: define {{.*}}void @_Z2f0PU5AS10010OpaqueType
+// WIN-LABEL: define {{.*}}void @"?f0@@YAXPEAU?$_AS@$0GE@$$CAUOpaqueType@@@__clang@@@Z"
 void f0(OpaqueTypePtr) { }
 
 // CHECK-LABEL: define {{.*}}void @_Z2f1PU3AS1Kc
-void f1(char __attribute__((address_space(1))) const *p) {}
\ No newline at end of file
+// WIN-LABEL: define {{.*}}void @"?f1@@YAXPEAU?$_AS@$00$$CBD@__clang@@@Z"
+void f1(char __attribute__((address_space(1))) const *p) {}
+
+// Ensure we can do return values, which change in MS mode.
+// CHECK-LABEL: define {{.*}}float addrspace(1)* @_Z2f1PU3AS2Kc
+// WIN-LABEL: define {{.*}}float addrspace(1)* @"?f1@@YAPEAU?$_AS@$00$$CAM@__clang@@PEAU?$_AS@$01$$CBD@2@@Z"
+__attribute__((address_space(1))) float *f1(char __attribute__((address_space(2))) const *p) { return 0;}
+
+#if !defined(__OPENCL_CPP_VERSION__)
+// Return value of address space without a pointer is invalid in opencl.
+// Ensure we skip return values, since non-pointers aren't supposed to have an AS.
+// CHECKNOOCL-LABEL: define {{.*}}float @_Z2f2PU3AS2Kc
+// WINNOOCL-LABEL: define {{.*}}float @"?f2@@YA?AMQEAU?$_AS@$01$$CBD@__clang@@@Z"
+__attribute__((address_space(1))) float f2(char __attribute__((address_space(2))) const * const p) { return 0;}
+#endif
+
+#ifdef __OPENCL_CPP_VERSION__
+// CHECKOCL-LABEL: define {{.*}}void @_Z6ocl_f0PU9CLprivatec
+// WINOCL-LABEL: define {{.*}}void @"?ocl_f0@@YAXPEAU?$_ASCLprivate@$$CAD@__clang@@@Z"
+void ocl_f0(char __private *p) { }
+
+struct ocl_OpaqueType;
+typedef ocl_OpaqueType __global * ocl_OpaqueTypePtr;
+
+// CHECKOCL-LABEL: define {{.*}}void @_Z6ocl_f0PU8CLglobal14ocl_OpaqueType
+// WINOCL-LABEL: define {{.*}}void @"?ocl_f0@@YAXPEAU?$_ASCLglobal@$$CAUocl_OpaqueType@@@__clang@@@Z"
+void ocl_f0(ocl_OpaqueTypePtr) { }
+
+// CHECKOCL-LABEL: define {{.*}}void @_Z6ocl_f1PU10CLconstantKc
+// WINOCL-LABEL: define {{.*}}void @"?ocl_f1@@YAXPEAU?$_ASCLconstant@$$CBD@__clang@@@Z"
+void ocl_f1(char __constant const *p) {}
+
+// Ensure we can do return values, which change in MS mode.
+// CHECKOCL-LABEL: define {{.*}}float* @_Z6ocl_f1PU9CLgenericKc
+// WINOCL-LABEL: define {{.*}}float* @"?ocl_f1@@YAPEAU?$_ASCLconstant@$$CAM@__clang@@PEAU?$_ASCLgeneric@$$CBD@2@@Z"
+__constant float *ocl_f1(char __generic const *p) { return 0;}
+
+// Ensure we skip return values, since non-pointers aren't supposed to have an AS.
+// CHECKOCL-LABEL: define {{.*}}float* @_Z6ocl_f2PU9CLgenericKc
+// WINOCL-LABEL: define {{.*}}float* @"?ocl_f2@@YAPEAU?$_ASCLgeneric@$$CAM@__clang@@QEAU?$_ASCLgeneric@$$CBD@2@@Z"
+__generic float *ocl_f2(__generic char const * const p) { return 0;}
+#endif
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -311,6 +311,7 @@
   void mangleTagTypeKind(TagTypeKind TK);
   void mangleArtificialTagType(TagTypeKind TK, StringRef UnqualifiedName,
   ArrayRef NestedNames = None);
+  void mangleAddressSpaceType(QualType T, Qualifiers Quals, SourceRange Range);
   void mangleType(QualType T, SourceRange Range,
   QualifierMangleMode QMM = QMM_Mangle);
   void mangleFunctionType(const FunctionType *T,
@@ -1777,12 +1778,77 @@
   }
 }
 
+void Micros

[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore

[Artem Dergachev via Phabricator via cfe-commits]

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

Thanks! Neat test.

Can we now `assert(LocationE->isGLValue())` in `evalStore()`? What about 
`evalLoad()`?

Also, were no other checkers affected? Like, will null pointer dereference 
checker now warn upon something like `(*0)++` or `++(*0)`?

You can commit the patch, but i would also love to see these questions 
investigated.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55701



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