[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41005



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

[PATCH] D53866: [Preamble] Stop circular inclusion of main file when building preamble

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866



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

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I see, alright.




Comment at: lib/CodeGen/CGObjC.cpp:3215
+if (be->getBlockDecl()->canAvoidCopyToHeap())
+  return value;
+

Can this just be a case in `ARCRetainExprEmitter`?  I think that should subsume 
both this and the logic in `EmitARCStoreStrong`.



Comment at: lib/Sema/SemaDecl.cpp:11262
+if (auto *E = dyn_cast(Init))
+  if (auto *BE = dyn_cast(E->getSubExpr()))
+if (VDecl->hasLocalStorage())

I won't insist that you look through arbitrary value-propagating expressions 
like commas and conditionals, but please do at least call `IgnoreParens()` here 
and in the assignment case.



Comment at: lib/Sema/SemaExpr.cpp:12466
+if (VD->hasLocalStorage() && getCurScope()->isDeclScope(VD))
+  BE->getBlockDecl()->setCanAvoidCopyToHeap();
 break;

Please check for a block-expression RHS first, it is far more likely to 
short-circuit this check than anything else.

Also, I think the right place for this check is up with the calls to 
`DiagnoseSelfAssignment` and `DiagnoseSelfMove`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 187904.
ahatanak added a comment.

Remove the code that is needed to check whether the address of a local variable 
is taken.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  lib/AST/Decl.cpp
  lib/CodeGen/CGObjC.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/arc-block-copy-escape.m
  test/CodeGenObjC/arc-blocks.m
  test/CodeGenObjCXX/arc-blocks.mm
  test/PCH/arc-blocks.mm

Index: test/PCH/arc-blocks.mm
===
--- /dev/null
+++ test/PCH/arc-blocks.mm
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -fobjc-arc -fblocks -std=c++1y -emit-pch %s -o %t
+// RUN: %clang_cc1 -fobjc-arc -fblocks -std=c++1y -include-pch %t -emit-llvm -o - %s | FileCheck %s
+
+#ifndef HEADER_INCLUDED
+#define HEADER_INCLUDED
+
+namespace test_block_retain {
+  typedef void (^BlockTy)();
+  void foo1(id);
+
+  inline void initialization(id a) {
+// Call to @llvm.objc.retainBlock isn't needed.
+BlockTy b0 = ^{ foo1(a); };
+b0();
+  }
+
+  inline void assignmentConditional(id a, bool c) {
+BlockTy b0;
+if (c)
+  // @llvm.objc.retainBlock is called since 'b0' is declared in the outer scope.
+  b0 = ^{ foo1(a); };
+b0();
+  }
+}
+
+#else
+
+// CHECK: %[[STRUCT_BLOCK_DESCRIPTOR:.*]] = type { i64, i64 }
+
+namespace test_block_retain {
+// CHECK-LABEL: define linkonce_odr void @_ZN17test_block_retain14initializationEP11objc_object(
+// CHECK-NOT: call i8* @llvm.objc.retainBlock(
+
+  void test_initialization(id a) {
+initialization(a);
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain26test_assignmentConditionalEP11objc_objectb(
+// CHECK: %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
+// CHECK: %[[V4:.*]] = bitcast <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]] to void ()*
+// CHECK: %[[V5:.*]] = bitcast void ()* %[[V4]] to i8*
+// CHECK: call i8* @llvm.objc.retainBlock(i8* %[[V5]])
+
+  void test_assignmentConditional(id a, bool c) {
+assignmentConditional(a, c);
+  }
+}
+
+#endif
Index: test/CodeGenObjCXX/arc-blocks.mm
===
--- test/CodeGenObjCXX/arc-blocks.mm
+++ test/CodeGenObjCXX/arc-blocks.mm
@@ -201,3 +201,121 @@
   ^{ (void)t0; (void)t1; (void)t2; (void)t3; (void)t4; (void)t5; };
 }
 }
+
+// Test that calls to @llvm.objc.retainBlock aren't emitted in some cases.
+
+namespace test_block_retain {
+  typedef void (^BlockTy)();
+
+  void foo1(id);
+
+// CHECK-LABEL: define void @_ZN17test_block_retain14initializationEP11objc_object(
+// CHECK-NOT: @llvm.objc.retainBlock(
+  void initialization(id a) {
+BlockTy b0 = ^{ foo1(a); };
+b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain20initializationStaticEP11objc_object(
+// CHECK: @llvm.objc.retainBlock(
+  void initializationStatic(id a) {
+static BlockTy b0 = ^{ foo1(a); };
+b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain15initialization2EP11objc_object
+// CHECK: %[[B0:.*]] = alloca void ()*, align 8
+// CHECK: %[[B1:.*]] = alloca void ()*, align 8
+// CHECK: load void ()*, void ()** %[[B0]], align 8
+// CHECK-NOT: @llvm.objc.retainBlock
+// CHECK: %[[V9:.*]] = load void ()*, void ()** %[[B0]], align 8
+// CHECK: %[[V10:.*]] = bitcast void ()* %[[V9]] to i8*
+// CHECK: %[[V11:.*]] = call i8* @llvm.objc.retainBlock(i8* %[[V10]])
+// CHECK: %[[V12:.*]] = bitcast i8* %[[V11]] to void ()*
+// CHECK: store void ()* %[[V12]], void ()** %[[B1]], align 8
+  void initialization2(id a) {
+BlockTy b0 = ^{ foo1(a); };
+b0();
+BlockTy b1 = b0; // can't optimize this yet.
+b1();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain10assignmentEP11objc_object(
+// CHECK-NOT: @llvm.objc.retainBlock(
+  void assignment(id a) {
+BlockTy b0;
+b0 = ^{ foo1(a); };
+b0();
+b0 = ^{ foo1(a); };
+b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain16assignmentStaticEP11objc_object(
+// CHECK: @llvm.objc.retainBlock(
+  void assignmentStatic(id a) {
+static BlockTy b0;
+b0 = ^{ foo1(a); };
+b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain21assignmentConditionalEP11objc_objectb(
+// CHECK: @llvm.objc.retainBlock(
+  void assignmentConditional(id a, bool c) {
+BlockTy b0;
+if (c)
+  // can't optimize this since 'b0' is declared in the outer scope.
+  b0 = ^{ foo1(a); };
+b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain11assignment2EP11objc_object(
+// CHECK: %[[B0:.*]] = alloca void ()*, align 8
+// CHECK: %[[B1:.*]] = alloca void ()*, align 8
+// CHECK-NOT: @llvm.objc.retainBlock
+// CHECK: store void ()* null, void ()** %[[B1]], align 

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

For assignment, the optimization isn't performed if the local variable isn't 
declared in the scope that introduced the block (see the code and comment in 
SemaExpr.cpp).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle updated this revision to Diff 187902.
malaperle added a comment.

Remove Markdown support.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250

Files:
  clangd/XRefs.cpp
  unittests/clangd/XRefsTests.cpp


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -648,7 +648,25 @@
 #define MACRO 2
 #undef macro
   )cpp",
-  "#define MACRO",
+  "#define MACRO 1",
+  },
+  {
+  R"cpp(// Macro
+#define MACRO 0
+#define MACRO2 ^MACRO
+  )cpp",
+  "#define MACRO 0",
+  },
+  {
+  R"cpp(// Macro
+#define MACRO {\
+  return 0;\
+}
+int main() ^MACRO
+  )cpp",
+  R"cpp(#define MACRO {\
+  return 0;\
+})cpp",
   },
   {
   R"cpp(// Forward class declaration
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -537,13 +537,30 @@
   return H;
 }
 
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
-  Hover H;
-
-  H.contents.value = "#define ";
-  H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST ) {
+  SourceManager  = AST.getASTContext().getSourceManager();
+  std::string Definition = Decl.Name;
+
+  // Try to get the full definition, not just the name
+  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+  SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+  if (EndLoc.isValid()) {
+EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+AST.getASTContext().getLangOpts());
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), );
+if (!Invalid) {
+  unsigned StartOffset = SM.getFileOffset(StartLoc);
+  unsigned EndOffset = SM.getFileOffset(EndLoc);
+  if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+}
+  }
 
+  Hover H;
+  H.contents.kind = MarkupKind::PlainText;
+  H.contents.value = "#define " + Definition;
   return H;
 }
 
@@ -667,7 +684,7 @@
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   if (!Symbols.Macros.empty())
-return getHoverContents(Symbols.Macros[0].Name);
+return getHoverContents(Symbols.Macros[0], AST);
 
   if (!Symbols.Decls.empty())
 return getHoverContents(Symbols.Decls[0].D);


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -648,7 +648,25 @@
 #define MACRO 2
 #undef macro
   )cpp",
-  "#define MACRO",
+  "#define MACRO 1",
+  },
+  {
+  R"cpp(// Macro
+#define MACRO 0
+#define MACRO2 ^MACRO
+  )cpp",
+  "#define MACRO 0",
+  },
+  {
+  R"cpp(// Macro
+#define MACRO {\
+  return 0;\
+}
+int main() ^MACRO
+  )cpp",
+  R"cpp(#define MACRO {\
+  return 0;\
+})cpp",
   },
   {
   R"cpp(// Forward class declaration
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -537,13 +537,30 @@
   return H;
 }
 
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
-  Hover H;
-
-  H.contents.value = "#define ";
-  H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST ) {
+  SourceManager  = AST.getASTContext().getSourceManager();
+  std::string Definition = Decl.Name;
+
+  // Try to get the full definition, not just the name
+  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+  SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+  if (EndLoc.isValid()) {
+EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+AST.getASTContext().getLangOpts());
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), );
+if (!Invalid) {
+  unsigned StartOffset = SM.getFileOffset(StartLoc);
+  unsigned EndOffset = SM.getFileOffset(EndLoc);
+  if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+}
+  }
 
+  Hover H;
+  

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

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



Comment at: clangd/ClangdLSPServer.cpp:816
+// If the client supports Markdown, convert from plaintext here.
+if (*H && HoverSupportsMarkdown) {
+  (*H)->contents.kind = MarkupKind::Markdown;

ilya-biryukov wrote:
> malaperle wrote:
> > I don't know if you meant plain text as non-language-specific markdown or 
> > no markdown at all. In VSCode, non-markdown for multiline macros looks bad 
> > because the newlines are ignored.
> Did not know about that, thanks for pointing it out.
> It does not ignore double newlines though (that's what we use in 
> declarations). I suspect it treats plaintext as markdown, but can't be sure.
> 
> In any case, converting to a markdown code block here looks incredibly hacky 
> and shaky.
> Could we use double-newlines for line breaks in our implementation instead?
> 
> This aligns with what we did before this patch for declarations.
> If that doesn't work, breaking the multi-line macros in VSCode looks ok, this 
> really feels like a bug in VSCode.
> In any case, converting to a markdown code block here looks incredibly hacky 
> and shaky.

I'm not sure why? ClangdLSPServer knows about client capabilities so it made 
sense to me to convert it there. Either way, it sounds like I should just 
remove "HoverSupportsMarkdown" altogether if we're not going to use Markdown.

> Could we use double-newlines for line breaks in our implementation instead?

It looks odd. When I double them in the hover contents, the lines are displayed 
as doubled and some extra backslashes are added on all non-empty lines except 
the first line. Single new lines and correct backslashes are displayed 
correctly when Markdown is used. We can just display multiline line macros as 
one line until we want to handle Markdown (which is how exactly?). 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250



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

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The correctness condition here is solely that we cannot allow the block literal 
to go out of scope before the variable that it is assigned to.  (Local block 
literals with captures have lifetimes like C compound literals: until the end 
of the enclosing block, rather than the end of the full-expression.)  So doing 
this blindly for assignments is problematic because you'd need to reason about 
relative lifetimes, but doing it for initialization should always be fine.  I 
don't see any reason why address-taken-ness would matter as long as that 
condition holds.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I can understand Zach's position here, but LLDB has historically never 
conformed to the general LLVM naming or other conventions due to its heritage.  
It should not be taken as precedent that the rest of the project should follow.

In any case, I think that it is best for this discussion to take place on the 
llvm-dev list where it is likely to get the most visibility.  Would you mind 
moving comments and discussions there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D58164: Block+lambda: allow reference capture

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I agree.  There should just never be a copy expression if the capture is of 
reference type, and there should therefore be no need to special-case that in 
IRGen.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58164



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

r354651 - revert r354615: [HIP] change kernel stub name

[Yaxun Liu via cfe-commits]

Author: yaxunl
Date: Thu Feb 21 20:20:12 2019
New Revision: 354651

URL: http://llvm.org/viewvc/llvm-project?rev=354651=rev
Log:
revert r354615: [HIP] change kernel stub name

It caused regressions.

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

Modified:
cfe/trunk/lib/CodeGen/CGCUDANV.cpp
cfe/trunk/test/CodeGenCUDA/device-stub.cu

Modified: cfe/trunk/lib/CodeGen/CGCUDANV.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCUDANV.cpp?rev=354651=354650=354651=diff
==
--- cfe/trunk/lib/CodeGen/CGCUDANV.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCUDANV.cpp Thu Feb 21 20:20:12 2019
@@ -227,12 +227,6 @@ void CGNVCUDARuntime::emitDeviceStub(Cod
 emitDeviceStubBodyNew(CGF, Args);
   else
 emitDeviceStubBodyLegacy(CGF, Args);
-
-  // Postfix kernel stub names with .stub to differentiate them from kernel
-  // names in device binaries. This is to facilitate the debugger to find
-  // the correct symbols for kernels in the device binary.
-  if (CGF.getLangOpts().HIP)
-CGF.CurFn->setName(CGF.CurFn->getName() + ".stub");
 }
 
 // CUDA 9.0+ uses new way to launch kernels. Parameters are packed in a local

Modified: cfe/trunk/test/CodeGenCUDA/device-stub.cu
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCUDA/device-stub.cu?rev=354651=354650=354651=diff
==
--- cfe/trunk/test/CodeGenCUDA/device-stub.cu (original)
+++ cfe/trunk/test/CodeGenCUDA/device-stub.cu Thu Feb 21 20:20:12 2019
@@ -145,8 +145,7 @@ void use_pointers() {
 // Test that we build the correct number of calls to cudaSetupArgument followed
 // by a call to cudaLaunch.
 
-// CUDA-LABEL: define{{.*}}kernelfunc
-// HIP-LABEL: define{{.*}}@_Z10kernelfunciii.stub
+// LNX: define{{.*}}kernelfunc
 
 // New launch sequence stores arguments into local buffer and passes array of
 // pointers to them directly to cudaLaunchKernel


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

[PATCH] D58537: lib/Header: Simplify CMakeLists.txt

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai accepted this revision.
smeenai added reviewers: beanz, phosek.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM, though I generally prefer functions to macros.




Comment at: clang/lib/Headers/CMakeLists.txt:127
 
-# Generate arm_neon.h
-clang_tablegen(arm_neon.h -gen-arm-neon
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_neon.td)
-# Generate arm_fp16.h
-clang_tablegen(arm_fp16.h -gen-arm-fp16
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_fp16.td)
-
-set(out_files)
-foreach( f ${files} ${cuda_wrapper_files} )
-  set( src ${CMAKE_CURRENT_SOURCE_DIR}/${f} )
-  set( dst ${output_dir}/${f} )
+macro(copy_header_to_output_dir src_dir file)
+  set(src ${src_dir}/${file})

I'd prefer this be converted to a function and the append to  `out_files` be 
handled explicitly via `PARENT_SCOPE` (you could either hardcode the name or 
take in a parameter for the name of the list to append to). Macros pollute 
variables and their parameters behave weirdly.



Comment at: clang/lib/Headers/CMakeLists.txt:137
+
+macro(clang_generate_header td_option td_file out_file)
+  clang_tablegen(${out_file} ${td_option}

This one can trivially be a function instead of a macro, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58537



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

[PATCH] D16351: [FIX] Bug 25404 - Crash on typedef in OpenCL 2.0

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.
Herald added subscribers: jdoerfert, jfb, yaxunl, wdng.

Is this still needed? The bug is still open


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

https://reviews.llvm.org/D16351



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

[PATCH] D58537: lib/Header: Simplify CMakeLists.txt

[Tom Stellard via Phabricator via cfe-commits]

tstellar created this revision.
tstellar added reviewers: chandlerc, smeenai, mgorny.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Replace cut and pasted code with cmake macros and reduce the number of
install commands.  This fixes an issue where the headers were being
installed twice.

This clean up should also make future modifications easier, like
adding a cmake option to install header files into a custom resource
directory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58537

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -124,59 +124,48 @@
 
 set(output_dir ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/include)
 
-# Generate arm_neon.h
-clang_tablegen(arm_neon.h -gen-arm-neon
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_neon.td)
-# Generate arm_fp16.h
-clang_tablegen(arm_fp16.h -gen-arm-fp16
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_fp16.td)
-
-set(out_files)
-foreach( f ${files} ${cuda_wrapper_files} )
-  set( src ${CMAKE_CURRENT_SOURCE_DIR}/${f} )
-  set( dst ${output_dir}/${f} )
+macro(copy_header_to_output_dir src_dir file)
+  set(src ${src_dir}/${file})
+  set(dst ${output_dir}/${file})
   add_custom_command(OUTPUT ${dst}
 DEPENDS ${src}
 COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
-COMMENT "Copying clang's ${f}...")
+COMMENT "Copying clang's ${file}...")
   list(APPEND out_files ${dst})
+endmacro(copy_header_to_output_dir)
+
+macro(clang_generate_header td_option td_file out_file)
+  clang_tablegen(${out_file} ${td_option}
+  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
+  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/${td_file})
+
+  copy_header_to_output_dir(${CMAKE_CURRENT_BINARY_DIR} ${out_file})
+endmacro(clang_generate_header)
+
+
+# Copy header files from the source directory to the build directory
+set(out_files)
+foreach( f ${files} ${cuda_wrapper_files} )
+  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR} ${f})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h
-  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
-  COMMAND ${CMAKE_COMMAND} -E copy_if_different 
${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
-  COMMENT "Copying clang's arm_neon.h...")
-list(APPEND out_files ${output_dir}/arm_neon.h)
-add_custom_command(OUTPUT ${output_dir}/arm_fp16.h
-  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h
-  COMMAND ${CMAKE_COMMAND} -E copy_if_different 
${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h ${output_dir}/arm_fp16.h
-  COMMENT "Copying clang's arm_fp16.h...")
-list(APPEND out_files ${output_dir}/arm_fp16.h)
+# Generate header files and copy them to the build directory
+# Generate arm_neon.h
+clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
+# Generate arm_fp16.h
+clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
 set_target_properties(clang-headers PROPERTIES
   FOLDER "Misc"
   RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
-install(
-  FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include)
-
-install(
-  FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include)
+set(header_install_dir lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
 
 install(
-  FILES ${cuda_wrapper_files}
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION 
lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include/cuda_wrappers)
+  DIRECTORY ${output_dir}
+  DESTINATION ${header_install_dir}
+  COMPONENT clang-headers)
 
 if (NOT LLVM_ENABLE_IDE)
   add_llvm_install_targets(install-clang-headers


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -124,59 +124,48 @@
 
 set(output_dir ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/include)
 
-# Generate arm_neon.h
-clang_tablegen(arm_neon.h -gen-arm-neon
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_neon.td)
-# Generate arm_fp16.h
-clang_tablegen(arm_fp16.h -gen-arm-fp16
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_fp16.td)
-
-set(out_files)
-foreach( f ${files} ${cuda_wrapper_files} )
-  set( src ${CMAKE_CURRENT_SOURCE_DIR}/${f} )
-  set( dst ${output_dir}/${f} )

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Jacob Keller via Phabricator via cfe-commits]

jacob-keller added a comment.

In regards to whether clang-format should support this feature, I think it's 
pretty clear that a large number of users want it to. The tool already supports 
formatting various other definitions in a similar manner, including variables 
and struct members. So I don't see how a similar features which aligns 
consecutive macros is something which doesn't make sense.

Additionally, because the formatting will *undo* such formatting if done 
manually, it means that the existing source code formatting this way cannot be 
handled via clang-format. In my case, it essentially means that I cannot use 
clang-format to enforce the style guidelines, as the macros definitions are 
aligned.

Additionally, aligning a series of macros makes sense because it helps improve 
readability, and is thus something that I'd rather not lose if I were to switch 
to using clang-format without the feature.


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

https://reviews.llvm.org/D28462



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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: test/Sema/inline-asm-x86-constraint.c:2
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1  %s -o %t
+typedef unsigned char __attribute__((vector_size(8))) _m64u8;

Test belongs in test/CodeGen.  Please use -emit-llvm and FileCheck the IR, 
instead of checking how the backend handles it.  Shouldn't require 
x86-registered-target (the x86 frontend bits are always compiled in).

Actually, you could probably stick the function into the existing test 
test/CodeGen/asm-inout.c, which has another similar test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56990



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

[PATCH] D57835: Fix -ftime-report with -x ir

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.
Herald added a subscriber: jdoerfert.

ping


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

https://reviews.llvm.org/D57835



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

[PATCH] D18360: Add AIX Target/ToolChain to Clang Driver

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D18360#1406455 , @apaprocki wrote:

> @hubert.reinterpretcast Yes, this patch is available under the new license.


Thank you, @apaprocki. We will be splitting this into updated and more granular 
patches.


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

https://reviews.llvm.org/D18360



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

Buildbot numbers for the week of 02/10/2019 - 02/16/2019

[Galina Kistanova via cfe-commits]

Hello everyone,

Below are some buildbot numbers for the last week of 02/10/2019 -
02/16/2019.

Please see the same data in attached csv files:

The longest time each builder was red during the week;
"Status change ratio" by active builder (percent of builds that changed the
builder status from greed to red or from red to green);
Count of commits by project;
Number of completed builds, failed builds and average build time for
successful builds per active builder;
Average waiting time for a revision to get build result per active builder
(response time).

Thanks

Galina


The longest time each builder was red during the week:
   buildername| was_red
--+-
 clang-cmake-aarch64-lld  | 35:30:54
 clang-cmake-aarch64-full | 34:08:37
 clang-ppc64be-linux-lnt  | 34:03:46
 sanitizer-ppc64le-linux  | 27:34:23
 llvm-clang-x86_64-expensive-checks-win   | 26:56:10
 sanitizer-ppc64be-linux  | 26:40:50
 clang-ppc64le-linux-lnt  | 26:39:59
 clang-ppc64le-linux-multistage   | 26:32:10
 clang-ppc64le-linux  | 26:29:31
 clang-ppc64be-linux  | 26:05:11
 lldb-amd64-ninja-netbsd8 | 23:55:26
 clang-ppc64be-linux-multistage   | 23:35:48
 clang-with-thin-lto-ubuntu   | 22:11:42
 clang-with-lto-ubuntu| 21:50:36
 clang-x64-windows-msvc   | 21:46:51
 clang-lld-x86_64-2stage  | 21:16:21
 llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast | 21:00:48
 clang-cmake-x86_64-avx2-linux| 20:23:02
 clang-atom-d525-fedora-rel   | 20:05:09
 clang-cmake-x86_64-sde-avx512-linux  | 20:01:07
 lldb-x64-windows-ninja   | 17:20:38
 clang-x86_64-linux-selfhost-modules  | 15:44:15
 clang-cmake-armv7-selfhost-neon  | 09:13:54
 clang-cmake-armv7-selfhost   | 09:13:48
 clang-cmake-armv8-selfhost-neon  | 07:48:50
 clang-cmake-thumbv7-full-sh  | 07:47:40
 clang-cmake-armv7-global-isel| 05:19:00
 lld-x86_64-darwin13  | 05:04:59
 clang-cmake-armv7-quick  | 04:24:47
 sanitizer-x86_64-linux   | 04:11:25
 clang-cmake-armv8-lld| 04:10:50
 ppc64le-lld-multistage-test  | 04:04:59
 clang-cmake-armv7-lnt| 03:51:54
 clang-cuda-build | 03:42:09
 clang-cmake-aarch64-global-isel  | 03:31:12
 clang-cmake-armv8-global-isel| 03:29:20
 clang-cmake-aarch64-quick| 03:28:00
 clang-s390x-linux-multistage | 03:27:11
 llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast   | 03:06:12
 sanitizer-x86_64-linux-bootstrap | 02:44:44
 clang-s390x-linux| 02:42:50
 clang-s390x-linux-lnt| 02:26:05
 clang-cmake-armv8-quick  | 02:18:46
 clang-cmake-armv8-full   | 02:10:24
 sanitizer-x86_64-linux-bootstrap-msan| 02:07:17
 sanitizer-x86_64-linux-bootstrap-ubsan   | 02:06:44
 sanitizer-x86_64-linux-fast  | 02:03:37
 clang-cmake-thumbv8-full-sh  | 01:56:37
 clang-hexagon-elf| 01:52:51
 clang-sphinx-docs| 01:31:47
 llvm-hexagon-elf | 01:30:21
 clang-cmake-armv7-full   | 01:15:15
 polly-arm-linux  | 00:59:41
 sanitizer-windows| 00:59:24
 clang-cmake-x86_64-avx2-linux-perf   | 00:59:22
 sanitizer-x86_64-linux-autoconf  | 00:59:11
 lld-perf-testsuite   | 00:45:45
 clang-aarch64-linux-build-cache  | 00:42:13
 sanitizer-x86_64-linux-fuzzer| 00:41:02
 sanitizer-x86_64-linux-android   | 00:41:01
 clang-armv7-linux-build-cache| 00:37:49
 polly-amd64-linux| 00:33:24
 lld-x86_64-win7  | 00:30:47
 clang-x86_64-linux-abi-test  | 00:24:46
(64 rows)


"Status change ratio" by active builder (percent of builds that changed the
builder status from greed to red or from red to green):
   buildername   | builds | changes
| status_change_ratio
-++-+
 

Buildbot numbers for the week of 02/03/2019 - 02/09/2019

[Galina Kistanova via cfe-commits]

Hello everyone,

Below are some buildbot numbers for the week of 02/03/2019 - 02/09/2019.

Please see the same data in attached csv files:

The longest time each builder was red during the week;
"Status change ratio" by active builder (percent of builds that changed the
builder status from greed to red or from red to green);
Count of commits by project;
Number of completed builds, failed builds and average build time for
successful builds per active builder;
Average waiting time for a revision to get build result per active builder
(response time).

Thanks

Galina


The longest time each builder was red during the week:
   buildername| was_red
--+-
 clang-ppc64be-linux-multistage   | 23:03:12
 clang-ppc64be-linux  | 22:03:53
 clang-s390x-linux| 21:58:32
 clang-s390x-linux-lnt| 21:56:11
 clang-ppc64be-linux-lnt  | 21:43:26
 sanitizer-x86_64-linux   | 20:23:26
 sanitizer-ppc64be-linux  | 19:57:36
 sanitizer-ppc64le-linux  | 19:31:14
 clang-s390x-linux-multistage | 19:20:22
 ppc64le-lld-multistage-test  | 19:19:13
 clang-with-lto-ubuntu| 19:10:19
 sanitizer-x86_64-linux-android   | 17:48:35
 clang-cmake-armv8-selfhost-neon  | 15:38:12
 clang-cmake-aarch64-full | 14:54:26
 clang-cmake-armv8-lld| 14:27:23
 clang-cmake-aarch64-lld  | 14:15:12
 clang-cmake-thumbv8-full-sh  | 14:01:26
 clang-lld-x86_64-2stage  | 13:17:53
 clang-x86_64-linux-selfhost-modules  | 12:36:34
 llvm-clang-x86_64-expensive-checks-win   | 12:11:30
 clang-sphinx-docs| 09:40:23
 llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast | 09:14:33
 lldb-amd64-ninja-netbsd8 | 09:11:00
 clang-x64-windows-msvc   | 08:55:41
 clang-cmake-armv7-global-isel| 08:37:27
 clang-cmake-x86_64-avx2-linux| 08:18:17
 sanitizer-x86_64-linux-bootstrap-msan| 08:16:47
 clang-cmake-aarch64-global-isel  | 08:10:52
 clang-hexagon-elf| 08:08:50
 clang-cmake-aarch64-quick| 08:08:19
 clang-cmake-armv7-quick  | 08:06:20
 clang-ppc64le-linux  | 07:58:24
 clang-cmake-armv8-global-isel| 07:54:17
 clang-ppc64le-linux-lnt  | 07:44:32
 clang-cmake-thumbv7-full-sh  | 07:42:10
 clang-cmake-armv7-full   | 07:29:17
 clang-cmake-armv7-selfhost   | 06:28:25
 clang-cmake-armv7-selfhost-neon  | 06:23:05
 clang-ppc64le-linux-multistage   | 06:16:12
 sanitizer-x86_64-linux-bootstrap-ubsan   | 06:11:34
 clang-atom-d525-fedora-rel   | 05:28:47
 clang-cmake-armv8-quick  | 05:02:08
 clang-cmake-armv8-full   | 04:58:24
 llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast   | 04:56:06
 clang-with-thin-lto-ubuntu   | 04:46:36
 clang-cuda-build | 04:17:02
 sanitizer-x86_64-linux-bootstrap | 03:35:26
 sanitizer-x86_64-linux-fast  | 03:21:07
 clang-x86_64-debian-fast | 03:19:43
 clang-cmake-x86_64-sde-avx512-linux  | 03:19:43
 llvm-hexagon-elf | 02:14:06
 lldb-x86_64-ubuntu-14.04-buildserver | 02:08:59
 lldb-x64-windows-ninja   | 01:47:20
 sanitizer-windows| 01:37:36
 lld-perf-testsuite   | 01:35:07
 polly-arm-linux  | 01:34:06
 clang-aarch64-linux-build-cache  | 01:33:22
 clang-armv7-linux-build-cache| 01:32:44
 lld-x86_64-freebsd   | 01:29:18
 lld-x86_64-darwin13  | 01:28:00
 sanitizer-x86_64-linux-autoconf  | 01:27:01
 clang-cmake-x86_64-avx2-linux-perf   | 01:04:08
 clang-x86_64-linux-abi-test  | 00:49:10
 lld-x86_64-win7  | 00:29:26
 sanitizer-x86_64-linux-fuzzer| 00:23:12
 llvm-sphinx-docs | 00:21:15
(66 rows)


"Status change ratio" by active builder (percent of builds that changed the
builder status from greed to red or from red to green):
   buildername   | builds | changes
| 

[PATCH] D52956: Support `-fno-visibility-inlines-hidden`

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

You should ask the reviewers to commit it in your behalf. You can request 
commit access after you've had a few patches accepted and committed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52956



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

[PATCH] D52956: Support `-fno-visibility-inlines-hidden`

[Andrew Gallagher via Phabricator via cfe-commits]

andrewjcg added a comment.
Herald added a project: clang.

Sorry for the delay here, but this should be ready to go.

As this is my first accepted diff to LLVM, should I follow 
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access to get 
commit access, or is there some other process for first-time committers?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52956



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

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D58368#1406490 , @NoQ wrote:

> Address comments.
>
> @Charusso: I agreed not to rush for D58367  
> and implemented an old-style visitor here instead :)


Thanks you! I wanted to accept it when you remove it from the review-chain and 
after rewriting the summary. The latter is not that big deal, just wanted to 
let you know it remained the same.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58368



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

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

A separate point is whether it makes sense to be emitting this warning in the 
first place for `GE_Missing_type`. I'd argue that, if we don't know the type of 
the builtin, we should never emit the warning, since otherwise we always emit 
the warning even for legitimate instances, though that's not really important 
now we're not hitting that case any more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531



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

[PATCH] D58529: [analyzer] MIGChecker: Enable by default.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ closed this revision.
NoQ added a comment.

Committed as rC354644  but i forgot to 
update the link as usual.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58529



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

r354644 - [analyzer] MIGChecker: Enable by default as `osx.MIG'.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:18:46 2019
New Revision: 354644

URL: http://llvm.org/viewvc/llvm-project?rev=354644=rev
Log:
[analyzer] MIGChecker: Enable by default as `osx.MIG'.

With r354643, the checker is feature-rich and polished enough.

rdar://problem/35380337

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

Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=354644=354643=354644=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Thu Feb 21 
16:18:46 2019
@@ -712,6 +712,11 @@ def MacOSKeychainAPIChecker : Checker<"S
   HelpText<"Check for proper uses of Secure Keychain APIs">,
   Documentation;
 
+def MIGChecker : Checker<"MIG">,
+  HelpText<"Find violations of the Mach Interface Generator "
+   "calling convention">,
+  Documentation;
+
 def ObjCPropertyChecker : Checker<"ObjCProperty">,
   HelpText<"Check for proper uses of Objective-C properties">,
   Documentation;
@@ -828,15 +833,6 @@ def OSObjectCStyleCast : Checker<"OSObje
 
 } // end "optin.osx"
 
-let ParentPackage = OSXAlpha in {
-
-def MIGChecker : Checker<"MIG">,
-  HelpText<"Find violations of the Mach Interface Generator "
-   "calling convention">,
-  Documentation;
-
-} // end "alpha.osx"
-
 let ParentPackage = CocoaAlpha in {
 
 def IvarInvalidationModeling : Checker<"IvarInvalidationModeling">,

Modified: cfe/trunk/test/Analysis/mig.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=354644=354643=354644=diff
==
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Thu Feb 21 16:18:46 2019
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,osx.MIG\
 // RUN:   -analyzer-output=text -fblocks -verify %s
 
 typedef unsigned uint32_t;


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

[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354643: [analyzer] MIGChecker: Add support for more APIs. 
(authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58397?vs=187876=187887#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58397

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  cfe/trunk/test/Analysis/mig.mm

Index: cfe/trunk/test/Analysis/mig.mm
===
--- cfe/trunk/test/Analysis/mig.mm
+++ cfe/trunk/test/Analysis/mig.mm
@@ -1,20 +1,55 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
 // RUN:   -analyzer-output=text -fblocks -verify %s
 
+typedef unsigned uint32_t;
+
 // XNU APIs.
 
 typedef int kern_return_t;
 #define KERN_SUCCESS 0
 #define KERN_ERROR 1
+#define MIG_NO_REPLY (-305)
 
 typedef unsigned mach_port_name_t;
 typedef unsigned vm_address_t;
 typedef unsigned vm_size_t;
+typedef void *ipc_space_t;
+typedef unsigned long io_user_reference_t;
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+void mig_deallocate(vm_address_t, vm_size_t);
+kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
+// IOKit wrappers.
+
+class OSObject;
+typedef kern_return_t IOReturn;
+#define kIOReturnError 1
+
+enum {
+  kOSAsyncRef64Count = 8,
+};
+
+typedef io_user_reference_t OSAsyncReference64[kOSAsyncRef64Count];
+
+struct IOExternalMethodArguments {
+  io_user_reference_t *asyncReference;
+};
+
+struct IOExternalMethodDispatch {};
+
+class IOUserClient {
+public:
+  static IOReturn releaseAsyncReference64(OSAsyncReference64);
+
+  MIG_SERVER_ROUTINE
+  virtual IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+  IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0);
+};
+
 
 // Tests.
 
@@ -123,3 +158,52 @@
 return Empty{}; // no-crash
   };
 }
+
+// Test various APIs.
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_vm_deallocate(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  mach_vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_port_deallocate(ipc_space_t space,
+mach_port_name_t port) {
+  mach_port_deallocate(space, port); // expected-note{{Value passed through parameter 'port' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mig_deallocate(vm_address_t address, vm_size_t size) {
+  mig_deallocate(address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+// Let's try the C++11 attribute spelling syntax as well.
+[[clang::mig_server_routine]]
+IOReturn test_releaseAsyncReference64(IOExternalMethodArguments *arguments) {
+  IOUserClient::releaseAsyncReference64(arguments->asyncReference); // expected-note{{Value passed through parameter 'arguments' is deallocated}}
+  return kIOReturnError;// expected-warning{{MIG callback fails with error after deallocating argument value}}
+// expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_no_reply(ipc_space_t space, mach_port_name_t port) {
+  mach_port_deallocate(space, port);
+  return MIG_NO_REPLY; // no-warning
+}
+
+class MyClient: public IOUserClient {
+  // The MIG_SERVER_ROUTINE annotation is intentionally skipped.
+  // It should be picked up from the superclass.
+  IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+  IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0) override {
+
+

r354643 - [analyzer] MIGChecker: Add support for more APIs.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:15:14 2019
New Revision: 354643

URL: http://llvm.org/viewvc/llvm-project?rev=354643=rev
Log:
[analyzer] MIGChecker: Add support for more APIs.

Add more "consuming" functions. For now only vm_deallocate() was supported.

Add a non-zero value that isn't an error; this value is -305 ("MIG_NO_REPLY")
and it's fine to deallocate data when you are returning this error.

Make sure that the mig_server_routine annotation is inherited.

rdar://problem/35380337

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354643=354642=354643=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 16:15:14 
2019
@@ -8,13 +8,14 @@
 //
 // This file defines MIGChecker, a Mach Interface Generator calling convention
 // checker. Namely, in MIG callback implementation the following rules apply:
-// - When a server routine returns KERN_SUCCESS, it must take ownership of
-//   resources (and eventually release them).
-// - Additionally, when returning KERN_SUCCESS, all out-parameters must be
+// - When a server routine returns an error code that represents success, it
+//   must take ownership of resources passed to it (and eventually release
+//   them).
+// - Additionally, when returning success, all out-parameters must be
 //   initialized.
-// - When it returns anything except KERN_SUCCESS it must not take ownership,
-//   because the message and its descriptors will be destroyed by the server
-//   function.
+// - When it returns any other error code, it must not take ownership,
+//   because the message and its out-of-line parameters will be destroyed
+//   by the client that called the function.
 // For now we only check the last rule, as its violations lead to dangerous
 // use-after-free exploits.
 //
@@ -37,7 +38,29 @@ class MIGChecker : public Checker> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...)  
\
+  {{{__VA_ARGS__}, required_args}, deallocated_arg}
+  // E.g., if the checker sees a C function 'vm_deallocate' that is
+  // defined on class 'IOUserClient' that has exactly 3 parameters, it 
knows
+  // that argument #1 (starting from 0, i.e. the second argument) is going
+  // to be consumed in the sense of the MIG consume-on-success convention.
+  CALL(3, 1, "vm_deallocate"),
+  CALL(3, 1, "mach_vm_deallocate"),
+  CALL(2, 0, "mig_deallocate"),
+  CALL(2, 1, "mach_port_deallocate"),
+  // E.g., if the checker sees a method 'releaseAsyncReference64()' that is
+  // defined on class 'IOUserClient' that takes exactly 1 argument, it 
knows
+  // that the argument is going to be consumed in the sense of the MIG
+  // consume-on-success convention.
+  CALL(1, 0, "IOUserClient", "releaseAsyncReference64"),
+#undef CALL
+  };
 
   void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
 
@@ -100,10 +123,23 @@ static const ParmVarDecl *getOriginParam
   if (!Sym)
 return nullptr;
 
-  const auto *VR = dyn_cast_or_null(Sym->getOriginRegion());
-  if (VR && VR->hasStackParametersStorage() &&
- VR->getStackFrame()->inTopFrame())
-return cast(VR->getDecl());
+  // If we optimistically assume that the MIG routine never re-uses the storage
+  // that was passed to it as arguments when it invalidates it (but at most 
when
+  // it assigns to parameter variables directly), this procedure correctly
+  // determines if the value was loaded from the transitive closure of MIG
+  // routine arguments in the heap.
+  while (const MemRegion *MR = Sym->getOriginRegion()) {
+const auto *VR = dyn_cast(MR);
+if (VR && VR->hasStackParametersStorage() &&
+   VR->getStackFrame()->inTopFrame())
+  return cast(VR->getDecl());
+
+const SymbolicRegion *SR = MR->getSymbolicBase();
+if (!SR)
+  return nullptr;
+
+Sym = SR->getSymbol();
+  }
 
   return nullptr;
 }
@@ -133,6 +169,12 @@ static bool isInMIGCall(CheckerContext &
   if (D->hasAttr())
 return true;
 
+  // See if there's an annotated method in the superclass.
+  if (const auto *MD = dyn_cast(D))
+for (const auto *OMD: MD->overridden_methods())
+  if (OMD->hasAttr())
+return true;
+
   return false;
 }
 
@@ -140,14 +182,15 @@ void MIGChecker::checkPostCall(const Cal
   if (!isInMIGCall(C))
 return;
 
-  if (!Call.isGlobalCFunction())
+  auto I = std::find_if(Deallocators.begin(), Deallocators.end(),
+[&](const std::pair ) {
+  return Call.isCalled(Item.first);
+});
+  if (I 

[PATCH] D58392: [analyzer] MIGChecker: Fix false negatives for releases in automatic destructors.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354642: [analyzer] MIGChecker: Fix an FN when the object is 
released in a destructor. (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58392?vs=187875=187886#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58392

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  cfe/trunk/test/Analysis/mig.mm


Index: cfe/trunk/test/Analysis/mig.mm
===
--- cfe/trunk/test/Analysis/mig.mm
+++ cfe/trunk/test/Analysis/mig.mm
@@ -56,6 +56,31 @@
  // expected-note@-1{{MIG callback fails with error after 
deallocating argument value. This is a use-after-free vulnerability because the 
caller will try to deallocate it again}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, 
vm_address_t address, vm_size_t size) {
+  struct WillDeallocate {
+mach_port_name_t port;
+vm_address_t address;
+vm_size_t size;
+~WillDeallocate() {
+  vm_deallocate(port, address, size); // expected-note{{Value passed 
through parameter 'address' is deallocated}}
+}
+  } will_deallocate{port, address, size};
+
+ if (size > 10) {
+// expected-note@-1{{Assuming 'size' is > 10}}
+// expected-note@-2{{Taking true branch}}
+return KERN_ERROR;
+// expected-note@-1{{Calling '~WillDeallocate'}}
+// expected-note@-2{{Returning from '~WillDeallocate'}}
+// expected-warning@-3{{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+// expected-note@-4   {{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+  }
+  return KERN_SUCCESS;
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port 
withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -32,15 +32,30 @@
 using namespace ento;
 
 namespace {
-class MIGChecker : public Checker> 
{
+class MIGChecker : public Checker,
+  check::EndFunction> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
  categories::MemoryError};
 
   CallDescription vm_deallocate { "vm_deallocate", 3 };
 
+  void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
+
 public:
   void checkPostCall(const CallEvent , CheckerContext ) const;
-  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+
+  // HACK: We're making two attempts to find the bug: checkEndFunction
+  // should normally be enough but it fails when the return value is a literal
+  // that never gets put into the Environment and ends of function with 
multiple
+  // returns get agglutinated across returns, preventing us from obtaining
+  // the return value. The problem is similar to 
https://reviews.llvm.org/D25326
+  // but now we step into it in the top-level function.
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
+  void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
 
   class Visitor : public BugReporterVisitor {
   public:
@@ -140,7 +155,7 @@
   C.addTransition(C.getState()->set(PVD));
 }
 
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext ) const 
{
   // It is very unlikely that a MIG callback will be called from anywhere
   // within the project under analysis and the caller isn't itself a routine
   // that follows the MIG calling convention. Therefore we're safe to believe


Index: cfe/trunk/test/Analysis/mig.mm
===
--- cfe/trunk/test/Analysis/mig.mm
+++ cfe/trunk/test/Analysis/mig.mm
@@ -56,6 +56,31 @@
  // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, vm_address_t address, vm_size_t 

[PATCH] D58530: Add PragmaHandler for MSVC pragma execution_character_set

[Matt Gardner via Phabricator via cfe-commits]

sigatrev created this revision.
sigatrev added reviewers: cfe-commits, rnk.
Herald added subscribers: jdoerfert, jsji, kbarton, nemanjai.
Herald added a project: clang.

__pragma(execution_character_set(push, "UTF-8")) is used in 
TraceLoggingProvider.h. This commit 
implements a no-op handler for compatability, similar to how the flag 
-fexec_charset is handled.


Repository:
  rC Clang

https://reviews.llvm.org/D58530

Files:
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.h
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/lib/Lex/Pragma.cpp

Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -1368,6 +1368,70 @@
   }
 };
 
+/// "\#pragma execution_character_set(...)". MSVC supports this pragma only
+/// for "UTF-8". We parse it and ignore it if UTF-8 is provided and warn
+/// otherwise to avoid -Wunknown-pragma warnings.
+struct PragmaExecCharsetHandler : public PragmaHandler {
+  PragmaExecCharsetHandler() : PragmaHandler("execution_character_set") {}
+
+  void HandlePragma(Preprocessor , PragmaIntroducerKind Introducer,
+Token ) override {
+// Parse things like:
+// execution_character_set(push, "UTF-8")
+// execution_character_set(pop)
+SourceLocation DiagLoc = Tok.getLocation();
+PPCallbacks *Callbacks = PP.getPPCallbacks();
+
+PP.Lex(Tok);
+if (Tok.isNot(tok::l_paren)) {
+  PP.Diag(Tok, diag::warn_pragma_exec_charset_expected) << "(";
+  return;
+}
+
+PP.Lex(Tok);
+IdentifierInfo *II = Tok.getIdentifierInfo();
+
+if (II && II->isStr("push")) {
+  // #pragma execution_character_set( push[ , string ] )
+  PP.Lex(Tok);
+  if (Tok.is(tok::comma)) {
+PP.Lex(Tok);
+
+std::string ExecCharset;
+if (!PP.FinishLexStringLiteral(Tok, ExecCharset,
+   "pragma execution_character_set",
+   /*MacroExpansion=*/false))
+  return;
+
+// MSVC supports either of these, but nothing else.
+if (ExecCharset != "UTF-8" && ExecCharset != "utf-8") {
+  PP.Diag(Tok, diag::warn_pragma_exec_charset_spec_invalid);
+  return;
+}
+  }
+  if (Callbacks)
+Callbacks->PragmaExecCharsetPush(DiagLoc, "UTF-8");
+} else if (II && II->isStr("pop")) {
+  // #pragma execution_character_set( pop )
+  PP.Lex(Tok);
+  if (Callbacks)
+Callbacks->PragmaExecCharsetPop(DiagLoc);
+} else {
+  PP.Diag(Tok, diag::warn_pragma_exec_charset_invalid_token);
+  return;
+}
+
+if (Tok.isNot(tok::r_paren)) {
+  PP.Diag(Tok, diag::warn_pragma_exec_charset_expected) << ")";
+  return;
+}
+
+PP.Lex(Tok);
+if (Tok.isNot(tok::eod))
+  PP.Diag(Tok, diag::ext_pp_extra_tokens_at_eol) << "pragma execution_character_set";
+  }
+};
+
 /// PragmaIncludeAliasHandler - "\#pragma include_alias("...")".
 struct PragmaIncludeAliasHandler : public PragmaHandler {
   PragmaIncludeAliasHandler() : PragmaHandler("include_alias") {}
@@ -1823,6 +1887,7 @@
   // MS extensions.
   if (LangOpts.MicrosoftExt) {
 AddPragmaHandler(new PragmaWarningHandler());
+AddPragmaHandler(new PragmaExecCharsetHandler());
 AddPragmaHandler(new PragmaIncludeAliasHandler());
 AddPragmaHandler(new PragmaHdrstopHandler());
   }
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -143,6 +143,8 @@
  ArrayRef Ids) override;
   void PragmaWarningPush(SourceLocation Loc, int Level) override;
   void PragmaWarningPop(SourceLocation Loc) override;
+  void PragmaExecCharsetPush(SourceLocation Loc, StringRef Str) override;
+  void PragmaExecCharsetPop(SourceLocation Loc) override;
   void PragmaAssumeNonNullBegin(SourceLocation Loc) override;
   void PragmaAssumeNonNullEnd(SourceLocation Loc) override;
 
@@ -553,6 +555,24 @@
   setEmittedDirectiveOnThisLine();
 }
 
+void PrintPPOutputPPCallbacks::PragmaExecCharsetPush(SourceLocation Loc,
+ StringRef Str) {
+  startNewLineIfNeeded();
+  MoveToLine(Loc);
+  OS << "#pragma character_execution_set(push";
+  if (!Str.empty())
+OS << ", " << Str;
+  OS << ')';
+  setEmittedDirectiveOnThisLine();
+}
+
+void PrintPPOutputPPCallbacks::PragmaExecCharsetPop(SourceLocation Loc) {
+  startNewLineIfNeeded();
+  MoveToLine(Loc);
+  OS << "#pragma character_execution_set(pop)";
+  setEmittedDirectiveOnThisLine();
+}
+
 void PrintPPOutputPPCallbacks::
 PragmaAssumeNonNullBegin(SourceLocation Loc) {
   startNewLineIfNeeded();
Index: 

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[James Clarke via Phabricator via cfe-commits]

jrtc27 created this revision.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.

Currently, pthread_create's type string is empty, so GetBuiltinType
fails and any header declaring it gives a -Wbuiltin-requires-header
warning, which gives a false-positive when including pthread.h itself
with -Wsystem-headers. By specifying its type, this false-positive goes
away and it behaves like every other builtin declared in system headers,
only warning when the declaration does not match the expected type.

This requires introducing a way to declare function pointer types in
builtin type strings, which requires refactoring GetBuiltinType to allow
recursive parsing of function types, expressed as a type string within
parentheses.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58531

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Analysis/retain-release.m
  clang/test/Sema/implicit-builtin-decl.c

Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -66,3 +66,5 @@
 // CHECK: FunctionDecl {{.*}}  col:6 sigsetjmp '
 // CHECK-NOT: FunctionDecl
 // CHECK: ReturnsTwiceAttr {{.*}} <{{.*}}> Implicit
+
+int pthread_create(); // expected-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header }}
Index: clang/test/Analysis/retain-release.m
===
--- clang/test/Analysis/retain-release.m
+++ clang/test/Analysis/retain-release.m
@@ -1231,7 +1231,7 @@
 typedef unsigned long __darwin_pthread_key_t;
 typedef __darwin_pthread_key_t pthread_key_t;
 
-int pthread_create(pthread_t *, const pthread_attr_t *,  // C-warning{{declaration of built-in function 'pthread_create' requires inclusion of the header }}
+int pthread_create(pthread_t *, const pthread_attr_t *,  // no-warning
void *(*)(void *), void *);
 
 int pthread_setspecific(pthread_key_t key, const void *value);
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4904,6 +4904,8 @@
   AddTypeRef(Context.ObjCClassRedefinitionType, SpecialTypes);
   AddTypeRef(Context.ObjCSelRedefinitionType, SpecialTypes);
   AddTypeRef(Context.getucontext_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_tType(), SpecialTypes);
+  AddTypeRef(Context.getpthread_attr_tType(), SpecialTypes);
 
   if (Chain) {
 // Write the mapping information describing our module dependencies and how
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4560,6 +4560,42 @@
 }
   }
 }
+
+if (unsigned Pthread_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_T]) {
+  QualType Pthread_tType = GetType(Pthread_t);
+  if (Pthread_tType.isNull()) {
+Error("pthread_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_tDecl) {
+if (const TypedefType *Typedef = Pthread_tType->getAs())
+  Context.setpthread_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_tType->getAs();
+  assert(Tag && "Invalid pthread_t type in AST file");
+  Context.setpthread_tDecl(Tag->getDecl());
+}
+  }
+}
+
+if (unsigned Pthread_attr_t = SpecialTypes[SPECIAL_TYPE_PTHREAD_ATTR_T]) {
+  QualType Pthread_attr_tType = GetType(Pthread_attr_t);
+  if (Pthread_attr_tType.isNull()) {
+Error("pthread_attr_t type is NULL");
+return;
+  }
+
+  if (!Context.pthread_attr_tDecl) {
+if (const TypedefType *Typedef = Pthread_attr_tType->getAs())
+  Context.setpthread_attr_tDecl(Typedef->getDecl());
+else {
+  const TagType *Tag = Pthread_attr_tType->getAs();
+  assert(Tag && "Invalid pthread_attr_t type in AST file");
+  Context.setpthread_attr_tDecl(Tag->getDecl());
+}
+  }
+}
   }
 
   ReadPragmaDiagnosticMappings(Context.getDiagnostics());
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -1943,6 +1943,8 @@
 return "setjmp.h";
   case ASTContext::GE_Missing_ucontext:
 return "ucontext.h";
+  case ASTContext::GE_Missing_pthread:
+return "pthread.h";
   }
   llvm_unreachable("unhandled error kind");
 }
@@ -5809,6 +5811,10 @@
 

[PATCH] D58366: [analyzer] MIGChecker: Make use of the server routine annotation.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ closed this revision.
NoQ added a comment.

Committed as rC354638  but i screwed up the 
link in the commit message.


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

https://reviews.llvm.org/D58366



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

r354642 - [analyzer] MIGChecker: Fix an FN when the object is released in a destructor.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:09:56 2019
New Revision: 354642

URL: http://llvm.org/viewvc/llvm-project?rev=354642=rev
Log:
[analyzer] MIGChecker: Fix an FN when the object is released in a destructor.

When a MIG server routine argument is released in an automatic destructor,
the Static Analyzer thinks that this happens after the return statement, and so
the violation of the MIG convention doesn't happen.

Of course, it doesn't quite work that way, so this is a false negative.

Add a hack that makes the checker double-check at the end of function
that no argument was released when the routine fails with an error.

rdar://problem/35380337

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354642=354641=354642=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 16:09:56 
2019
@@ -32,15 +32,30 @@ using namespace clang;
 using namespace ento;
 
 namespace {
-class MIGChecker : public Checker> 
{
+class MIGChecker : public Checker,
+  check::EndFunction> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
  categories::MemoryError};
 
   CallDescription vm_deallocate { "vm_deallocate", 3 };
 
+  void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
+
 public:
   void checkPostCall(const CallEvent , CheckerContext ) const;
-  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+
+  // HACK: We're making two attempts to find the bug: checkEndFunction
+  // should normally be enough but it fails when the return value is a literal
+  // that never gets put into the Environment and ends of function with 
multiple
+  // returns get agglutinated across returns, preventing us from obtaining
+  // the return value. The problem is similar to 
https://reviews.llvm.org/D25326
+  // but now we step into it in the top-level function.
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
+  void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
 
   class Visitor : public BugReporterVisitor {
   public:
@@ -140,7 +155,7 @@ void MIGChecker::checkPostCall(const Cal
   C.addTransition(C.getState()->set(PVD));
 }
 
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext ) const 
{
   // It is very unlikely that a MIG callback will be called from anywhere
   // within the project under analysis and the caller isn't itself a routine
   // that follows the MIG calling convention. Therefore we're safe to believe

Modified: cfe/trunk/test/Analysis/mig.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=354642=354641=354642=diff
==
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Thu Feb 21 16:09:56 2019
@@ -56,6 +56,31 @@ kern_return_t release_twice(mach_port_na
  // expected-note@-1{{MIG callback fails with error after 
deallocating argument value. This is a use-after-free vulnerability because the 
caller will try to deallocate it again}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, 
vm_address_t address, vm_size_t size) {
+  struct WillDeallocate {
+mach_port_name_t port;
+vm_address_t address;
+vm_size_t size;
+~WillDeallocate() {
+  vm_deallocate(port, address, size); // expected-note{{Value passed 
through parameter 'address' is deallocated}}
+}
+  } will_deallocate{port, address, size};
+
+ if (size > 10) {
+// expected-note@-1{{Assuming 'size' is > 10}}
+// expected-note@-2{{Taking true branch}}
+return KERN_ERROR;
+// expected-note@-1{{Calling '~WillDeallocate'}}
+// expected-note@-2{{Returning from '~WillDeallocate'}}
+// expected-warning@-3{{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+// expected-note@-4   {{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+  }
+  return KERN_SUCCESS;
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port 

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Phabricator via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354641: [analyzer] MIGChecker: Improve intermediate 
diagnostic notes. (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58368?vs=187873=187883#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58368

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  cfe/trunk/test/Analysis/mig.mm

Index: cfe/trunk/test/Analysis/mig.mm
===
--- cfe/trunk/test/Analysis/mig.mm
+++ cfe/trunk/test/Analysis/mig.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN:-fblocks -verify %s
+// RUN:   -analyzer-output=text -fblocks -verify %s
 
 // XNU APIs.
 
@@ -20,9 +20,11 @@
 
 MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
-  vm_deallocate(port, address, size);
-  if (size > 10) {
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+   // expected-note@-1{{Taking true branch}}
 return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+			 // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
   }
   return KERN_SUCCESS;
 }
@@ -42,6 +44,18 @@
   vm_deallocate(port, address, size);
 }
 
+// When releasing two parameters, add a note for both of them.
+// Also when returning a variable, explain why do we think that it contains
+// a non-success code.
+MIG_SERVER_ROUTINE
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_address_t addr2, vm_size_t size) {
+  kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+  vm_deallocate(port, addr1, size); // expected-note{{Value passed through parameter 'addr1' is deallocated}}
+  vm_deallocate(port, addr2, size); // expected-note{{Value passed through parameter 'addr2' is deallocated}}
+  return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
@@ -51,8 +65,9 @@
 - (kern_return_t)fooAtPort:(mach_port_name_t)port
withAddress:(vm_address_t)address
 ofSize:(vm_size_t)size MIG_SERVER_ROUTINE {
-  vm_deallocate(port, address, size);
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
   return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
 }
 @end
 
@@ -60,8 +75,9 @@
   kern_return_t (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
   ^MIG_SERVER_ROUTINE (mach_port_name_t port,
vm_address_t address, vm_size_t size) {
-vm_deallocate(port, address, size);
+vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
     return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+   // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
   };
 }
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -41,19 +41,56 @@
 public:
   void 

r354641 - [analyzer] MIGChecker: Improve intermediate diagnostic notes.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:06:30 2019
New Revision: 354641

URL: http://llvm.org/viewvc/llvm-project?rev=354641=rev
Log:
[analyzer] MIGChecker: Improve intermediate diagnostic notes.

Add a BugReporterVisitor for highlighting the events of deallocating a
parameter. All such events are relevant to the emitted report (as long as the
report is indeed emitted), so all of them will get highlighted.

Add a trackExpressionValue visitor for highlighting where does the error return
code come from.

Do not add a trackExpressionValue visitor for highlighting how the deallocated
argument(s) was(were) copied around. This still remains to be implemented.

rdar://problem/35380337

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354641=354640=354641=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 16:06:30 
2019
@@ -41,19 +41,56 @@ class MIGChecker : public Checker VisitNode(const ExplodedNode *N,
+BugReporterContext , BugReport );
+  };
 };
 } // end anonymous namespace
 
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
+// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
+// specialization for this sort of types.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
+
+std::shared_ptr
+MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext ,
+   BugReport ) {
+  const auto *NewPVD = static_cast(
+  N->getState()->get());
+  const auto *OldPVD = static_cast(
+  N->getFirstPred()->getState()->get());
+  if (OldPVD == NewPVD)
+return nullptr;
+
+  assert(NewPVD && "What is deallocated cannot be un-deallocated!");
+  SmallString<64> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Value passed through parameter '" << NewPVD->getName()
+ << "' is deallocated";
+
+  PathDiagnosticLocation Loc =
+  PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
+  return std::make_shared(Loc, OS.str());
+}
 
-static bool isCurrentArgSVal(SVal V, CheckerContext ) {
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ) {
   SymbolRef Sym = V.getAsSymbol();
   if (!Sym)
-return false;
+return nullptr;
 
   const auto *VR = dyn_cast_or_null(Sym->getOriginRegion());
-  return VR && VR->hasStackParametersStorage() &&
- VR->getStackFrame()->inTopFrame();
+  if (VR && VR->hasStackParametersStorage() &&
+ VR->getStackFrame()->inTopFrame())
+return cast(VR->getDecl());
+
+  return nullptr;
 }
 
 static bool isInMIGCall(CheckerContext ) {
@@ -96,8 +133,11 @@ void MIGChecker::checkPostCall(const Cal
 
   // TODO: Unhardcode "1".
   SVal Arg = Call.getArgSVal(1);
-  if (isCurrentArgSVal(Arg, C))
-C.addTransition(C.getState()->set(true));
+  const ParmVarDecl *PVD = getOriginParam(Arg, C);
+  if (!PVD)
+return;
+
+  C.addTransition(C.getState()->set(PVD));
 }
 
 void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
@@ -139,6 +179,9 @@ void MIGChecker::checkPreStmt(const Retu
   "deallocate it again",
   N);
 
+  R->addRange(RS->getSourceRange());
+  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+  R->addVisitor(llvm::make_unique());
   C.emitReport(std::move(R));
 }
 

Modified: cfe/trunk/test/Analysis/mig.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=354641=354640=354641=diff
==
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Thu Feb 21 16:06:30 2019
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN:-fblocks -verify %s
+// RUN:   -analyzer-output=text -fblocks -verify %s
 
 // XNU APIs.
 
@@ -20,9 +20,11 @@ kern_return_t vm_deallocate(mach_port_na
 
 MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, 
vm_size_t size) {
-  vm_deallocate(port, address, size);
-  if (size > 10) {
+  vm_deallocate(port, address, size); // expected-note{{Value passed through 
parameter 'address' is deallocated}}
+  if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+   // expected-note@-1{{Taking true branch}}
 return KERN_ERROR; // expected-warning{{MIG callback fails with error 
after deallocating argument value. This is a use-after-free vulnerability 
because the caller will try to deallocate it again}}
+   
   

r354638 - [analyzer] MIGChecker: Take advantage of the mig_server_routine annotation.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:02:47 2019
New Revision: 354638

URL: http://llvm.org/viewvc/llvm-project?rev=354638=rev
Log:
[analyzer] MIGChecker: Take advantage of the mig_server_routine annotation.

r354530 has added a new function/block/message attribute "mig_server_routine"
that attracts compiler's attention to functions that need to follow the MIG
server routine convention with respect to deallocating out-of-line data that
was passed to them as an argument.

Teach the checker to identify MIG routines by looking at this attribute,
rather than by making heuristic-based guesses.

rdar://problem/35380337

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354638=354637=354638=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 16:02:47 
2019
@@ -20,6 +20,7 @@
 //
 
//===--===//
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -55,8 +56,8 @@ static bool isCurrentArgSVal(SVal V, Che
  VR->getStackFrame()->inTopFrame();
 }
 
-// This function will probably be replaced with looking up annotations.
-static bool isInMIGCall(const LocationContext *LC) {
+static bool isInMIGCall(CheckerContext ) {
+  const LocationContext *LC = C.getLocationContext();
   const StackFrameContext *SFC;
   // Find the top frame.
   while (LC) {
@@ -64,21 +65,27 @@ static bool isInMIGCall(const LocationCo
 LC = SFC->getParent();
   }
 
-  const auto *FD = dyn_cast(SFC->getDecl());
-  if (!FD)
-return false;
+  const Decl *D = SFC->getDecl();
+
+  if (Optional AC = AnyCall::forDecl(D)) {
+// Even though there's a Sema warning when the return type of an annotated
+// function is not a kern_return_t, this warning isn't an error, so we need
+// an extra sanity check here.
+// FIXME: AnyCall doesn't support blocks yet, so they remain unchecked
+// for now.
+if (!AC->getReturnType(C.getASTContext())
+ .getCanonicalType()->isSignedIntegerType())
+  return false;
+  }
+
+  if (D->hasAttr())
+return true;
 
-  // FIXME: This is an unreliable (even if surprisingly reliable) heuristic.
-  // The real solution here is to make MIG annotate its callbacks in
-  // autogenerated headers so that we didn't need to think hard if it's
-  // actually a MIG callback.
-  QualType T = FD->getReturnType();
-  return T.getCanonicalType()->isIntegerType() &&
- T.getAsString() == "kern_return_t";
+  return false;
 }
 
 void MIGChecker::checkPostCall(const CallEvent , CheckerContext ) const 
{
-  if (!isInMIGCall(C.getStackFrame()))
+  if (!isInMIGCall(C))
 return;
 
   if (!Call.isGlobalCFunction())
@@ -105,7 +112,7 @@ void MIGChecker::checkPreStmt(const Retu
   if (!C.inTopFrame())
 return;
 
-  if (!isInMIGCall(C.getStackFrame()))
+  if (!isInMIGCall(C))
 return;
 
   // We know that the function is non-void, but what if the return statement

Modified: cfe/trunk/test/Analysis/mig.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=354638=354637=354638=diff
==
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Thu Feb 21 16:02:47 2019
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN:-fblocks -verify %s
 
 // XNU APIs.
 
@@ -12,8 +13,12 @@ typedef unsigned vm_size_t;
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 
+#define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
+
+
 // Tests.
 
+MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, 
vm_size_t size) {
   vm_deallocate(port, address, size);
   if (size > 10) {
@@ -22,6 +27,7 @@ kern_return_t basic_test(mach_port_name_
   return KERN_SUCCESS;
 }
 
+MIG_SERVER_ROUTINE
 kern_return_t test_unknown_return_value(mach_port_name_t port, vm_address_t 
address, vm_size_t size) {
   extern kern_return_t foo();
 
@@ -31,6 +37,48 @@ kern_return_t test_unknown_return_value(
 }
 
 // Make sure we don't crash when they forgot to write the return statement.
+MIG_SERVER_ROUTINE
 kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t 
size) {
   vm_deallocate(port, address, size);
 }
+
+// Check that we 

[PATCH] D57558: [analyzer] MIGChecker: A checker for Mach Interface Generator calling convention.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC354635: [analyzer] MIGChecker: A checker for Mach Interface 
Generator conventions. (authored by dergachev, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57558?vs=187870=187881#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57558

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  test/Analysis/mig.mm

Index: test/Analysis/mig.mm
===
--- test/Analysis/mig.mm
+++ test/Analysis/mig.mm
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG -verify %s
+
+// XNU APIs.
+
+typedef int kern_return_t;
+#define KERN_SUCCESS 0
+#define KERN_ERROR 1
+
+typedef unsigned mach_port_name_t;
+typedef unsigned vm_address_t;
+typedef unsigned vm_size_t;
+
+kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+
+// Tests.
+
+kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  vm_deallocate(port, address, size);
+  if (size > 10) {
+return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+  }
+  return KERN_SUCCESS;
+}
+
+kern_return_t test_unknown_return_value(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  extern kern_return_t foo();
+
+  vm_deallocate(port, address, size);
+  // We don't know if it's a success or a failure.
+  return foo(); // no-warning
+}
+
+// Make sure we don't crash when they forgot to write the return statement.
+kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  vm_deallocate(port, address, size);
+}
Index: lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -0,0 +1,144 @@
+//== MIGChecker.cpp - MIG calling convention checker *- C++ -*--==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines MIGChecker, a Mach Interface Generator calling convention
+// checker. Namely, in MIG callback implementation the following rules apply:
+// - When a server routine returns KERN_SUCCESS, it must take ownership of
+//   resources (and eventually release them).
+// - Additionally, when returning KERN_SUCCESS, all out-parameters must be
+//   initialized.
+// - When it returns anything except KERN_SUCCESS it must not take ownership,
+//   because the message and its descriptors will be destroyed by the server
+//   function.
+// For now we only check the last rule, as its violations lead to dangerous
+// use-after-free exploits.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class MIGChecker : public Checker> {
+  BugType BT{this, "Use-after-free (MIG calling convention violation)",
+ categories::MemoryError};
+
+  CallDescription vm_deallocate { "vm_deallocate", 3 };
+
+public:
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+};
+} // end anonymous namespace
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
+
+static bool isCurrentArgSVal(SVal V, CheckerContext ) {
+  SymbolRef Sym = V.getAsSymbol();
+  if (!Sym)
+return false;
+
+  const auto *VR = dyn_cast_or_null(Sym->getOriginRegion());
+  return VR && VR->hasStackParametersStorage() &&
+ VR->getStackFrame()->inTopFrame();
+}
+
+// This function will probably be replaced with looking up annotations.
+static bool isInMIGCall(const LocationContext *LC) {
+  const StackFrameContext *SFC;
+  // Find the top frame.
+  while (LC) {
+SFC = LC->getStackFrame();
+LC = SFC->getParent();
+  }
+
+  const auto *FD = dyn_cast(SFC->getDecl());
+  if (!FD)
+return false;
+
+  // FIXME: This is an unreliable (even if surprisingly reliable) heuristic.
+  // The real solution here is to 

r354635 - [analyzer] MIGChecker: A checker for Mach Interface Generator conventions.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 15:55:28 2019
New Revision: 354635

URL: http://llvm.org/viewvc/llvm-project?rev=354635=rev
Log:
[analyzer] MIGChecker: A checker for Mach Interface Generator conventions.

This checker detects use-after-free bugs in (various forks of) the Mach kernel
that are caused by errors in MIG server routines - functions called remotely by
MIG clients. The MIG convention forces the server to only deallocate objects
it receives from the client when the routine is executed successfully.
Otherwise, if the server routine exits with an error, the client assumes that
it needs to deallocate the out-of-line data it passed to the server manually.
This means that deallocating such data within the MIG routine and then returning
a non-zero error code is always a dangerous use-after-free bug.

rdar://problem/35380337

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

Added:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=354635=354634=354635=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Thu Feb 21 
15:55:28 2019
@@ -828,6 +828,15 @@ def OSObjectCStyleCast : Checker<"OSObje
 
 } // end "optin.osx"
 
+let ParentPackage = OSXAlpha in {
+
+def MIGChecker : Checker<"MIG">,
+  HelpText<"Find violations of the Mach Interface Generator "
+   "calling convention">,
+  Documentation;
+
+} // end "alpha.osx"
+
 let ParentPackage = CocoaAlpha in {
 
 def IvarInvalidationModeling : Checker<"IvarInvalidationModeling">,

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=354635=354634=354635=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Thu Feb 21 15:55:28 
2019
@@ -51,6 +51,7 @@ add_clang_library(clangStaticAnalyzerChe
   MallocOverflowSecurityChecker.cpp
   MallocSizeofChecker.cpp
   MmapWriteExecChecker.cpp
+  MIGChecker.cpp
   MoveChecker.cpp
   MPI-Checker/MPIBugReporter.cpp
   MPI-Checker/MPIChecker.cpp

Added: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354635=auto
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (added)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 15:55:28 
2019
@@ -0,0 +1,144 @@
+//== MIGChecker.cpp - MIG calling convention checker *- C++ 
-*--==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines MIGChecker, a Mach Interface Generator calling convention
+// checker. Namely, in MIG callback implementation the following rules apply:
+// - When a server routine returns KERN_SUCCESS, it must take ownership of
+//   resources (and eventually release them).
+// - Additionally, when returning KERN_SUCCESS, all out-parameters must be
+//   initialized.
+// - When it returns anything except KERN_SUCCESS it must not take ownership,
+//   because the message and its descriptors will be destroyed by the server
+//   function.
+// For now we only check the last rule, as its violations lead to dangerous
+// use-after-free exploits.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class MIGChecker : public Checker> 
{
+  BugType BT{this, "Use-after-free (MIG calling convention violation)",
+ categories::MemoryError};
+
+  CallDescription vm_deallocate { "vm_deallocate", 3 };
+
+public:
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+};
+} // end 

[PATCH] D58529: [analyzer] MIGChecker: Enable by default.

[Devin Coughlin via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58529



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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Tom Roeder via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

tmroeder wrote:
> aaron.ballman wrote:
> > tmroeder wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > tmroeder wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > a_sidorin wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > > > > E->isConditionTrue();`
> > > > > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > > > > E->isConditionTrue();`?
> > > > > > > I like that even better than my suggestion. :-)
> > > > > > Wait, this doesn't have the same truth table as my original code.
> > > > > > 
> > > > > > let `CD = E->isConditionDependent()`
> > > > > > let `CT = E->isConditionTrue()`
> > > > > > 
> > > > > > in
> > > > > > 
> > > > > > ```
> > > > > > bool CondIsTrue = false;
> > > > > > if (!CD)
> > > > > >   CondIsTrue = CT;
> > > > > > ```
> > > > > > 
> > > > > > has the table for `CondIsTrue`:
> > > > > > 
> > > > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > > > | T | T | F |
> > > > > > | T | F | F |
> > > > > > | F | T | T |
> > > > > > | F | F | F |
> > > > > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, 
> > > > > > it's the value of CT.
> > > > > > 
> > > > > > The suggested line has the truth table
> > > > > > 
> > > > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > > > | T | T | T |
> > > > > > | T | F | F |
> > > > > > | F | T | F |
> > > > > > | F | F | F |
> > > > > > 
> > > > > > That is, the effect of CD is swapped.
> > > > > > 
> > > > > > Aaron's suggestion matches my original table. I based my code on 
> > > > > > include/clang/AST/Expr.h line 4032, which asserts 
> > > > > > !isConditionDependent() in the implementation of isConditionTrue.
> > > > > > 
> > > > > > I realized this after I "fixed" my comment to match the 
> > > > > > implementation change. Am I missing something? Or is the assertion 
> > > > > > in Expr.h wrong? I think this should be
> > > > > > 
> > > > > > ```
> > > > > > bool CondIsTrue = !E->isConditionDependent() && 
> > > > > > E->isConditionTrue();
> > > > > > ```
> > > > > > 
> > > > > > I've changed my code to that and reverted the comment change.
> > > > > Good catch -- I think my eyes just missed the change in logic. 
> > > > > Perhaps we should add a test case that exercises this?
> > > > Wow, that's a nice catch. Sorry for the misleading.
> > > I started to look for a way to test this, then realized that while it's 
> > > possible to test the code itself, it's impossible to make a ChooseExpr 
> > > with isConditionDependent() that returns true for real code.
> > > 
> > > TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a 
> > > C++-only condition; it's always false in C.
> > > 
> > > Details:
> > > 
> > > ChooseExpr only represents __builtin_choose_expr which is only valid in 
> > > C, not C++. That means ChooseExpr::isConditionDependent() will always be 
> > > false.
> > > 
> > > The definition is
> > > 
> > > ```
> > > bool isConditionDependent() const {
> > >   return getCond()->isTypeDependent() || getCond() ->isValueDependent();
> > > }
> > > ```
> > > 
> > > However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ 
> > > property to do with templates ([temp.dep.expr]): it is true if the type 
> > > of the expression could change from one template instantiation to another.
> > > 
> > > Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
> > > property to do with templates ([temp.dep.expr]): it is true for 
> > > value-dependent types in templates.
> > > 
> > > Both will always be false in all instantiations of ChooseExpr, due to the 
> > > language difference.
> > > 
> > > So, I think ChooseExpr can be refactored to remove isConditionDependent 
> > > and change its constructor to remove TypeDependent and ValueDependent.
> > > 
> > > If it's OK with you, I'll do that in a followup patch.
> > > ChooseExpr only represents __builtin_choose_expr which is only valid in 
> > > C, not C++. 
> > 
> > We allow it in C++, though: https://godbolt.org/z/_f1DPV
> Hmm. Is that by design or chance? GCC doesn't allow it: 
> https://godbolt.org/z/kvGCk1
> 
> Maybe it shouldn't be allowed?
Anyway, that means I can add a test; I'll look into doing that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58529: [analyzer] MIGChecker: Enable by default.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The TODOs for future polishing are:

- Add a second `trackExpressionValue()` visitor to track argument values as 
they're copied around.
- Introduce annotations for deallocator functions so that we no longer had to 
hardcode them and users could annotate their consuming functions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58529



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

[PATCH] D58529: [analyzer] MIGChecker: Enable by default.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
NoQ added a comment.

The TODOs for future polishing are:

- Add a second `trackExpressionValue()` visitor to track argument values as 
they're copied around.
- Introduce annotations for deallocator functions so that we no longer had to 
hardcode them and users could annotate their consuming functions.


The checker has already proven to be fairly useful and wanted and doesn't have 
any known false positives, mostly because its positives are usually fairly 
local and there's no room for running into infeasible paths.


Repository:
  rC Clang

https://reviews.llvm.org/D58529

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/test/Analysis/mig.mm


Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,osx.MIG\
 // RUN:   -analyzer-output=text -fblocks -verify %s
 
 typedef unsigned uint32_t;
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -703,6 +703,11 @@
   HelpText<"Check for proper uses of Secure Keychain APIs">,
   Documentation;
 
+def MIGChecker : Checker<"MIG">,
+  HelpText<"Find violations of the Mach Interface Generator "
+   "calling convention">,
+  Documentation;
+
 def ObjCPropertyChecker : Checker<"ObjCProperty">,
   HelpText<"Check for proper uses of Objective-C properties">,
   Documentation;
@@ -811,15 +816,6 @@
   Documentation;
 } // end "optin.performance"
 
-let ParentPackage = OSXAlpha in {
-
-def MIGChecker : Checker<"MIG">,
-  HelpText<"Find violations of the Mach Interface Generator "
-   "calling convention">,
-  Documentation;
-
-} // end "alpha.osx"
-
 let ParentPackage = CocoaAlpha in {
 
 def IvarInvalidationModeling : Checker<"IvarInvalidationModeling">,


Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,osx.MIG\
 // RUN:   -analyzer-output=text -fblocks -verify %s
 
 typedef unsigned uint32_t;
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -703,6 +703,11 @@
   HelpText<"Check for proper uses of Secure Keychain APIs">,
   Documentation;
 
+def MIGChecker : Checker<"MIG">,
+  HelpText<"Find violations of the Mach Interface Generator "
+   "calling convention">,
+  Documentation;
+
 def ObjCPropertyChecker : Checker<"ObjCProperty">,
   HelpText<"Check for proper uses of Objective-C properties">,
   Documentation;
@@ -811,15 +816,6 @@
   Documentation;
 } // end "optin.performance"
 
-let ParentPackage = OSXAlpha in {
-
-def MIGChecker : Checker<"MIG">,
-  HelpText<"Find violations of the Mach Interface Generator "
-   "calling convention">,
-  Documentation;
-
-} // end "alpha.osx"
-
 let ParentPackage = CocoaAlpha in {
 
 def IvarInvalidationModeling : Checker<"IvarInvalidationModeling">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

tmroeder wrote:
> a_sidorin wrote:
> > aaron.ballman wrote:
> > > tmroeder wrote:
> > > > aaron.ballman wrote:
> > > > > a_sidorin wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > > E->isConditionTrue();`
> > > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > > E->isConditionTrue();`?
> > > > > I like that even better than my suggestion. :-)
> > > > Wait, this doesn't have the same truth table as my original code.
> > > > 
> > > > let `CD = E->isConditionDependent()`
> > > > let `CT = E->isConditionTrue()`
> > > > 
> > > > in
> > > > 
> > > > ```
> > > > bool CondIsTrue = false;
> > > > if (!CD)
> > > >   CondIsTrue = CT;
> > > > ```
> > > > 
> > > > has the table for `CondIsTrue`:
> > > > 
> > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > | T | T | F |
> > > > | T | F | F |
> > > > | F | T | T |
> > > > | F | F | F |
> > > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's 
> > > > the value of CT.
> > > > 
> > > > The suggested line has the truth table
> > > > 
> > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > | T | T | T |
> > > > | T | F | F |
> > > > | F | T | F |
> > > > | F | F | F |
> > > > 
> > > > That is, the effect of CD is swapped.
> > > > 
> > > > Aaron's suggestion matches my original table. I based my code on 
> > > > include/clang/AST/Expr.h line 4032, which asserts 
> > > > !isConditionDependent() in the implementation of isConditionTrue.
> > > > 
> > > > I realized this after I "fixed" my comment to match the implementation 
> > > > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > > > think this should be
> > > > 
> > > > ```
> > > > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > > > ```
> > > > 
> > > > I've changed my code to that and reverted the comment change.
> > > Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> > > should add a test case that exercises this?
> > Wow, that's a nice catch. Sorry for the misleading.
> I started to look for a way to test this, then realized that while it's 
> possible to test the code itself, it's impossible to make a ChooseExpr with 
> isConditionDependent() that returns true for real code.
> 
> TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a 
> C++-only condition; it's always false in C.
> 
> Details:
> 
> ChooseExpr only represents __builtin_choose_expr which is only valid in C, 
> not C++. That means ChooseExpr::isConditionDependent() will always be false.
> 
> The definition is
> 
> ```
> bool isConditionDependent() const {
>   return getCond()->isTypeDependent() || getCond() ->isValueDependent();
> }
> ```
> 
> However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ 
> property to do with templates ([temp.dep.expr]): it is true if the type of 
> the expression could change from one template instantiation to another.
> 
> Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
> property to do with templates ([temp.dep.expr]): it is true for 
> value-dependent types in templates.
> 
> Both will always be false in all instantiations of ChooseExpr, due to the 
> language difference.
> 
> So, I think ChooseExpr can be refactored to remove isConditionDependent and 
> change its constructor to remove TypeDependent and ValueDependent.
> 
> If it's OK with you, I'll do that in a followup patch.
> ChooseExpr only represents __builtin_choose_expr which is only valid in C, 
> not C++. 

We allow it in C++, though: https://godbolt.org/z/_f1DPV


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

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

[Artem Dergachev via Phabricator via cfe-commits]

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

Unblock the unrelated MIGChecker patches by untangling them from this core 
change. This patch will land after them and now includes an update to the 
checker that demonstrates the use of (and, well, tests) the new API.

Comments were not addressed yet :)


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

https://reviews.llvm.org/D58367

Files:
  clang/include/clang/Analysis/ProgramPoint.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2461,6 +2461,30 @@
   return nullptr;
 }
 
+int NoteTag::Kind = 0;
+
+void TagVisitor::Profile(llvm::FoldingSetNodeID ) const {
+  static int Tag = 0;
+  ID.AddPointer();
+}
+
+std::shared_ptr
+TagVisitor::VisitNode(const ExplodedNode *N, BugReporterContext ,
+  BugReport ) {
+  ProgramPoint PP = N->getLocation();
+  const NoteTag *T = dyn_cast_or_null(PP.getTag());
+  if (!T)
+return nullptr;
+
+  if (Optional Msg = T->getNote(BRC, R)) {
+PathDiagnosticLocation Loc =
+PathDiagnosticLocation::create(PP, BRC.getSourceManager());
+return std::make_shared(Loc, *Msg);
+  }
+
+  return nullptr;
+}
+
 void FalsePositiveRefutationBRVisitor::Profile(
 llvm::FoldingSetNodeID ) const {
   static int Tag = 0;
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2612,6 +2612,7 @@
 R->addVisitor(llvm::make_unique());
 R->addVisitor(llvm::make_unique());
 R->addVisitor(llvm::make_unique());
+R->addVisitor(llvm::make_unique());
 
 BugReporterContext BRC(Reporter, ErrorGraph.BackMap);
 
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -80,43 +80,10 @@
 checkReturnAux(RS, C);
   }
 
-  class Visitor : public BugReporterVisitor {
-  public:
-void Profile(llvm::FoldingSetNodeID ) const {
-  static int X = 0;
-  ID.AddPointer();
-}
-
-std::shared_ptr VisitNode(const ExplodedNode *N,
-BugReporterContext , BugReport );
-  };
 };
 } // end anonymous namespace
 
-// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
-// specialization for this sort of types.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
-
-std::shared_ptr
-MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext ,
-   BugReport ) {
-  const auto *NewPVD = static_cast(
-  N->getState()->get());
-  const auto *OldPVD = static_cast(
-  N->getFirstPred()->getState()->get());
-  if (OldPVD == NewPVD)
-return nullptr;
-
-  assert(NewPVD && "What is deallocated cannot be un-deallocated!");
-  SmallString<64> Str;
-  llvm::raw_svector_ostream OS(Str);
-  OS << "Value passed through parameter '" << NewPVD->getName()
- << "' is deallocated";
-
-  PathDiagnosticLocation Loc =
-  PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
-  return std::make_shared(Loc, OS.str());
-}
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
 
 static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ) {
   SymbolRef Sym = V.getAsSymbol();
@@ -195,7 +162,14 @@
   if (!PVD)
 return;
 
-  C.addTransition(C.getState()->set(PVD));
+  const NoteTag *T = C.getNoteTag([PVD]() -> std::string {
+SmallString<64> Str;
+llvm::raw_svector_ostream OS(Str);
+OS << "Value passed through parameter '" << PVD->getName()
+   << "\' is deallocated";
+return OS.str();
+  });
+  C.addTransition(C.getState()->set(true), T);
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.
@@ -260,7 +234,6 @@
 
   R->addRange(RS->getSourceRange());
   bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
-  R->addVisitor(llvm::make_unique());
   C.emitReport(std::move(R));
 }
 
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -22,6 +22,7 @@
 #include 

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added a comment.
This revision is now accepted and ready to land.
Herald added a project: clang.

LGTM. Everything still seems to work on my end after applying this diff.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57226



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

[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.

[Artem Dergachev via Phabricator via cfe-commits]

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

Address comments.


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

https://reviews.llvm.org/D58397

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm

Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,20 +1,55 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
 // RUN:   -analyzer-output=text -fblocks -verify %s
 
+typedef unsigned uint32_t;
+
 // XNU APIs.
 
 typedef int kern_return_t;
 #define KERN_SUCCESS 0
 #define KERN_ERROR 1
+#define MIG_NO_REPLY (-305)
 
 typedef unsigned mach_port_name_t;
 typedef unsigned vm_address_t;
 typedef unsigned vm_size_t;
+typedef void *ipc_space_t;
+typedef unsigned long io_user_reference_t;
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+void mig_deallocate(vm_address_t, vm_size_t);
+kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
+// IOKit wrappers.
+
+class OSObject;
+typedef kern_return_t IOReturn;
+#define kIOReturnError 1
+
+enum {
+  kOSAsyncRef64Count = 8,
+};
+
+typedef io_user_reference_t OSAsyncReference64[kOSAsyncRef64Count];
+
+struct IOExternalMethodArguments {
+  io_user_reference_t *asyncReference;
+};
+
+struct IOExternalMethodDispatch {};
+
+class IOUserClient {
+public:
+  static IOReturn releaseAsyncReference64(OSAsyncReference64);
+
+  MIG_SERVER_ROUTINE
+  virtual IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+  IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0);
+};
+
 
 // Tests.
 
@@ -123,3 +158,52 @@
 return Empty{}; // no-crash
   };
 }
+
+// Test various APIs.
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_vm_deallocate(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  mach_vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_port_deallocate(ipc_space_t space,
+mach_port_name_t port) {
+  mach_port_deallocate(space, port); // expected-note{{Value passed through parameter 'port' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mig_deallocate(vm_address_t address, vm_size_t size) {
+  mig_deallocate(address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+// Let's try the C++11 attribute spelling syntax as well.
+[[clang::mig_server_routine]]
+IOReturn test_releaseAsyncReference64(IOExternalMethodArguments *arguments) {
+  IOUserClient::releaseAsyncReference64(arguments->asyncReference); // expected-note{{Value passed through parameter 'arguments' is deallocated}}
+  return kIOReturnError;// expected-warning{{MIG callback fails with error after deallocating argument value}}
+// expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_no_reply(ipc_space_t space, mach_port_name_t port) {
+  mach_port_deallocate(space, port);
+  return MIG_NO_REPLY; // no-warning
+}
+
+class MyClient: public IOUserClient {
+  // The MIG_SERVER_ROUTINE annotation is intentionally skipped.
+  // It should be picked up from the superclass.
+  IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+  IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0) override {
+
+releaseAsyncReference64(arguments->asyncReference); // expected-note{{Value passed through parameter 'arguments' is deallocated}}
+return kIOReturnError;  // expected-warning{{MIG callback fails with error after deallocating argument value}}
+

[PATCH] D58392: [analyzer] MIGChecker: Fix false negatives for releases in automatic destructors.

[Artem Dergachev via Phabricator via cfe-commits]

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

Rebase.


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

https://reviews.llvm.org/D58392

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm


Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -56,6 +56,31 @@
  // expected-note@-1{{MIG callback fails with error after 
deallocating argument value. This is a use-after-free vulnerability because the 
caller will try to deallocate it again}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, 
vm_address_t address, vm_size_t size) {
+  struct WillDeallocate {
+mach_port_name_t port;
+vm_address_t address;
+vm_size_t size;
+~WillDeallocate() {
+  vm_deallocate(port, address, size); // expected-note{{Value passed 
through parameter 'address' is deallocated}}
+}
+  } will_deallocate{port, address, size};
+
+ if (size > 10) {
+// expected-note@-1{{Assuming 'size' is > 10}}
+// expected-note@-2{{Taking true branch}}
+return KERN_ERROR;
+// expected-note@-1{{Calling '~WillDeallocate'}}
+// expected-note@-2{{Returning from '~WillDeallocate'}}
+// expected-warning@-3{{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+// expected-note@-4   {{MIG callback fails with error after deallocating 
argument value. This is a use-after-free vulnerability because the caller will 
try to deallocate it again}}
+  }
+  return KERN_SUCCESS;
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port 
withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -32,15 +32,30 @@
 using namespace ento;
 
 namespace {
-class MIGChecker : public Checker> 
{
+class MIGChecker : public Checker,
+  check::EndFunction> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
  categories::MemoryError};
 
   CallDescription vm_deallocate { "vm_deallocate", 3 };
 
+  void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
+
 public:
   void checkPostCall(const CallEvent , CheckerContext ) const;
-  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+
+  // HACK: We're making two attempts to find the bug: checkEndFunction
+  // should normally be enough but it fails when the return value is a literal
+  // that never gets put into the Environment and ends of function with 
multiple
+  // returns get agglutinated across returns, preventing us from obtaining
+  // the return value. The problem is similar to 
https://reviews.llvm.org/D25326
+  // but now we step into it in the top-level function.
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
+  void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const {
+checkReturnAux(RS, C);
+  }
 
   class Visitor : public BugReporterVisitor {
   public:
@@ -140,7 +155,7 @@
   C.addTransition(C.getState()->set(PVD));
 }
 
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext ) const {
+void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext ) const 
{
   // It is very unlikely that a MIG callback will be called from anywhere
   // within the project under analysis and the caller isn't itself a routine
   // that follows the MIG calling convention. Therefore we're safe to believe


Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -56,6 +56,31 @@
  // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  struct WillDeallocate {
+mach_port_name_t port;
+vm_address_t address;
+vm_size_t size;
+~WillDeallocate() {
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+}
+  } will_deallocate{port, address, size};
+
+ if (size > 10) {
+// expected-note@-1{{Assuming 'size' 

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 187873.
NoQ marked an inline comment as done.
NoQ added a comment.

Address comments.

@Charusso: I agreed not to rush for D58367  
and implemented an old-style visitor here instead :)


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

https://reviews.llvm.org/D58368

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm

Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN:-fblocks -verify %s
+// RUN:   -analyzer-output=text -fblocks -verify %s
 
 // XNU APIs.
 
@@ -20,9 +20,11 @@
 
 MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
-  vm_deallocate(port, address, size);
-  if (size > 10) {
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+   // expected-note@-1{{Taking true branch}}
 return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+			 // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
   }
   return KERN_SUCCESS;
 }
@@ -42,6 +44,18 @@
   vm_deallocate(port, address, size);
 }
 
+// When releasing two parameters, add a note for both of them.
+// Also when returning a variable, explain why do we think that it contains
+// a non-success code.
+MIG_SERVER_ROUTINE
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_address_t addr2, vm_size_t size) {
+  kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+  vm_deallocate(port, addr1, size); // expected-note{{Value passed through parameter 'addr1' is deallocated}}
+  vm_deallocate(port, addr2, size); // expected-note{{Value passed through parameter 'addr2' is deallocated}}
+  return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
@@ -51,8 +65,9 @@
 - (kern_return_t)fooAtPort:(mach_port_name_t)port
withAddress:(vm_address_t)address
 ofSize:(vm_size_t)size MIG_SERVER_ROUTINE {
-  vm_deallocate(port, address, size);
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
   return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+ // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
 }
 @end
 
@@ -60,8 +75,9 @@
   kern_return_t (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
   ^MIG_SERVER_ROUTINE (mach_port_name_t port,
vm_address_t address, vm_size_t size) {
-vm_deallocate(port, address, size);
+vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
     return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+   // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
   };
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -41,19 +41,56 @@
 public:
   void checkPostCall(const CallEvent , CheckerContext ) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
+
+  class Visitor : public BugReporterVisitor {
+  public:
+void Profile(llvm::FoldingSetNodeID ) const {
+  static int X = 0;

[PATCH] D58366: [analyzer] MIGChecker: Make use of the server routine annotation.

[Artem Dergachev via Phabricator via cfe-commits]

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

Rebase.


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

https://reviews.llvm.org/D58366

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm

Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN:-fblocks -verify %s
 
 // XNU APIs.
 
@@ -12,8 +13,12 @@
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 
+#define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
+
+
 // Tests.
 
+MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
   vm_deallocate(port, address, size);
   if (size > 10) {
@@ -22,6 +27,7 @@
   return KERN_SUCCESS;
 }
 
+MIG_SERVER_ROUTINE
 kern_return_t test_unknown_return_value(mach_port_name_t port, vm_address_t address, vm_size_t size) {
   extern kern_return_t foo();
 
@@ -31,6 +37,48 @@
 }
 
 // Make sure we don't crash when they forgot to write the return statement.
+MIG_SERVER_ROUTINE
 kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t size) {
   vm_deallocate(port, address, size);
 }
+
+// Check that we work on Objective-C messages and blocks.
+@interface I
+- (kern_return_t)fooAtPort:(mach_port_name_t)port withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
+@end
+
+@implementation I
+- (kern_return_t)fooAtPort:(mach_port_name_t)port
+   withAddress:(vm_address_t)address
+ofSize:(vm_size_t)size MIG_SERVER_ROUTINE {
+  vm_deallocate(port, address, size);
+  return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+}
+@end
+
+void test_block() {
+  kern_return_t (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
+  ^MIG_SERVER_ROUTINE (mach_port_name_t port,
+   vm_address_t address, vm_size_t size) {
+vm_deallocate(port, address, size);
+    return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+  };
+}
+
+void test_block_with_weird_return_type() {
+  struct Empty {};
+
+  // The block is written within a function so that it was actually analyzed as
+  // a top-level function during analysis. If we were to write it as a global
+  // variable of block type instead, it would not have been analyzed, because
+  // ASTConsumer won't find the block's code body within the VarDecl.
+  // At the same time, we shouldn't call it from the function, because otherwise
+  // it will be analyzed as an inlined function rather than as a top-level
+  // function.
+  Empty (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
+  ^MIG_SERVER_ROUTINE(mach_port_name_t port,
+  vm_address_t address, vm_size_t size) {
+vm_deallocate(port, address, size);
+return Empty{}; // no-crash
+  };
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -20,6 +20,7 @@
 //
 //===--===//
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -55,8 +56,8 @@
  VR->getStackFrame()->inTopFrame();
 }
 
-// This function will probably be replaced with looking up annotations.
-static bool isInMIGCall(const LocationContext *LC) {
+static bool isInMIGCall(CheckerContext ) {
+  const LocationContext *LC = C.getLocationContext();
   const StackFrameContext *SFC;
   // Find the top frame.
   while (LC) {
@@ -64,21 +65,27 @@
 LC = SFC->getParent();
   }
 
-  const auto *FD = dyn_cast(SFC->getDecl());
-  if (!FD)
-return false;
+  const Decl *D = SFC->getDecl();
+
+  if (Optional AC = AnyCall::forDecl(D)) {
+// Even though there's a Sema warning when the return type of an annotated
+// function is not a kern_return_t, this warning isn't an error, so we need
+// an extra sanity check here.
+// FIXME: AnyCall doesn't support blocks yet, so they remain unchecked
+// for now.
+if (!AC->getReturnType(C.getASTContext())
+ .getCanonicalType()->isSignedIntegerType())
+  return false;
+  }
+
+  if (D->hasAttr())
+return true;
 
-  // FIXME: This 

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:47
 
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool);
 

Charusso wrote:
> `;` is not necessary.
Addressed in the earlier patch, D57558.


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

https://reviews.llvm.org/D58368



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

r354633 - Use _Q as MS ABI mangling for char8_t.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Thu Feb 21 15:04:35 2019
New Revision: 354633

URL: http://llvm.org/viewvc/llvm-project?rev=354633=rev
Log:
Use _Q as MS ABI mangling for char8_t.

Thanks to Yuriy Solodkyy for letting us know the mangling here.

Modified:
cfe/trunk/lib/AST/MicrosoftMangle.cpp
cfe/trunk/test/CodeGenCXX/char8_t.cpp

Modified: cfe/trunk/lib/AST/MicrosoftMangle.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/MicrosoftMangle.cpp?rev=354633=354632=354633=diff
==
--- cfe/trunk/lib/AST/MicrosoftMangle.cpp (original)
+++ cfe/trunk/lib/AST/MicrosoftMangle.cpp Thu Feb 21 15:04:35 2019
@@ -1937,8 +1937,9 @@ void MicrosoftCXXNameMangler::mangleType
   // ::= _M # unsigned __int128
   // ::= _N # bool
   // _O # 
-  // ::= _T # __float80 (Intel)
+  // ::= _Q # char8_t
   // ::= _S # char16_t
+  // ::= _T # __float80 (Intel)
   // ::= _U # char32_t
   // ::= _W # wchar_t
   // ::= _Z # __float80 (Digital Mars)
@@ -1999,6 +2000,9 @@ void MicrosoftCXXNameMangler::mangleType
   case BuiltinType::Bool:
 Out << "_N";
 break;
+  case BuiltinType::Char8:
+Out << "_Q";
+break;
   case BuiltinType::Char16:
 Out << "_S";
 break;
@@ -2094,7 +2098,6 @@ void MicrosoftCXXNameMangler::mangleType
   case BuiltinType::SatUShortFract:
   case BuiltinType::SatUFract:
   case BuiltinType::SatULongFract:
-  case BuiltinType::Char8:
   case BuiltinType::Float128: {
 DiagnosticsEngine  = Context.getDiags();
 unsigned DiagID = Diags.getCustomDiagID(

Modified: cfe/trunk/test/CodeGenCXX/char8_t.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/char8_t.cpp?rev=354633=354632=354633=diff
==
--- cfe/trunk/test/CodeGenCXX/char8_t.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/char8_t.cpp Thu Feb 21 15:04:35 2019
@@ -1,9 +1,11 @@
-// RUN: %clang_cc1 -std=c++17 -emit-llvm -fchar8_t -triple x86_64-linux %s -o 
- | FileCheck %s
-// RUN: %clang_cc1 -std=c++17 -emit-llvm -fchar8_t -triple x86_64-windows %s 
-o - -verify
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -fchar8_t -triple x86_64-linux %s -o 
- | FileCheck %s --check-prefix=ITANIUM
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -fchar8_t -triple x86_64-windows %s 
-o - | FileCheck %s --check-prefix=MSABI
 
-// CHECK: define void @_Z1fDu(
-void f(char8_t c) {} // expected-error {{cannot mangle this built-in char8_t 
type yet}}
+// ITANIUM: define void @_Z1fDu(
+// MSABI: define {{.*}}void @"?f@@YAX_Q@Z"(
+void f(char8_t c) {}
 
-// CHECK: define weak_odr void @_Z1gIiEvDTplplcvT__ELA4_KDuELDu114EE
+// ITANIUM: define weak_odr void @_Z1gIiEvDTplplcvT__ELA4_KDuELDu114EE(
+// MSABI: define weak_odr {{.*}}void @"??$g@H@@YAXPEB_Q@Z"(
 template void g(decltype(T() + u8"foo" + u8'r')) {}
 template void g(const char8_t*);


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

[PATCH] D58523: [OpenMP 5.0] Parsing/sema support for to and from clauses with mapper modifier

[Lingda Li via Phabricator via cfe-commits]

lildmh updated this revision to Diff 187869.
lildmh marked 3 inline comments as done.

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

https://reviews.llvm.org/D58523

Files:
  include/clang/AST/OpenMPClause.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/OpenMPKinds.def
  include/clang/Basic/OpenMPKinds.h
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/AST/OpenMPClause.cpp
  lib/Basic/OpenMPKinds.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/OpenMP/declare_mapper_ast_print.c
  test/OpenMP/declare_mapper_ast_print.cpp
  test/OpenMP/declare_mapper_codegen.cpp
  test/OpenMP/declare_mapper_messages.c
  test/OpenMP/declare_mapper_messages.cpp

Index: test/OpenMP/declare_mapper_messages.cpp
===
--- test/OpenMP/declare_mapper_messages.cpp
+++ test/OpenMP/declare_mapper_messages.cpp
@@ -29,7 +29,7 @@
 #pragma omp declare mapper(bb: vec v) private(v)// expected-error {{expected at least one clause on '#pragma omp declare mapper' directive}} // expected-error {{unexpected OpenMP clause 'private' in directive '#pragma omp declare mapper'}}
 #pragma omp declare mapper(cc: vec v) map(v) (  // expected-warning {{extra tokens at the end of '#pragma omp declare mapper' are ignored}}
 
-#pragma omp declare mapper(++: vec v) map(v.len)// expected-error {{illegal identifier on 'omp declare mapper' directive}}
+#pragma omp declare mapper(++: vec v) map(v.len)// expected-error {{illegal OpenMP user-defined mapper identifier}}
 #pragma omp declare mapper(id1: vec v) map(v.len, temp) // expected-error {{only variable v is allowed in map clauses of this 'omp declare mapper' directive}}
 #pragma omp declare mapper(default : vec kk) map(kk.data[0:2])  // expected-note {{previous definition is here}}
 #pragma omp declare mapper(vec v) map(v.len)// expected-error {{redefinition of user-defined mapper for type 'vec' with name 'default'}}
@@ -74,9 +74,9 @@
   {}
 #pragma omp target map(mapper(ab) :vv)  // expected-error {{missing map type}} expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'ab'}}
   {}
-#pragma omp target map(mapper(N2::) :vv)// expected-error {{use of undeclared identifier 'N2'}} expected-error {{illegal identifier on 'omp declare mapper' directive}}
+#pragma omp target map(mapper(N2::) :vv)// expected-error {{use of undeclared identifier 'N2'}} expected-error {{illegal OpenMP user-defined mapper identifier}}
   {}
-#pragma omp target map(mapper(N1::) :vv)// expected-error {{illegal identifier on 'omp declare mapper' directive}}
+#pragma omp target map(mapper(N1::) :vv)// expected-error {{illegal OpenMP user-defined mapper identifier}}
   {}
 #pragma omp target map(mapper(aa) :vv)  // expected-error {{missing map type}}
   {}
@@ -86,6 +86,32 @@
   {}
 #pragma omp target map(mapper(N1::stack::id) to:vv)
   {}
+
+#pragma omp target update to(mapper)// expected-error {{expected '(' after 'mapper'}} expected-error {{expected expression}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper()   // expected-error {{illegal OpenMP user-defined mapper identifier}} expected-error {{expected expression}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper:vv) // expected-error {{expected '(' after 'mapper'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper(:vv)// expected-error {{illegal OpenMP user-defined mapper identifier}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper(aa :vv) // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper(N2:: :vv)   // expected-error {{use of undeclared identifier 'N2'}} expected-error {{illegal OpenMP user-defined mapper identifier}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}

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

[Jan Korous via Phabricator via cfe-commits]

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

Fixed closing of FD.


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

https://reviews.llvm.org/D58418



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

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406412 , @MyDeveloperDay 
wrote:

> In D57896#1406407 , @zturner wrote:
>
> > If I read the post correctly, it was actually agreeing with me (because it 
> > said "to reinforce your point...".  Meaning that something such as 
> > `lowerCaseCamel` would be the third style being referred to
>
>
> Correct! just acknowledging your point from a different perspective.


Doh!  Sorry for the noise.

It looks like the RFC thread has mostly turned into a transition-plan debate, 
so should we work on the actual convention description here? Extracting the 
naming conventions from the 3.6-era link mentioned above, we have:

- types and classes are UpperCamelCase (this is unchanged from current LLVM 
style)
- methods are UpperCamelCase (this is also the old LLVM style IIRC)
- variables are snake_case
- static data members add "g_" prefix to variable style (although I see a 
proposal for "s_" instead)
- nonstatic data members add "m_" prefix to variable style

Did I miss anything really important?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

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

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187868.
jkorous added a comment.

- handle errors returned by close


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf 

[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:125
 
+  // See if there's an annotated method in the superclass.
+  if (const auto *MD = dyn_cast(D))

dcoughlin wrote:
> Perhaps we could make it a Sema error if a method doesn't have a mig server 
> routine annotation but it overrides a method that does. From a 
> documentation/usability perspective it would be unfortunate if a human 
> looking at a method to determine its convention would have to look at its 
> super classes to see whether they have an annotation too.
> 
> Although, thinking about it more that might make it a source-breaking change 
> if an API author were to add annotation on a super class. Then API clients 
> would have to add the annotations to get their projects to build, which would 
> not be great..
Yup, i believe that given that the checker is an optional thing (even if we 
make it opt-out the hard way), annotating APIs should also be optional. The 
primary use case for this override trick is the 
`IOUserClient::externalMethod()` API that gets annotated within IOKit itself 
and makes the attribute automatically apply to all overrides in all IOKit 
projects, automagically making them subject to testing. But if we make it an 
error to not annotate the overrides, it'd break every single IOKit project and 
require them to add dozens of annotations before it compiles, so i think it's 
not feasible.

We might as well hardcode the `IOUserClient::externalMethod()` thing (instead 
of annotating it) and in this case it would make much more sense to enforce 
fully annotating all overrides.


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

https://reviews.llvm.org/D58397



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

[PATCH] D18360: Add AIX Target/ToolChain to Clang Driver

[Andrew Paprocki via Phabricator via cfe-commits]

apaprocki added a comment.

@hubert.reinterpretcast Yes, this patch is available under the new license.


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

https://reviews.llvm.org/D18360



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

[PATCH] D57716: [CUDA][HIP] Check calling convention based on function target

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: test/SemaCUDA/amdgpu-windows-vectorcall.cu:3
+
+// expected-no-diagnostics
+template

tra wrote:
> It may be good to add a check where we *would* expect to see the diagnostics.
> It may be good to add a check where we *would* expect to see the diagnostics.

Scratch that.  Wrong calling convention attribute will be ignored. 



Comment at: test/SemaCUDA/amdgpu-windows-vectorcall.cu:9-10
+
+template struct A<_Ret (__cdecl 
_Arg0::*)(_Types) > { };
+template struct A<_Ret (__vectorcall 
_Arg0::*)(_Types) > {};
+

I don't think we need templates here. We're only making sure that the function 
attributes are handled correctly.
Can we get by with a regular function declaration or definition?




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

https://reviews.llvm.org/D57716



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

[PATCH] D58523: [OpenMP 5.0] Parsing/sema support for to and from clauses with mapper modifier

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 12 inline comments as done.
lildmh added inline comments.



Comment at: lib/AST/OpenMPClause.cpp:1469
 
 void OMPClausePrinter::VisitOMPFromClause(OMPFromClause *Node) {
   if (!Node->varlist_empty()) {

ABataev wrote:
> Better to split this patch into 2: one for `to` clause and another one for 
> `from` clause
Hi Alexey,

Thanks for the quick review! The code for `to` and `from` clauses are almost 
identical, so I prefer to have them in the same patch. Besides I don't have 
commit privilege, so it causes more time to me to have multiple patches. But if 
you think it's crucial to split it. I can do it. What do you think?



Comment at: lib/Sema/SemaOpenMP.cpp:13061
 MapperIdScopeSpec.getWithLocInContext(SemaRef.Context), MapperId,
-/*ADL=*/false, /*Overloaded=*/true, URS.begin(), URS.end());
+/*ADL=*/true, /*Overloaded=*/true, URS.begin(), URS.end());
   }

ABataev wrote:
> Better to fix this in a separate patch.
I can do that.



Comment at: lib/Sema/SemaOpenMP.cpp:13356
 
-  // Try to find the associated user-defined mapper.
-  ExprResult ER = buildUserDefinedMapperRef(
-  SemaRef, DSAS->getCurScope(), *MapperIdScopeSpec, *MapperId,
-  Type.getCanonicalType(), UnresolvedMapper);
-  if (ER.isInvalid())
-continue;
-  MVLI.UDMapperList.push_back(ER.get());
-} else {
-  MVLI.UDMapperList.push_back(nullptr);
 }
 

ABataev wrote:
> Are you sure about this change? This might trigger some asserts.
Yes. The original code before the mapper patch is like this. By moving it out, 
`to` and `from` clauses will also call `buildUserDefinedMapperRef` check the 
mapper as well.



Comment at: lib/Sema/SemaOpenMP.cpp:13413
 
-  // If the identifier of user-defined mapper is not specified, it is 
"default".
-  if (!MapperId.getName() || MapperId.getName().isEmpty()) {

ABataev wrote:
> Again, better to do this in a separate patch
This modification is for `to` and `from` clauses, so they also have a default 
mapper name. I think it is appropriate to have this piece in this patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58523



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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Tom Roeder via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

a_sidorin wrote:
> aaron.ballman wrote:
> > tmroeder wrote:
> > > aaron.ballman wrote:
> > > > a_sidorin wrote:
> > > > > aaron.ballman wrote:
> > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > E->isConditionTrue();`
> > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > E->isConditionTrue();`?
> > > > I like that even better than my suggestion. :-)
> > > Wait, this doesn't have the same truth table as my original code.
> > > 
> > > let `CD = E->isConditionDependent()`
> > > let `CT = E->isConditionTrue()`
> > > 
> > > in
> > > 
> > > ```
> > > bool CondIsTrue = false;
> > > if (!CD)
> > >   CondIsTrue = CT;
> > > ```
> > > 
> > > has the table for `CondIsTrue`:
> > > 
> > > | `CD` | `CT` |  `CondIsTrue` |
> > > | T | T | F |
> > > | T | F | F |
> > > | F | T | T |
> > > | F | F | F |
> > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> > > value of CT.
> > > 
> > > The suggested line has the truth table
> > > 
> > > | `CD` | `CT` |  `CondIsTrue` |
> > > | T | T | T |
> > > | T | F | F |
> > > | F | T | F |
> > > | F | F | F |
> > > 
> > > That is, the effect of CD is swapped.
> > > 
> > > Aaron's suggestion matches my original table. I based my code on 
> > > include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() 
> > > in the implementation of isConditionTrue.
> > > 
> > > I realized this after I "fixed" my comment to match the implementation 
> > > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > > think this should be
> > > 
> > > ```
> > > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > > ```
> > > 
> > > I've changed my code to that and reverted the comment change.
> > Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> > should add a test case that exercises this?
> Wow, that's a nice catch. Sorry for the misleading.
I started to look for a way to test this, then realized that while it's 
possible to test the code itself, it's impossible to make a ChooseExpr with 
isConditionDependent() that returns true for real code.

TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a C++-only 
condition; it's always false in C.

Details:

ChooseExpr only represents __builtin_choose_expr which is only valid in C, not 
C++. That means ChooseExpr::isConditionDependent() will always be false.

The definition is

```
bool isConditionDependent() const {
  return getCond()->isTypeDependent() || getCond() ->isValueDependent();
}
```

However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ property 
to do with templates ([temp.dep.expr]): it is true if the type of the 
expression could change from one template instantiation to another.

Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
property to do with templates ([temp.dep.expr]): it is true for value-dependent 
types in templates.

Both will always be false in all instantiations of ChooseExpr, due to the 
language difference.

So, I think ChooseExpr can be refactored to remove isConditionDependent and 
change its constructor to remove TypeDependent and ValueDependent.

If it's OK with you, I'll do that in a followup patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Jan Korous via Phabricator via cfe-commits]

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

Done.


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

https://reviews.llvm.org/D58478



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187863.

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

https://reviews.llvm.org/D58478

Files:
  clang/include/clang/Index/DeclOccurrence.h
  clang/lib/Index/CMakeLists.txt
  clang/lib/Index/FileIndexRecord.cpp
  clang/lib/Index/FileIndexRecord.h

Index: clang/lib/Index/FileIndexRecord.h
===
--- /dev/null
+++ clang/lib/Index/FileIndexRecord.h
@@ -0,0 +1,57 @@
+//===--- FileIndexRecord.h - Index data per file *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
+#define LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Index/DeclOccurrence.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallVector.h"
+#include 
+
+namespace clang {
+class IdentifierInfo;
+
+namespace index {
+
+/// Stores the declaration occurrences seen in a particular source or header
+/// file of a translation unit
+class FileIndexRecord {
+private:
+  FileID FID;
+  bool IsSystem;
+  std::vector Decls;
+
+public:
+  FileIndexRecord(FileID FID, bool IsSystem) : FID(FID), IsSystem(IsSystem) {}
+
+  ArrayRef getDeclOccurrencesSortedByOffset() const {
+return Decls;
+  }
+
+  FileID getFileID() const { return FID; }
+  bool isSystem() const { return IsSystem; }
+
+  /// Adds an occurrence of the canonical declaration \c D at the supplied
+  /// \c Offset
+  ///
+  /// \param Roles the roles the occurrence fulfills in this position.
+  /// \param Offset the offset in the file of this occurrence.
+  /// \param D the canonical declaration this is an occurrence of.
+  /// \param Relations the set of symbols related to this occurrence.
+  void addDeclOccurence(SymbolRoleSet Roles, unsigned Offset, const Decl *D,
+ArrayRef Relations);
+  void print(llvm::raw_ostream ) const;
+};
+
+} // end namespace index
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
Index: clang/lib/Index/FileIndexRecord.cpp
===
--- /dev/null
+++ clang/lib/Index/FileIndexRecord.cpp
@@ -0,0 +1,59 @@
+//===--- FileIndexRecord.cpp - Index data per file --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FileIndexRecord.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Path.h"
+
+using namespace clang;
+using namespace clang::index;
+
+void FileIndexRecord::addDeclOccurence(SymbolRoleSet Roles, unsigned Offset,
+   const Decl *D,
+   ArrayRef Relations) {
+  assert(D->isCanonicalDecl() &&
+ "Occurrences should be associated with their canonical decl");
+
+  auto IsNextOccurence = [&]() -> bool {
+if (Decls.empty())
+  return true;
+auto  = Decls.back();
+return Last.Offset < Offset;
+  };
+
+  if (IsNextOccurence()) {
+Decls.emplace_back(Roles, Offset, D, Relations);
+return;
+  }
+
+  DeclOccurrence NewInfo(Roles, Offset, D, Relations);
+  auto It = std::upper_bound(Decls.begin(), Decls.end(), NewInfo);
+  Decls.insert(It, std::move(NewInfo));
+}
+
+void FileIndexRecord::print(llvm::raw_ostream ) const {
+  OS << "DECLS BEGIN ---\n";
+  for (auto  : Decls) {
+const Decl *D = DclInfo.Dcl;
+SourceManager  = D->getASTContext().getSourceManager();
+SourceLocation Loc = SM.getFileLoc(D->getLocation());
+PresumedLoc PLoc = SM.getPresumedLoc(Loc);
+OS << llvm::sys::path::filename(PLoc.getFilename()) << ':' << PLoc.getLine()
+   << ':' << PLoc.getColumn();
+
+if (auto ND = dyn_cast(D)) {
+  OS << ' ' << ND->getNameAsString();
+}
+
+OS << '\n';
+  }
+  OS << "DECLS END ---\n";
+}
Index: clang/lib/Index/CMakeLists.txt
===
--- clang/lib/Index/CMakeLists.txt
+++ clang/lib/Index/CMakeLists.txt
@@ -6,6 +6,7 @@
 add_clang_library(clangIndex
   CodegenNameGenerator.cpp
   CommentToXML.cpp
+  FileIndexRecord.cpp
   IndexBody.cpp
   IndexDecl.cpp
   IndexingAction.cpp
Index: clang/include/clang/Index/DeclOccurrence.h
===
--- /dev/null
+++ clang/include/clang/Index/DeclOccurrence.h
@@ -0,0 +1,41 @@
+//===- DeclOccurrence.h - An occurrence of a 

[PATCH] D57112: [ASTTypeTraits] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Maybe ping @klimek then? :)

In D57112#1400046 , @alexfh wrote:

> In D57112#1399516 , @lebedev.ri 
> wrote:
>
> > Ping @hokein / @alexfh (as per git blame).
> >  Not sure who is best suited to review this.
>
>
> I only made a couple of random fixes to these files, so I don't feel 
> particularly competent to review them. Though overall the changes in this 
> patch seem reasonable to me. No concerns from me ;)


Makes sense, thanks for looking!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57896#1406407 , @zturner wrote:

> If I read the post correctly, it was actually agreeing with me (because it 
> said "to reinforce your point...".  Meaning that something such as 
> `lowerCaseCamel` would be the third style being referred to


Correct! just acknowledging your point from a different perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1406401 , @probinson wrote:

> In D57896#1406353 , @MyDeveloperDay 
> wrote:
>
> > I can't argue with anything you say.. but I guess to reinforce your point 
> > introducing what is effectively a 3rd style would likely cause even more 
> > jarring...
>
>
> Zach isn't introducing a new style, the style already exists and is 
> consistently used by what I think is our 3rd largest subproject.  It happens 
> not to be used at all by the two largest subprojects, but those subprojects 
> already aren't consistent with themselves.
>  I would not mind a more concerted effort to migrate to whatever style we 
> pick, which was notably lacking last time around. Then the jarring 
> inconsistencies would go away, and we could all get back to complaining about 
> content and not style.


If I read the post correctly, it was actually agreeing with me (because it said 
"to reinforce your point...".  Meaning that something such as `lowerCaseCamel` 
would be the third style being referred to, while my proposal keeps the number 
of styles to 2.  But, maybe I read it wrong.  If I read it right, then 
obviously I agree :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57716: [CUDA][HIP] Check calling convention based on function target

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 187862.
yaxunl added a comment.
Herald added a subscriber: jdoerfert.

Revised by Artem's comments.


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

https://reviews.llvm.org/D57716

Files:
  lib/Sema/SemaDeclAttr.cpp
  test/SemaCUDA/amdgpu-windows-vectorcall.cu


Index: test/SemaCUDA/amdgpu-windows-vectorcall.cu
===
--- /dev/null
+++ test/SemaCUDA/amdgpu-windows-vectorcall.cu
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple 
x86_64-pc-windows-msvc -fms-compatibility -fcuda-is-device -fsyntax-only 
-verify %s
+
+// expected-no-diagnostics
+template
+ struct A
+ {
+ };
+
+template struct A<_Ret (__cdecl 
_Arg0::*)(_Types) > { };
+template struct A<_Ret (__vectorcall 
_Arg0::*)(_Types) > {};
+
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4615,8 +4615,36 @@
   default: llvm_unreachable("unexpected attribute kind");
   }
 
+  TargetInfo::CallingConvCheckResult A = TargetInfo::CCCR_OK;
   const TargetInfo  = Context.getTargetInfo();
-  TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC);
+  auto *Aux = Context.getAuxTargetInfo();
+  if (LangOpts.CUDA) {
+auto CudaTarget = IdentifyCUDATarget(FD);
+bool CheckHost = false, CheckDevice = false;
+switch (CudaTarget) {
+case CFT_HostDevice:
+  CheckHost = true;
+  CheckDevice = true;
+  break;
+case CFT_Host:
+  CheckHost = true;
+  break;
+case CFT_Device:
+case CFT_Global:
+  CheckDevice = true;
+  break;
+case CFT_InvalidTarget:
+  llvm_unreachable("unexpected cuda target");
+}
+auto *HostTI = LangOpts.CUDAIsDevice ? Aux : 
+auto *DeviceTI = LangOpts.CUDAIsDevice ?  : Aux;
+if (CheckHost && HostTI)
+  A = HostTI->checkCallingConvention(CC);
+if (A == TargetInfo::CCCR_OK && CheckDevice && DeviceTI)
+  A = DeviceTI->checkCallingConvention(CC);
+  } else {
+A = TI.checkCallingConvention(CC);
+  }
   if (A != TargetInfo::CCCR_OK) {
 if (A == TargetInfo::CCCR_Warning)
   Diag(Attrs.getLoc(), diag::warn_cconv_ignored) << Attrs;


Index: test/SemaCUDA/amdgpu-windows-vectorcall.cu
===
--- /dev/null
+++ test/SemaCUDA/amdgpu-windows-vectorcall.cu
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-pc-windows-msvc -fms-compatibility -fcuda-is-device -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+template
+ struct A
+ {
+ };
+
+template struct A<_Ret (__cdecl _Arg0::*)(_Types) > { };
+template struct A<_Ret (__vectorcall _Arg0::*)(_Types) > {};
+
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4615,8 +4615,36 @@
   default: llvm_unreachable("unexpected attribute kind");
   }
 
+  TargetInfo::CallingConvCheckResult A = TargetInfo::CCCR_OK;
   const TargetInfo  = Context.getTargetInfo();
-  TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC);
+  auto *Aux = Context.getAuxTargetInfo();
+  if (LangOpts.CUDA) {
+auto CudaTarget = IdentifyCUDATarget(FD);
+bool CheckHost = false, CheckDevice = false;
+switch (CudaTarget) {
+case CFT_HostDevice:
+  CheckHost = true;
+  CheckDevice = true;
+  break;
+case CFT_Host:
+  CheckHost = true;
+  break;
+case CFT_Device:
+case CFT_Global:
+  CheckDevice = true;
+  break;
+case CFT_InvalidTarget:
+  llvm_unreachable("unexpected cuda target");
+}
+auto *HostTI = LangOpts.CUDAIsDevice ? Aux : 
+auto *DeviceTI = LangOpts.CUDAIsDevice ?  : Aux;
+if (CheckHost && HostTI)
+  A = HostTI->checkCallingConvention(CC);
+if (A == TargetInfo::CCCR_OK && CheckDevice && DeviceTI)
+  A = DeviceTI->checkCallingConvention(CC);
+  } else {
+A = TI.checkCallingConvention(CC);
+  }
   if (A != TargetInfo::CCCR_OK) {
 if (A == TargetInfo::CCCR_Warning)
   Diag(Attrs.getLoc(), diag::warn_cconv_ignored) << Attrs;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406353 , @MyDeveloperDay 
wrote:

> I can't argue with anything you say.. but I guess to reinforce your point 
> introducing what is effectively a 3rd style would likely cause even more 
> jarring...


Zach isn't introducing a new style, the style already exists and is 
consistently used by what I think is our 3rd largest subproject.  It happens 
not to be used at all by the two largest subprojects, but those subprojects 
already aren't consistent with themselves.
I would not mind a more concerted effort to migrate to whatever style we pick, 
which was notably lacking last time around. Then the jarring inconsistencies 
would go away, and we could all get back to complaining about content and not 
style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

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

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for taking a look Michał! I'll try to address the rest of your comments 
asap.


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

https://reviews.llvm.org/D58418



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

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

[Jan Korous via Phabricator via cfe-commits]

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

- handle EINTR
- assert we are reading whole INotifyEvents


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef 

r354626 - [clang-cl] Whitelist -fbracket-depth=123 in clang-cl

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Thu Feb 21 13:53:12 2019
New Revision: 354626

URL: http://llvm.org/viewvc/llvm-project?rev=354626=rev
Log:
[clang-cl] Whitelist -fbracket-depth=123 in clang-cl

Users apparently need it when expanding large quantities of macros.

Fixes PR38685

Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/test/Driver/cl-options.c

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=354626=354625=354626=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Thu Feb 21 13:53:12 2019
@@ -901,7 +901,7 @@ def fno_fast_math : Flag<["-"], "fno-fas
 def fmath_errno : Flag<["-"], "fmath-errno">, Group, 
Flags<[CC1Option]>,
   HelpText<"Require math functions to indicate errors by setting errno">;
 def fno_math_errno : Flag<["-"], "fno-math-errno">, Group;
-def fbracket_depth_EQ : Joined<["-"], "fbracket-depth=">, Group;
+def fbracket_depth_EQ : Joined<["-"], "fbracket-depth=">, Group, 
Flags<[CoreOption]>;
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 def fjump_tables : Flag<["-"], "fjump-tables">, Group;

Modified: cfe/trunk/test/Driver/cl-options.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=354626=354625=354626=diff
==
--- cfe/trunk/test/Driver/cl-options.c (original)
+++ cfe/trunk/test/Driver/cl-options.c Thu Feb 21 13:53:12 2019
@@ -623,6 +623,7 @@
 // RUN: -fmerge-all-constants \
 // RUN: -no-canonical-prefixes \
 // RUN: -march=skylake \
+// RUN: -fbracket-depth=123 \
 // RUN: --version \
 // RUN: -Werror /Zs -- %s 2>&1
 


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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

aaron.ballman wrote:
> tmroeder wrote:
> > aaron.ballman wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > E->isConditionTrue();`
> > > > `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
> > > I like that even better than my suggestion. :-)
> > Wait, this doesn't have the same truth table as my original code.
> > 
> > let `CD = E->isConditionDependent()`
> > let `CT = E->isConditionTrue()`
> > 
> > in
> > 
> > ```
> > bool CondIsTrue = false;
> > if (!CD)
> >   CondIsTrue = CT;
> > ```
> > 
> > has the table for `CondIsTrue`:
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | F |
> > | T | F | F |
> > | F | T | T |
> > | F | F | F |
> > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> > value of CT.
> > 
> > The suggested line has the truth table
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | T |
> > | T | F | F |
> > | F | T | F |
> > | F | F | F |
> > 
> > That is, the effect of CD is swapped.
> > 
> > Aaron's suggestion matches my original table. I based my code on 
> > include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() 
> > in the implementation of isConditionTrue.
> > 
> > I realized this after I "fixed" my comment to match the implementation 
> > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > think this should be
> > 
> > ```
> > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > ```
> > 
> > I've changed my code to that and reverted the comment change.
> Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> should add a test case that exercises this?
Wow, that's a nice catch. Sorry for the misleading.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Oh and I think that you will also have to update the serialization 
code/de-serialization code in `ASTReaderDecl.cpp` / `ASTWriterDecl.cpp`. You 
might also have to update `TreeTransform` but I am less familiar with this.


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

https://reviews.llvm.org/D58035



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

[PATCH] D58526: AMDGPU: Don't emit debugger subtarget features

[Matt Arsenault via Phabricator via cfe-commits]

arsenm closed this revision.
arsenm added a comment.

r354624


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

https://reviews.llvm.org/D58526



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

r354624 - AMDGPU: Don't emit debugger subtarget features

[Matt Arsenault via cfe-commits]

Author: arsenm
Date: Thu Feb 21 13:31:43 2019
New Revision: 354624

URL: http://llvm.org/viewvc/llvm-project?rev=354624=rev
Log:
AMDGPU: Don't emit debugger subtarget features

Keep the flag around for compatability.

Modified:
cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp
cfe/trunk/test/Driver/amdgpu-features.c

Modified: cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp?rev=354624=354623=354624=diff
==
--- cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp Thu Feb 21 13:31:43 2019
@@ -38,15 +38,8 @@ void amdgpu::Linker::ConstructJob(Compil
 void amdgpu::getAMDGPUTargetFeatures(const Driver ,
  const llvm::opt::ArgList ,
  std::vector ) {
-  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
-StringRef value = dAbi->getValue();
-if (value == "1.0") {
-  Features.push_back("+amdgpu-debugger-insert-nops");
-  Features.push_back("+amdgpu-debugger-emit-prologue");
-} else {
-  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
-}
-  }
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi))
+D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
 
   handleTargetFeaturesGroup(
 Args, Features, options::OPT_m_amdgpu_Features_Group);

Modified: cfe/trunk/test/Driver/amdgpu-features.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/amdgpu-features.c?rev=354624=354623=354624=diff
==
--- cfe/trunk/test/Driver/amdgpu-features.c (original)
+++ cfe/trunk/test/Driver/amdgpu-features.c Thu Feb 21 13:31:43 2019
@@ -4,7 +4,7 @@
 
 // RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=1.0'
 
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | 
FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: "-target-feature" "+code-object-v3"


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

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57896#1406336 , @zturner wrote:

>


...

I can't argue with anything you say.. but I guess to reinforce your point 
introducing what is effectively a 3rd style would likely cause even more 
jarring...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang/include/clang/Index/DeclOccurrence.h:1
+//===--- DeclOccurrence.h - An occurrence of a decl within a file - C++ 
-*-===//
+//

Tag should contain *- C++ -*, so you could remove dashes at beginning and equal 
signs to fit.


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

https://reviews.llvm.org/D58478



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

[PATCH] D58477: [Driver] Fix float ABI default for Android ARMv8.

[Dan Albert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354622: [Driver] Fix float ABI default for Android ARMv8. 
(authored by danalbert, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58477?vs=187673=187851#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58477

Files:
  cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
  cfe/trunk/test/Driver/arm-float-abi.c


Index: cfe/trunk/test/Driver/arm-float-abi.c
===
--- cfe/trunk/test/Driver/arm-float-abi.c
+++ cfe/trunk/test/Driver/arm-float-abi.c
@@ -4,3 +4,13 @@
 
 // ARMV7-ERROR: unsupported option '-mfloat-abi=hard' for target 'thumbv7'
 // NOERROR-NOT: unsupported option
+
+// RUN: %clang -target armv7-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM7-ANDROID %s
+// CHECK-ARM7-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM7-ANDROID: "-target-feature" "+soft-float-abi"
+
+// RUN: %clang -target armv8-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM8-ANDROID %s
+// CHECK-ARM8-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM8-ANDROID: "-target-feature" "+soft-float-abi"
Index: cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -248,7 +248,7 @@
 ABI = FloatABI::SoftFP;
 break;
   case llvm::Triple::Android:
-ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
+ABI = (SubArch >= 7) ? FloatABI::SoftFP : FloatABI::Soft;
 break;
   default:
 // Assume "soft", but warn the user we are guessing.


Index: cfe/trunk/test/Driver/arm-float-abi.c
===
--- cfe/trunk/test/Driver/arm-float-abi.c
+++ cfe/trunk/test/Driver/arm-float-abi.c
@@ -4,3 +4,13 @@
 
 // ARMV7-ERROR: unsupported option '-mfloat-abi=hard' for target 'thumbv7'
 // NOERROR-NOT: unsupported option
+
+// RUN: %clang -target armv7-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM7-ANDROID %s
+// CHECK-ARM7-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM7-ANDROID: "-target-feature" "+soft-float-abi"
+
+// RUN: %clang -target armv8-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM8-ANDROID %s
+// CHECK-ARM8-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM8-ANDROID: "-target-feature" "+soft-float-abi"
Index: cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -248,7 +248,7 @@
 ABI = FloatABI::SoftFP;
 break;
   case llvm::Triple::Android:
-ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
+ABI = (SubArch >= 7) ? FloatABI::SoftFP : FloatABI::Soft;
 break;
   default:
 // Assume "soft", but warn the user we are guessing.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

If this bit is relevant to function parameters, why is `getIsArgumentModified` 
in `VarDecl` and not in `ParamVarDecl` ? What you can do is move the relevant 
methods to `ParamVarDecl`, and stash the bit in `ParmVarDeclBitfields`.




Comment at: include/clang/AST/Decl.h:843
 
+  mutable bool IsArgumentModified = false;
+

Yeah, please don't waste a whole pointer just to store a bit.


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

https://reviews.llvm.org/D58035



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1405334 , @MyDeveloperDay 
wrote:

> In D57896#1402280 , @zturner wrote:
>
> > Since someone already accepted this, I suppose I should mark require 
> > changes to formalize my dissent
>
>
> As it was Chris @lattner who accepted it, is your request for changes just 
> based on the fact that it doesn't fit LLDB style?


(Side note, but I think everyones' opinions hold the same weight with regards 
to issues like this, and that is in part why changes like this are so difficult 
to move forward with. Because it takes a lot of consensus, not just one person, 
to drive a change.)

To answer your question: In a way, yes.  To be clear, I don't actually care 
what the style we end up with is and I think arguing over which specific style 
we end up adopting is a silly argument.  No style is going to be aesthetically 
pleasing to everyone, and I conjecture that any style we choose will have just 
as many people who dislike it as there are that like it.  A coding style should 
serve exactly two purposes (in this order of importance): 1) Consistency across 
codebase, and 2) Visually distinguish semantically names that refer to 
semantically different things.

As long as it satisfies those two things, the specific choice of style is 
almost incosequential.

My objection is based on the fact adopting LLDB's style makes #1 
**significantly better** at literally no incremental cost, while maintaining 
#2.  So, the benefit of changing to literally any other style would be dwarfed 
by the benefit of changing to this particular style, because we would get 
instant consistency across a large swath of code.

If someone wants to propose a mass change of LLDB's names, I would actually be 
fine with that, but I suspect that will be just as difficult to drive, and so 
the path of least resistance here is to just use it and move on with our lives.

> I was trying to find where the LLDB coding style was documented but could 
> only find this 
> https://llvm.org/svn/llvm-project/lldb/branches/release_36/www/lldb-coding-conventions.html,
>  seemingly this file has been move/removed around release 3.9.
> 
> But reading that link its seems unlikely to find a concencous between either 
> naming conventions or formatting style between LLDB and the rest of LLVM, 
> unless of course the solution would be to adopt LLDB style completely (for 
> which I'd suspect there would be counter objections)

If there are counter objections, I'd like to hear them.  "I'm not a fan of that 
style" is not really a strong counter-objection in my opinion, because if we 
require a unanimous consensus on the most aesthetically pleasing style, I'm 
pretty sure nothing will ever happen.  After all, I'm not a huge fan of LLDB's 
style myself.  But as with any coding standard, you just deal with it.

> If that isn't a reality is blocking the rest of the LLVM community from 
> relieving some of their eye strain an acceptable solution?

Inconsistency is worse than eye strain, because it *causes* eye strain, as well 
as discourages people from contributing to the code at all.  Anyone who has 
worked on both LLDB and LLVM can attest to how jarring the shift is moving back 
and forth between them, and that is a much more serious problem than a subset 
of developers who don't like something and another subset who do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

r354622 - [Driver] Fix float ABI default for Android ARMv8.

[Dan Albert via cfe-commits]

Author: danalbert
Date: Thu Feb 21 13:13:03 2019
New Revision: 354622

URL: http://llvm.org/viewvc/llvm-project?rev=354622=rev
Log:
[Driver] Fix float ABI default for Android ARMv8.

Summary: Android doesn't regress back to soft float after ARMv7 :)

Reviewers: srhines, pirama

Reviewed By: srhines, pirama

Subscribers: javed.absar, kristof.beyls, cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
cfe/trunk/test/Driver/arm-float-abi.c

Modified: cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp?rev=354622=354621=354622=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Arch/ARM.cpp Thu Feb 21 13:13:03 2019
@@ -248,7 +248,7 @@ arm::FloatABI arm::getARMFloatABI(const
 ABI = FloatABI::SoftFP;
 break;
   case llvm::Triple::Android:
-ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
+ABI = (SubArch >= 7) ? FloatABI::SoftFP : FloatABI::Soft;
 break;
   default:
 // Assume "soft", but warn the user we are guessing.

Modified: cfe/trunk/test/Driver/arm-float-abi.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/arm-float-abi.c?rev=354622=354621=354622=diff
==
--- cfe/trunk/test/Driver/arm-float-abi.c (original)
+++ cfe/trunk/test/Driver/arm-float-abi.c Thu Feb 21 13:13:03 2019
@@ -4,3 +4,13 @@
 
 // ARMV7-ERROR: unsupported option '-mfloat-abi=hard' for target 'thumbv7'
 // NOERROR-NOT: unsupported option
+
+// RUN: %clang -target armv7-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM7-ANDROID %s
+// CHECK-ARM7-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM7-ANDROID: "-target-feature" "+soft-float-abi"
+
+// RUN: %clang -target armv8-linux-androideabi21 %s -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM8-ANDROID %s
+// CHECK-ARM8-ANDROID-NOT: "-target-feature" "+soft-float"
+// CHECK-ARM8-ANDROID: "-target-feature" "+soft-float-abi"


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

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

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187850.
jkorous added a comment.

- license
- end-of-include-guard comments


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+

[PATCH] D58526: AMDGPU: Don't emit debugger subtarget features

[Matt Arsenault via Phabricator via cfe-commits]

arsenm updated this revision to Diff 187849.
arsenm added a comment.

Undelete run line


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

https://reviews.llvm.org/D58526

Files:
  lib/Driver/ToolChains/AMDGPU.cpp
  test/Driver/amdgpu-features.c


Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -4,7 +4,7 @@
 
 // RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=1.0'
 
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | 
FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: "-target-feature" "+code-object-v3"
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -38,15 +38,8 @@
 void amdgpu::getAMDGPUTargetFeatures(const Driver ,
  const llvm::opt::ArgList ,
  std::vector ) {
-  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
-StringRef value = dAbi->getValue();
-if (value == "1.0") {
-  Features.push_back("+amdgpu-debugger-insert-nops");
-  Features.push_back("+amdgpu-debugger-emit-prologue");
-} else {
-  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
-}
-  }
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi))
+D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
 
   handleTargetFeaturesGroup(
 Args, Features, options::OPT_m_amdgpu_Features_Group);


Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -4,7 +4,7 @@
 
 // RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri -mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" "+amdgpu-debugger-insert-nops" "-target-feature" "+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support '-mamdgpu-debugger-abi=1.0'
 
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: "-target-feature" "+code-object-v3"
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -38,15 +38,8 @@
 void amdgpu::getAMDGPUTargetFeatures(const Driver ,
  const llvm::opt::ArgList ,
  std::vector ) {
-  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
-StringRef value = dAbi->getValue();
-if (value == "1.0") {
-  Features.push_back("+amdgpu-debugger-insert-nops");
-  Features.push_back("+amdgpu-debugger-emit-prologue");
-} else {
-  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
-}
-  }
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi))
+D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
 
   handleTargetFeaturesGroup(
 Args, Features, options::OPT_m_amdgpu_Features_Group);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58526: AMDGPU: Don't emit debugger subtarget features

[Konstantin Zhuravlyov via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D58526



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

[PATCH] D58518: [HIP] change kernel stub name

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D58518#1406274 , @t-tye wrote:

> To clarify, I am saying that the stub does have a different name since it is 
> conceptually part of the implementation of doing the call to the device 
> function implementation, and is not in fact the the device function being 
> called itself. However, when we generate code for a function that is present 
> on both the host and device, both copies of the code are for the same source 
> level function and so can have the same symbol name (which was a question 
> that was asked)


Got it. Agreed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58518



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks Eugene.


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

https://reviews.llvm.org/D58478



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

[PATCH] D58478: [index-while-building] FileIndexRecord

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187848.
jkorous marked 5 inline comments as done.
jkorous added a comment.

- license
- remove auto


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

https://reviews.llvm.org/D58478

Files:
  clang/include/clang/Index/DeclOccurrence.h
  clang/lib/Index/CMakeLists.txt
  clang/lib/Index/FileIndexRecord.cpp
  clang/lib/Index/FileIndexRecord.h

Index: clang/lib/Index/FileIndexRecord.h
===
--- /dev/null
+++ clang/lib/Index/FileIndexRecord.h
@@ -0,0 +1,57 @@
+//===--- FileIndexRecord.h - Index data per file *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
+#define LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Index/DeclOccurrence.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallVector.h"
+#include 
+
+namespace clang {
+class IdentifierInfo;
+
+namespace index {
+
+/// Stores the declaration occurrences seen in a particular source or header
+/// file of a translation unit
+class FileIndexRecord {
+private:
+  FileID FID;
+  bool IsSystem;
+  std::vector Decls;
+
+public:
+  FileIndexRecord(FileID FID, bool IsSystem) : FID(FID), IsSystem(IsSystem) {}
+
+  ArrayRef getDeclOccurrencesSortedByOffset() const {
+return Decls;
+  }
+
+  FileID getFileID() const { return FID; }
+  bool isSystem() const { return IsSystem; }
+
+  /// Adds an occurrence of the canonical declaration \c D at the supplied
+  /// \c Offset
+  ///
+  /// \param Roles the roles the occurrence fulfills in this position.
+  /// \param Offset the offset in the file of this occurrence.
+  /// \param D the canonical declaration this is an occurrence of.
+  /// \param Relations the set of symbols related to this occurrence.
+  void addDeclOccurence(SymbolRoleSet Roles, unsigned Offset, const Decl *D,
+ArrayRef Relations);
+  void print(llvm::raw_ostream ) const;
+};
+
+} // end namespace index
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_INDEX_FILEINDEXRECORD_H
Index: clang/lib/Index/FileIndexRecord.cpp
===
--- /dev/null
+++ clang/lib/Index/FileIndexRecord.cpp
@@ -0,0 +1,59 @@
+//===--- FileIndexRecord.cpp - Index data per file ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FileIndexRecord.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Path.h"
+
+using namespace clang;
+using namespace clang::index;
+
+void FileIndexRecord::addDeclOccurence(SymbolRoleSet Roles, unsigned Offset,
+   const Decl *D,
+   ArrayRef Relations) {
+  assert(D->isCanonicalDecl() &&
+ "Occurrences should be associated with their canonical decl");
+
+  auto IsNextOccurence = [&]() -> bool {
+if (Decls.empty())
+  return true;
+auto  = Decls.back();
+return Last.Offset < Offset;
+  };
+
+  if (IsNextOccurence()) {
+Decls.emplace_back(Roles, Offset, D, Relations);
+return;
+  }
+
+  DeclOccurrence NewInfo(Roles, Offset, D, Relations);
+  auto It = std::upper_bound(Decls.begin(), Decls.end(), NewInfo);
+  Decls.insert(It, std::move(NewInfo));
+}
+
+void FileIndexRecord::print(llvm::raw_ostream ) const {
+  OS << "DECLS BEGIN ---\n";
+  for (auto  : Decls) {
+const Decl *D = DclInfo.Dcl;
+SourceManager  = D->getASTContext().getSourceManager();
+SourceLocation Loc = SM.getFileLoc(D->getLocation());
+PresumedLoc PLoc = SM.getPresumedLoc(Loc);
+OS << llvm::sys::path::filename(PLoc.getFilename()) << ':' << PLoc.getLine()
+   << ':' << PLoc.getColumn();
+
+if (auto ND = dyn_cast(D)) {
+  OS << ' ' << ND->getNameAsString();
+}
+
+OS << '\n';
+  }
+  OS << "DECLS END ---\n";
+}
Index: clang/lib/Index/CMakeLists.txt
===
--- clang/lib/Index/CMakeLists.txt
+++ clang/lib/Index/CMakeLists.txt
@@ -6,6 +6,7 @@
 add_clang_library(clangIndex
   CodegenNameGenerator.cpp
   CommentToXML.cpp
+  FileIndexRecord.cpp
   IndexBody.cpp
   IndexDecl.cpp
   IndexingAction.cpp
Index: clang/include/clang/Index/DeclOccurrence.h
===
--- /dev/null
+++ 

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Dan Albert via Phabricator via cfe-commits]

danalbert marked 3 inline comments as done.
danalbert added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:389
+std::string DefaultFPU = getDefaultFPUName(Triple);
+if (DefaultFPU != "") {
+  if (!llvm::ARM::getFPUFeatures(llvm::ARM::parseFPU(DefaultFPU), 
Features))

peter.smith wrote:
> danalbert wrote:
> > peter.smith wrote:
> > > I'm wondering whether you need this bit of code anymore? In D53121 there 
> > > needed to be a switch between vfpv3-d16 and neon based on Android 
> > > version. With --target=armv7a-linux-android or --target=arm-linux-android 
> > > -march=armv7a or any v7a -mcpu applicable to Android then you'll get 
> > > feature Neon by default and won't need to do this? We could then move 
> > > getDefaultFPUName out of ARM.cpp
> > I don't understand. This bit of code is the piece that provides the NEON 
> > default. If I remove this then Android ARMv7 targets revert to no FPU 
> > features.
> My understanding is that adding the equivalent of -fpu=neon here has the 
> effect of adding extra feature flags to the call to clang -cc1. I can observe 
> the difference with -###. However when compiling these are then added back in 
> by TargetInfo::CreateTargetInfo() by calls like initFeatureMap that take the 
> default features from the target. When invoking -cc1as the 
> createMCSubtargetInfo via a long chain of function calls will add the 
> FeatureNEON bit for armv7-a unless the -neon feature has been cleared.  
> 
> If I have it right I think the lack of floating point features in the clang 
> -cc1 command line is not a problem as the defaults for armv7-a will put them 
> in anyway unless they've been explicitly turned off via something like 
> -mfloat-abi=soft. My information is based on reverse engineering the code so 
> I could easily be missing something important though?
Oh, I thought that the cc1 args were the full story. I'll need to do some more 
digging then to make sure that the behavior actually does match.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:251
   case llvm::Triple::Android:
 ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
 break;

peter.smith wrote:
> Not strictly related to this patch, but you'll probably want to make that 
> (SubArch >= 7), I tried --target=armv8a-linux-android as an experiment and 
> got my floating point support removed.
I actually uploaded a fix for that yesterday: https://reviews.llvm.org/D58477

Didn't submit because I wanted to wait until I knew I'd be around to babysit 
build bots just in case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58314



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

r354621 - [Fixed Point Arithmetic] Fixed Point Comparisons

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Thu Feb 21 12:50:09 2019
New Revision: 354621

URL: http://llvm.org/viewvc/llvm-project?rev=354621=rev
Log:
[Fixed Point Arithmetic] Fixed Point Comparisons

This patch implements fixed point comparisons with other fixed point types and
integers. This also provides constant expression evaluation for them.

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

Added:
cfe/trunk/test/Frontend/fixed_point_comparisons.c
Modified:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=354621=354620=354621=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Thu Feb 21 12:50:09 2019
@@ -9144,6 +9144,22 @@ EvaluateComparisonBinaryOperator(EvalInf
 return Success(CCR::Equal, E);
   }
 
+  if (LHSTy->isFixedPointType() || RHSTy->isFixedPointType()) {
+APFixedPoint LHSFX(Info.Ctx.getFixedPointSemantics(LHSTy));
+APFixedPoint RHSFX(Info.Ctx.getFixedPointSemantics(RHSTy));
+
+bool LHSOK = EvaluateFixedPointOrInteger(E->getLHS(), LHSFX, Info);
+if (!LHSOK && !Info.noteFailure())
+  return false;
+if (!EvaluateFixedPointOrInteger(E->getRHS(), RHSFX, Info) || !LHSOK)
+  return false;
+if (LHSFX < RHSFX)
+  return Success(CCR::Less, E);
+if (LHSFX > RHSFX)
+  return Success(CCR::Greater, E);
+return Success(CCR::Equal, E);
+  }
+
   if (LHSTy->isAnyComplexType() || RHSTy->isAnyComplexType()) {
 ComplexValue LHS, RHS;
 bool LHSOK;

Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=354621=354620=354621=diff
==
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Thu Feb 21 12:50:09 2019
@@ -125,11 +125,19 @@ struct BinOpInfo {
 return true;
   }
 
-  /// Check if either operand is a fixed point type, in which case, this
+  /// Check if either operand is a fixed point type or integer type, with at
+  /// least one being a fixed point type. In any case, this
   /// operation did not follow usual arithmetic conversion and both operands 
may
   /// not be the same.
   bool isFixedPointBinOp() const {
-return isa(E) && Ty->isFixedPointType();
+// We cannot simply check the result type since comparison operations 
return
+// an int.
+if (const auto *BinOp = dyn_cast(E)) {
+  QualType LHSType = BinOp->getLHS()->getType();
+  QualType RHSType = BinOp->getRHS()->getType();
+  return LHSType->isFixedPointType() || RHSType->isFixedPointType();
+}
+return false;
   }
 };
 
@@ -3372,8 +3380,6 @@ Value *ScalarExprEmitter::EmitFixedPoint
   using llvm::ConstantInt;
 
   const auto *BinOp = cast(op.E);
-  assert((BinOp->getOpcode() == BO_Add || BinOp->getOpcode() == BO_Sub) &&
- "Expected operation to be addition or subtraction");
 
   // The result is a fixed point type and at least one of the operands is fixed
   // point while the other is either fixed point or an int. This resulting type
@@ -3421,17 +3427,30 @@ Value *ScalarExprEmitter::EmitFixedPoint
 }
 break;
   }
-  case BO_Mul:
-  case BO_Div:
-  case BO_Shl:
-  case BO_Shr:
-  case BO_Cmp:
   case BO_LT:
+return CommonFixedSema.isSigned() ? Builder.CreateICmpSLT(FullLHS, FullRHS)
+  : Builder.CreateICmpULT(FullLHS, 
FullRHS);
   case BO_GT:
+return CommonFixedSema.isSigned() ? Builder.CreateICmpSGT(FullLHS, FullRHS)
+  : Builder.CreateICmpUGT(FullLHS, 
FullRHS);
   case BO_LE:
+return CommonFixedSema.isSigned() ? Builder.CreateICmpSLE(FullLHS, FullRHS)
+  : Builder.CreateICmpULE(FullLHS, 
FullRHS);
   case BO_GE:
+return CommonFixedSema.isSigned() ? Builder.CreateICmpSGE(FullLHS, FullRHS)
+  : Builder.CreateICmpUGE(FullLHS, 
FullRHS);
   case BO_EQ:
+// For equality operations, we assume any padding bits on unsigned types 
are
+// zero'd out. They could be overwritten through non-saturating operations
+// that cause overflow, but this leads to undefined behavior.
+return Builder.CreateICmpEQ(FullLHS, FullRHS);
   case BO_NE:
+return Builder.CreateICmpNE(FullLHS, FullRHS);
+  case BO_Mul:
+  case BO_Div:
+  case BO_Shl:
+  case BO_Shr:
+  case BO_Cmp:
   case BO_LAnd:
   case BO_LOr:
   case BO_MulAssign:
@@ -3714,8 +3733,9 @@ Value *ScalarExprEmitter::EmitCompare(co
 Result = CGF.CGM.getCXXABI().EmitMemberPointerComparison(
CGF, LHS, RHS, MPT, E->getOpcode() == BO_NE);
   } else if (!LHSTy->isAnyComplexType() && !RHSTy->isAnyComplexType()) {
-Value *LHS = 

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

[Leonard Chan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC354621: [Fixed Point Arithmetic] Fixed Point Comparisons 
(authored by leonardchan, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57219?vs=184886=187847#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57219

Files:
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGExprScalar.cpp
  test/Frontend/fixed_point_comparisons.c

Index: test/Frontend/fixed_point_comparisons.c
===
--- test/Frontend/fixed_point_comparisons.c
+++ test/Frontend/fixed_point_comparisons.c
@@ -0,0 +1,378 @@
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,UNPADDED
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -fpadding-on-unsigned-fixed-point -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,PADDED
+
+// Fixed point against other fixed point
+_Bool b_eq_true = 2.5hk == 2.5uhk;  // CHECK-DAG: @b_eq_true  = {{.*}}global i8 1, align 1
+_Bool b_eq_false = 2.5hk == 2.4uhk; // CHECK-DAG: @b_eq_false = {{.*}}global i8 0, align 1
+
+_Bool b_ne_true = 2.5hk != 2.4uhk;  // CHECK-DAG: @b_ne_true  = {{.*}}global i8 1, align 1
+_Bool b_ne_false = 2.5hk != 2.5uhk; // CHECK-DAG: @b_ne_false = {{.*}}global i8 0, align 1
+
+_Bool b_lt_true = 2.5hk < 2.75uhk; // CHECK-DAG: @b_lt_true  = {{.*}}global i8 1, align 1
+_Bool b_lt_false = 2.5hk < 2.5uhk; // CHECK-DAG: @b_lt_false = {{.*}}global i8 0, align 1
+
+_Bool b_le_true = 2.5hk <= 2.75uhk; // CHECK-DAG: @b_le_true  = {{.*}}global i8 1, align 1
+_Bool b_le_true2 = 2.5hk <= 2.5uhk; // CHECK-DAG: @b_le_true2 = {{.*}}global i8 1, align 1
+_Bool b_le_false = 2.5hk <= 2.4uhk; // CHECK-DAG: @b_le_false = {{.*}}global i8 0, align 1
+
+_Bool b_gt_true = 2.75hk > 2.5uhk;   // CHECK-DAG: @b_gt_true  = {{.*}}global i8 1, align 1
+_Bool b_gt_false = 2.75hk > 2.75uhk; // CHECK-DAG: @b_gt_false = {{.*}}global i8 0, align 1
+
+_Bool b_ge_true = 2.75hk >= 2.5uhk;   // CHECK-DAG: @b_ge_true  = {{.*}}global i8 1, align 1
+_Bool b_ge_true2 = 2.75hk >= 2.75uhk; // CHECK-DAG: @b_ge_true2 = {{.*}}global i8 1, align 1
+_Bool b_ge_false = 2.5hk >= 2.75uhk;  // CHECK-DAG: @b_ge_false = {{.*}}global i8 0, align 1
+
+// Fixed point against int
+_Bool b_ieq_true = 2.0hk == 2;  // CHECK-DAG: @b_ieq_true  = {{.*}}global i8 1, align 1
+_Bool b_ieq_false = 2.0hk == 3; // CHECK-DAG: @b_ieq_false = {{.*}}global i8 0, align 1
+
+_Bool b_ine_true = 2.0hk != 3;  // CHECK-DAG: @b_ine_true  = {{.*}}global i8 1, align 1
+_Bool b_ine_false = 2.0hk != 2; // CHECK-DAG: @b_ine_false = {{.*}}global i8 0, align 1
+
+_Bool b_ilt_true = 2.0hk < 3;  // CHECK-DAG: @b_ilt_true  = {{.*}}global i8 1, align 1
+_Bool b_ilt_false = 2.0hk < 2; // CHECK-DAG: @b_ilt_false = {{.*}}global i8 0, align 1
+
+_Bool b_ile_true = 2.0hk <= 3;  // CHECK-DAG: @b_ile_true  = {{.*}}global i8 1, align 1
+_Bool b_ile_true2 = 2.0hk <= 2; // CHECK-DAG: @b_ile_true2 = {{.*}}global i8 1, align 1
+_Bool b_ile_false = 2.0hk <= 1; // CHECK-DAG: @b_ile_false = {{.*}}global i8 0, align 1
+
+_Bool b_igt_true = 2.0hk > 1;  // CHECK-DAG: @b_igt_true  = {{.*}}global i8 1, align 1
+_Bool b_igt_false = 2.0hk > 2; // CHECK-DAG: @b_igt_false = {{.*}}global i8 0, align 1
+
+_Bool b_ige_true = 2.0hk >= 1;  // CHECK-DAG: @b_ige_true  = {{.*}}global i8 1, align 1
+_Bool b_ige_true2 = 2.0hk >= 2; // CHECK-DAG: @b_ige_true2 = {{.*}}global i8 1, align 1
+_Bool b_ige_false = 2.0hk >= 3; // CHECK-DAG: @b_ige_false = {{.*}}global i8 0, align 1
+
+// Different signage
+// Since we can have different precisions, non powers of 2 fractions may have
+// different actual values when being compared.
+_Bool b_sne_true = 2.6hk != 2.6uhk;
+// UNPADDED-DAG:   @b_sne_true = {{.*}}global i8 1, align 1
+// PADDED-DAG: @b_sne_true = {{.*}}global i8 0, align 1
+
+_Bool b_seq_true = 2.0hk == 2u;  // CHECK-DAG: @b_seq_true  = {{.*}}global i8 1, align 1
+_Bool b_seq_true2 = 2.0uhk == 2; // CHECK-DAG: @b_seq_true2 = {{.*}}global i8 1, align 1
+
+void TestComparisons() {
+  short _Accum sa;
+  _Accum a;
+  unsigned short _Accum usa;
+  unsigned _Accum ua;
+
+  // Each of these should be a fixed point conversion followed by the actual
+  // comparison operation.
+  sa == a;
+  // CHECK:  [[A:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[A2:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[RESIZE_A:%[a-z0-9]+]] = sext i16 [[A]] to i32
+  // CHECK-NEXT: [[UPSCALE_A:%[a-z0-9]+]] = shl i32 [[RESIZE_A]], 8
+  // CHECK-NEXT: {{.*}} = icmp eq i32 [[UPSCALE_A]], [[A2]]
+
+  sa != a;
+  // CHECK:  [[A:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[A2:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[RESIZE_A:%[a-z0-9]+]] = sext i16 [[A]] to i32
+  // CHECK-NEXT: [[UPSCALE_A:%[a-z0-9]+]] = shl i32 [[RESIZE_A]], 8
+  // CHECK-NEXT: 

[PATCH] D58526: AMDGPU: Don't emit debugger subtarget features

[Konstantin Zhuravlyov via Phabricator via cfe-commits]

kzhuravl added inline comments.



Comment at: test/Driver/amdgpu-features.c:4
 // CHECK-MAMDGPU-DEBUGGER-ABI-0-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=0.0'
-
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=0.0'
 

I do not see this prefix in the run line?


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

https://reviews.llvm.org/D58526



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

[PATCH] D58526: AMDGPU: Don't emit debugger subtarget features

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: yaxunl, kzhuravl, t-tye.
Herald added subscribers: jdoerfert, tpr, dstuttard, nhaehnle, wdng, jvesely.

Keep the flag around for compatability.


https://reviews.llvm.org/D58526

Files:
  lib/Driver/ToolChains/AMDGPU.cpp
  test/Driver/amdgpu-features.c


Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -1,10 +1,7 @@
 // RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=0.0 %s -o - 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-0-0 %s
 // CHECK-MAMDGPU-DEBUGGER-ABI-0-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=0.0'
-
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=0.0'
 
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | 
FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: "-target-feature" "+code-object-v3"
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -38,15 +38,8 @@
 void amdgpu::getAMDGPUTargetFeatures(const Driver ,
  const llvm::opt::ArgList ,
  std::vector ) {
-  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
-StringRef value = dAbi->getValue();
-if (value == "1.0") {
-  Features.push_back("+amdgpu-debugger-insert-nops");
-  Features.push_back("+amdgpu-debugger-emit-prologue");
-} else {
-  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
-}
-  }
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi))
+D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
 
   handleTargetFeaturesGroup(
 Args, Features, options::OPT_m_amdgpu_Features_Group);


Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -1,10 +1,7 @@
 // RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri -mamdgpu-debugger-abi=0.0 %s -o - 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-0-0 %s
 // CHECK-MAMDGPU-DEBUGGER-ABI-0-0: the clang compiler does not support '-mamdgpu-debugger-abi=0.0'
-
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri -mamdgpu-debugger-abi=1.0 %s -o - 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
-// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" "+amdgpu-debugger-insert-nops" "-target-feature" "+amdgpu-debugger-emit-prologue"
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: the clang compiler does not support '-mamdgpu-debugger-abi=0.0'
 
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: "-target-feature" "+code-object-v3"
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -38,15 +38,8 @@
 void amdgpu::getAMDGPUTargetFeatures(const Driver ,
  const llvm::opt::ArgList ,
  std::vector ) {
-  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
-StringRef value = dAbi->getValue();
-if (value == "1.0") {
-  Features.push_back("+amdgpu-debugger-insert-nops");
-  Features.push_back("+amdgpu-debugger-emit-prologue");
-} else {
-  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
-}
-  }
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi))
+D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
 
   handleTargetFeaturesGroup(
 Args, Features, options::OPT_m_amdgpu_Features_Group);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58525: [clangd] Don't attach FixIt to the source code in macro.

[Haojian Wu via Phabricator via cfe-commits]

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

We are less certain it is the correct fix. Also, clang doesn't apply FixIt to
the source code in macro.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D58525

Files:
  clangd/Diagnostics.cpp
  unittests/clangd/DiagnosticsTests.cpp


Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -215,6 +215,18 @@
"'int *' with an rvalue of type 'int'")));
 }
 
+TEST(DiagnosticsTest, NoFixItInMacro) {
+  Annotations Test(R"cpp(
+#define Define(name) void name() {}
+
+[[Define]](main)
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
+Not(WithFix(_);
+}
+
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.Message = "something terrible happened";
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -335,6 +335,11 @@
 
 llvm::SmallVector Edits;
 for (auto  : Info.getFixItHints()) {
+  // Follow clang's behavior, don't apply FixIt to the code in macros,
+  // we are less certain it is the right fix.
+  if (FixIt.RemoveRange.getBegin().isMacroID() ||
+  FixIt.RemoveRange.getEnd().isMacroID())
+return false;
   if (!isInsideMainFile(FixIt.RemoveRange.getBegin(),
 Info.getSourceManager()))
 return false;


Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -215,6 +215,18 @@
"'int *' with an rvalue of type 'int'")));
 }
 
+TEST(DiagnosticsTest, NoFixItInMacro) {
+  Annotations Test(R"cpp(
+#define Define(name) void name() {}
+
+[[Define]](main)
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
+Not(WithFix(_);
+}
+
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.Message = "something terrible happened";
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -335,6 +335,11 @@
 
 llvm::SmallVector Edits;
 for (auto  : Info.getFixItHints()) {
+  // Follow clang's behavior, don't apply FixIt to the code in macros,
+  // we are less certain it is the right fix.
+  if (FixIt.RemoveRange.getBegin().isMacroID() ||
+  FixIt.RemoveRange.getEnd().isMacroID())
+return false;
   if (!isInsideMainFile(FixIt.RemoveRange.getBegin(),
 Info.getSourceManager()))
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58518: [HIP] change kernel stub name

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC354615: [HIP] change kernel stub name (authored by yaxunl, 
committed by ).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D58518?vs=187815=187840#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58518

Files:
  lib/CodeGen/CGCUDANV.cpp
  test/CodeGenCUDA/device-stub.cu


Index: lib/CodeGen/CGCUDANV.cpp
===
--- lib/CodeGen/CGCUDANV.cpp
+++ lib/CodeGen/CGCUDANV.cpp
@@ -227,6 +227,12 @@
 emitDeviceStubBodyNew(CGF, Args);
   else
 emitDeviceStubBodyLegacy(CGF, Args);
+
+  // Postfix kernel stub names with .stub to differentiate them from kernel
+  // names in device binaries. This is to facilitate the debugger to find
+  // the correct symbols for kernels in the device binary.
+  if (CGF.getLangOpts().HIP)
+CGF.CurFn->setName(CGF.CurFn->getName() + ".stub");
 }
 
 // CUDA 9.0+ uses new way to launch kernels. Parameters are packed in a local
Index: test/CodeGenCUDA/device-stub.cu
===
--- test/CodeGenCUDA/device-stub.cu
+++ test/CodeGenCUDA/device-stub.cu
@@ -145,7 +145,8 @@
 // Test that we build the correct number of calls to cudaSetupArgument followed
 // by a call to cudaLaunch.
 
-// LNX: define{{.*}}kernelfunc
+// CUDA-LABEL: define{{.*}}kernelfunc
+// HIP-LABEL: define{{.*}}@_Z10kernelfunciii.stub
 
 // New launch sequence stores arguments into local buffer and passes array of
 // pointers to them directly to cudaLaunchKernel


Index: lib/CodeGen/CGCUDANV.cpp
===
--- lib/CodeGen/CGCUDANV.cpp
+++ lib/CodeGen/CGCUDANV.cpp
@@ -227,6 +227,12 @@
 emitDeviceStubBodyNew(CGF, Args);
   else
 emitDeviceStubBodyLegacy(CGF, Args);
+
+  // Postfix kernel stub names with .stub to differentiate them from kernel
+  // names in device binaries. This is to facilitate the debugger to find
+  // the correct symbols for kernels in the device binary.
+  if (CGF.getLangOpts().HIP)
+CGF.CurFn->setName(CGF.CurFn->getName() + ".stub");
 }
 
 // CUDA 9.0+ uses new way to launch kernels. Parameters are packed in a local
Index: test/CodeGenCUDA/device-stub.cu
===
--- test/CodeGenCUDA/device-stub.cu
+++ test/CodeGenCUDA/device-stub.cu
@@ -145,7 +145,8 @@
 // Test that we build the correct number of calls to cudaSetupArgument followed
 // by a call to cudaLaunch.
 
-// LNX: define{{.*}}kernelfunc
+// CUDA-LABEL: define{{.*}}kernelfunc
+// HIP-LABEL: define{{.*}}@_Z10kernelfunciii.stub
 
 // New launch sequence stores arguments into local buffer and passes array of
 // pointers to them directly to cudaLaunchKernel
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

[John McCall via Phabricator via cfe-commits]

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

No, this LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57219



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

[PATCH] D58518: [HIP] change kernel stub name

[Tony Tye via Phabricator via cfe-commits]

t-tye added a comment.

To clarify, I am saying that the stub does have a different name since it is 
conceptually part of the implementation of doing the call to the device 
function implementation, and is not in fact the the device function being 
called itself. However, when we generate code for a function that is present on 
both the host and device, both copies of the code are for the same source level 
function and so can have the same symbol name (which was a question that was 
asked).


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

https://reviews.llvm.org/D58518



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

r354615 - [HIP] change kernel stub name

[Yaxun Liu via cfe-commits]

Author: yaxunl
Date: Thu Feb 21 12:12:16 2019
New Revision: 354615

URL: http://llvm.org/viewvc/llvm-project?rev=354615=rev
Log:
[HIP] change kernel stub name

Add .stub to kernel stub function name so that it is different from kernel
name in device code. This is necessary to let debugger find correct symbol
for kernel

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

Modified:
cfe/trunk/lib/CodeGen/CGCUDANV.cpp
cfe/trunk/test/CodeGenCUDA/device-stub.cu

Modified: cfe/trunk/lib/CodeGen/CGCUDANV.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCUDANV.cpp?rev=354615=354614=354615=diff
==
--- cfe/trunk/lib/CodeGen/CGCUDANV.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCUDANV.cpp Thu Feb 21 12:12:16 2019
@@ -227,6 +227,12 @@ void CGNVCUDARuntime::emitDeviceStub(Cod
 emitDeviceStubBodyNew(CGF, Args);
   else
 emitDeviceStubBodyLegacy(CGF, Args);
+
+  // Postfix kernel stub names with .stub to differentiate them from kernel
+  // names in device binaries. This is to facilitate the debugger to find
+  // the correct symbols for kernels in the device binary.
+  if (CGF.getLangOpts().HIP)
+CGF.CurFn->setName(CGF.CurFn->getName() + ".stub");
 }
 
 // CUDA 9.0+ uses new way to launch kernels. Parameters are packed in a local

Modified: cfe/trunk/test/CodeGenCUDA/device-stub.cu
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCUDA/device-stub.cu?rev=354615=354614=354615=diff
==
--- cfe/trunk/test/CodeGenCUDA/device-stub.cu (original)
+++ cfe/trunk/test/CodeGenCUDA/device-stub.cu Thu Feb 21 12:12:16 2019
@@ -145,7 +145,8 @@ void use_pointers() {
 // Test that we build the correct number of calls to cudaSetupArgument followed
 // by a call to cudaLaunch.
 
-// LNX: define{{.*}}kernelfunc
+// CUDA-LABEL: define{{.*}}kernelfunc
+// HIP-LABEL: define{{.*}}@_Z10kernelfunciii.stub
 
 // New launch sequence stores arguments into local buffer and passes array of
 // pointers to them directly to cudaLaunchKernel


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

[PATCH] D58523: [OpenMP 5.0] Parsing/sema support for to and from clauses with mapper modifier

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: include/clang/Basic/OpenMPKinds.def:585
+// Modifiers for 'to' and 'from' clause.
+OPENMP_TO_FROM_MODIFIER_KIND(mapper)
+

Maybe, better to split it. There must be separate modifiers for `from` clause 
and for `to` clause just for possible future changes.



Comment at: lib/AST/OpenMPClause.cpp:876
 
-  OMPToClause *Clause = new (Mem) OMPToClause(Locs, Sizes);
+  OMPToClause *Clause =
+  new (Mem) OMPToClause(UDMQualifierLoc, MapperId, Locs, Sizes);

`auto *`



Comment at: lib/AST/OpenMPClause.cpp:924
 
-  OMPFromClause *Clause = new (Mem) OMPFromClause(Locs, Sizes);
+  OMPFromClause *Clause =
+  new (Mem) OMPFromClause(UDMQualifierLoc, MapperId, Locs, Sizes);

`auto *`



Comment at: lib/AST/OpenMPClause.cpp:1469
 
 void OMPClausePrinter::VisitOMPFromClause(OMPFromClause *Node) {
   if (!Node->varlist_empty()) {

Better to split this patch into 2: one for `to` clause and another one for 
`from` clause



Comment at: lib/Parse/ParseOpenMP.cpp:2154
 parseMapType(*this, Data);
+  else {
+SkipUntil(tok::colon, tok::annot_pragma_openmp_end, StopBeforeMatch);

Remove braces



Comment at: lib/Parse/ParseOpenMP.cpp:2174-2175
+if (Tok.isNot(tok::colon)) {
+  if (!IsInvalidMapperModifier)
+Diag(Tok, diag::warn_pragma_expected_colon) << "mapper(...)";
+  SkipUntil(tok::colon, tok::r_paren, tok::annot_pragma_openmp_end,

`mapper(...)` does not look good here, we never ever used this kind of 
construct before. Better to use something that was used already.



Comment at: lib/Sema/SemaOpenMP.cpp:13061
 MapperIdScopeSpec.getWithLocInContext(SemaRef.Context), MapperId,
-/*ADL=*/false, /*Overloaded=*/true, URS.begin(), URS.end());
+/*ADL=*/true, /*Overloaded=*/true, URS.begin(), URS.end());
   }

Better to fix this in a separate patch.



Comment at: lib/Sema/SemaOpenMP.cpp:13356
 
-  // Try to find the associated user-defined mapper.
-  ExprResult ER = buildUserDefinedMapperRef(
-  SemaRef, DSAS->getCurScope(), *MapperIdScopeSpec, *MapperId,
-  Type.getCanonicalType(), UnresolvedMapper);
-  if (ER.isInvalid())
-continue;
-  MVLI.UDMapperList.push_back(ER.get());
-} else {
-  MVLI.UDMapperList.push_back(nullptr);
 }
 

Are you sure about this change? This might trigger some asserts.



Comment at: lib/Sema/SemaOpenMP.cpp:13413
 
-  // If the identifier of user-defined mapper is not specified, it is 
"default".
-  if (!MapperId.getName() || MapperId.getName().isEmpty()) {

Again, better to do this in a separate patch


Repository:
  rC Clang

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

https://reviews.llvm.org/D58523



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

[PATCH] D58518: [HIP] change kernel stub name

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D58518#1406202 , @t-tye wrote:

> Yes this relates to supporting the debugger.
>
> For the same function being present on both host and device, having the same 
> name is correct as the debugger must set a breakpoint at both places. This is 
> similar to needing to set a breakpoint at every place a function is inlined.


I'm confused. Are you saying that HIP does *not* need a different name for the 
stub then?


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

https://reviews.llvm.org/D58518



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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D58345#1401907 , @sammccall wrote:

> In D58345#1401213 , @hokein wrote:
>
> > In D58345#1401040 , @ioeric wrote:
> >
> > > Looks great! Thanks for doing this.
> > >
> > > Could you also check in the tool that is used to generate the mapping? We 
> > > need a way to update the mapping when cppreference is updated.
> >
> >
> > The tool is not ready yet, I'm still polishing it, but the results are 
> > good, I think this patch should not be blocked by the tool.
>
>
> Generated files aren't really source code, I agree with Eric we should check 
> in the tool with this patch.


Added the tool in this patch, but I think a separate repository under GitHub's 
clangd org might be a more suitable place.

For simplicity, I re-scoped this patch -- only emits unique symbols. We have 
ideas to handle ambiguous symbols, will address them in a followup.




Comment at: clangd/StdSymbolMap.imp:410
+{ "std::gslice_array", { "" } },
+{ "std::hardware_constructive_interference_size", { "" } }, // C++11
+{ "std::hardware_destructive_interference_size", { "" } }, // C++11

sammccall wrote:
> I'm not sure if the language comments are useful to human readers, who can 
> just look this stuff up. If you think they might be useful to code, we could 
> include them in  tablegen-style macros.
> 
> In any case, this is a C++17 symbol, so maybe something's wrong with the 
> generator.
> 
> In any case, this is a C++17 symbol, so maybe something's wrong with the 
> generator.

hmm, it is a bug in cppreference index page, this symbol is wrongly marked 
C++11.



Comment at: clangd/StdSymbolMap.imp:1283
+{ "std::log10", { "", "", "" } },
+{ "std::make_error_code", { "", "" } }, // C++11
+{ "std::make_error_condition", { "", "" } }, // C++11

sammccall wrote:
> we're missing the `future` variant I think?
looks like it was a bug in the generator code, fixed.



Comment at: clangd/StdSymbolMap.imp:1291
+{ "std::sinh", { "", "", "" } },
+{ "std::size_t", { "", "", "", "", 
"", "" } },
+{ "std::sqrt", { "", "", "" } },

sammccall wrote:
> this is interesting: any of the options are valid. IMO the best one here is 
> ``.
> But we should be picking by policy, not by looking at the header where the 
> symbol is actually defined. We could do this on either the generator or 
> consumer side.
this can be addressed by using a whitelist in the generator.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



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