[PATCH] D136259: Fix crash in constraining partial specialization on nested template.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136259

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

[PATCH] D136282: [clang] [CMake] Link libclangBasic against libatomic when necessary.

[Arfrever Frehtes Taifersar Arahesis via Phabricator via cfe-commits]

Arfrever added inline comments.



Comment at: clang/lib/Basic/CMakeLists.txt:114-117
+target_link_libraries(clangBasic
+  PRIVATE
+  ${LLVM_ATOMIC_LIB}
+)

mgorny wrote:
> Is this the right place? Grepping for `std::atomic`, I see lib/Frontend and a 
> bunch of tools, plus a few places in clang-tools-extra.
`clang-15.0.2:20221005-132622.log` (from https://bugs.gentoo.org/874024) says 
that this is the place where linking is needed:
```
/usr/bin/powerpc-unknown-linux-gnu-ld: lib/libclangBasic.a(FileManager.cpp.o): 
undefined reference to symbol '__atomic_load_8@@LIBATOMIC_1.0'
/usr/bin/powerpc-unknown-linux-gnu-ld: 
/usr/lib/gcc/powerpc-unknown-linux-gnu/11.3.1/libatomic.so.1: error adding 
symbols: DSO missing from command line
```

There were no reportedly no other errors with missing linking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136282

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

[clang] 4c87e12 - Fix crash in constraining partial specialization on nested template.

[Utkarsh Saxena via cfe-commits]

Author: Utkarsh Saxena
Date: 2022-10-20T10:46:18+02:00
New Revision: 4c87e12484b39409f4d3c02e2c99042c67a76132

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

LOG: Fix crash in constraining partial specialization on nested template.

Fixes: https://github.com/llvm/llvm-project/issues/53354

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

Added: 
clang/test/SemaTemplate/concepts-GH53354.cpp

Modified: 
clang/lib/Sema/SemaTemplateDeduction.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaTemplateDeduction.cpp 
b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 0572a663561c7..a731ff674f882 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5803,8 +5803,8 @@ struct MarkUsedTemplateParameterVisitor :
   }
 
   bool TraverseTemplateName(TemplateName Template) {
-if (auto *TTP =
-dyn_cast(Template.getAsTemplateDecl()))
+if (auto *TTP = llvm::dyn_cast_or_null(
+Template.getAsTemplateDecl()))
   if (TTP->getDepth() == Depth)
 Used[TTP->getIndex()] = true;
 RecursiveASTVisitor::

diff  --git a/clang/test/SemaTemplate/concepts-GH53354.cpp 
b/clang/test/SemaTemplate/concepts-GH53354.cpp
new file mode 100644
index 0..4fdf8bdd712a4
--- /dev/null
+++ b/clang/test/SemaTemplate/concepts-GH53354.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+template  class>
+struct S
+{};
+
+template 
+concept C1 = requires
+{
+  typename S;
+};
+
+template 
+requires C1
+struct A {};
+
+template 
+requires C1 && true
+struct A {};



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

[PATCH] D136259: Fix crash in constraining partial specialization on nested template.

[Utkarsh Saxena 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 rG4c87e12484b3: Fix crash in constraining partial 
specialization on nested template. (authored by usaxena95).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136259

Files:
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/SemaTemplate/concepts-GH53354.cpp


Index: clang/test/SemaTemplate/concepts-GH53354.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/concepts-GH53354.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+template  class>
+struct S
+{};
+
+template 
+concept C1 = requires
+{
+  typename S;
+};
+
+template 
+requires C1
+struct A {};
+
+template 
+requires C1 && true
+struct A {};
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5803,8 +5803,8 @@
   }
 
   bool TraverseTemplateName(TemplateName Template) {
-if (auto *TTP =
-dyn_cast(Template.getAsTemplateDecl()))
+if (auto *TTP = llvm::dyn_cast_or_null(
+Template.getAsTemplateDecl()))
   if (TTP->getDepth() == Depth)
 Used[TTP->getIndex()] = true;
 RecursiveASTVisitor::


Index: clang/test/SemaTemplate/concepts-GH53354.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/concepts-GH53354.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+template  class>
+struct S
+{};
+
+template 
+concept C1 = requires
+{
+  typename S;
+};
+
+template 
+requires C1
+struct A {};
+
+template 
+requires C1 && true
+struct A {};
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5803,8 +5803,8 @@
   }
 
   bool TraverseTemplateName(TemplateName Template) {
-if (auto *TTP =
-dyn_cast(Template.getAsTemplateDecl()))
+if (auto *TTP = llvm::dyn_cast_or_null(
+Template.getAsTemplateDecl()))
   if (TTP->getDepth() == Depth)
 Used[TTP->getIndex()] = true;
 RecursiveASTVisitor::
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Chuanqi Xu via Phabricator via cfe-commits]

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

Use `-fc++-module-file-output` to address the comments.


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

https://reviews.llvm.org/D134267

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/create_module_cache.cpp
  clang/test/Modules/one-phase-compilation-named-modules.cppm

Index: clang/test/Modules/one-phase-compilation-named-modules.cppm
===
--- /dev/null
+++ clang/test/Modules/one-phase-compilation-named-modules.cppm
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/M.cppm -c -o - -fc++-modules-cache-path=%t/pcm.cache
+// RUN: %clang -std=c++20 %t/Use.cpp -fsyntax-only  -fprebuilt-module-path=%t/pcm.cache -Xclang -verify
+//
+// RUN: rm -f %t/M.pcm
+//
+// RUN: %clang -std=c++20 %t/MismatchedName.cppm -c -o - -fc++-module-file-output=%t/pcm.cache/M.pcm
+// RUN: %clang -std=c++20 %t/Use.cpp -fsyntax-only  -fprebuilt-module-path=%t/pcm.cache -Xclang -verify
+
+//--- M.cppm
+export module M;
+export int getValue();
+
+//--- MismatchedName.cppm
+export module M;
+export int getValue();
+
+//--- Use.cpp
+// expected-no-diagnostics
+import M;
+int Use() {
+return getValue();
+}
Index: clang/test/Driver/create_module_cache.cpp
===
--- /dev/null
+++ clang/test/Driver/create_module_cache.cpp
@@ -0,0 +1,31 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: not %clang -std=c++20 %t/M.cppm -fc++-modules-cache-path= -c -o %t/M.tmp.o 2>&1 | FileCheck %t/M.cppm --check-prefix=ERROR
+// RUN: %clang -std=c++20 %t/M.cppm -fc++-modules-cache-path=%t/abc -c -o -
+// RUN: ls %t | FileCheck %t/M.cppm --check-prefix=CHECK-AVAILABLE
+// RUN: %clang -std=c++20 %t/M.cppm -fc++-modules-cache-path=%t/abc -fno-implicit-modules -c -o -
+// RUN: ls %t | FileCheck %t/M.cppm --check-prefix=CHECK-AVAILABLE
+//
+// RUN: %clang -std=c++20 %t/Use.cpp -fc++-modules-cache-path=abc -### 2>&1 | FileCheck %t/Use.cpp
+// RUN: %clang -std=c++20 %t/Use.cpp -fc++-modules-cache-path=abc -fno-implicit-modules -### 2>&1 | FileCheck %t/Use.cpp
+//
+// Check that the compiler will generate M-Part.pcm correctly.
+// RUN: %clang -std=c++20 %t/Part.cppm -fc++-module-file-output=%t/M-Part.pcm -c -o -
+// RUN: ls %t | FileCheck %t/Part.cppm --check-prefix=CHECK-AVAILABLE
+
+//--- M.cppm
+export module M;
+
+// ERROR: unable to create default module cache path "": No such file or directory
+// CHECK-AVAILABLE: abc
+
+//--- Use.cpp
+import M;
+
+// CHECK: -fprebuilt-module-path=abc
+
+//--- Part.cppm
+export module M:Part;
+// CHECK-AVAILABLE: M-Part.pcm
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3750,6 +3750,7 @@
   std::string("-fprebuilt-module-path=") + A->getValue()));
   A->claim();
 }
+
 if (Args.hasFlag(options::OPT_fprebuilt_implicit_modules,
  options::OPT_fno_prebuilt_implicit_modules, false))
   CmdArgs.push_back("-fprebuilt-implicit-modules");
@@ -3759,6 +3760,18 @@
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
   }
 
+  // If we're in standard c++ modules, lookup in cache path automatically.
+  if (HaveStdCXXModules) {
+SmallString<128> Path;
+if (Arg *A = Args.getLastArg(options::OPT_fcxx_modules_cache_path))
+  Path = A->getValue();
+else
+  Driver::getDefaultModuleCachePath(Path);
+if (!Path.empty())
+  CmdArgs.push_back(Args.MakeArgString(
+  std::string("-fprebuilt-module-path=") + Path));
+  }
+
   // -fmodule-name specifies the module that is currently being built (or
   // used for header checking by -fmodule-maps).
   Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -5558,6 +5558,30 @@
 return "-";
   }
 
+  if (isa(JA) && JA.getType() == types::TY_ModuleFile) {
+if (Arg *A = C.getArgs().getLastArg(options::OPT_fcxx_module_file_output))
+  return C.addResultFile(A->getValue(),  &JA);
+
+SmallString<128> Path;
+if (Arg *A = C.getArgs().getLastArg(options::OPT_fcxx_modules_cache_path))
+  Path = A->getValue();
+else
+  Driver::getDefaultModuleCachePath(Path);
+
+std::error_code EC =
+llvm::sys::fs::create_directories(Path, /*IgnoreExisting =*/true);
+if (EC)
+  Diag(clang::diag::err_creating_default_module_cache_path)
+  << Path << EC.message();
+
+StringRef Name = l

[PATCH] D134637: clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[Nicolai Hähnle via Phabricator via cfe-commits]

nhaehnle added a comment.

Gentle ping :)

If there are no objections, I'd like to commit this in a few days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134637

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

[PATCH] D97993: [Driver] Suppress GCC detection under -B

[Malhar via Phabricator via cfe-commits]

malharJ added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

Hi,
Has anything been done to address this bug (related to COMPILER_PATH no longer 
being supported) ?
https://bugs.llvm.org/show_bug.cgi?id=52009


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97993

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

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Tom Eccles via Phabricator via cfe-commits]

tblah added inline comments.



Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,

vzakhari wrote:
> tblah wrote:
> > awarzynski wrote:
> > > What are these pragmas? Perhaps you can add a test that would include 
> > > them?
> > I copied this comment from clang. I believe the pragma is 
> > ```
> > #pragma clang fp contract(fast)
> > ```
> > 
> > See 
> > https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags
> > 
> > This patch only adds support for argument processing, so I can't test for 
> > the pragmas.
> I do not think we should blindly copy this from clang.  I believe 
> `-ffp-contract=on`  is there to do the contraction complying to the language 
> standard - but Fortran standard says nothing about contraction.  I am also 
> not aware about a Fortran compiler that supports directives related to 
> contraction, so `fast-honor-pragmas` does not seem to be applicable as well.  
> Basically, we end up with just `off` and `fast`.
> 
> Now, it may be reasonable to support the same `-ffp-contract=` arguments so 
> that users can apply the same options sets for C/C++ and Fortran 
> compilations.  If we want to do this, we need to map `on` and 
> `fast-honor-pragmas` to something, e.g. `fast`.  A driver warning (not an 
> error) may be useful to make the option's behavior clear when `on` and 
> `fast-honor-pragmas` are passed.
From the clang help text:
```
Form fused FP ops (e.g. FMAs):
fast (fuses across statements disregarding pragmas)
| on (only fuses in the same statement unless dictated by pragmas)
| off (never fuses)
Default is 'on'
```

So if we removed `on` and set the default to `off` we would no longer fuse 
within the same statement by default.

Classic-flang seems to support `on`, `off` and `fast`: 
https://github.com/flang-compiler/flang/blob/master/test/llvm_ir_correct/fma.f90



Comment at: flang/test/Driver/driver-help-hidden.f90:34
 ! CHECK-NEXT: Use  as character line width in fixed mode
+! CHECK-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs): fast (fuses 
across statements disregarding pragmas) | on (only fuses in the same statement 
unless dictated by pragmas) | off (never fuses) | fast-honor-pragmas (fuses 
across statements unless diectated by pragmas). Default is 'fast' for CUDA, 
'fast-honor-pragmas' for HIP, and 'on' otherwise.
 ! CHECK-NEXT: -ffree-formProcess source files in free form

vzakhari wrote:
> Is it easy to emit a different help message for Flang to say that there are 
> only two modes for Fortran?
@awarzynski tells me there is no way to do this short of having separate 
`Options.td` for flang and clang. Once we have settled on which arguments to 
support, I will update the shared help text to mention flang.


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

https://reviews.llvm.org/D136080

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

[PATCH] D136022: [clang] Add time profile for constant evaluation

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 469159.
Izaron added a comment.

Fix optionals with `value_or`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136022

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/Support/CMakeLists.txt
  clang/unittests/Support/TimeProfilerTest.cpp

Index: clang/unittests/Support/TimeProfilerTest.cpp
===
--- /dev/null
+++ clang/unittests/Support/TimeProfilerTest.cpp
@@ -0,0 +1,199 @@
+//===- unittests/Support/TimeProfilerTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/TimeProfiler.h"
+
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+// Should be called before testing.
+void setupProfiler() {
+  timeTraceProfilerInitialize(/*TimeTraceGranularity=*/0, "test");
+}
+
+// Should be called after `compileFromString()`.
+// Returns profiler's JSON dump.
+std::string teardownProfiler() {
+  SmallVector SmallVec;
+  raw_svector_ostream OS(SmallVec);
+  timeTraceProfilerWrite(OS);
+  timeTraceProfilerCleanup();
+  return OS.str().str();
+}
+
+// Returns true if code compiles successfully.
+// We only parse AST here. This is enough for constexpr evaluation.
+bool compileFromString(StringRef Code) {
+  CompilerInstance Compiler;
+  Compiler.createDiagnostics();
+
+  auto Invocation = std::make_shared();
+  Invocation->getPreprocessorOpts().addRemappedFile(
+  "test.cc", MemoryBuffer::getMemBuffer(Code).release());
+  const char *Args[] = {"-std=c++20", "test.cc"};
+  CompilerInvocation::CreateFromArgs(*Invocation, Args,
+ Compiler.getDiagnostics());
+  Compiler.setInvocation(std::move(Invocation));
+
+  class TestFrontendAction : public ASTFrontendAction {
+  private:
+std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+   StringRef InFile) override {
+  return std::make_unique();
+}
+  } Action;
+  return Compiler.ExecuteAction(Action);
+}
+
+// Returns pretty-printed trace graph.
+std::string buildTraceGraph(StringRef Json) {
+  struct EventRecord {
+int64_t TimestampBegin;
+int64_t TimestampEnd;
+StringRef Name;
+StringRef Detail;
+  };
+  std::vector Events;
+
+  // Parse `EventRecord`s from JSON dump.
+  Expected Root = json::parse(Json);
+  if (!Root)
+return "";
+  for (json::Value &TraceEventValue :
+   *Root->getAsObject()->getArray("traceEvents")) {
+json::Object *TraceEventObj = TraceEventValue.getAsObject();
+
+int64_t TimestampBegin = TraceEventObj->getInteger("ts").value_or(0);
+int64_t TimestampEnd =
+TimestampBegin + TraceEventObj->getInteger("dur").value_or(0);
+StringRef Name = TraceEventObj->getString("name").value_or("");
+StringRef Detail = "";
+if (json::Object *Args = TraceEventObj->getObject("args"))
+  Detail = Args->getString("detail").value_or("");
+
+// This is a "summary" event, like "Total PerformPendingInstantiations",
+// skip it
+if (TimestampBegin == 0)
+  continue;
+
+Events.emplace_back(
+EventRecord{TimestampBegin, TimestampEnd, Name, Detail});
+  }
+
+  // There can be nested events that are very fast, for example:
+  // {"name":"EvaluateAsBooleanCondition",... ,"ts":2380,"dur":1}
+  // {"name":"EvaluateAsRValue",... ,"ts":2380,"dur":1}
+  // Therefore we should reverse the events list, so that events that have
+  // started earlier are first in the list.
+  // Then do a stable sort, we need it for the trace graph.
+  std::reverse(Events.begin(), Events.end());
+  std::stable_sort(
+  Events.begin(), Events.end(), [](const auto &lhs, const auto &rhs) {
+return std::make_pair(lhs.TimestampBegin, -lhs.TimestampEnd) <
+   std::make_pair(rhs.TimestampBegin, -rhs.TimestampEnd);
+  });
+
+  std::stringstream Stream;
+  // Write a newline for better testing with multiline string literal.
+  Stream << "\n";
+
+  // Keep the current event stack.
+  std::stack EventStack;
+  for (const auto &Event : Events) {
+// Pop every event in the stack until meeting the parent event.
+while (!EventStack.empty()) {
+  bool InsideCurrentEvent =
+  Event.TimestampBegin >= EventStack.top()->TimestampBegin &&
+  Event.TimestampEnd <= EventStack.top()->TimestampEnd;
+  if (!InsideCurrentEvent)
+EventStack.pop();
+ 

[PATCH] D136336: [clang-format] Mark pragma region lines as StringLiterals

[Tobias Hieta via Phabricator via cfe-commits]

thieta created this revision.
thieta added reviewers: MyDeveloperDay, curdeius, owenpan.
Herald added a project: All.
thieta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In our code-base we auto-generate pragma regions the regions
look like method signatures like:

`#pragma region MYREGION(Foo: bar)`

The problem here was that the rest of the line after region
was not marked as stringliteral as in the case of pragma mark
so clang-format tried to change the formatting based on the
method signature.

Added test and mark it similar as pragma mark.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136336

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19968,6 +19968,11 @@
 
   EXPECT_EQ("#pragma region TEST(FOO : BAR)",
 format("#pragma region TEST(FOO : BAR)", Style));
+
+  verifyFormat("#pragma region TEST(FOO: NOSPACE)", Style);
+  EXPECT_EQ("#pragma region TEST(FOO: NOSPACE)",
+format("#pragma region TEST(FOO: NOSPACE)", Style));
+
 }
 
 TEST_F(FormatTest, OptimizeBreakPenaltyVsExcess) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1337,11 +1337,11 @@
 if (CurrentToken &&
 CurrentToken->isOneOf(Keywords.kw_mark, Keywords.kw_option,
   Keywords.kw_region)) {
-  bool IsMark = CurrentToken->is(Keywords.kw_mark);
+  bool IsMarkOrRegion = CurrentToken->isOneOf(Keywords.kw_mark, 
Keywords.kw_region);
   next();
   next(); // Consume first token (so we fix leading whitespace).
   while (CurrentToken) {
-if (IsMark || CurrentToken->Previous->is(TT_BinaryOperator))
+if (IsMarkOrRegion || CurrentToken->Previous->is(TT_BinaryOperator))
   CurrentToken->setType(TT_ImplicitStringLiteral);
 next();
   }


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19968,6 +19968,11 @@
 
   EXPECT_EQ("#pragma region TEST(FOO : BAR)",
 format("#pragma region TEST(FOO : BAR)", Style));
+
+  verifyFormat("#pragma region TEST(FOO: NOSPACE)", Style);
+  EXPECT_EQ("#pragma region TEST(FOO: NOSPACE)",
+format("#pragma region TEST(FOO: NOSPACE)", Style));
+
 }
 
 TEST_F(FormatTest, OptimizeBreakPenaltyVsExcess) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1337,11 +1337,11 @@
 if (CurrentToken &&
 CurrentToken->isOneOf(Keywords.kw_mark, Keywords.kw_option,
   Keywords.kw_region)) {
-  bool IsMark = CurrentToken->is(Keywords.kw_mark);
+  bool IsMarkOrRegion = CurrentToken->isOneOf(Keywords.kw_mark, Keywords.kw_region);
   next();
   next(); // Consume first token (so we fix leading whitespace).
   while (CurrentToken) {
-if (IsMark || CurrentToken->Previous->is(TT_BinaryOperator))
+if (IsMarkOrRegion || CurrentToken->Previous->is(TT_BinaryOperator))
   CurrentToken->setType(TT_ImplicitStringLiteral);
 next();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136337: [clang-format] Discard pre-processor statements in parseBracedList()

[Tobias Hieta via Phabricator via cfe-commits]

thieta created this revision.
thieta added reviewers: MyDeveloperDay, curdeius, owenpan.
Herald added a project: All.
thieta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We had some code that looked like this:

  int foo()
  {
#pragma region hello(foo)
#pragma endregion
  }
  
  foo* bar() {}

This was confusingly indented like:

  int foo()
{
  #pragma region...
}

After some investigation I noticed that this is because
`parseBracedList()` thought that this was a initializer list.

The check here: 
https://github.com/tru/llvm-project/blob/main/clang/lib/Format/UnwrappedLineParser.cpp#L709
will mark the code above as ProbablyBracedList and then it
will format it totally wrong depending on your style settings.

My initial fix was to change the check above, but it became
really complicated to keep both initializer lists and my code
working.

My approach here instead is to discard any line that starts with #
since that is a pre-processor statement we shouldn't really care
about it in this case.

This fix passes all the unittests and our internal code-base, so
I am fairly confident in it, but I am no clang-format expert.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136337

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19970,6 +19970,14 @@
 format("#pragma region TEST(FOO : BAR)", Style));
 }
 
+TEST_F(FormatTest, PragmaRegionShouldNotBeBraceInitializers) {
+  auto Style = getLLVMStyleWithColumns(0);
+  verifyFormat("CxxClass::CxxClass() {\n#pragma region test(hello)\n}", Style);
+  EXPECT_EQ("CxxClass::CxxClass()\n#pragma region TEST(bar: foo)\n{\n#pragma 
region TEST(foo: bar)\n#pragma endregion\n}\nCxxClass *Hello() {}",
+format("CxxClass::CxxClass()\n#pragma region TEST(bar: foo)\n{\n  
#pragma region TEST(foo: bar)\n  #pragma endregion\n}\nCxxClass* Hello()\n{}", 
Style));
+
+}
+
 TEST_F(FormatTest, OptimizeBreakPenaltyVsExcess) {
   FormatStyle Style = getLLVMStyleWithColumns(20);
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -751,6 +751,11 @@
   if (!LBraceStack.empty() && LBraceStack.back()->is(BK_Unknown))
 LBraceStack.back()->setBlockKind(BK_Block);
   break;
+case tok::hash:
+  do {
+NextTok = Tokens->getNextToken();
+  } while (NextTok->isNot(tok::eof));
+  break;
 default:
   break;
 }


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19970,6 +19970,14 @@
 format("#pragma region TEST(FOO : BAR)", Style));
 }
 
+TEST_F(FormatTest, PragmaRegionShouldNotBeBraceInitializers) {
+  auto Style = getLLVMStyleWithColumns(0);
+  verifyFormat("CxxClass::CxxClass() {\n#pragma region test(hello)\n}", Style);
+  EXPECT_EQ("CxxClass::CxxClass()\n#pragma region TEST(bar: foo)\n{\n#pragma region TEST(foo: bar)\n#pragma endregion\n}\nCxxClass *Hello() {}",
+format("CxxClass::CxxClass()\n#pragma region TEST(bar: foo)\n{\n  #pragma region TEST(foo: bar)\n  #pragma endregion\n}\nCxxClass* Hello()\n{}", Style));
+
+}
+
 TEST_F(FormatTest, OptimizeBreakPenaltyVsExcess) {
   FormatStyle Style = getLLVMStyleWithColumns(20);
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -751,6 +751,11 @@
   if (!LBraceStack.empty() && LBraceStack.back()->is(BK_Unknown))
 LBraceStack.back()->setBlockKind(BK_Block);
   break;
+case tok::hash:
+  do {
+NextTok = Tokens->getNextToken();
+  } while (NextTok->isNot(tok::eof));
+  break;
 default:
   break;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 469166.
tbaeder added a comment.

Another (and even longer) version.

Introduce special opcodes for floating point operations and pass the rounding 
mode to them. Also create special int->float and float->int casts so we can 
handle that properly.

This makes `clang/tests/SemaCXX/rouding-math.cpp` work. I've only enabled one 
of the two RUN lines though. The other one fails because we don't handle 
`CXXNewExpr`s yet.


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

https://reviews.llvm.org/D134859

Files:
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Boolean.h
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.h
  clang/lib/AST/Interp/Context.cpp
  clang/lib/AST/Interp/Descriptor.cpp
  clang/lib/AST/Interp/Floating.cpp
  clang/lib/AST/Interp/Floating.h
  clang/lib/AST/Interp/Integral.h
  clang/lib/AST/Interp/Interp.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/InterpStack.h
  clang/lib/AST/Interp/InterpState.h
  clang/lib/AST/Interp/Opcodes.td
  clang/lib/AST/Interp/PrimType.h
  clang/lib/AST/Interp/Primitives.h
  clang/lib/AST/Interp/State.h
  clang/test/AST/Interp/literals.cpp
  clang/test/SemaCXX/rounding-math.cpp

Index: clang/test/SemaCXX/rounding-math.cpp
===
--- clang/test/SemaCXX/rounding-math.cpp
+++ clang/test/SemaCXX/rounding-math.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-linux -verify=norounding -Wno-unknown-pragmas %s
 // RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math -Wno-unknown-pragmas
+// RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math -fexperimental-new-constant-interpreter -Wno-unknown-pragmas
 // rounding-no-diagnostics
 
 #define fold(x) (__builtin_constant_p(x) ? (x) : (x))
Index: clang/test/AST/Interp/literals.cpp
===
--- clang/test/AST/Interp/literals.cpp
+++ clang/test/AST/Interp/literals.cpp
@@ -322,3 +322,42 @@
   // expected-error{{wide character literals may not contain multiple characters}}
 #pragma clang diagnostic pop
 };
+
+namespace floats {
+  constexpr int i = 2;
+  constexpr float f = 1.0f;
+  static_assert(f == 1.0f, "");
+
+  constexpr float f2 = 1u * f;
+  static_assert(f2 == 1.0f, "");
+
+  static_assert(1.0f + 3u == 4, "");
+  static_assert(4.0f / 1.0f == 4, "");
+  static_assert(10.0f * false == 0, "");
+
+  constexpr float floats[] = {1.0f, 2.0f, 3.0f, 4.0f};
+
+  constexpr float m = 5.0f / 0.0f; // ref-error {{must be initialized by a constant expression}} \
+   // ref-note {{division by zero}} \
+   // expected-error {{must be initialized by a constant expression}} \
+   // expected-note {{division by zero}}
+
+  static_assert(~2.0f == 3, ""); // ref-error {{invalid argument type 'float' to unary expression}} \
+ // expected-error {{invalid argument type 'float' to unary expression}}
+
+  /// Initialized by a double.
+  constexpr float df = 0.0;
+  /// The other way around.
+  constexpr double fd = 0.0f;
+
+  static_assert(0.0f == -0.0f, "");
+
+  const int k = 3 * (1.0f / 3.0f);
+  static_assert(k == 1, "");
+
+  constexpr bool b = 1.0;
+  static_assert(b, "");
+
+  constexpr double db = true;
+  static_assert(db == 1.0, "");
+};
Index: clang/lib/AST/Interp/State.h
===
--- clang/lib/AST/Interp/State.h
+++ clang/lib/AST/Interp/State.h
@@ -71,6 +71,7 @@
   virtual unsigned getCallStackDepth() = 0;
 
 public:
+  State() : InConstantContext(false) {}
   // Diagnose that the evaluation could not be folded (FF => FoldFailure)
   OptionalDiagnostic
   FFDiag(SourceLocation Loc,
@@ -118,6 +119,10 @@
 
   const LangOptions &getLangOpts() const;
 
+  /// Whether or not we're in a context where the front end requires a
+  /// constant value.
+  bool InConstantContext;
+
 private:
   void addCallStack(unsigned Limit);
 
Index: clang/lib/AST/Interp/Primitives.h
===
--- /dev/null
+++ clang/lib/AST/Interp/Primitives.h
@@ -0,0 +1,36 @@
+//===-- Primitives.h - Types for the constexpr VM ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Utilities and helper functions for all primitive types:
+//  - Integral
+//  - Floating
+//  - Boolean
+//
+//===--===//
+
+#ifndef LLVM_CLANG_AST_INTERP_PRIMITIVES_H
+#define LLVM_CLANG_AST_IN

[PATCH] D135060: [HLSL] Add groupshare address space.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/ParserHLSL/group_shared_202x.hlsl:5-6
+
+// expected-error@+1 {{return type cannot be qualified with address space}}
+auto l = []() -> groupshared void {};
+// expected-error@+1 {{expected a type}}

python3kgae wrote:
> aaron.ballman wrote:
> > One test I think we need in SemaCXX for this is what happens when the 
> > lambda return type (or function return type) is deduced. e.g.,
> > ```
> > extern groupshared float f;
> > 
> > auto func() {
> >   return f;
> > }
> > 
> > void other() {
> >   [&]() { return f; };
> > }
> > ```
> > Does use of `return f` cause us to deduce a return type that's annotated 
> > with `groupshared` or does that get stripped off thanks to lvalue to rvalue 
> > conversions and so we deduce `float`?
> groupshared is stripped because of lvalue to rvalue conversions.
> 
> And even hlsl202x not enable CXX14 :( .
Excellent, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060

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

[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 469173.

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

https://reviews.llvm.org/D134859

Files:
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Boolean.h
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.h
  clang/lib/AST/Interp/Context.cpp
  clang/lib/AST/Interp/Descriptor.cpp
  clang/lib/AST/Interp/Floating.cpp
  clang/lib/AST/Interp/Floating.h
  clang/lib/AST/Interp/Integral.h
  clang/lib/AST/Interp/Interp.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/InterpStack.h
  clang/lib/AST/Interp/InterpState.h
  clang/lib/AST/Interp/Opcodes.td
  clang/lib/AST/Interp/PrimType.h
  clang/lib/AST/Interp/Primitives.h
  clang/lib/AST/Interp/State.h
  clang/test/AST/Interp/literals.cpp
  clang/test/SemaCXX/rounding-math.cpp

Index: clang/test/SemaCXX/rounding-math.cpp
===
--- clang/test/SemaCXX/rounding-math.cpp
+++ clang/test/SemaCXX/rounding-math.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-linux -verify=norounding -Wno-unknown-pragmas %s
 // RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math -Wno-unknown-pragmas
+// RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math -fexperimental-new-constant-interpreter -Wno-unknown-pragmas
 // rounding-no-diagnostics
 
 #define fold(x) (__builtin_constant_p(x) ? (x) : (x))
Index: clang/test/AST/Interp/literals.cpp
===
--- clang/test/AST/Interp/literals.cpp
+++ clang/test/AST/Interp/literals.cpp
@@ -322,3 +322,42 @@
   // expected-error{{wide character literals may not contain multiple characters}}
 #pragma clang diagnostic pop
 };
+
+namespace floats {
+  constexpr int i = 2;
+  constexpr float f = 1.0f;
+  static_assert(f == 1.0f, "");
+
+  constexpr float f2 = 1u * f;
+  static_assert(f2 == 1.0f, "");
+
+  static_assert(1.0f + 3u == 4, "");
+  static_assert(4.0f / 1.0f == 4, "");
+  static_assert(10.0f * false == 0, "");
+
+  constexpr float floats[] = {1.0f, 2.0f, 3.0f, 4.0f};
+
+  constexpr float m = 5.0f / 0.0f; // ref-error {{must be initialized by a constant expression}} \
+   // ref-note {{division by zero}} \
+   // expected-error {{must be initialized by a constant expression}} \
+   // expected-note {{division by zero}}
+
+  static_assert(~2.0f == 3, ""); // ref-error {{invalid argument type 'float' to unary expression}} \
+ // expected-error {{invalid argument type 'float' to unary expression}}
+
+  /// Initialized by a double.
+  constexpr float df = 0.0;
+  /// The other way around.
+  constexpr double fd = 0.0f;
+
+  static_assert(0.0f == -0.0f, "");
+
+  const int k = 3 * (1.0f / 3.0f);
+  static_assert(k == 1, "");
+
+  constexpr bool b = 1.0;
+  static_assert(b, "");
+
+  constexpr double db = true;
+  static_assert(db == 1.0, "");
+};
Index: clang/lib/AST/Interp/State.h
===
--- clang/lib/AST/Interp/State.h
+++ clang/lib/AST/Interp/State.h
@@ -71,6 +71,7 @@
   virtual unsigned getCallStackDepth() = 0;
 
 public:
+  State() : InConstantContext(false) {}
   // Diagnose that the evaluation could not be folded (FF => FoldFailure)
   OptionalDiagnostic
   FFDiag(SourceLocation Loc,
@@ -118,6 +119,10 @@
 
   const LangOptions &getLangOpts() const;
 
+  /// Whether or not we're in a context where the front end requires a
+  /// constant value.
+  bool InConstantContext;
+
 private:
   void addCallStack(unsigned Limit);
 
Index: clang/lib/AST/Interp/Primitives.h
===
--- /dev/null
+++ clang/lib/AST/Interp/Primitives.h
@@ -0,0 +1,36 @@
+//===-- Primitives.h - Types for the constexpr VM ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Utilities and helper functions for all primitive types:
+//  - Integral
+//  - Floating
+//  - Boolean
+//
+//===--===//
+
+#ifndef LLVM_CLANG_AST_INTERP_PRIMITIVES_H
+#define LLVM_CLANG_AST_INTERP_PRIMITIVES_H
+
+#include "clang/AST/ComparisonCategories.h"
+
+namespace clang {
+namespace interp {
+
+/// Helper to compare two comparable types.
+template  ComparisonCategoryResult Compare(const T &X, const T &Y) {
+  if (X < Y)
+return ComparisonCategoryResult::Less;
+  if (X > Y)
+return ComparisonCategoryResult::Greater;
+  return ComparisonCategoryResult::Equal;
+}
+
+} // namespace interp
+

[clang] ea8aebf - [analyzer] Move unexecuted test block into it's own source file

[via cfe-commits]

Author: isuckatcs
Date: 2022-10-20T14:14:52+02:00
New Revision: ea8aebf9eb7f0762d357e02524be9f65cfdb4f58

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

LOG: [analyzer] Move unexecuted test block into it's own source file

Inside lambdas.cpp a block of code wasn't executed,
because it required the standard to be at least c++14.
This patch moves this block of code into it's own
source file and makes sure it's tested.

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

Added: 
clang/test/Analysis/lambdas-modern.cpp

Modified: 
clang/test/Analysis/lambdas.cpp

Removed: 




diff  --git a/clang/test/Analysis/lambdas-modern.cpp 
b/clang/test/Analysis/lambdas-modern.cpp
new file mode 100644
index 0..ce75acf087174
--- /dev/null
+++ b/clang/test/Analysis/lambdas-modern.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -std=c++14 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
inline-lambdas=true -verify %s
+// RUN: %clang_analyze_cc1 -std=c++17 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
inline-lambdas=true -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
+// Capture copy elided object.
+struct Elided{
+  int x = 14;
+  Elided(int) {}
+};
+
+void testCopyElidedObjectCaptured(int x) {
+  int r = [e = Elided(x)] {
+return e.x;
+  }();
+  
+  clang_analyzer_eval(r == 14); // expected-warning{{TRUE}}
+}
+
+static auto MakeUniquePtr() { return std::make_unique>(); }
+
+void testCopyElidedUniquePtr() {
+  [uniquePtr = MakeUniquePtr()] {}();
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}

diff  --git a/clang/test/Analysis/lambdas.cpp b/clang/test/Analysis/lambdas.cpp
index b1d9784dcc0bf..f86a6d472226e 100644
--- a/clang/test/Analysis/lambdas.cpp
+++ b/clang/test/Analysis/lambdas.cpp
@@ -205,29 +205,6 @@ void testVariableLengthArrayCaptured() {
   clang_analyzer_eval(i == 7); // expected-warning{{TRUE}}
 }
 
-#if __cplusplus >= 201402L
-// Capture copy elided object.
-
-struct Elided{
-  int x = 0;
-  Elided(int) {}
-};
-
-void testCopyElidedObjectCaptured(int x) {
-  [e = Elided(x)] {
-clang_analyzer_eval(e.x == 0); // expected-warning{{TRUE}}
-  }();
-}
-
-static auto MakeUniquePtr() { return std::make_unique>(); }
-
-void testCopyElidedUniquePtr() {
-  [uniquePtr = MakeUniquePtr()] {}();
-  clang_analyzer_warnIfReached(); // expected-warning{{TRUE}}
-}
-
-#endif
-
 // Test inline defensive checks
 int getNum();
 



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

[PATCH] D135965: [analyzer] Move unexecuted test block into it's own source file

[Domján Dániel via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGea8aebf9eb7f: [analyzer] Move unexecuted test block into 
it's own source file (authored by isuckatcs).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135965

Files:
  clang/test/Analysis/lambdas-modern.cpp
  clang/test/Analysis/lambdas.cpp


Index: clang/test/Analysis/lambdas.cpp
===
--- clang/test/Analysis/lambdas.cpp
+++ clang/test/Analysis/lambdas.cpp
@@ -205,29 +205,6 @@
   clang_analyzer_eval(i == 7); // expected-warning{{TRUE}}
 }
 
-#if __cplusplus >= 201402L
-// Capture copy elided object.
-
-struct Elided{
-  int x = 0;
-  Elided(int) {}
-};
-
-void testCopyElidedObjectCaptured(int x) {
-  [e = Elided(x)] {
-clang_analyzer_eval(e.x == 0); // expected-warning{{TRUE}}
-  }();
-}
-
-static auto MakeUniquePtr() { return std::make_unique>(); }
-
-void testCopyElidedUniquePtr() {
-  [uniquePtr = MakeUniquePtr()] {}();
-  clang_analyzer_warnIfReached(); // expected-warning{{TRUE}}
-}
-
-#endif
-
 // Test inline defensive checks
 int getNum();
 
Index: clang/test/Analysis/lambdas-modern.cpp
===
--- /dev/null
+++ clang/test/Analysis/lambdas-modern.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -std=c++14 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
inline-lambdas=true -verify %s
+// RUN: %clang_analyze_cc1 -std=c++17 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
inline-lambdas=true -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
+// Capture copy elided object.
+struct Elided{
+  int x = 14;
+  Elided(int) {}
+};
+
+void testCopyElidedObjectCaptured(int x) {
+  int r = [e = Elided(x)] {
+return e.x;
+  }();
+  
+  clang_analyzer_eval(r == 14); // expected-warning{{TRUE}}
+}
+
+static auto MakeUniquePtr() { return std::make_unique>(); }
+
+void testCopyElidedUniquePtr() {
+  [uniquePtr = MakeUniquePtr()] {}();
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}


Index: clang/test/Analysis/lambdas.cpp
===
--- clang/test/Analysis/lambdas.cpp
+++ clang/test/Analysis/lambdas.cpp
@@ -205,29 +205,6 @@
   clang_analyzer_eval(i == 7); // expected-warning{{TRUE}}
 }
 
-#if __cplusplus >= 201402L
-// Capture copy elided object.
-
-struct Elided{
-  int x = 0;
-  Elided(int) {}
-};
-
-void testCopyElidedObjectCaptured(int x) {
-  [e = Elided(x)] {
-clang_analyzer_eval(e.x == 0); // expected-warning{{TRUE}}
-  }();
-}
-
-static auto MakeUniquePtr() { return std::make_unique>(); }
-
-void testCopyElidedUniquePtr() {
-  [uniquePtr = MakeUniquePtr()] {}();
-  clang_analyzer_warnIfReached(); // expected-warning{{TRUE}}
-}
-
-#endif
-
 // Test inline defensive checks
 int getNum();
 
Index: clang/test/Analysis/lambdas-modern.cpp
===
--- /dev/null
+++ clang/test/Analysis/lambdas-modern.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -std=c++14 -analyzer-checker=core,debug.ExprInspection -analyzer-config inline-lambdas=true -verify %s
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,debug.ExprInspection -analyzer-config inline-lambdas=true -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
+// Capture copy elided object.
+struct Elided{
+  int x = 14;
+  Elided(int) {}
+};
+
+void testCopyElidedObjectCaptured(int x) {
+  int r = [e = Elided(x)] {
+return e.x;
+  }();
+  
+  clang_analyzer_eval(r == 14); // expected-warning{{TRUE}}
+}
+
+static auto MakeUniquePtr() { return std::make_unique>(); }
+
+void testCopyElidedUniquePtr() {
+  [uniquePtr = MakeUniquePtr()] {}();
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109460: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Louis Dionne via Phabricator via cfe-commits]

ldionne abandoned this revision.
ldionne added a comment.

Abandoning in favor of D136315 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109460

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Thanks for picking this up! I looked at my local changes and I had started 
modifying `inferDeploymentTargetFromSDK`. I had left the comment:

  /// TODO: We should only infer it if the SDK was provided with SDKROOT or 
-isysroot.
  ///   If we inferred the SDK with xcrun, it doesn't make sense to infer 
the
  ///   deployment target because 

I don't remember what I was thinking at the time, but I encourage you to pause 
for a minute and think about it. I can't think of the reason anymore. If you 
cant' either, maybe just disregard my comment entirely.




Comment at: clang/test/Driver/darwin-sdkroot.c:43-44
 
+// Check that we default to running `xcrun --show-sdk-path` if there is no
+// SDKROOT defined in the environment.
+//

You should test with multiple target triples if that's possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134453#3869610 , @dblaikie wrote:

> In D134453#3869201 , @aaron.ballman 
> wrote:
>
>> In D134453#3868821 , @DoDoENT 
>> wrote:
>>
>>> In D134453#3868789 , @dblaikie 
>>> wrote:
>>>
 I still don't think "keep full NTTP type printing behind a policy, for 
 those that want/need that" is a policy we should add. String printed names 
 aren't meant to be a tool for type reflection - the AST can be queried for 
 that information.
>>>
>>> I agree on that matter.
>>>
>>> However, I'm building my clang with this policy enabled by default to 
>>> provide my developers with GCC/MSVC-like verbosity in diagnostic messages. 
>>> They prefer it that way.
>>
>> @dblaikie -- but I thought we agreed that we would always print the full 
>> type, so the policy in this case is to print information *less* suitable for 
>> type reflection, right?
>
> I'm confused - I'll try to say some things that might lead to less confusion, 
> but I'm far from sure.
>
> (I'm confused by the question/statement above by itself - "agreed we would 
> always print the full type" + "print information *less* suitable for type 
> reflection". Are those meant to go together? I think printing the "full" type 
> (that might be ambiguous/unclear what full means in this conversation, but 
> best guess) would make something more suitable for type reflection? (type 
> reflection in the sense of "I can look at/analyze/split up the string and see 
> type names that may be useful for some tooling purpose"))
>
> My understanding is that there are 3 possible ways of printing under 
> discussion (all demonstrated here: https://godbolt.org/z/1s8Mf6b8K ):
>
>   struct t1 { int v1; };
>   struct t2 { t1 v1; };
>   template
>   struct t3 { };
>
> 1. `t3`: Fully explicit - all types provided. Valid code, though 
> not strictly necessary.
> 2. `t3`: Semi-explicit. Necessary for validity, even if the NTTP is 
> not `auto`, but instead explicitly `t2` (C++ won't deduce it - requiring it 
> to be explicit).
> 3. `t3<{{3}}>`: Not actually valid code (even if the NTTP is `t2`)
>
> Clang produces (3) in some cases, and (2) in others.
> MSVC and GCC always use (1).
>
> When you say "always print the full type" - it sounds like (1)? But I 
> thought/my preference has been that we should always use (2) because (1) can 
> be quite verbose.

Ah, and I thought/my preference was that we should always use (1) because (2) 
requires the reader to infer information that may not be obvious from local 
context.

> The patch I think currently makes Clang use (2) more consistently, and 
> provides a PrintingPolicy flag to do (1).
>
> I think the fix to use (2) more consistently is good, but I don't think we 
> should add functionality for (1).

I was thinking we should use (1) consistently and not add the functionality for 
(2). Huttah, we found the confusion! :-D

>> However, given that we don't think we'll use that policy in the tree, can 
>> that policy can be kept in your downstream instead @DoDoENT?
>
> That would tend to be my conclusion/preference at this point.
>
> But open to your perspective/preferences/etc, @aaron.ballman

I got my perspective, perhaps incorrectly, from this bit of the conversation:

In D134453#3834659 , @dblaikie wrote:

> Ok, so sounds like we're on the same page that `t1<{}>` is a diagnostic 
> quality bug and we'd love to see a fix for it to include the top level type 
> of the NTTP, as in `t1`, and that shouldn't need a new printing policy 
> - because we never want to print `t1<{}>` and anywhere we do is a bug to be 
> fixed.

I think my confusion came from `<{}>` -- I was interpreting that as meaning any 
braces without a type preceding them, which `t2{}` would resolve. I think you 
intended me to read that as empty braces without a preceding type, specifically.

I can sympathize with the argument that brevity can be good thing, but I think 
that biases towards expert users over novice users of the language in terms of 
making the information understandable, at least in some small way. I tend to 
come down on the side of thinking verbosity is better than terseness for 
non-expert users because I think it's easier for humans to ignore information 
they think is irrelevant compared to correctly inferring information where it's 
missing, but I can also see the argument that you can overwhelm people with too 
much information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D136343: [Lex] Add compatibility with MSVC

[Qfrost via Phabricator via cfe-commits]

Qfrost911 created this revision.
Qfrost911 added a reviewer: shafik.
Herald added a project: All.
Qfrost911 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The code below is valid in MSVC, but it is not allowed in clang

  printf(__FUNCTION__ "Hello World");

This patch fixes the compatibility with MSVC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136343

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/PPMacroExpansion.cpp


Index: clang/lib/Lex/PPMacroExpansion.cpp
===
--- clang/lib/Lex/PPMacroExpansion.cpp
+++ clang/lib/Lex/PPMacroExpansion.cpp
@@ -337,6 +337,11 @@
 /// RegisterBuiltinMacros - Register builtin macros, such as __LINE__ with the
 /// identifier table.
 void Preprocessor::RegisterBuiltinMacros() {
+#ifdef _WIN32
+  Ident__FUNCTION__ = RegisterBuiltinMacro(*this, "__FUNCTION__");
+#else
+  Ident__FUNCTION__ = Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__");
+#endif
   Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__");
   Ident__FILE__ = RegisterBuiltinMacro(*this, "__FILE__");
   Ident__DATE__ = RegisterBuiltinMacro(*this, "__DATE__");
@@ -1561,6 +1566,13 @@
   FN += PLoc.getFilename();
   } else {
 FN += PLoc.getFilename();
+#ifdef _WIN32
+if (II == Ident__FUNCTION__) {
+  FN += "(";
+  FN += Twine(PLoc.getLine()).str();
+  FN += ") ";
+}
+#endif
   }
   processPathForFileMacro(FN, getLangOpts(), getTargetInfo());
   Lexer::Stringify(FN);
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -150,7 +150,7 @@
   llvm::BumpPtrAllocator BP;
 
   /// Identifiers for builtin macros and other builtins.
-  IdentifierInfo *Ident__LINE__, *Ident__FILE__;   // __LINE__, __FILE__
+  IdentifierInfo *Ident__LINE__, *Ident__FILE__, *Ident__FUNCTION__;   // 
__LINE__, __FILE__, __FUNCTION__
   IdentifierInfo *Ident__DATE__, *Ident__TIME__;   // __DATE__, __TIME__
   IdentifierInfo *Ident__INCLUDE_LEVEL__;  // __INCLUDE_LEVEL__
   IdentifierInfo *Ident__BASE_FILE__;  // __BASE_FILE__


Index: clang/lib/Lex/PPMacroExpansion.cpp
===
--- clang/lib/Lex/PPMacroExpansion.cpp
+++ clang/lib/Lex/PPMacroExpansion.cpp
@@ -337,6 +337,11 @@
 /// RegisterBuiltinMacros - Register builtin macros, such as __LINE__ with the
 /// identifier table.
 void Preprocessor::RegisterBuiltinMacros() {
+#ifdef _WIN32
+  Ident__FUNCTION__ = RegisterBuiltinMacro(*this, "__FUNCTION__");
+#else
+  Ident__FUNCTION__ = Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__");
+#endif
   Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__");
   Ident__FILE__ = RegisterBuiltinMacro(*this, "__FILE__");
   Ident__DATE__ = RegisterBuiltinMacro(*this, "__DATE__");
@@ -1561,6 +1566,13 @@
   FN += PLoc.getFilename();
   } else {
 FN += PLoc.getFilename();
+#ifdef _WIN32
+if (II == Ident__FUNCTION__) {
+  FN += "(";
+  FN += Twine(PLoc.getLine()).str();
+  FN += ") ";
+}
+#endif
   }
   processPathForFileMacro(FN, getLangOpts(), getTargetInfo());
   Lexer::Stringify(FN);
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -150,7 +150,7 @@
   llvm::BumpPtrAllocator BP;
 
   /// Identifiers for builtin macros and other builtins.
-  IdentifierInfo *Ident__LINE__, *Ident__FILE__;   // __LINE__, __FILE__
+  IdentifierInfo *Ident__LINE__, *Ident__FILE__, *Ident__FUNCTION__;   // __LINE__, __FILE__, __FUNCTION__
   IdentifierInfo *Ident__DATE__, *Ident__TIME__;   // __DATE__, __TIME__
   IdentifierInfo *Ident__INCLUDE_LEVEL__;  // __INCLUDE_LEVEL__
   IdentifierInfo *Ident__BASE_FILE__;  // __BASE_FILE__
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134878: Update developer policy on potentially breaking changes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134878#3869947 , @mehdi_amini 
wrote:

> From what I saw when you posted the discourse thread initially, I understand 
> you're targeting user-visible features, and mostly from the "toolchain" side 
> of the project.
> However I find the language here to be potentially confusing for the API 
> surface of the libraries: that is how much of this applies to the LLVM C++ 
> libraries API surface?
> If this is out-of-scope, can this be called out more explicitly?

Sure! Would it help to add a paragraph before the bulleted list in `breaking` 
that says something along the lines of:

The C++ interfaces of individual library components of projects like LLVM or 
Clang are not intended to be stable interfaces. Potentially disruptive changes 
to such C++ interfaces do not constitute a breaking change unless the 
disruption is exposed via another mechanism such as a stable C API.

(Feel free to wordsmith it!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134878

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

[PATCH] D136293: [IncludeCleaner] Add public API

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.



Comment at: 
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:7
+//
+//===--===//
+

would be nice to have some high-level descriptions in the file comment.



Comment at: 
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:29
+  // FIXME: Add support for macros.
+  std::variant Storage;
+};

I see we use `Decl*` here and elsewhere (`WalkAST` etc), is there any reason of 
not using `const Decl*`?



Comment at: 
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:48
+/// FIXME: Provide signals about the reference type and providing headers.
+using UsedSymbolVisitor = llvm::function_ref Providers)>;

nit: rename the Visitor to CB? Visitor reminds me too much of `ASTVisitor`...



Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:60
 
-} // namespace include_cleaner
-} // namespace clang
+void walkUsed(llvm::ArrayRef ASTRoots, UsedSymbolVisitor CB) {
+  tooling::stdlib::Recognizer Recognizer;

sammccall wrote:
> IMO this function really deserves its own impl file I think (at least 
> Analysis.cpp) - I think it + walkAST are going to be the two biggest bundles 
> of complexity in the library. (if so, same with the tests)
+1



Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:71
+  // FIXME: Handle IWYU pragmas, non self-contained files.
+  llvm::errs() << "Returning fileentry\n";
+  if (auto *FE = SM.getFileEntryForID(SM.getFileID(ND.getLocation(

nit: remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136293

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

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

I've moved the tests to `literals.cpp`, is there anything else left?


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

https://reviews.llvm.org/D135366

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

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

@aaron.ballman Do you maybe have a different reproducer or know more about the 
expected behavior of a `ImplicitValueInitExpr` for array and record types?


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

https://reviews.llvm.org/D135013

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

[PATCH] D135750: [clang][Interp] Track initialization state of local variables

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135750

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

[PATCH] D135764: [clang][Interp] Implement for loops

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135764

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

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D135220#3862893 , @dexonsmith 
wrote:

> In D135220#3862058 , @hans wrote:
>
>> The build system folks replied saying they're not using symlinks, but hard 
>> links for compiler inputs. Dropping the `-s` flag in the repro above 
>> demonstrates this.
>
> I think this only happened to work before. If `/tmp/a.cc` changes to these 
> file contents:
>
>   int foo_table[] = {
> #include "/tmp/foo.h"
>   };
>   int bar_table[] = {
> #include "/tmp/bar.h"
>   };
>   int foo2_table[] = {
> #include "/tmp/foo.h"
>   };
>
> then I'm getting the following with my system clang:
>
>   # 1 "/tmp/a.cc"
>   # 1 "" 1
>   # 1 "" 3
>   # 414 "" 3
>   # 1 "" 1
>   # 1 "" 2
>   # 1 "/tmp/a.cc" 2
>int foo_table[] = {
>   # 1 "/tmp/foo.h" 1
>   1, 2, 3
>   # 3 "/tmp/a.cc" 2
>};
>int bar_table[] = {
>   # 1 "/tmp/bar.h" 1
>   1, 2, 3
>   # 6 "/tmp/a.cc" 2
>};
>int foo2_table[] = {
>   # 1 "/tmp/bar.h" 1
>   1, 2, 3
>   # 9 "/tmp/a.cc" 2
>};
>
> Note that `foo2_table` now points at `bar.h` but should point at `foo.h`. I 
> presume after the present patch it'll point at `foo.h` (although I admit I 
> didn't test).

Interesting, thanks!

> The root cause is the same, and matches @benlangmuir's analysis in response 
> to the testcase failure on Windows:
>
> - The FileManager is merging files based on their observed inode (regardless 
> of symlink or hardlink).
> - Previously, filenames were reported based on the most recent access path: 
> "last-ref".
> - Now, they're reported based on the original access path: "first-ref".
> - In neither case is it sound.

Agreed.

> Here are a few solutions I can see:
>
> 1. Change FileManager to only merge files with matching "realpath", seeing 
> through symlinks but not hardlinks (could use observed inode collisions as a 
> hint to do an `lstat` to avoid running an `lstat` every time). Then 
> SourceManager will give a different FileID to these files.
> 2. Change SourceManager to have different FileID to these files, even though 
> they were merged by FileManager.
> 3. Change the `#include` stack to track the accessed filename (the 
> `FileEntryRef`) in addition to the SLoc, even though they have the same 
> FileID.
> 4. Require clients to keep contents distinct if they it matters (the 
> (surprising) status quo).
>
> What's in tree (and has been for a long time) is (4). (1) seems like the 
> right solution to me.
>
> As a heuristic on top of (4), "last-ref" (before this patch) probably gets 
> the answer the user expects more often than "first-ref" (this patch) does, 
> which I assume is why it was originally implemented. However, it's 
> unpredictable and requires mutating a bunch of state where mutations are hard 
> to reason about.
>
> IMO, "first-ref" is easier to make sound, since it's an immutable property, 
> so this patch is an incremental improvement; it makes the fuzzy modeling 
> easier to observe but also perhaps more predictable. If we care about fixing 
> hardlink include names ("correct-ref") we should fix them in all cases 
> (ideally with (1)), but I wonder how urgent it is.
>
> @hans, WDYT?

I feel I don't have enough background here to say whether (1) would work. Why 
is FileManager merging files in the first place?

This is not urgent (at least not for us), but it's a pretty surprising behavior.

One thought, which I'm not sure is relevant, is that this is only observable 
for headers which are included more than once, which is rare because normally 
there are include guards (or pragma once). `isFileMultipleIncludeGuarded` isn't 
tracked by the FileManager, but otherwise maybe that could have been a useful 
heuristic: don't merge header files without include guards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135220

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

[PATCH] D136282: [clang] [CMake] Link libclangBasic against libatomic when necessary.

[Michał Górny via Phabricator via cfe-commits]

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

I suppose it comes indirectly then. I'd prefer that you tested it with git 
main, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136282

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

[PATCH] D135764: [clang][Interp] Implement for loops

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some minor points.




Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:361
+  this->emitLabel(IncLabel);
+  if (Inc && !this->discard(Inc))
+return false;

What does the call to `discard()` do? Evaluate the expression and discard the 
results? (If so, the API might want for a better name at some point, as it can 
also read like "do nothing with this expression".)



Comment at: clang/test/AST/Interp/loops.cpp:216-226
+  constexpr int f4() {
+int i = 0;
+for (;;) {
+  i = i + 1;
+
+  if (i == 5)
+break;

I'd love to see a stress test for the compile time performance of:
```
int func() {
  int i = 0;
  for (;;) {
if (i == __INT_MAX__)
  break;
++i;
  }
  return i;
}
static_assert(func() == __INT_MAX__);
```
I don't think there's value to testing specifically how long it takes for this 
to execute, but we should probably start thinking about how to validate 
performance characteristics pretty soon. Perhaps relative performance compared 
to the existing constant expression evaluator would be interesting though?



Comment at: clang/test/AST/Interp/loops.cpp:271
+  ///   be rejected.
+  constexpr int f6() {
+while(true);

Another test to add here: `for (;;);` is just as infinite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135764

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

[PATCH] D136343: [Lex] Add compatibility with MSVC

[Aaron Ballman via Phabricator via cfe-commits]

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

Thank you for the patch! It looks like it's missing test coverage for the 
changes, can you add some to `clang/test/Preprocessor`?

One question I have is: if we're going to support `__FUNCTION__`, shouldn't we 
also support `__FUNCDNAME__` and `__FUNCSIG__` as well, as those are also 
Microsoft predefined macros in the same general space?




Comment at: clang/include/clang/Lex/Preprocessor.h:153
   /// Identifiers for builtin macros and other builtins.
-  IdentifierInfo *Ident__LINE__, *Ident__FILE__;   // __LINE__, __FILE__
+  IdentifierInfo *Ident__LINE__, *Ident__FILE__, *Ident__FUNCTION__;   // 
__LINE__, __FILE__, __FUNCTION__
   IdentifierInfo *Ident__DATE__, *Ident__TIME__;   // __DATE__, __TIME__

You should put your declaration on a new line so the line length doesn't exceed 
80 columns (it's part of our coding style: 
https://llvm.org/docs/CodingStandards.html).



Comment at: clang/lib/Lex/PPMacroExpansion.cpp:340-344
+#ifdef _WIN32
+  Ident__FUNCTION__ = RegisterBuiltinMacro(*this, "__FUNCTION__");
+#else
+  Ident__FUNCTION__ = Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__");
+#endif

This looks incorrect in a few ways. The first is that you shouldn't use `#if 
_WIN32` for this -- that means the compiler, when compiled for Win32, will have 
`__FUNCTION__` which isn't what you were trying for (you'd want to use the 
language options to determine if you're compiling for Win32 or not, regardless 
of what OS the host compiler is running on).

Also, in the `#else` block you are registering `__LINE__` as the identifier for 
`__FUNCTION__`, which isn't correct.

However, if we're going to support `__FUNCTION__`, we should support it on all 
targets instead of just Windows, so I don't think you need a conditional at all 
here.



Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1569-1575
+#ifdef _WIN32
+if (II == Ident__FUNCTION__) {
+  FN += "(";
+  FN += Twine(PLoc.getLine()).str();
+  FN += ") ";
+}
+#endif

You don't want the `#ifdef` here either, but also, this looks incorrect 
compared to the output you get from MSVC. `__FUNCTION__` is the unmangled name 
of the function without any parameter or return type information, and this is 
printing line information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136343

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

[PATCH] D136354: [Driver] Enable nested configuration files

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff created this revision.
sepavloff added reviewers: aaron.ballman, kadircet, MaskRay, mgorny.
Herald added subscribers: StephenFan, hiraditya.
Herald added a project: All.
sepavloff requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: llvm-commits.

Users may partition parameters specified by configuration file and put
different groups into separate files. These files are inserted into the
main file using constructs `@file`. Relative file names in it are
resolved relative to the including configuration file and this is not
convenient in some cases. A configuration file, which resides in system
directory, may need to include a file with user-defined parameters and
still provide default definitions if such file is absent.

To solve such problems, the option `--config` is allowed inside
configuration files. Like `@file` it results in insertion of
command-line arguments but the algorithm of file search is different and
allows overriding system definitions with user ones.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136354

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/Inputs/config-6.cfg
  clang/test/Driver/config-file-errs.c
  clang/test/Driver/config-file.c
  clang/unittests/Driver/ToolChainTest.cpp
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1182,33 +1182,60 @@
   // Tokenize the contents into NewArgv.
   Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
-  if (!RelativeNames)
+  // Expanded file content may require additional transformations, like using
+  // absolute paths instead of relative in '@file' constructs or expanding
+  // macros.
+  if (!RelativeNames && !InConfigFile)
 return Error::success();
-  llvm::StringRef BasePath = llvm::sys::path::parent_path(FName);
-  // If names of nested response files should be resolved relative to including
-  // file, replace the included response file names with their full paths
-  // obtained by required resolution.
-  for (auto &Arg : NewArgv) {
-if (!Arg)
+
+  StringRef BasePath = llvm::sys::path::parent_path(FName);
+  for (auto I = NewArgv.begin(), E = NewArgv.end(); I != E; ++I) {
+const char *&Arg = *I;
+if (Arg == nullptr)
   continue;
 
 // Substitute  with the file's base path.
 if (InConfigFile)
   ExpandBasePaths(BasePath, Saver, Arg);
 
-// Skip non-rsp file arguments.
-if (Arg[0] != '@')
-  continue;
-
-StringRef FileName(Arg + 1);
-// Skip if non-relative.
-if (!llvm::sys::path::is_relative(FileName))
+// Get expanded file name.
+StringRef ArgStr(Arg);
+StringRef FileName;
+bool ConfigInclusion = false;
+if (ArgStr[0] == '@') {
+  FileName = ArgStr.drop_front(1);
+  if (!llvm::sys::path::is_relative(FileName))
+continue;
+} else if (ArgStr == "--config") {
+  if (I + 1 == E)
+return createStringError(std::errc::invalid_argument,
+ "Option '--config' requires an argument");
+  I = NewArgv.erase(I);
+  E = NewArgv.end();
+  FileName = *I;
+  ConfigInclusion = true;
+} else if (ArgStr.startswith("--config=")) {
+  FileName = ArgStr.drop_front(std::char_traits::length("--config="));
+  ConfigInclusion = true;
+} else {
   continue;
+}
 
 SmallString<128> ResponseFile;
 ResponseFile.push_back('@');
-ResponseFile.append(BasePath);
-llvm::sys::path::append(ResponseFile, FileName);
+
+// Update expansion construct.
+if (ConfigInclusion && !llvm::sys::path::has_parent_path(FileName)) {
+  SmallString<128> FilePath;
+  if (!findConfigFile(FileName, FilePath))
+return createStringError(
+std::make_error_code(std::errc::no_such_file_or_directory),
+"cannot not find configuration file: " + FileName);
+  ResponseFile.append(FilePath);
+} else {
+  ResponseFile.append(BasePath);
+  llvm::sys::path::append(ResponseFile, FileName);
+}
 Arg = Saver.save(ResponseFile.str()).data();
   }
   return Error::success();
Index: clang/unittests/Driver/ToolChainTest.cpp
===
--- clang/unittests/Driver/ToolChainTest.cpp
+++ clang/unittests/Driver/ToolChainTest.cpp
@@ -541,4 +541,58 @@
   }
 }
 
+TEST(ToolChainTest, NestedConfigFile) {
+  IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
+  IntrusiveRefCntPtr DiagID(new DiagnosticIDs());
+  struct TestDiagnosticConsumer : public DiagnosticConsumer {};
+  DiagnosticsEngine Diags(DiagID, &*DiagOpts, new TestDiagnosticConsumer);
+  IntrusiveRefCntPtr FS(
+  new llvm::vfs::InMemoryFileSystem);
+
+#ifdef _WIN32
+  const char *TestRoot = "C:\\";
+#else
+  const ch

[PATCH] D136355: [clang][Sema] Fix caret position to be on the non null parameter

[Arthur Grillo Queiroz Cabral via Phabricator via cfe-commits]

Grillo created this revision.
Grillo added a reviewer: aaron.ballman.
Herald added a project: All.
Grillo requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When checking for non null arguments the wrong SourceLocation was given,
this fix to pass the proper argument's location.

Reported in PR58273.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136355

Files:
  clang/lib/Sema/SemaChecking.cpp


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5638,7 +5638,7 @@
   for (unsigned ArgIndex = 0, ArgIndexEnd = NonNullArgs.size();
ArgIndex != ArgIndexEnd; ++ArgIndex) {
 if (NonNullArgs[ArgIndex])
-  CheckNonNullArgument(S, Args[ArgIndex], CallSiteLoc);
+  CheckNonNullArgument(S, Args[ArgIndex], Args[ArgIndex]->getExprLoc());
   }
 }
 


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5638,7 +5638,7 @@
   for (unsigned ArgIndex = 0, ArgIndexEnd = NonNullArgs.size();
ArgIndex != ArgIndexEnd; ++ArgIndex) {
 if (NonNullArgs[ArgIndex])
-  CheckNonNullArgument(S, Args[ArgIndex], CallSiteLoc);
+  CheckNonNullArgument(S, Args[ArgIndex], Args[ArgIndex]->getExprLoc());
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135013#3870894 , @tbaeder wrote:

> @aaron.ballman Do you maybe have a different reproducer or know more about 
> the expected behavior of a `ImplicitValueInitExpr` for array and record types?

So here's another reproducer with a record type: 
https://godbolt.org/z/EhT4oqT3s and here's one with an array type: 
https://godbolt.org/z/Pbncnq418

My understanding of `ImplicitValueInitExpr` is that it's used to represent some 
of the "holes" in the middle of an array/record that need to implicit get some 
values.


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

https://reviews.llvm.org/D135013

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

[PATCH] D133325: [Driver] Allow search of included response files as configuration files

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

An alternative implementation is in D136354 . 
Instead of using new command-line option, it uses `--config` to include file, 
which is searched for as a configuration file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133325

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

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 469223.
qiongsiwu1 removed a project: All.
qiongsiwu1 added a comment.
Herald added a project: All.

Adding two test cases to match `G_NOTUNING`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136187

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -116,11 +116,19 @@
 // RUN: %clang -### %s -g -target x86_64-scei-ps5 2>&1 \
 // RUN: | FileCheck -check-prefix=LDGARANGE %s
 
-// On the AIX, -g defaults to -gdbx and limited debug info.
+// On the AIX, -g defaults to limited debug info.
 // RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
-// RUN: | FileCheck -check-prefix=G_LIMITED -check-prefix=G_DBX %s
+// RUN: | FileCheck -check-prefix=G_LIMITED %s
 // RUN: %clang -### -c -g %s -target powerpc64-ibm-aix-xcoff 2>&1 \
-// RUN: | FileCheck -check-prefix=G_LIMITED -check-prefix=G_DBX %s
+// RUN: | FileCheck -check-prefix=G_LIMITED %s
+// RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefix=G_NOTUNING %s
+// RUN: %clang -### -c -g %s -target powerpc64-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefix=G_NOTUNING %s
+// RUN: %clang -### -c -g -gdbx %s -target powerpc-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefixes=G_LIMITED,G_DBX %s
+// RUN: %clang -### -c -g -gdbx %s -target powerpc64-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefixes=G_LIMITED,G_DBX %s
 
 // For DBX, -g defaults to -gstrict-dwarf.
 // RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4187,8 +4187,10 @@
   }
 
   // If a debugger tuning argument appeared, remember it.
+  bool HasDebuggerTuning = false;
   if (const Arg *A =
   Args.getLastArg(options::OPT_gTune_Group, options::OPT_ggdbN_Group)) 
{
+HasDebuggerTuning = true;
 if (checkDebugInfoOption(A, Args, D, TC)) {
   if (A->getOption().matches(options::OPT_glldb))
 DebuggerTuning = llvm::DebuggerKind::LLDB;
@@ -4364,8 +4366,12 @@
   // Adjust the debug info kind for the given toolchain.
   TC.adjustDebugInfoKind(DebugInfoKind, Args);
 
+  // On AIX, the debugger tuning option can be omitted if it is not explicitly
+  // set.
   RenderDebugEnablingArgs(Args, CmdArgs, DebugInfoKind, EffectiveDWARFVersion,
-  DebuggerTuning);
+  T.isOSAIX() && !HasDebuggerTuning
+  ? llvm::DebuggerKind::Default
+  : DebuggerTuning);
 
   // -fdebug-macro turns on macro debug info generation.
   if (Args.hasFlag(options::OPT_fdebug_macro, options::OPT_fno_debug_macro,


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -116,11 +116,19 @@
 // RUN: %clang -### %s -g -target x86_64-scei-ps5 2>&1 \
 // RUN: | FileCheck -check-prefix=LDGARANGE %s
 
-// On the AIX, -g defaults to -gdbx and limited debug info.
+// On the AIX, -g defaults to limited debug info.
 // RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
-// RUN: | FileCheck -check-prefix=G_LIMITED -check-prefix=G_DBX %s
+// RUN: | FileCheck -check-prefix=G_LIMITED %s
 // RUN: %clang -### -c -g %s -target powerpc64-ibm-aix-xcoff 2>&1 \
-// RUN: | FileCheck -check-prefix=G_LIMITED -check-prefix=G_DBX %s
+// RUN: | FileCheck -check-prefix=G_LIMITED %s
+// RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefix=G_NOTUNING %s
+// RUN: %clang -### -c -g %s -target powerpc64-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefix=G_NOTUNING %s
+// RUN: %clang -### -c -g -gdbx %s -target powerpc-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefixes=G_LIMITED,G_DBX %s
+// RUN: %clang -### -c -g -gdbx %s -target powerpc64-ibm-aix-xcoff 2>&1 \
+// RUN: | FileCheck -check-prefixes=G_LIMITED,G_DBX %s
 
 // For DBX, -g defaults to -gstrict-dwarf.
 // RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4187,8 +4187,10 @@
   }
 
   // If a debugger tuning argument appeared, remember it.
+  bool H

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

[Qiongsi Wu via Phabricator via cfe-commits]

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



Comment at: clang/test/Driver/debug-options.c:121
 // RUN: %clang -### -c -g %s -target powerpc-ibm-aix-xcoff 2>&1 \
-// RUN: | FileCheck -check-prefix=G_LIMITED -check-prefix=G_DBX %s
+// RUN: | FileCheck -check-prefix=G_LIMITED %s
 // RUN: %clang -### -c -g %s -target powerpc64-ibm-aix-xcoff 2>&1 \

shchenz wrote:
> nit: we can check that no `-debugger-tuning=` is emitted by check `G_NOTUNING`
Ah thanks for catching this! Fixed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136187

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

[PATCH] D135764: [clang][Interp] Implement for loops

[Timm Bäder via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:361
+  this->emitLabel(IncLabel);
+  if (Inc && !this->discard(Inc))
+return false;

aaron.ballman wrote:
> What does the call to `discard()` do? Evaluate the expression and discard the 
> results? (If so, the API might want for a better name at some point, as it 
> can also read like "do nothing with this expression".)
Yes, it's the same problem that https://reviews.llvm.org/D136013 tries to 
solve. If `Inc` evaluates to a value, it will leave that on the stack and 
//something// needs to remove it from there, which is what `discard()` does.



Comment at: clang/test/AST/Interp/loops.cpp:216-226
+  constexpr int f4() {
+int i = 0;
+for (;;) {
+  i = i + 1;
+
+  if (i == 5)
+break;

aaron.ballman wrote:
> I'd love to see a stress test for the compile time performance of:
> ```
> int func() {
>   int i = 0;
>   for (;;) {
> if (i == __INT_MAX__)
>   break;
> ++i;
>   }
>   return i;
> }
> static_assert(func() == __INT_MAX__);
> ```
> I don't think there's value to testing specifically how long it takes for 
> this to execute, but we should probably start thinking about how to validate 
> performance characteristics pretty soon. Perhaps relative performance 
> compared to the existing constant expression evaluator would be interesting 
> though?
> Perhaps relative performance compared to the existing constant expression 
> evaluator would be interesting though?

Definitely. My plan was basically to get some base functionality going and then 
always keep an eye on performance. Your test doesn't work because the current 
interpreter has a step limit, but for a simple loop up until `100'000`, the 
current interpreter takes 42s here and the new one takes 2s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135764

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

[PATCH] D135764: [clang][Interp] Implement for loops

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/AST/Interp/loops.cpp:271
+  ///   be rejected.
+  constexpr int f6() {
+while(true);

aaron.ballman wrote:
> Another test to add here: `for (;;);` is just as infinite.
Heh, that's what this was supposed to be :) I already have the `while(true)` 
one above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135764

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

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/bindings/python/clang/cindex.py:1530
+
+def record_needs_implicit_default_constructor(self):
+"""Returns True if the cursor refers to a C++ record declaration

anderslanglands wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > dblaikie wrote:
> > > > anderslanglands wrote:
> > > > > dblaikie wrote:
> > > > > > royjacobson wrote:
> > > > > > > anderslanglands wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > > I don't think we should expose any of 
> > > > > > > > > > > > > > > > > > > > > the "needs" functions like this -- 
> > > > > > > > > > > > > > > > > > > > > those are internal implementation 
> > > > > > > > > > > > > > > > > > > > > details of the class and I don't 
> > > > > > > > > > > > > > > > > > > > > think we want to calcify that into 
> > > > > > > > > > > > > > > > > > > > > something we have to support forever. 
> > > > > > > > > > > > > > > > > > > > > As we add members to a class, we 
> > > > > > > > > > > > > > > > > > > > > recalculate whether the added member 
> > > > > > > > > > > > > > > > > > > > > causes us to delete defaulted special 
> > > > > > > > > > > > > > > > > > > > > members (among other things), and the 
> > > > > > > > > > > > > > > > > > > > > "needs" functions are basically used 
> > > > > > > > > > > > > > > > > > > > > when the class is completed to handle 
> > > > > > > > > > > > > > > > > > > > > lazily created special members. I'm 
> > > > > > > > > > > > > > > > > > > > > pretty sure that lazy creation is not 
> > > > > > > > > > > > > > > > > > > > > mandated by the standard, which is 
> > > > > > > > > > > > > > > > > > > > > why I think the "needs" functions are 
> > > > > > > > > > > > > > > > > > > > > more of an implementation detail.
> > > > > > > > > > > > > > > > > > > > CC @erichkeane and @royjacobson as 
> > > > > > > > > > > > > > > > > > > > folks who have been in this same area 
> > > > > > > > > > > > > > > > > > > > of the compiler to see if they agree or 
> > > > > > > > > > > > > > > > > > > > disagree with my assessment there.
> > > > > > > > > > > > > > > > > > > I think so. The 'needs_*' functions query 
> > > > > > > > > > > > > > > > > > > `DeclaredSpecialMembers` and I'm pretty 
> > > > > > > > > > > > > > > > > > > sure it's modified when we add the 
> > > > > > > > > > > > > > > > > > > implicit definitions in the class 
> > > > > > > > > > > > > > > > > > > completion code. So this looks a bit 
> > > > > > > > > > > > > > > > > > > suspicious. Is this API //meant// to be 
> > > > > > > > > > > > > > > > > > > used with incomplete classes?
> > > > > > > > > > > > > > > > > > > For complete classes I think looking up 
> > > > > > > > > > > > > > > > > > > the default/move/copy constructor and 
> > > > > > > > > > > > > > > > > > > calling `isImplicit()` is the way to do 
> > > > > > > > > > > > > > > > > > > it.
> > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > About the 'is deleted' API - can't the 
> > > > > > > > > > > > > > > > > > > same be done for those functions as well 
> > > > > > > > > > > > > > > > > > > so we have a smaller API? 
> > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > If this //is// meant to be used with 
> > > > > > > > > > > > > > > > > > > incomplete classes for efficiency that 
> > > > > > > > > > > > > > > > > > > would be another thing, I guess.
> > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > So the intended use case here is I'm using 
> > > > > > > > > > > > > > > > > > libclang to parse an existing C++ libray's 
> > > > > > > > > > > > > > > > > > headers and generate a C interface to it. 
> > > > > > > > > > > > > > > > > > To do that I need to know if I need to 
> > > > > > > > > > > > > > > > > > generate default constructors etc, which 
> > > > > > > > > > > > > > > > > > the needs* methods do for me (I believe). 
> > > > > > > > > > > > > > > > > > The alternative is I have to check manually 
> > > > > > > > > > > > > > > > > > whether all the constructors/assignment 
> > > > > > > > > > > > > > > > > > operators exist, then implement the 
> > > > > > > > > > > > > > > > > > implicit declaration rules myself correctly 
> > > > > > > > > > > > > > > > > > for each version of the standard, which I'd 
> > > > >

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D135013#3871209 , @aaron.ballman 
wrote:

> In D135013#3870894 , @tbaeder wrote:
>
>> @aaron.ballman Do you maybe have a different reproducer or know more about 
>> the expected behavior of a `ImplicitValueInitExpr` for array and record 
>> types?
>
> So here's another reproducer with a record type: 
> https://godbolt.org/z/EhT4oqT3s and here's one with an array type: 
> https://godbolt.org/z/Pbncnq418
>
> My understanding of `ImplicitValueInitExpr` is that it's used to represent 
> some of the "holes" in the middle of an array/record that need to implicit 
> get some values.

They are //inside// a struct or array, but not of array or struct type, if I 
understand correctly. But I see what you're getting at, I guess this should 
basically go through `visitZeroInitializer()` (which is currently unused), 
which should be extended to handle structs and arrays.


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

https://reviews.llvm.org/D135013

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@aaron.ballman: thanks for tracking down the source of confusion between our 
perspectives. (& yeah, sorry, could've included less ambiguous examples with 
the extra nesting so we'd have avoided all that confusion)

Do you have particular examples where you find the fully explicit (1) 
especially helpful (to a novice or otherwise) over (2)?

It seems to me once you've got the top level type you have enough info to 
compare two things, even if you don't know the nested member types - "X" is distinct from "X" it doesn't seem to matter what the 
intermediate type is? but yeah, I guess it raises the question "is it that the 
length or the height is mismatched here" and more explicit would avoid that 
question. It does seem unfortunately verbose to my mind - like if we improved 
the printer to show only the relevant types when diffing template names, for 
instance, that'd be an improvement (though I guess we already do fancy things 
for template type diffing that don't show complete copy-paste-into-source 
usable names anyway, so that's a different situation - and doesn't necessarily 
say what we should do when the type appears alone in a diagnostic, not as a 
comparison)

*shrug* Sorry to you both for all the talking/back and forth then. Adding the 
fully explicit mode will address @dodoent's string-based reflection needs and 
@aaron.ballman so there's no tension between Clang's diagnostic goals and 
@DoDoENT's reflection goals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135013#3871250 , @tbaeder wrote:

> In D135013#3871209 , @aaron.ballman 
> wrote:
>
>> In D135013#3870894 , @tbaeder 
>> wrote:
>>
>>> @aaron.ballman Do you maybe have a different reproducer or know more about 
>>> the expected behavior of a `ImplicitValueInitExpr` for array and record 
>>> types?
>>
>> So here's another reproducer with a record type: 
>> https://godbolt.org/z/EhT4oqT3s and here's one with an array type: 
>> https://godbolt.org/z/Pbncnq418
>>
>> My understanding of `ImplicitValueInitExpr` is that it's used to represent 
>> some of the "holes" in the middle of an array/record that need to implicit 
>> get some values.
>
> They are //inside// a struct or array, but not of array or struct type, if I 
> understand correctly. But I see what you're getting at, I guess this should 
> basically go through `visitZeroInitializer()` (which is currently unused), 
> which should be extended to handle structs and arrays.

Ah, yes! That's true, they are inside the struct or array. I'm not certain you 
can have one *of* struct type in C++ because it's either going to be zeroed or 
left uninitialized (due to constructors).


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

https://reviews.llvm.org/D135013

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

[PATCH] D135938: [X86] Add AVX-VNNI-INT8 instructions.

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

In D135938#3867170 , @FreddyYe wrote:

> I get your point of "close to each other" and updated. And I merged the 
> Disasm tests, while I didn't merge the MC tests because it is not so 
> convenient to do. See latest updated.
>
> Do we need to rename old tests to follow this rule? Old tests: 
> https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/X86 and 
> https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/Disassembler/X86

Its not a priority, but if you are ever bored and want to do some cleaning then 
it help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135938

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

[PATCH] D135937: [X86] Support -march=raptorlake, meteorlake

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: clang/test/Driver/x86-march.c:120
+// RUN:   | FileCheck %s -check-prefix=meteorlake
+// meteorlake: "-target-cpu" "meteorlake"
 //

Move these after alderlake instead of the old atom cores?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135937

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134453#3871251 , @dblaikie wrote:

> @aaron.ballman: thanks for tracking down the source of confusion between our 
> perspectives. (& yeah, sorry, could've included less ambiguous examples with 
> the extra nesting so we'd have avoided all that confusion)

No worries, text is such a fantastic medium for confusion. :-D

> Do you have particular examples where you find the fully explicit (1) 
> especially helpful (to a novice or otherwise) over (2)?
>
> It seems to me once you've got the top level type you have enough info to 
> compare two things, even if you don't know the nested member types - 
> "X" is distinct from "X" it doesn't seem to matter 
> what the intermediate type is? but yeah, I guess it raises the question "is 
> it that the length or the height is mismatched here" and more explicit would 
> avoid that question.

That's largely the kind of situation I was imagining. When you just have a pile 
of integers and braces, you have to mentally map the braces to how many levels 
of initialization exist and you have to map the integers to what specifically 
is being initialized. Those mappings gives very little context as to what types 
are involved. e.g., it's easy to see that `{{3}, {5}}` doesn't match `{{4}, 
{5}}` in the text, but it's less easy to see if that's because of `Width` vs 
`Height` problems or because there was a surprise specialization involved, etc.

Sometimes you don't need that extra information and sometimes it may be 
helpful, and I'm not certain we can heuristically tell those situations apart 
for the user.

> It does seem unfortunately verbose to my mind - like if we improved the 
> printer to show only the relevant types when diffing template names, for 
> instance, that'd be an improvement (though I guess we already do fancy things 
> for template type diffing that don't show complete copy-paste-into-source 
> usable names anyway, so that's a different situation - and doesn't 
> necessarily say what we should do when the type appears alone in a 
> diagnostic, not as a comparison)

Yeah, it'd be nice to find a happy medium. CC @cjdb due to the exploratory work 
he's doing with improving the way we display diagnostics. Maybe Chris has a 
good idea here (or can help us resolve which direction to go)?

> *shrug* Sorry to you both for all the talking/back and forth then. Adding the 
> fully explicit mode will address @dodoent's string-based reflection needs and 
> @aaron.ballman so there's no tension between Clang's diagnostic goals and 
> @DoDoENT's reflection goals.

No worries about the back and forth (at least for me) -- I appreciate that 
we're taking the time to carefully consider the user experience.

In terms of next steps, I think we should try to see if there's a "clear" path 
forward in terms of printing the types vs not printing the types. If we find 
one, then great, we go that way. But if we don't seem to have a clear path 
forward (relatively quickly; I don't think we want to drag this discussion on 
for months trying to find the perfect answer), then I think I'm fine with the 
patch as-is. It fixes the issue of `t<{}>` (with empty braces specifically) 
while retaining the status quo in other areas, but still exposes useful 
functionality through the additional printing policy. Does that sound 
reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 469230.
tbaeder added a comment.

Thinking about it some more, doesn't make much sense to add all that right now 
when we don't have a reproducer to test it anyway. I think the patch as it is 
makes sense and it it's not enough in the future, there's a proper assert in 
place.


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

https://reviews.llvm.org/D135013

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/test/AST/Interp/arrays.cpp


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,15 @@
// expected-error {{must be initialized 
by a constant expression}} \
// expected-note {{cannot refer to 
element -2 of array of 10}}
 };
+
+namespace DefaultInit {
+  template 
+  struct B {
+T a[N];
+  };
+
+  int f() {
+ constexpr B arr = {};
+ constexpr int x = arr.a[0];
+  }
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -721,6 +721,27 @@
   if (!this->emitPopPtr(Initializer))
 return false;
 }
+return true;
+  } else if (const auto *IVIE = dyn_cast(Initializer)) {
+auto ArrayType = IVIE->getType()->getAsArrayTypeUnsafe();
+assert(ArrayType);
+const auto *CAT = dyn_cast(ArrayType);
+const size_t NumElems = CAT->getSize().getZExtValue();
+
+if (Optional ElemT = classify(CAT->getElementType())) {
+  // TODO(perf): For int and bool types, we can probably just skip this
+  //   since we memset our Block*s to 0 and so we have the desired value
+  //   without this.
+  for (size_t I = 0; I != NumElems; ++I) {
+if (!this->emitZero(*ElemT, Initializer))
+  return false;
+if (!this->emitInitElem(*ElemT, I, Initializer))
+  return false;
+  }
+} else {
+  assert(false && "default initializer for non-primitive type");
+}
+
 return true;
   }
 


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,15 @@
// expected-error {{must be initialized by a constant expression}} \
// expected-note {{cannot refer to element -2 of array of 10}}
 };
+
+namespace DefaultInit {
+  template 
+  struct B {
+T a[N];
+  };
+
+  int f() {
+ constexpr B arr = {};
+ constexpr int x = arr.a[0];
+  }
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -721,6 +721,27 @@
   if (!this->emitPopPtr(Initializer))
 return false;
 }
+return true;
+  } else if (const auto *IVIE = dyn_cast(Initializer)) {
+auto ArrayType = IVIE->getType()->getAsArrayTypeUnsafe();
+assert(ArrayType);
+const auto *CAT = dyn_cast(ArrayType);
+const size_t NumElems = CAT->getSize().getZExtValue();
+
+if (Optional ElemT = classify(CAT->getElementType())) {
+  // TODO(perf): For int and bool types, we can probably just skip this
+  //   since we memset our Block*s to 0 and so we have the desired value
+  //   without this.
+  for (size_t I = 0; I != NumElems; ++I) {
+if (!this->emitZero(*ElemT, Initializer))
+  return false;
+if (!this->emitInitElem(*ElemT, I, Initializer))
+  return false;
+  }
+} else {
+  assert(false && "default initializer for non-primitive type");
+}
+
 return true;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D136106#3863202 , @reames wrote:

> In D136106#3863147 , @craig.topper 
> wrote:
>
>>> In the original review, I'd mentioned that MinVLEN was sometimes zero. 
>>> That's still true, but apparently only happens if you specify multiple 
>>> extensions in the -target_feature string. So, we can at least test "v" and 
>>> "zve64x" on their own before I go figure out exactly what's going wrong 
>>> regarding e.g. parsing "+v,+zvl512b".
>>
>> I think you need to use `-target-feature "v" -target-feature "zvl512b".
>
> So, this was part right.  The other issue is I'd been using Zvl512b, and 
> apparently this bit of parsing code is case sensitive.  I'm going to look 
> into improving the error reporting here.

Replying back to myself here as I'm not quite sure where to put this...

So, it turns out target-feature is currently quite weird.  At the *clang* 
level, we require individual target-feature options for each extensions.  Using 
a compound "+v,+zvl512b" is not recognized by the clang code (i.e, the bit this 
patch was modifying).  However, this raw string gets joined with the valid 
attributes, and set into the IR as "target-features"="+64bit,+v,+zvl512b" on 
the function.  This string is then parsed by later consumers, and thus the 
extensions listed in the compound list *are* picked up during e.g. codegen.  
So, it turns out we both do, and don't, parse these compound target-feature 
strings.

I'm leaning in the direction of having clang just support the compound 
target-feature strings, but I haven't found the right place for that just yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136106

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.



> In terms of next steps, I think we should try to see if there's a "clear" 
> path forward in terms of printing the types vs not printing the types. If we 
> find one, then great, we go that way. But if we don't seem to have a clear 
> path forward (relatively quickly; I don't think we want to drag this 
> discussion on for months trying to find the perfect answer), then I think I'm 
> fine with the patch as-is. It fixes the issue of `t<{}>` (with empty braces 
> specifically) while retaining the status quo in other areas, but still 
> exposes useful functionality through the additional printing policy. Does 
> that sound reasonable?

If you reckon (1) is better overall anyway - happy enough to defer to your 
opinion there and go with that, skip/omit the printing policy.

I think the policy is like adding off-by-default warnings, and supporting a use 
case (using the string name for reflection when we'd recommend the AST) I don't 
think we want to encourage/support.

Admittedly going with (1) means that @DoDoENT's use case will then work, 
until/unless we come back around and make more strategic use of type names in 
this printing in the future to bring down the verbosity - so I'd still 
discourage that use case & warn that this isn't a guarantee that all type names 
will be included going forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D136154: [clang-format] Fix the continuation indenter

[Henrik Lafrenz via Phabricator via cfe-commits]

hel-ableton added a comment.

I'm not sure I'm following where you're getting at. So far I'm getting the 
following:

- my proposed fix was not ideal,  and only "accidentally" fixed our issue
- the fix including `Previous.isOneOf(TT_BinaryOperator...` is a better fix
- we should write a proper test case for that fix, as the one I submitted 
referred to the wrong fix
- the example you gave now (as a model to construct such a better test?) 
doesn't involve `TT_CtorInitializerColon` or any of the like, so...

I'm confused. :-)


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

https://reviews.llvm.org/D136154

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

[clang] ab8257c - [NFC] Fix a few whitespace inconsistencies.

[Paul Walker via cfe-commits]

Author: Paul Walker
Date: 2022-10-20T14:52:25Z
New Revision: ab8257ca0e9389d39c081d7bb8bd4afceb2215cc

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

LOG: [NFC] Fix a few whitespace inconsistencies.

Added: 


Modified: 
clang/include/clang/Basic/DiagnosticSemaKinds.td
llvm/include/llvm/IR/DIBuilder.h
llvm/lib/Analysis/CostModel.cpp
llvm/lib/Analysis/VFABIDemangling.cpp
llvm/lib/Analysis/ValueTracking.cpp
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
llvm/lib/Passes/PassBuilderPipelines.cpp
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h

Removed: 




diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f8fe9555f8c02..1ef1f23e8c876 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9960,10 +9960,10 @@ def err_argument_not_shifted_byte_or_xxff : Error<
   "argument should be an 8-bit value shifted by a multiple of 8 bits, or in 
the form 0x??FF">;
 def err_argument_not_contiguous_bit_field : Error<
   "argument %0 value should represent a contiguous bit field">;
-def err_rotation_argument_to_cadd
-: Error<"argument should be the value 90 or 270">;
-def err_rotation_argument_to_cmla
-: Error<"argument should be the value 0, 90, 180 or 270">;
+def err_rotation_argument_to_cadd : Error<
+  "argument should be the value 90 or 270">;
+def err_rotation_argument_to_cmla : Error<
+  "argument should be the value 0, 90, 180 or 270">;
 def warn_neon_vector_initializer_non_portable : Warning<
   "vector initializers are not compatible with NEON intrinsics in big endian "
   "mode">, InGroup>;

diff  --git a/llvm/include/llvm/IR/DIBuilder.h 
b/llvm/include/llvm/IR/DIBuilder.h
index 371a70fd1ee9c..dfb65ce341296 100644
--- a/llvm/include/llvm/IR/DIBuilder.h
+++ b/llvm/include/llvm/IR/DIBuilder.h
@@ -685,7 +685,7 @@ namespace llvm {
 DIGlobalVariable *createTempGlobalVariableFwdDecl(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
 unsigned LineNo, DIType *Ty, bool IsLocalToUnit, MDNode *Decl = 
nullptr,
-MDTuple *TemplateParams= nullptr, uint32_t AlignInBits = 0);
+MDTuple *TemplateParams = nullptr, uint32_t AlignInBits = 0);
 
 /// Create a new descriptor for an auto variable.  This is a local variable
 /// that is not a subprogram parameter.

diff  --git a/llvm/lib/Analysis/CostModel.cpp b/llvm/lib/Analysis/CostModel.cpp
index c6a68522993b8..1782b399e7fd0 100644
--- a/llvm/lib/Analysis/CostModel.cpp
+++ b/llvm/lib/Analysis/CostModel.cpp
@@ -109,6 +109,7 @@ void CostModelAnalysis::print(raw_ostream &OS, const 
Module*) const {
   else {
 Cost = TTI->getInstructionCost(&Inst, CostKind);
   }
+
   if (auto CostVal = Cost.getValue())
 OS << "Cost Model: Found an estimated cost of " << *CostVal;
   else
@@ -137,6 +138,7 @@ PreservedAnalyses CostModelPrinterPass::run(Function &F,
   else {
 Cost = TTI.getInstructionCost(&Inst, CostKind);
   }
+
   if (auto CostVal = Cost.getValue())
 OS << "Cost Model: Found an estimated cost of " << *CostVal;
   else

diff  --git a/llvm/lib/Analysis/VFABIDemangling.cpp 
b/llvm/lib/Analysis/VFABIDemangling.cpp
index fa483d00826a4..aa38c39ae71a1 100644
--- a/llvm/lib/Analysis/VFABIDemangling.cpp
+++ b/llvm/lib/Analysis/VFABIDemangling.cpp
@@ -272,6 +272,7 @@ ParseRet tryParseAlign(StringRef &ParseString, Align 
&Alignment) {
 
   return ParseRet::None;
 }
+
 #ifndef NDEBUG
 // Verify the assumtion that all vectors in the signature of a vector
 // function have the same number of elements.
@@ -292,7 +293,6 @@ bool verifyAllVectorsHaveSameWidth(FunctionType *Signature) 
{
 return (EC == VTy->getElementCount());
   });
 }
-
 #endif // NDEBUG
 
 // Extract the VectorizationFactor from a given function signature,

diff  --git a/llvm/lib/Analysis/ValueTracking.cpp 
b/llvm/lib/Analysis/ValueTracking.cpp
index 685910c94b5f0..563929a9771c7 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -2297,7 +2297,7 @@ static bool isGEPKnownNonNull(const GEPOperator *GEP, 
unsigned Depth,
 }
 
 // If we have a zero-sized type, the index doesn't matter. Keep looping.
-if (Q.DL.getTypeAllocSize(GTI.getIndexedType()).getKnownMinSize() == 0)
+if (Q.DL.getTypeAllocSize(GTI.getIndexedType()).isZero())
   continue;
 
 // Fast path the constant operand case both for efficiency and so we don't

diff  --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp 
b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
index 96d120d513ade..7b8e0423041f6 100644
--- a/llvm/lib/CodeGen/SelectionDAG/Legali

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 469234.
steakhal retitled this revision from "[analyzer] Fix assertion failure in 
RegionStore within bindArray()" to "[analyzer] Fix assertion failure with 
conflicting prototype calls".
steakhal edited the summary of this revision.
steakhal added a comment.

- Updated the summary.
- Implementing a completely new approach focusing on the runtime definition & 
argument-parameter binding


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136162

Files:
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/test/Analysis/region-store.c

Index: clang/test/Analysis/region-store.c
===
--- clang/test/Analysis/region-store.c
+++ clang/test/Analysis/region-store.c
@@ -1,7 +1,12 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection \
+// RUN:-verify -analyzer-config eagerly-assume=false -std=c99 %s \
+// RUN:-Wno-implicit-function-declaration
 
 int printf(const char *restrict,...);
 
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int*);
+
 // Testing core functionality of the region store.
 // radar://10127782
 int compoundLiteralTest(void) {
@@ -39,7 +44,6 @@
   return l.mem; // no-warning
 }
 
-void clang_analyzer_eval(int);
 void testConstraintOnRegionOffset(int *values, int length, int i){
   if (values[1] == 4) {
 values[i] = 5;
@@ -54,3 +58,24 @@
 clang_analyzer_eval(values[0] == 4);// expected-warning {{UNKNOWN}}
   }
 }
+
+int buffer[10];
+void b(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is treated as a zero-parameter prototype in C2x, conflicting with a subsequent definition}}
+void missingPrototypeCallsiteMatchingArgsAndParams() {
+  // expected-warning@+1 {{passing arguments to 'b' without a prototype is deprecated in all versions of C and is not supported in C2x}}
+  b(&buffer);
+}
+void b(int *c) { // expected-note {{conflicting prototype is here}}
+  clang_analyzer_dump(c); // expected-warning {{&Element{buffer,0 S64b,int}}}
+  *c = 42; // no-crash
+}
+
+void c(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is treated as a zero-parameter prototype in C2x, conflicting with a subsequent definition}}
+void missingPrototypeCallsiteMismatchingArgsAndParams() {
+  // expected-warning@+1 {{passing arguments to 'c' without a prototype is deprecated in all versions of C and is not supported in C2x}}
+  c(&buffer, &buffer);
+}
+void c(int *c) { // expected-note {{conflicting prototype is here}}
+  clang_analyzer_dump(c); // expected-warning {{Unknown}}
+  *c = 42; // no-crash
+}
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -424,6 +424,38 @@
   return Value;
 }
 
+/// Cast the argument value to the type of the parameter at the function
+/// declaration.
+/// Returns the argument value if it didn't need a cast.
+/// Or returns the cast argument if it needed a cast.
+/// Or returns 'Unknown' if it would need a cast but the callsite and the
+/// runtime definition don't match in terms of argument and parameter count.
+static SVal castArgToParamTypeIfNeeded(const CallEvent &Call, unsigned ArgIdx,
+   SVal ArgVal, SValBuilder &SVB) {
+  const FunctionDecl *RTDecl =
+  Call.getRuntimeDefinition().getDecl()->getAsFunction();
+  const auto *CallExprDecl = dyn_cast_or_null(Call.getDecl());
+
+  if (!RTDecl || !CallExprDecl)
+return ArgVal;
+
+  // The function decl of the Call (in the AST) will not have any parameter
+  // declarations, if it was 'only' declared without a prototype. However, the
+  // engine will find the appropriate runtime definition - basically a
+  // redeclaration, which has a function body (and a function prototype).
+  if (CallExprDecl->hasPrototype() || !RTDecl->hasPrototype())
+return ArgVal;
+
+  // Only do this cast if the number arguments at the callsite matches with
+  // the parameters at the runtime definition.
+  if (Call.getNumArgs() != RTDecl->getNumParams())
+return UnknownVal();
+
+  const Expr *ArgExpr = Call.getArgExpr(ArgIdx);
+  const ParmVarDecl *Param = RTDecl->getParamDecl(ArgIdx);
+  return SVB.evalCast(ArgVal, Param->getType(), ArgExpr->getType());
+}
+
 static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
  CallEvent::BindingsTy &Bindings,
  SValBuilder &SVB,
@@ -449,12 +481,18 @@
 // which makes getArgSVal() fail and return UnknownVal.
 SVal ArgVal = Call.getArgS

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D136162#3869645 , @NoQ wrote:

>> In this example, we try to store 42 to the Elem{buffer, 0}.
>
> In this case the natural question is, why does it go through `bindArray()`? 
> We're not binding an array. Can we try to preserve the contract that 
> `bindArray()` only deals with arrays?
>
> @martong's comment makes sense to me, it sounds like this could be a case of 
> `CallEvent::getRuntimeDefinition()` not picking up the right definition, at 
> least not consistently.

Thank you @martong and @NoQ for the helpful comments. They really helped a lot 
to look at this problem from a new perspective!
I hope, this new approach is more aligned with your expectations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136162

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

[PATCH] D120395: [X86] Prohibit arithmetic operations on type `__bfloat16`

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.
Herald added a subscriber: StephenFan.

@pengfei Do we still need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120395

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134453#3871386 , @dblaikie wrote:

>> In terms of next steps, I think we should try to see if there's a "clear" 
>> path forward in terms of printing the types vs not printing the types. If we 
>> find one, then great, we go that way. But if we don't seem to have a clear 
>> path forward (relatively quickly; I don't think we want to drag this 
>> discussion on for months trying to find the perfect answer), then I think 
>> I'm fine with the patch as-is. It fixes the issue of `t<{}>` (with empty 
>> braces specifically) while retaining the status quo in other areas, but 
>> still exposes useful functionality through the additional printing policy. 
>> Does that sound reasonable?
>
> If you reckon (1) is better overall anyway - happy enough to defer to your 
> opinion there and go with that, skip/omit the printing policy.

Okay, thanks for that! I'm still happy to consider alternatives if we can think 
of any, FWIW.

> I think the policy is like adding off-by-default warnings, and supporting a 
> use case (using the string name for reflection when we'd recommend the AST) I 
> don't think we want to encourage/support.

That's a fair point. So if we find no better approach, drop the policy and just 
always print types.

> Admittedly going with (1) means that @DoDoENT's use case will then work, 
> until/unless we come back around and make more strategic use of type names in 
> this printing in the future to bring down the verbosity - so I'd still 
> discourage that use case & warn that this isn't a guarantee that all type 
> names will be included going forward.

Absolutely agreed! IMO, whatever strings you get out of something with a 
printing policy generally have very little/no stability guarantees (it's part 
of the C++ API surface, so it's exactly as stable as any other C++ API). We'll 
change printing behavior whenever we feel the need to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM! Please give @rjmccall a few days to look it over as well before landing.




Comment at: clang/include/clang/AST/ASTContext.h:2379
 
+  /// More type predicates useful for type checking/promotion
+  bool isPromotableIntegerType(QualType T) const; // C99 6.3.1.1p2




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133668

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

[PATCH] D136013: [clang][Interp] Fix ignoring expression return values

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


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

https://reviews.llvm.org/D136013

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

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/bindings/python/clang/cindex.py:1530
+
+def record_needs_implicit_default_constructor(self):
+"""Returns True if the cursor refers to a C++ record declaration

aaron.ballman wrote:
> anderslanglands wrote:
> > dblaikie wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > anderslanglands wrote:
> > > > > > dblaikie wrote:
> > > > > > > royjacobson wrote:
> > > > > > > > anderslanglands wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > > > I don't think we should expose any 
> > > > > > > > > > > > > > > > > > > > > > of the "needs" functions like this 
> > > > > > > > > > > > > > > > > > > > > > -- those are internal 
> > > > > > > > > > > > > > > > > > > > > > implementation details of the class 
> > > > > > > > > > > > > > > > > > > > > > and I don't think we want to 
> > > > > > > > > > > > > > > > > > > > > > calcify that into something we have 
> > > > > > > > > > > > > > > > > > > > > > to support forever. As we add 
> > > > > > > > > > > > > > > > > > > > > > members to a class, we recalculate 
> > > > > > > > > > > > > > > > > > > > > > whether the added member causes us 
> > > > > > > > > > > > > > > > > > > > > > to delete defaulted special members 
> > > > > > > > > > > > > > > > > > > > > > (among other things), and the 
> > > > > > > > > > > > > > > > > > > > > > "needs" functions are basically 
> > > > > > > > > > > > > > > > > > > > > > used when the class is completed to 
> > > > > > > > > > > > > > > > > > > > > > handle lazily created special 
> > > > > > > > > > > > > > > > > > > > > > members. I'm pretty sure that lazy 
> > > > > > > > > > > > > > > > > > > > > > creation is not mandated by the 
> > > > > > > > > > > > > > > > > > > > > > standard, which is why I think the 
> > > > > > > > > > > > > > > > > > > > > > "needs" functions are more of an 
> > > > > > > > > > > > > > > > > > > > > > implementation detail.
> > > > > > > > > > > > > > > > > > > > > CC @erichkeane and @royjacobson as 
> > > > > > > > > > > > > > > > > > > > > folks who have been in this same area 
> > > > > > > > > > > > > > > > > > > > > of the compiler to see if they agree 
> > > > > > > > > > > > > > > > > > > > > or disagree with my assessment there.
> > > > > > > > > > > > > > > > > > > > I think so. The 'needs_*' functions 
> > > > > > > > > > > > > > > > > > > > query `DeclaredSpecialMembers` and I'm 
> > > > > > > > > > > > > > > > > > > > pretty sure it's modified when we add 
> > > > > > > > > > > > > > > > > > > > the implicit definitions in the class 
> > > > > > > > > > > > > > > > > > > > completion code. So this looks a bit 
> > > > > > > > > > > > > > > > > > > > suspicious. Is this API //meant// to be 
> > > > > > > > > > > > > > > > > > > > used with incomplete classes?
> > > > > > > > > > > > > > > > > > > > For complete classes I think looking up 
> > > > > > > > > > > > > > > > > > > > the default/move/copy constructor and 
> > > > > > > > > > > > > > > > > > > > calling `isImplicit()` is the way to do 
> > > > > > > > > > > > > > > > > > > > it.
> > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > About the 'is deleted' API - can't the 
> > > > > > > > > > > > > > > > > > > > same be done for those functions as 
> > > > > > > > > > > > > > > > > > > > well so we have a smaller API? 
> > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > If this //is// meant to be used with 
> > > > > > > > > > > > > > > > > > > > incomplete classes for efficiency that 
> > > > > > > > > > > > > > > > > > > > would be another thing, I guess.
> > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > So the intended use case here is I'm 
> > > > > > > > > > > > > > > > > > > using libclang to parse an existing C++ 
> > > > > > > > > > > > > > > > > > > libray's headers and generate a C 
> > > > > > > > > > > > > > > > > > > interface to it. To do that I need to 
> > > > > > > > > > > > > > > > > > > know if I need to generate default 
> > > > > > > > > > > > > > > > > > > constructors etc, which the needs* 
> > > > > > > > > > > > > > > > > > > methods do for me (I believe). The 
> > > > > > > > > > > > > > > > > > > alternative is I have to check manually 
> > > > > > > > > > > > > > > > > > > 

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:726
+  } else if (const auto *IVIE = dyn_cast(Initializer)) {
+auto ArrayType = IVIE->getType()->getAsArrayTypeUnsafe();
+assert(ArrayType);

Please spell out the type.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:728
+assert(ArrayType);
+const auto *CAT = dyn_cast(ArrayType);
+const size_t NumElems = CAT->getSize().getZExtValue();

Should this be using `cast<>` as the code below is assuming this pointer will 
never be null?



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:729
+const auto *CAT = dyn_cast(ArrayType);
+const size_t NumElems = CAT->getSize().getZExtValue();
+




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

https://reviews.llvm.org/D135013

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D134453#3871414 , @aaron.ballman 
wrote:

> In D134453#3871386 , @dblaikie 
> wrote:
>
>>> In terms of next steps, I think we should try to see if there's a "clear" 
>>> path forward in terms of printing the types vs not printing the types. If 
>>> we find one, then great, we go that way. But if we don't seem to have a 
>>> clear path forward (relatively quickly; I don't think we want to drag this 
>>> discussion on for months trying to find the perfect answer), then I think 
>>> I'm fine with the patch as-is. It fixes the issue of `t<{}>` (with empty 
>>> braces specifically) while retaining the status quo in other areas, but 
>>> still exposes useful functionality through the additional printing policy. 
>>> Does that sound reasonable?
>>
>> If you reckon (1) is better overall anyway - happy enough to defer to your 
>> opinion there and go with that, skip/omit the printing policy.
>
> Okay, thanks for that! I'm still happy to consider alternatives if we can 
> think of any, FWIW.
>
>> I think the policy is like adding off-by-default warnings, and supporting a 
>> use case (using the string name for reflection when we'd recommend the AST) 
>> I don't think we want to encourage/support.
>
> That's a fair point. So if we find no better approach, drop the policy and 
> just always print types.

Yeah - I don't think I have any better ideas that are reasonably within reach 
for a newer contributor nor worth holding up fixing the `t1<{}>` bug for.

Yeah, for type comparisons/fancy template type diffing we could trim things 
down if we aren't already(we probably aren't), but that doesn't address the 
cases where the name appears alone and not as part of a comparison. And when 
there's no particular context, I can't think of any cues we could use to decide 
which nested names "matter".

(rabbit hole/tangent: Arguably what might be more useful to the user might be a 
name that's not even usable in C++ - using the member variable names, as well 
as the type names, though that'd be even more verbose... like `t1` - which, I guess, if you had user defined types for some kind of 
extra type safety, would get as verbose as `t1` though I would expect that would be the minority)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/bindings/python/clang/cindex.py:1530
+
+def record_needs_implicit_default_constructor(self):
+"""Returns True if the cursor refers to a C++ record declaration

dblaikie wrote:
> aaron.ballman wrote:
> > anderslanglands wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > dblaikie wrote:
> > > > > > anderslanglands wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > royjacobson wrote:
> > > > > > > > > anderslanglands wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > > > > > I don't think we should expose 
> > > > > > > > > > > > > > > > > > > > > > > any of the "needs" functions like 
> > > > > > > > > > > > > > > > > > > > > > > this -- those are internal 
> > > > > > > > > > > > > > > > > > > > > > > implementation details of the 
> > > > > > > > > > > > > > > > > > > > > > > class and I don't think we want 
> > > > > > > > > > > > > > > > > > > > > > > to calcify that into something we 
> > > > > > > > > > > > > > > > > > > > > > > have to support forever. As we 
> > > > > > > > > > > > > > > > > > > > > > > add members to a class, we 
> > > > > > > > > > > > > > > > > > > > > > > recalculate whether the added 
> > > > > > > > > > > > > > > > > > > > > > > member causes us to delete 
> > > > > > > > > > > > > > > > > > > > > > > defaulted special members (among 
> > > > > > > > > > > > > > > > > > > > > > > other things), and the "needs" 
> > > > > > > > > > > > > > > > > > > > > > > functions are basically used when 
> > > > > > > > > > > > > > > > > > > > > > > the class is completed to handle 
> > > > > > > > > > > > > > > > > > > > > > > lazily created special members. 
> > > > > > > > > > > > > > > > > > > > > > > I'm pretty sure that lazy 
> > > > > > > > > > > > > > > > > > > > > > > creation is not mandated by the 
> > > > > > > > > > > > > > > > > > > > > > > standard, which is why I think 
> > > > > > > > > > > > > > > > > > > > > > > the "needs" functions are more of 
> > > > > > > > > > > > > > > > > > > > > > > an implementation detail.
> > > > > > > > > > > > > > > > > > > > > > CC @erichkeane and @royjacobson as 
> > > > > > > > > > > > > > > > > > > > > > folks who have been in this same 
> > > > > > > > > > > > > > > > > > > > > > area of the compiler to see if they 
> > > > > > > > > > > > > > > > > > > > > > agree or disagree with my 
> > > > > > > > > > > > > > > > > > > > > > assessment there.
> > > > > > > > > > > > > > > > > > > > > I think so. The 'needs_*' functions 
> > > > > > > > > > > > > > > > > > > > > query `DeclaredSpecialMembers` and 
> > > > > > > > > > > > > > > > > > > > > I'm pretty sure it's modified when we 
> > > > > > > > > > > > > > > > > > > > > add the implicit definitions in the 
> > > > > > > > > > > > > > > > > > > > > class completion code. So this looks 
> > > > > > > > > > > > > > > > > > > > > a bit suspicious. Is this API 
> > > > > > > > > > > > > > > > > > > > > //meant// to be used with incomplete 
> > > > > > > > > > > > > > > > > > > > > classes?
> > > > > > > > > > > > > > > > > > > > > For complete classes I think looking 
> > > > > > > > > > > > > > > > > > > > > up the default/move/copy constructor 
> > > > > > > > > > > > > > > > > > > > > and calling `isImplicit()` is the way 
> > > > > > > > > > > > > > > > > > > > > to do it.
> > > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > > About the 'is deleted' API - can't 
> > > > > > > > > > > > > > > > > > > > > the same be done for those functions 
> > > > > > > > > > > > > > > > > > > > > as well so we have a smaller API? 
> > > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > > If this //is// meant to be used with 
> > > > > > > > > > > > > > > > > > > > > incomplete classes for efficiency 
> > > > > > > > > > > > > > > > > > > > > that would be another thing, I guess.
> > > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > So the intended use case here is I'm 
> > > > > > > > > > > > > > > > > > > > using libclang to parse an existing C++ 
> > > > > > > > > > > > > > > > > > > > libray's headers and generate a C 
> > > > > > > > > > > > > > > > > > > > interface to it. To do that I need to 
> > > > > > > > > > > > > > > > > > > > know 

[PATCH] D134453: Disambiguate type names when printing NTTP types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D134453#3871458 , @dblaikie wrote:

> In D134453#3871414 , @aaron.ballman 
> wrote:
>
>> In D134453#3871386 , @dblaikie 
>> wrote:
>>
 In terms of next steps, I think we should try to see if there's a "clear" 
 path forward in terms of printing the types vs not printing the types. If 
 we find one, then great, we go that way. But if we don't seem to have a 
 clear path forward (relatively quickly; I don't think we want to drag this 
 discussion on for months trying to find the perfect answer), then I think 
 I'm fine with the patch as-is. It fixes the issue of `t<{}>` (with empty 
 braces specifically) while retaining the status quo in other areas, but 
 still exposes useful functionality through the additional printing policy. 
 Does that sound reasonable?
>>>
>>> If you reckon (1) is better overall anyway - happy enough to defer to your 
>>> opinion there and go with that, skip/omit the printing policy.
>>
>> Okay, thanks for that! I'm still happy to consider alternatives if we can 
>> think of any, FWIW.
>>
>>> I think the policy is like adding off-by-default warnings, and supporting a 
>>> use case (using the string name for reflection when we'd recommend the AST) 
>>> I don't think we want to encourage/support.
>>
>> That's a fair point. So if we find no better approach, drop the policy and 
>> just always print types.
>
> Yeah - I don't think I have any better ideas that are reasonably within reach 
> for a newer contributor nor worth holding up fixing the `t1<{}>` bug for.
>
> Yeah, for type comparisons/fancy template type diffing we could trim things 
> down if we aren't already(we probably aren't), but that doesn't address the 
> cases where the name appears alone and not as part of a comparison. And when 
> there's no particular context, I can't think of any cues we could use to 
> decide which nested names "matter".
>
> (rabbit hole/tangent: Arguably what might be more useful to the user might be 
> a name that's not even usable in C++ - using the member variable names, as 
> well as the type names, though that'd be even more verbose... like 
> `t1` - which, I guess, if you had user defined types 
> for some kind of extra type safety, would get as verbose as 
> `t1` though I would expect 
> that would be the minority)

Correction, apparently that is valid syntax, which is nice: 
https://godbolt.org/z/xaYYo3fva


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D136354: [Driver] Enable nested configuration files

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

I like this more than the alternative solution. The code still has some 
complexity but the behavior is fairly logical and consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136354

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

That's a good point, I was a little suspicious about that function.

I'm not sure it makes sense to infer the target at all from the SDK (e.g. macOS 
could be x86_64 or arm64) rather than infer the SDK from the target.  Rust, for 
example, disregards `SYSROOT` and runs `xcrun` if it doesn't match the target 
triple.  I wonder if that behavior is cemented enough that it shouldn't change, 
though.  At a minimum, you're right that we should skip inferring the target in 
the case where the sysroot isn't specified at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 469243.
tbaeder marked 3 inline comments as done.

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

https://reviews.llvm.org/D135013

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/test/AST/Interp/arrays.cpp


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,15 @@
// expected-error {{must be initialized 
by a constant expression}} \
// expected-note {{cannot refer to 
element -2 of array of 10}}
 };
+
+namespace DefaultInit {
+  template 
+  struct B {
+T a[N];
+  };
+
+  int f() {
+ constexpr B arr = {};
+ constexpr int x = arr.a[0];
+  }
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -721,6 +721,27 @@
   if (!this->emitPopPtr(Initializer))
 return false;
 }
+return true;
+  } else if (const auto *IVIE = dyn_cast(Initializer)) {
+const ArrayType *ArrayType = IVIE->getType()->getAsArrayTypeUnsafe();
+assert(ArrayType);
+const auto *CAT = cast(ArrayType);
+size_t NumElems = CAT->getSize().getZExtValue();
+
+if (Optional ElemT = classify(CAT->getElementType())) {
+  // TODO(perf): For int and bool types, we can probably just skip this
+  //   since we memset our Block*s to 0 and so we have the desired value
+  //   without this.
+  for (size_t I = 0; I != NumElems; ++I) {
+if (!this->emitZero(*ElemT, Initializer))
+  return false;
+if (!this->emitInitElem(*ElemT, I, Initializer))
+  return false;
+  }
+} else {
+  assert(false && "default initializer for non-primitive type");
+}
+
 return true;
   }
 


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,15 @@
// expected-error {{must be initialized by a constant expression}} \
// expected-note {{cannot refer to element -2 of array of 10}}
 };
+
+namespace DefaultInit {
+  template 
+  struct B {
+T a[N];
+  };
+
+  int f() {
+ constexpr B arr = {};
+ constexpr int x = arr.a[0];
+  }
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -721,6 +721,27 @@
   if (!this->emitPopPtr(Initializer))
 return false;
 }
+return true;
+  } else if (const auto *IVIE = dyn_cast(Initializer)) {
+const ArrayType *ArrayType = IVIE->getType()->getAsArrayTypeUnsafe();
+assert(ArrayType);
+const auto *CAT = cast(ArrayType);
+size_t NumElems = CAT->getSize().getZExtValue();
+
+if (Optional ElemT = classify(CAT->getElementType())) {
+  // TODO(perf): For int and bool types, we can probably just skip this
+  //   since we memset our Block*s to 0 and so we have the desired value
+  //   without this.
+  for (size_t I = 0; I != NumElems; ++I) {
+if (!this->emitZero(*ElemT, Initializer))
+  return false;
+if (!this->emitInitElem(*ElemT, I, Initializer))
+  return false;
+  }
+} else {
+  assert(false && "default initializer for non-primitive type");
+}
+
 return true;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

[Tomasz Kamiński via Phabricator via cfe-commits]

tomasz-kaminski-sonarsource added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:490
+// edge-cases.
+ArgVal = castArgToParamTypeIfNeeded(Call, Idx, ArgVal, SVB);
+

Previously we didng make bindings if `ArgVal` was unknown, and we may want to 
preserve this invariant.



Comment at: clang/test/Analysis/region-store.c:66
+  // expected-warning@+1 {{passing arguments to 'b' without a prototype is 
deprecated in all versions of C and is not supported in C2x}}
+  b(&buffer);
+}

I would like to see an example where the called function is implicitly defined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136162

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

If `clang -target arm64-apple-ios -isysroot path/to/MacOSX.sdk` changes the 
target to `x86_64-apple-macos` to match the SDK, that would be very confusing 
behavior :) I'll have to investigate that function some more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

(pulling this out of inline comments because Phab's reply quoting doesn't work 
so well there & the threading/ordering gets weird too)

>> Well, I'm hoping we can find a way to avoid doing that in the general case 
>> while still giving users a way to opt into that behavior if they need it for 
>> the C APIs or AST matching.
>
> Ah, I was figuring not wanting to diverge/provide toggles if we can help it - 
> like unused warnings, opt-in features may go undertested (especially for 
> these sort of operations being a bit subtle in some ways). Like we've 
> discussed (at least I remember it coming up many years ago) trying to unify 
> more of the static analyzer and IRGen to avoid divergence there - adding more 
> divergence between static analysis and IRGen/etc seems liable to create more 
> risk of bugs/inconsistencies.
>
> I definitely see the appeal to not having a toggle. I was envisioning was 
> more "generating an AST for matching over and C indexing API to create an AST 
> will set this flag" rather than "user-facing way to decide on the behavior in 
> arbitrary situations", but that does make testing more complicated because 
> the C index code wouldn't be generating the same AST as a "normal" 
> compilation.

*nod* Perhaps it's not the end of the world - just my thoughts/concerns. I 
won't feel too poorly if you go another way with this.

> Hmmm. This might be one of those times when we have to say "sorry, this is 
> what the AST looks like and these APIs operate on the AST" without making 
> changes. That would be pretty unfortunate because I think the use case from 
> @anderslanglands is reasonable, but it also doesn't seem reasonable enough to 
> warrant impacting the performance of all C++ compilations with Clang.

I was hoping the rephrasing (is this really a question about which ctors the 
type has, or about how the type can be constructible) might offer us a way out 
for this use case, at least - if it's about how the type is constructible, then 
the AST wouldn't be the ideal/complete solution anyway and we could move more 
towards exposing the 5 special member ops as "can I do this thing/would this 
expression be valid".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135557

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

[PATCH] D136363: [OpenMP] Remove `-Bsymbolic` flag for device linking in the GNU toolchain

[Joseph Huber via Phabricator via cfe-commits]

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

Previously, OpenMP linking would be done explicitly in a linker stage.
For `x86_64` offloading this would just use the host linker, which could
be the `bfd` linker. This linker had problems linking relocations
against variables with protected visibility so we force `-Bsymbolic`
when linking. After the deprecation of the old offloading driver this
code is no longer used and can be removed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136363

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp


Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -586,13 +586,6 @@
 CmdArgs.push_back("-lm");
   }
 
-  // If we are linking for the device all symbols should be bound locally. The
-  // symbols are already protected which makes this redundant. This is only
-  // necessary to work around a problem in bfd.
-  // TODO: Remove this once 'lld' becomes the only linker for offloading.
-  if (JA.isDeviceOffloading(Action::OFK_OpenMP))
-CmdArgs.push_back("-Bsymbolic");
-
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 


Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -586,13 +586,6 @@
 CmdArgs.push_back("-lm");
   }
 
-  // If we are linking for the device all symbols should be bound locally. The
-  // symbols are already protected which makes this redundant. This is only
-  // necessary to work around a problem in bfd.
-  // TODO: Remove this once 'lld' becomes the only linker for offloading.
-  if (JA.isDeviceOffloading(Action::OFK_OpenMP))
-CmdArgs.push_back("-Bsymbolic");
-
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136248: [Index] consider `delete X` a reference to ~X() if it can be resolved

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for chiming in, left a comment on GitHub 
. 
TLDR; destructors are easy to find and this is not the case for user-defined 
`operator delete`. Maybe this is a good reason to have `operator delete` 
references (either instead of in addition to destructors)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136248

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

[PATCH] D136363: [OpenMP] Remove `-Bsymbolic` flag for device linking in the GNU toolchain

[Shilei Tian via Phabricator via cfe-commits]

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

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136363

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

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4374
+  ? llvm::DebuggerKind::Default
+  : DebuggerTuning);
 

Seems like you should be able to return `DebuggerKind::Default` from 
`getDefaultDebuggerTuning` then you wouldn't need this complication?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136187

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

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/driver-help-hidden.f90:34
 ! CHECK-NEXT: Use  as character line width in fixed mode
+! CHECK-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs): fast (fuses 
across statements disregarding pragmas) | on (only fuses in the same statement 
unless dictated by pragmas) | off (never fuses) | fast-honor-pragmas (fuses 
across statements unless diectated by pragmas). Default is 'fast' for CUDA, 
'fast-honor-pragmas' for HIP, and 'on' otherwise.
 ! CHECK-NEXT: -ffree-formProcess source files in free form

tblah wrote:
> vzakhari wrote:
> > Is it easy to emit a different help message for Flang to say that there are 
> > only two modes for Fortran?
> @awarzynski tells me there is no way to do this short of having separate 
> `Options.td` for flang and clang. Once we have settled on which arguments to 
> support, I will update the shared help text to mention flang.
Both `clang -help` and `flang-new -help` must be 100% correct. As this help 
text is not valid in the case of LLVM Flang, it needs to be updated 
accordingly. 

As @tblah points out, there's no straightforward mechanism for having a custom 
help texts for `clang` and `flang-new` in Clang's driver library ATM. But I 
think that this can be achieved even without creating a separate "Options.td" 
file. One would have to define a new tablegen record in "Options.td". That 
would be a separate patch though, probably accompanied by an RFC.

There's a different solution too. Note that currently the definition uses the 
`HelpText` field. However, you could use [[ 
https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/include/clang/Driver/Options.td#L81-L82
 | DocBrief ]] instead (which we used to solve a similar issue with [[ 
https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/include/clang/Driver/Options.td#L696-L704
 | -I ]]). That's what I suggest that you do. Basically, copy the contents of 
`HelpText` for `-ffp-contract` into a `DocBrief` field (we don't use this field 
in Flang and it should probably be renamed as `DocBriefClang`). `HelpText` 
should be replaced with something brief that applies both to Clang and Flang.

Btw, what's the help-text/spelling in `gfortran`?


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

https://reviews.llvm.org/D136080

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: clang/test/Driver/as-options.s:121
+// Test that -g is passed through to GAS.
+// RUN: %clang -fno-integrated-as -g %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=CHECK-DEBUG %s

Please use an explicit triple here.  I believe not all targets support 
`-fno-integrated-as`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

[Tomasz Kamiński via Phabricator via cfe-commits]

tomasz-kaminski-sonarsource added inline comments.



Comment at: clang/test/Analysis/region-store.c:66
+  // expected-warning@+1 {{passing arguments to 'b' without a prototype is 
deprecated in all versions of C and is not supported in C2x}}
+  b(&buffer);
+}

tomasz-kaminski-sonarsource wrote:
> I would like to see an example where the called function is implicitly 
> defined.
After rethinking it, I have not idea how to construct that example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136162

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 469256.
nickdesaulniers marked 2 inline comments as done.
nickdesaulniers added a comment.

- add triple, use Arg::AddLastArg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/as-options.s


Index: clang/test/Driver/as-options.s
===
--- clang/test/Driver/as-options.s
+++ clang/test/Driver/as-options.s
@@ -116,3 +116,8 @@
 // RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused
+
+// Test that -g is passed through to GAS.
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g %s -### 2>&1 | 
\
+// RUN:   FileCheck --check-prefix=CHECK-DEBUG %s
+// CHECK-DEBUG: "-g"
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -976,6 +976,9 @@
   for (const auto &II : Inputs)
 CmdArgs.push_back(II.getFilename());
 
+  if (Arg *A = Args.getLastArg(options::OPT_g_Flag))
+Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath(DefaultAssembler));
   C.addCommand(std::make_unique(JA, *this,


Index: clang/test/Driver/as-options.s
===
--- clang/test/Driver/as-options.s
+++ clang/test/Driver/as-options.s
@@ -116,3 +116,8 @@
 // RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused
+
+// Test that -g is passed through to GAS.
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=CHECK-DEBUG %s
+// CHECK-DEBUG: "-g"
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -976,6 +976,9 @@
   for (const auto &II : Inputs)
 CmdArgs.push_back(II.getFilename());
 
+  if (Arg *A = Args.getLastArg(options::OPT_g_Flag))
+Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath(DefaultAssembler));
   C.addCommand(std::make_unique(JA, *this,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

@MaskRay do you think I should make it so that `-g -g0` disables passing 
through `-g`?




Comment at: clang/test/Driver/as-options.s:120
+
+// Test that -g is passed through to GAS.
+// RUN: %clang -fno-integrated-as -g %s -### 2>&1 | \

MaskRay wrote:
> -g can be tested along with other pass-through options. This way we test the 
> relative order (though usually it doesn't matter but it improves the check) 
> and decreases the number of clang invocations.
Is the suggestion to:
1. combine this test with a pre-existing RUN line? or
2. Test all of the pass through options in this run line.

If 1, then I'm not sure which test best to combine this with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

[Balázs Benics via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/region-store.c:66
+  // expected-warning@+1 {{passing arguments to 'b' without a prototype is 
deprecated in all versions of C and is not supported in C2x}}
+  b(&buffer);
+}

tomasz-kaminski-sonarsource wrote:
> tomasz-kaminski-sonarsource wrote:
> > I would like to see an example where the called function is implicitly 
> > defined.
> After rethinking it, I have not idea how to construct that example.
I could not construct such an example.
It seems like clang errors out for cases when an implicit declaration of a call 
mismatches with the definition of that function.
https://godbolt.org/z/rM9ajeTf7


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136162

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

[PATCH] D136154: [clang-format] Fix the continuation indenter

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

You can remove this line in my view...

> Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) {


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

https://reviews.llvm.org/D136154

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

[clang] 2a9a13d - [OpenMP] Remove `-Bsymbolic` flag for device linking in the GNU toolchain

[Joseph Huber via cfe-commits]

Author: Joseph Huber
Date: 2022-10-20T11:25:16-05:00
New Revision: 2a9a13d9cc5fceeb2cdb293dffeca0bc326c479f

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

LOG: [OpenMP] Remove `-Bsymbolic` flag for device linking in the GNU toolchain

Previously, OpenMP linking would be done explicitly in a linker stage.
For `x86_64` offloading this would just use the host linker, which could
be the `bfd` linker. This linker had problems linking relocations
against variables with protected visibility so we force `-Bsymbolic`
when linking. After the deprecation of the old offloading driver this
code is no longer used and can be removed.

Reviewed By: tianshilei1992

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

Added: 


Modified: 
clang/lib/Driver/ToolChains/Gnu.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/Gnu.cpp 
b/clang/lib/Driver/ToolChains/Gnu.cpp
index 9d3490f2acbb..f717ca3ed849 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -586,13 +586,6 @@ void tools::gnutools::Linker::ConstructJob(Compilation &C, 
const JobAction &JA,
 CmdArgs.push_back("-lm");
   }
 
-  // If we are linking for the device all symbols should be bound locally. The
-  // symbols are already protected which makes this redundant. This is only
-  // necessary to work around a problem in bfd.
-  // TODO: Remove this once 'lld' becomes the only linker for offloading.
-  if (JA.isDeviceOffloading(Action::OFK_OpenMP))
-CmdArgs.push_back("-Bsymbolic");
-
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 



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

[PATCH] D136363: [OpenMP] Remove `-Bsymbolic` flag for device linking in the GNU toolchain

[Joseph Huber via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a9a13d9cc5f: [OpenMP] Remove `-Bsymbolic` flag for device 
linking in the GNU toolchain (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136363

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp


Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -586,13 +586,6 @@
 CmdArgs.push_back("-lm");
   }
 
-  // If we are linking for the device all symbols should be bound locally. The
-  // symbols are already protected which makes this redundant. This is only
-  // necessary to work around a problem in bfd.
-  // TODO: Remove this once 'lld' becomes the only linker for offloading.
-  if (JA.isDeviceOffloading(Action::OFK_OpenMP))
-CmdArgs.push_back("-Bsymbolic");
-
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 


Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -586,13 +586,6 @@
 CmdArgs.push_back("-lm");
   }
 
-  // If we are linking for the device all symbols should be bound locally. The
-  // symbols are already protected which makes this redundant. This is only
-  // necessary to work around a problem in bfd.
-  // TODO: Remove this once 'lld' becomes the only linker for offloading.
-  if (JA.isDeviceOffloading(Action::OFK_OpenMP))
-CmdArgs.push_back("-Bsymbolic");
-
   // Silence warnings when linking C code with a C++ '-stdlib' argument.
   Args.ClaimAllArgs(options::OPT_stdlib_EQ);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135060: [HLSL] Add groupshare address space.

[Xiang Li via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
python3kgae marked an inline comment as done.
Closed by commit rG7e04c0ad6325: [HLSL] Add groupshare address space. (authored 
by python3kgae).

Changed prior to commit:
  https://reviews.llvm.org/D135060?vs=469048&id=469259#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060

Files:
  clang/include/clang/Basic/AddressSpaces.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/DirectX.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/SPIR.h
  clang/lib/Basic/Targets/TCE.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/HLSL/group_shared.hlsl
  clang/test/CodeGenHLSL/group_shared.hlsl
  clang/test/Parser/opencl-cxx-keywords.cl
  clang/test/ParserHLSL/group_shared.hlsl
  clang/test/ParserHLSL/group_shared_202x.hlsl
  clang/test/SemaHLSL/group_shared.hlsl
  clang/test/SemaHLSL/group_shared_202x.hlsl
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/invalid-kernel.cl
  clang/test/SemaTemplate/address_space-dependent.cpp

Index: clang/test/SemaTemplate/address_space-dependent.cpp
===
--- clang/test/SemaTemplate/address_space-dependent.cpp
+++ clang/test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
 
 template 
 void tooBig() {
-  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388588)}}
+  __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388587)}}
 }
 
 template 
Index: clang/test/SemaOpenCL/invalid-kernel.cl
===
--- clang/test/SemaOpenCL/invalid-kernel.cl
+++ clang/test/SemaOpenCL/invalid-kernel.cl
@@ -13,14 +13,14 @@
   return 0;
 }
 
-int* global x(int* x) { // expected-error {{return value cannot be qualified with address space}}
+int* global x(int* x) { // expected-error {{return type cannot be qualified with address space}}
   return x + 1;
 }
 
-int* local x(int* x) { // expected-error {{return value cannot be qualified with address space}}
+int* local x(int* x) { // expected-error {{return type cannot be qualified with address space}}
   return x + 1;
 }
 
-int* constant x(int* x) { // expected-error {{return value cannot be qualified with address space}}
+int* constant x(int* x) { // expected-error {{return type cannot be qualified with address space}}
   return x + 1;
 }
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -231,12 +231,12 @@
 }
 #endif
 
-__private int func_return_priv(void);   //expected-error {{return value cannot be qualified with address space}}
-__global int func_return_global(void);  //expected-error {{return value cannot be qualified with address space}}
-__local int func_return_local(void);//expected-error {{return value cannot be qualified with address space}}
-__constant int func_return_constant(void);  //expected-error {{return value cannot be qualified with address space}}
+__private int func_return_priv(void);   //expected-error {{return type cannot be qualified with address space}}
+__global int func_return_global(void);  //expected-error {{return type cannot be qualified with address space}}
+__local int func_return_local(void);//expected-error {{return type cannot be qualified with address space}}
+__constant int func_return_constant(void);  //expected-error {{return type cannot be qualified with address space}}
 #if __OPENCL_C_VERSION__ >= 200
-__generic int func_return_generic(void);//expected-error {{return value cannot be qualified with address space}}
+__generic int func_return_generic(void);//expected-error {{return type cannot be qualified with address space}}
 #endif
 
 void func_multiple_addr(void) {
Index: clang/test/SemaHLSL/group_shared_202x.hlsl
===
--- /dev/null
+++ clang/test/SemaHLSL/group_shared_202x.hlsl
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -std=hlsl202x  -o - -fsyntax-only %s -verify
+
+// expected-error@+1 {{return type cannot be qualified with address space}}
+auto func()

[clang] 7e04c0a - [HLSL] Add groupshare address space.

[Xiang Li via cfe-commits]

Author: Xiang Li
Date: 2022-10-20T09:29:09-07:00
New Revision: 7e04c0ad632527df0a4c4d34a6ac6ec6a3888dfe

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

LOG: [HLSL] Add groupshare address space.

Added keyword, LangAS and TypeAttrbute for groupshared.

Tanslate it to LangAS with asHLSLLangAS.

Make sure it translated into address space 3 for DirectX target.

Reviewed By: aaron.ballman

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

Added: 
clang/test/AST/HLSL/group_shared.hlsl
clang/test/CodeGenHLSL/group_shared.hlsl
clang/test/ParserHLSL/group_shared.hlsl
clang/test/ParserHLSL/group_shared_202x.hlsl
clang/test/SemaHLSL/group_shared.hlsl
clang/test/SemaHLSL/group_shared_202x.hlsl

Modified: 
clang/include/clang/Basic/AddressSpaces.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Basic/TokenKinds.def
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/ParsedAttr.h
clang/lib/AST/ASTContext.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Basic/Targets/AMDGPU.cpp
clang/lib/Basic/Targets/DirectX.h
clang/lib/Basic/Targets/NVPTX.h
clang/lib/Basic/Targets/SPIR.h
clang/lib/Basic/Targets/TCE.h
clang/lib/Basic/Targets/X86.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseExprCXX.cpp
clang/lib/Parse/ParseTentative.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaLambda.cpp
clang/lib/Sema/SemaType.cpp
clang/test/Parser/opencl-cxx-keywords.cl
clang/test/SemaOpenCL/address-spaces.cl
clang/test/SemaOpenCL/invalid-kernel.cl
clang/test/SemaTemplate/address_space-dependent.cpp

Removed: 




diff  --git a/clang/include/clang/Basic/AddressSpaces.h 
b/clang/include/clang/Basic/AddressSpaces.h
index 99bb67fd26d1..2f2c5d5826bc 100644
--- a/clang/include/clang/Basic/AddressSpaces.h
+++ b/clang/include/clang/Basic/AddressSpaces.h
@@ -56,6 +56,9 @@ enum class LangAS : unsigned {
   ptr32_uptr,
   ptr64,
 
+  // HLSL specific address spaces.
+  hlsl_groupshared,
+
   // This denotes the count of language-specific address spaces and also
   // the offset added to the target-specific address spaces, which are usually
   // specified by address space attributes __attribute__(address_space(n))).

diff  --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 58ac67e92588..16cf932c3760 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4065,6 +4065,12 @@ def HLSLResource : InheritableAttr {
   let Documentation = [InternalOnly];
 }
 
+def HLSLGroupSharedAddressSpace : TypeAttr {
+  let Spellings = [Keyword<"groupshared">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [HLSLGroupSharedAddressSpaceDocs];
+}
+
 def RandomizeLayout : InheritableAttr {
   let Spellings = [GCC<"randomize_layout">];
   let Subjects = SubjectList<[Record]>;

diff  --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index 484052f4db8a..3b441e757fae 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6633,6 +6633,22 @@ The full documentation is available here: 
https://docs.microsoft.com/en-us/windo
   }];
 }
 
+def HLSLGroupSharedAddressSpaceDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+HLSL enables threads of a compute shader to exchange values via shared memory.
+HLSL provides barrier primitives such as GroupMemoryBarrierWithGroupSync,
+and so on to ensure the correct ordering of reads and writes to shared memory
+in the shader and to avoid data races.
+Here's an example to declare a groupshared variable.
+.. code-block:: c++
+
+  groupshared GSData data[5*5*1];
+
+The full documentation is available here: 
https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-syntax#group-shared
+  }];
+}
+
 def AnnotateTypeDocs : Documentation {
   let Category = DocCatType;
   let Heading = "annotate_type";

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1ef1f23e8c87..0396d6fae26a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10301,8 +10301,8 @@ def err_reference_pipe_type : Error <
 def err_opencl_no_main : Error<"%select{function|kernel}0 cannot be called 
'main'">;
 def err_opencl_kernel_attr :
   Error<"attribute %0 can only be applied to an OpenCL kernel function">;
-def err_opencl_return_value_with_address_space : Error<
-  "return value cannot be qualified with address space">;
+def err_return_value_with_address_space : Error<
+  "return type

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135557#3871482 , @dblaikie wrote:

> I was hoping the rephrasing (is this really a question about which ctors the 
> type has, or about how the type can be constructible) might offer us a way 
> out for this use case, at least - if it's about how the type is 
> constructible, then the AST wouldn't be the ideal/complete solution anyway 
> and we could move more towards exposing the 5 special member ops as "can I do 
> this thing/would this expression be valid".

FWIW, I'm about 99% sure there's no way to do that without use of `Sema`, which 
would require significant work to expose to the C indexing APIs and to AST 
matchers. At the AST level, we have "does this type have these methods" but it 
requires semantic analysis to determine whether something is copy constructible 
or not. e.g.,

  struct S {
S(S&) {}
  };

This class has a usable copy constructor, yet it is not copy constructible per 
the type trait: https://godbolt.org/z/srefPjzxj


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135557

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

[PATCH] D136134: [NFC] [DirectX backend] move ResourceClass into llvm.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:20
 #include "clang/Sema/Sema.h"
+#include "llvm/Frontend/HLSL/HLSLResource.h"
 

You need to add FrontendHLSL to the Sema/CMakeLists.txt file too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136134

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

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4374
+  ? llvm::DebuggerKind::Default
+  : DebuggerTuning);
 

probinson wrote:
> Seems like you should be able to return `DebuggerKind::Default` from 
> `getDefaultDebuggerTuning` then you wouldn't need this complication?
Ah thanks for the suggestion! 

We looked into an approach along this line in an earlier version (see 
https://reviews.llvm.org/D136187?id=468626), but it seemed that modifying the 
AIX default was not ideal. There are two drawbacks. 

1. It complicates [[ 
https://github.com/llvm/llvm-project/blob/7e04c0ad632527df0a4c4d34a6ac6ec6a3888dfe/clang/lib/Driver/ToolChains/Clang.cpp#L4267
 | setting ]] `gstrict-dwarf` and [[ 
https://github.com/llvm/llvm-project/blob/7e04c0ad632527df0a4c4d34a6ac6ec6a3888dfe/clang/lib/Driver/ToolChains/Clang.cpp#L4284
 | setting ]] `gno-column-info`. The earlier version had to revise those 
conditions.
2. It may complicate future development that relies on the AIX tuning 
defaulting to DBX. 

Therefore we ended up with this local change. That said, I am all ears for 
different ways to avoid the complication. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136187

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> (rabbit hole/tangent: Arguably what might be more useful to the user might be 
> a name that's not even usable in C++ - using the member variable names, as 
> well as the type names, though that'd be even more verbose... like 
> `t1` - which, I guess, if you had user defined types 
> for some kind of extra type safety, would get as verbose as 
> `t1` though I would expect 
> that would be the minority)

True, but if user defined type is defined as CRTP (very common for strong 
typedefs):

  template< typename T, typename U >
  struct StrongTypedef { T value; };
  
  struct Length : StrongTypedef< unsigned, Length > {};
  struct Width : StrongTypedef< unsigned, Width > {};
  
  struct Shape
  {
  Length length;
  Width  width;
  };
  
  template< Shape > struct S {};

you would get something like `S{.value = 50}}, .width = Width{StrongTypedef{.value = 70}}}>` (inspired by this GCC output 
), which is truly verbose. However, the 
current way of printing (assuming member names are printed) it would print 
something like `S<{.length = {{.value = 50}}, .width = {{.value = 70}}}>`. 
Ideally, in this scenario, probably the best possible output would be 
`S`. 
however, I'm not exactly sure how to achieve this (my patch would print 
`Shape`, but not `Length` and `Width` with my policy disabled.

Personally, I prefer full verbosity in all cases. Yes, it's a bit more to read, 
but does not confuse our less experienced c++ developers. This is why our 
internal build of clang has this enabled by default and I would be actually 
very happy if this patch gets accepted in a form where full types are always 
printed without the need for any new policies. If you are OK with that, I can 
update the patch. Just, please, let me know.




Comment at: clang/include/clang/AST/PrettyPrinter.h:78
 CleanUglifiedParameters(false), EntireContentsOfLargeArray(true),
-UseEnumerators(true) {}
+UseEnumerators(true), 
AlwaysIncludeTypeForNonTypeTemplateArgument(false) {}
 

aaron.ballman wrote:
> Should this be defaulting to false? I thought we wanted to always include the 
> type for NTTP printing?
I've set this to `false` by default to not interfere with the current behavior 
of the clang. 

However, after today's discussion, I think I can completely remove the policy 
and simply always print the full type. That would be option (1) from [[ 
https://reviews.llvm.org/D134453/new/#3869610 | this comment ]] and would make 
clang behave the same as GCC and MSVC.

If you are OK with that, I'll be happy to update the patch.



Comment at: clang/test/CodeGenCXX/debug-info-template.cpp:224
 template void f1();
-// CHECK: !DISubprogram(name: "f1", 
+// CHECK: !DISubprogram(name: "f1",
 // CHECK-SAME: templateParams: ![[RAW_FUNC_QUAL_ARGS:[0-9]*]],

dblaikie wrote:
> Looks like some unintended whitespace changes? (removing trailing whitespace 
> from these lines) 
Sorry about that. I've configured my editor to always remove trailing 
whitespace (we have this policy in the company) on save action. I can return 
the trailing whitespace if you want, although I would like to understand the 
reasoning for keeping the trailing whitespace...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 469270.
fhahn added a comment.

Rebase on top of current `main`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32199

Files:
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/Sanitizers.def
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTBAA.cpp
  clang/lib/CodeGen/SanitizerMetadata.cpp
  clang/lib/CodeGen/SanitizerMetadata.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/test/CodeGen/sanitize-type-attr.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
@@ -260,6 +260,28 @@
 // CHECK-ASAN-MYRIAD-NOT: "-lc"
 // CHECK-ASAN-MYRIAD: libclang_rt.asan-sparcel.a"
 
+// RUN: %clangxx %s -### -o %t.o 2>&1 \
+// RUN: --target=x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
+// RUN: -fsanitize=type \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-TYSAN-LINUX-CXX %s
+//
+// CHECK-TYSAN-LINUX-CXX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-TYSAN-LINUX-CXX-NOT: stdc++
+// CHECK-TYSAN-LINUX-CXX: "--whole-archive" "{{.*}}libclang_rt.tysan{{[^.]*}}.a" "--no-whole-archive"
+// CHECK-TYSAN-LINUX-CXX: stdc++
+
+// RUN: %clangxx -fsanitize=type -### %s 2>&1 \
+// RUN: -mmacosx-version-min=10.6 \
+// RUN: --target=x86_64-apple-darwin13.4.0 -fuse-ld=ld -stdlib=platform \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-TYSAN-DARWIN-CXX %s
+// CHECK-TYSAN-DARWIN-CXX: "{{.*}}ld{{(.exe)?}}"
+// CHECK-TYSAN-DARWIN-CXX: libclang_rt.tysan_osx_dynamic.dylib
+// CHECK-TYSAN-DARWIN-CXX-NOT: -lc++abi
+
 // RUN: %clangxx -### %s 2>&1 \
 // RUN: --target=x86_64-unknown-linux -fuse-ld=ld -stdlib=platform -lstdc++ \
 // RUN: -fsanitize=thread \
Index: clang/test/CodeGen/sanitize-type-attr.cpp
===
--- /dev/null
+++ clang/test/CodeGen/sanitize-type-attr.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type | FileCheck -check-prefix=TYSAN %s
+// RUN: echo "src:%s" | sed -e 's/\\//g' > %t
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=type -fsanitize-blacklist=%t | FileCheck -check-prefix=BL %s
+
+// The sanitize_type attribute should be attached to functions
+// when TypeSanitizer is enabled, unless no_sanitize("type") attribute
+// is present.
+
+// WITHOUT:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN1{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type"))) int NoTYSAN1(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN2{{.*}}) [[NOATTR]]
+// BL:  NoTYSAN2{{.*}}) [[NOATTR]]
+// TYSAN:  NoTYSAN2{{.*}}) [[NOATTR]]
+__attribute__((no_sanitize("type"))) int NoTYSAN2(int *a);
+int NoTYSAN2(int *a) { return *a; }
+
+// WITHOUT:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// BL:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+// TYSAN:  NoTYSAN3{{.*}}) [[NOATTR:#[0-9]+]]
+__attribute__((no_sanitize("type"))) int NoTYSAN3(int *a) { return *a; }
+
+// WITHOUT:  TYSANOk{{.*}}) [[NOATTR]]
+// BL:  TYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TYSANOk{{.*}}) [[WITH:#[0-9]+]]
+int TYSANOk(int *a) { return *a; }
+
+// WITHOUT:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// BL:  TemplateTYSANOk{{.*}}) [[NOATTR]]
+// TYSAN: TemplateTYSANOk{{.*}}) [[WITH]]
+template 
+int TemplateTYSANOk() { return i; }
+
+// WITHOUT:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// BL:  TemplateNoTYSAN{{.*}}) [[NOATTR]]
+// TYSAN: TemplateNoTYSAN{{.*}}) [[NOATTR]]
+template 
+__attribute__((no_sanitize("type"))) int TemplateNoTYSAN() { return i; }
+
+int force_instance = TemplateTYSANOk<42>() + TemplateNoTYSAN<42>();
+
+// Check that __cxx_global_var_init* get the sanitize_type attribute.
+int global1 = 0;
+int global2 = *(int *)((char *)&global1 + 1);
+// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// BL: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
+// TYSAN: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
+
+// Make sure that we don't add globals to the list for which we don't have a
+// specific type description.
+// FIXME: We now have a type description for this type and a global is added. Should it?
+struct SX {
+  int a, b;
+};
+SX sx;

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D136309#3871599 , @nickdesaulniers 
wrote:

> @MaskRay do you think I should make it so that `-g -g0` disables passing 
> through `-g`?

Looks like this pattern can be used:

  if (const Arg *A = Args.getLastArg(options::OPT_g_Group)) {
if (!A->getOption().matches(options::OPT_g0))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136134: [NFC] [DirectX backend] move ResourceClass into llvm.

[Xiang Li via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:20
 #include "clang/Sema/Sema.h"
+#include "llvm/Frontend/HLSL/HLSLResource.h"
 

beanz wrote:
> You need to add FrontendHLSL to the Sema/CMakeLists.txt file too.
Good catch.
Not sure why both local build and the pre-commit check cannot hit it. :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136134

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

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D135557#3871622 , @aaron.ballman 
wrote:

> In D135557#3871482 , @dblaikie 
> wrote:
>
>> I was hoping the rephrasing (is this really a question about which ctors the 
>> type has, or about how the type can be constructible) might offer us a way 
>> out for this use case, at least - if it's about how the type is 
>> constructible, then the AST wouldn't be the ideal/complete solution anyway 
>> and we could move more towards exposing the 5 special member ops as "can I 
>> do this thing/would this expression be valid".
>
> FWIW, I'm about 99% sure there's no way to do that without use of `Sema`, 
> which would require significant work to expose to the C indexing APIs and to 
> AST matchers.

Fair enough :/ not sure what to do about that.

> At the AST level, we have "does this type have these methods" but it requires 
> semantic analysis to determine whether something is copy constructible or 
> not. e.g.,
>
>   struct S {
> S(S&) {}
>   };
>
> This class has a usable copy constructor, yet it is not copy constructible 
> per the type trait: https://godbolt.org/z/srefPjzxj

Ah, indeed, the inverse case - technically a copy ctor, but not one you can use 
in the ways you'd expect. Perhaps another question for @anderslanglands to 
consider.

(I'm still sort of curious how the AST matchers deal with all this - I guess 
they must have Sema available, because I'd assume they make all sorts of 
queries like "is this constructible from that" - since they're often trying to 
generate new code and want to know what constructs will be valid, which is 
different from the indexing use case, admittedly)

Maybe the answer is the C API isn't for this sort of thing/it's too nuanced to 
expose there?

But if you reckon the inconsistency isn't so bad, I won't be up in arms if you 
decide to go with having the indexing C API instantiate all the implicit 
special members all the time. I can see the value/it's only restricted to the 
indexing API. Does seem a bit unfortunate in tetrms of consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135557

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Many -g options unfortunately enable/disable debug info emission: -g, -g[0123], 
-ggdb[0123], etc. The full rules are very complex. I think it makes sense to 
support a subset which is mostly likely used, i.e. -g, -g[0123]. So you may 
check `OPT_g_Group` and possibly reuse `DebugLevelToInfoKind` (if you want to 
support -ggdb0) or just hard code OPT_g0 if you just want to support -g0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Nick Desaulniers via Phabricator via cfe-commits]

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

handle -g0 positionally


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/as-options.s


Index: clang/test/Driver/as-options.s
===
--- clang/test/Driver/as-options.s
+++ clang/test/Driver/as-options.s
@@ -116,3 +116,13 @@
 // RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused
+
+// Test that -g is passed through to GAS.
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g %s -### 2>&1 | 
\
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g0 -g %s -### 
2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// DEBUG: "-g"
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g -g0 %s -### 
2>&1 | \
+// RUN:   FileCheck --check-prefix=NODEBUG %s
+// NODEBUG-NOT: "-g"
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -976,6 +976,10 @@
   for (const auto &II : Inputs)
 CmdArgs.push_back(II.getFilename());
 
+  if (Arg *A = Args.getLastArg(options::OPT_g_Flag, options::OPT_gN_Group))
+if (!A->getOption().matches(options::OPT_g0))
+  Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath(DefaultAssembler));
   C.addCommand(std::make_unique(JA, *this,


Index: clang/test/Driver/as-options.s
===
--- clang/test/Driver/as-options.s
+++ clang/test/Driver/as-options.s
@@ -116,3 +116,13 @@
 // RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused
+
+// Test that -g is passed through to GAS.
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g0 -g %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// DEBUG: "-g"
+// RUN: %clang --target=aarch64-linux-gnu -fno-integrated-as -g -g0 %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=NODEBUG %s
+// NODEBUG-NOT: "-g"
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -976,6 +976,10 @@
   for (const auto &II : Inputs)
 CmdArgs.push_back(II.getFilename());
 
+  if (Arg *A = Args.getLastArg(options::OPT_g_Flag, options::OPT_gN_Group))
+if (!A->getOption().matches(options::OPT_g0))
+  Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath(DefaultAssembler));
   C.addCommand(std::make_unique(JA, *this,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/as-options.s:120
+
+// Test that -g is passed through to GAS.
+// RUN: %clang -fno-integrated-as -g %s -### 2>&1 | \

nickdesaulniers wrote:
> MaskRay wrote:
> > -g can be tested along with other pass-through options. This way we test 
> > the relative order (though usually it doesn't matter but it improves the 
> > check) and decreases the number of clang invocations.
> Is the suggestion to:
> 1. combine this test with a pre-existing RUN line? or
> 2. Test all of the pass through options in this run line.
> 
> If 1, then I'm not sure which test best to combine this with.
If there are many independent pass-through options and one invocation suffices 
to test them all, you may try that. But I'll not insist on that if you think 
separate invocations is clearer.

If you add more debug info tests, consider whether other files may be more 
suitable (e.g. debug-options.c, clang-g-opts.c). For now I think as-options.s 
is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

(Note that assembler synthesized debug info for assembly files is a very minor 
feature. I don't know why the Linux kernel is so fond of it..).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D136309#3871769 , @MaskRay wrote:

> (Note that assembler synthesized debug info for assembly files is a very 
> minor feature. I don't know why the Linux kernel is so fond of it..).

This is _required_ for symbolicating symbols defined in assembler for stack 
traces. The primary use is for system wide profiling.  See also b/249023842 and 
b/236733195.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136309

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

[PATCH] D136090: Handle errors in expansion of response files

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 469286.
sepavloff added a comment.

Make UTF-16 string properly aligned


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136090

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/test/Driver/config-file-errs.c
  clang/tools/driver/driver.cpp
  clang/unittests/Driver/ToolChainTest.cpp
  flang/tools/flang-driver/driver.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -871,7 +871,7 @@
   llvm::BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true);
-  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
+  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
   EXPECT_THAT(Argv, testing::Pointwise(
 StringEquality(),
 {"test/test", "-flag_1", "-option_1", "-option_2",
@@ -933,7 +933,14 @@
 #endif
   llvm::cl::ExpansionContext ECtx(A, Tokenizer);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot);
-  ASSERT_FALSE(ECtx.expandResponseFiles(Argv));
+  llvm::Error Err = ECtx.expandResponseFiles(Argv);
+  ASSERT_TRUE((bool)Err);
+  SmallString<128> FilePath = SelfFilePath;
+  std::error_code EC = FS.makeAbsolute(FilePath);
+  ASSERT_FALSE((bool)EC);
+  std::string ExpectedMessage =
+  std::string("recursive expansion of: '") + std::string(FilePath) + "'";
+  ASSERT_TRUE(toString(std::move(Err)) == ExpectedMessage);
 
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(),
@@ -971,7 +978,7 @@
   BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot);
-  ASSERT_FALSE(ECtx.expandResponseFiles(Argv));
+  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
   ASSERT_EQ(Argv.size(), 1 + NON_RSP_AT_ARGS + 2);
@@ -1005,7 +1012,7 @@
   BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true);
-  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
+  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
   EXPECT_THAT(Argv,
   testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
 }
@@ -1025,7 +1032,7 @@
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeWindowsCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setMarkEOLs(true).setRelativeNames(
   true);
-  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
+  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
 "input.cpp"};
   ASSERT_EQ(std::size(Expected), Argv.size());
@@ -1038,6 +1045,30 @@
   }
 }
 
+TEST(CommandLineTest, BadResponseFile) {
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  TempDir ADir("dir", /*Unique*/ true);
+  SmallString<128> AFilePath = ADir.path();
+  llvm::sys::path::append(AFilePath, "file.rsp");
+  std::string AFileExp = std::string("@") + std::string(AFilePath.str());
+  SmallVector Argv = {"clang", AFileExp.c_str()};
+
+  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
+  ASSERT_TRUE(Res);
+  ASSERT_EQ(2U, Argv.size());
+  ASSERT_STREQ(Argv[0], "clang");
+  ASSERT_STREQ(Argv[1], AFileExp.c_str());
+
+  std::string ADirExp = std::string("@") + std::string(ADir.path());
+  Argv = {"clang", ADirExp.c_str()};
+  Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
+  ASSERT_FALSE(Res);
+  ASSERT_EQ(2U, Argv.size());
+  ASSERT_STREQ(Argv[0], "clang");
+  ASSERT_STREQ(Argv[1], ADirExp.c_str());
+}
+
 TEST(CommandLineTest, SetDefaultValue) {
   cl::ResetCommandLineParser();
 
@@ -1145,9 +1176,9 @@
 
   llvm::BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::tokenizeConfigFile);
-  bool Result = ECtx.readConfigFile(ConfigFile.path(), Argv);
+  llvm::Error Result = ECtx.readConfigFile(ConfigFile.path(), Argv);
 
-  EXPECT_TRUE(Result);
+  EXPECT_FALSE((bool)Result);
   EXPECT_EQ(Argv.size(), 13U);
   EXPECT_STREQ(Argv[0], "-option_1");
   EXPECT_STREQ(Argv[1],
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1153,14 +1153,14 @@
 }
 
 // FName must be an absolute path.
-llvm::Error
-ExpansionContext::expandResponseFile(StringRef FName,
- SmallVectorImpl &NewArgv) {
+Error ExpansionCont

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135557#3871716 , @dblaikie wrote:

> (I'm still sort of curious how the AST matchers deal with all this - I guess 
> they must have Sema available, because I'd assume they make all sorts of 
> queries like "is this constructible from that" - since they're often trying 
> to generate new code and want to know what constructs will be valid, which is 
> different from the indexing use case, admittedly)

Nope, AST matchers don't have Sema available, so they don't have a way to query 
"is this constructible from that". You've got the AST itself (and the 
`ASTContext`) and not a whole lot else beyond that.

> Maybe the answer is the C API isn't for this sort of thing/it's too nuanced 
> to expose there?
>
> But if you reckon the inconsistency isn't so bad, I won't be up in arms if 
> you decide to go with having the indexing C API instantiate all the implicit 
> special members all the time. I can see the value/it's only restricted to the 
> indexing API. Does seem a bit unfortunate in tetrms of consistency.

Yeah, I think you convinced me that the consistency issue is something we 
should be wary of. I don't think we want to get into a situation where C 
index/AST matching is fundamentally a different AST than the rest of the 
compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135557

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

[PATCH] D136355: [clang][Sema] Fix caret position to be on the non null parameter

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the fix!

Can you add test coverage to show the behavioral change? You can either 
position the problematic null argument on its own line (so you can validate the 
correct position) or use `-fdiagnostics-print-source-range-info` to check a 
specific range. Also, can you add a release note about the fix to 
`clang/docs/ReleaseNotes.rst`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136355

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

[PATCH] D136134: [NFC] [DirectX backend] move ResourceClass into llvm.

[Xiang Li via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:20
 #include "clang/Sema/Sema.h"
+#include "llvm/Frontend/HLSL/HLSLResource.h"
 

python3kgae wrote:
> beanz wrote:
> > You need to add FrontendHLSL to the Sema/CMakeLists.txt file too.
> Good catch.
> Not sure why both local build and the pre-commit check cannot hit it. :(
I think the reason it works is that it only used the enum decl in the header, 
not anything which needs to link.
Do we need to add FrontendHLSL to CMakeLists in this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136134

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

[PATCH] D136022: [clang] Add time profile for constant evaluation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/unittests/Support/TimeProfilerTest.cpp:197-198
+
+  // NOTE: If this test is failing, run this test with
+  // `llvm::errs() << TraceGraph;` and change the assert above.
+}

This bit worries me because I suspect we'll get pretty wide variation between 
test bots in the build lab. Do you have an idea of how likely it is that this 
test will have different behavior depending on the machine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136022

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