[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Please note: I have a patch that disables warnings from system macros for all 
clang-tidy warnings, it would be good to review it so we don't need to 
implement the same mechanism in all 400+ checks :) 
https://reviews.llvm.org/D116378


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.

In D115921#3224329 , @jrtc27 wrote:

> In D115921#3224324 , @zixuan-wu 
> wrote:
>
>> In D115921#3224284 , @jrtc27 wrote:
>>
>>> but also with RISC-V extensions not being changed once ratified any more 
>>> (changes mean new extensions entirely, not new versions)
>>
>> I don't think so. Or why is there version in RISC-V spec?
>
> That was added years ago before there was any plan/policy for ratifying new 
> extensions beyond the initial GC set. 
> https://docs.google.com/presentation/d/1nQ5uFb39KA6gvUi5SReWfIQSiRN7hp6z7ZPfctE4mKk/edit#slide=id.p1
>  is the current lifecycle; note that //only// errata can be fixed after 
> ratification, everything else requires a new extension (see the blue arrow 
> and brown box below it). As far as I can tell, for new extensions it serves 
> no purpose other than to distinguish draft specs from ratified ones.
>
>> And not only for standard extension, but also it's needed in custom 
>> extension.
>
> Vendor extensions are going to be enough of a support pain in Clang. I 
> sincerely hope they don't make life worse by defining multiple versions, 
> rather than doing it properly and defining new extensions every time they add 
> things. It's not just for the toolchain's benefit; it also improves forwards 
> compatibility for kernels/loaders, as they won't know about new versions, but 
> may know about existing versions, so if they support "Xvendorbase" then 
> software that wants to take advantage of "Xvendorbase" and "Xvendornew" can 
> still use "Xvendorbase", but if the kernel/loader only supports "Xvendor1p0" 
> and software wants to use "Xvendor2p0" then it can't do anything, it'd need 
> to be more careful and also have an "Xvendor1p0" implementation. Extending 
> rather than redefining comes with real benefits.
>
>> BTW, it's been supported to parse version of -march in clang side.
>
> It parses and checks the version, but it only allows //the// version of the 
> extension Clang currently implements. Parsing the version is a hard 
> requirement since it can be part of a valid arch string. Supporting multiple 
> versions is not.

If I don't understand wrong, all you want to say is that the extension version 
is just for indication, and not for functionality? So the RV spec does not 
require compiler to support multi-version.

Anybody else has more comments about support multi-version extension? Or it has 
been decided already?


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

https://reviews.llvm.org/D115921

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

[PATCH] D116025: [analyzer][NFC] Refactor GenericTaintChecker to use CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

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

I haven't read this whole patch with full scrutiny but it sure looks lovely. I 
now also see what the problem is with non-static strings in call descriptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116025

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

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Interesting. Might it be that in this scenario in order to be of interest to 
the user the condition value has to be trackable back to the current stack 
frame?

> the popular feedback we hear from some of our users, namely that they can 
> never have too much information

They should try `prune-paths=false` in C++. Hundreds of inlined 
copy-constructors will definitely give them the desired experience ;)




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1931-1933
+  // If the condition was a function call, we likely won't gain much from
+  // tracking it either. Evidence suggests that it will mostly trigger in
+  // scenarios like this:

Let's make it clear that this decision is purely stochastic: we can totally 
build an artificial example where this results in bad behavior but we've never 
seen one in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116597

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

[PATCH] D116636: [WIP] Enable `-Wstrict-calls-without-prototype` by default

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ooof you fixed a lot of static analyzer tests. If you ever get tired of this, 
feel free to `-w` them entirely because the driver's `--analyze` flag implies 
`-w` anyway. Hmm, maybe we should even change `%clang_analyze_cc1` to include 
`-w`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116636

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

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.
Herald added a reviewer: Szelethus.
Herald added a subscriber: steakhal.

FWIW, ASTUnit seems to save an effective triple while the current AST uses the 
nominal triple. (for example, `armv7a-xx-xx-eabihf` vs `armv7a-xx-xx-eabi`). 
I'm not sure if there is a good solution other than using heuristics to allow 
some differences.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55134

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:61
 {"zvlsseg", RISCVExtensionVersion{0, 10}},
+//{"zvlsseg", RISCVExtensionVersion{0, 7}},
 

This one too?..


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

https://reviews.llvm.org/D115921

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D115921#3224324 , @zixuan-wu wrote:

> In D115921#3224284 , @jrtc27 wrote:
>
>> but also with RISC-V extensions not being changed once ratified any more 
>> (changes mean new extensions entirely, not new versions)
>
> I don't think so. Or why is there version in RISC-V spec?

That was added years ago before there was any plan/policy for ratifying new 
extensions beyond the initial GC set. 
https://docs.google.com/presentation/d/1nQ5uFb39KA6gvUi5SReWfIQSiRN7hp6z7ZPfctE4mKk/edit#slide=id.p1
 is the current lifecycle; note that //only// errata can be fixed after 
ratification, everything else requires a new extension (see the blue arrow and 
brown box below it). As far as I can tell it serves no purpose other than to 
distinguish draft specs from ratified ones.

> And not only for standard extension, but also it's needed in custom extension.

Vendor extensions are going to be enough of a support pain in Clang. I 
sincerely hope they don't make life worse by defining multiple versions, rather 
than doing it properly and defining new extensions every time they add things. 
It's not just for the toolchain's benefit; it also improves forwards 
compatibility for kernels/loaders, as they won't know about new versions, but 
may know about existing versions, so if they support "Xvendorbase" then 
software that wants to take advantage of "Xvendorbase" and "Xvendornew" can 
still use "Xvendorbase", but if the kernel/loader only supports "Xvendor1p0" 
and software wants to use "Xvendor2p0" then it can't do anything, it'd need to 
be more careful and also have an "Xvendor1p0" implementation. Extending rather 
than redefining comes with real benefits.

> BTW, it's been supported to parse version of -march in clang side.

It parses and checks the version, but it only allows //the// version of the 
extension Clang currently implements. Parsing the version is a hard requirement 
since it can be part of a valid arch string. Supporting multiple versions is 
not.


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

https://reviews.llvm.org/D115921

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added inline comments.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:48
 {"v", RISCVExtensionVersion{0, 10}},
+//{"v", RISCVExtensionVersion{0, 7}},
 {"zba", RISCVExtensionVersion{1, 0}},

jrtc27 wrote:
> Don't do this
This nit will be removed before commit


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

https://reviews.llvm.org/D115921

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.

In D115921#3224284 , @jrtc27 wrote:

> but also with RISC-V extensions not being changed once ratified any more 
> (changes mean new extensions entirely, not new versions)

I don't think so. Or why is there version in RISC-V spec? 
And not only for standard extension, but also it's needed in custom extension.

BTW, it's been supported to parse version of -march in clang side.


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

https://reviews.llvm.org/D115921

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

[PATCH] D108694: [RISCV] Add the zvl extension according to the v1.0 spec

[Yueh-Ting Chen via Phabricator via cfe-commits]

eopXD added a comment.

ping again, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108694

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

[PATCH] D116316: [clang-format] Add an experimental option to remove optional control statement braces in LLVM C++ code

[Owen Pan via Phabricator via cfe-commits]

owenpan updated this revision to Diff 397783.
owenpan added a comment.

Fixing a couple of major bugs found by running check-clang plus minor bug fixes 
and cleanup:

- In `parseLevel()`, `HasOpeningBrace` and `case tok::r_brace:` don't 
necessarily mean the tokens are `{` and `}`, respectively.
- A pair of braces may be subject to removal even if the `}` is followed by a 
non-trailing comment.
- The newline before a  wrapped `{` should be deleted when removing the braces.
- `precededByCommentOrPPDirective()` should be a `const` member function.
- The assertion that a `{` of a control statement block is not parsed more than 
once is false.

Now this patch not only successfully builds clang but also passes check-clang 
(by not failing more tests than the current clang-format).


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

https://reviews.llvm.org/D116316

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -18682,6 +18682,7 @@
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
+  CHECK_PARSE_BOOL(RemoveBracesLLVM);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -23020,6 +23021,319 @@
Style);
 }
 
+TEST_F(FormatTest, RemoveBraces) {
+  FormatStyle Style = getLLVMStyle();
+  Style.RemoveBracesLLVM = true;
+
+  // The following eight test cases are fully-braced versions of the examples at
+  // "llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-
+  // statement-bodies-of-if-else-loop-statements".
+
+  // 1. Omit the braces, since the body is simple and clearly associated with
+  // the if.
+  EXPECT_EQ("if (isa(D))\n"
+"  handleFunctionDecl(D);\n"
+"else if (isa(D))\n"
+"  handleVarDecl(D);",
+format("if (isa(D)) {\n"
+   "  handleFunctionDecl(D);\n"
+   "} else if (isa(D)) {\n"
+   "  handleVarDecl(D);\n"
+   "}",
+   Style));
+
+  // 2. Here we document the condition itself and not the body.
+  verifyFormat("if (isa(D)) {\n"
+   "  // It is necessary that we explain the situation with this\n"
+   "  // surprisingly long comment, so it would be unclear\n"
+   "  // without the braces whether the following statement is in\n"
+   "  // the scope of the `if`.\n"
+   "  // Because the condition is documented, we can't really\n"
+   "  // hoist this comment that applies to the body above the\n"
+   "  // if.\n"
+   "  handleOtherDecl(D);\n"
+   "}",
+   Style);
+
+  // 3. Use braces on the outer `if` to avoid a potential dangling else
+  // situation.
+  EXPECT_EQ("if (isa(D)) {\n"
+"  for (auto *A : D.attrs())\n"
+"if (shouldProcessAttr(A))\n"
+"  handleAttr(A);\n"
+"}",
+format("if (isa(D)) {\n"
+   "  for (auto *A : D.attrs()) {\n"
+   "if (shouldProcessAttr(A)) {\n"
+   "  handleAttr(A);\n"
+   "}\n"
+   "  }\n"
+   "}",
+   Style));
+
+  // 4. Use braces for the `if` block to keep it uniform with the else block.
+  verifyFormat("if (isa(D)) {\n"
+   "  handleFunctionDecl(D);\n"
+   "} else {\n"
+   "  // In this else case, it is necessary that we explain the\n"
+   "  // situation with this surprisingly long comment, so it\n"
+   "  // would be unclear without the braces whether the\n"
+   "  // following statement is in the scope of the `if`.\n"
+   "  handleOtherDecl(D);\n"
+   "}",
+   Style);
+
+  // 5. This should also omit braces.  The `for` loop contains only a single
+  // statement, so it shouldn't have braces.  The `if` also only contains a
+  // single simple statement (the for loop), so it also should omit braces.
+  EXPECT_EQ("if (isa(D))\n"
+"  for (auto *A : D.attrs())\n"
+"handleAttr(A);",
+format("if (isa(D)) {\n"
+   "  for (auto *A : D.attrs()) {\n"
+   "handleAttr(A);\n"
+   "  }\n"
+   "}",
+   Style));
+
+  // 6. Use braces for the outer `if` since the 

[PATCH] D116722: [clang] Verify ssp buffer size is a valid integer

[Alex via Phabricator via cfe-commits]

alextsao1999 created this revision.
alextsao1999 added a reviewer: compnerd.
alextsao1999 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch adds to verify whether the ssp buffer size is a legal integer and a 
new diagnostic driver kind has been added.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116722

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,26 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto  : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
 if (Str.startswith("ssp-buffer-size=")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+auto BufferSize = Str.drop_front(16);
+if (IsInteger(BufferSize)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(BufferSize));
+} else
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in 
environment">;


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,26 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto  : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
 if (Str.startswith("ssp-buffer-size=")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+auto BufferSize = Str.drop_front(16);
+if (IsInteger(BufferSize)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(BufferSize));
+} else
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in environment">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I'm unconvinced about landing something like this until there's an actual use 
case in the tree. How do we know this will actually work the way we want it to 
if there's nothing proving it? It's still unclear to me how exactly this is 
going to be represented in the target features, but also with RISC-V extensions 
not being changed once ratified any more (changes mean new extensions entirely, 
not new versions) I don't know whether this is actually needed or we can just 
deal with the couple of existing cases more simply.




Comment at: llvm/lib/Support/RISCVISAInfo.cpp:48
 {"v", RISCVExtensionVersion{0, 10}},
+//{"v", RISCVExtensionVersion{0, 7}},
 {"zba", RISCVExtensionVersion{1, 0}},

Don't do this


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

https://reviews.llvm.org/D115921

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

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.

ping...


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

https://reviews.llvm.org/D115921

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

[PATCH] D116721: [Tooling] When transferring compile commands between files, always use '--'

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

"driver  -- " is a particularly convenient form of the
compile command to manipulate, with fewer special cases to handle.

Guaranteeing that the output command is of that form is cheap and makes
it easier to consume the result in some cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116721

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp


Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -739,6 +739,9 @@
 EXPECT_EQ(Results[0].CommandLine.back(), MakeNative ? path(F) : F)
 << "Last arg should be the file";
 Results[0].CommandLine.pop_back();
+EXPECT_EQ(Results[0].CommandLine.back(), "--")
+<< "Second-last arg should be --";
+Results[0].CommandLine.pop_back();
 return llvm::join(Results[0].CommandLine, " ");
   }
 
@@ -826,18 +829,6 @@
   EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall 
-std=c++14");
 }
 
-TEST_F(InterpolateTest, InsertDoubleDash) {
-  add("dir/foo.cpp", "-o foo.o -std=c++14 -Wall");
-  EXPECT_EQ(getCommand("-dir/bar.cpp", false),
-"clang -D dir/foo.cpp -Wall -std=c++14 --");
-}
-
-TEST_F(InterpolateTest, InsertDoubleDashForClangCL) {
-  add("dir/foo.cpp", "clang-cl", "/std:c++14 /W4");
-  EXPECT_EQ(getCommand("/dir/bar.cpp", false),
-"clang-cl -D dir/foo.cpp /W4 /std:c++14 --");
-}
-
 TEST_F(InterpolateTest, Case) {
   add("FOO/BAR/BAZ/SHOUT.cc");
   add("foo/bar/baz/quiet.cc");
@@ -879,7 +870,7 @@
   CompileCommand Transferred = transferCompileCommand(std::move(Cmd), "foo.h");
   EXPECT_EQ(Transferred.Filename, "foo.h");
   EXPECT_THAT(Transferred.CommandLine,
-  ElementsAre("clang", "-Wall", "-x", "c++-header", "foo.h"));
+  ElementsAre("clang", "-Wall", "-x", "c++-header", "--", 
"foo.h"));
   EXPECT_EQ(Transferred.Directory, "dir");
 }
 
Index: clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
===
--- clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -243,8 +243,7 @@
   llvm::Twine(ClangCLMode ? "/std:" : "-std=") +
   LangStandard::getLangStandardForKind(Std).getName()).str());
 }
-if (Filename.startswith("-") || (ClangCLMode && Filename.startswith("/")))
-  Result.CommandLine.push_back("--");
+Result.CommandLine.push_back("--");
 Result.CommandLine.push_back(std::string(Filename));
 return Result;
   }
Index: clang/include/clang/Tooling/CompilationDatabase.h
===
--- clang/include/clang/Tooling/CompilationDatabase.h
+++ clang/include/clang/Tooling/CompilationDatabase.h
@@ -216,6 +216,8 @@
 /// Transforms a compile command so that it applies the same configuration to
 /// a different file. Most args are left intact, but tweaks may be needed
 /// to certain flags (-x, -std etc).
+///
+/// The output command will always end in {"--", Filename}.
 tooling::CompileCommand transferCompileCommand(tooling::CompileCommand,
StringRef Filename);
 
Index: clang-tools-extra/clangd/CompileCommands.cpp
===
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ clang-tools-extra/clangd/CompileCommands.cpp
@@ -290,16 +290,9 @@
 TransferCmd.CommandLine = std::move(Cmd);
 TransferCmd = transferCompileCommand(std::move(TransferCmd), File);
 Cmd = std::move(TransferCmd.CommandLine);
-
-// Restore the canonical "driver --opts -- filename" form we expect.
-// FIXME: This is ugly and coupled. Make transferCompileCommand ensure it?
-assert(!Cmd.empty() && Cmd.back() == File);
-Cmd.pop_back();
-if (!Cmd.empty() && Cmd.back() == "--")
-  Cmd.pop_back();
-assert(!llvm::is_contained(Cmd, "--"));
-Cmd.push_back("--");
-Cmd.push_back(File.str());
+assert(Cmd.size() >= 2 && Cmd.back() == File &&
+   Cmd[Cmd.size() - 2] == "--" &&
+   "TransferCommand should produce a command ending in -- filename");
   }
 
   for (auto  : Config::current().CompileFlags.Edits)


Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ 

[PATCH] D116337: [clang] set __NO_MATH_ERRNO__ if -fno-math-errno

[Alex Xu (Hello71) via Phabricator via cfe-commits]

alxu updated this revision to Diff 39.
alxu added a comment.

Sort test defines orthographically, not chronologically.


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

https://reviews.llvm.org/D116337

Files:
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/predefined-macros.c


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -60,6 +60,15 @@
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
+//
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
 //
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s 
--check-prefix=CHECK-FINITE-MATH-ONLY
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1039,6 +1039,9 @@
 
   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());
 
+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -60,6 +60,15 @@
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
+//
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
 //
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FINITE-MATH-ONLY
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1039,6 +1039,9 @@
 
   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());
 
+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115060: [clang-format][NFC] Code Tidies in UnwrappedLineFormatter

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

> But I suspect it is the Assignment of the `PreviousLine` since this is not 
> existent every time.

Yep!

> So I see the following solutions:
>
> 1. Only name `NextLine`, and use `I[-1]`.
> 2. `const auto HasPreviousLine = I != AnnotatedLines.begin(); const auto 
>  = HasPreviousLine ? *I[-1] : *I;` which is safe, since 
> `PreviousLine` is only used if `HasPreviousLine` is true, but is a bit 
> confusing. It would get an explaining comment.
> 3. Rearrange the statements so that we can have only one check if there is a 
> previous line and define `PreviousLine` inside that `if`. It remains to be 
> seen if that's NFC.
>
> I would prefer option 3, but if that would change the behavior would go for 
> option 2.

There is option 4: change the type of `PreviousLine` to a pointer, and replace 
`PreviousLine.` with `PreviousLine->`. (You can rename `PreviousLine` if you 
think the naming is inconsistent with `NextLine`.) This would keep the patch 
being NFC.


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

https://reviews.llvm.org/D115060

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

[PATCH] D102669: [analyzer][ctu] Fix wrong 'multiple definitions' errors caused by space characters in lookup names when parsing the ctu index file

[Ella Ma via Phabricator via cfe-commits]

OikawaKirie updated this revision to Diff 397767.
OikawaKirie added a comment.

To make it work on Windows, Linux, and Mac OS, using `echo` to create the 
external function map, and using AST file for CTU analysis.
Tested on Windows, Linux, and Mac OS under x64.


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

https://reviews.llvm.org/D102669

Files:
  clang/docs/analyzer/user-docs/CrossTranslationUnit.rst
  clang/include/clang/Basic/DiagnosticCrossTUKinds.td
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/test/Analysis/Inputs/ctu-import.c.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp
  clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.ast-dump.txt
  
clang/test/Analysis/Inputs/plist-macros-with-expansion-ctu.c.externalDefMap.txt
  clang/test/Analysis/ctu-inherited-default-ctor.cpp
  clang/test/Analysis/ctu-lookup-name-with-space.cpp
  clang/test/Analysis/func-mapping-test.cpp
  clang/unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -61,7 +61,7 @@
 ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("index", "txt", IndexFD,
 IndexFileName));
 llvm::ToolOutputFile IndexFile(IndexFileName, IndexFD);
-IndexFile.os() << "c:@F@f#I# " << ASTFileName << "\n";
+IndexFile.os() << "9:c:@F@f#I# " << ASTFileName << "\n";
 IndexFile.os().flush();
 EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
 
Index: clang/test/Analysis/func-mapping-test.cpp
===
--- clang/test/Analysis/func-mapping-test.cpp
+++ clang/test/Analysis/func-mapping-test.cpp
@@ -3,10 +3,10 @@
 int f(int) {
   return 0;
 }
-// CHECK-DAG: c:@F@f#I#
+// CHECK-DAG: 9:c:@F@f#I#
 
 extern const int x = 5;
-// CHECK-DAG: c:@x
+// CHECK-DAG: 4:c:@x
 
 // Non-const variables should not be collected.
 int y = 5;
@@ -18,29 +18,29 @@
   int a;
 };
 extern S const s = {.a = 2};
-// CHECK-DAG: c:@s
+// CHECK-DAG: 4:c:@s
 
 struct SF {
   const int a;
 };
 SF sf = {.a = 2};
-// CHECK-DAG: c:@sf
+// CHECK-DAG: 5:c:@sf
 
 struct SStatic {
   static const int a = 4;
 };
 const int SStatic::a;
-// CHECK-DAG: c:@S@SStatic@a
+// CHECK-DAG: 14:c:@S@SStatic@a
 
 extern int const arr[5] = { 0, 1 };
-// CHECK-DAG: c:@arr
+// CHECK-DAG: 6:c:@arr
 
 union U {
   const int a;
   const unsigned int b;
 };
 U u = {.a = 6};
-// CHECK-DAG: c:@u
+// CHECK-DAG: 4:c:@u
 
 // No USR can be generated for this.
 // Check for no crash in this case.
@@ -48,3 +48,15 @@
   float uf;
   const int ui;
 };
+
+void f(int (*)(char));
+void f(bool (*)(char));
+
+struct G {
+  G() {
+f([](char) -> int { return 42; });
+// CHECK-DAG: 41:c:@S@G@F@G#@Sa@F@operator int (*)(char)#1
+f([](char) -> bool { return true; });
+// CHECK-DAG: 42:c:@S@G@F@G#@Sa@F@operator bool (*)(char)#1
+  }
+};
Index: clang/test/Analysis/ctu-lookup-name-with-space.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-lookup-name-with-space.cpp
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: echo '41:c:@S@G@F@G#@Sa@F@operator void (*)(int)#1 %/t/importee.ast' >> %t/externalDefMap.txt
+// RUN: echo '38:c:@S@G@F@G#@Sa@F@operator void (*)()#1 %/t/importee.ast' >> %t/externalDefMap.txt
+// RUN: echo '14:c:@F@importee# %/t/importee.ast' >> %t/externalDefMap.txt
+// RUN: %clang_cc1 -emit-pch %/S/Inputs/ctu-lookup-name-with-space.cpp -o %t/importee.ast
+
+// RUN: cd %t
+// RUN: %clang_cc1 -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=. \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -verify %s 2>&1 | FileCheck %s
+
+// CHECK: CTU loaded AST file
+
+void importee();
+
+void trigger() {
+  // Call an external function to trigger the parsing process of CTU index.
+  // Refer to file Inputs/ctu-lookup-name-with-space.cpp for more details.
+
+  importee(); // expected-no-diagnostics
+}
Index: clang/test/Analysis/ctu-inherited-default-ctor.cpp
===
--- clang/test/Analysis/ctu-inherited-default-ctor.cpp
+++ clang/test/Analysis/ctu-inherited-default-ctor.cpp
@@ -4,7 +4,7 @@
 // RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
 // RUN:   -emit-pch -o %t/ctudir/ctu-inherited-default-ctor-other.cpp.ast \
 // RUN:%S/Inputs/ctu-inherited-default-ctor-other.cpp
-// RUN: echo "c:@N@clang@S@DeclContextLookupResult@SingleElementDummyList ctu-inherited-default-ctor-other.cpp.ast" \
+// RUN: echo 

[PATCH] D116513: [clang-tidy] Fix bugs in misc-unused-parameters for Constructors calls site

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Thanks @aaron.ballman for the review!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
 // CHECK-FIXES: C() {}
   C(int i) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning

aaron.ballman wrote:
> I think this fix is incorrect and should not be applied; it changes the 
> meaning of the interface from having a converting constructor to having a 
> default constructor. I think that needs to be optional behavior because it's 
> a pretty invasive change to apply automatically. This patch builds on top of 
> the existing incorrect behavior, but would be fine if it was only applied 
> when the option to modify constructors is enabled.
I'm not against an option, but I'd like to get to a default behavior that is 
"safe". So I guess my first patch should be to undo the transformation 
happening here in the test already.
We can either never touch any C++ constructor or try to find a conservative 
logic for it.
I mentioned in the other review that we may avoid touching a Ctor with a single 
parameter. 

Then we also can't do it for a Ctor with two parameters as it'll turn it into a 
converting ctor. Unless you can eliminate both parameters, in which case it is 
become a default ctor (which can conflict with an existing one, in which case 
it can be just deleted?)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116513

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

[PATCH] D116717: [CodeCompletion] Complete designators for fields in anonymous structs/unions

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added a subscriber: usaxena95.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

Fixes https://github.com/clangd/clangd/issues/836


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116717

Files:
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/desig-init.cpp


Index: clang/test/CodeCompletion/desig-init.cpp
===
--- clang/test/CodeCompletion/desig-init.cpp
+++ clang/test/CodeCompletion/desig-init.cpp
@@ -77,3 +77,11 @@
   // CHECK-SIGNATURE-REGRESSION-NOT: OVERLOAD: [#int#]wrongFunction
 }
 
+struct WithAnon {
+  int outer;
+  struct { int inner; };
+};
+auto TestWithAnon = WithAnon { .inner = 2 };
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-patterns 
-code-completion-at=%s:84:33 %s -o - -std=c++2a | FileCheck 
-check-prefix=CHECK-CC5 %s
+  // CHECK-CC5: COMPLETION: inner : [#int#]inner
+  // CHECK-CC5: COMPLETION: outer : [#int#]outer
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -6344,7 +6344,15 @@
 CodeCompleter->getCodeCompletionTUInfo(), CCC);
 
   Results.EnterNewScope();
-  for (const auto *FD : RD->fields()) {
+  for (const Decl *D : RD->decls()) {
+const FieldDecl *FD;
+if (auto *IFD = dyn_cast(D))
+  FD = IFD->getAnonField();
+else if (auto *DFD = dyn_cast(D))
+  FD = DFD;
+else
+  continue;
+
 // FIXME: Make use of previous designators to mark any fields before those
 // inaccessible, and also compute the next initializer priority.
 ResultBuilder::Result Result(FD, Results.getBasePriority(FD));


Index: clang/test/CodeCompletion/desig-init.cpp
===
--- clang/test/CodeCompletion/desig-init.cpp
+++ clang/test/CodeCompletion/desig-init.cpp
@@ -77,3 +77,11 @@
   // CHECK-SIGNATURE-REGRESSION-NOT: OVERLOAD: [#int#]wrongFunction
 }
 
+struct WithAnon {
+  int outer;
+  struct { int inner; };
+};
+auto TestWithAnon = WithAnon { .inner = 2 };
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:84:33 %s -o - -std=c++2a | FileCheck -check-prefix=CHECK-CC5 %s
+  // CHECK-CC5: COMPLETION: inner : [#int#]inner
+  // CHECK-CC5: COMPLETION: outer : [#int#]outer
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -6344,7 +6344,15 @@
 CodeCompleter->getCodeCompletionTUInfo(), CCC);
 
   Results.EnterNewScope();
-  for (const auto *FD : RD->fields()) {
+  for (const Decl *D : RD->decls()) {
+const FieldDecl *FD;
+if (auto *IFD = dyn_cast(D))
+  FD = IFD->getAnonField();
+else if (auto *DFD = dyn_cast(D))
+  FD = DFD;
+else
+  continue;
+
 // FIXME: Make use of previous designators to mark any fields before those
 // inaccessible, and also compute the next initializer priority.
 ResultBuilder::Result Result(FD, Results.getBasePriority(FD));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+   a human reader, and there's basically no place for a bug to hide. On the 
other
+   hand for non-public functions, all the call-sites are visible and the 
parameter
+   can be eliminated entirely.

aaron.ballman wrote:
> Call sites are not always visible for protected functions, so this seems a 
> bit suspicious. The changes are missing test coverage for that situation.
You're using `public` for "access control" while I was using the linkage 
aspect: my reasoning is that if a method isn't "externally visible" from the 
current translation unit, you see all the call-sites. This is orthogonal to 
public/private/protected as far as I know.

I am likely missing a check for "isDefinedInMainFile" (or whatever the api is 
called) to not flag included headers.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
+// CHECK-FIXES: C() {}
+  C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning

aaron.ballman wrote:
> I think this demonstrates a bad fix -- this changes code meaning from being a 
> converting constructor to being a default constructor, which are very 
> different things.
Oh you're right: so we can't do it for a Ctor with a single parameter...

But we also can't do it for a Ctor with two parameters as it'll turn it into a 
converting ctor. Unless you can eliminate both parameters, in which case it is 
become a default ctor (which can conflict with an existing one, in which case 
it can be just deleted?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116512

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

[PATCH] D116351: Update Bug report URL to Github Issues

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 397763.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D116351

Files:
  clang-tools-extra/docs/clang-doc.rst
  clang/docs/CommandGuide/clang.rst
  clang/www/c_status.html
  clang/www/cxx_status.html
  clang/www/get_involved.html
  clang/www/get_started.html
  clang/www/menu.html.incl
  libcxx/docs/index.rst
  libunwind/docs/index.rst
  lldb/docs/index.rst
  llvm/CMakeLists.txt
  llvm/docs/CommandGuide/llvm-install-name-tool.rst
  llvm/docs/CommandGuide/llvm-libtool-darwin.rst
  llvm/docs/CommandGuide/llvm-lipo.rst
  llvm/docs/CommandGuide/llvm-objcopy.rst
  llvm/docs/CommandGuide/llvm-objdump.rst
  llvm/docs/CommandGuide/llvm-otool.rst
  llvm/docs/CommandGuide/llvm-size.rst
  llvm/docs/CommandGuide/llvm-strings.rst
  llvm/docs/CommandGuide/llvm-strip.rst
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
  utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h

Index: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
===
--- utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -24,7 +24,7 @@
 #include "llvm/Config/llvm-config.h"
 
 /* Bug report URL. */
-#define BUG_REPORT_URL "https://bugs.llvm.org/;
+#define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Define to 1 to enable backtraces, and to 0 otherwise. */
 #define ENABLE_BACKTRACES 1
@@ -332,7 +332,7 @@
 /* LTDL_SHLIB_EXT defined in Bazel */
 
 /* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "https://bugs.llvm.org/;
+#define PACKAGE_BUGREPORT "https://github.com/llvm/llvm-project/issues/;
 
 /* Define to the full name of this package. */
 #define PACKAGE_NAME "LLVM"
Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -20,7 +20,7 @@
 #define CLANG_CONFIG_H
 
 /* Bug report URL. */
-#define BUG_REPORT_URL "https://bugs.llvm.org/;
+#define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Default to -fPIE and -pie on Linux. */
 #define CLANG_DEFAULT_PIE_ON_LINUX 0
Index: llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
@@ -72,7 +72,7 @@
   input = "config.h.cmake"
   output = "$target_gen_dir/config.h"
   values = [
-"BUG_REPORT_URL=https://bugs.llvm.org/;,
+"BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
 "ENABLE_BACKTRACES=1",
 "ENABLE_CRASH_OVERRIDES=1",
 "BACKTRACE_HEADER=execinfo.h",
@@ -120,7 +120,7 @@
 "LLVM_VERSION_INFO=",
 "LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO=1",
 "LLVM_WINDOWS_PREFER_FORWARD_SLASH=",
-"PACKAGE_BUGREPORT=https://bugs.llvm.org/;,
+"PACKAGE_BUGREPORT=https://github.com/llvm/llvm-project/issues/;,
 "PACKAGE_NAME=LLVM",
 "PACKAGE_STRING=LLVM ${llvm_version}git",
 "PACKAGE_VERSION=${llvm_version}git",
Index: llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
+++ llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
@@ -8,7 +8,7 @@
   input = "config.h.cmake"
   output = "$target_gen_dir/config.h"
   values = [
-"BUG_REPORT_URL=https://bugs.llvm.org/;,
+"BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
 "CLANG_DEFAULT_PIE_ON_LINUX=",
 "CLANG_DEFAULT_LINKER=",
 "CLANG_DEFAULT_STD_C=",
Index: llvm/docs/CommandGuide/llvm-strip.rst
===
--- llvm/docs/CommandGuide/llvm-strip.rst
+++ llvm/docs/CommandGuide/llvm-strip.rst
@@ -194,7 +194,7 @@
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .
 
 SEE ALSO
 
Index: llvm/docs/CommandGuide/llvm-strings.rst
===
--- llvm/docs/CommandGuide/llvm-strings.rst
+++ llvm/docs/CommandGuide/llvm-strings.rst
@@ -123,4 +123,4 @@
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .
Index: 

[PATCH] D116256: [-fms-extensions] Make some exception specification warnings/errors compatible with what cl.exe does

[Amy Huang via Phabricator via cfe-commits]

akhuang added a comment.

In D116256#3215801 , @thakis wrote:

> Thanks for the patch! This looks roughly right to me.
>
> Maybe the list of ESTs that are allowed to be mismatched should be opt-in 
> instead of opt-out? (i.e. instead of checking for "not EST_BasicNoexcept /  
> EST_DependentNoexcept", check for EST_NoThrow (I think?) Not sure which way 
> is better.
>
> (Looks like the old code was added for https://llvm.org/PR25265)
>
> Could you check that we still emit the warning on line 5 in 
> https://godbolt.org/z/bxfx8jsjd ? The test is mostly from 
> https://docs.microsoft.com/en-us/cpp/build/reference/zc-noexcepttypes?view=msvc-170
>  -- it feels like we might want to have different behavior in c++17 (and 
> later) and c++14 (and earlier) for some of the diags, possibly. Looks like 
> MSVC also has an error on line 15 by default (with /std:c++17, it seems to 
> accept it with /std:c++14), while we only warn.
>
> (I'm a bit surprised the `noexcept` bit doesn't make it into the mangled name 
> in the ms abi, but we're consistent with msvc about this so that's all good.)

It did not still emit the warning but I've changed it now. I'm not sure why 
https://llvm.org/PR25265 added a new warning 
(ext_ms_missing_exception_specification) instead of just using 
ext_missing_exception_specification; I guess I'll get rid of it anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116256

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

[PATCH] D116256: [-fms-extensions] Make some exception specification warnings/errors compatible with what cl.exe does

[Amy Huang via Phabricator via cfe-commits]

akhuang updated this revision to Diff 397760.
akhuang marked an inline comment as done.
akhuang added a comment.

Fix warning behavior


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/test/SemaCXX/MicrosoftCompatibility.cpp


Index: clang/test/SemaCXX/MicrosoftCompatibility.cpp
===
--- clang/test/SemaCXX/MicrosoftCompatibility.cpp
+++ clang/test/SemaCXX/MicrosoftCompatibility.cpp
@@ -377,14 +377,14 @@
 #endif
 };
 
-}
+void f4() throw(); // expected-note {{previous declaration is here}}
+void f4() {}   // expected-warning {{'f4' is missing exception 
specification 'throw()'}}
 
-namespace PR25265 {
-struct S {
-  int fn() throw(); // expected-note {{previous declaration is here}}
-};
+__declspec(nothrow) void f5();
+void f5() {}
 
-int S::fn() { return 0; } // expected-warning {{is missing exception 
specification}}
+void f6() noexcept; // expected-note {{previous declaration is here}}
+void f6() {}// expected-error {{'f6' is missing exception 
specification 'noexcept'}}
 }
 
 namespace PR43265 {
Index: clang/lib/Sema/SemaExceptionSpec.cpp
===
--- clang/lib/Sema/SemaExceptionSpec.cpp
+++ clang/lib/Sema/SemaExceptionSpec.cpp
@@ -391,9 +391,8 @@
 NewProto->getExtProtoInfo().withExceptionSpec(ESI)));
   }
 
-  if (getLangOpts().MSVCCompat && ESI.Type != EST_DependentNoexcept) {
-// Allow missing exception specifications in redeclarations as an 
extension.
-DiagID = diag::ext_ms_missing_exception_specification;
+  if (getLangOpts().MSVCCompat && isDynamicExceptionSpec(ESI.Type)) {
+DiagID = diag::ext_missing_exception_specification;
 ReturnValueOnError = false;
   } else if (New->isReplaceableGlobalAllocationFunction() &&
  ESI.Type != EST_DependentNoexcept) {
@@ -402,6 +401,10 @@
 DiagID = diag::ext_missing_exception_specification;
 ReturnValueOnError = false;
   } else if (ESI.Type == EST_NoThrow) {
+// Don't emit any warning for missing 'nothrow' in MSVC.
+if (getLangOpts().MSVCCompat) {
+  return false;
+}
 // Allow missing attribute 'nothrow' in redeclarations, since this is a 
very
 // common omission.
 DiagID = diag::ext_missing_exception_specification;
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1695,9 +1695,6 @@
 def ext_missing_exception_specification : ExtWarn<
   err_missing_exception_specification.Text>,
   InGroup>;
-def ext_ms_missing_exception_specification : ExtWarn<
-  err_missing_exception_specification.Text>,
-  InGroup;
 def err_noexcept_needs_constant_expression : Error<
   "argument to noexcept specifier must be a constant expression">;
 def err_exception_spec_not_parsed : Error<


Index: clang/test/SemaCXX/MicrosoftCompatibility.cpp
===
--- clang/test/SemaCXX/MicrosoftCompatibility.cpp
+++ clang/test/SemaCXX/MicrosoftCompatibility.cpp
@@ -377,14 +377,14 @@
 #endif
 };
 
-}
+void f4() throw(); // expected-note {{previous declaration is here}}
+void f4() {}   // expected-warning {{'f4' is missing exception specification 'throw()'}}
 
-namespace PR25265 {
-struct S {
-  int fn() throw(); // expected-note {{previous declaration is here}}
-};
+__declspec(nothrow) void f5();
+void f5() {}
 
-int S::fn() { return 0; } // expected-warning {{is missing exception specification}}
+void f6() noexcept; // expected-note {{previous declaration is here}}
+void f6() {}// expected-error {{'f6' is missing exception specification 'noexcept'}}
 }
 
 namespace PR43265 {
Index: clang/lib/Sema/SemaExceptionSpec.cpp
===
--- clang/lib/Sema/SemaExceptionSpec.cpp
+++ clang/lib/Sema/SemaExceptionSpec.cpp
@@ -391,9 +391,8 @@
 NewProto->getExtProtoInfo().withExceptionSpec(ESI)));
   }
 
-  if (getLangOpts().MSVCCompat && ESI.Type != EST_DependentNoexcept) {
-// Allow missing exception specifications in redeclarations as an extension.
-DiagID = diag::ext_ms_missing_exception_specification;
+  if (getLangOpts().MSVCCompat && isDynamicExceptionSpec(ESI.Type)) {
+DiagID = diag::ext_missing_exception_specification;
 ReturnValueOnError = false;
   } else if (New->isReplaceableGlobalAllocationFunction() &&
  ESI.Type != EST_DependentNoexcept) {
@@ -402,6 +401,10 @@
 DiagID = diag::ext_missing_exception_specification;
 ReturnValueOnError = false;
   } else if (ESI.Type == EST_NoThrow) {
+// Don't emit any warning for missing 'nothrow' in MSVC.
+if 

[PATCH] D115183: [clang][HeaderSearch] Support framework includes in suggestPath...

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, test seems sensible.
I remain unconvinced that the umbrella stuff belongs here.
Maybe pull it out of this patch and send it separately if you want to discuss 
further, or get more opinions?




Comment at: clang/lib/Lex/HeaderSearch.cpp:1962
+} else {
+  // Prefer using `` for non-`UIKit` main files.
+  //

dgoldman wrote:
> sammccall wrote:
> > This heuristic looks iffy to me, it's not just trying to help the caller 
> > insert the right header, but tell them they want a different one for style 
> > reasons.
> > 
> > The first example framework in 
> > https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html
> >  doesn't include an umbrella header of this form.
> > 
> > Moreover, non-framework libraries have umbrella headers too, this seems 
> > like an orthogonal concern best caught in some other way.
> See 
> https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html,
>  it's common to use this umbrella header (especially for Apple SDKs) and some 
> frameworks require it (as opposed to including the header directly), thus I 
> think this default makes sense even if it isn't 100%.
> 
> Any other ideas on how we can help here? I guess if we had FS access we could 
> see if the umbrella header exists...
I get that it's often a good idea/mandatory to include the umbrella header (for 
many libraries, not just frameworks), but it's not this function's job to 
choose which file to include, it's to find the best spelling of the file that 
was requested.

Similarly it doesn't check if the filename is "bits/shared_ptr.h" and suggest 
`` instead.

> Any other ideas on how we can help here?
It depends what you're trying to achieve, maybe write a clang-tidy check 
instead? Or if this is something the compiler itself needs to care about, write 
a separate function to do the heuristic correction.
If you need the spelled, heuristically-corrected name, you can correct it first 
and then call this code, rather than having the spelling code do heuristic 
correction.



Comment at: clang/test/Modules/double-quotes.m:27
 // CHECK: double-quoted include "A0.h" in framework header, expected 
angle-bracketed instead
+// CHECK: 
 // CHECK: double-quoted include "B.h" in framework header, expected 
angle-bracketed instead

it's hard to understand the context of how these headers are named, is there 
any other text on the line you can include in the CHECK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115183

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

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in 
function declaration

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > LegalizeAdulthood wrote:
> > > aaron.ballman wrote:
> > > > Can you also add a test for:
> > > > ```
> > > > void func(return_t(void));
> > > > ```
> > > `:-)`
> > > 
> > > What are you suggesting the result should be?  Honestly, looking at that, 
> > > I'm not sure myself `:)`
> > > 
> > > IMO, if I saw this in a code review, I would flag it because you're using 
> > > a macro called "return type" to specify the type of an argument.
> > LoL, yeah, the name `return_t` would certainly be novel to use in a 
> > parameter list, but what I was hoping to test is whether we try to fix the 
> > use of the macro within the parameter list or not. I *think* it probably 
> > makes sense to issue the diagnostic, but I don't think it makes sense to 
> > try to fix it because the macro could be defined differently for different 
> > configurations. But the diagnostic is silenced as well as the fix-it, I 
> > wouldn't lose a whole lot of sleep over it.
> Well it could conceivably be used to declare a function pointer argument like 
> this:
> 
> `void func(return_t(void) (*fp)(void));`
> 
> In that case, my expectation is that the check would fix the void arg, but 
> not the arg to the macro.
OK, that was a good idea to add the test I described above because it failed 
`:)`,
so let me improve the check some more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116425

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

[PATCH] D114908: [clang] Don't call inheritDefaultTemplateArguments() on CXXDeductionGuideDecl's template parameters

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Friendly ping for a modules CTAD bugfix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

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

[PATCH] D116713: [clangd] Support configuration of inlay hints.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: nridge, hokein.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

The idea is that the feature will always be advertised at the LSP level, but
depending on config we'll return partial or no responses.

We try to avoid doing the analysis for hints we're not going to return.

Examples of syntax:

  # No hints
  InlayHints:
Enabled: No
  ---
  # Turn off a default category
  InlayHints:
ParameterNames: No
  ---
  # Turn on some categories
  InlayHints:
ParameterNames: Yes
DeducedTypes: Yes


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116713

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -6,11 +6,13 @@
 //
 //===--===//
 #include "Annotations.h"
+#include "Config.h"
 #include "InlayHints.h"
 #include "Protocol.h"
 #include "TestTU.h"
 #include "TestWorkspace.h"
 #include "XRefs.h"
+#include "support/Context.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -23,6 +25,7 @@
 
 namespace {
 
+using ::testing::IsEmpty;
 using ::testing::UnorderedElementsAre;
 
 std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
@@ -49,6 +52,13 @@
  arg.range == Code.range(Expected.RangeName);
 }
 
+Config noHintsConfig() {
+  Config C;
+  C.InlayHints.Parameters = false;
+  C.InlayHints.DeducedTypes = false;
+  return C;
+}
+
 template 
 void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
  ExpectedHints... Expected) {
@@ -59,6 +69,10 @@
 
   EXPECT_THAT(hintsOfKind(AST, Kind),
   UnorderedElementsAre(HintMatcher(Expected, Source)...));
+  // Sneak in a cross-cutting check that hints are disabled by config.
+  // We'll hit an assertion failure if addInlayHint still gets called.
+  WithContextValue WithCfg(Config::Key, noHintsConfig());
+  EXPECT_THAT(inlayHints(AST), IsEmpty());
 }
 
 template 
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -228,6 +228,23 @@
   ASSERT_EQ(Results.size(), 1u);
   EXPECT_THAT(Results[0].Hover.ShowAKA, llvm::ValueIs(Val(true)));
 }
+
+TEST(ParseYAML, InlayHints) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+InlayHints:
+  Enabled: No
+  ParameterNames: Yes
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].InlayHints.Enabled, llvm::ValueIs(Val(false)));
+  EXPECT_THAT(Results[0].InlayHints.ParameterNames, llvm::ValueIs(Val(true)));
+  EXPECT_EQ(Results[0].InlayHints.DeducedTypes, llvm::None);
+}
+
 } // namespace
 } // namespace config
 } // namespace clangd
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 #include "InlayHints.h"
+#include "Config.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
 #include "support/Logger.h"
@@ -20,8 +21,9 @@
 
 class InlayHintVisitor : public RecursiveASTVisitor {
 public:
-  InlayHintVisitor(std::vector , ParsedAST )
-  : Results(Results), AST(AST.getASTContext()),
+  InlayHintVisitor(std::vector , ParsedAST ,
+   const Config )
+  : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
 MainFileID(AST.getSourceManager().getMainFileID()),
 Resolver(AST.getHeuristicResolver()),
 TypeHintPolicy(this->AST.getPrintingPolicy()),
@@ -46,6 +48,9 @@
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+  return true;
+
 // Weed out constructor calls that don't look like a function call with
 // an argument list, by checking the validity of getParenOrBraceRange().
 // Also weed out std::initializer_list constructors as there are no names
@@ -61,6 +66,9 @@
   }
 
   bool VisitCallExpr(CallExpr *E) {
+if 

[PATCH] D116615: [Clang] Extract availability mapping from VersionMap for watchOS/tvOS

[Alex Lorenz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG809c6a5a1d2f: [Clang] Extract availability mapping from 
VersionMap for watchOS/tvOS (authored by egorzhdan, committed by arphaman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116615

Files:
  clang/lib/Basic/DarwinSDKInfo.cpp
  clang/unittests/Basic/DarwinSDKInfoTest.cpp

Index: clang/unittests/Basic/DarwinSDKInfoTest.cpp
===
--- clang/unittests/Basic/DarwinSDKInfoTest.cpp
+++ clang/unittests/Basic/DarwinSDKInfoTest.cpp
@@ -13,7 +13,68 @@
 using namespace llvm;
 using namespace clang;
 
-TEST(DarwinSDKInfoTest, ParseAndTestMapping) {
+// Check the version mapping logic in DarwinSDKInfo.
+TEST(DarwinSDKInfo, VersionMapping) {
+  llvm::json::Object Obj({{"3.0", "1.0"}, {"3.1", "1.2"}});
+  Optional Mapping =
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj,
+VersionTuple());
+  EXPECT_TRUE(Mapping.hasValue());
+  EXPECT_EQ(Mapping->getMinimumValue(), VersionTuple(1));
+
+  // Exact mapping.
+  EXPECT_EQ(Mapping->map(VersionTuple(3), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0, 0), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 1), VersionTuple(0, 1), None),
+VersionTuple(1, 2));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 1, 0), VersionTuple(0, 1), None),
+VersionTuple(1, 2));
+
+  // Missing mapping - fallback to major.
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0, 1), VersionTuple(0, 1), None),
+VersionTuple(1));
+
+  // Minimum
+  EXPECT_EQ(Mapping->map(VersionTuple(2), VersionTuple(0, 1), None),
+VersionTuple(0, 1));
+
+  // Maximum
+  EXPECT_EQ(
+  Mapping->map(VersionTuple(4), VersionTuple(0, 1), VersionTuple(100)),
+  VersionTuple(100));
+}
+
+// Check the version mapping logic in DarwinSDKInfo.
+TEST(DarwinSDKInfo, VersionMappingMissingKey) {
+  llvm::json::Object Obj({{"3.0", "1.0"}, {"5.0", "1.2"}});
+  Optional Mapping =
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj,
+VersionTuple());
+  EXPECT_TRUE(Mapping.hasValue());
+  EXPECT_EQ(
+  Mapping->map(VersionTuple(4), VersionTuple(0, 1), VersionTuple(100)),
+  None);
+}
+
+TEST(DarwinSDKInfo, VersionMappingParseEmpty) {
+  llvm::json::Object Obj({});
+  EXPECT_FALSE(
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj, VersionTuple())
+  .hasValue());
+}
+
+TEST(DarwinSDKInfo, VersionMappingParseError) {
+  llvm::json::Object Obj({{"test", "1.2"}});
+  EXPECT_FALSE(
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj, VersionTuple())
+  .hasValue());
+}
+
+TEST(DarwinSDKInfoTest, ParseAndTestMappingMacCatalyst) {
   llvm::json::Object Obj;
   Obj["Version"] = "11.0";
   Obj["MaximumDeploymentTarget"] = "11.99";
@@ -58,6 +119,51 @@
 VersionTuple(99, 99));
 }
 
+TEST(DarwinSDKInfoTest, ParseAndTestMappingIOSDerived) {
+  llvm::json::Object Obj;
+  Obj["Version"] = "15.0";
+  Obj["MaximumDeploymentTarget"] = "15.0.99";
+  llvm::json::Object VersionMap;
+  VersionMap["10.0"] = "10.0";
+  VersionMap["10.3.1"] = "10.2";
+  VersionMap["11.0"] = "11.0";
+  llvm::json::Object IOSToTvOS;
+  IOSToTvOS["iOS_tvOS"] = std::move(VersionMap);
+  Obj["VersionMap"] = std::move(IOSToTvOS);
+
+  auto SDKInfo = DarwinSDKInfo::parseDarwinSDKSettingsJSON();
+  ASSERT_TRUE(SDKInfo);
+  EXPECT_EQ(SDKInfo->getVersion(), VersionTuple(15, 0));
+
+  // Verify that mapping is present for platforms that derive from iOS.
+  const auto *Mapping = SDKInfo->getVersionMapping(DarwinSDKInfo::OSEnvPair(
+  llvm::Triple::IOS, llvm::Triple::UnknownEnvironment, llvm::Triple::TvOS,
+  llvm::Triple::UnknownEnvironment));
+  ASSERT_TRUE(Mapping);
+
+  // Verify that the iOS versions that are present in the map are translated
+  // directly to their corresponding tvOS versions.
+  EXPECT_EQ(*Mapping->map(VersionTuple(10, 0), VersionTuple(), None),
+VersionTuple(10, 0));
+  EXPECT_EQ(*Mapping->map(VersionTuple(10, 3, 1), VersionTuple(), None),
+VersionTuple(10, 2));
+  EXPECT_EQ(*Mapping->map(VersionTuple(11, 0), VersionTuple(), None),
+VersionTuple(11, 0));
+
+  // Verify that an iOS version that's not present in the map is translated
+  // like the nearest major OS version.
+  EXPECT_EQ(*Mapping->map(VersionTuple(10, 1), VersionTuple(), None),
+VersionTuple(10, 0));
+
+  // Verify that the iOS versions that are outside of the mapped version
+  // range map to the 

[clang] 809c6a5 - [Clang] Extract availability mapping from VersionMap for watchOS/tvOS

[Alex Lorenz via cfe-commits]

Author: Egor Zhdan
Date: 2022-01-05T17:00:03-08:00
New Revision: 809c6a5a1d2f4366ab0e602c9d963b73f380b74e

URL: 
https://github.com/llvm/llvm-project/commit/809c6a5a1d2f4366ab0e602c9d963b73f380b74e
DIFF: 
https://github.com/llvm/llvm-project/commit/809c6a5a1d2f4366ab0e602c9d963b73f380b74e.diff

LOG: [Clang] Extract availability mapping from VersionMap for watchOS/tvOS

This change makes it possible to extract iOS-to-another-platform version 
mappings from `VersionMap` in the `SDKSettings.json` file in Darwin SDKs, for 
example, `iOS_watchOS` and `iOS_tvOS`.

This code was originally authored by Alex Lorenz.

rdar://81491680

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

Added: 


Modified: 
clang/lib/Basic/DarwinSDKInfo.cpp
clang/unittests/Basic/DarwinSDKInfoTest.cpp

Removed: 




diff  --git a/clang/lib/Basic/DarwinSDKInfo.cpp 
b/clang/lib/Basic/DarwinSDKInfo.cpp
index fe35f77782c92..64bcb45a4cd85 100644
--- a/clang/lib/Basic/DarwinSDKInfo.cpp
+++ b/clang/lib/Basic/DarwinSDKInfo.cpp
@@ -84,6 +84,25 @@ DarwinSDKInfo::parseDarwinSDKSettingsJSON(const 
llvm::json::Object *Obj) {
   llvm::DenseMap>
   VersionMappings;
   if (const auto *VM = Obj->getObject("VersionMap")) {
+// FIXME: Generalize this out beyond iOS-deriving targets.
+// Look for ios_ version mapping for targets that derive from 
ios.
+for (const auto  : *VM) {
+  auto Pair = StringRef(KV.getFirst()).split("_");
+  if (Pair.first.compare_insensitive("ios") == 0) {
+llvm::Triple TT(llvm::Twine("--") + Pair.second.lower());
+if (TT.getOS() != llvm::Triple::UnknownOS) {
+  auto Mapping = RelatedTargetVersionMapping::parseJSON(
+  *KV.getSecond().getAsObject(), *MaximumDeploymentVersion);
+  if (Mapping)
+VersionMappings[OSEnvPair(llvm::Triple::IOS,
+  llvm::Triple::UnknownEnvironment,
+  TT.getOS(),
+  llvm::Triple::UnknownEnvironment)
+.Value] = std::move(Mapping);
+}
+  }
+}
+
 if (const auto *Mapping = VM->getObject("macOS_iOSMac")) {
   auto VersionMap = RelatedTargetVersionMapping::parseJSON(
   *Mapping, *MaximumDeploymentVersion);

diff  --git a/clang/unittests/Basic/DarwinSDKInfoTest.cpp 
b/clang/unittests/Basic/DarwinSDKInfoTest.cpp
index f845e1536da8a..aa1feeb293c0e 100644
--- a/clang/unittests/Basic/DarwinSDKInfoTest.cpp
+++ b/clang/unittests/Basic/DarwinSDKInfoTest.cpp
@@ -13,7 +13,68 @@
 using namespace llvm;
 using namespace clang;
 
-TEST(DarwinSDKInfoTest, ParseAndTestMapping) {
+// Check the version mapping logic in DarwinSDKInfo.
+TEST(DarwinSDKInfo, VersionMapping) {
+  llvm::json::Object Obj({{"3.0", "1.0"}, {"3.1", "1.2"}});
+  Optional Mapping =
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj,
+VersionTuple());
+  EXPECT_TRUE(Mapping.hasValue());
+  EXPECT_EQ(Mapping->getMinimumValue(), VersionTuple(1));
+
+  // Exact mapping.
+  EXPECT_EQ(Mapping->map(VersionTuple(3), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0, 0), VersionTuple(0, 1), None),
+VersionTuple(1));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 1), VersionTuple(0, 1), None),
+VersionTuple(1, 2));
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 1, 0), VersionTuple(0, 1), None),
+VersionTuple(1, 2));
+
+  // Missing mapping - fallback to major.
+  EXPECT_EQ(Mapping->map(VersionTuple(3, 0, 1), VersionTuple(0, 1), None),
+VersionTuple(1));
+
+  // Minimum
+  EXPECT_EQ(Mapping->map(VersionTuple(2), VersionTuple(0, 1), None),
+VersionTuple(0, 1));
+
+  // Maximum
+  EXPECT_EQ(
+  Mapping->map(VersionTuple(4), VersionTuple(0, 1), VersionTuple(100)),
+  VersionTuple(100));
+}
+
+// Check the version mapping logic in DarwinSDKInfo.
+TEST(DarwinSDKInfo, VersionMappingMissingKey) {
+  llvm::json::Object Obj({{"3.0", "1.0"}, {"5.0", "1.2"}});
+  Optional Mapping =
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj,
+VersionTuple());
+  EXPECT_TRUE(Mapping.hasValue());
+  EXPECT_EQ(
+  Mapping->map(VersionTuple(4), VersionTuple(0, 1), VersionTuple(100)),
+  None);
+}
+
+TEST(DarwinSDKInfo, VersionMappingParseEmpty) {
+  llvm::json::Object Obj({});
+  EXPECT_FALSE(
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj, 
VersionTuple())
+  .hasValue());
+}
+
+TEST(DarwinSDKInfo, VersionMappingParseError) {
+  llvm::json::Object Obj({{"test", "1.2"}});
+  EXPECT_FALSE(
+  DarwinSDKInfo::RelatedTargetVersionMapping::parseJSON(Obj, 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

delcypher wrote:
> delcypher wrote:
> > delcypher wrote:
> > > @NoQ Any ideas about this?  It seems kind of weird that when merging 
> > > `not_a_prototype3` prototype with the K style definition of 
> > > `not_a_prototype3` that the resulting FunctionDecl we see at the call 
> > > site in `call_to_function_without_prototype3` is marked as not having a 
> > > prototype.
> > > 
> > > If I flip the order (see `not_a_prototype6`) then the merged declaration 
> > > is marked as having a prototype.
> > > 
> > > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this 
> > > just a peculiarity of K style function definitions.
> > I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> > 
> > ```lang=c++
> >// C: Function types need to be compatible, not identical. This handles
> >   // duplicate function decls like "void f(int); void f(enum X);" properly.
> >   if (!getLangOpts().CPlusPlus &&
> >   Context.typesAreCompatible(OldQType, NewQType)) {
> > const FunctionType *OldFuncType = OldQType->getAs();
> > const FunctionType *NewFuncType = NewQType->getAs();
> > const FunctionProtoType *OldProto = nullptr;
> > if (MergeTypeWithOld && isa(NewFuncType) &&
> > (OldProto = dyn_cast(OldFuncType))) {
> >   // The old declaration provided a function prototype, but the
> >   // new declaration does not. Merge in the prototype.
> > ```
> > 
> > ` isa(NewFuncType)` is false in this particular case, 
> > however `New` doesn't have a prototype (i.e. `New->hasPrototype()` is 
> > false). One fix might be to replace `isa(NewFuncType)` 
> > with 
> > 
> > ```
> > (isa(NewFuncType) || !New->hasPrototype())
> > ```
> > 
> > However, I don't really know this code well enough to know if that's the 
> > right fix.
> Okay. I think the above would actually be the wrong location for a fix 
> because in this case we don't need to go down the path that synthesizes the 
> parameters because we already know them for both `old` and `new` in this 
> situation.
> 
> Instead I think the change would have to be in 
> `Sema::MergeCompatibleFunctionDecls` to do something like.
> 
> ```lang=c++
>   // If New is a K function definition it will be marked
>   // as not having a prototype. If `Old` has a prototype
>   // then to "merge" we should mark the K function as having a prototype.
>   if (!getLangOpts().CPlusPlus && Old->hasPrototype() && !New->hasPrototype())
> New->setHasInheritedPrototype(); 
> ```
> 
> What I'm not sure about is if this is semantically the right thing to do. 
> Thoughts?
Ok dunno but I definitely find this whole thing surprising. I'd expect this 
example to be the entirely normal situation for this code, where it sees that 
the new declaration has no prototype so it "inherits" it from the old 
declaration. But you're saying that

> `isa(NewFuncType)` is false in this particular case

Where does that proto-type come from then? I only see this code affecting the 
type
```lang=c++
   3523   if (RequiresAdjustment) {
   3524 const FunctionType *AdjustedType = 
New->getType()->getAs();
   3525 AdjustedType = Context.adjustFunctionType(AdjustedType, 
NewTypeInfo);
   3526 New->setType(QualType(AdjustedType, 0));
   3527 NewQType = Context.getCanonicalType(New->getType());
   3528   }
```
which doesn't seem to touch no-proto-types:
```lang=c++
   3094 const FunctionType *ASTContext::adjustFunctionType(const FunctionType 
*T,
   3095
FunctionType::ExtInfo Info) {
   3096   if (T->getExtInfo() == Info)
   3097 return T;
   3098
   3099   QualType Result;
   3100   if (const auto *FNPT = dyn_cast(T)) {
   3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
   ...
   3107   }
   3108
   3109   return cast(Result.getTypePtr());
   3110 }
```
So it sounds like `New->getType()` was already with prototype from the start? 
Maybe whatever code has set that type should also have set the 
`HasInheritedPrototype` flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Kevin Athey via Phabricator via cfe-commits]

kda added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+if ((CodeGenOpts.EnableNoundefAttrs ||
+ CodeGenOpts.SanitizeMemoryParamRetval) &&
+ArgNoUndef)

eugenis wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > > SanitizeMemoryParamRetval is provided?
> > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > > SanitizeMemoryParamRetval is provided?
> > 
> > To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, unless 
> > @eugenis or someone else thinks EnableNoundefAttrs reset is better.
> I don't think this is right at all. An argument being noundef has nothing to 
> do at all with memory sanitization. It affects optimization, too. 
> SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does 
> with noundef attributes.
Is the problem the form of the new code?
  - instead of introducing a single new flag to flip 4 others, should there be 
1 flag to pick up the CodeGen changes, and another for the Sanitizer?  (Is 2 
flags better than 4?)
  - another option is have the changes propagate in the other direction: use 
the flag (-fsanitize-memory-param-retval), to passes along '-mllvm 
-enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).

OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" flag?
  - then leave existing code, just don't do anything fancy to change 
EnableNoundefAttrs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

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

[PATCH] D116599: Simplify AttrBuilder storage for target dependent attributes

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D116599#3221724 , @nikic wrote:

> LG from my side, but I'd like a second opinion for the "require LLVMContext 
> in AttrBuilder" part of the change, as that's the main API impact.

I think every non-toy frontend for LLVM probably uses the AttrBuilder API to 
construct attributes, so it's a pretty broad impact. However, I don't see a 
good way around it, and I think the improvements to string attribute handling 
are worth taking the API break.

I will say that the Google was recently affected by the opaque pointer 
IRBuilder API removal (rG89eb85ac6ea 
), and the 
ability to pre-migrate code to the new API was very valuable. At least in our 
usage, where we progressively pick new LLVM versions, it would be helpful to 
land a no-op AttrBuilder constructor that takes and ignores an LLVMContext. I 
don't know how common that usage model is for others.


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

https://reviews.llvm.org/D116599

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

[PATCH] D115183: [clang][HeaderSearch] Support framework includes in suggestPath...

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:782
 if (IsIncludeeInFramework) {
-  NewInclude += ToFramework.str().drop_back(10); // drop .framework
-  NewInclude += "/";
+  NewInclude += ToIncludeSpelling;
+  NewInclude += ">";

sammccall wrote:
> Nice!
> 
> No good deed goes un-punished: it should be possible to add a diagnostic test?
> There are some under clang/test/Preprocessor/*framework*
I couldn't seem to get it to generate a warning for a subframework, for both 
there and in double-quotes.m under Modules, so instead I added a simple test to 
headersearch for the sub framework case and made sure the fix it in 
double-quotes.m was OK for the existing cases



Comment at: clang/lib/Lex/HeaderSearch.cpp:1899
+  // symlink like `iPhoneSimulator14.5.sdk` while the file is instead
+  // located in `iPhoneSimulator.sdk` (the real folder).
+  if (NI->endswith(".sdk") && DI->endswith(".sdk")) {

sammccall wrote:
> Nice comment, very helpful!
> 
> Just wanted to check you're sure the versioned one is the symlink and the 
> unversioned one is the real folder, rather than the other way. (Plausible, 
> just surprising to me)
Yep,

$ readlink 
/Applications/Xcode_13.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.0.sdk

   
MacOSX.sdk




Comment at: clang/lib/Lex/HeaderSearch.cpp:1962
+} else {
+  // Prefer using `` for non-`UIKit` main files.
+  //

sammccall wrote:
> This heuristic looks iffy to me, it's not just trying to help the caller 
> insert the right header, but tell them they want a different one for style 
> reasons.
> 
> The first example framework in 
> https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html
>  doesn't include an umbrella header of this form.
> 
> Moreover, non-framework libraries have umbrella headers too, this seems like 
> an orthogonal concern best caught in some other way.
See 
https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html,
 it's common to use this umbrella header (especially for Apple SDKs) and some 
frameworks require it (as opposed to including the header directly), thus I 
think this default makes sense even if it isn't 100%.

Any other ideas on how we can help here? I guess if we had FS access we could 
see if the umbrella header exists...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115183

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

[PATCH] D115183: [clang][HeaderSearch] Support framework includes in suggestPath...

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 397733.
dgoldman marked 4 inline comments as done.
dgoldman added a comment.
Herald added a subscriber: ormris.

Fixes for review - update test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115183

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/double-quotes.m
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -47,6 +47,15 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addSystemFrameworkSearchDir(llvm::StringRef Dir) {
+VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+ /*Group=*/None, llvm::sys::fs::file_type::directory_file);
+auto DE = FileMgr.getOptionalDirectoryRef(Dir);
+assert(DE);
+auto DL = DirectoryLookup(*DE, SrcMgr::C_System, /*isFramework=*/true);
+Search.AddSystemSearchPath(DL);
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
 std::unique_ptr Buf,
 bool isAngled = false) {
@@ -155,6 +164,44 @@
 "y/z/t.h");
 }
 
+TEST_F(HeaderSearchTest, SdkFramework) {
+  addSystemFrameworkSearchDir(
+  "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/");
+  bool IsSystem = false;
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+"/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
+"Frameworks/AppKit.framework/Headers/NSView.h",
+/*WorkingDir=*/"",
+/*MainFile=*/"", ),
+"AppKit/AppKit.h");
+  EXPECT_TRUE(IsSystem);
+}
+
+TEST_F(HeaderSearchTest, SdkIntraFramework) {
+  addSystemFrameworkSearchDir(
+  "/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/Frameworks/");
+  bool IsSystem = false;
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+"/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
+"Frameworks/AppKit.framework/Headers/NSView.h",
+/*WorkingDir=*/"",
+"/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"
+"Frameworks/AppKit.framework/Headers/NSViewController.h",
+),
+"AppKit/NSView.h");
+  EXPECT_TRUE(IsSystem);
+}
+
+TEST_F(HeaderSearchTest, NestedFramework) {
+  addSystemFrameworkSearchDir("/Platforms/MacOSX/Frameworks");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics(
+"/Platforms/MacOSX/Frameworks/AppKit.framework/Frameworks/"
+"Sub.framework/Headers/Sub.h",
+/*WorkingDir=*/"",
+/*MainFile=*/""),
+"Sub/Sub.h");
+}
+
 // Helper struct with null terminator character to make MemoryBuffer happy.
 template 
 struct NullTerminatedFile : public FileTy {
Index: clang/test/Modules/double-quotes.m
===
--- clang/test/Modules/double-quotes.m
+++ clang/test/Modules/double-quotes.m
@@ -24,8 +24,11 @@
 // because they only show up under the module A building context.
 // RUN: FileCheck --input-file=%t/stderr %s
 // CHECK: double-quoted include "A0.h" in framework header, expected angle-bracketed instead
+// CHECK: 
 // CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+// CHECK: 
 // CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+// CHECK: 
 
 #import "A.h"
 #import 
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -721,7 +721,8 @@
 }
 
 static bool isFrameworkStylePath(StringRef Path, bool ,
- SmallVectorImpl ) {
+ SmallVectorImpl ,
+ SmallVectorImpl ) {
   using namespace llvm::sys;
   path::const_iterator I = path::begin(Path);
   path::const_iterator E = path::end(Path);
@@ -737,15 +738,22 @@
   // and some other variations among these lines.
   int FoundComp = 0;
   while (I != E) {
-if (*I == "Headers")
+if (*I == "Headers") {
   ++FoundComp;
-if (I->endswith(".framework")) {
-  FrameworkName.append(I->begin(), I->end());
-  ++FoundComp;
-}
-if (*I == "PrivateHeaders") {
+} else if (*I == "PrivateHeaders") {
   ++FoundComp;
   IsPrivateHeader = true;
+} else if (I->endswith(".framework")) {
+  StringRef Name = I->drop_back(10); // Drop .framework
+  // Need to reset the strings and counter to support nested frameworks.
+  FrameworkName.clear();
+  FrameworkName.append(Name.begin(), Name.end());
+  IncludeSpelling.clear();
+  

[PATCH] D116059: [Clang][CFG] check children statements of asm goto

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 397728.
nickdesaulniers added a comment.

- don't bother checking if a Val is Initialized


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116059

Files:
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/asm-goto.cpp


Index: clang/test/Analysis/asm-goto.cpp
===
--- clang/test/Analysis/asm-goto.cpp
+++ clang/test/Analysis/asm-goto.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1  -triple i386-pc-linux-gnu 
-analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1  -triple x86_64-pc-linux-gnu 
-analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple i386-pc-linux-gnu 
-analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu 
-analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
 
 int foo(int cond)
 {
@@ -17,11 +17,12 @@
 // CHECK-NEXT: Succs (1): B0
 
 // CHECK-LABEL: label_true
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B3.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B3 B4
 // CHECK-NEXT: Succs (3): B2 B3 B1
 
-
 int bar(int cond)
 {
   asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::L1, L2);
@@ -32,7 +33,9 @@
 }
 
 // CHECK: [B4]
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (1): B5
 // CHECK-NEXT: Succs (3): B3 B2 B1
 
@@ -48,6 +51,20 @@
 }
 
 // CHECK-LABEL: A1
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: n
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B5 B4
 // CHECK-NEXT: Succs (5): B3 B4 B2 B1 B5
+
+void baz(void)
+{
+  asm goto("" :: "r"(1 ? 2 : 0 << -1) :: error);
+error:;
+}
+
+// CHECK: [B2]
+// CHECK-NEXT: 1: [B5.2] ? [B3.1] : [B4.4]
+// CHECK-NEXT: T: asm goto ("" :  : "r" ([B2.1]) :  : error);
+// CHECK-NEXT: Preds (2): B3 B4
+// CHECK-NEXT: Succs (1): B1
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -819,12 +819,11 @@
 while (const auto *UO = dyn_cast(Ex))
   Ex = stripCasts(C, UO->getSubExpr());
 
+// Mark the variable as potentially uninitialized for those cases where
+// it's used on an indirect path, where it's not guaranteed to be
+// defined.
 if (const VarDecl *VD = findVar(Ex).getDecl())
-  if (vals[VD] != Initialized)
-// If the variable isn't initialized by the time we get here, then we
-// mark it as potentially uninitialized for those cases where it's used
-// on an indirect path, where it's not guaranteed to be defined.
-vals[VD] = MayUninitialized;
+  vals[VD] = MayUninitialized;
   }
 }
 
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -3354,7 +3354,7 @@
   // Save "Succ" in BackpatchBlocks. In the backpatch processing, "Succ" is
   // used to avoid adding "Succ" again.
   BackpatchBlocks.push_back(JumpSource(Succ, ScopePos));
-  return Block;
+  return VisitChildren(G);
 }
 
 CFGBlock *CFGBuilder::VisitForStmt(ForStmt *F) {


Index: clang/test/Analysis/asm-goto.cpp
===
--- clang/test/Analysis/asm-goto.cpp
+++ clang/test/Analysis/asm-goto.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1  -triple i386-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1  -triple x86_64-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple i386-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
 
 int foo(int cond)
 {
@@ -17,11 +17,12 @@
 // CHECK-NEXT: Succs (1): B0
 
 // CHECK-LABEL: label_true
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B3.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B3 B4
 // CHECK-NEXT: Succs (3): B2 B3 B1
 
-
 int bar(int cond)
 {
   asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::L1, L2);
@@ -32,7 +33,9 @@
 }
 
 // CHECK: [B4]
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (1): B5
 // CHECK-NEXT: Succs (3): B3 B2 B1
 
@@ -48,6 +51,20 @@
 }
 
 // CHECK-LABEL: A1
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: n
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B5 B4
 // CHECK-NEXT: Succs (5): B3 B4 B2 B1 B5
+
+void baz(void)
+{
+  asm goto("" :: "r"(1 ? 2 : 0 << -1) :: error);
+error:;
+}
+
+// CHECK: [B2]
+// CHECK-NEXT: 1: [B5.2] ? [B3.1] : 

[PATCH] D116550: [clang-tidy] Recognize transformer checks as providing fixits

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd7b6574c3bf6: [clang-tidy] Recognize transformer checks as 
providing fixits (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116550

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
  clang-tools-extra/docs/clang-tidy/checks/list.rst

Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -116,13 +116,12 @@
`cert-dcl50-cpp `_,
`cert-dcl58-cpp `_,
`cert-env33-c `_,
+   `cert-err33-c `_,
`cert-err34-c `_,
`cert-err52-cpp `_,
`cert-err58-cpp `_,
`cert-err60-cpp `_,
-   `cert-exp42-c `_,
`cert-flp30-c `_,
-   `cert-flp37-c `_,
`cert-mem57-cpp `_,
`cert-msc50-cpp `_,
`cert-msc51-cpp `_,
@@ -213,7 +212,7 @@
`llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
-   `misc-misleading-identifier `_,
+   `misc-misleading-identifier `_,
`misc-misplaced-const `_,
`misc-new-delete-overloads `_,
`misc-no-recursion `_,
@@ -260,8 +259,8 @@
`modernize-use-using `_, "Yes"
`mpi-buffer-deref `_, "Yes"
`mpi-type-mismatch `_, "Yes"
-   `objc-avoid-nserror-init `_,
`objc-assert-equals `_, "Yes"
+   `objc-avoid-nserror-init `_,
`objc-dealloc-in-category `_,
`objc-forbidden-subclassing `_,
`objc-missing-hash `_,
@@ -283,16 +282,16 @@
`performance-noexcept-move-constructor `_, "Yes"
`performance-trivially-destructible `_, "Yes"
`performance-type-promotion-in-math-fn `_, "Yes"
-   `performance-unnecessary-copy-initialization `_,
+   `performance-unnecessary-copy-initialization `_, "Yes"
`performance-unnecessary-value-param `_, "Yes"
`portability-restrict-system-includes `_, "Yes"
`portability-simd-intrinsics `_,
-   `readability-avoid-const-params-in-decls `_,
+   `readability-avoid-const-params-in-decls `_, "Yes"
`readability-braces-around-statements `_, "Yes"
`readability-const-return-type `_, "Yes"
`readability-container-data-pointer `_, "Yes"
`readability-container-size-empty `_, "Yes"
-   `readability-convert-member-functions-to-static `_,
+   `readability-convert-member-functions-to-static `_, "Yes"
`readability-delete-null-pointer `_, "Yes"
`readability-else-after-return `_, "Yes"
`readability-function-cognitive-complexity `_,
@@ -338,13 +337,14 @@
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
`cert-dcl16-c `_, `readability-uppercase-literal-suffix `_, "Yes"
`cert-dcl37-c `_, `bugprone-reserved-identifier `_, "Yes"
-   `cert-err33-c `_, `bugprone-unused-return-value `_,
`cert-dcl51-cpp `_, `bugprone-reserved-identifier `_, "Yes"
`cert-dcl54-cpp `_, `misc-new-delete-overloads `_,
`cert-dcl59-cpp `_, `google-build-namespaces `_,
`cert-err09-cpp `_, `misc-throw-by-value-catch-by-reference `_,
`cert-err61-cpp `_, `misc-throw-by-value-catch-by-reference `_,
+   `cert-exp42-c `_, `bugprone-suspicious-memory-comparison `_,
`cert-fio38-c `_, `misc-non-copyable-objects `_,
+   `cert-flp37-c `_, `bugprone-suspicious-memory-comparison `_,
`cert-msc30-c `_, `cert-msc50-cpp `_,
`cert-msc32-c `_, `cert-msc51-cpp `_,
`cert-oop11-cpp `_, `performance-move-constructor-init `_,
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -54,7 +54,7 @@
 StringRef Name, ClangTidyContext *Context);
 
   /// Convenience overload of the constructor when the rule doesn't have any
-  /// dependies.
+  /// dependencies.
   TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
 ClangTidyContext *Context);
 
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -324,16 +324,20 @@
 dirname, _, check_name = check_name.partition("-")
 
 checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
-   get_camel_name(check_name) + '.cpp')
-
+   get_camel_check_name(check_name) + '.cpp')
 if not os.path.isfile(checker_code):
-  return ""
+  # Some older checks don't end with 'Check.cpp'
+  checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
+ 

[clang-tools-extra] d7b6574 - [clang-tidy] Recognize transformer checks as providing fixits

[via cfe-commits]

Author: Richard
Date: 2022-01-05T16:13:52-07:00
New Revision: d7b6574c3bf671d70acd751a8c85d3a062dcc7c6

URL: 
https://github.com/llvm/llvm-project/commit/d7b6574c3bf671d70acd751a8c85d3a062dcc7c6
DIFF: 
https://github.com/llvm/llvm-project/commit/d7b6574c3bf671d70acd751a8c85d3a062dcc7c6.diff

LOG: [clang-tidy] Recognize transformer checks as providing fixits

- Recognize older checks that might not end with Check.cpp
- Update list of checks based on improvements to add_new_check
- Fix spelling error in TransformerClangTidyCheck.h

Fixes #52962

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

Added: 


Modified: 
clang-tools-extra/clang-tidy/add_new_check.py
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
clang-tools-extra/docs/clang-tidy/checks/list.rst

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/add_new_check.py 
b/clang-tools-extra/clang-tidy/add_new_check.py
index a3554b0959759..1e26b07121c61 100755
--- a/clang-tools-extra/clang-tidy/add_new_check.py
+++ b/clang-tools-extra/clang-tidy/add_new_check.py
@@ -324,16 +324,20 @@ def has_auto_fix(check_name):
 dirname, _, check_name = check_name.partition("-")
 
 checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
-   get_camel_name(check_name) + '.cpp')
-
+   get_camel_check_name(check_name) + 
'.cpp')
 if not os.path.isfile(checker_code):
-  return ""
+  # Some older checks don't end with 'Check.cpp'
+  checker_code = get_actual_filename(os.path.join(clang_tidy_path, 
dirname),
+ get_camel_name(check_name) + '.cpp')
+  if not os.path.isfile(checker_code):
+return ''
 
 with io.open(checker_code, encoding='utf8') as f:
   code = f.read()
-  if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
-# Some simple heuristics to figure out if a checker has an autofix or 
not.
-return ' "Yes"'
+  for needle in ['FixItHint', 'ReplacementText', 'fixit', 
'TransformerClangTidyCheck']:
+if needle in code:
+  # Some simple heuristics to figure out if a checker has an autofix 
or not.
+  return ' "Yes"'
 return ""
 
   def process_doc(doc_file):
@@ -416,7 +420,11 @@ def write_docs(module_path, module, check_name):
 
 def get_camel_name(check_name):
   return ''.join(map(lambda elem: elem.capitalize(),
- check_name.split('-'))) + 'Check'
+ check_name.split('-')))
+
+
+def get_camel_check_name(check_name):
+  return get_camel_name(check_name) + 'Check'
 
 
 def main():
@@ -458,7 +466,7 @@ def main():
 
   module = args.module
   check_name = args.check
-  check_name_camel = get_camel_name(check_name)
+  check_name_camel = get_camel_check_name(check_name)
   if check_name.startswith(module):
 print('Check name "%s" must not start with the module "%s". Exiting.' % (
 check_name, module))

diff  --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h 
b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
index 9736e64e7c311..d26737935b1aa 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -54,7 +54,7 @@ class TransformerClangTidyCheck : public ClangTidyCheck {
 StringRef Name, ClangTidyContext *Context);
 
   /// Convenience overload of the constructor when the rule doesn't have any
-  /// dependies.
+  /// dependencies.
   TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
 ClangTidyContext *Context);
 

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst 
b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 1e6936f9cbdf9..8d0a568cff881 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -116,13 +116,12 @@ Clang-Tidy Checks
`cert-dcl50-cpp `_,
`cert-dcl58-cpp `_,
`cert-env33-c `_,
+   `cert-err33-c `_,
`cert-err34-c `_,
`cert-err52-cpp `_,
`cert-err58-cpp `_,
`cert-err60-cpp `_,
-   `cert-exp42-c `_,
`cert-flp30-c `_,
-   `cert-flp37-c `_,
`cert-mem57-cpp `_,
`cert-msc50-cpp `_,
`cert-msc51-cpp `_,
@@ -213,7 +212,7 @@ Clang-Tidy Checks
`llvmlibc-implementation-in-namespace 
`_,
`llvmlibc-restrict-system-libc-headers 
`_, "Yes"
`misc-definitions-in-headers `_, "Yes"
-   `misc-misleading-identifier `_,
+   `misc-misleading-identifier `_,
`misc-misplaced-const `_,
`misc-new-delete-overloads `_,
`misc-no-recursion `_,
@@ -260,8 +259,8 @@ Clang-Tidy Checks
`modernize-use-using `_, "Yes"
`mpi-buffer-deref `_, "Yes"
`mpi-type-mismatch `_, "Yes"
-   `objc-avoid-nserror-init `_,

[PATCH] D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 397719.
mstorsjo marked 2 inline comments as done.
mstorsjo added a comment.

Add testcases for using the new option with clang-cl, use `--target=` 
everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116503

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/diagnostics.c

Index: clang/test/Driver/diagnostics.c
===
--- clang/test/Driver/diagnostics.c
+++ clang/test/Driver/diagnostics.c
@@ -1,9 +1,53 @@
 // Parse diagnostic arguments in the driver
-// PR12181
 
-// RUN: not %clang -target x86_64-apple-darwin10 \
-// RUN:   -fsyntax-only -fzyzzybalubah \
-// RUN:   -Werror=unused-command-line-argument %s
+// Exactly which arguments are warned about and which aren't differ based
+// on what target is selected. -stdlib= and -fuse-ld= emit diagnostics when
+// compiling C code, for e.g. *-linux-gnu. Linker inputs, like -lfoo, emit
+// diagnostics when only compiling for all targets.
 
-// RUN: not %clang -target x86_64-apple-darwin10 \
-// RUN:   -fsyntax-only -fzyzzybalubah -Werror %s
+// This is normally a non-fatal warning:
+// RUN: %clang --target=x86_64-apple-darwin10 \
+// RUN:   -fsyntax-only -lfoo %s 2>&1 | FileCheck %s
+
+// Either with a specific -Werror=unused.. or a blanket -Werror, this
+// causes the command to fail.
+// RUN: not %clang --target=x86_64-apple-darwin10 \
+// RUN:   -fsyntax-only -lfoo \
+// RUN:   -Werror=unused-command-line-argument %s 2>&1 | FileCheck %s
+
+// RUN: not %clang --target=x86_64-apple-darwin10 \
+// RUN:   -fsyntax-only -lfoo -Werror %s 2>&1 | FileCheck %s
+
+// With a specific -Wno-..., no diagnostic should be printed.
+// RUN: %clang --target=x86_64-apple-darwin10 \
+// RUN:   -fsyntax-only -lfoo -Werror \
+// RUN:   -Wno-unused-command-line-argument %s 2>&1 | count 0
+
+// With -Qunused-arguments, no diagnostic should be printed.
+// RUN: %clang --target=x86_64-apple-darwin10 \
+// RUN:   -fsyntax-only -lfoo -Werror \
+// RUN:   -Qunused-arguments %s 2>&1 | count 0
+
+// With the argument enclosed in --{start,end}-no-unused-arguments,
+// there's no diagnostic.
+// RUN: %clang --target=x86_64-apple-darwin10 -fsyntax-only \
+// RUN:   --start-no-unused-arguments -lfoo --end-no-unused-arguments \
+// RUN:   -Werror %s 2>&1 | count 0
+
+// With --{start,end}-no-unused-argument around a different argument, it
+// still warns about the unused argument.
+// RUN: not %clang --target=x86_64-apple-darwin10 \
+// RUN:   --start-no-unused-arguments -fsyntax-only --end-no-unused-arguments \
+// RUN:   -lfoo -Werror %s 2>&1 | FileCheck %s
+
+// Test clang-cl warning about unused linker options.
+// RUN: not %clang_cl -fsyntax-only /WX %s \
+// RUN:   -link 2>&1 | FileCheck %s --check-prefix=CL-WARNING
+
+// Test clang-cl ignoring the warning with --start-no-unused-arguments.
+// RUN: %clang_cl -fsyntax-only /WX %s \
+// RUN:   --start-no-unused-arguments -link --end-no-unused-arguments 2>&1 | count 0
+
+// CHECK: -lfoo: 'linker' input unused
+
+// CL-WARNING: argument unused during compilation: '-link'
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -367,7 +367,20 @@
   bool HasNostdlib = Args.hasArg(options::OPT_nostdlib);
   bool HasNostdlibxx = Args.hasArg(options::OPT_nostdlibxx);
   bool HasNodefaultlib = Args.hasArg(options::OPT_nodefaultlibs);
+  bool IgnoreUnused = false;
   for (Arg *A : Args) {
+if (IgnoreUnused)
+  A->claim();
+
+if (A->getOption().matches(options::OPT_start_no_unused_arguments)) {
+  IgnoreUnused = true;
+  continue;
+}
+if (A->getOption().matches(options::OPT_end_no_unused_arguments)) {
+  IgnoreUnused = false;
+  continue;
+}
+
 // Unfortunately, we have to parse some forwarding options (-Xassembler,
 // -Xlinker, -Xpreprocessor) because we either integrate their functionality
 // (assembler and preprocessor), or bypass a previous driver ('collect2').
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1076,6 +1076,8 @@
 def emit_merged_ifs : Flag<["-"], "emit-merged-ifs">,
   Flags<[CC1Option]>, Group,
   HelpText<"Generate Interface Stub Files, emit merged text not binary.">;
+def end_no_unused_arguments : Flag<["--"], "end-no-unused-arguments">, Flags<[CoreOption]>,
+  HelpText<"Start emitting warnings for unused driver arguments">;
 def interface_stub_version_EQ : JoinedOrSeparate<["-"], "interface-stub-version=">, Flags<[CC1Option]>;
 def exported__symbols__list : Separate<["-"], "exported_symbols_list">;
 def e : JoinedOrSeparate<["-"], "e">, 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

delcypher wrote:
> delcypher wrote:
> > @NoQ Any ideas about this?  It seems kind of weird that when merging 
> > `not_a_prototype3` prototype with the K style definition of 
> > `not_a_prototype3` that the resulting FunctionDecl we see at the call site 
> > in `call_to_function_without_prototype3` is marked as not having a 
> > prototype.
> > 
> > If I flip the order (see `not_a_prototype6`) then the merged declaration is 
> > marked as having a prototype.
> > 
> > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this just 
> > a peculiarity of K style function definitions.
> I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> 
> ```lang=c++
>// C: Function types need to be compatible, not identical. This handles
>   // duplicate function decls like "void f(int); void f(enum X);" properly.
>   if (!getLangOpts().CPlusPlus &&
>   Context.typesAreCompatible(OldQType, NewQType)) {
> const FunctionType *OldFuncType = OldQType->getAs();
> const FunctionType *NewFuncType = NewQType->getAs();
> const FunctionProtoType *OldProto = nullptr;
> if (MergeTypeWithOld && isa(NewFuncType) &&
> (OldProto = dyn_cast(OldFuncType))) {
>   // The old declaration provided a function prototype, but the
>   // new declaration does not. Merge in the prototype.
> ```
> 
> ` isa(NewFuncType)` is false in this particular case, 
> however `New` doesn't have a prototype (i.e. `New->hasPrototype()` is false). 
> One fix might be to replace `isa(NewFuncType)` with 
> 
> ```
> (isa(NewFuncType) || !New->hasPrototype())
> ```
> 
> However, I don't really know this code well enough to know if that's the 
> right fix.
Okay. I think the above would actually be the wrong location for a fix because 
in this case we don't need to go down the path that synthesizes the parameters 
because we already know them for both `old` and `new` in this situation.

Instead I think the change would have to be in 
`Sema::MergeCompatibleFunctionDecls` to do something like.

```lang=c++
  // If New is a K function definition it will be marked
  // as not having a prototype. If `Old` has a prototype
  // then to "merge" we should mark the K function as having a prototype.
  if (!getLangOpts().CPlusPlus && Old->hasPrototype() && !New->hasPrototype())
New->setHasInheritedPrototype(); 
```

What I'm not sure about is if this is semantically the right thing to do. 
Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5530
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;
 def warn_missing_variable_declarations : Warning<

fcloutier wrote:
> sp: I think this should be called "strict-call-without-prototype" (singular 
> call) as most warning flag names seem to prefer singulars.
@fcloutier 
I'm open to renaming the warning. I originally made the warning plural because 
there were warnings near by that used it. E.g.

* `missing-variable-declarations`
* `strict-prototypes`
* `missing-prototypes`

If no one else chimes in I'll make it singular.

Also, do you think we should drop the "strict" prefix? I.e. 
`call-without-prototype` instead of `strict-call-without-prototype`. The reason 
I'm thinking that using `strict` might be a bad idea is:

1. The warning isn't actually strict. A strict version of the warning would 
warn on **all calls to functions without prototypes**. We've decided to only 
warn on calls with arguments to avoid noisy warnings that result from people 
frequently forgetting to explicitly mark functions as taking no arguments.

2. Calling this warning  `strict-call-without-prototype` precludes from adding 
a stricter version of warning in the future (i.e. we'd be force to name it 
something like `strict-strict-call-without-prototype` which is super confusing).

3. The intention in a future patch is to enable the warning by default. It 
seems odd to have a "strict" warning on by default given that there is 
precedence for doing the opposite (e.g. `-Wstrict-prototypes` is off by 
default).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+if ((CodeGenOpts.EnableNoundefAttrs ||
+ CodeGenOpts.SanitizeMemoryParamRetval) &&
+ArgNoUndef)

vitalybuka wrote:
> vitalybuka wrote:
> > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > SanitizeMemoryParamRetval is provided?
> > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > SanitizeMemoryParamRetval is provided?
> 
> To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, unless 
> @eugenis or someone else thinks EnableNoundefAttrs reset is better.
I don't think this is right at all. An argument being noundef has nothing to do 
at all with memory sanitization. It affects optimization, too. 
SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does with 
noundef attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

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

[PATCH] D116337: [clang] set __NO_MATH_ERRNO__ if -fno-math-errno

[Alex Xu (Hello71) via Phabricator via cfe-commits]

alxu updated this revision to Diff 397712.
alxu added a comment.

Try -p1 compatible diff.


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

https://reviews.llvm.org/D116337

Files:
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/predefined-macros.c


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -59,8 +59,17 @@
 // RUN: %clang_cc1 %s -E -dM -ffast-math -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
 //
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
+//
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s 
--check-prefix=CHECK-FINITE-MATH-ONLY
 // CHECK-FINITE-MATH-ONLY: #define __FINITE_MATH_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1039,6 +1039,9 @@

   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());

+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -59,8 +59,17 @@
 // RUN: %clang_cc1 %s -E -dM -ffast-math -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
 //
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
+//
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FINITE-MATH-ONLY
 // CHECK-FINITE-MATH-ONLY: #define __FINITE_MATH_ONLY__ 1
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -1039,6 +1039,9 @@

   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());

+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in 
function declaration

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > Can you also add a test for:
> > > ```
> > > void func(return_t(void));
> > > ```
> > `:-)`
> > 
> > What are you suggesting the result should be?  Honestly, looking at that, 
> > I'm not sure myself `:)`
> > 
> > IMO, if I saw this in a code review, I would flag it because you're using a 
> > macro called "return type" to specify the type of an argument.
> LoL, yeah, the name `return_t` would certainly be novel to use in a parameter 
> list, but what I was hoping to test is whether we try to fix the use of the 
> macro within the parameter list or not. I *think* it probably makes sense to 
> issue the diagnostic, but I don't think it makes sense to try to fix it 
> because the macro could be defined differently for different configurations. 
> But the diagnostic is silenced as well as the fix-it, I wouldn't lose a whole 
> lot of sleep over it.
Well it could conceivably be used to declare a function pointer argument like 
this:

`void func(return_t(void) (*fp)(void));`

In that case, my expectation is that the check would fix the void arg, but not 
the arg to the macro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116425

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

[PATCH] D116337: [clang] set __NO_MATH_ERRNO__ if -fno-math-errno

[Alex Xu (Hello71) via Phabricator via cfe-commits]

alxu added inline comments.



Comment at: clang/lib/Frontend/InitPreprocessor.cpp:1022
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)

aaron.ballman wrote:
> Does GCC gate on `-ffast-math`? My testing suggests that `-fno-math-errno` 
> alone is what sets this macro in GCC.
I thought that MathErrno was not set if -ffast-math is specified, but that is 
apparently not true. It looks like lib/Driver/ToolChains/Clang.cpp should set 
FiniteMathOnly for -ffast-math, and the check below should be adjusted; 
otherwise it is wrong for -ffast-math -fno-finite-math-only.


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

https://reviews.llvm.org/D116337

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

[PATCH] D116337: [clang] set __NO_MATH_ERRNO__ if -fno-math-errno

[Alex Xu (Hello71) via Phabricator via cfe-commits]

alxu updated this revision to Diff 397708.
alxu added a comment.

In D116337#3217955 , @aaron.ballman 
wrote:

> Thanks for this! I'm adding some more reviewers to the list to help get this 
> reviewed.
>
> The patch doesn't seem to apply cleanly, so precommit CI isn't running on it. 
> Can you rectify that?

GNU patch applies the patch with offset 20 and no fuzz on llvm-project main. 
However, I have included a rebase from llvm 13 to llvm main along with the 
tests in the new patch.

> Also, the change needs test coverage of some kind.

Added.


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

https://reviews.llvm.org/D116337

Files:
  ../clang/lib/Frontend/InitPreprocessor.cpp 
  ../clang/test/Preprocessor/predefined-macros.c


Index: ../clang/test/Preprocessor/predefined-macros.c 
===
--- ../clang/test/Preprocessor/predefined-macros.c 
+++ ../clang/test/Preprocessor/predefined-macros.c 
@@ -59,8 +59,17 @@
 // RUN: %clang_cc1 %s -E -dM -ffast-math -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
 //
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
+//
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s 
--check-prefix=CHECK-FINITE-MATH-ONLY
 // CHECK-FINITE-MATH-ONLY: #define __FINITE_MATH_ONLY__ 1
Index: ../clang/lib/Frontend/InitPreprocessor.cpp 
===
--- ../clang/lib/Frontend/InitPreprocessor.cpp 
+++ ../clang/lib/Frontend/InitPreprocessor.cpp 
@@ -1039,6 +1039,9 @@

   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());

+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else


Index: ../clang/test/Preprocessor/predefined-macros.c 
===
--- ../clang/test/Preprocessor/predefined-macros.c 
+++ ../clang/test/Preprocessor/predefined-macros.c 
@@ -59,8 +59,17 @@
 // RUN: %clang_cc1 %s -E -dM -ffast-math -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FAST-MATH
 // CHECK-FAST-MATH: #define __FAST_MATH__ 1
+// CHECK-FAST-MATH: #define __NO_MATH_ERRNO__ 1
 // CHECK-FAST-MATH: #define __FINITE_MATH_ONLY__ 1
 //
+// RUN: %clang_cc1 %s -E -dM -fmath-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-MATH-ERRNO
+// CHECK-MATH-ERRNO-NOT: __NO_MATH_ERRNO__
+//
+// RUN: %clang %s -E -dM -fno-math-errno -o - \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-NO-MATH-ERRNO
+// CHECK-NO-MATH-ERRNO: #define __NO_MATH_ERRNO__ 1
+//
 // RUN: %clang_cc1 %s -E -dM -ffinite-math-only -o - \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-FINITE-MATH-ONLY
 // CHECK-FINITE-MATH-ONLY: #define __FINITE_MATH_ONLY__ 1
Index: ../clang/lib/Frontend/InitPreprocessor.cpp 
===
--- ../clang/lib/Frontend/InitPreprocessor.cpp 
+++ ../clang/lib/Frontend/InitPreprocessor.cpp 
@@ -1039,6 +1039,9 @@

   Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());

+  if (LangOpts.MathErrno)
+Builder.defineMacro("__NO_MATH_ERRNO__");
+
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116701: Respect -fsanitize-memory-param-retval flag.

[Kevin Athey via Phabricator via cfe-commits]

kda created this revision.
kda added a reviewer: vitalybuka.
kda requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Implementation of flag introduced in https://reviews.llvm.org/D116633


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116701

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/attr-noundef.cpp
  clang/test/CodeGen/indirect-noundef.cpp


Index: clang/test/CodeGen/indirect-noundef.cpp
===
--- clang/test/CodeGen/indirect-noundef.cpp
+++ clang/test/CodeGen/indirect-noundef.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm 
-enable-noundef-analysis -o - %s | FileCheck %s
+// RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm 
-fsanitize-memory-param-retval -o - %s | FileCheck %s
 
 union u1 {
   int val;
Index: clang/test/CodeGen/attr-noundef.cpp
===
--- clang/test/CodeGen/attr-noundef.cpp
+++ clang/test/CodeGen/attr-noundef.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm 
-enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-INTEL
 // RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm 
-enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-AARCH
+// RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm 
-fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-INTEL
+// RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm 
-fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-AARCH
 
 // Passing structs by value
 // TODO: No structs may currently be marked noundef
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2243,7 +2243,9 @@
  getLangOpts().Sanitize.has(SanitizerKind::Return);
 
   // Determine if the return type could be partially undef
-  if (CodeGenOpts.EnableNoundefAttrs && HasStrictReturn) {
+  if ((CodeGenOpts.EnableNoundefAttrs ||
+   CodeGenOpts.SanitizeMemoryParamRetval) &&
+  HasStrictReturn) {
 if (!RetTy->isVoidType() && RetAI.getKind() != ABIArgInfo::Indirect &&
 DetermineNoUndef(RetTy, getTypes(), DL, RetAI))
   RetAttrs.addAttribute(llvm::Attribute::NoUndef);
@@ -2378,7 +2380,9 @@
 
 // Decide whether the argument we're handling could be partially undef
 bool ArgNoUndef = DetermineNoUndef(ParamType, getTypes(), DL, AI);
-if (CodeGenOpts.EnableNoundefAttrs && ArgNoUndef)
+if ((CodeGenOpts.EnableNoundefAttrs ||
+ CodeGenOpts.SanitizeMemoryParamRetval) &&
+ArgNoUndef)
   Attrs.addAttribute(llvm::Attribute::NoUndef);
 
 // 'restrict' -> 'noalias' is done in EmitFunctionProlog when we


Index: clang/test/CodeGen/indirect-noundef.cpp
===
--- clang/test/CodeGen/indirect-noundef.cpp
+++ clang/test/CodeGen/indirect-noundef.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -enable-noundef-analysis -o - %s | FileCheck %s
+// RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -fsanitize-memory-param-retval -o - %s | FileCheck %s
 
 union u1 {
   int val;
Index: clang/test/CodeGen/attr-noundef.cpp
===
--- clang/test/CodeGen/attr-noundef.cpp
+++ clang/test/CodeGen/attr-noundef.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
 // RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-AARCH
+// RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm -fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
+// RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm -fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-AARCH
 
 // Passing structs by value
 // TODO: No structs may currently be marked noundef
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2243,7 +2243,9 @@
  getLangOpts().Sanitize.has(SanitizerKind::Return);
 
   // Determine if the return type could be partially undef
-  if (CodeGenOpts.EnableNoundefAttrs && HasStrictReturn) {
+  if ((CodeGenOpts.EnableNoundefAttrs ||
+   

[PATCH] D116699: [clangd] Polish clangd/inlayHints and expose them by default.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: kadircet, nridge.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This means it's a "real feature" in clangd 14, albeit one that requires special
client support.

- remove "preview" from the flag description
- expose the `clangdInlayHints` capability by default
- provide `position` as well as `range`
- support `InlayHintsParams.range` to restrict the range retrieved
- inlay hint list is in document order (sorted by position)

Still to come: control feature via config rather than flag.

Fixes https://github.com/clangd/clangd/issues/313
Protocol doc is in https://github.com/llvm/clangd-www/pull/56/files


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116699

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/InlayHints.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/inlayHints.test
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -23,20 +23,23 @@
 
 namespace {
 
-using ::testing::UnorderedElementsAre;
+using ::testing::ElementsAre;
 
 std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
   std::vector Result;
-  for (auto  : inlayHints(AST)) {
+  for (auto  : inlayHints(AST, /*RestrictRange=*/llvm::None)) {
 if (Hint.kind == Kind)
   Result.push_back(Hint);
   }
   return Result;
 }
 
+enum HintSide { Left, Right };
+
 struct ExpectedHint {
   std::string Label;
   std::string RangeName;
+  HintSide Side = Left;
 
   friend std::ostream <<(std::ostream ,
   const ExpectedHint ) {
@@ -46,9 +49,13 @@
 
 MATCHER_P2(HintMatcher, Expected, Code, "") {
   return arg.label == Expected.Label &&
- arg.range == Code.range(Expected.RangeName);
+ arg.range == Code.range(Expected.RangeName) &&
+ arg.position ==
+ ((Expected.Side == Left) ? arg.range.start : arg.range.end);
 }
 
+MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
+
 template 
 void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
  ExpectedHints... Expected) {
@@ -58,18 +65,23 @@
   auto AST = TU.build();
 
   EXPECT_THAT(hintsOfKind(AST, Kind),
-  UnorderedElementsAre(HintMatcher(Expected, Source)...));
+  ElementsAre(HintMatcher(Expected, Source)...));
 }
 
+// Hack to allow expression-statements operating on parameter packs in C++14.
+template  void ignore(T &&...) {}
+
 template 
 void assertParameterHints(llvm::StringRef AnnotatedSource,
   ExpectedHints... Expected) {
+  ignore(Expected.Side = Left...);
   assertHints(InlayHintKind::ParameterHint, AnnotatedSource, Expected...);
 }
 
 template 
 void assertTypeHints(llvm::StringRef AnnotatedSource,
  ExpectedHints... Expected) {
+  ignore(Expected.Side = Right...);
   assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
 }
 
@@ -631,6 +643,18 @@
   ExpectedHint{": int", "var"});
 }
 
+TEST(InlayHints, RestrictRange) {
+  Annotations Code(R"cpp(
+auto a = false;
+[[auto b = 1;
+auto c = '2';]]
+auto d = 3.f;
+  )cpp");
+  auto AST = TestTU::withCode(Code.code()).build();
+  EXPECT_THAT(inlayHints(AST, Code.range()),
+  ElementsAre(labelIs(": int"), labelIs(": char")));
+}
+
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -327,8 +327,14 @@
 Hidden,
 };
 
-opt InlayHints{"inlay-hints", cat(Features),
- desc("Enable preview of InlayHints feature"), init(false)};
+opt InlayHints{
+"inlay-hints",
+cat(Features),
+desc("Enable InlayHints feature"),
+init(ClangdLSPServer::Options().InlayHints),
+// FIXME: allow inlayHints to be disabled in Config and remove this option.
+Hidden,
+};
 
 opt WorkerThreadsCount{
 "j",
Index: clang-tools-extra/clangd/test/inlayHints.test
===
--- /dev/null
+++ 

[PATCH] D115049: Fall back on Android triple w/o API level for runtimes search

[Nico Weber via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e08a1208889: [clang] Fall back on Android triple w/o API 
level for runtimes search (authored by collinbaker, committed by thakis).

Changed prior to commit:
  https://reviews.llvm.org/D115049?vs=397394=397685#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115049

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -25,3 +25,21 @@
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-X8664 %s
 // CHECK-FILE-NAME-X8664: lib{{/|\\}}x86_64-unknown-linux-gnu{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android21 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android23 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID %s
+// CHECK-FILE-NAME-ANDROID: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
Index: clang/lib/Driver/ToolChains/VEToolchain.cpp
===
--- clang/lib/Driver/ToolChains/VEToolchain.cpp
+++ clang/lib/Driver/ToolChains/VEToolchain.cpp
@@ -48,7 +48,8 @@
   //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPath)
   //   ${RESOURCEDIR}/lib/linux/ve, (== getArchSpecificLibPath)
   //   ${SYSROOT}/opt/nec/ve/lib,
-  getFilePaths().push_back(getStdlibPath());
+  for (auto  : getStdlibPaths())
+getFilePaths().push_back(std::move(Path));
   getFilePaths().push_back(getArchSpecificLibPath());
   getFilePaths().push_back(computeSysRoot() + "/opt/nec/ve/lib");
 }
Index: clang/lib/Driver/ToolChains/Fuchsia.cpp
===
--- clang/lib/Driver/ToolChains/Fuchsia.cpp
+++ clang/lib/Driver/ToolChains/Fuchsia.cpp
@@ -191,9 +191,11 @@
 
   auto FilePaths = [&](const Multilib ) -> std::vector {
 std::vector FP;
-SmallString<128> P(getStdlibPath());
-llvm::sys::path::append(P, M.gccSuffix());
-FP.push_back(std::string(P.str()));
+for (const std::string  : getStdlibPaths()) {
+  SmallString<128> P(Path);
+  llvm::sys::path::append(P, M.gccSuffix());
+  FP.push_back(std::string(P.str()));
+}
 return FP;
   };
 
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -75,17 +75,16 @@
  const ArgList )
 : D(D), Triple(T), Args(Args), CachedRTTIArg(GetRTTIArgument(Args)),
   CachedRTTIMode(CalculateRTTIMode(Args, Triple, CachedRTTIArg)) {
-  std::string RuntimePath = getRuntimePath();
-  if (getVFS().exists(RuntimePath))
-getLibraryPaths().push_back(RuntimePath);
-
-  std::string StdlibPath = getStdlibPath();
-  if (getVFS().exists(StdlibPath))
-getFilePaths().push_back(StdlibPath);
+  auto addIfExists = [this](path_list , const std::string ) {
+if (getVFS().exists(Path))
+  List.push_back(Path);
+  };
 
-  std::string CandidateLibPath = getArchSpecificLibPath();
-  if (getVFS().exists(CandidateLibPath))
-getFilePaths().push_back(CandidateLibPath);
+  for (const auto  : getRuntimePaths())
+addIfExists(getLibraryPaths(), Path);
+  for (const auto  : getStdlibPaths())
+

[clang] 7e08a12 - [clang] Fall back on Android triple w/o API level for runtimes search

[Nico Weber via cfe-commits]

Author: Collin Baker
Date: 2022-01-05T16:00:48-05:00
New Revision: 7e08a1208889756bb7c44121f63a1b32f6c87ea5

URL: 
https://github.com/llvm/llvm-project/commit/7e08a1208889756bb7c44121f63a1b32f6c87ea5
DIFF: 
https://github.com/llvm/llvm-project/commit/7e08a1208889756bb7c44121f63a1b32f6c87ea5.diff

LOG: [clang] Fall back on Android triple w/o API level for runtimes search

Clang searches for runtimes (e.g. libclang_rt*) first in a
subdirectory named for the target triple (corresponding to
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON), then if it's not found uses
.../lib//libclang_rt* with a suffix corresponding to the arch and
environment name.

Android triples optionally include an API level indicating the minimum
Android version to be run on
(e.g. aarch64-unknown-linux-android21). When compiler-rt is built with
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON this API level is part of the
output path.

Linking code built for a later API level against a runtime built for
an earlier one is safe. In projects with several API level targets
this is desireable to avoid re-building the same runtimes many
times. This is difficult with the current runtime search method: if
the API levels don't exactly match Clang gives up on the per-target
runtime directory path.

To enable this more simply, this change tries target triple without
the API level before falling back on the old layout.

Another option would be to try every API level in the triple,
e.g. check aarch-64-unknown-linux-android21, then ...20, then ...19,
etc.

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

Added: 

clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android/libclang_rt.builtins.a

clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a

Modified: 
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/Driver.cpp
clang/lib/Driver/ToolChain.cpp
clang/lib/Driver/ToolChains/Fuchsia.cpp
clang/lib/Driver/ToolChains/VEToolchain.cpp
clang/test/Driver/linux-per-target-runtime-dir.c

Removed: 




diff  --git a/clang/include/clang/Driver/ToolChain.h 
b/clang/include/clang/Driver/ToolChain.h
index 4afc9bf36b5f7..eb95806a2f75d 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -452,11 +452,11 @@ class ToolChain {
 StringRef Component,
 FileType Type = ToolChain::FT_Static) 
const;
 
-  // Returns target specific runtime path if it exists.
-  virtual std::string getRuntimePath() const;
+  // Returns target specific runtime paths.
+  path_list getRuntimePaths() const;
 
-  // Returns target specific standard library path if it exists.
-  virtual std::string getStdlibPath() const;
+  // Returns target specific standard library paths.
+  path_list getStdlibPaths() const;
 
   // Returns /lib//.  This is used by runtimes (such
   // as OpenMP) to find arch-specific libraries.

diff  --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 4ac48cc280169..bb7ccf7dd97eb 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1869,9 +1869,16 @@ bool Driver::HandleImmediateArgs(const Compilation ) {
   }
 
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
-std::string CandidateRuntimePath = TC.getRuntimePath();
-if (getVFS().exists(CandidateRuntimePath))
-  llvm::outs() << CandidateRuntimePath << '\n';
+std::string RuntimePath;
+// Get the first existing path, if any.
+for (auto Path : TC.getRuntimePaths()) {
+  if (getVFS().exists(Path)) {
+RuntimePath = Path;
+break;
+  }
+}
+if (!RuntimePath.empty())
+  llvm::outs() << RuntimePath << '\n';
 else
   llvm::outs() << TC.getCompilerRTPath() << '\n';
 return false;

diff  --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 50c89aaadc189..7551ee4aeb79f 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -75,17 +75,16 @@ ToolChain::ToolChain(const Driver , const llvm::Triple ,
  const ArgList )
 : D(D), Triple(T), Args(Args), CachedRTTIArg(GetRTTIArgument(Args)),
   CachedRTTIMode(CalculateRTTIMode(Args, Triple, CachedRTTIArg)) {
-  std::string RuntimePath = getRuntimePath();
-  if (getVFS().exists(RuntimePath))
-getLibraryPaths().push_back(RuntimePath);
-
-  std::string StdlibPath = getStdlibPath();
-  if (getVFS().exists(StdlibPath))
-getFilePaths().push_back(StdlibPath);
+  auto addIfExists = [this](path_list , const std::string ) {
+if (getVFS().exists(Path))
+  List.push_back(Path);
+  };
 
-  std::string CandidateLibPath = getArchSpecificLibPath();
-  if (getVFS().exists(CandidateLibPath))
-getFilePaths().push_back(CandidateLibPath);
+  for (const auto  : 

[PATCH] D115670: Implement some constexpr vector unary operators, fix boolean-ops

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added a comment.

In D115670#3220052 , @erichkeane 
wrote:

> Should be fixed here: 2edc21e8566be8fa9b20e0bb71a83af90ec9aa97 
> 
>
> Thanks!

Thanks so much for the fix Erich! I can confirm the `altivec` test cases are 
all passing now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115670

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

[PATCH] D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.

LGTM this is something we have a use case for this in Fuchsia as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116503

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

[PATCH] D115049: Fall back on Android triple w/o API level for runtimes search

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.

lg!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115049

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

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116328#3223299 , @aaron.ballman 
wrote:

> In D116328#3223268 , 
> @LegalizeAdulthood wrote:
>
>> In D116328#3221995 , 
>> @aaron.ballman wrote:
>>
>>> 
>
> `has()` matches the direct descendant AST node, whereas `hasDescendant()` 
> matches *any* descendant AST node.
> [...] I don't know of any performance concerns with `has()` though.

Thanks for the explanation!

>> Honestly, at this point, I become lost in manually tracing the code through 
>> "go to definition" in my IDE.
>
> I'm impressed you got this far -- the AST matching machinery under the hood 
> is really quite complex! :-)

Having CMake generate a VS project and drilling in with ReSharper for C++ is
pretty powerful :), but you have to manually keep track of how the template
and function arguments are being resolved in the dynamic execution.  So it's
pretty easy to get lost in template oriented code, which is understandably
pretty common in AST land.

> My understanding (and my recollection here may be wrong) is that `has()` can 
> memoize the results
> [...] but you can't get the same behavior from `hasDescendant()` or 
> `hasAncestor()`

OK, that makes sense.

> I'm adding @sammccall in case he has information about the performance 
> characteristics or thoughts about
> the proposed matcher in general.

So let me summarize what we've learned so far:

- `has` only descends one level, is memoizable, and is less expensive than 
`hasDescendant`
- `has` uses the Visitor pattern to compute the result that is memoized
- @sammccall  might be aware of any performance related issues with `has`

My takeaway:

- if `has` isn't expensive, I can either ditch this public matcher or move it 
to be a private matcher used in my check


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

https://reviews.llvm.org/D116328

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

[PATCH] D116518: [ast-matchers] Add hasSubstatementSequence matcher

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5435-5442
+/// Matches two consecutive statements within a compound statement.
+///
+/// Given
+/// \code
+///   { if (x > 0) return true; return false; }
+/// \endcode
+/// compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > LegalizeAdulthood wrote:
> > > aaron.ballman wrote:
> > > > LegalizeAdulthood wrote:
> > > > > aaron.ballman wrote:
> > > > > > How do we extend this to support testing arbitrary sequences of 
> > > > > > statements? (If this supports two statements, someone will find a 
> > > > > > need for three, etc).
> > > > > Yeah, I was wondering that too.  I didn't see (but didn't look 
> > > > > extensively) any support for variadic matchers taking a parameter 
> > > > > pack.
> > > > > 
> > > > > I stopped at 2 because this suits my immediate needs with 
> > > > > `readability-simplify-boolean-expr` where I have to manually loop 
> > > > > over `CompoundStmt` matches in order to verify that the `if (x) 
> > > > > return true; return false;` constructs consist of two adjacent 
> > > > > statements.
> > > > I don't think we have any variadic matchers yet to model on, but I 
> > > > think if this is only needed for one check, we can add it in the 
> > > > current form as a local matcher for that check. Whenever we figure out 
> > > > how to give the better interface through the static and dynamic 
> > > > matchers, then we can figure out how to lift this to the general 
> > > > matcher interface.
> > > > 
> > > > WDYT?
> > > I don't think it is harmful to make it visible to all and I think it is 
> > > helpful.
> > > Defining it in ASTMatchers, enables using it in `clang-query`, for 
> > > instance.
> > > 
> > I contend this is not a generally useful matcher without supporting an 
> > arbitrary number of statements. Even then, to be honest, it's questionable 
> > whether there's sufficient need for this to be a public matcher. Typically, 
> > we don't expose a new public matcher unless there's a general need for it, 
> > and this one is already pretty borderline even if it's fully generalized. 
> > This file is *very* expensive to instantiate and it gets used in a lot of 
> > places, so that's one of the primary reasons we don't expose matchers from 
> > here unless they're generally useful.
> > 
> > Unless @klimek or another AST matcher code owner thinks this is useful in 
> > general (to multiple checks people are likely to want to write even if 
> > they're not contributing the checks to the community), I'm opposed to 
> > exposing this as-is. However, adding it as a private matcher for the check 
> > that needs the limited functionality would get no opposition from me (and 
> > this implementation looks correct as well).
> My thoughts:
> 
> I'm OK with moving it as a private matcher as it would simplify a big chunk 
> of code
> in the simplify-boolean-expr check.
> 
> I agree that ASTMatchers.h is used all over the place and at a minimum causes 
> a
> huge amount of rebuild.
> 
> Regarding the general usefulness of the matcher, let me elaborate on my 
> motivation
> for adding this matcher.
> 
> I see it from the viewpoint of a developer of checks/refactorings.
> 
> I really want us to get to a world where a complete refactoring can be 
> specified as
> a script input to a refactoring tool.  Eliminating the need to write C++ that 
> directly
> manipulates the AST and the edits will lower the bar for entry for other 
> people
> writing automated changes to their codebases.
> 
> Since the last time I worked in the clang codebase, the transformer library 
> has
> been added.  With this new library, I think all we're missing is a parser 
> that matches
> the input script to the necessary calls to code in the transformer library.  
> Once we
> do this, I think the need for more and higher-level matchers will become 
> evident
> and a matcher like this is the only way you can specify that a statement Y
> immediately follows statement X purely through matchers.  Current matchers
> don't even let you specify relative ordering of statements.  The best you can 
> do
> is assert that a block contains the statements of interest and then you must 
> write
> your own C++ code to walk the block and determine if they fit your actual
> match criteria.
Also, regarding a variadic version of this matcher, I'd be curious to try it out
just from a learning/programming perspective, but I'm not sure how I'd go about
it.  Suggestions on a plan of attack would be most welcome! `:)`



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

https://reviews.llvm.org/D116518

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

[PATCH] D116518: [ast-matchers] Add hasSubstatementSequence matcher

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5435-5442
+/// Matches two consecutive statements within a compound statement.
+///
+/// Given
+/// \code
+///   { if (x > 0) return true; return false; }
+/// \endcode
+/// compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > How do we extend this to support testing arbitrary sequences of 
> > > > > statements? (If this supports two statements, someone will find a 
> > > > > need for three, etc).
> > > > Yeah, I was wondering that too.  I didn't see (but didn't look 
> > > > extensively) any support for variadic matchers taking a parameter pack.
> > > > 
> > > > I stopped at 2 because this suits my immediate needs with 
> > > > `readability-simplify-boolean-expr` where I have to manually loop over 
> > > > `CompoundStmt` matches in order to verify that the `if (x) return true; 
> > > > return false;` constructs consist of two adjacent statements.
> > > I don't think we have any variadic matchers yet to model on, but I think 
> > > if this is only needed for one check, we can add it in the current form 
> > > as a local matcher for that check. Whenever we figure out how to give the 
> > > better interface through the static and dynamic matchers, then we can 
> > > figure out how to lift this to the general matcher interface.
> > > 
> > > WDYT?
> > I don't think it is harmful to make it visible to all and I think it is 
> > helpful.
> > Defining it in ASTMatchers, enables using it in `clang-query`, for instance.
> > 
> I contend this is not a generally useful matcher without supporting an 
> arbitrary number of statements. Even then, to be honest, it's questionable 
> whether there's sufficient need for this to be a public matcher. Typically, 
> we don't expose a new public matcher unless there's a general need for it, 
> and this one is already pretty borderline even if it's fully generalized. 
> This file is *very* expensive to instantiate and it gets used in a lot of 
> places, so that's one of the primary reasons we don't expose matchers from 
> here unless they're generally useful.
> 
> Unless @klimek or another AST matcher code owner thinks this is useful in 
> general (to multiple checks people are likely to want to write even if 
> they're not contributing the checks to the community), I'm opposed to 
> exposing this as-is. However, adding it as a private matcher for the check 
> that needs the limited functionality would get no opposition from me (and 
> this implementation looks correct as well).
My thoughts:

I'm OK with moving it as a private matcher as it would simplify a big chunk of 
code
in the simplify-boolean-expr check.

I agree that ASTMatchers.h is used all over the place and at a minimum causes a
huge amount of rebuild.

Regarding the general usefulness of the matcher, let me elaborate on my 
motivation
for adding this matcher.

I see it from the viewpoint of a developer of checks/refactorings.

I really want us to get to a world where a complete refactoring can be 
specified as
a script input to a refactoring tool.  Eliminating the need to write C++ that 
directly
manipulates the AST and the edits will lower the bar for entry for other people
writing automated changes to their codebases.

Since the last time I worked in the clang codebase, the transformer library has
been added.  With this new library, I think all we're missing is a parser that 
matches
the input script to the necessary calls to code in the transformer library.  
Once we
do this, I think the need for more and higher-level matchers will become evident
and a matcher like this is the only way you can specify that a statement Y
immediately follows statement X purely through matchers.  Current matchers
don't even let you specify relative ordering of statements.  The best you can do
is assert that a block contains the statements of interest and then you must 
write
your own C++ code to walk the block and determine if they fit your actual
match criteria.


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

https://reviews.llvm.org/D116518

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

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in 
function declaration

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > Can you also add a test for:
> > ```
> > void func(return_t(void));
> > ```
> `:-)`
> 
> What are you suggesting the result should be?  Honestly, looking at that, I'm 
> not sure myself `:)`
> 
> IMO, if I saw this in a code review, I would flag it because you're using a 
> macro called "return type" to specify the type of an argument.
LoL, yeah, the name `return_t` would certainly be novel to use in a parameter 
list, but what I was hoping to test is whether we try to fix the use of the 
macro within the parameter list or not. I *think* it probably makes sense to 
issue the diagnostic, but I don't think it makes sense to try to fix it because 
the macro could be defined differently for different configurations. But the 
diagnostic is silenced as well as the fix-it, I wouldn't lose a whole lot of 
sleep over it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116425

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

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Aaron Ballman via Phabricator via cfe-commits]

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

In D116328#3223268 , 
@LegalizeAdulthood wrote:

> In D116328#3221995 , @aaron.ballman 
> wrote:
>
>>> Previously if you wanted to match the statement associated with a case, 
>>> default, or labelled statement, you had to use hasDescendant.
>>
>> This isn't true. You can use `has()` to do direct substatemematching, and 
>> I'm wondering whether we need this new matcher at all. e.g., 
>> https://godbolt.org/z/K5sYP757r
>
> Well, the whole point of this was that previous review comments were 
> complaining that I was using an "expensive" matcher
> instead of one that got right to the point.  (What's the difference between 
> `has` and `hasDescendent` anyway?)

`has()` matches the direct descendant AST node, whereas `hasDescendant()` 
matches *any* descendant AST node. So `hasDescendant()` actually is expensive 
-- it can traverse the AST more times than you'd expect. I don't know of any 
performance concerns with `has()` though.

e.g., https://godbolt.org/z/n3adEz4qW (the first query matches nothing, the 
second query has a match)

> I looked at the implementation of "has" and it has a huge amount of machinery 
> underneath it.  I drilled in as follows (my ToT hash is 
> 773ab3c6655f4d2beec25bb3516b4d4fe2eea990 
> ):
> ASTMatchers.h:3391 declares `has` as an instance of 
> `internal::ArgumentAdaptingMatcherFunc`
> `ArgumentAdaptingMatcherFun` appears just to be a factory mechanism for 
> creating the appropriate matcher.
> `HasMatcher` seems to do the actual matching
> `HasMatcher::matches` delegates to 
> `ASTMatchFinder::matchesChildOf(const T , ...)`
> `ASTMatchFinder::matchesChildOf(const T , ...)` delegates to another 
> overload of `matchesChildOf`(const DynTypedNode , ...)` after asserting 
> a static type relationship (ASTMatchersInternal.h:762)
> `ASTMatchFinder::matchesChildOf(const DynTypedNode , ...)` is a virtual 
> function with one implementation in `MatchASTVisitor` (ASTMatchFinder.cpp:659)
> `MatchASTVisitor::matchesChildOf` delegates to `memoizedMatchesRecursively` 
> (ASTMatchFinder.cpp:664)
> For non-memoizable nodes, `MatchASTVisitor::memoizedMatchesRecursively` 
> delegates to `matchesRecursively` (ASTMatchFinder.cpp:599)
> `MatchASTVIsitor::matchesRecursively` instantiates a 
> `ASTNodeNotSpelledInSourceScope` and a `MatchChildASTVisitor` upon which it 
> calls `findMatch` (ASTMatchFinder.cpp:645)
> `MatchChildASTVisitor::findMatch` does a series of `DynNode.get` checks 
> and delegates to the appropriate overload of 
> `MatchChildASTVisitor::traverse`, in our case it would be the one for `Stmt`
> `MatchChildASTVisitor::traverse(const T )` delegates to 
> `MatchChildASTVisitor::match(const T )`
> `MatchChildASTVisitor::match(const T )` instantiates a 
> `BoundNodesTreeBuilder` and calls `match` on the held `Matcher`.
>
> Honestly, at this point, I become lost in manually tracing the code through 
> "go to definition" in my IDE.

I'm impressed you got this far -- the AST matching machinery under the hood is 
really quite complex! :-)

> So I switched my unit test for `HasCaseSubstmt` to use `has` and drilled in 
> through the debugger.
> What I saw echoes the above, although I took the wrong branch in my manual 
> analysis in `memoizedMatchesRecursively`, it goes down the memoization path.

Phew, that's good -- I would expect `has` to use the memoization pass.

> However, the whole point of memoizing a result is because that result was 
> expensive to compute and the memoization saves time on subsequent queries
> because the result has been memorized.  In this case, it's a simple getter on 
> `CaseStmt` to obtain the node against which you want to match, so I don't
> see the memoization as having any particular benefit.  (The inner matcher to 
> `hasSubstatement` may be expensive and could be memoized.)  For results
> obtain by the visitor pattern, I can see why you'd want to memoize them as 
> there is lots of code being executed to implement the Visitor pattern.

My understanding (and my recollection here may be wrong) is that `has()` can 
memoize the results because the sub-matcher will always be matched against the 
immediate direct descendant AST node, but you can't get the same behavior from 
`hasDescendant()` or `hasAncestor()` because any of the descendant (or 
ancestor) nodes can be matched, so you have to traverse them instead of 
memoizing. I'm adding @sammccall in case he has information about the 
performance characteristics or thoughts about the proposed matcher in general.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/include/clang/AST/Type.h:6481
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type  = **this;
+  if (Self.isObjectType() || Self.isReferenceType())

aaron.ballman wrote:
> This is unsafe -- the `Type *` can be null if the `QualType` is invalid.
I think this is fixed?



Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

aaron.ballman wrote:
> A function without a prototype is referenceable? (This is more of a "should 
> this predicate do anything in C?" kind of question, I suppose.)
Does C++ have a notion of non-prototyped functions? I don't think it does? As 
such, I'm struggling to model this in a way that makes sense :(

Maybe that's evidence for NAD, maybe it isn't :shrug:



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

aaron.ballman wrote:
> This can be reformatted, I believe, but did you look around to see if an 
> existing diagnostic would suffice?
Do you have any tips on how to narrow my search? I don't really want to 
//read// 11K lines of diagnostics.



Comment at: clang/lib/AST/ASTContext.cpp:5604
 /// savings are minimal and these are rare.
+// Update 2021-12-16: it might be worth revisiting this
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

WDYT @aaron.ballman?



Comment at: clang/lib/Sema/DeclSpec.cpp:597
   case DeclSpec::TST_decltype_auto: return "decltype(auto)";
+  // clang-format off
   case DeclSpec::TST_underlyingType: return "__underlying_type";

aaron.ballman wrote:
> We don't typically add clang-format markings to the source files. I think 
> this should be removed (it disables the formatting for the remainder of the 
> file).
My intention was always to delete these pre-merge. clang-format has some really 
strong opinions on this that breaks consistency with the rest of the file, and 
it was disrupting CI.

> (it disables the formatting for the remainder of the file).

Line 615 should prevent this :-)



Comment at: clang/lib/Sema/SemaType.cpp:1694
 if (Result.isNull()) {
-  Result = Context.IntTy;
+  if (DS.getTypeSpecType() == DeclSpec::TST_underlyingType)
+Result = Context.IntTy;

aaron.ballman wrote:
> The point to this was for error recovery; can we recover enough type 
> information from the non-underlying type cases to recover similarly?
Looks like we can get away with removing the selection.



Comment at: clang/lib/Sema/SemaType.cpp:9121
+SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);

aaron.ballman wrote:
> Should we care about ObjC pointers (which are a bit special)?
What's the compat story between ObjC and C++?



Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(

aaron.ballman wrote:
> What about things like type attributes (are those lost during decay)?
According to https://eel.is/c++draft/tab:meta.trans.other, no.



Comment at: clang/test/SemaCXX/type-traits.cpp:3447
+{ int a[T(__is_same(add_cv_t, const volatile M))]; }
+{ int a[T(__is_same(add_lvalue_reference_t, M))]; }
+{ int a[T(__is_same(add_pointer_t, M))]; }

aaron.ballman wrote:
> Shouldn't this be the same as `M&`?
> 
> Actually, something funky is going on that I've not looked into very far. 
> When I try the test out and use `M&` instead of `M` here: 
> https://godbolt.org/z/rx5bcsfqe, so we should be sure we're instantiating the 
> tests we expect to instantiate.
Appreciate that you're thorough in your reviews! (Seriously: it's easy to start 
glazing over tests like this).

Per https://eel.is/c++draft/meta.trans.ref, `add_lvalue_reference_t` should 
be `T` whenever it's not a referenceable type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

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

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in 
function declaration

aaron.ballman wrote:
> Can you also add a test for:
> ```
> void func(return_t(void));
> ```
`:-)`

What are you suggesting the result should be?  Honestly, looking at that, I'm 
not sure myself `:)`

IMO, if I saw this in a code review, I would flag it because you're using a 
macro called "return type" to specify the type of an argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116425

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

[PATCH] D116272: [Clang][Sema] Avoid crashing for va_arg expressions with bool argument

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a subscriber: tstellar.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for the fix!

I think this would also be a reasonable candidate for a 13.0.X release, in case 
you wanted to file an issue to backport it. I think 13.0.1 may be too late to 
hit though. CCing @tstellar just in case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116272

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

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

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

In D116328#3221995 , @aaron.ballman 
wrote:

>> Previously if you wanted to match the statement associated with a case, 
>> default, or labelled statement, you had to use hasDescendant.
>
> This isn't true. You can use `has()` to do direct substatemematching, and I'm 
> wondering whether we need this new matcher at all. e.g., 
> https://godbolt.org/z/K5sYP757r

Well, the whole point of this was that previous review comments were 
complaining that I was using an "expensive" matcher
instead of one that got right to the point.  (What's the difference between 
`has` and `hasDescendent` anyway?)

I looked at the implementation of "has" and it has a huge amount of machinery 
underneath it.  I drilled in as follows (my ToT hash is 
773ab3c6655f4d2beec25bb3516b4d4fe2eea990 
):
ASTMatchers.h:3391 declares `has` as an instance of 
`internal::ArgumentAdaptingMatcherFunc`
`ArgumentAdaptingMatcherFun` appears just to be a factory mechanism for 
creating the appropriate matcher.
`HasMatcher` seems to do the actual matching
`HasMatcher::matches` delegates to 
`ASTMatchFinder::matchesChildOf(const T , ...)`
`ASTMatchFinder::matchesChildOf(const T , ...)` delegates to another 
overload of `matchesChildOf`(const DynTypedNode , ...)` after asserting a 
static type relationship (ASTMatchersInternal.h:762)
`ASTMatchFinder::matchesChildOf(const DynTypedNode , ...)` is a virtual 
function with one implementation in `MatchASTVisitor` (ASTMatchFinder.cpp:659)
`MatchASTVisitor::matchesChildOf` delegates to `memoizedMatchesRecursively` 
(ASTMatchFinder.cpp:664)
For non-memoizable nodes, `MatchASTVisitor::memoizedMatchesRecursively` 
delegates to `matchesRecursively` (ASTMatchFinder.cpp:599)

  `MatchASTVIsitor::matchesRecursively` instantiates a 
`ASTNodeNotSpelledInSourceScope` and a `MatchChildASTVisitor` upon which it 
calls `findMatch` (ASTMatchFinder.cpp:645)
  `MatchChildASTVisitor::findMatch` does a series of `DynNode.get` checks 
and delegates to the appropriate overload of `MatchChildASTVisitor::traverse`, 
in our case it would be the one for `Stmt`
  `MatchChildASTVisitor::traverse(const T )` delegates to 
`MatchChildASTVisitor::match(const T )`
  `MatchChildASTVisitor::match(const T )` instantiates a 
`BoundNodesTreeBuilder` and calls `match` on the held `Matcher`.

Honestly, at this point, I become lost in manually tracing the code through "go 
to definition" in my IDE.  So I switched my unit test for `HasCaseSubstmt` to 
use `has` and drilled in through the debugger.
What I saw echoes the above, although I took the wrong branch in my manual 
analysis in `memoizedMatchesRecursively`, it goes down the memoization path.

However, the whole point of memoizing a result is because that result was 
expensive to compute and the memoization saves time on subsequent queries
because the result has been memorized.  In this case, it's a simple getter on 
`CaseStmt` to obtain the node against which you want to match, so I don't
see the memoization as having any particular benefit.  (The inner matcher to 
`hasSubstatement` may be expensive and could be memoized.)  For results
obtain by the visitor pattern, I can see why you'd want to memoize them as 
there is lots of code being executed to implement the Visitor pattern.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2432
 dependentCoawaitExpr;
+
 /// Matches co_yield expressions.

aaron.ballman wrote:
> Spurious newline?
I didn't add this intentionally and I can remove it, but the general style in 
this file is to separate top-level decls by a blank line.


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

https://reviews.llvm.org/D116328

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

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Ping. @mojca, do you need help landing the patch?


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

https://reviews.llvm.org/D114601

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

[PATCH] D116425: [clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Generally LGTM, just a testing request.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in 
function declaration

Can you also add a test for:
```
void func(return_t(void));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116425

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

[PATCH] D116673: [Clang][NVPTX]Add NVPTX intrinsics and builtins for CUDA PTX cvt sm80 instructions

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

LGTM overall.




Comment at: clang/include/clang/Basic/BuiltinsNVPTX.def:405
 
+TARGET_BUILTIN(__nvvm_ff2v2bf_rn, "ZUiff", "", AND(SM_80,PTX70))
+TARGET_BUILTIN(__nvvm_ff2v2bf_rn_relu, "ZUiff", "", AND(SM_80,PTX70))

Nit: `ff2v2bf` is a bit hard to parse. I initially tried to interpret it as 
"convert ff2v to bf" and was confused about  what exactly does `2v` part mean 
-- we already have `ff` to denote two floats.

Perhaps `ff2bf16x2` would be a bit easier to read and understand. It would also 
work consistently for `f16` and `tf32` variants below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116673

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

[PATCH] D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/diagnostics.c:2
 // Parse diagnostic arguments in the driver
 // PR12181
 

If you are going to update comments, remove this PR12181. It says an issue 
about `-Werror=unused-command-line-argument` but your comment below has covered 
the information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116503

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

[PATCH] D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/diagnostics.c:34
+// there's no diagnostic.
+// RUN: %clang -target x86_64-apple-darwin10 -fsyntax-only \
+// RUN:   --start-no-unused-arguments -lfoo --end-no-unused-arguments \

You may change some `x86_64-apple-darwin10` to other triples, probably 
including `clang-cl` (to test `CoreOption`).

The canonical spelling for `-target ` is `---target=`. `-target ` (space 
separated driver option) is just abused so much that it is unrealistic to 
remove support now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116503

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Kevin Athey via Phabricator via cfe-commits]

kda updated this revision to Diff 397648.
kda added a comment.

trying again


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1667,6 +1667,13 @@
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
   MarshallingInfoEnum, "Global">;
+defm sanitize_memory_param_retval
+  : BoolOption<"f", "sanitize-memory-param-retval",
+  CodeGenOpts<"SanitizeMemoryParamRetval">,
+  DefaultFalse,
+  PosFlag, NegFlag,
+  BothFlags<[], "Detect uninitialized parameters and return values.">>,
+Group;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -231,6 +231,8 @@
 ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
 llvm::AsanDtorKind::Global)  ///< Set how ASan global
  ///< destructors are emitted.
+CODEGENOPT(SanitizeMemoryParamRetval, 1, 0) ///Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1667,6 +1667,13 @@
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
   MarshallingInfoEnum, "Global">;
+defm sanitize_memory_param_retval
+  : BoolOption<"f", "sanitize-memory-param-retval",
+  CodeGenOpts<"SanitizeMemoryParamRetval">,
+  DefaultFalse,
+  PosFlag, NegFlag,
+  BothFlags<[], "Detect uninitialized parameters and return values.">>,
+Group;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -231,6 +231,8 @@
 ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
 llvm::AsanDtorKind::Global)  ///< Set how ASan global
  ///< destructors are emitted.
+CODEGENOPT(SanitizeMemoryParamRetval, 1, 0) ///___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

[Fangrui Song via Phabricator via cfe-commits]

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

I forwarded this to some groups who want this functionality. Jannik2099 from 
Gentoo likes the change.

But make sure to wait a bit to see what others think.




Comment at: clang/test/Driver/diagnostics.c:21
+// RUN:   -fsyntax-only -lfoo -Werror %s 2>&1 | FileCheck %s
 
+// With a specific -Wno-..., no diagnostic should be printed.

mstorsjo wrote:
> MaskRay wrote:
> > Looks like no command has an output. In case there is one, make sure `-o 
> > /dev/null`
> As all commands use `-fsyntax-only`, I presume there should be no output. So 
> do I read your comment correctly that we should have `-o /dev/null` 
> specifically _if_ we'd have a test that lack `-fsyntax-only`, or do you want 
> me to add it just in case? (The preexisting test doesn't have that and just 
> use `-fsyntax-only`.)
-fsyntax-only doesn't need `-o /dev/null`.

Some downstream users (including Google) may run the test in the source 
directory and make the source directory in a read-only filesystem, so printing 
files other than somewhere in `%t*` will lead to an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116503

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Kevin Athey via Phabricator via cfe-commits]

kda added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1675
+  PosFlag, NegFlag,
+  BothFlags<[], "eager param-retval uninitialized use detection in 
MemorySanitizer">>,
+Group;

vitalybuka wrote:
> Maybe "Detect initialized parameters and return values."? and above
I think you meant 'uninitialized'.  Otherwise changed as requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88833#3223077 , @estan wrote:

> That did the trick, thanks @aaron.ballman.

Glad that worked!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D115249: Clang-Tidy implicit bool conversion check use upercase suffixes

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Something stylistic like this should likely be configurable. Lowercase literals 
are a part of the language and there is likely a codebase out there that 
explicitly prefers them


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115249

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Kevin Athey via Phabricator via cfe-commits]

kda updated this revision to Diff 397647.
kda marked an inline comment as done.
kda added a comment.

updated help text and dropped implementation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/attr-noundef.cpp
  clang/test/CodeGen/indirect-noundef.cpp


Index: clang/test/CodeGen/indirect-noundef.cpp
===
--- clang/test/CodeGen/indirect-noundef.cpp
+++ clang/test/CodeGen/indirect-noundef.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm 
-enable-noundef-analysis -o - %s | FileCheck %s
-// RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm 
-fsanitize-memory-param-retval -o - %s | FileCheck %s
 
 union u1 {
   int val;
Index: clang/test/CodeGen/attr-noundef.cpp
===
--- clang/test/CodeGen/attr-noundef.cpp
+++ clang/test/CodeGen/attr-noundef.cpp
@@ -1,7 +1,5 @@
 // RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm 
-enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-INTEL
 // RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm 
-enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-AARCH
-// RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm 
-fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-INTEL
-// RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm 
-fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK 
--check-prefix=CHECK-AARCH
 
 // Passing structs by value
 // TODO: No structs may currently be marked noundef
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2243,9 +2243,7 @@
  getLangOpts().Sanitize.has(SanitizerKind::Return);
 
   // Determine if the return type could be partially undef
-  if ((CodeGenOpts.EnableNoundefAttrs ||
-   CodeGenOpts.SanitizeMemoryParamRetval) &&
-  HasStrictReturn) {
+  if (CodeGenOpts.EnableNoundefAttrs && HasStrictReturn) {
 if (!RetTy->isVoidType() && RetAI.getKind() != ABIArgInfo::Indirect &&
 DetermineNoUndef(RetTy, getTypes(), DL, RetAI))
   RetAttrs.addAttribute(llvm::Attribute::NoUndef);
@@ -2380,9 +2378,7 @@
 
 // Decide whether the argument we're handling could be partially undef
 bool ArgNoUndef = DetermineNoUndef(ParamType, getTypes(), DL, AI);
-if ((CodeGenOpts.EnableNoundefAttrs ||
- CodeGenOpts.SanitizeMemoryParamRetval) &&
-ArgNoUndef)
+if (CodeGenOpts.EnableNoundefAttrs && ArgNoUndef)
   Attrs.addAttribute(llvm::Attribute::NoUndef);
 
 // 'restrict' -> 'noalias' is done in EmitFunctionProlog when we
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1672,7 +1672,7 @@
   CodeGenOpts<"SanitizeMemoryParamRetval">,
   DefaultFalse,
   PosFlag, NegFlag,
-  BothFlags<[], "eager param-retval uninitialized use detection in 
MemorySanitizer">>,
+  BothFlags<[], "Detect uninitialized parameters and return values.">>,
 Group;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is


Index: clang/test/CodeGen/indirect-noundef.cpp
===
--- clang/test/CodeGen/indirect-noundef.cpp
+++ clang/test/CodeGen/indirect-noundef.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -enable-noundef-analysis -o - %s | FileCheck %s
-// RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -fsanitize-memory-param-retval -o - %s | FileCheck %s
 
 union u1 {
   int val;
Index: clang/test/CodeGen/attr-noundef.cpp
===
--- clang/test/CodeGen/attr-noundef.cpp
+++ clang/test/CodeGen/attr-noundef.cpp
@@ -1,7 +1,5 @@
 // RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
 // RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-AARCH
-// RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm -fsanitize-memory-param-retval %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
-// RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S 

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

That did the trick, thanks @aaron.ballman.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

Ah yes I had a hunch it had to be "clang;clang-tools-extra" so running a build 
with that now. Almost done. It crashed at the very end due to OOM while 
linking, but reduced parallelism now and running again. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88833#3223055 , @estan wrote:

> @aaron.ballman A bit off topic, but I followed 
> https://github.com/llvm/llvm-project/blob/main/README.md#getting-the-source-code-and-building-llvm
>  to build and install llvm-project main branch as follows:
>
>   git clone https://github.com/llvm/llvm-project.git
>   cd llvm-project
>   cmake -S llvm -B build -G Ninja -DLLVM_ENABLE_PROJECTS=clang-tools-extra 
> -DCMAKE_INSTALL_PREFIX=/home/estan/orexplore/llvm-project-inst
>   cmake --build build
>   cmake --install build
>
> But I did not get any clang-tidy in the build or installation directory. The 
> CMake output said "clang-tools-extra project is enabled". Any tips on where I 
> went wrong?

That looks generally correct, though I think you may need to add clang to the 
list of LLVM projects to enable (clang-tidy relies on clang and I've never 
tried building clang-tools-extra without clang before). Also, you shouldn't 
need to do the install step for local development (so my own setup doesn't set 
the install prefix). FWIW, I do my development on Windows using Visual Studio 
and its integrated CMake support, so my CMake settings are going to be a bit 
different than yours anyway. FWIW, I use `-DLLVM_TARGETS_TO_BUILD="X86" 
-DLLVM_ENABLE_IDE=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" 
-DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:65
 void ProBoundsArrayToPointerDecayCheck::registerMatchers(MatchFinder *Finder) {
-  // The only allowed array to pointer decay
+  // We only allowed array to pointer decays
   // 1) just before array subscription




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

@aaron.ballman A bit off topic, but I followed 
https://github.com/llvm/llvm-project/blob/main/README.md#getting-the-source-code-and-building-llvm
 to build and install llvm-project main branch as follows:

  git clone https://github.com/llvm/llvm-project.git
  cd llvm-project
  cmake -S llvm -B build -G Ninja -DLLVM_ENABLE_PROJECTS=clang-tools-extra 
-DCMAKE_INSTALL_PREFIX=/home/estan/orexplore/llvm-project-inst
  cmake --build build
  cmake --install build

But I did not get any clang-tidy in the build or installation directory. The 
CMake output said "clang-tools-extra project is enabled". Any tips on where I 
went wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D116583: Change the default optimisation level of PTXAS from -O0 to -O3. This makes the optimisation levels of PTXAS and the ptxjitcompiler equal (ptxjitcompiler defaults to -O3).

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:433
   } else {
-// If no -O was passed, pass -O0 to ptxas -- no opt flag should correspond
-// to no optimizations, but ptxas's default is -O3.
-CmdArgs.push_back("-O0");
+// If no -O was passed, pass -O3 to ptxas -- this makes ptxas's
+// optimization level the same as the ptxjitcompiler.

hdelan wrote:
> tra wrote:
> > I think this would be contrary to the expectation that lack of `-O` in 
> > clang means - `do not optimize` and it generally implies the whole 
> > compilation chain, including assembler. Matching whatever nvidia tools do 
> > is an insufficient reason for breaking this assumption, IMO. 
> > 
> > If you do want do run optimized ptxas on unoptimized PTX, you can use 
> > `-Xcuda-ptxas -O3`.
> I think for the average user, consistency across the `ptxjitcompiler` and 
> `ptxas` is far more important than assuming that no `-O` means no 
> optimization. I think most users will assume that no `-O` will assume that 
> whatever tools being used will take their default optimization level, which 
> in the case of clang is `-O0` and in the case of `ptxas` is `-O3`. 
> 
> We have had a few bugs with `ptxas`/`ptxjitcompiler` at higher optimization 
> levels, which were quite hard to pin down since offline `ptxas` and 
> `ptxjitcompiler` were using different optimisation levels, making bugs appear 
> in one and not the other. Of course we are aware of this now but this 
> inconsistency can result in bugs that are difficult to diagnose. Having 
> consistency between the `ptxjitcompiler` and `ptxas` is therefore of 
> practical benefit. Whereas if we are to leave it as is, with `ptxas` 
> defaulting to `-O0`, the benefit is purely semantic and not practical.
> I think for the average user, consistency across the ptxjitcompiler and ptxas 
> is far more important than assuming that no -O means no optimization. 

The default is intended to provide the least amount of surprises for the most 
users. There are more users of clang as a CUDA compiler than users of clang as 
a cuda compiler who care about consistency with ptxjitcompiler. My point is 
that the improvements for a subset of users should be balanced vs usability in 
the common case. In this case the benefit does not justify the downsides, IMO.

Please add me as a reviewer when the patch is ready for public review and we'll 
discuss it in a wider LLVM community.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116583

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:149-151
+  const auto *ReceivingCallExpr = dyn_cast(ReceivingExpr);
+  const auto *ReceivingConstructExpr =
+  dyn_cast(ReceivingExpr);

It looks like `ReceivingExpr` can be null (see line 78 that you added), so the 
first `dyn_cast` will crash if given null. Further, if the result of the first 
dynamic cast is null, then the second `dyn_cast` will also crash.

Should there be a null pointer check? Should we use `cast` instead of 
`dyn_cast` for the first one?



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:153
+  if ((!ReceivingCallExpr ||
+   ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&
+  (!ReceivingConstructExpr ||

`getDirectCallee()` can return null.



Comment at: 
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:157-163
+  const NamedDecl *FunctionName = nullptr;
+  if (ReceivingCallExpr)
+FunctionName =
+ReceivingCallExpr->getDirectCallee()->getUnderlyingDecl();
+  else
+FunctionName =
+ReceivingConstructExpr->getConstructor()->getUnderlyingDecl();

`getDirectCallee()` can return nullptr.


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

https://reviews.llvm.org/D107450

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

[PATCH] D116675: [OpenMP] Search for static libraries in offload linker tool

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, gregrodgers, JonChesterfield, ronlieb.
Herald added subscribers: guansong, yaxunl.
jhuber6 requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

This patch adds support for searching through the linker library paths
to identify static libraries that may contain device code. If device
code is present it will be extracted. This should ideally fully support
static linking with OpenMP offloading.

Depends on D116627 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116675

Files:
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp


Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -65,7 +65,9 @@
 /// Path of the current binary.
 static std::string LinkerExecutable;
 
+/// Temporary files created by the linker wrapper.
 static SmallVector TempFiles;
+
 /// Magic section string that marks the existence of offloading data. The
 /// section string will be formatted as `.llvm.offloading..`.
 #define OFFLOAD_SECTION_MAGIC_STR ".llvm.offloading."
@@ -570,6 +572,44 @@
   return static_cast(ObjectFile);
 }
 
+Optional findFile(StringRef Dir, const Twine ) {
+  SmallString<128> Path;
+  // TODO: Parse `--sysroot` somewhere and use it here.
+  sys::path::append(Path, Dir, Name);
+  if (sys::fs::exists(Path))
+return static_cast(Path);
+  return None;
+}
+
+Optional findFromSearchPaths(StringRef Name,
+  ArrayRef SearchPaths) {
+  for (StringRef Dir : SearchPaths)
+if (Optional File = findFile(Dir, Name))
+  return File;
+  return None;
+}
+
+Optional searchLibraryBaseName(StringRef Name,
+ArrayRef SearchPaths) {
+  for (StringRef Dir : SearchPaths) {
+if (Optional File = findFile(Dir, "lib" + Name + ".a"))
+  return File;
+  }
+  return None;
+}
+
+/// Search for static libraries in the linker's library path given input like
+/// `-lfoo` or `-l:libfoo.a`.
+Optional searchLibrary(StringRef Input,
+ArrayRef SearchPaths) {
+  if (!Input.startswith("-l"))
+return None;
+  StringRef Name = Input.drop_front(2);
+  if (Name.startswith(":"))
+return findFromSearchPaths(Name.drop_front(), SearchPaths);
+  return searchLibraryBaseName(Name, SearchPaths);
+}
+
 } // namespace
 
 int main(int argc, const char **argv) {
@@ -600,16 +640,26 @@
   for (const std::string  : HostLinkerArgs)
 LinkerArgs.push_back(Arg);
 
+  SmallVector LibraryPaths;
+  for (const StringRef Arg : LinkerArgs)
+if (Arg.startswith("-L"))
+  LibraryPaths.push_back(Arg.drop_front(2));
+
   // Try to extract device code from the linker input and replace the linker
   // input with a new file that has the device section stripped.
   SmallVector DeviceFiles;
   for (std::string  : LinkerArgs) {
-if (sys::path::extension(Arg) == ".o" ||
-sys::path::extension(Arg) == ".a") {
+// Search for static libraries in the library link path.
+std::string Filename = Arg;
+if (Optional Library = searchLibrary(Arg, LibraryPaths))
+  Filename = *Library;
+
+if (sys::path::extension(Filename) == ".o" ||
+sys::path::extension(Filename) == ".a") {
   ErrorOr> BufferOrErr =
-  MemoryBuffer::getFileOrSTDIN(Arg);
+  MemoryBuffer::getFileOrSTDIN(Filename);
   if (std::error_code EC = BufferOrErr.getError())
-return reportError(createFileError(Arg, EC));
+return reportError(createFileError(Filename, EC));
 
   auto NewFileOrErr =
   extractFromBuffer(std::move(*BufferOrErr), DeviceFiles);


Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -65,7 +65,9 @@
 /// Path of the current binary.
 static std::string LinkerExecutable;
 
+/// Temporary files created by the linker wrapper.
 static SmallVector TempFiles;
+
 /// Magic section string that marks the existence of offloading data. The
 /// section string will be formatted as `.llvm.offloading..`.
 #define OFFLOAD_SECTION_MAGIC_STR ".llvm.offloading."
@@ -570,6 +572,44 @@
   return static_cast(ObjectFile);
 }
 
+Optional findFile(StringRef Dir, const Twine ) {
+  SmallString<128> Path;
+  // TODO: Parse `--sysroot` somewhere and use it here.
+  sys::path::append(Path, Dir, Name);
+  if (sys::fs::exists(Path))
+return static_cast(Path);
+  return None;
+}
+
+Optional findFromSearchPaths(StringRef Name,
+  ArrayRef SearchPaths) {
+  for (StringRef Dir : SearchPaths)
+ 

[PATCH] D115960: Revert D109159 "[amdgpu] Enable selection of `s_cselect_b64`."

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Not sure what happened here but this change added back a whole bunch of old 
code. I reverted this in 085f078307bac264301b07f6e47e2a04e90a6f1d 
 . Please 
carefully check `git diff origin/main` before committing next time :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115960

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

[PATCH] D115960: Revert D109159 "[amdgpu] Enable selection of `s_cselect_b64`."

[Fangrui Song via Phabricator via cfe-commits]

MaskRay reopened this revision.
MaskRay added a subscriber: ldionne.
MaskRay added a comment.

(CC @ldionne @smeenai)

The revert 859ebca744e634dcc89a2294ffa41574f947bd62 
 included 
many unintended changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115960

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88833#3222828 , @fiesh wrote:

>> My expectation was that @fiesh would be updating the review if they wanted 
>> this to land. If they indicate they're no longer interested in working on 
>> the patch, then I think it's fine for you to commandeer the patch. But you 
>> should give them a chance to speak up in case they're still intending to 
>> finish this. @fiesh, are you expecting to work on this further?
>
> Oh sorry, this has somehow completely escaped my attention.  I'm perfectly 
> fine with somebody else finalizing this patch as I will not be able to do so 
> in the foreseeable future.
>
> Happy new year!

Thank you for getting back to us and getting a start on this patch!

In D88833#3222843 , @estan wrote:

> And there he is :) I've never worked on LLVM / clang-tidy but could have a 
> look at making it opt-in. Now we know the status at least.

I'd say you're clear to commandeer (the Add Action pulldown has this option). 
As for making it an option, a lot of other checks have config options, so you 
can use one of them to model your changes after, like: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp#L33


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

And there he is :) I've never worked on LLVM / clang-tidy but could have a look 
at making it opt-in. Now we know the status at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D115425: [clangd] Generate ConfigFragment/YAML/docs from one tablegen source

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D115425#3222782 , @njames93 wrote:

> For the website, we use tags to specify the clangd version that the option 
> was first supported in.
> I'd suggest that we also add a `Version` field to the `Field` class.
> This could also cause problem down the line if we ever wanted to remove a 
> config option.

I definitely agree.
FWIW, I didn't send this for review because I wasn't sure about the idea.

Talked to @kadircet offline a bit, and I hope he doesn't mind me trying to 
summarize...

- avoiding a couple of duplicated parts is definitely good
- there's still config.h and the mapping thereto, which I don't think can be 
tablegenerated
- tablegen syntax is really bad and also hard to browse with the code (I agree)
- I don't see a better representation within the tablegen language (which is 
dumb, because this is a simple tree, but tablegen isn't good at trees)
- We couldn't think of a representation that would be *nice* to edit/maintain 
that's easy to have in-tree (apart from the current C++ structs, but parsing 
C++ at build time is terrible)

Actually we didn't discuss the option of using YAML itself as the format.
The downsides I can think of (vs current C++):

- no type system/checking/assistance for our weird DSL (tablegen a bit)
- have to use strings as comments rather than using comments as comments 
(though at least YAML has nice strings for this purpose)
- YAML is a pretty quirky language
- the inherent level-confusion of using YAML to describe a YAML schema

Maybe I'll mock this up, but I have some more pressing things to do so if 
anyone wants to shoot the idea first that'd be nice :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115425

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

Sounds good @aaron.ballman, let's wait for @fiesh.

Though I realize now that the scope of this patch is probably not enough to 
solve a problem we have in our code base. The check will warn about (for 
example) things like this:

In a third party lib outside our control:

  void f(double out[3]);

In our code:

  double out[3];
  fn_in_third_party_lib(out);

Include paths for the third party lib are added with -isystem.

Am I right that we're still going to get warnings for this with this patch?

Full disclosure. The third party lib is VTK, which is littered with APIs like 
these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

[fiesh via Phabricator via cfe-commits]

fiesh added a comment.

> My expectation was that @fiesh would be updating the review if they wanted 
> this to land. If they indicate they're no longer interested in working on the 
> patch, then I think it's fine for you to commandeer the patch. But you should 
> give them a chance to speak up in case they're still intending to finish 
> this. @fiesh, are you expecting to work on this further?

Oh sorry, this has somehow completely escaped my attention.  I'm perfectly fine 
with somebody else finalizing this patch as I will not be able to do so in the 
foreseeable future.

Happy new year!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D115960: Revert D109159 "[amdgpu] Enable selection of `s_cselect_b64`."

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.
Herald added a subscriber: JDevlieghere.

I believe you landed the wrong commit. 
rG859ebca744e634dcc89a2294ffa41574f947bd62 
 looks 
like the previous versions of this diff, where lots of extraneous changes were 
made. Could you please revert and land the correct commit? Feel free to ask for 
assistance if you need it :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115960

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

[PATCH] D115960: Revert D109159 "[amdgpu] Enable selection of `s_cselect_b64`."

[David Salinas via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG859ebca744e6: Revert D109159 [amdgpu] Enable selection 
of `s_cselect_b64`. (authored by david-salinas).
Herald added subscribers: cfe-commits, libcxx-commits, lldb-commits, 
Sanitizers, luke957, abrachet, ormris, dang, jdoerfert, sstefan1, phosek, 
usaxena95, pengfei, s.egerton, asbirlea, mstorsjo, lebedev.ri, kadircet, 
rupprecht, arphaman, steven_wu, mgrang, simoncook, fedor.sergeev, krytarowski, 
arichardson, mgorny, emaste, dschuff.
Herald added a reviewer: lebedev.ri.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: jhenderson.
Herald added projects: clang, Sanitizers, LLDB, libc++, lld-macho, 
clang-tools-extra.
Herald added a reviewer: libc++.
Herald added a reviewer: lld-macho.

Changed prior to commit:
  https://reviews.llvm.org/D115960?vs=395475=397626#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115960

Files:
  clang-tools-extra/clangd/unittests/TestScheme.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/HIP.h
  clang/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml
  clang/test/CodeGen/catch-alignment-assumption-blacklist.c
  clang/test/CodeGen/catch-nullptr-and-nonzero-offset-blacklist.c
  clang/test/CodeGen/ubsan-blacklist.c
  clang/test/CodeGenCXX/cfi-blacklist.cpp
  clang/test/Driver/debug-var-experimental-switch.c
  clang/test/Sema/branch-protection-attr-err.c
  compiler-rt/cmake/Modules/CustomLibcxx/CMakeLists.txt
  compiler-rt/lib/sanitizer_common/sanitizer_persistent_allocator.h
  compiler-rt/lib/tsan/rtl/tsan_update_shadow_word.inc
  compiler-rt/test/fuzzer/EntropicScalePerExecTimeTest.cpp
  compiler-rt/test/fuzzer/entropic-scale-per-exec-time.test
  compiler-rt/test/memprof/TestCases/mem_info_cache_entries.cpp
  compiler-rt/test/memprof/TestCases/print_miss_rate.cpp
  compiler-rt/test/ubsan/TestCases/Pointer/alignment-assumption-ignorelist.cppp
  libcxx/cmake/caches/Generic-32bits.cmake
  libcxx/include/__memory/pointer_safety.h
  libcxx/test/libcxx/atomics/ext-int.verify.cpp
  libcxx/test/libcxx/atomics/libcpp-has-no-threads.compile.fail.cpp
  libcxx/test/libcxx/atomics/libcpp-has-no-threads.pass.cpp
  libcxx/test/std/algorithms/robust_against_adl.pass.cpp
  
libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.compile.fail.cpp
  
libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_value.pass.cpp
  
libcxx/test/std/language.support/support.limits/support.limits.general/charconv.pass.cpp
  
libcxx/test/std/language.support/support.limits/support.limits.general/memory_resource.version.pass.cpp
  libcxx/test/std/numerics/c.math/abs.fail.cpp
  libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp
  
libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
  
libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.fail.cpp
  
libcxx/test/std/utilities/memory/util.dynamic.safety/declare_no_pointers.pass.cpp
  
libcxx/test/std/utilities/memory/util.dynamic.safety/declare_reachable.pass.cpp
  
libcxx/test/std/utilities/memory/util.dynamic.safety/get_pointer_safety.pass.cpp
  
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
  libcxx/test/support/coroutine_types.h
  libcxx/test/support/tracked_value.h
  libcxx/utils/google-benchmark/.clang-format
  libcxx/utils/google-benchmark/.github/.libcxx-setup.sh
  libcxx/utils/google-benchmark/.github/ISSUE_TEMPLATE/bug_report.md
  libcxx/utils/google-benchmark/.github/ISSUE_TEMPLATE/feature_request.md
  libcxx/utils/google-benchmark/.github/workflows/bazel.yml
  
libcxx/utils/google-benchmark/.github/workflows/build-and-test-perfcounters.yml
  libcxx/utils/google-benchmark/.github/workflows/build-and-test.yml
  libcxx/utils/google-benchmark/.github/workflows/pylint.yml
  libcxx/utils/google-benchmark/.github/workflows/sanitizer.yml
  libcxx/utils/google-benchmark/.github/workflows/test_bindings.yml
  libcxx/utils/google-benchmark/.gitignore
  libcxx/utils/google-benchmark/.travis.yml
  libcxx/utils/google-benchmark/.ycm_extra_conf.py
  libcxx/utils/google-benchmark/AUTHORS
  libcxx/utils/google-benchmark/BUILD.bazel
  libcxx/utils/google-benchmark/CMakeLists.txt
  libcxx/utils/google-benchmark/CONTRIBUTING.md
  libcxx/utils/google-benchmark/CONTRIBUTORS
  libcxx/utils/google-benchmark/LICENSE
  libcxx/utils/google-benchmark/README.md
  libcxx/utils/google-benchmark/WORKSPACE
  libcxx/utils/google-benchmark/_config.yml
  libcxx/utils/google-benchmark/appveyor.yml
  libcxx/utils/google-benchmark/bindings/python/BUILD
  libcxx/utils/google-benchmark/bindings/python/build_defs.bzl
  libcxx/utils/google-benchmark/bindings/python/google_benchmark/BUILD
  libcxx/utils/google-benchmark/bindings/python/google_benchmark/__init__.py
  

[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D115106#3218117 , @sammccall wrote:

> In D115106#3218000 , @aaron.ballman 
> wrote:
>
>>> I think `anyRedeclaration(functionDecl(isStaticStorageClass))`is too 
>>> difficult to get right and too easy to get wrong for such a common/simple 
>>> concept.
>>
>> Get right in terms of implementation, or in terms of use, or both?
>
> Use, if I'm understanding your question.
> If we don't expose this as a matcher with some name, it's a recipe the person 
> writing the matcher has to remember.
> There's no good way to look it up, and it's easy to write something that 
> seems good but isn't.
>
> (This problem is pervasive with AST and matchers, and this isn't the worst 
> example.)

Ah, I see what you're driving at better now, thank you!

>>> I know what a static method is, but I would have made the same mistake 
>>> here. In general asking people to describe syntax to answer a semantic 
>>> question invites this.
>>
>> Because our AST is intended to reflect the syntax, our AST matchers kind of 
>> *do* match syntax -- I think it's the reason we're in the problem (and this 
>> is not the first time it's come up).
>
> The clang AST is the best tool we have to describe syntax *and* to describe 
> semantics.
> Maybe by design it captures "all" syntax and "enough" semantics, but users 
> reasonably care less about the design than about their alternatives and 
> problems. The clang AST is useful for answering syntax questions (better than 
> regex!) but it's _irreplaceable_ for answering semantics questions (better 
> than... err?).
>
> So we have syntax and semantics in one namespace, and static is the worst 
> because it means like 3 syntactic and 5 semantic things :-(

Heh, those are fair points. I especially appreciate the clarity about whether 
we're matching syntax or semantics, that's really the crux of my concerns when 
you distill it.

>> There's a cognitive disconnect between the AST matchers traversing the AST 
>> and applying a match on all node and the C & C++ concept of redeclarations.
>
> Definitely, this highlights the disconnect really well.
> And some of the accessors on a redecl will delegate to the canonical while 
> others won't. (Can we call this "syntax semantics" vs "semantics semantics"?!)
>
> I definitely think we *should* have a redecl matcher - I just don't think we 
> should make users use it to answer common semantic questions.
> That leaves matcher authors remembering and reciting the rules about how 
> syntax aggregates over redecls to produce semantics. We'll make mistakes.

A, okay, I thought you were opposed to the notion entirely. I agree with 
you that we need to be very careful not to add undue user burdens for common 
semantic questions!

>>> Here's a hack: CXXMethodDecl has both `isStatic()` and `isInstance()`. I 
>>> agree that `isStatic` is a risky name, but I don't think `isInstance` is. 
>>> Can we just spell the matcher you want `not(isInstance())`?
>>
>> This could work, but I don't think it's particularly satisfying in 
>> situations that aren't binary.
>
> Oh, I totally agree. I just meant specifically for this case, as a way to 
> avoid the dilemma of ambiguous vs unwieldy.

Now that I understand better, +1 -- that would unblock the needs for this patch 
without having to solve the larger problem, which is great.

>> This could work, but I don't think it's particularly satisfying in 
>> situations that aren't binary. Like asking "does this have static storage 
>> duration" (as opposed to thread, automatic, or dynamic storage duration). 
>> And it turns out that the behavior there is opposite to asking about the 
>> storage class specifier: https://godbolt.org/z/hqserfxzs
>
> I'm not totally surprised by that, because "storage duration" sounds like a 
> semantic property to me, while "storage class" (specifier) is a syntactic 
> feature.
> But it's not a distinction I'd pick up on if I wasn't looking for it, and 
> different people are going to read things different ways.
>
> My favorite naming convention I've seen in the AST is e.g. `getBlob()` vs 
> `getBlobAsWritten()`.
> Maybe `getEffectiveBlob()` would be even clearer for the semantic version.
> But I'm not sure how much it's possible to realign matcher names around that 
> sort of thing if the AST names aren't going to change.

Yeah, there's definitely some tension between our desire for AST matchers to 
use the same terminology as used by the AST itself and picking names that are 
usable and clear for users not steeped in compiler internals lore. Personally, 
I am very comfortable with the idea of renaming some of the AST functionality 
within Clang in this area (at least regarding storage duration, linkage, etc) 
because this problem comes up outside of AST matchers as well. It's an NFC 
change and there will be some churn from it, but I think the churn is well 
motivated.


Repository:
  rG 

[PATCH] D115425: [clangd] Generate ConfigFragment/YAML/docs from one tablegen source

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

For the website, we use tags to specify the clangd version that the option was 
first supported in.
I'd suggest that we also add a `Version` field to the `Field` class.
This could also cause problem down the line if we ever wanted to remove a 
config option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115425

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

[PATCH] D116673: [Clang][NVPTX]Add NVPTX intrinsics and builtins for CUDA PTX cvt sm80 instructions

[Jack Kirk via Phabricator via cfe-commits]

JackAKirk created this revision.
JackAKirk added reviewers: Naghasan, tra, hiraditya, yaxunl.
JackAKirk added projects: clang, LLVM.
Herald added subscribers: asavonic, jholewinski.
JackAKirk requested review of this revision.
Herald added a subscriber: jdoerfert.

Adds NVPTX intrinsics and builtins for CUDA PTX cvt instructions for sm80 
architectures and above.  Requires ptx 7.0.

PTX ISA description of cvt instructions : 
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#data-movement-and-conversion-instructions-cvt

Signed-off-by: JackAKirk 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116673

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGen/builtins-nvptx.c
  llvm/include/llvm/IR/IntrinsicsNVVM.td
  llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXInstPrinter.cpp
  llvm/lib/Target/NVPTX/NVPTX.h
  llvm/lib/Target/NVPTX/NVPTXInstrInfo.td
  llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
  llvm/test/CodeGen/NVPTX/convert-sm80.ll

Index: llvm/test/CodeGen/NVPTX/convert-sm80.ll
===
--- /dev/null
+++ llvm/test/CodeGen/NVPTX/convert-sm80.ll
@@ -0,0 +1,136 @@
+; RUN: llc < %s -march=nvptx64 -mcpu=sm_80 -mattr=+ptx70 | FileCheck %s
+
+
+; CHECK-LABEL: cvt_rn_bf16x2_f32
+define i32 @cvt_rn_bf16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rn.bf16x2.f32
+  %val = call i32 @llvm.nvvm.ff2v2bf.rn(float %f1, float %f2);
+
+ret i32 %val
+}
+
+; CHECK-LABEL: cvt_rn_relu_bf16x2_f32
+define i32 @cvt_rn_relu_bf16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rn.relu.bf16x2.f32
+%val = call i32 @llvm.nvvm.ff2v2bf.rn.relu(float %f1, float %f2);
+
+ret i32 %val
+}
+
+; CHECK-LABEL: cvt_rz_bf16x2_f32
+define i32 @cvt_rz_bf16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rz.bf16x2.f32
+  %val = call i32 @llvm.nvvm.ff2v2bf.rz(float %f1, float %f2);
+
+ret i32 %val
+}
+
+; CHECK-LABEL: cvt_rz_relu_bf16x2_f32
+define i32 @cvt_rz_relu_bf16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rz.relu.bf16x2.f32
+%val = call i32 @llvm.nvvm.ff2v2bf.rz.relu(float %f1, float %f2);
+
+ret i32 %val
+}
+
+declare i32 @llvm.nvvm.ff2v2bf.rn(float, float)
+declare i32 @llvm.nvvm.ff2v2bf.rn.relu(float, float) 
+declare i32 @llvm.nvvm.ff2v2bf.rz(float, float) 
+declare i32 @llvm.nvvm.ff2v2bf.rz.relu(float, float)
+
+; CHECK-LABEL: cvt_rn_f16x2_f32
+define <2 x half> @cvt_rn_f16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rn.f16x2.f32
+  %val = call <2 x half> @llvm.nvvm.ff2v2f16.rn(float %f1, float %f2);
+
+ret <2 x half> %val
+}
+
+; CHECK-LABEL: cvt_rn_relu_f16x2_f32
+define <2 x half> @cvt_rn_relu_f16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rn.relu.f16x2.f32
+%val = call <2 x half> @llvm.nvvm.ff2v2f16.rn.relu(float %f1, float %f2);
+
+ret <2 x half> %val
+}
+
+; CHECK-LABEL: cvt_rz_f16x2_f32
+define <2 x half> @cvt_rz_f16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rz.f16x2.f32
+  %val = call <2 x half> @llvm.nvvm.ff2v2f16.rz(float %f1, float %f2);
+
+ret <2 x half> %val
+}
+
+; CHECK-LABEL: cvt_rz_relu_f16x2_f32
+define <2 x half> @cvt_rz_relu_f16x2_f32(float %f1, float %f2) {
+
+; CHECK: cvt.rz.relu.f16x2.f32
+%val = call <2 x half> @llvm.nvvm.ff2v2f16.rz.relu(float %f1, float %f2);
+
+ret <2 x half> %val
+}
+
+declare <2 x half> @llvm.nvvm.ff2v2f16.rn(float, float)
+declare <2 x half> @llvm.nvvm.ff2v2f16.rn.relu(float, float)
+declare <2 x half> @llvm.nvvm.ff2v2f16.rz(float, float)
+declare <2 x half> @llvm.nvvm.ff2v2f16.rz.relu(float, float)
+
+; CHECK-LABEL: cvt_rn_bf16_f32
+define i16 @cvt_rn_bf16_f32(float %f1) {
+
+; CHECK: cvt.rn.bf16.f32
+  %val = call i16 @llvm.nvvm.f2bf.rn(float %f1);
+
+ret i16 %val
+}
+
+; CHECK-LABEL: cvt_rn_relu_bf16_f32
+define i16 @cvt_rn_relu_bf16_f32(float %f1) {
+
+; CHECK: cvt.rn.relu.bf16.f32
+%val = call i16 @llvm.nvvm.f2bf.rn.relu(float %f1);
+
+ret i16 %val
+}
+
+; CHECK-LABEL: cvt_rz_bf16_f32
+define i16 @cvt_rz_bf16_f32(float %f1) {
+
+; CHECK: cvt.rz.bf16.f32
+  %val = call i16 @llvm.nvvm.f2bf.rz(float %f1);
+
+ret i16 %val
+}
+
+; CHECK-LABEL: cvt_rz_relu_bf16_f32
+define i16 @cvt_rz_relu_bf16_f32(float %f1) {
+
+; CHECK: cvt.rz.relu.bf16.f32
+%val = call i16 @llvm.nvvm.f2bf.rz.relu(float %f1);
+
+ret i16 %val
+}
+
+declare i16 @llvm.nvvm.f2bf.rn(float)
+declare i16 @llvm.nvvm.f2bf.rn.relu(float) 
+declare i16 @llvm.nvvm.f2bf.rz(float) 
+declare i16 @llvm.nvvm.f2bf.rz.relu(float)
+
+; CHECK-LABEL: cvt_rna_tf32_f32
+define i32 @cvt_rna_tf32_f32(float %f1) {
+
+; CHECK: cvt.rna.tf32.f32
+  %val = call i32 @llvm.nvvm.f2tf.rna(float %f1);
+
+ret i32 %val
+}
+
+declare i32 @llvm.nvvm.f2tf.rna(float)
Index: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
===
--- llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
+++ llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
@@ -1046,6 +1046,38 @@
 def : Pat<(int_nvvm_ui2f_rp Int32Regs:$a),
   (CVT_f32_u32 Int32Regs:$a, CvtRP)>;
 
+def : Pat<(int_nvvm_ff2v2bf_rn Float32Regs:$a, Float32Regs:$b),
+ 

[clang] 7df2371 - Add codegen for allocate directive's 'align' clause

[Aaron Ballman via cfe-commits]

Author: David Pagan
Date: 2022-01-05T12:40:58-05:00
New Revision: 7df2371bc6518a63bdbe5f3c44bd064940808e35

URL: 
https://github.com/llvm/llvm-project/commit/7df2371bc6518a63bdbe5f3c44bd064940808e35
DIFF: 
https://github.com/llvm/llvm-project/commit/7df2371bc6518a63bdbe5f3c44bd064940808e35.diff

LOG: Add codegen for allocate directive's 'align' clause

Added: 
clang/test/OpenMP/align_clause_codegen.cpp

Modified: 
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/test/OpenMP/allocate_codegen.cpp
clang/test/OpenMP/allocate_codegen_attr.cpp
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Removed: 




diff  --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp 
b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 40e2094ea4ce7..9c93181b0395d 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3466,8 +3466,7 @@ static bool isAllocatableDecl(const VarDecl *VD) {
 return false;
   const auto *AA = CVD->getAttr();
   // Use the default allocation.
-  return !((AA->getAllocatorType() == OMPAllocateDeclAttr::OMPDefaultMemAlloc 
||
-AA->getAllocatorType() == OMPAllocateDeclAttr::OMPNullMemAlloc) &&
+  return !(AA->getAllocatorType() == OMPAllocateDeclAttr::OMPDefaultMemAlloc &&
!AA->getAllocator());
 }
 
@@ -12217,6 +12216,26 @@ Address 
CGOpenMPRuntime::getParameterAddress(CodeGenFunction ,
   return CGF.GetAddrOfLocalVar(NativeParam);
 }
 
+/// Return allocator value from expression, or return a null allocator (default
+/// when no allocator specified).
+static llvm::Value *getAllocatorVal(CodeGenFunction ,
+const Expr *Allocator) {
+  llvm::Value *AllocVal;
+  if (Allocator) {
+AllocVal = CGF.EmitScalarExpr(Allocator);
+// According to the standard, the original allocator type is a enum
+// (integer). Convert to pointer type, if required.
+AllocVal = CGF.EmitScalarConversion(AllocVal, Allocator->getType(),
+CGF.getContext().VoidPtrTy,
+Allocator->getExprLoc());
+  } else {
+// If no allocator specified, it defaults to the null allocator.
+AllocVal = llvm::Constant::getNullValue(
+CGF.CGM.getTypes().ConvertType(CGF.getContext().VoidPtrTy));
+  }
+  return AllocVal;
+}
+
 Address CGOpenMPRuntime::getAddressOfLocalVariable(CodeGenFunction ,
const VarDecl *VD) {
   if (!VD)
@@ -12253,20 +12272,24 @@ Address 
CGOpenMPRuntime::getAddressOfLocalVariable(CodeGenFunction ,
 }
 llvm::Value *ThreadID = getThreadID(CGF, CVD->getBeginLoc());
 const auto *AA = CVD->getAttr();
-assert(AA->getAllocator() &&
-   "Expected allocator expression for non-default allocator.");
-llvm::Value *Allocator = CGF.EmitScalarExpr(AA->getAllocator());
-// According to the standard, the original allocator type is a enum
-// (integer). Convert to pointer type, if required.
-Allocator = CGF.EmitScalarConversion(
-Allocator, AA->getAllocator()->getType(), CGF.getContext().VoidPtrTy,
-AA->getAllocator()->getExprLoc());
-llvm::Value *Args[] = {ThreadID, Size, Allocator};
-
-llvm::Value *Addr =
-CGF.EmitRuntimeCall(OMPBuilder.getOrCreateRuntimeFunction(
-CGM.getModule(), OMPRTL___kmpc_alloc),
-Args, getName({CVD->getName(), ".void.addr"}));
+const Expr *Allocator = AA->getAllocator();
+llvm::Value *AllocVal = getAllocatorVal(CGF, Allocator);
+llvm::Value *Alignment =
+AA->getAlignment()
+? CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(AA->getAlignment()),
+CGM.SizeTy, /*isSigned=*/false)
+: nullptr;
+SmallVector Args;
+Args.push_back(ThreadID);
+if (Alignment)
+  Args.push_back(Alignment);
+Args.push_back(Size);
+Args.push_back(AllocVal);
+llvm::omp::RuntimeFunction FnID =
+Alignment ? OMPRTL___kmpc_aligned_alloc : OMPRTL___kmpc_alloc;
+llvm::Value *Addr = CGF.EmitRuntimeCall(
+OMPBuilder.getOrCreateRuntimeFunction(CGM.getModule(), FnID), Args,
+getName({CVD->getName(), ".void.addr"}));
 llvm::FunctionCallee FiniRTLFn = OMPBuilder.getOrCreateRuntimeFunction(
 CGM.getModule(), OMPRTL___kmpc_free);
 QualType Ty = CGM.getContext().getPointerType(CVD->getType());
@@ -12280,14 +12303,14 @@ Address 
CGOpenMPRuntime::getAddressOfLocalVariable(CodeGenFunction ,
   llvm::FunctionCallee RTLFn;
   SourceLocation::UIntTy LocEncoding;
   Address Addr;
-  const Expr *Allocator;
+  const Expr *AllocExpr;
 
 public:
   OMPAllocateCleanupTy(llvm::FunctionCallee RTLFn,
SourceLocation::UIntTy LocEncoding, Address Addr,
-   const Expr *Allocator)
+  

[PATCH] D116670: [ASan] Driver changes to always link-in asan_static library.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov created this revision.
kstoimenov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This enables the changes from D116182 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116670

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/sanitizer-ld.c


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -22,7 +22,7 @@
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-NO-LINK-RUNTIME-LINUX %s
 //
-// CHECK-ASAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.asan
+// CHECK-ASAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.asan-x86_64
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target i386-unknown-linux -fuse-ld=ld -fsanitize=address 
-shared-libsan \
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -826,6 +826,11 @@
   if (SanArgs.needsStatsRt() && SanArgs.linkRuntimes())
 StaticRuntimes.push_back("stats_client");
 
+  // Always link the static runtime regardless of DSO or executable.
+  if (SanArgs.needsAsanRt()) {
+HelperStaticRuntimes.push_back("asan_static");
+  }
+
   // Collect static runtimes.
   if (Args.hasArg(options::OPT_shared)) {
 // Don't link static runtimes into DSOs.


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -22,7 +22,7 @@
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-NO-LINK-RUNTIME-LINUX %s
 //
-// CHECK-ASAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.asan
+// CHECK-ASAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.asan-x86_64
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target i386-unknown-linux -fuse-ld=ld -fsanitize=address -shared-libsan \
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -826,6 +826,11 @@
   if (SanArgs.needsStatsRt() && SanArgs.linkRuntimes())
 StaticRuntimes.push_back("stats_client");
 
+  // Always link the static runtime regardless of DSO or executable.
+  if (SanArgs.needsAsanRt()) {
+HelperStaticRuntimes.push_back("asan_static");
+  }
+
   // Collect static runtimes.
   if (Args.hasArg(options::OPT_shared)) {
 // Don't link static runtimes into DSOs.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D116638: [clang-format] Fix ignoring JavaScriptWrapImport when ColumnWidth: 0

[Andrey Mishchenko via cfe-commits]

That's what happens when you hit the column limit, when there is a column
limit. But do we really want every one-symbol import to wrap to 3 lines
when `ColumnLimit: 0`? Slash to force the user to unwrap every import, even
20-symbol 300-column imports, to a single line?

On Wed, Jan 5, 2022 at 12:13 PM MyDeveloperDay via Phabricator <
revi...@reviews.llvm.org> wrote:

> MyDeveloperDay added a comment.
>
> > Make JavaScriptWrapImports: true *always* wrap imports to multiple
> lines. This will be noisy and ugly.
>
> Isn't this what `prettier` does when effectively the ColumnLimit is
> exceeded.
>
> i.e.
>
>   import { Controller, Get, Post, Req } from '@nestjs/common';
>
> becomes as I hit the 80 column mark
>
>   `
>   import {
> Controller,
> Get,
> Post,
> Req,
> Request,
> Param,
> Query,
> StreamableFile,
> Body,
>   } from '@nestjs/common';
>
> So if ColumnLimit is 0 it should wrap them shouldn't it if
> `JavaScriptWrapImports: true`
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D116638/new/
>
> https://reviews.llvm.org/D116638
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116182: [ASan] Moved optimized callbacks into a separate library.

[Kirill Stoimenov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG027ffb173a68: [ASan] Moved optimized callbacks into a 
separate library. (authored by kstoimenov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116182

Files:
  compiler-rt/lib/asan/CMakeLists.txt
  compiler-rt/lib/asan/asan_rtl_static.cpp
  compiler-rt/lib/asan/tests/CMakeLists.txt

Index: compiler-rt/lib/asan/tests/CMakeLists.txt
===
--- compiler-rt/lib/asan/tests/CMakeLists.txt
+++ compiler-rt/lib/asan/tests/CMakeLists.txt
@@ -261,6 +261,7 @@
   set(ASAN_TEST_RUNTIME_OBJECTS
 $
 $
+$
 $
 $
 $
@@ -286,6 +287,7 @@
 # Test w/o ASan instrumentation. Link it with ASan statically.
 add_executable(AsanNoinstTest # FIXME: .arch?
   $
+  $
   $
   $
   $
Index: compiler-rt/lib/asan/asan_rtl_static.cpp
===
--- /dev/null
+++ compiler-rt/lib/asan/asan_rtl_static.cpp
@@ -0,0 +1,15 @@
+//===-- asan_static_rtl.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
+//
+//===--===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Main file of the ASan run-time library.
+//===--===//
+
+// This file is empty for now. Main reason to have it is workaround for Windows
+// build, which complains because no files are part of the asan_static lib.
Index: compiler-rt/lib/asan/CMakeLists.txt
===
--- compiler-rt/lib/asan/CMakeLists.txt
+++ compiler-rt/lib/asan/CMakeLists.txt
@@ -34,7 +34,6 @@
 
 if (NOT WIN32 AND NOT APPLE)
   list(APPEND ASAN_SOURCES
-asan_rtl_x86_64.S
 asan_interceptors_vfork.S
 )
 endif()
@@ -43,6 +42,16 @@
   asan_new_delete.cpp
   )
 
+set(ASAN_STATIC_SOURCES
+  asan_rtl_static.cpp
+  )
+
+if (NOT WIN32 AND NOT APPLE)
+  list(APPEND ASAN_STATIC_SOURCES
+asan_rtl_x86_64.S
+  )
+endif()
+
 set(ASAN_PREINIT_SOURCES
   asan_preinit.cpp
   )
@@ -135,6 +144,12 @@
 ADDITIONAL_HEADERS ${ASAN_HEADERS}
 CFLAGS ${ASAN_CFLAGS}
 DEFS ${ASAN_COMMON_DEFINITIONS})
+  add_compiler_rt_object_libraries(RTAsan_static
+ARCHS ${ASAN_SUPPORTED_ARCH}
+SOURCES ${ASAN_STATIC_SOURCES}
+ADDITIONAL_HEADERS ${ASAN_HEADERS}
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS})
   add_compiler_rt_object_libraries(RTAsan_preinit
 ARCHS ${ASAN_SUPPORTED_ARCH}
 SOURCES ${ASAN_PREINIT_SOURCES}
@@ -176,6 +191,14 @@
 LINK_FLAGS ${WEAK_SYMBOL_LINK_FLAGS}
 DEFS ${ASAN_DYNAMIC_DEFINITIONS}
 PARENT_TARGET asan)
+
+  add_compiler_rt_runtime(clang_rt.asan_static
+STATIC
+ARCHS ${ASAN_SUPPORTED_ARCH}
+OBJECT_LIBS RTAsan_static
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS}
+PARENT_TARGET asan)
 else()
   # Build separate libraries for each target.
 
@@ -207,6 +230,14 @@
 DEFS ${ASAN_COMMON_DEFINITIONS}
 PARENT_TARGET asan)
 
+  add_compiler_rt_runtime(clang_rt.asan_static
+STATIC
+ARCHS ${ASAN_SUPPORTED_ARCH}
+OBJECT_LIBS RTAsan_static
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS}
+PARENT_TARGET asan)
+
   add_compiler_rt_runtime(clang_rt.asan-preinit
 STATIC
 ARCHS ${ASAN_SUPPORTED_ARCH}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116182: [ASan] Moved optimized callbacks into a separate library.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov updated this revision to Diff 397613.
kstoimenov added a comment.

Removed driver part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116182

Files:
  compiler-rt/lib/asan/CMakeLists.txt
  compiler-rt/lib/asan/asan_rtl_static.cpp
  compiler-rt/lib/asan/tests/CMakeLists.txt

Index: compiler-rt/lib/asan/tests/CMakeLists.txt
===
--- compiler-rt/lib/asan/tests/CMakeLists.txt
+++ compiler-rt/lib/asan/tests/CMakeLists.txt
@@ -261,6 +261,7 @@
   set(ASAN_TEST_RUNTIME_OBJECTS
 $
 $
+$
 $
 $
 $
@@ -286,6 +287,7 @@
 # Test w/o ASan instrumentation. Link it with ASan statically.
 add_executable(AsanNoinstTest # FIXME: .arch?
   $
+  $
   $
   $
   $
Index: compiler-rt/lib/asan/asan_rtl_static.cpp
===
--- /dev/null
+++ compiler-rt/lib/asan/asan_rtl_static.cpp
@@ -0,0 +1,15 @@
+//===-- asan_static_rtl.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
+//
+//===--===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Main file of the ASan run-time library.
+//===--===//
+
+// This file is empty for now. Main reason to have it is workaround for Windows
+// build, which complains because no files are part of the asan_static lib.
Index: compiler-rt/lib/asan/CMakeLists.txt
===
--- compiler-rt/lib/asan/CMakeLists.txt
+++ compiler-rt/lib/asan/CMakeLists.txt
@@ -34,7 +34,6 @@
 
 if (NOT WIN32 AND NOT APPLE)
   list(APPEND ASAN_SOURCES
-asan_rtl_x86_64.S
 asan_interceptors_vfork.S
 )
 endif()
@@ -43,6 +42,16 @@
   asan_new_delete.cpp
   )
 
+set(ASAN_STATIC_SOURCES
+  asan_rtl_static.cpp
+  )
+
+if (NOT WIN32 AND NOT APPLE)
+  list(APPEND ASAN_STATIC_SOURCES
+asan_rtl_x86_64.S
+  )
+endif()
+
 set(ASAN_PREINIT_SOURCES
   asan_preinit.cpp
   )
@@ -135,6 +144,12 @@
 ADDITIONAL_HEADERS ${ASAN_HEADERS}
 CFLAGS ${ASAN_CFLAGS}
 DEFS ${ASAN_COMMON_DEFINITIONS})
+  add_compiler_rt_object_libraries(RTAsan_static
+ARCHS ${ASAN_SUPPORTED_ARCH}
+SOURCES ${ASAN_STATIC_SOURCES}
+ADDITIONAL_HEADERS ${ASAN_HEADERS}
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS})
   add_compiler_rt_object_libraries(RTAsan_preinit
 ARCHS ${ASAN_SUPPORTED_ARCH}
 SOURCES ${ASAN_PREINIT_SOURCES}
@@ -176,6 +191,14 @@
 LINK_FLAGS ${WEAK_SYMBOL_LINK_FLAGS}
 DEFS ${ASAN_DYNAMIC_DEFINITIONS}
 PARENT_TARGET asan)
+
+  add_compiler_rt_runtime(clang_rt.asan_static
+STATIC
+ARCHS ${ASAN_SUPPORTED_ARCH}
+OBJECT_LIBS RTAsan_static
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS}
+PARENT_TARGET asan)
 else()
   # Build separate libraries for each target.
 
@@ -207,6 +230,14 @@
 DEFS ${ASAN_COMMON_DEFINITIONS}
 PARENT_TARGET asan)
 
+  add_compiler_rt_runtime(clang_rt.asan_static
+STATIC
+ARCHS ${ASAN_SUPPORTED_ARCH}
+OBJECT_LIBS RTAsan_static
+CFLAGS ${ASAN_CFLAGS}
+DEFS ${ASAN_COMMON_DEFINITIONS}
+PARENT_TARGET asan)
+
   add_compiler_rt_runtime(clang_rt.asan-preinit
 STATIC
 ARCHS ${ASAN_SUPPORTED_ARCH}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116596: [clang][dataflow] Add transfer functions for assignment

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

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:36
+/// FIXME: Consider replacing this with a model that is more aligned with C++
+/// value categories.
+enum class SkipPast {

ymandel wrote:
> I'm not sure that value categories are right here.  The key is that skipping 
> is optional, unlike value categories which are fixed.  I think the problem is 
> just that C++ references are weird (for lack of a technical term). A 
> reference variable exists simultaneously in two different categories -- a 
> pointer and the value it points to. That's what this is trying to capture.
> 
> So, I'd recommend just changing the name to reflect that. WDYT of something 
> like this?
> 
> ```
> /// Indicates how to treat references, if encountered -- as a reference or 
> the value referred to -- when retrieving
> /// storage locations or values.
> enum class RefTreatment {
>/// Consider the reference itself.
>   AsSelf,
>   /// Consider the referent.
>   AsReferent,
> };
> ```
Yeah, value categories are indeed fixed, I was thinking along the lines of 
treating expressions as if there was an `LValueToRValue` or `RValueToLValue` 
conversion. I think some of this problem might come from the ambiguity. E.g., 
when I want to query the analysis state about the value of `*p`, what do I 
want? Do I want the value for `*p` as an lvalue or `*p` as an rvalue? I would 
get a different answer in both cases. If I see `*p` in the source code it is 
fixed but when I want to query the analysis state it is not.

On the other hand I see that `SkipPast` is used in the transfer functions, and 
I am wondering if that is justified. According to the language rules, I can 
never observe the actual value of the reference. I can modify the pointee, 
(`ref = val`) or create a pointer to the same value (``), but I cannot 
actually read or modify what is inside the reference. So I wonder if it might 
be less error prone if there would be no way in the transfer functions to 
observe the values of the references either. 

But I do not have a strong feeling about any of this at this point so feel free 
to leave everything as it is. 

> So, I'd recommend just changing the name to reflect that.

I am not sure how clear `Referent` is. For a non-native speaker `Pointee` might 
be clearer, although I do see how it can be confusing. 



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:95
+StorageLocation ::createStorageLocation(const Expr ) {
+  // Evaluated expressions are always assigned the same storage locations to
+  // ensure that the environment stabilizes across loop iterations. Storage

sgatev wrote:
> xazax.hun wrote:
> > What is the model for expressions that evaluate to different locations in 
> > every loop iterations, e.g.:
> > ```
> > for(int *p = array; p != array + size; ++p)
> >   *p; // The dereference expression
> > ```
> > 
> > Here, having `*p` always evaluate to the same location is not correct, we'd 
> > probably need a way to widen this. 
> In the current model, the storage location for `p` is stable and the value 
> that is assigned to it changes. So, in one iteration the value assigned to 
> the storage location for `p` is `val1` which is a `PointerValue` that points 
> to `loc1` and in another iteration the value assigned to the storage location 
> for `p` is `val2` which is a `PointerValue` that points to `loc2`. The 
> storage location for `*p` is also stable and the value that is assigned to it 
> is a `ReferenceValue` that points to either `loc1` or `loc2`. I implemented 
> this and added a test.
In this case, does it really make sense to materialize this mapping? If every 
subexpression corresponds to a unique location, and always the same location, 
we could use the subexpression directly as a location. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116596

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