[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 216520.
rsmith marked 5 inline comments as done.
rsmith added a comment.

- Address review comments from Aaron.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/Stack.h
  include/clang/Sema/Sema.h
  lib/Basic/CMakeLists.txt
  lib/Basic/Stack.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/CMakeLists.txt
  test/SemaTemplate/stack-exhaustion.cpp
  test/lit.cfg.py
  test/lit.site.cfg.py.in
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
@@ -319,6 +320,7 @@
 }
 
 int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
   llvm::InitLLVM X(argc_, argv_);
   SmallVector argv(argv_, argv_ + argc_);
 
Index: test/lit.site.cfg.py.in
===
--- test/lit.site.cfg.py.in
+++ test/lit.site.cfg.py.in
@@ -25,6 +25,7 @@
 config.enable_shared = @ENABLE_SHARED@
 config.enable_backtrace = @ENABLE_BACKTRACES@
 config.enable_experimental_new_pass_manager = @ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER@
+config.enable_threads = @LLVM_ENABLE_THREADS@
 config.host_arch = "@HOST_ARCH@"
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.use_z3_solver = lit_config.params.get('USE_Z3_SOLVER', "@USE_Z3_SOLVER@")
Index: test/lit.cfg.py
===
--- test/lit.cfg.py
+++ test/lit.cfg.py
@@ -175,6 +175,9 @@
 if config.enable_backtrace:
 config.available_features.add('backtrace')
 
+if config.enable_threads:
+config.available_features.add('thread_support')
+
 # Check if we should allow outputs to console.
 run_console_tests = int(lit_config.params.get('enable_console', '0'))
 if run_console_tests != 0:
Index: test/SemaTemplate/stack-exhaustion.cpp
===
--- /dev/null
+++ test/SemaTemplate/stack-exhaustion.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -verify %s
+// REQUIRES: thread_support
+
+// expected-warning@* 0-1{{stack nearly exhausted}}
+// expected-note@* 0+{{}}
+
+template struct X : X {};
+template<> struct X<0> {};
+X<1000> x;
+
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) {} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }
+
+int f(X<0>);
+template auto f(X) -> f(X());
+
+int k = f(X<1000>());
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -26,7 +26,8 @@
   ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER
   HAVE_LIBZ
   LLVM_ENABLE_PER_TARGET_RUNTIME_DIR
-  LLVM_ENABLE_PLUGINS)
+  LLVM_ENABLE_PLUGINS
+  LLVM_ENABLE_THREADS)
 
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7715,7 +7715,9 @@
 auto *Def = Var->getDefinition();
 if (!Def) {
   SourceLocation PointOfInstantiation = E->getExprLoc();
-  InstantiateVariableDefinition(PointOfInstantiation, Var);
+  runWithSufficientStackSpace(PointOfInstantiation, [&] {
+InstantiateVariableDefinition(PointOfInstantiation, Var);
+  });
   Def = Var->getDefinition();
 
   // If we don't already have a point of instantiation, and we managed
@@ -8053,9 +8055,11 @@
 } else if (auto *ClassTemplateSpec =
 dyn_cast(RD)) {
   if (ClassTemplateSpec->getSpecializationKind() == TSK_Undeclared) {
-Diagnosed = InstantiateClassTemplateSpecialization(
-Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
-/*Complain=*/Diagnoser);
+runWithSufficientStackSpace(Loc, [&] {
+  Diagnosed = InstantiateClassTemplateSpecialization(
+  Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
+  /*Complain=*/Diagnoser);
+});
 Instantiated = true;
   }
 } else {
@@ -8066,10 +8070,12 @@
 // This record was instantiated from a class within a template.
 if (MSI->getTemplateSpecializationKind() !=
 TSK_ExplicitSpecialization) {
-  

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:14
 let Component = "Sema" in {
-let CategoryName = "Semantic Issue" in {
+def warn_stack_exhausted : Warning<
+  "stack nearly exhausted; compilation time may suffer, and "

aaron.ballman wrote:
> Should this be a Sema warning as opposed to a Basic warning? It seems to me 
> that we may want to guard against similar stack exhaustion from the parser as 
> well, wouldn't we?
Sure, moved to DiagnosticCommonKinds.td. We have no uses of it outside Sema 
yet, but it's definitely plausible that some would be added.



Comment at: lib/Sema/SemaExpr.cpp:15070-15079
+  // Trivial default constructors and destructors are never actually used.
+  // FIXME: What about other special members?
+  if (Func->isTrivial() && !Func->hasAttr() &&
+  OdrUse == OdrUseContext::Used) {
+if (auto *Constructor = dyn_cast(Func))
+  if (Constructor->isDefaultConstructor())
+OdrUse = OdrUseContext::FormallyOdrUsed;

aaron.ballman wrote:
> This seems unrelated to the patch?
I agree it seems that way, but it is related:

The block of code below that got turned into a lambda contains early exits via 
`return` for the cases that get downgraded to `FormallyOdrUsed` here. We used 
to bail out of the whole function and not mark these trivial special member 
functions as "used" (in the code after the `NeedDefinition && !Func->getBody()` 
condition), which seems somewhat reasonable since we don't actually need 
definitions of them; other parts of Clang (specifically the static analyzer) 
have developed a reliance on that behavior.

So this is preserving the existing behavior and (hopefully) making it more 
obvious what that behavior is, rather than getting that behavior by an early 
exit from the "need a definition?" portion of this function leaking into the 
semantics of the "mark used" portion.

I can split this out and make this change first if you like.



Comment at: test/SemaTemplate/stack-exhaustion.cpp:10
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) 
{} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }

rsmith wrote:
> rnk wrote:
> > This test seems like it could be fragile. If threads are disabled, it will 
> > probably crash.
> I've handled the "threads are disabled" case. Is there anything else that can 
> meaningfully be detected here? (ASan maybe? Are there any other cases that 
> lead to a non-linear stack layout that we can detect?)
I've read through the ASan UAR design and played with some testcases, and I 
think this is fine. ASan will allocate the locals for some frames on the heap 
instead of on the stack, but that's OK, it just might mean that the stack usage 
grows more slowly. The stack pointer still points to the real stack, 
`__builtin_frame_address(0)` still returns a stack pointer (and critically, not 
a pointer to the ASan-heap-allocated frame), and we can still detect when we're 
getting close to stack exhaustion -- it just happens more slowly in this mode 
because we have another pool of places to allocate frames that's tried before 
we fall back to the stack.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361



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

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The `ALPHA_OFFSET` code seems to be unnecessarily "clever" to me. I think the 
warning can be suppressed by reversing the operands:

`ALPHA_OFFSET ^ 2`

But if I were maintaining that code I would get rid of the xor hack entirely 
and use

  #define CHANNEL_OFFSET(i) (i)

or

  #define CHANNEL_OFFSET(i) (3-(i))


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

https://reviews.llvm.org/D66397



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

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D66364#1633981 , @aaron.ballman 
wrote:

> My motivation is for portability. _Thread_local (and all the rest) do not 
> exist in C99 or earlier (or C++), so having some way to warn users of that is 
> useful. I agree that we should be consistent and go with all or none, but my 
> preference is for all (esp since this is a -pedantic warning class).


OK, if the motivation is to catch cases where people thought they were writing 
portable C99 code, but they weren't then I can see this being a useful warning. 
And that suggests that we should warn on all C11 `_Keywords` when used in C99 
or earlier (or in C++). And I suppose it's reasonable to split the hair between 
"this is code that someone might think is valid C99" (eg, use of 
`_Static_assert`) and "this is code that is using language extensions that 
no-one is likely to think is valid C99" (eg, use of `__builtin_*`).

That said, this direction will presumably mean that we start to reject (eg) 
libc++ when built with `-Wsystem-headers -pedantic-errors` (because it uses 
`_Atomic` to implement `std::atomic`), which doesn't seem ideal to me. Do you 
have any thoughts on that? Maybe we should change libc++ to use `__extension__` 
in those instances?


Repository:
  rC Clang

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

https://reviews.llvm.org/D66364



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

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

`_Thread_local` is a reserved identifier; we generally don't produce extension 
warnings for uses of reserved identifiers. (Eg, there's no warning for 
`_Atomic` in C++ or `_Bool` in C89, and no warning for uses of `__type_traits` 
or `__builtins`.)

But I note that we *do* warn for some of these already (eg, `_Generic`, 
`_Static_assert`, `_Alignas`, and `_Alignof` get a warning). We should choose a 
rule and apply it consistently.

What's the motivation for warning on this? Maybe that can inform whether these 
warnings are useful in general.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66364



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

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Basic/Stack.cpp:53-54
+  // If the stack pointer has a surprising value, we do not understand this
+  // stack usage scheme. (Perhaps the target allocates new stack regions on
+  // demand for us.) Don't try to guess what's going on.
+  if (StackUsage > DesiredStackSize)

rnk wrote:
> So, for example, ASan with UAR, where frames are heap allocated. I suppose in 
> that case we will go down the __builtin_frame_address path, though.
Can we detect that? It seems better to turn this all off if we think there's 
anything funny going on with the stack layout.



Comment at: test/SemaTemplate/stack-exhaustion.cpp:10
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) 
{} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }

rnk wrote:
> This test seems like it could be fragile. If threads are disabled, it will 
> probably crash.
I've handled the "threads are disabled" case. Is there anything else that can 
meaningfully be detected here? (ASan maybe? Are there any other cases that lead 
to a non-linear stack layout that we can detect?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361



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

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 215680.
rsmith added a comment.

- Disable stack exhaustion test if threads are disabled.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/Stack.h
  include/clang/Sema/Sema.h
  lib/Basic/CMakeLists.txt
  lib/Basic/Stack.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/CMakeLists.txt
  test/SemaTemplate/stack-exhaustion.cpp
  test/lit.cfg.py
  test/lit.site.cfg.py.in
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
@@ -319,6 +320,7 @@
 }
 
 int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
   llvm::InitLLVM X(argc_, argv_);
   SmallVector argv(argv_, argv_ + argc_);
 
Index: test/lit.site.cfg.py.in
===
--- test/lit.site.cfg.py.in
+++ test/lit.site.cfg.py.in
@@ -25,6 +25,7 @@
 config.enable_shared = @ENABLE_SHARED@
 config.enable_backtrace = @ENABLE_BACKTRACES@
 config.enable_experimental_new_pass_manager = @ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER@
+config.enable_threads = @LLVM_ENABLE_THREADS@
 config.host_arch = "@HOST_ARCH@"
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.use_z3_solver = lit_config.params.get('USE_Z3_SOLVER', "@USE_Z3_SOLVER@")
Index: test/lit.cfg.py
===
--- test/lit.cfg.py
+++ test/lit.cfg.py
@@ -175,6 +175,9 @@
 if config.enable_backtrace:
 config.available_features.add('backtrace')
 
+if config.enable_threads:
+config.available_features.add('thread_support')
+
 # Check if we should allow outputs to console.
 run_console_tests = int(lit_config.params.get('enable_console', '0'))
 if run_console_tests != 0:
Index: test/SemaTemplate/stack-exhaustion.cpp
===
--- /dev/null
+++ test/SemaTemplate/stack-exhaustion.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -verify %s
+// REQUIRES: thread_support
+
+// expected-warning@* 0-1{{stack nearly exhausted}}
+// expected-note@* 0+{{}}
+
+template struct X : X {};
+template<> struct X<0> {};
+X<1000> x;
+
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) {} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }
+
+int f(X<0>);
+template auto f(X) -> f(X());
+
+int k = f(X<1000>());
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -26,7 +26,8 @@
   ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER
   HAVE_LIBZ
   LLVM_ENABLE_PER_TARGET_RUNTIME_DIR
-  LLVM_ENABLE_PLUGINS)
+  LLVM_ENABLE_PLUGINS
+  LLVM_ENABLE_THREADS)
 
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7715,7 +7715,9 @@
 auto *Def = Var->getDefinition();
 if (!Def) {
   SourceLocation PointOfInstantiation = E->getExprLoc();
-  InstantiateVariableDefinition(PointOfInstantiation, Var);
+  runWithSufficientStackSpace(PointOfInstantiation, [&] {
+InstantiateVariableDefinition(PointOfInstantiation, Var);
+  });
   Def = Var->getDefinition();
 
   // If we don't already have a point of instantiation, and we managed
@@ -8053,9 +8055,11 @@
 } else if (auto *ClassTemplateSpec =
 dyn_cast(RD)) {
   if (ClassTemplateSpec->getSpecializationKind() == TSK_Undeclared) {
-Diagnosed = InstantiateClassTemplateSpecialization(
-Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
-/*Complain=*/Diagnoser);
+runWithSufficientStackSpace(Loc, [&] {
+  Diagnosed = InstantiateClassTemplateSpecialization(
+  Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
+  /*Complain=*/Diagnoser);
+});
 Instantiated = true;
   }
 } else {
@@ -8066,10 +8070,12 @@
 // This record was instantiated from a class within a template.
 if (MSI->getTemplateSpecializationKind() !=
 TSK_ExplicitSpecialization) {
-  Diagnosed = InstantiateClass(Loc, RD, Pattern,
-

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 215678.
rsmith marked 2 inline comments as done.
rsmith added a comment.

- Review feedback: use _AddressOfReturnAddress with MSVC, and simplify


Repository:
  rC Clang

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

https://reviews.llvm.org/D66361

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/Stack.h
  include/clang/Sema/Sema.h
  lib/Basic/CMakeLists.txt
  lib/Basic/Stack.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/SemaTemplate/stack-exhaustion.cpp
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
@@ -319,6 +320,7 @@
 }
 
 int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
   llvm::InitLLVM X(argc_, argv_);
   SmallVector argv(argv_, argv_ + argc_);
 
Index: test/SemaTemplate/stack-exhaustion.cpp
===
--- /dev/null
+++ test/SemaTemplate/stack-exhaustion.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -verify %s
+// expected-warning@* 0-1{{stack nearly exhausted}}
+// expected-note@* 0+{{}}
+
+template struct X : X {};
+template<> struct X<0> {};
+X<1000> x;
+
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) {} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }
+
+int f(X<0>);
+template auto f(X) -> f(X());
+
+int k = f(X<1000>());
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7715,7 +7715,9 @@
 auto *Def = Var->getDefinition();
 if (!Def) {
   SourceLocation PointOfInstantiation = E->getExprLoc();
-  InstantiateVariableDefinition(PointOfInstantiation, Var);
+  runWithSufficientStackSpace(PointOfInstantiation, [&] {
+InstantiateVariableDefinition(PointOfInstantiation, Var);
+  });
   Def = Var->getDefinition();
 
   // If we don't already have a point of instantiation, and we managed
@@ -8053,9 +8055,11 @@
 } else if (auto *ClassTemplateSpec =
 dyn_cast(RD)) {
   if (ClassTemplateSpec->getSpecializationKind() == TSK_Undeclared) {
-Diagnosed = InstantiateClassTemplateSpecialization(
-Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
-/*Complain=*/Diagnoser);
+runWithSufficientStackSpace(Loc, [&] {
+  Diagnosed = InstantiateClassTemplateSpecialization(
+  Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
+  /*Complain=*/Diagnoser);
+});
 Instantiated = true;
   }
 } else {
@@ -8066,10 +8070,12 @@
 // This record was instantiated from a class within a template.
 if (MSI->getTemplateSpecializationKind() !=
 TSK_ExplicitSpecialization) {
-  Diagnosed = InstantiateClass(Loc, RD, Pattern,
-   getTemplateInstantiationArgs(RD),
-   TSK_ImplicitInstantiation,
-   /*Complain=*/Diagnoser);
+  runWithSufficientStackSpace(Loc, [&] {
+Diagnosed = InstantiateClass(Loc, RD, Pattern,
+ getTemplateInstantiationArgs(RD),
+ TSK_ImplicitInstantiation,
+ /*Complain=*/Diagnoser);
+  });
   Instantiated = true;
 }
   }
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3415,7 +3415,11 @@
   if (D->isInvalidDecl())
 return nullptr;
 
-  return Instantiator.Visit(D);
+  Decl *SubstD;
+  runWithSufficientStackSpace(D->getLocation(), [&] {
+SubstD = Instantiator.Visit(D);
+  });
+  return SubstD;
 }
 
 /// Instantiates a nested template parameter list in the current
Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/Stack.h"
 #include 

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added reviewers: rnk, aaron.ballman.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.

Clang performs various recursive operations (such as template instantiation),
and may use non-trivial amounts of stack space in each recursive step (for
instance, due to recursive AST walks). While we try to keep the stack space
used by such steps to a minimum and we have explicit limits on the number of
such steps we perform, it's impractical to guarantee that we won't blow out the
stack on deeply recursive template instantiations on complex ASTs, even with
only a moderately high instantiation depth limit.

The user experience in these cases is generally terrible: we crash with
no hint of what went wrong. Under this patch, we attempt to do better:

- Detect when the stack is nearly exhausted, and produce a warning with a nice 
template instantiation backtrace, telling the user that we might run slowly or 
crash.
- For cases where we're forced to trigger recursive template instantiation in 
arbitrarily-deeply-nested contexts, check whether we're nearly out of stack 
space and allocate a new stack (by spawning a new thread) after producing the 
warning.


Repository:
  rC Clang

https://reviews.llvm.org/D66361

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/Stack.h
  include/clang/Sema/Sema.h
  lib/Basic/CMakeLists.txt
  lib/Basic/Stack.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/SemaTemplate/stack-exhaustion.cpp
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
@@ -319,6 +320,7 @@
 }
 
 int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
   llvm::InitLLVM X(argc_, argv_);
   SmallVector argv(argv_, argv_ + argc_);
 
Index: test/SemaTemplate/stack-exhaustion.cpp
===
--- /dev/null
+++ test/SemaTemplate/stack-exhaustion.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -verify %s
+// expected-warning@* 0-1{{stack nearly exhausted}}
+// expected-note@* 0+{{}}
+
+template struct X : X {};
+template<> struct X<0> {};
+X<1000> x;
+
+template struct tuple {};
+template auto f(tuple t) -> decltype(f(tuple(t))) {} // expected-error {{exceeded maximum depth}}
+void g() { f(tuple()); }
+
+int f(X<0>);
+template auto f(X) -> f(X());
+
+int k = f(X<1000>());
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7715,7 +7715,9 @@
 auto *Def = Var->getDefinition();
 if (!Def) {
   SourceLocation PointOfInstantiation = E->getExprLoc();
-  InstantiateVariableDefinition(PointOfInstantiation, Var);
+  runWithSufficientStackSpace(PointOfInstantiation, [&] {
+InstantiateVariableDefinition(PointOfInstantiation, Var);
+  });
   Def = Var->getDefinition();
 
   // If we don't already have a point of instantiation, and we managed
@@ -8053,9 +8055,11 @@
 } else if (auto *ClassTemplateSpec =
 dyn_cast(RD)) {
   if (ClassTemplateSpec->getSpecializationKind() == TSK_Undeclared) {
-Diagnosed = InstantiateClassTemplateSpecialization(
-Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
-/*Complain=*/Diagnoser);
+runWithSufficientStackSpace(Loc, [&] {
+  Diagnosed = InstantiateClassTemplateSpecialization(
+  Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
+  /*Complain=*/Diagnoser);
+});
 Instantiated = true;
   }
 } else {
@@ -8066,10 +8070,12 @@
 // This record was instantiated from a class within a template.
 if (MSI->getTemplateSpecializationKind() !=
 TSK_ExplicitSpecialization) {
-  Diagnosed = InstantiateClass(Loc, RD, Pattern,
-   getTemplateInstantiationArgs(RD),
-   TSK_ImplicitInstantiation,
-   /*Complain=*/Diagnoser);
+  runWithSufficientStackSpace(Loc, [&] {
+Diagnosed = InstantiateClass(Loc, RD, Pattern,
+ getTemplateInstantiationArgs(RD),
+ TSK_ImplicitInstantiation,
+ 

[PATCH] D63889: Check possible warnings on global initializers for reachability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1684-1686
+  void pushDeclForInitializer(Decl *D) { DeclForInitializer.push_back(D); }
+
+  void popDeclForInitializer() { DeclForInitializer.pop_back(); }

I don't think a simple list of these can work. Consider a case like:

```
auto x = [] {
  // something with a runtime diagnostic
};
```

Here, we'll have a `DeclForInitializer` set, so we'll suppress warnings on the 
body of the lambda, but we shouldn't: the body of the lambda is a completely 
different evaluation context from the initialization of the variable `x`.

Can you store the declaration on the `ExpressionEvaluationContextRecord` 
instead of storing a list of them directly on `Sema`? (You shouldn't need a 
list there, just a single pointer should work, since we can't parse two nested 
initializers without an intervening evaluation context.)



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2015
+
+  bool analyzed = false;
+

Please capitalize the names of variables.



Comment at: clang/lib/Sema/SemaExpr.cpp:16685-16690
 if (auto *VD = dyn_cast_or_null(
 ExprEvalContexts.back().ManglingContextDecl)) {
   if (VD->isConstexpr() ||
   (VD->isStaticDataMember() && VD->isFirstDecl() && !VD->isInline()))
 break;
+}

Please get rid of the pre-existing hacky use of `ManglingContextDecl` here and 
move this into the `getDeclForInitializer` block below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63889



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

[PATCH] D65695: Implements CWG 1601 in [over.ics.rank/4.2]

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:3758
+static QualType
+getFixedEnumUnderlayingType(const StandardConversionSequence ) {
+

Underlaying -> Underlying (here and below)



Comment at: clang/lib/Sema/SemaOverload.cpp:3821-3828
+  QualType UnderlayingType1 = getFixedEnumUnderlayingType(SCS1);
+  QualType UnderlayingType2 = getFixedEnumUnderlayingType(SCS2);
+  if (!UnderlayingType1.isNull() && !UnderlayingType2.isNull()) {
+if (SCS1.getToType(1) == UnderlayingType1 &&
+SCS2.getToType(1) != UnderlayingType2)
+  return ImplicitConversionSequence::Better;
+else if (SCS1.getToType(1) != UnderlayingType1 &&

I think this would be cleaner if you moved the comparison of the target type 
against the underlying type into `getFixedEnumUnderlyingType`, and changed it 
to return an enumeration of "not a fixed enum promotion", or "fixed enum 
promotion to underlying type", or "fixed enum promotion to promoted underlying 
type".



Comment at: clang/test/CXX/drs/dr16xx.cpp:26
 
+namespace dr1601 { // dr1601: 10 c++11
+#if __cplusplus >= 201103L

No need for the "c++11" marker here. (We accept fixed underlying types in C++98 
as an extension, and your change applies there too.)


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

https://reviews.llvm.org/D65695



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

[PATCH] D66040: [Sema] Implement DR2386 for C++17 structured binding

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LGTM with minor adjustments to the test.




Comment at: clang/test/CXX/drs/dr23xx.cpp:43-57
+#if __cplusplus >= 201707L
+// Otherwise, if the qualified-id std::tuple_size names a complete class
+// type **with a member value**, the expression std::tuple_size::value shall
+// be a well-formed integral constant expression
+namespace std {
+template  struct tuple_size;
+struct Bad1 { int a, b; };

Please add a comment

```
// dr2386: 9
```

so that the script that generates cxx_dr_status.html knows to mark that issue 
as done.

It'd be good to also move as much of this test into a `namespace dr2386` as 
possible. (Clearly some parts of it need to be in `namespace std`, but I'd 
prefer that those parts be kept as small as possible to isolate this test from 
others in the same file.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66040



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

[PATCH] D66100: Add __has_builtin support for builtin function-like type traits.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368785: Add __has_builtin support for builtin function-like 
type traits. (authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66100?vs=214673=215017#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66100

Files:
  cfe/trunk/docs/LanguageExtensions.rst
  cfe/trunk/include/clang/Basic/Features.def
  cfe/trunk/include/clang/Basic/TokenKinds.def
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/test/Preprocessor/feature_tests.c
  cfe/trunk/test/Preprocessor/feature_tests.cpp

Index: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
===
--- cfe/trunk/lib/Lex/PPMacroExpansion.cpp
+++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp
@@ -1617,21 +1617,38 @@
 return true;
   }
   return true;
+} else if (II->getTokenID() != tok::identifier ||
+   II->hasRevertedTokenIDToIdentifier()) {
+  // Treat all keywords that introduce a custom syntax of the form
+  //
+  //   '__some_keyword' '(' [...] ')'
+  //
+  // as being "builtin functions", even if the syntax isn't a valid
+  // function call (for example, because the builtin takes a type
+  // argument).
+  if (II->getName().startswith("__builtin_") ||
+  II->getName().startswith("__is_") ||
+  II->getName().startswith("__has_"))
+return true;
+  return llvm::StringSwitch(II->getName())
+  .Case("__array_rank", true)
+  .Case("__array_extent", true)
+  .Case("__reference_binds_to_temporary", true)
+  .Case("__underlying_type", true)
+  .Default(false);
 } else {
   return llvm::StringSwitch(II->getName())
-  .Case("__make_integer_seq", LangOpts.CPlusPlus)
-  .Case("__type_pack_element", LangOpts.CPlusPlus)
-  .Case("__builtin_available", true)
-  .Case("__is_target_arch", true)
-  .Case("__is_target_vendor", true)
-  .Case("__is_target_os", true)
-  .Case("__is_target_environment", true)
-  .Case("__builtin_LINE", true)
-  .Case("__builtin_FILE", true)
-  .Case("__builtin_FUNCTION", true)
-  .Case("__builtin_COLUMN", true)
-  .Case("__builtin_bit_cast", true)
-  .Default(false);
+  // Report builtin templates as being builtins.
+  .Case("__make_integer_seq", LangOpts.CPlusPlus)
+  .Case("__type_pack_element", LangOpts.CPlusPlus)
+  // Likewise for some builtin preprocessor macros.
+  // FIXME: This is inconsistent; we usually suggest detecting
+  // builtin macros via #ifdef. Don't add more cases here.
+  .Case("__is_target_arch", true)
+  .Case("__is_target_vendor", true)
+  .Case("__is_target_os", true)
+  .Case("__is_target_environment", true)
+  .Default(false);
 }
   });
   } else if (II == Ident__is_identifier) {
Index: cfe/trunk/docs/LanguageExtensions.rst
===
--- cfe/trunk/docs/LanguageExtensions.rst
+++ cfe/trunk/docs/LanguageExtensions.rst
@@ -38,7 +38,9 @@
 -
 
 This function-like macro takes a single identifier argument that is the name of
-a builtin function.  It evaluates to 1 if the builtin is supported or 0 if not.
+a builtin function, a builtin pseudo-function (taking one or more type
+arguments), or a builtin template.
+It evaluates to 1 if the builtin is supported or 0 if not.
 It can be used like this:
 
 .. code-block:: c++
@@ -55,6 +57,14 @@
   #endif
   ...
 
+.. note::
+
+  Prior to Clang 10, ``__has_builtin`` could not be used to detect most builtin
+  pseudo-functions.
+
+  ``__has_builtin`` should not be used to detect support for a builtin macro;
+  use ``#ifdef`` instead.
+
 .. _langext-__has_feature-__has_extension:
 
 ``__has_feature`` and ``__has_extension``
@@ -1041,8 +1051,8 @@
 
 More information could be found `here `_.
 
-Checks for Type Trait Primitives
-
+Type Trait Primitives
+=
 
 Type trait primitives are special builtin constant expressions that can be used
 by the standard C++ library to facilitate or simplify the implementation of
@@ -1058,20 +1068,173 @@
 
 Clang supports the `GNU C++ type traits
 `_ and a subset of the
-`Microsoft Visual C++ Type traits

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I would like to better understand the big picture for descriptors, pointers, 
and so on. I'm not yet seeing how the pieces are going to fit together and not 
frequently require expensive operations. For example, pointer arithmetic 
requires determining the array bound of the pointee; pointer subtraction 
requires determining whether two pointers point into the same array; pointer 
comparisons require determining the divergence point in the access paths 
between two pointers. These path-based operations were easy in the old 
representation, and seem to be difficult in the new representation, so I want 
to make sure that they've been properly considered. It's also not clear to me 
how you'll model pointers that have lost their designator (through constant 
folding), where you have a runtime offset but no compile-time offset, or 
pointers that point to objects whose values are not part of the evaluation (for 
example, `extern` globals), or pointers that have a designator but no base (for 
`__builtin_object_size` handling).

This choice of representation also seems to make it really easy for a minor bug 
in the interpreter to turn into undefined behavior during compilation. Have you 
given any thought to how that might be avoided or mitigated?




Comment at: clang/include/clang/Basic/LangOptions.def:291-294
+BENIGN_LANGOPT(EnableClangInterp, 1, 0,
+   "enable the experimental clang interpreter")
+BENIGN_LANGOPT(ForceClangInterp, 1, 0,
+   "force the use of the experimental constexpr interpreter")

"Clang" is redundant here (what else could it be?). If we're calling the new 
thing "interp" internally, then I guess "EnableInterp" and "ForceInterp" seem 
OK, but given that this is just scaffolding until the new interpreter is done, 
maybe "EnableNewInterp" and "ForceNewInterp" would be better. Suggestion:

```
BENIGN_LANGOPT(EnableNewInterp, 1, 0,
   "enable the experimental new constexpr interpreter")
BENIGN_LANGOPT(ForceNewInterp, 1, 0,
   "force the use of the experimental new constexpr interpreter")
```



Comment at: clang/include/clang/Driver/Options.td:836-839
+def fexperimental_clang_interpreter : Flag<["-"], 
"fexperimental-clang-interpreter">, Group,
+  HelpText<"Enable the experimental clang interpreter">, Flags<[CC1Option]>;
+def fforce_experimental_clang_interpreter : Flag<["-"], 
"fforce-experimental-clang-interpreter">, Group,
+  HelpText<"Force the use of the experimental clang interpreter, failing on 
missing features">, Flags<[CC1Option]>;

On reflection, let's put "new-" in here too, to match 
`-fexperimental-new-pass-manager` (so 
`-fexperimental-new-constexpr-interpreter`).  (As above, this flag name should 
not contain the string "clang".)



Comment at: clang/lib/AST/CMakeLists.txt:85-87
+  clangInterp
   clangBasic
   clangLex

Please keep these in alphabetical order.



Comment at: clang/lib/AST/ExprConstant.cpp:55-56
 #include "llvm/ADT/SmallBitVector.h"
+#include "clang/Basic/OptionalDiagnostic.h"
+#include "clang/Basic/TargetInfo.h"
 #include "llvm/Support/SaveAndRestore.h"

Please keep these in alphabetical order.



Comment at: clang/lib/AST/ExprConstant.cpp:5039-5040
+  return true;
+case interp::InterpResult::Fail:
+  return false;
+case interp::InterpResult::Bail:

Missing a check for `ForceClangInterp` here.



Comment at: clang/lib/AST/ExprConstant.cpp:12135-12136
+  return true;
+case interp::InterpResult::Fail:
+  return false;
+case interp::InterpResult::Bail:

Missing a check for `ForceClangInterp` here.



Comment at: clang/lib/AST/ExprConstant.cpp:12362-12364
+  // If the interpreter is forced, do not compute any other value.
+  if (Info.ForceClangInterp)
+return true;

Isn't this in the wrong place? If interp succeeds, I think we always want to 
stop; it's only if interp fails / bails that `ForceClangInterp` should matter. 
Or did I misunderstand what that flag is for?



Comment at: clang/lib/AST/ExprVM/Pointer.h:30
+
+/// Describes a memory block - generated once by the compiler.
+struct Descriptor {

nand wrote:
> rsmith wrote:
> > How is this going to work for subobjects? We can't generate the full space 
> > of all possible descriptors, as that would be unmanageable for large arrays 
> > of complicated types.
> Subobjects will have pointers to descriptors embedded into their data - a 
> pointer inside a block can follow that chain of descriptors up to the root.
I don't think I understand what you're suggesting here. If I have, for example:

```
struct X { char c; };
X x[100];
```

... are you saying you would embed 100 descriptors into the representation 
of `x`? That seems extremely 

[PATCH] D66100: Add __has_builtin support for builtin function-like type traits.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, kristina, jfb.
Herald added a project: clang.

Previously __has_builtin(__builtin_*) would return false for
__builtin_*s that we modeled as keywords rather than as functions
(because they take type arguments). With this patch, all builtins
that are called with function-call-like syntax return true from
__has_builtin (covering __builtin_* and also the __is_* and __has_* type
traits and the handful of similar builtins without such a prefix).

Update the documentation on __has_builtin and on type traits to match.
While doing this I noticed the type trait documentation was out of date
and incomplete; that's fixed here too.


Repository:
  rC Clang

https://reviews.llvm.org/D66100

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Features.def
  include/clang/Basic/TokenKinds.def
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/feature_tests.cpp

Index: test/Preprocessor/feature_tests.cpp
===
--- /dev/null
+++ test/Preprocessor/feature_tests.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 %s -triple=i686-apple-darwin9 -verify -DVERIFY
+// expected-no-diagnostics
+
+#ifndef __has_feature
+#error Should have __has_feature
+#endif
+
+#if __has_feature(something_we_dont_have)
+#error Bad
+#endif
+
+#if  !__has_builtin(__builtin_huge_val) || \
+ !__has_builtin(__builtin_shufflevector) || \
+ !__has_builtin(__builtin_convertvector) || \
+ !__has_builtin(__builtin_trap) || \
+ !__has_builtin(__c11_atomic_init) || \
+ !__has_builtin(__builtin_launder) || \
+ !__has_feature(attribute_analyzer_noreturn) || \
+ !__has_feature(attribute_overloadable)
+#error Clang should have these
+#endif
+
+// These are technically implemented as keywords, but __has_builtin should
+// still return true.
+#if !__has_builtin(__builtin_LINE) || \
+!__has_builtin(__builtin_FILE) || \
+!__has_builtin(__builtin_FUNCTION) || \
+!__has_builtin(__builtin_COLUMN) || \
+!__has_builtin(__array_rank) || \
+!__has_builtin(__underlying_type) || \
+!__has_builtin(__is_trivial) || \
+!__has_builtin(__has_unique_object_representations)
+#error Clang should have these
+#endif
+
+// This is a C-only builtin.
+#if __has_builtin(__builtin_types_compatible_p)
+#error Clang should not have this in C++ mode
+#endif
+
+#if __has_builtin(__builtin_insanity)
+#error Clang should not have this
+#endif
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -25,10 +25,18 @@
 #if !__has_builtin(__builtin_LINE) || \
 !__has_builtin(__builtin_FILE) || \
 !__has_builtin(__builtin_FUNCTION) || \
-!__has_builtin(__builtin_COLUMN)
+!__has_builtin(__builtin_COLUMN) || \
+!__has_builtin(__builtin_types_compatible_p)
 #error Clang should have these
 #endif
 
+// These are C++-only builtins.
+#if __has_builtin(__array_rank) || \
+__has_builtin(__underlying_type) || \
+__has_builtin(__is_trivial)
+#error Clang should not have these in C mode
+#endif
+
 #if __has_builtin(__builtin_insanity)
 #error Clang should not have this
 #endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1617,21 +1617,38 @@
 return true;
   }
   return true;
+} else if (II->getTokenID() != tok::identifier ||
+   II->hasRevertedTokenIDToIdentifier()) {
+  // Treat all keywords that introduce a custom syntax of the form
+  //
+  //   '__some_keyword' '(' [...] ')'
+  //
+  // as being "builtin functions", even if the syntax isn't a valid
+  // function call (for example, because the builtin takes a type
+  // argument).
+  if (II->getName().startswith("__builtin_") ||
+  II->getName().startswith("__is_") ||
+  II->getName().startswith("__has_"))
+return true;
+  return llvm::StringSwitch(II->getName())
+  .Case("__array_rank", true)
+  .Case("__array_extent", true)
+  .Case("__reference_binds_to_temporary", true)
+  .Case("__underlying_type", true)
+  .Default(false);
 } else {
   return llvm::StringSwitch(II->getName())
-  .Case("__make_integer_seq", LangOpts.CPlusPlus)
-  .Case("__type_pack_element", LangOpts.CPlusPlus)
-  .Case("__builtin_available", true)
-  .Case("__is_target_arch", true)
-  .Case("__is_target_vendor", true)
-  .Case("__is_target_os", true)
-  

[PATCH] D66040: [Sema] Implement DR2386 for C++17 structured binding

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Looks good. Can you add a test to `test/CXX/drs/dr23xx.cpp` with a suitable 
`dr2386: 10` comment to track that we've implemented DR2386 in Clang 10, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66040



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

[PATCH] D65696: Implements CWG 2082 Referring to parameters in unevaluated operands of default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:103-105
+  //   A local variable cannot be odr-used (6.2) in a default argument.
+  if (DRE->isNonOdrUse() != NOUR_None)
+return false;

Please add tests for the distinction between "potentially-evaluated" and 
"odr-used" here, for example:

```
void f() {
  const int n = 123;
  void g(int k = n); // ok, not an odr-use
}
```



Comment at: clang/www/cxx_dr_status.html:3
   "http://www.w3.org/TR/html4/strict.dtd;>
 
 

Note that this is an auto-generated file. To update it, you need to add a test 
to the relevant file (`test/CXX/drs/dr20xx.cpp`) with a suitable comment (`// 
dr2082: 10` to mark this implemented in Clang 10), grab a recent 
`cwg_index.html` file, and run the `make_cxx_dr_status` script.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65696



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Can you reduce this patch to only the `&&` within `||` and `&` within `|` 
changes? I think we have reasonable consensus that that should not be enabled 
by default, so let's land that part now. If you want to continue discussing 
`-Wshift-op-parentheses` after that, we can.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I don't think you need to change the `TreeTranform` base class to support this; 
`TreeTransform::TransformExpr` is an extension point which you can override 
from `TransformTypos` to inject the custom logic you need. But I also don't 
think this `TreeTransform::TransformExpr` approach works in general anyway, as 
noted below.




Comment at: lib/Sema/TreeTransform.h:3485-3489
+  // If the transformed Expr is valid, check if it's a TypoExpr so we can keep
+  // track of them. Otherwise, if the transform result is invalid, clear any
+  // TypoExprs that might have been created recursively (TODO: verify that
+  // this can happen in practice here instead of via an error trap).
+  if (Res.isUsable()) {

It's not safe to assume that all created `TypoExpr`s will be produced by 
`TransformExpr`. We might (for example) produce a new `TypoExpr` for the callee 
expression while transforming a `CallExpr`, and you won't catch that here.

It would seem cleaner and more robust to me to collect the typos produced 
during typo correction from within `Sema::createDelayedTypo`. (Eg, set some 
state on `Sema` for the duration of typo correction, and use that state to 
communicate any created typos back from `createDelayedTypo` to typo correction.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64799#1605211 , @ilya-biryukov 
wrote:

> @rsmith, emitting the typos on pop expression evaluation context resulted in 
> a bunch of failures when trying to transform the resulting errors,  see a 
> typical stacktrace below.
>  It seems we can actually pop expression evaluation contexts between 
> producing and correcting a typo expression.


I think I see the problem: if we have expression `E1` as a subexpression of 
`E2`, but `E1` has its own expression evaluation context (eg, maybe it's in a 
`sizeof` expression or similar), we'll now diagnose `E1`'s typos when we leave 
that context. That by itself seems fine, but then when we run typo correction 
for `E2`, we rediscover the typo from `E1` and are surprised to find that the 
correction information has been discarded but the `TypoExpr` is still reachable 
in the AST. It seems like we could handle that by tracking the 
already-corrected `TypoExpr`s and teaching `TransformTypos` to substitute in 
the known correction in this case rather than trying to pick a correction for 
itself.

> Would you be ok with landing the workaround as is? Alternatively, any ideas 
> on how we can avoid this problem without extending the scope of the patch too 
> much?

As above, I think we can probably fix this without changing too much by 
tracking which typos we've already resolved rather than throwing away the 
information about them. Failing that, I can live with this landing as-is, but 
delaying these diagnostics to the end of the file is going to result in a bad 
user experience whenever it happens -- and I suspect it'll start happening 
significantly more than the assert currently fails, because we'll no longer 
have this assertion forcing people to call `CorrectDelayedTyposInExpr` when 
discarding an expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

For the `&&` vs `||` and `&` vs `|` warnings, it seems extremely implausible to 
me that they meet the "few or no false-positives" criterion for an 
enabled-by-default warning. Even assuming that people don't know the relative 
precedences of those operators, we'd expect a false-positive rate of at least 
50%. So disabling those by default seems reasonable to me, and in line with 
prior guidance for what makes a good on-by-default warning.

For the other two changes, I'm not yet convinced we should change the default, 
but could be convinced by data about false positive rates.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5647-5650
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
+  InGroup, DefaultIgnore;

I think this should remain enabled by default unless you have evidence of false 
positives. In my experience, this catches bugs like

```
ostream << "got expected result: " << x == 0;
```

... and very little else.

That said, perhaps we could do better here, since this warning is typically 
followed by an error: if we detect the special case of overload resolution 
failure for an operator with an `<<` (or `>>`) operator expression on its 
left-hand side, we could produce a much more targeted diagnostic for this case 
and avoid the current situation of a warning followed by an error for the same 
problem. If we did that, we could probably remove this warning entirely.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
+  "'%1' will be evaluated first">, InGroup, DefaultIgnore;

Do you have evidence that this warning has a significant false-positive rate? 
In my experience it's very common for people to think of `<<` as being a 
multiplication-like operator and be surprised when it turns out to have lower 
precedence than addition.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64146#1604717 , @nand wrote:

> > How do you intend to represent pointers cast to integer types? Allocating 
> > 64 bits of state for a 64-bit integer is insufficient to model that case.
>
> Is this ever going to be allowed in constexpr?


It's not sufficient for this to handle only the things that are allowed in 
constant expressions; you also need to allow the things that are allowed by 
Clang's current constant evaluator, which includes this case. There are also 
constructs that allow arbitrary constant folding within the context of constant 
expression evaluation (such as a `__builtin_constant_p` conditional). So yes, 
you need to deal with this.

> If that is the case, we'll add a separate type for all integers which are 
> large enough to hold a pointer, a tagged union indicating whether the value 
> is a number or a pointer. This would add significant overhead, but I don't 
> see any other way which can correctly diagnose UB when casting a random 
> integer to a pointer.

These cases are likely to be rare enough that separately-allocated storage for 
this case could work. You'll need at least one bit somewhere to track whether 
you're in the "pointer cast to integer" case, though.

(You also need to be able to distinguish between an integer value and an 
uninitialized integer and an out-of-lifetime integer, so you'll presumably need 
some side-table to track the state of subobjects anyway.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64146



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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

How do you intend to represent pointers cast to integer types? Allocating 64 
bits of state for a 64-bit integer is insufficient to model that case.




Comment at: clang/include/clang/AST/ASTContext.h:583-584
 
+  /// Returns the constexpr VM context.
+  vm::Context ();
+

You have too many names for the same thing (`vm`, `ExprVM`, ConstExprPreter, 
and `ConstexprCtx` here -- and `ExprVM` is a bad choice of name since a lot of 
the purpose is to evaluate statements rather than expressions). Please pick a 
single name and apply it consistently.

Suggestion: use namespace `clang::interp`, in `{include/clang,lib}/AST/Interp`, 
and generally refer to this implementation as "the Clang interpreter". This 
function should be called `getInterpContext` or similar. (If you prefer, use VM 
intead of Interp, but I'd be concerned about confusion with LLVM.)



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:231-234
+def err_constexpr_compilation_failed : Error<
+  "constexpr cannot be compiled by the experimental interpreter">;
+def err_constexpr_interp_failed : Error<
+  "constexpr cannot be evaluated by the experimental interpreter">;

`constexpr` is an adjective; this sentence doesn't parse. Do you mean "this 
constexpr function" or something like that?



Comment at: clang/include/clang/Driver/Options.td:838-839
+  HelpText<"Enable the experimental constexpr interpreter">, 
Flags<[CC1Option]>;
+def fexperimental_constexpr_force_interpreter : Flag<["-"], 
"fexperimental-constexpr-force-interpreter">, Group,
+  HelpText<"Force the use of the experimental constexpr interpreter, failing 
on missing features">, Flags<[CC1Option]>;
 def fconstexpr_backtrace_limit_EQ : Joined<["-"], 
"fconstexpr-backtrace-limit=">,

If the name we're presenting to users is "the experimental constexpr 
interpreter", then "force" shouldn't appear part way through that name. 
`-fforce-experimental-constexpr-interpreter` maybe?



Comment at: clang/lib/AST/CMakeLists.txt:85
   LINK_LIBS
+  clangExpr
   clangBasic

What is the `clangExpr` library? Should this say `clangExprVM` (modulo 
renaming)?



Comment at: clang/lib/AST/ExprConstant.cpp:691-696
+/// Force the use of the constexpr interpreter, bailing out with an error
+/// if a feature is unsupported.
+bool ForceInterp;
+
+/// Enable the constexpr interpreter.
+bool EnableInterp;

These comments don't make sense: `ExprConstant.cpp` is a constexpr interpreter 
too. Maybe "the new interpreter"?



Comment at: clang/lib/AST/ExprVM/CMakeLists.txt:4
+
+add_clang_library(clangExpr
+  Compiler.cpp

This is not a reasonable name for this library.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:79-82
+  for (auto *ParamDecl : F->parameters()) {
+if (auto T = Ctx.classify(ParamDecl->getType())) {
+  auto Size = primSize(*T);
+  auto Desc = llvm::make_unique(ParamDecl, Size, Size, *T,

You are overusing `auto` here. The types of at least `T` and `Size` are 
non-obvious, and should be written explicitly.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:88
+  Params.insert({ParamDecl, ParamOffset});
+  ParamOffset += align(Size);
+  Args.push_back(*T);

What happens if this overflows (due to many large local variables)?



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:99
+Start = P.getOffset();
+if (auto E = compile(F->getBody()))
+  return std::move(E);

Don't use `auto` here. The code would be much clearer if you explicitly write 
`llvm::Error`.

(I'm not going to call out further overuse of `auto`; generally [the rule we 
want to 
follow](http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)
 is "Use auto if and only if it makes the code more readable or easier to 
maintain.", which many of the uses of `auto` here do not.)



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:144
+  } else {
+return discard(static_cast(S));
+  }

Use `E` here rather than casting `S` again.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:168-169
+  if (!VD->hasLocalStorage()) {
+// TODO: implement non-local variables.
+return bail(DS);
+  }

Non-local variables should not require any code generation, since they can't 
have dynamic initialization or destruction. (You can just return 
`Error::success()`.)



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:321-339
+if (auto T = Ctx.classify(CE->getType())) {
+  if (auto *DE = dyn_cast(SubExpr)) {
+bool IsReference = DE->getDecl()->getType()->isReferenceType();
+if (!IsReference) {
+  if (auto *PD = 

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64838#1602840 , 
@Nathan-Huckleberry wrote:

> I agree that parsing according to attribute name/type is not a good solution.
>
> It sounds like we have narrowed it down to two choices:
>  Do we want to follow the gcc method of parsing once and falling back if 
> parsing fails?
>  Do we want to parse attributes first and then wait until we see a 
> decl-specifier (breaking the implicit int case)?


I don't think so. A GCC attribute is a decl-specifier, so should trigger 
implicit-int in the languages that have it.

Option 1: teach the statement/declaration disambiguation code that an initial 
GNU attribute does not resolve the ambiguity and that it needs to disambiguate 
past one.

Option 2: parse the attributes and then call the disambiguation code and tell 
it that we've already consumed a decl-specifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64838



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, LGTM. Do you need someone to commit this for you?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64799#1592263 , @rnk wrote:

> In D64799#1591732 , @ilya-biryukov 
> wrote:
>
> > @rsmith, I'll look into emitting the typos when we pop expression 
> > evaluation context, but do we expect this to cover **all** cases where 
> > `TypoExpr`s are produced?
> >  (conservatively assuming that the answer is "no") should we land this 
> > patch and also emit at the end of TU in addition to expression evaluation 
> > context?
>
>
> I was going to pose the question this way: suppose clang already diagnosed 
> typos when leaving an expr evaluation context, when appropriate. Would it 
> still make sense to relax this assertion to diagnose any remaining ones at 
> end of TU? Are we confident that we can catch all the typos, always? I'm not 
> confident that everything will be handled, so I think we should take this 
> change as is.


There may still be uncorrected typos left behind in the outermost 
`ExprEvalContext` (created by the `Sema` constructor). In principle we should 
be able to get rid of that and parse all expressions in an evaluation context 
created for that expression (and anywhere we can't do that is a bug because 
we'll be parsing an expression without specifying whether it's 
potentially-evaluated, etc), but in practice it looks like there are still at 
least a few places where we parse expressions with no expression evaluation 
context, so cleaning up the typos in `ExprEvalContext[0]` from 
`ActOnEndOfTranslationUnitFragment` seems reasonable to me too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

[PATCH] D63889: Check possible warnings on global initializers for reachability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:352
   SetParamDefaultArgument(Param, DefaultArg, EqualLoc);
+  CurContext->removeDecl(Param);
+  CurContext = Cur;

We may need to delay the diagnostics here until the default argument is *used*: 
if a default argument references a template instantiation, the instantiation is 
not performed until that point, which may mean that our semantic checking can't 
complete correctly until use.



Comment at: clang/lib/Sema/SemaExpr.cpp:16694
+// initializer and check whether the context in question is reachable.
+if (auto *VD = dyn_cast_or_null(CurContext->getLastDecl())) {
+  if (VD->getDefinition()) {

It's not correct to assume that the last declaration in the context is the 
right one to be considering here. Instead, you should track whether you're in a 
variable initializer (and for what) in `Sema`. Examples:

```
int x = ([]{}(), x); // in C++; last decl is the lambda
```

```
int y = (struct Q { int n; }){y}; // in C; last decl is the struct
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63889



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

[PATCH] D64914: Implement P1771

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Please update cxx_status.html to mark P1771R1 as implemented in SVN.




Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17 
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17 
extension}}
 #endif

aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > Is Core treating this paper as a DR? I don't have a strong opinion here, 
> > > but for the nodiscard with a message version, I made it a C++2a 
> > > extension. I don't have a strong opinion, but I sort of prefer doing 
> > > whatever Core decides.
> > I am unfamiliar with what Core is treating it as, but my understanding is 
> > that EWG encouraged implementers to treat it as such.  
> We expose the attribute in all its glory in all language modes, so these 
> changes already do what we want in effect. The only real question is whether 
> we want to claim it's a C++17 extension or a C++2a extension. If a user turns 
> on extension warnings, we should probably tell them when the feature was 
> added, which is C++2a. It would be a bit weird to claim this is a C++17 when 
> the feature test for it is `__has_attribute(nodiscard) == 201907L` (due to 
> the normative wording changes).
> 
> But if Core moves it as a DR, then C++17 is fine, though I suppose SD-6 would 
> need to make clear what is required for each given attribute feature test 
> value to give us the answer.
We moved this change as a DR, so this feature should be treated as if it were 
always part of the spec.


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

https://reviews.llvm.org/D64914



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

[PATCH] D65022: [Sema] Always instantiate the initializer of a variable template with undeduced type (8.0 regression)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Well, this restores an incorrect behaviour: we're not permitted to substitute 
into the initializer of the variable template here.

Can you look into fixing this properly, by instantiating the type when forming 
the MemberExpr? If not, taking this to fix the regression seems OK, but please 
file a bug for the introduced misbehaviour.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65022



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

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64799#1589557 , @ilya-biryukov 
wrote:

> I tried to find a good place to emit unresolved typos earlier (to make sure 
> CodeGen does not ever get `TypoExpr`), but couldn't find one.
>  Please let me know if there's some obvious place I'm missing.


The original plan when we were first designing the feature was to emit these 
diagnostics when we pop an expression evaluation context. Maybe we could try 
that? If there's a reason to defer typo correction across such contexts, it 
might probably be rare enough that we can explicitly handle that and manually 
move the delayed typos to the surrounding context.

> unless people object I would propose to land it even if it does not solve all 
> of the problems around delayed exprs.

Sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

[PATCH] D64678: [Sema] Fix -Wuninitialized for struct assignment from GNU C statement expression

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I don't think this problem really has anything to do with statement 
expressions; consider:

  struct Widget a = (init2(), a);

... which has the same behaviour and presumably produces the same warning.

It's just a C / C++ difference. In C++, these examples are undefined because 
the lifetime of the object hasn't started yet, but I think in C they're valid. 
We should just disable the whole warning in C mode and leave it to the CFG 
analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64678



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

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseTentative.cpp:2130
+  ParsedAttributesWithRange attrs(AttrFactory);
+  ParseGNUAttributes(attrs, nullptr, nullptr);
+  if (attrs.size() <= 0) {

It's not correct in general to call arbitrary parsing actions in a 
tentatively-parsed region; they might annotate or otherwise mess with the token 
stream in undesirable ways, or produce diagnostics, etc. If we keep this 
tentative parsing formulation, you'll need to instead recognize the 
`__attribute__` token then manually skip its attribute list.



Comment at: clang/lib/Parse/ParseTentative.cpp:2146
+bool Parser::isNullStmtWithAttributes() {
+  RevertingTentativeParsingAction PA(*this);
+  return TryParseNullStmtWithAttributes() == TPResult::True;

xbolva00 wrote:
> Is this “cheap” in terms of compile time?
No; this is not a reasonable thing to do for every block-scope statement or 
declaration that we parse.

We should do the same thing that we do for all other kinds of attribute: parse 
them unconditionally then reject them in the contexts where they should not be 
permitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64838



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert

This will run the backend and (I think) drop an unwanted output file somewhere. 
You can use `-emit-llvm-only` instead of `-emit-obj` so that we stop after 
creating the IR and don't actually write it anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added subscribers: aaron.ballman, rsmith.
rsmith added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2773
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let MeaningfulToClassTemplateDefinition = 1;
   let Documentation = [LifetimeOwnerDocs];

On the surface this doesn't appear to make sense, but I think that's a 
pre-existing bug.

@aaron.ballman Is this field name reversed from the intent? I think what it 
means is "meaningful on (non-defining) class declaration", which is... the 
opposite of what its name suggests. Looking at the tablegen implementation, it 
appears that setting this to 1 causes the attribute to be instantiated when 
instantiating a class declaration, not only when instantiating a class 
definition.



Comment at: clang/include/clang/Basic/AttrDocs.td:4167-4168
 pointer/reference to the data owned by ``O``. The owned data is assumed to end
-its lifetime once the owning object's lifetime ends.
+its lifetime once the owning object's lifetime ends. The argument ``T`` is
+optional.
 

... and what does the attribute mean when the argument is omitted?



Comment at: clang/include/clang/Basic/AttrDocs.td:4180
 non-owning type whose objects can point to ``T`` type objects or dangle.
+The argument ``T`` is optional.
+

Similarly, what happens in that case?



Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type   , KEYCXX)

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > mgehre wrote:
> > > > gribozavr wrote:
> > > > > I'd suggest to split type trait implementations into a separate patch.
> > > > > 
> > > > > Are these type traits being exposed only for testing? I'm not sure it 
> > > > > is a good idea to do that -- people will end up using them in 
> > > > > production code -- is that a concern? If so, maybe it would be better 
> > > > > to test through warnings.
> > > > I copied that approach from https://reviews.llvm.org/D50119.
> > > > If people do it in production code, then at least the two leading 
> > > > underscores should tell them "I'm not supposed to use this".
> > > > 
> > > > I considered a test through warnings, but how would I trigger them? Add 
> > > > `-Wlifetime-categories-debug` which emits notes whenever a 
> > > > [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?
> > > > If people do it in production code, then at least the two leading 
> > > > underscores should tell them "I'm not supposed to use this".
> > > 
> > > That's not what two underscores mean. These custom type traits, being 
> > > language extensions, must have a name that won't collide with any 
> > > user-defined name, hence two underscores. Two underscores mean nothing 
> > > about whether the user is allowed to use it or not. Sure the code might 
> > > become non-portable to other compilers, but that's not what the concern 
> > > is. My concern is that code that people write might become unportable to 
> > > future versions of Clang, where we would have to change behavior of these 
> > > type traits (or it would just subtly change in some corner case and we 
> > > won't notice since we don't consider it a public API).
> > > 
> > > > I considered a test through warnings, but how would I trigger them?
> > > 
> > > I meant regular warnings, that are the objective of this analysis -- 
> > > warnings about dereferencing dangling pointers. If we get those warnings 
> > > for the types-under-test, then obviously they have the correct 
> > > annotations from the compiler's point of view.
> > I spent a lot of time debugging on the full lifetime analysis, and knowing 
> > exactly which type has which Attribute (especially if inference is 
> > involved) was quite important.
> > I would say that adding additional code to trigger a real lifetime warning 
> > - requires that we first add some lifetime warnings to clang (currently in 
> > a different PR)
> > - obscures the purpose of the check, i.e. checking the attributes and not 
> > the warnings themselves
> > - and makes debugging hard when the test fails (warnings involve at least 
> > one Owner and one Pointer, so either of those could have made the test fail)
> > I'd prefer if we can test the attributes directly.
> I agree that being able to test these properties definitely simplifies 
> testing. I am worried about adding language extension though, and would like 
> someone like Richard Smith to LGTM this approach.
If you just want this for testing, could you instead use an `-ast-dump` test 
and see if the attributes were added? Alternatively I'd be OK having these as 
just-for-testing traits if you give them names that makes their status as being 
for-testing, unsupported, may-be-removed-or-changed-at-any-time clear. (Eg, 

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:4-24
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+

Please use a more minimal testcase, such as:

```
struct P { ~P(); };
struct Q { Q(); ~Q(); };
struct R : P, Q {};
R c = { P(), Q() }; 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D63508: make -frewrite-includes also rewrite conditions in #if/#elif

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/Frontend/rewrite-includes-conditions.c:17
+line4
+#elif value2 < value2
+line5

Did you mean for this to be `value1 < value2` rather than `value2 < value2`?



Comment at: clang/test/Frontend/rewrite-includes-conditions.c:54-67
+// CHECK: #if 0 /* disabled by -frewrite-includes */
+// CHECK-NEXT: #if value1 == value2
+// CHECK-NEXT: #endif
+// CHECK-NEXT: #endif /* disabled by -frewrite-includes */
+// CHECK-NEXT: #if 0 /* evaluated by -frewrite-includes */
+// CHECK-NEXT: # 14 "{{.*}}rewrite-includes-conditions.c"
+

I find this output pretty hard to read -- it's hard to tell what's an original 
`#if` / `#endif` that's been effectively commented out, and what's an added 
`#if` / `#endif` that's doing the commenting.

What do you think about modifying the original line so it's no longer 
recognized as a preprocessing directive? For instance, instead of:

```
#if 0 /* disabled by -frewrite-includes */
#if value1 == value2
#endif
#endif /* disabled by -frewrite-includes */
#if 0 /* evaluated by -frewrite-includes */
# 14 "{{.*}}rewrite-includes-conditions.c"
line3
#if 0 /* disabled by -frewrite-includes */
#if 0
#elif value1 > value2
#endif
#endif /* disabled by -frewrite-includes */
#elif 0 /* evaluated by -frewrite-includes */
# 16 "{{.*}}rewrite-includes-conditions.c"
line4
#if 0 /* disabled by -frewrite-includes */
#if 0
#elif value1 < value2
#endif
#endif /* disabled by -frewrite-includes */
#elif 1 /* evaluated by -frewrite-includes */
# 18 "{{.*}}rewrite-includes-conditions.c"
line5
[...]
```

you might produce

```
#if 0 /* rewritten by -frewrite-includes */
!#if value1 == value2
#endif
#if 0 /* evaluated by -frewrite-includes */
# 14 "{{.*}}rewrite-includes-conditions.c"
line3
#if 0 /* rewritten by -frewrite-includes */
!#elif value1 > value2
#endif
#elif 0 /* evaluated by -frewrite-includes */
# 16 "{{.*}}rewrite-includes-conditions.c"
line4
#if 0 /* rewritten by -frewrite-includes */
!#elif value1 < value2
#endif
#elif 1 /* evaluated by -frewrite-includes */
# 18 "{{.*}}rewrite-includes-conditions.c"
line5
[...]
```

(or whatever transformation you like that prevents recognition of a 
`#if`/`#elif` within the `#if 0` block).

Also, maybe we could move the quoted directives inside their respective `#if` / 
`#elif` block, and only comment them out if the block was entered:

```
#if 0 /* evaluated by -frewrite-includes */
was: #if value1 == value2
# 14 "{{.*}}rewrite-includes-conditions.c"
line3
#elif 0 /* evaluated by -frewrite-includes */
was: #elif value1 > value2
# 16 "{{.*}}rewrite-includes-conditions.c"
line4
#elif 1 /* evaluated by -frewrite-includes */
#if 0
was: #elif value1 < value2
#endif
# 18 "{{.*}}rewrite-includes-conditions.c"
line5
[...]
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D63508



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

[PATCH] D64380: Add 'require_designated_init' and 'required' attribute to clang

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

C++20 designated initializers don't permit mixing designated fields with 
non-designated ones, so some of the examples here are invalid. However, I think 
we should be looking for an attribute design that works well in both C and C++, 
and with the various Clang extensions that permit the full generality of C 
designated initializers in other language modes. I also think this patch is 
combining multiple features that would be useful to expose separately. To that 
end, I think something like this might make sense:

- An attribute that can be applied to either a field or to a struct that 
requires a designator to be used on any initializer for that field (and for a 
struct, is equivalent to specifying the attribute on all fields)
- An attribute that can be applied to either a field or to a struct that 
requires an explicit initializer to be used for that field, for both aggregate 
initialization and constructor mem-initializer lists (and for a struct, is 
equivalent to specifying the attribute on all fields with no default member 
initializers)

Maybe `requires_designator` and `requires_init` or similar?

And I think these attributes should be made available in both C++11 attribute 
syntax and GNU attribute syntax. Inventing a C++-only extension to improve 
support for designated initializers doesn't seem like the right choice to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64380



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

[PATCH] D50763: [Parser] Refactor and fix bugs in late-parsing

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.
Herald added a project: clang.



Comment at: lib/Parse/ParseCXXInlineMethods.cpp:266
+public:
+  UseCachedTokensRAII(Parser , CachedTokens , const void *Data)
+  : Self(Self), Data(Data) {

Can you pass ownership of the cached tokens into here instead of passing a 
mutable reference? It makes me uncomfortable for this object to be modifying a 
list of tokens that it doesn't own (by adding an EOF token) -- that's not 
something I'd ever expect a utility with this name to do!


Repository:
  rC Clang

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

https://reviews.llvm.org/D50763



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

[PATCH] D64317: [Driver] Add float-divide-by-zero back to supported sanitizers after D63793/rC365272

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D64317



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

[PATCH] D63894: [CXX] Exercise all paths through these tests

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/SemaCXX/linkage2.cpp:3
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-c++11-extensions 
-Wno-local-type-template-args %s -std=gnu++98
 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-c++11-extensions 
-Wno-local-type-template-args -fmodules %s
 

Should this one be run in C++98 mode too? (The use of `-Wno-c++11-extensions` 
suggests to me that C++98 was expected.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63894



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

[PATCH] D64058: [cxx2a] P0624R2 fix: only lambdas with no lambda-capture are default-constructible and assignable.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

Do you have commit access or do you need someone to commit this for you?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64058



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

[PATCH] D64317: [Driver] Add float-divide-by-zero back to supported sanitizers after D63793/rC365272

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Driver/SanitizerArgs.cpp:27-30
 static const SanitizerMask NeedsUbsanRt =
 SanitizerKind::Undefined | SanitizerKind::Integer |
 SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+SanitizerKind::CFI | SanitizerKind::FloatDivideByZero;

Duplicating this list of "all sanitizers covered by the ubsan runtime" in many 
places is leading to lots of bugs.

This change misses `getPPTransparentSanitizers()` in 
`include/clang/Basic/Sanitizers.h`. Previous changes forgot to add 
`UnsignedIntegerOverflow` and `LocalBounds` to that function and also here and 
in `SupportsCoverage` and `RecoverableByDefault` below (but did update 
`TrappingSupported` and `getSupportedSanitizers`).

A better approach would be to change `Sanitizers.def` to specify the relevant 
properties of the sanitizer (whether it's in the ubsan runtime, whether it's 
recoverable, whether it supports coverage, whether it supports trapping, 
whether it affects preprocessing) along with its definition.



Comment at: test/Driver/fsanitize.c:844
+
+// RUN: %clang -target x86_64-linux -fsanitize=float-divide-by-zero %s -### 
2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-RECOVER
+// RUN: %clang -target x86_64-linux -fsanitize=float-divide-by-zero %s 
-fno-sanitize-recover=float-divide-by-zero -### 2>&1 | FileCheck %s 
--check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-NORECOVER

I think these tests should be target-independent. We should support this 
sanitizer (and indeed almost all of ubsan) regardless of the target. (Currently 
this is true on all targets except Myriad, where the latter is disallowed only 
due to a bug in r281071.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D64317



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

[PATCH] D63793: Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365272: Treat the range of representable values of 
floating-point types as [-inf, +inf]… (authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63793?vs=206532=208283#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63793

Files:
  cfe/trunk/docs/UndefinedBehaviorSanitizer.rst
  cfe/trunk/include/clang/Basic/Sanitizers.def
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
  cfe/trunk/test/CodeGen/catch-undef-behavior.c
  cfe/trunk/test/Driver/fsanitize.c
  cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

Index: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp
@@ -313,7 +313,7 @@
   /// boolean (i1) truth value.  This is equivalent to "Val != 0".
   Value *EmitConversionToBool(Value *Src, QualType DstTy);
 
-  /// Emit a check that a conversion to or from a floating-point type does not
+  /// Emit a check that a conversion from a floating-point type does not
   /// overflow.
   void EmitFloatConversionCheck(Value *OrigSrc, QualType OrigSrcType,
 Value *Src, QualType SrcType, QualType DstType,
@@ -864,128 +864,63 @@
 void ScalarExprEmitter::EmitFloatConversionCheck(
 Value *OrigSrc, QualType OrigSrcType, Value *Src, QualType SrcType,
 QualType DstType, llvm::Type *DstTy, SourceLocation Loc) {
+  assert(SrcType->isFloatingType() && "not a conversion from floating point");
+  if (!isa(DstTy))
+return;
+
   CodeGenFunction::SanitizerScope SanScope();
   using llvm::APFloat;
   using llvm::APSInt;
 
-  llvm::Type *SrcTy = Src->getType();
-
   llvm::Value *Check = nullptr;
-  if (llvm::IntegerType *IntTy = dyn_cast(SrcTy)) {
-// Integer to floating-point. This can fail for unsigned short -> __half
-// or unsigned __int128 -> float.
-assert(DstType->isFloatingType());
-bool SrcIsUnsigned = OrigSrcType->isUnsignedIntegerOrEnumerationType();
-
-APFloat LargestFloat =
-  APFloat::getLargest(CGF.getContext().getFloatTypeSemantics(DstType));
-APSInt LargestInt(IntTy->getBitWidth(), SrcIsUnsigned);
-
-bool IsExact;
-if (LargestFloat.convertToInteger(LargestInt, APFloat::rmTowardZero,
-  ) != APFloat::opOK)
-  // The range of representable values of this floating point type includes
-  // all values of this integer type. Don't need an overflow check.
-  return;
+  const llvm::fltSemantics  =
+CGF.getContext().getFloatTypeSemantics(OrigSrcType);
 
-llvm::Value *Max = llvm::ConstantInt::get(VMContext, LargestInt);
-if (SrcIsUnsigned)
-  Check = Builder.CreateICmpULE(Src, Max);
-else {
-  llvm::Value *Min = llvm::ConstantInt::get(VMContext, -LargestInt);
-  llvm::Value *GE = Builder.CreateICmpSGE(Src, Min);
-  llvm::Value *LE = Builder.CreateICmpSLE(Src, Max);
-  Check = Builder.CreateAnd(GE, LE);
-}
-  } else {
-const llvm::fltSemantics  =
-  CGF.getContext().getFloatTypeSemantics(OrigSrcType);
-if (isa(DstTy)) {
-  // Floating-point to integer. This has undefined behavior if the source is
-  // +-Inf, NaN, or doesn't fit into the destination type (after truncation
-  // to an integer).
-  unsigned Width = CGF.getContext().getIntWidth(DstType);
-  bool Unsigned = DstType->isUnsignedIntegerOrEnumerationType();
-
-  APSInt Min = APSInt::getMinValue(Width, Unsigned);
-  APFloat MinSrc(SrcSema, APFloat::uninitialized);
-  if (MinSrc.convertFromAPInt(Min, !Unsigned, APFloat::rmTowardZero) &
-  APFloat::opOverflow)
-// Don't need an overflow check for lower bound. Just check for
-// -Inf/NaN.
-MinSrc = APFloat::getInf(SrcSema, true);
-  else
-// Find the largest value which is too small to represent (before
-// truncation toward zero).
-MinSrc.subtract(APFloat(SrcSema, 1), APFloat::rmTowardNegative);
-
-  APSInt Max = APSInt::getMaxValue(Width, Unsigned);
-  APFloat MaxSrc(SrcSema, APFloat::uninitialized);
-  if (MaxSrc.convertFromAPInt(Max, !Unsigned, APFloat::rmTowardZero) &
-  APFloat::opOverflow)
-// Don't need an overflow check for upper bound. Just check for
-// +Inf/NaN.
-MaxSrc = APFloat::getInf(SrcSema, false);
-  else
-// Find the smallest value which is too large to represent (before
-// truncation toward zero).
-MaxSrc.add(APFloat(SrcSema, 1), APFloat::rmTowardPositive);
-
-  // If we're converting from __half, convert the range to float to match
-  // the type of src.
-  if 

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LG with a few tweaks.




Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:228-230
+def note_constexpr_bit_cast_indet_dest : Note<
+  "indeterminate value can only initialize an object of type 'unsigned char' "
+  "or 'std::byte'; %0 is invalid">;

This is incorrect; you can also bitcast to `char` if it's unsigned (under 
`-funsigned-char`). Use `getLangOpts().CharIsSigned` to detect whether we're in 
that mode.



Comment at: clang/lib/AST/ExprConstant.cpp:5380
 
+class APBuffer {
+  // FIXME: We're going to need bit-level granularity when we support

I don't think there's really anything "arbitrary-precision" about this 
`APBuffer`. (It's a bad name for `APValue` and a worse name here.) Maybe 
`BitCastBuffer` would be better?



Comment at: clang/lib/AST/ExprConstant.cpp:5430
+/// would represent the value at runtime.
+class BitCastReader {
+  EvalInfo 

Every time I come back to this patch I find these class names confusing -- the 
reader writes to the buffer, and the writer reads from it. I think more 
explicit names would help: `APValueToAPBufferConverter` and 
`APBufferToAPValueConverter` maybe?



Comment at: clang/lib/CodeGen/CGExprComplex.cpp:474-476
+CharUnits Align = std::min(Ctx.getTypeAlignInChars(Op->getType()),
+   Ctx.getTypeAlignInChars(DestTy));
+DestLV.setAlignment(Align);

Do we need to change the alignment here at all? The alignment on `DestLV` 
should already be taken from `Addr` (whose alignment in turn is taken from 
`SourceLVal`), and should be the alignment computed when emitting the source 
lvalue expression, which seems like the right alignment to use for the load. 
The natural alignment of the destination type (or the source type for that 
matter) doesn't seem relevant.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2043-2045
+CharUnits Align = std::min(Ctx.getTypeAlignInChars(E->getType()),
+   Ctx.getTypeAlignInChars(DestTy));
+DestLV.setAlignment(Align);

Likewise here, I think we should not be changing the alignment.



Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:18-20
+void test_aggregate_to_scalar(two_ints ) {
+  // CHECK-LABEL: define void @_Z24test_aggregate_to_scalarR8two_ints
+  __builtin_bit_cast(unsigned long, ti);

Change the return type to `unsigned long` and `return __builtin_bit_cast(...)` 
so that future improvements to discarded value expression lowering don't 
invalidate your test. (That'll also better mirror the expected use in 
`std::bit_cast`.)



Comment at: clang/test/SemaCXX/builtin-bit-cast.cpp:35
+// expected-error@+1{{__builtin_bit_cast source type must be trivially 
copyable}}
+constexpr unsigned long ul = __builtin_bit_cast(unsigned long, 
not_trivially_copyable{});

Please also explicitly test `__builtin_bit_cast` to a reference type. 
(Reference types aren't trivially-copyable, but this seems like an important 
enough special case to be worth calling out in the tests -- usually, casting to 
a reference is the same thing as casting the address of the operand to a 
pointer and dereferencing, but not here.)



Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:30
+template 
+constexpr int round_trip(const Init ) {
+  return bit_cast(bit_cast(init)) == init;

Return `bool` here rather than `int`.



Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:230
+constexpr int test_to_nullptr() {
+  nullptr_t npt = __builtin_bit_cast(nullptr_t, 0ul);
+  void *ptr = npt;

Please also test bitcasting from uninitialized and out of lifetime objects to 
`nullptr_t`.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7764
+  llvm::SmallDenseMap NewTransformCache;
+  auto SavedTransformCache = TransformCache;
+  TransformCache = NewTransformCache;

dgoldman wrote:
> Should I do the same `std::move` and `clear` here as well?
Yes, please.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this looks good; just some nits.




Comment at: lib/Sema/SemaExprCXX.cpp:7616
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  void EmitAllDiagnostics(bool isAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {

Please capitalize this parameter name to match the surrounding code style.



Comment at: lib/Sema/SemaExprCXX.cpp:7620-7621
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = isAmbiguous ? TypoCorrection() : 
State.Consumer->getCurrentCorrection();
+ExprResult Replacement = isAmbiguous ? ExprError() : 
TransformCache[TE];
 

Reflow to 80 columns.



Comment at: lib/Sema/SemaExprCXX.cpp:7686
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult CheckForRecursiveTypos(ExprResult Res, bool ) {
+if (Res.isInvalid()) {

`outIsAmbiguous` -> `IsAmbiguous` (we don't use an `out` prefix for output 
parameters).



Comment at: lib/Sema/SemaExprCXX.cpp:7694-7695
+
+auto SavedTypoExprs = TypoExprs;
+auto SavedAmbiguousTypoExprs = AmbiguousTypoExprs;
+llvm::SmallSetVector

Use `std::move` here.



Comment at: lib/Sema/SemaExprCXX.cpp:7696-7699
+llvm::SmallSetVector
+RecursiveTypoExprs, RecursiveAmbiguousTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+AmbiguousTypoExprs = RecursiveAmbiguousTypoExprs;

Call `TypoExprs.clear()` rather than copying from a default-constructed object.



Comment at: lib/Sema/SemaExprCXX.cpp:7723-7724
+
+TypoExprs = SavedTypoExprs;
+AmbiguousTypoExprs = SavedAmbiguousTypoExprs;
+

And `std::move` here too.



Comment at: lib/Sema/SemaExprCXX.cpp:7778-7779
+  }
+} while((Next = State.Consumer->peekNextCorrection()) &&
+Next.getEditDistance(false) == TC.getEditDistance(false));
+

Apply clang-format here. (Or: add space after `while` on the first line, and 
indent the second line so it begins in the same column as `(Next =[...]`).



Comment at: lib/Sema/SemaExprCXX.cpp:7781-7785
+if (!outIsAmbiguous) {
+  AmbiguousTypoExprs.remove(TE);
+  State.Consumer->restoreSavedPosition();
+} else
+  break;

Reverse the condition of this `if` so you can early-exit from the loop and 
remove the `else`.



Comment at: lib/Sema/SemaExprCXX.cpp:7819
   ExprResult Transform(Expr *E) {
-ExprResult Res;
-while (true) {
-  Res = TryTransform(E);
-
-  // Exit if either the transform was valid or if there were no TypoExprs
-  // to transform that still have any untried correction candidates..
-  if (!Res.isInvalid() ||
-  !CheckAndAdvanceTypoExprCorrectionStreams())
-break;
-}
-
-// Ensure none of the TypoExprs have multiple typo correction candidates
-// with the same edit length that pass all the checks and filters.
-// TODO: Properly handle various permutations of possible corrections when
-// there is more than one potentially ambiguous typo correction.
-// Also, disable typo correction while attempting the transform when
-// handling potentially ambiguous typo corrections as any new TypoExprs 
will
-// have been introduced by the application of one of the correction
-// candidates and add little to no value if corrected.
-SemaRef.DisableTypoCorrection = true;
-while (!AmbiguousTypoExprs.empty()) {
-  auto TE  = AmbiguousTypoExprs.back();
-  auto Cached = TransformCache[TE];
-  auto  = SemaRef.getTypoExprState(TE);
-  State.Consumer->saveCurrentPosition();
-  TransformCache.erase(TE);
-  if (!TryTransform(E).isInvalid()) {
-State.Consumer->resetCorrectionStream();
-TransformCache.erase(TE);
-Res = ExprError();
-break;
-  }
-  AmbiguousTypoExprs.remove(TE);
-  State.Consumer->restoreSavedPosition();
-  TransformCache[TE] = Cached;
-}
-SemaRef.DisableTypoCorrection = false;
+bool isAmbiguous = false;
+ExprResult Res = RecursiveTransformLoop(E, isAmbiguous);

`isAmbiguous` -> `IsAmbiguous`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D63508: make -frewrite-includes also rewrite conditions in #if/#elif

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Patch generally looks good; just a minor concern about the output format.




Comment at: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp:487-490
+OS << " - evaluated by -frewrite-includes */" << MainEOL;
+OS << (elif ? "#elif" : "#if");
+OS << " (" << (isTrue ? "1" : "0")
+   << ") /* evaluated by -frewrite-includes */";

Adding an extra line here is going to mess up presumed line numbering. Also, we 
don't need parentheses around the constant `0` or `1`. Perhaps instead we could 
prepend a `#if 0 /*` or `#if 1 /*` to the directive:

```
#if FOO(x) == \
  BAZ
#ifndef BAR
```

->

```
#if 1 /*#if FOO(x) == \
  BAZ*/
#if 0 /*#ifndef BAR*/
```

I don't think we really need the "evaluated by -frewrite-includes" part, but I 
have no objection to including it. The best place is probably between the `/*` 
and the `#`:

```
#if 1 /*evaluated by -frewrite-includes: #if FOO(x) == \
  BAZ*/
#if 0 /*evaluated by -frewrite-includes: #ifndef BAR*/
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D63508



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

[PATCH] D63793: Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added reviewers: rnk, BillyONeal.
Herald added a project: clang.

Prior to r329065, we used [-max, max] as the range of representable
values because LLVM's `fptrunc` did not guarantee defined behavior when
truncating from a larger floating-point type to a smaller one. Now that
has been fixed, we can make clang follow normal IEEE 754 semantics in this
regard and take the larger range [-inf, +inf] as the range of representable
values.

In practice, this affects two parts of the frontend:

- the constant evaluator no longer treats floating-point evaluations that 
result in +-inf as being undefined (because they no longer leave the range of 
representable values of the type)
- UBSan no longer treats conversions to floating-point type that are outside 
the [-max, +max] range as being undefined

In passing, also remove the float-divide-by-zero sanitizer from
-fsanitize=undefined, on the basis that while it's undefined per C++
rules (and we disallow it in constant expressions for that reason), it
is defined by Clang / LLVM / IEEE 754.


Repository:
  rC Clang

https://reviews.llvm.org/D63793

Files:
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGExprScalar.cpp
  test/CXX/expr/expr.const/p2-0x.cpp
  test/CodeGen/catch-undef-behavior.c
  test/Driver/fsanitize.c
  test/SemaCXX/constant-expression-cxx1y.cpp

Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -425,7 +425,7 @@
   constexpr bool test_overflow() {
 T a = 1;
 while (a != a / 2)
-  a *= 2; // expected-note {{value 2147483648 is outside the range}} expected-note {{ 9223372036854775808 }} expected-note {{floating point arithmetic produces an infinity}}
+  a *= 2; // expected-note {{value 2147483648 is outside the range}} expected-note {{ 9223372036854775808 }}
 return true;
   }
 
@@ -435,7 +435,8 @@
   static_assert(test_overflow(), ""); // ok
   static_assert(test_overflow(), ""); // ok
   static_assert(test_overflow(), ""); // expected-error {{constant}} expected-note {{call}}
-  static_assert(test_overflow(), ""); // expected-error {{constant}} expected-note {{call}}
+  static_assert(test_overflow(), ""); // ok
+  static_assert(test_overflow(), ""); // ok
 
   constexpr short test_promotion(short k) {
 short s = k;
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -4,15 +4,15 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
 // RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
 // CHECK-UNDEFINED-TRAP-NOT: -fsanitize-recover
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: 

[PATCH] D62293: [modules] Add PP callbacks for entering and leaving a submodule.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

`PPCallbacks` seems to be missing the addition of `EnteredSubmodule` and 
`LeftSubmodule`; `PPChainedCallbacks` seems to be missing the addition of 
`LeftSubmodule`.

Generally this seems reasonable.




Comment at: lib/Lex/PPLexerChange.cpp:783
+if (Callbacks)
+  Callbacks->LeftSubmodule(ForPragma);
+

Would it be useful to pass in the module (and maybe the import location) here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62293



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

[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/Decl.h:4303
+
+   StringLiteral *getSTLUuid() { return STLUuid; }
+};

What does "STL" mean here?



Comment at: include/clang/Sema/ParsedAttr.h:337-346
+  /// Constructor for __declspec(uuid) attribute.
+  ParsedAttr(IdentifierInfo *attrName, SourceRange attrRange,
+ IdentifierInfo *scopeName, SourceLocation scopeLoc,
+ StringLiteral *stluuid, ParsedAttr::Syntax syntaxUsed)
+  : AttrName(attrName), ScopeName(scopeName), AttrRange(attrRange),
+ScopeLoc(scopeLoc), Invalid(false), HasParsedType(false),
+SyntaxUsed(syntaxUsed), NumArgs(1) {

I don't think we need a custom constructor for this any more; a `StringLiteral` 
is an `Expr`, so can be stored as a regular argument. It's also suspicious that 
this constructor doesn't use its `stluuid` parameter -- maybe it's already 
unused?



Comment at: include/clang/Sema/ParsedAttr.h:822
+  ParsedAttr *
+  createUuidDeclSpecAttribute(IdentifierInfo *attrName, SourceRange attrRange,
+  IdentifierInfo *scopeName, SourceLocation 
scopeLoc,

(Likewise we shouldn't need this any more...)



Comment at: include/clang/Sema/ParsedAttr.h:1031
+  ParsedAttr *
+  addNew(IdentifierInfo *attrName, SourceRange attrRange, IdentifierInfo 
*scopeName,
+ SourceLocation scopeLoc, StringLiteral *stluuid, ParsedAttr::Syntax 
syntaxUsed) {

(... or this.)



Comment at: include/clang/Sema/Sema.h:408
 
+  std::map DeclToDeclSpecUuidDecl;
+

This map is not necessary (you can get the `DeclSpecUuidDecl` from the 
`UuidAttr` on the `Decl`) and not correct (it's not serialized and 
deserialized). Please remove it.



Comment at: lib/CodeGen/CodeGenModule.cpp:1071-1073
+  const auto ExistingRecord = Manglings.find(MangledName);
+  if (ExistingRecord != std::end(Manglings))
+Manglings.remove(&(*ExistingRecord));

Was this supposed to be included in this patch? It looks like this is papering 
over a bug elsewhere.



Comment at: lib/Sema/SemaDeclAttr.cpp:5405-5408
+if (UA->getDeclSpecUuidDecl()->getSTLUuid()->getString().equals_lower(
+Uuid->getSTLUuid()->getString()) &&
+declaresSameEntity(DeclToDeclSpecUuidDecl.find(D)->first, D))
   return nullptr;

This `if` should be:

```
if (declaresSameEntity(UA->getDeclSpecUuidDecl(), Uuid))
```

(Not a string comparison. We already combined `UuidDecl`s with the same string 
into the same entity.)



Comment at: lib/Sema/SemaDeclAttr.cpp:5452-5460
+  QualType ResTy;
+  StringLiteral *Stl = nullptr;
+  llvm::APInt Length(32, StrRef.size() + 1);
+  ResTy = S.getASTContext().adjustStringLiteralBaseType(
+  S.getASTContext().WideCharTy.withConst());
+  ResTy = S.getASTContext().getConstantArrayType(ResTy, Length,
+ ArrayType::Normal, 0);

You already have a suitable string literal as `AL.getArgAsExpr(0)`. There's no 
need to fabricate another one here.



Comment at: lib/Sema/SemaDeclAttr.cpp:5471-5474
+  DeclSpecUuidDecl *ArgDecl;
+
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(), 
S.getFunctionLevelDeclContext(),
+ SourceLocation(), Stl);

Please combine these lines.



Comment at: lib/Sema/SemaDeclAttr.cpp:5473-5474
+
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(), 
S.getFunctionLevelDeclContext(),
+ SourceLocation(), Stl);
+

You should check for a prior `DeclSpecUuidDecl` with the same UUID here and set 
it as the previous declaration of the new one, so that they're treated as 
declaring the same entity.



Comment at: lib/Sema/SemaDeclAttr.cpp:5476
+
+  S.DeclToDeclSpecUuidDecl[D] = ArgDecl;
+

(Remove this; with the other change I suggested above this should be 
unnecessary.)


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

https://reviews.llvm.org/D43576



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:223
+def note_constexpr_bit_cast_invalid_type : Note<
+  "cannot constexpr evaluate a bit_cast with a "
+  "%select{union|pointer|member pointer|volatile|struct with a reference 
member}0"

"constexpr evaluate" doesn't really mean anything. Also, the "struct with a 
reference member type X" case seems a bit strangely phrased (and it need not be 
a struct anyway...).

Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member 
pointer|volatile|reference}2 {type|member}1 in a constant expression"?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9732
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not equal destination size (%0 and 
%1)">;
 } // end of sema component.

We would usually use `vs` rather than `and` here.



Comment at: clang/lib/AST/ExprConstant.cpp:5454-5457
+case APValue::Indeterminate:
+  Info.FFDiag(BCE->getExprLoc(), diag::note_constexpr_access_uninit)
+  << 0 << 1;
+  return false;

I've checked the intent on the reflectors, and we should drop both this and the 
`APValue::None` check here. Bits corresponding to uninitialized or 
out-of-lifetime subobjects should just be left uninitialized (exactly like 
padding bits).

Example:

```
struct X { char c; int n; };
struct Y { char data[sizeof(X)]; };
constexpr bool test() {
  X x = {1, 2};
  Y y = __builtin_bit_cast(Y, x); // OK, y.data[1] - y.data[3] are 
APValue::Indeterminate
  X z = __builtin_bit_cast(X, y); // OK, both members are initialized
  return x.c == z.c && x.n == z.n;
}
static_assert(test());
```



Comment at: clang/lib/AST/ExprConstant.cpp:5614-5615
+SmallVector Bytes;
+if (!Buffer.readObject(Offset, SizeOf, Bytes))
+  return APValue::IndeterminateValue();
+

This should fail with a diagnostic if `T` is not a byte-like type (an unsigned 
narrow character type or `std::byte`), due to [basic.indet]p2.



Comment at: clang/lib/CodeGen/CGExprComplex.cpp:472-475
+Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(Op->getType()), Align,
+"bit_cast.temp");
+CGF.EmitAnyExprToMem(Op, Temp, Op->getType().getQualifiers(),
+ /*isInit=*/false);

This seems to unnecessarily force an extra round-trip through memory. `Op` is 
an lvalue, so it already describes a value in memory, and you should be able to 
use `EmitLValue(Op)` to get the lvalue you want to load from.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2045-2048
+Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(E->getType()), Align,
+"bit_cast.temp");
+CGF.EmitAnyExprToMem(E, Temp, E->getType().getQualifiers(),
+ /*isInit=*/false);

Likewise here.


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

https://reviews.llvm.org/D62825



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

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: test/CodeGenCXX/devirtualize-dtor-final.cpp:20
+  void evil(D *p) {
+// CHECK-NOT: call void@_ZN5Test11DD1Ev
+delete p;

Missing space after `void` here


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Thanks! Minor comment then feel free to commit.




Comment at: lib/CodeGen/CGExprCXX.cpp:1877
+  DevirtualizedDtor->getParent();
+  if (getCXXRecord(Base) == DevirtualizedClass) {
+// Devirtualized to the class of the base type (the type of the

There's no guarantee that you get the same declaration of the class in both 
cases; use `declaresSameEntity` here instead of `==` to compare whether you 
have the same class.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGExprAgg.cpp:1850
+AggValueSlot::Overlap_t
+CodeGenFunction::overlapForFieldInit(const FieldDecl *FD) {
+  if (!FD->hasAttr() || !FD->getType()->isRecordType())

rjmccall wrote:
> rsmith wrote:
> > rsmith wrote:
> > > rjmccall wrote:
> > > > `getOverlapForFieldInit`?  `overlap` is a verb.
> > > Good idea. (Both this and `overlapForBaseInit` are pre-existing; I'll 
> > > rename both.)
> > I'm going to do this in a separate change since there are quite a few uses 
> > of these and it'll add noise to the patch.
> SGTM
Done in r363980.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63451



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363976: P0840R2: support for [[no_unique_address]] attribute 
(authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63451

Files:
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/include/clang/AST/DeclCXX.h
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/lib/AST/Decl.cpp
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/CodeGen/CGExprAgg.cpp
  cfe/trunk/lib/CodeGen/CGExprConstant.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
  cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/Parse/ParseDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/CodeGenCXX/no-unique-address.cpp
  cfe/trunk/test/CodeGenCXX/tail-padding.cpp
  cfe/trunk/test/Layout/no-unique-address.cpp
  cfe/trunk/test/SemaCXX/cxx2a-no-unique-address.cpp
  cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
  cfe/trunk/www/cxx_status.html

Index: cfe/trunk/www/cxx_status.html
===
--- cfe/trunk/www/cxx_status.html
+++ cfe/trunk/www/cxx_status.html
@@ -934,7 +934,7 @@
 
   [[no_unique_address]] attribute
   http://wg21.link/p0840r2;>P0840R2
-  No
+  SVN
 
 
   [[likely]] and [[unlikely]] attributes
Index: cfe/trunk/include/clang/AST/DeclCXX.h
===
--- cfe/trunk/include/clang/AST/DeclCXX.h
+++ cfe/trunk/include/clang/AST/DeclCXX.h
@@ -334,10 +334,12 @@
 /// True when this class is a POD-type.
 unsigned PlainOldData : 1;
 
-/// true when this class is empty for traits purposes,
-/// i.e. has no data members other than 0-width bit-fields, has no
-/// virtual function/base, and doesn't inherit from a non-empty
-/// class. Doesn't take union-ness into account.
+/// True when this class is empty for traits purposes, that is:
+///  * has no data members other than 0-width bit-fields and empty fields
+///marked [[no_unique_address]]
+///  * has no virtual function/base, and
+///  * doesn't inherit from a non-empty class.
+/// Doesn't take union-ness into account.
 unsigned Empty : 1;
 
 /// True when this class is polymorphic, i.e., has at
Index: cfe/trunk/include/clang/AST/Decl.h
===
--- cfe/trunk/include/clang/AST/Decl.h
+++ cfe/trunk/include/clang/AST/Decl.h
@@ -2729,6 +2729,11 @@
   /// bit-fields.
   bool isZeroLengthBitField(const ASTContext ) const;
 
+  /// Determine if this field is a subobject of zero size, that is, either a
+  /// zero-length bit-field or a field of empty class type with the
+  /// [[no_unique_address]] attribute.
+  bool isZeroSize(const ASTContext ) const;
+
   /// Get the kind of (C++11) default member initializer that this field has.
   InClassInitStyle getInClassInitStyle() const {
 InitStorageKind storageKind = InitStorage.getInt();
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -14,6 +14,7 @@
 }
 def DocCatFunction : DocumentationCategory<"Function Attributes">;
 def DocCatVariable : DocumentationCategory<"Variable Attributes">;
+def DocCatField : DocumentationCategory<"Field Attributes">;
 def DocCatType : DocumentationCategory<"Type Attributes">;
 def DocCatStmt : DocumentationCategory<"Statement Attributes">;
 def DocCatDecl : DocumentationCategory<"Declaration Attributes">;
@@ -315,12 +316,14 @@
   // Specifies Operating Systems for which the target applies, based off the
   // OSType enumeration in Triple.h
   list OSes;
-  // Specifies the C++ ABIs for which the target applies, based off the
-  // TargetCXXABI::Kind in TargetCXXABI.h.
-  list CXXABIs;
   // Specifies Object Formats for which the target applies, based off the
   // ObjectFormatType enumeration in Triple.h
   list ObjectFormats;
+  // A custom predicate, written as an expression evaluated in a context
+  // with the following declarations in scope:
+  //   const clang::TargetInfo 
+  //   const llvm::Triple  = Target.getTriple();
+  code CustomCode = [{}];
 }
 
 class TargetArch arches> : TargetSpec {
@@ -338,8 +341,11 @@
 def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {
   let OSes = ["Win32"];
 }
+def TargetItaniumCXXABI : TargetSpec {
+  let CustomCode = [{ Target.getCXXABI().isItaniumFamily() }];
+}
 def TargetMicrosoftCXXABI : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {
-  let CXXABIs = ["Microsoft"];
+  let CustomCode = [{ 

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D63451#1549563 , @rjmccall wrote:

> Can this attribute not be applied to a base class, or to a type?


The standard attribute forbids that right now. We could add a custom attribute 
that permits it, but we're required to diagnose application of the standard 
attribute to a type -- though a warning would suffice to satisfy that 
requirement. (Because this gives "same as a base class" layout, adding it to a 
base class wouldn't have any effect right now, but we could certainly say that 
the attribute on a class type also implies the class is not POD for the purpose 
of layout, to permit tail padding reuse.)

> I think every time I've seen someone get bitten by the unique-address rule, 
> it was because they had a base class that deleted some constructors (or 
> something like that) and which was a base of a million different types; I'm 
> sure the language model they'd prefer would be to just add an attribute to 
> that one type instead of chasing down every place where they declared a field 
> of the type.

The place where we expect this to be used is where a class template wants to 
store a copy of some user-provided type that may well be empty (eg, an 
allocator, a comparator, that kind of thing). There are probably more producers 
of such types than consumers, so putting the attribute on the consumer seems to 
make some sense (though it really could go in either place, and both probably 
have reasonable use cases).

I'd be happy to go back to the committee with a proposal to extend this 
attribute to also apply to classes if you can provide a few convincing examples.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205892.
rsmith marked 4 inline comments as done.
rsmith added a comment.

- Use custom code to specify CXXABI requirements on attributes.
- Remove dead code that would have handled [[no_unique_address]] in C.
- Extend documentation to include an example and to mention that this is a 
standard C++ attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGRecordLayoutBuilder.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Parse/ParseDeclCXX.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGenCXX/no-unique-address.cpp
  test/CodeGenCXX/tail-padding.cpp
  test/Layout/no-unique-address.cpp
  test/SemaCXX/cxx2a-no-unique-address.cpp
  utils/TableGen/ClangAttrEmitter.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -934,7 +934,7 @@
 
   [[no_unique_address]] attribute
   http://wg21.link/p0840r2;>P0840R2
-  No
+  SVN
 
 
   [[likely]] and [[unlikely]] attributes
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2810,7 +2810,7 @@
 
 // Helper function for GenerateTargetSpecificAttrChecks that alters the 'Test'
 // parameter with only a single check type, if applicable.
-static void GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
+static bool GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
 std::string *FnName,
 StringRef ListName,
 StringRef CheckAgainst,
@@ -2830,7 +2830,9 @@
 *FnName += Part;
 }
 Test += ")";
+return true;
   }
+  return false;
 }
 
 // Generate a conditional expression to check if the current target satisfies
@@ -2838,10 +2840,12 @@
 // those checks to the Test string. If the FnName string pointer is non-null,
 // append a unique suffix to distinguish this set of target checks from other
 // TargetSpecificAttr records.
-static void GenerateTargetSpecificAttrChecks(const Record *R,
+static bool GenerateTargetSpecificAttrChecks(const Record *R,
  std::vector ,
  std::string ,
  std::string *FnName) {
+  bool AnyTargetChecks = false;
+
   // It is assumed that there will be an llvm::Triple object
   // named "T" and a TargetInfo object named "Target" within
   // scope that can be used to determine whether the attribute exists in
@@ -2851,6 +2855,7 @@
   // differently because GenerateTargetRequirements needs to combine the list
   // with ParseKind.
   if (!Arches.empty()) {
+AnyTargetChecks = true;
 Test += " && (";
 for (auto I = Arches.begin(), E = Arches.end(); I != E; ++I) {
   StringRef Part = *I;
@@ -2865,16 +2870,24 @@
   }
 
   // If the attribute is specific to particular OSes, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "OSes", "T.getOS()",
-  "llvm::Triple::");
+  AnyTargetChecks |= GenerateTargetSpecificAttrCheck(
+  R, Test, FnName, "OSes", "T.getOS()", "llvm::Triple::");
 
-  // If one or more CXX ABIs are specified, check those as well.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "CXXABIs",
-  "Target.getCXXABI().getKind()",
-  "TargetCXXABI::");
   // If one or more object formats is specified, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
-  "T.getObjectFormat()", "llvm::Triple::");
+  AnyTargetChecks |=
+  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
+  "T.getObjectFormat()", "llvm::Triple::");
+
+  // If custom code is specified, emit it.
+  StringRef Code = R->getValueAsString("CustomCode");
+  if (!Code.empty()) {
+AnyTargetChecks = true;
+Test += " && (";
+Test += Code;
+Test += ")";
+  }
+
+  return AnyTargetChecks;
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
@@ -3510,7 +3523,7 @@
 
   std::string FnName = "isTarget";
   std::string Test;
-  GenerateTargetSpecificAttrChecks(R, Arches, Test, );
+  bool UsesT = GenerateTargetSpecificAttrChecks(R, Arches, Test, );
 
   // If this code has already been generated, simply return the previous
   

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:1850
+AggValueSlot::Overlap_t
+CodeGenFunction::overlapForFieldInit(const FieldDecl *FD) {
+  if (!FD->hasAttr() || !FD->getType()->isRecordType())

rsmith wrote:
> rjmccall wrote:
> > `getOverlapForFieldInit`?  `overlap` is a verb.
> Good idea. (Both this and `overlapForBaseInit` are pre-existing; I'll rename 
> both.)
I'm going to do this in a separate change since there are quite a few uses of 
these and it'll add noise to the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451



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

[PATCH] D63376: [clang] Small improvments after Adding APValue to ConstantExpr

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:10298
+ bool AllowFold = true,
+ bool StoreResult = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,

Tyker wrote:
> rsmith wrote:
> > Do you need this new flag? No callers are passing it.
> the idea behind it is that not all integral constant expression benefit from 
> storing the result. i have not analyzed which benefit from it and which 
> don't. adding a FIXME would have probably be more appropriate.
It's useful to have a consistent AST representation, so I'd be inclined to at 
least always create a `ConstantExpr` node. Also, once we start allowing 
`consteval` evaluations that have side effects (for example, reflection-related 
actions that modify the AST), I think we'll need to always store the result to 
ensure we never evaluate a constant expression twice (and trigger its 
side-effects to occur twice).


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

https://reviews.llvm.org/D63376



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

[PATCH] D63508: make -frewrite-includes handle __has_include wrapped in a macro

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Perhaps we should rewrite all `#if`-like directives to `#if 0` or `#if 1`? 
Either that or we could emit pragmas indicating the values of later 
`__has_include`s. Special-casing macros of this specific form is at best a 
partial solution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63508



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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



Comment at: lib/AST/Decl.cpp:3937
+  // -- [has] virtual member functions or virtual base classes, or
+  // -- has subobjects of nonzero size or bit-fields of nonzero length
+  if (const auto *CXXRD = dyn_cast(RD)) {

rjmccall wrote:
> Surely a bit-field of nonzero length is a subobject of nonzero size.
Usually, but not if it's unnamed (unnamed bit-fields aren't subobjects). In any 
case, this is a quote from the C++ standard.



Comment at: lib/AST/Decl.cpp:3945
+return false;
+  }
+

rjmccall wrote:
> Is this a C/C++ modules interaction?
We don't allow C modules to be imported into C++ compilations or vice versa, so 
this should be unreachable unless we start allowing the attribute in C. Nice 
catch.

I guess the question is, then: should we allow this attribute in C (either with 
a GNU `__attribute__` spelling or as a C20 `[[clang::attribute]]`)? I don't 
think it's really useful in C (empty structs are ill-formed, and you can't 
reuse tail padding because structs are always trivial, at least in standard C), 
so I'm inclined to say no.



Comment at: lib/CodeGen/CGExprAgg.cpp:1850
+AggValueSlot::Overlap_t
+CodeGenFunction::overlapForFieldInit(const FieldDecl *FD) {
+  if (!FD->hasAttr() || !FD->getType()->isRecordType())

rjmccall wrote:
> `getOverlapForFieldInit`?  `overlap` is a verb.
Good idea. (Both this and `overlapForBaseInit` are pre-existing; I'll rename 
both.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205488.
rsmith added a comment.

- Remove accidentally-added file


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGRecordLayoutBuilder.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Parse/ParseDeclCXX.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGenCXX/no-unique-address.cpp
  test/CodeGenCXX/tail-padding.cpp
  test/Layout/no-unique-address.cpp
  test/SemaCXX/cxx2a-no-unique-address.cpp
  utils/TableGen/ClangAttrEmitter.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -934,7 +934,7 @@
 
   [[no_unique_address]] attribute
   http://wg21.link/p0840r2;>P0840R2
-  No
+  SVN
 
 
   [[likely]] and [[unlikely]] attributes
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2810,7 +2810,7 @@
 
 // Helper function for GenerateTargetSpecificAttrChecks that alters the 'Test'
 // parameter with only a single check type, if applicable.
-static void GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
+static bool GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
 std::string *FnName,
 StringRef ListName,
 StringRef CheckAgainst,
@@ -2830,7 +2830,9 @@
 *FnName += Part;
 }
 Test += ")";
+return true;
   }
+  return false;
 }
 
 // Generate a conditional expression to check if the current target satisfies
@@ -2838,10 +2840,12 @@
 // those checks to the Test string. If the FnName string pointer is non-null,
 // append a unique suffix to distinguish this set of target checks from other
 // TargetSpecificAttr records.
-static void GenerateTargetSpecificAttrChecks(const Record *R,
+static bool GenerateTargetSpecificAttrChecks(const Record *R,
  std::vector ,
  std::string ,
  std::string *FnName) {
+  bool AnyTargetChecks = false;
+
   // It is assumed that there will be an llvm::Triple object
   // named "T" and a TargetInfo object named "Target" within
   // scope that can be used to determine whether the attribute exists in
@@ -2851,6 +2855,7 @@
   // differently because GenerateTargetRequirements needs to combine the list
   // with ParseKind.
   if (!Arches.empty()) {
+AnyTargetChecks = true;
 Test += " && (";
 for (auto I = Arches.begin(), E = Arches.end(); I != E; ++I) {
   StringRef Part = *I;
@@ -2865,16 +2870,19 @@
   }
 
   // If the attribute is specific to particular OSes, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "OSes", "T.getOS()",
-  "llvm::Triple::");
+  AnyTargetChecks |= GenerateTargetSpecificAttrCheck(
+  R, Test, FnName, "OSes", "T.getOS()", "llvm::Triple::");
 
   // If one or more CXX ABIs are specified, check those as well.
   GenerateTargetSpecificAttrCheck(R, Test, FnName, "CXXABIs",
   "Target.getCXXABI().getKind()",
   "TargetCXXABI::");
   // If one or more object formats is specified, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
-  "T.getObjectFormat()", "llvm::Triple::");
+  AnyTargetChecks |=
+  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
+  "T.getObjectFormat()", "llvm::Triple::");
+
+  return AnyTargetChecks;
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
@@ -3510,7 +3518,7 @@
 
   std::string FnName = "isTarget";
   std::string Test;
-  GenerateTargetSpecificAttrChecks(R, Arches, Test, );
+  bool UsesT = GenerateTargetSpecificAttrChecks(R, Arches, Test, );
 
   // If this code has already been generated, simply return the previous
   // instance of it.
@@ -3520,7 +3528,8 @@
 return *I;
 
   OS << "static bool " << FnName << "(const TargetInfo ) {\n";
-  OS << "  const llvm::Triple  = Target.getTriple();\n";
+  if (UsesT)
+OS << "  const llvm::Triple  = Target.getTriple();\n";
   OS << "  return " << Test << ";\n";
   OS << "}\n\n";
 
Index: test/SemaCXX/cxx2a-no-unique-address.cpp
===
--- /dev/null
+++ 

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/DeclCXX.h:337-341
 /// true when this class is empty for traits purposes,
-/// i.e. has no data members other than 0-width bit-fields, has no
+/// i.e. has no data members other than 0-width bit-fields and empty
+/// fields marked [[no_unique_address]], has no
 /// virtual function/base, and doesn't inherit from a non-empty
 /// class. Doesn't take union-ness into account.

aaron.ballman wrote:
> Do you mind re-flowing this entire comment to 80 cols?
Reflowed and turned into bulleted list for clarity.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205487.
rsmith marked 5 inline comments as done.
rsmith added a comment.
Herald added a reviewer: jfb.
Herald added subscribers: jfb, arphaman.

- Address review comments from @AaronBallman.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63451

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGRecordLayoutBuilder.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Parse/ParseDeclCXX.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGenCXX/no-unique-address.cpp
  test/CodeGenCXX/tail-padding.cpp
  test/Layout/no-unique-address.cpp
  test/SemaCXX/cxx2a-no-unique-address.cpp
  utils/TableGen/ClangAttrEmitter.cpp
  www/cwg_index.html
  www/cxx_status.html



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

[PATCH] D63490: Script for generating AST JSON dump test cases

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Thanks!

I think we can maybe make this a little more convenient to use. For the 
non-JSON `-ast-dump` format, I added `utils/make-ast-dump-check.sh` which can 
be used as (for example):

  $ lit -DFileCheck=$PWD/utils/make-ast-dump-check.sh test/AST/ast-dump-openmp-*

That is: run the tests, and instead of piping their output through `FileCheck`, 
run it through a script that converts the actual output into test expectations. 
That way, the script doesn't need to run `clang` (or know what command-line 
arguments would be used to do so); `lit` does that for you. I don't think that 
approach would be able to provide your `--filters` flag, but maybe that could 
be passed in as part of the `-D`?


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

https://reviews.llvm.org/D63490



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

[PATCH] D57086: Ignore trailing NullStmts in StmtExprs for GCC compatibility

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D57086#1546386 , @aaron.ballman 
wrote:

> In D57086#1535873 , @domdom wrote:
>
> > Something I should ask, it seems like GCC only ignores the NullStmts at the 
> > end if it's in C mode. Should clang match this behaviour exactly?
>
>
> I can't think of a reason that this should only happen in C mode, can you 
> @rsmith?


No, I can't think of such a reason either. When we've seen such weirdnesses 
before (eg, `break` or `continue` in a statement expression in a `for` loop 
increment expression) we generally try to pick a behavior that's consistent 
across languages, and warn on the cases where we give a different behavior from 
GCC as a result.


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

https://reviews.llvm.org/D57086



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

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprCXX.cpp:1887
+Dtor = DevirtualizedDtor;
+Ptr = CGF.EmitPointerWithAlignment(Inner);
+  } else {

yamauchi wrote:
> rsmith wrote:
> > In this case we'll emit the inner expression (including its side-effects) 
> > twice.
> > 
> > The simplest thing to do would be to just drop this `else if` case for now 
> > and add a FIXME.
> I'd go with that. I assume there isn't a simple way to adjust the this 
> pointer of a base class given a derived class? Sort of like 
> CodeGenFunction::GetAddressOfDerivedClass?
> 
I don't think we have anything quite like that. This won't be possible in the 
general case: there could be more than one base class of the current type 
within the derived class, so you'd need to take into account the path that the 
base expression took to reach the inner expression with the known dynamic type. 
(We could opportunistically do this in the common case where the derived class 
has only one base class of the base type, and it's a non-virtual base, but I 
don't think we have any existing code to do that either.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL363620: Rewrite ConstStructBuilder with a mechanism that can 
cope with splitting and… (authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63371?vs=205182=205184#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63371

Files:
  cfe/trunk/lib/CodeGen/CGExprConstant.cpp
  cfe/trunk/test/CodeGenCXX/designated-init.cpp

Index: cfe/trunk/test/CodeGenCXX/designated-init.cpp
===
--- cfe/trunk/test/CodeGenCXX/designated-init.cpp
+++ cfe/trunk/test/CodeGenCXX/designated-init.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++98 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+
+struct A { int x, y[3]; };
+struct B { A a; };
+
+// CHECK: @b = global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
+
+union U {
+  int n;
+  float f;
+};
+struct C {
+  int x;
+  U u[3];
+};
+struct D {
+  C c;
+};
+
+// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
+
+// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
+
+struct Bitfield {
+  int a : 3;
+  int b : 4;
+  int c : 5;
+};
+struct WithBitfield {
+  int n;
+  Bitfield b;
+};
+// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
+
+struct String {
+  const char buffer[12];
+};
+struct WithString {
+  String str;
+};
+// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
+
+struct LargeArray {
+  int arr[4096];
+};
+struct WithLargeArray {
+  LargeArray arr;
+};
+// CHECK: @large = global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
+
+union OverwritePaddingWithBitfield {
+  struct Padding { unsigned : 8; char c; } padding;
+  char bitfield : 3;
+};
+struct WithOverwritePaddingWithBitfield {
+  OverwritePaddingWithBitfield a;
+};
+// CHECK: @overwrite_padding = global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: cfe/trunk/lib/CodeGen/CGExprConstant.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprConstant.cpp
+++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp
@@ -22,6 +22,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
@@ -30,345 +32,636 @@
 using namespace CodeGen;
 
 //===--===//
-//ConstStructBuilder
+//ConstantAggregateBuilder
 //===--===//
 
 namespace {
 class ConstExprEmitter;
-class ConstStructBuilder {
-  CodeGenModule 
-  ConstantEmitter 
-
-  bool Packed;
-  CharUnits NextFieldOffsetInChars;
-  CharUnits LLVMStructAlignment;
-  SmallVector Elements;
-public:
-  static llvm::Constant *BuildStruct(ConstantEmitter ,
- ConstExprEmitter *ExprEmitter,
- llvm::Constant *Base,
- InitListExpr *Updater,
- QualType ValTy);
-  static llvm::Constant *BuildStruct(ConstantEmitter ,
- InitListExpr *ILE, QualType StructTy);
-  static llvm::Constant *BuildStruct(ConstantEmitter ,
- const APValue , QualType ValTy);
-
-private:
-  ConstStructBuilder(ConstantEmitter )
-: CGM(emitter.CGM), Emitter(emitter), Packed(false),
-NextFieldOffsetInChars(CharUnits::Zero()),
-LLVMStructAlignment(CharUnits::One()) { }
-
-  void AppendField(const FieldDecl *Field, uint64_t FieldOffset,
-   llvm::Constant *InitExpr);
-
-  void AppendBytes(CharUnits 

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGExprConstant.cpp:162
+  return replace(V, BeginOff, EndOff, Vals.begin(), Vals.end());
+}
+

rjmccall wrote:
> Can these go to STLExtras or somewhere similar?
Done. The use of offsets here is a bit special-case, so I've moved an iterator 
version to STLExtras and just left an offsets -> iterators adaptor here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371



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

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205182.
rsmith marked 2 inline comments as done.
rsmith added a comment.

- Address additional review comments from rjmccall.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371

Files:
  lib/CodeGen/CGExprConstant.cpp
  test/CodeGenCXX/designated-init.cpp

Index: test/CodeGenCXX/designated-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/designated-init.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++98 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+
+struct A { int x, y[3]; };
+struct B { A a; };
+
+// CHECK: @b = global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
+
+union U {
+  int n;
+  float f;
+};
+struct C {
+  int x;
+  U u[3];
+};
+struct D {
+  C c;
+};
+
+// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
+
+// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
+
+struct Bitfield {
+  int a : 3;
+  int b : 4;
+  int c : 5;
+};
+struct WithBitfield {
+  int n;
+  Bitfield b;
+};
+// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
+
+struct String {
+  const char buffer[12];
+};
+struct WithString {
+  String str;
+};
+// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
+
+struct LargeArray {
+  int arr[4096];
+};
+struct WithLargeArray {
+  LargeArray arr;
+};
+// CHECK: @large = global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
+
+union OverwritePaddingWithBitfield {
+  struct Padding { unsigned : 8; char c; } padding;
+  char bitfield : 3;
+};
+struct WithOverwritePaddingWithBitfield {
+  OverwritePaddingWithBitfield a;
+};
+// CHECK: @overwrite_padding = global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: lib/CodeGen/CGExprConstant.cpp
===
--- lib/CodeGen/CGExprConstant.cpp
+++ lib/CodeGen/CGExprConstant.cpp
@@ -22,6 +22,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
@@ -30,148 +32,562 @@
 using namespace CodeGen;
 
 //===--===//
-//ConstStructBuilder
+//ConstantAggregateBuilder
 //===--===//
 
 namespace {
 class ConstExprEmitter;
+
+struct ConstantAggregateBuilderUtils {
+  CodeGenModule 
+
+  ConstantAggregateBuilderUtils(CodeGenModule ) : CGM(CGM) {}
+
+  CharUnits getAlignment(const llvm::Constant *C) const {
+return CharUnits::fromQuantity(
+CGM.getDataLayout().getABITypeAlignment(C->getType()));
+  }
+
+  CharUnits getSize(llvm::Type *Ty) const {
+return CharUnits::fromQuantity(CGM.getDataLayout().getTypeAllocSize(Ty));
+  }
+
+  CharUnits getSize(const llvm::Constant *C) const {
+return getSize(C->getType());
+  }
+
+  llvm::Constant *getPadding(CharUnits PadSize) const {
+llvm::Type *Ty = CGM.Int8Ty;
+if (PadSize > CharUnits::One())
+  Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
+return llvm::UndefValue::get(Ty);
+  }
+
+  llvm::Constant *getZeroes(CharUnits ZeroSize) const {
+llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+return llvm::ConstantAggregateZero::get(Ty);
+  }
+};
+
+/// Incremental builder for an llvm::Constant* holding a struct or array
+/// constant.
+class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils {
+  /// The elements of the constant. These two arrays must have the same size;
+  /// Offsets[i] describes the offset of Elems[i] within the constant. The
+  /// elements are kept in increasing offset order, and we ensure that there
+  /// is no overlap: Offsets[i+1] >= Offsets[i] + getSize(Elemes[i]).
+  ///
+  /// This may contain explicit padding elements (in order to 

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGExprConstant.cpp:967
 // Constant folding is currently missing support for a few features supported
 // here: CK_ToUnion, CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr.
 class ConstExprEmitter :

efriedma wrote:
> Like the comment here says, we're awfully close to being able to just 
> completely kill off ConstExprEmitter etc. Do we really want to continue to 
> invest effort into this code?
The new code is also the mechanism by which we emit `APValue` constants, which 
is the replacement for `ConstExprEmitter`.

Also I don't think we have a fix for the performance issues we saw from 
performing frontend evaluation of large `InitListExpr`s yet, which is another 
blocker for removing `ConstExprEmitter` in favor of `APValue` emission (though 
I don't expect that to be a problem forever; `APValue` is long overdue an 
overhaul).


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371



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

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205162.
rsmith added a comment.

- Fix somewhat-incorrect comment on Size member.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371

Files:
  lib/CodeGen/CGExprConstant.cpp
  test/CodeGenCXX/designated-init.cpp

Index: test/CodeGenCXX/designated-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/designated-init.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++98 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+
+struct A { int x, y[3]; };
+struct B { A a; };
+
+// CHECK: @b = global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
+
+union U {
+  int n;
+  float f;
+};
+struct C {
+  int x;
+  U u[3];
+};
+struct D {
+  C c;
+};
+
+// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
+
+// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
+
+struct Bitfield {
+  int a : 3;
+  int b : 4;
+  int c : 5;
+};
+struct WithBitfield {
+  int n;
+  Bitfield b;
+};
+// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
+
+struct String {
+  const char buffer[12];
+};
+struct WithString {
+  String str;
+};
+// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
+
+struct LargeArray {
+  int arr[4096];
+};
+struct WithLargeArray {
+  LargeArray arr;
+};
+// CHECK: @large = global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
+
+union OverwritePaddingWithBitfield {
+  struct Padding { unsigned : 8; char c; } padding;
+  char bitfield : 3;
+};
+struct WithOverwritePaddingWithBitfield {
+  OverwritePaddingWithBitfield a;
+};
+// CHECK: @overwrite_padding = global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: lib/CodeGen/CGExprConstant.cpp
===
--- lib/CodeGen/CGExprConstant.cpp
+++ lib/CodeGen/CGExprConstant.cpp
@@ -22,6 +22,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
@@ -30,148 +32,585 @@
 using namespace CodeGen;
 
 //===--===//
-//ConstStructBuilder
+//ConstantAggregateBuilder
 //===--===//
 
 namespace {
 class ConstExprEmitter;
+
+struct ConstantAggregateBuilderUtils {
+  CodeGenModule 
+
+  ConstantAggregateBuilderUtils(CodeGenModule ) : CGM(CGM) {}
+
+  CharUnits getAlignment(const llvm::Constant *C) const {
+return CharUnits::fromQuantity(
+CGM.getDataLayout().getABITypeAlignment(C->getType()));
+  }
+
+  CharUnits getSize(llvm::Type *Ty) const {
+return CharUnits::fromQuantity(CGM.getDataLayout().getTypeAllocSize(Ty));
+  }
+
+  CharUnits getSize(const llvm::Constant *C) const {
+return getSize(C->getType());
+  }
+
+  llvm::Constant *getPadding(CharUnits PadSize) const {
+llvm::Type *Ty = CGM.Int8Ty;
+if (PadSize > CharUnits::One())
+  Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
+return llvm::UndefValue::get(Ty);
+  }
+
+  llvm::Constant *getZeroes(CharUnits ZeroSize) const {
+llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+return llvm::ConstantAggregateZero::get(Ty);
+  }
+};
+
+/// Incremental builder for an llvm::Constant* holding a struct or array
+/// constant.
+class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils {
+  /// The elements of the constant. These two arrays must have the same size;
+  /// Offsets[i] describes the offset of Elems[i] within the constant. The
+  /// elements are kept in increasing offset order, and we ensure that there
+  /// is no overlap: Offsets[i+1] >= Offsets[i] + getSize(Elemes[i]).
+  ///
+  /// This may contain explicit padding elements (in order to create a
+  /// natural layout), but need not. 

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprConstant.cpp:73
+/// Incremental builder for an llvm::Constant* holding a structure constant.
+class ConstantBuilder : private ConstantBuilderUtils {
+  llvm::SmallVector Elems;

rjmccall wrote:
> This seems like a very generic name for this type.
It is intended to be a very generic type. (I was trying to arrange it so that 
it could possibly be moved to LLVM eventually. I heard that globalopt would 
benefit from being able to do this kind of constant splitting / reforming.) Is 
`ConstantAggregateBuilder` sufficiently more precise?



Comment at: lib/CodeGen/CGExprConstant.cpp:75
+  llvm::SmallVector Elems;
+  llvm::SmallVector Offsets;
+  CharUnits Size = CharUnits::Zero();

rjmccall wrote:
> Are there invariants about these?  I assume they're parallel arrays; are they 
> kept sorted?
Added comments to explain.



Comment at: lib/CodeGen/CGExprConstant.cpp:76
+  llvm::SmallVector Offsets;
+  CharUnits Size = CharUnits::Zero();
+

rjmccall wrote:
> This is one past the last byte that's been covered by an actual `Constant*` 
> value, or does it include unoccupied padding, or does it exclude even 
> occupied padding?
Added comment to explain.



Comment at: lib/CodeGen/CGExprConstant.cpp:98
+Offsets.reserve(N);
+  }
+

rjmccall wrote:
> Might be worth clarifying what `N` is here.
Looks like this ended up being unused; removed.



Comment at: lib/CodeGen/CGExprConstant.cpp:190
+bool ConstantBuilder::addBits(llvm::APInt Bits, uint64_t OffsetInBits,
+  bool AllowOverwrite) {
+  const ASTContext  = CGM.getContext();

rjmccall wrote:
> `AllowOversized` (which you used in the interface) seems like a better name.
`AllowOversized` is used to mean "the size of the constant may be larger than 
the size of the type", and is a parameter to `build` / `buildFrom`.
`AllowOverwrite` is used to mean "adding this constant may overwrite something 
you've already been given", and is a parameter to `add` / `addBits`.

I can make these names more different from each other if that would help?



Comment at: lib/CodeGen/CGExprConstant.cpp:258
+  return false;
+assert(CI->getBitWidth() == CharWidth && "splitAt failed");
+assert((!(CI->getValue() & UpdateMask) || AllowOverwrite) &&

rjmccall wrote:
> Oh, because we're splitting before and after a single-`CharUnits` range?  
> That seems worthy of a somewhat clearer explanation in the code.
> 
> I guess we could have a non-`ConstantInt` single-byte value.  Unlikely but 
> not impossible. :)
Yes. It's not too hard to craft a testcase where we'd get an explicit `undef` 
here; added handling for that and for all-zeroes constants, which are both 
correctly handled by overwriting the whole byte.



Comment at: lib/CodeGen/CGExprConstant.cpp:375
+
+  // FIXME: We could theoretically split a ConstantInt if the need ever arose.
+

rjmccall wrote:
> Does this not come up all the time with bit-fields?  I guess we emit them in 
> single-`char` chunks, so it wouldn't.  Probably worth a comment.
Done.

We could hit this case for cases such as:

```
union U { int a; int b : 3; };
struct S { U u; };
S s = {(union U){1234}, .u.b = 5};
```

(which `CodeGen` currently rejects with "cannot compile this static initializer 
yet" in C), and splitting the `ConstantInt` would allow us to emit that 
initializer as a constant, but I'm not sure it's worthwhile unless it lets us 
simplify or improve bitfield emission in general. (The above isn't a case that 
C requires us to treat as a constant initializer, so rejecting it is not a 
conformance issue.)

Maybe instead of splitting bitfields into 1-byte chunks like we currently do, 
we should try to combine them into a single `iN`, like `CGRecordLayoutBuilder` 
does. But splitting to `i8` maintains the status quo, which is what I was 
aiming for in this patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371



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

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 205161.
rsmith marked 13 inline comments as done.
rsmith added a comment.

- Address review comments from rjmccall.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371

Files:
  lib/CodeGen/CGExprConstant.cpp
  test/CodeGenCXX/designated-init.cpp

Index: test/CodeGenCXX/designated-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/designated-init.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++98 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+
+struct A { int x, y[3]; };
+struct B { A a; };
+
+// CHECK: @b = global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
+
+union U {
+  int n;
+  float f;
+};
+struct C {
+  int x;
+  U u[3];
+};
+struct D {
+  C c;
+};
+
+// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
+
+// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
+
+struct Bitfield {
+  int a : 3;
+  int b : 4;
+  int c : 5;
+};
+struct WithBitfield {
+  int n;
+  Bitfield b;
+};
+// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
+
+struct String {
+  const char buffer[12];
+};
+struct WithString {
+  String str;
+};
+// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
+
+struct LargeArray {
+  int arr[4096];
+};
+struct WithLargeArray {
+  LargeArray arr;
+};
+// CHECK: @large = global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
+
+union OverwritePaddingWithBitfield {
+  struct Padding { unsigned : 8; char c; } padding;
+  char bitfield : 3;
+};
+struct WithOverwritePaddingWithBitfield {
+  OverwritePaddingWithBitfield a;
+};
+// CHECK: @overwrite_padding = global { { i8, i8 } } { { i8, i8 } { i8 3, i8 1 } }
+WithOverwritePaddingWithBitfield overwrite_padding = {(OverwritePaddingWithBitfield){1}, .a.bitfield = 3};
Index: lib/CodeGen/CGExprConstant.cpp
===
--- lib/CodeGen/CGExprConstant.cpp
+++ lib/CodeGen/CGExprConstant.cpp
@@ -22,6 +22,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
@@ -30,148 +32,583 @@
 using namespace CodeGen;
 
 //===--===//
-//ConstStructBuilder
+//ConstantAggregateBuilder
 //===--===//
 
 namespace {
 class ConstExprEmitter;
+
+struct ConstantAggregateBuilderUtils {
+  CodeGenModule 
+
+  ConstantAggregateBuilderUtils(CodeGenModule ) : CGM(CGM) {}
+
+  CharUnits getAlignment(const llvm::Constant *C) const {
+return CharUnits::fromQuantity(
+CGM.getDataLayout().getABITypeAlignment(C->getType()));
+  }
+
+  CharUnits getSize(llvm::Type *Ty) const {
+return CharUnits::fromQuantity(CGM.getDataLayout().getTypeAllocSize(Ty));
+  }
+
+  CharUnits getSize(const llvm::Constant *C) const {
+return getSize(C->getType());
+  }
+
+  llvm::Constant *getPadding(CharUnits PadSize) const {
+llvm::Type *Ty = CGM.Int8Ty;
+if (PadSize > CharUnits::One())
+  Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
+return llvm::UndefValue::get(Ty);
+  }
+
+  llvm::Constant *getZeroes(CharUnits ZeroSize) const {
+llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+return llvm::ConstantAggregateZero::get(Ty);
+  }
+};
+
+/// Incremental builder for an llvm::Constant* holding a struct or array
+/// constant.
+class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils {
+  /// The elements of the constant. These two arrays must have the same size;
+  /// Offsets[i] describes the offset of Elems[i] within the constant. The
+  /// elements are kept in increasing offset order, and we ensure that there
+  /// is no overlap: Offsets[i+1] >= Offsets[i] + getSize(Elemes[i]).
+  ///
+  /// This may contain explicit padding elements (in order to create a
+  /// 

[PATCH] D63376: [clang] Small improvments after Adding APValue to ConstantExpr

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Nice cleanup!




Comment at: clang/include/clang/Sema/Sema.h:10298
+ bool AllowFold = true,
+ bool StoreResult = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,

Do you need this new flag? No callers are passing it.


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

https://reviews.llvm.org/D63376



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

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: rjmccall.
Herald added subscribers: aheejin, dschuff.
Herald added a project: clang.

Add support for the C++2a [[no_unique_address]] attribute for targets using the 
Itanium C++ ABI.

This depends on D63371 .


Repository:
  rC Clang

https://reviews.llvm.org/D63451

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGRecordLayoutBuilder.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGenCXX/no-unique-address.cpp
  test/CodeGenCXX/tail-padding.cpp
  test/Layout/no-unique-address.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2810,7 +2810,7 @@
 
 // Helper function for GenerateTargetSpecificAttrChecks that alters the 'Test'
 // parameter with only a single check type, if applicable.
-static void GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
+static bool GenerateTargetSpecificAttrCheck(const Record *R, std::string ,
 std::string *FnName,
 StringRef ListName,
 StringRef CheckAgainst,
@@ -2830,7 +2830,9 @@
 *FnName += Part;
 }
 Test += ")";
+return true;
   }
+  return false;
 }
 
 // Generate a conditional expression to check if the current target satisfies
@@ -2838,10 +2840,12 @@
 // those checks to the Test string. If the FnName string pointer is non-null,
 // append a unique suffix to distinguish this set of target checks from other
 // TargetSpecificAttr records.
-static void GenerateTargetSpecificAttrChecks(const Record *R,
+static bool GenerateTargetSpecificAttrChecks(const Record *R,
  std::vector ,
  std::string ,
  std::string *FnName) {
+  bool AnyTargetChecks = false;
+
   // It is assumed that there will be an llvm::Triple object
   // named "T" and a TargetInfo object named "Target" within
   // scope that can be used to determine whether the attribute exists in
@@ -2851,6 +2855,7 @@
   // differently because GenerateTargetRequirements needs to combine the list
   // with ParseKind.
   if (!Arches.empty()) {
+AnyTargetChecks = true;
 Test += " && (";
 for (auto I = Arches.begin(), E = Arches.end(); I != E; ++I) {
   StringRef Part = *I;
@@ -2865,16 +2870,19 @@
   }
 
   // If the attribute is specific to particular OSes, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "OSes", "T.getOS()",
-  "llvm::Triple::");
+  AnyTargetChecks |= GenerateTargetSpecificAttrCheck(
+  R, Test, FnName, "OSes", "T.getOS()", "llvm::Triple::");
 
   // If one or more CXX ABIs are specified, check those as well.
   GenerateTargetSpecificAttrCheck(R, Test, FnName, "CXXABIs",
   "Target.getCXXABI().getKind()",
   "TargetCXXABI::");
   // If one or more object formats is specified, check those.
-  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
-  "T.getObjectFormat()", "llvm::Triple::");
+  AnyTargetChecks |=
+  GenerateTargetSpecificAttrCheck(R, Test, FnName, "ObjectFormats",
+  "T.getObjectFormat()", "llvm::Triple::");
+
+  return AnyTargetChecks;
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
@@ -3510,7 +3518,7 @@
 
   std::string FnName = "isTarget";
   std::string Test;
-  GenerateTargetSpecificAttrChecks(R, Arches, Test, );
+  bool UsesT = GenerateTargetSpecificAttrChecks(R, Arches, Test, );
 
   // If this code has already been generated, simply return the previous
   // instance of it.
@@ -3520,7 +3528,8 @@
 return *I;
 
   OS << "static bool " << FnName << "(const TargetInfo ) {\n";
-  OS << "  const llvm::Triple  = Target.getTriple();\n";
+  if (UsesT)
+OS << "  const llvm::Triple  = Target.getTriple();\n";
   OS << "  return " << Test << ";\n";
   OS << "}\n\n";
 
Index: test/Layout/no-unique-address.cpp
===
--- /dev/null
+++ test/Layout/no-unique-address.cpp
@@ -0,0 +1,265 @@
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s
+
+namespace Empty {
+  struct A {};
+  struct B { [[no_unique_address]] A a; char b; };
+  static_assert(sizeof(B) == 1);
+
+  // CHECK:*** Dumping AST 

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D63371#1546500 , @rjmccall wrote:

> Isn't `[[no_unique_address]]` only significant for empty members?  I'm not 
> sure why they need significant support from constant-building, since they 
> expand to no meaningful initializer.


It also permits reuse of tail padding for non-static data members, which is the 
complexity that this patch is dealing with (in addition to improving and 
generalizing the support for non-trivial designated initializers).

> We have some code we'll hopefully be upstreaming soon that relies on being 
> able to do things with address-of-position placeholder values; I'm a little 
> worried that the new structure here doesn't really support them.

Can you say a bit more about that? (Do you want to be able to emit a constant 
that denotes a pointer to somewhere else within the same constant being 
emitted, or something like that?) I think this approach should be strictly more 
general than what we had before, but perhaps that means it can't be extended in 
the direction you need?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371



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

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: rjmccall.
Herald added a project: clang.

This adds a ConstantBuilder class that deals with incrementally building
an aggregate constant, including support for overwriting
previously-emitted parts of the aggregate with new values.

This fixes a bunch of cases where we used to be unable to reduce a
DesignatedInitUpdateExpr down to an IR constant, and also lays some
groundwork for emission of class constants with [[no_unique_address]]
members.


Repository:
  rC Clang

https://reviews.llvm.org/D63371

Files:
  lib/CodeGen/CGExprConstant.cpp
  test/CodeGenCXX/designated-init.cpp

Index: test/CodeGenCXX/designated-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/designated-init.cpp
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -std=c++98 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -o - %s -triple x86_64-linux-gnu | FileCheck %s
+
+struct A { int x, y[3]; };
+struct B { A a; };
+
+// CHECK: @b = global %{{[^ ]*}} { %{{[^ ]*}} { i32 1, [3 x i32] [i32 2, i32 5, i32 4] } }
+B b = {(A){1, 2, 3, 4}, .a.y[1] = 5};
+
+union U {
+  int n;
+  float f;
+};
+struct C {
+  int x;
+  U u[3];
+};
+struct D {
+  C c;
+};
+
+// CHECK: @d1 = {{.*}} { i32 1, [3 x %[[U:.*]]] [%[[U]] { i32 2 }, %[[U]] { i32 5 }, %[[U]] { i32 4 }] }
+D d1 = {(C){1, {{.n=2}, {.f=3}, {.n=4}}}, .c.u[1].n = 5};
+
+// CHECK: @d2 = {{.*}} { i32 1, { %[[U]], float, %[[U]] } { %[[U]] { i32 2 }, float 5.{{0*}}e+00, %[[U]] { i32 4 } } }
+D d2 = {(C){1, 2, 3, 4}, .c.u[1].f = 5};
+
+struct Bitfield {
+  int a : 3;
+  int b : 4;
+  int c : 5;
+};
+struct WithBitfield {
+  int n;
+  Bitfield b;
+};
+// CHECK: @bitfield = {{.*}} { i32 1, { i8, i8, [2 x i8] } { i8 42, i8 2, [2 x i8] undef } }
+WithBitfield bitfield = {1, (Bitfield){2, 3, 4}, .b.b = 5};
+
+struct String {
+  const char buffer[12];
+};
+struct WithString {
+  String str;
+};
+// CHECK: @string = {{.*}} [12 x i8] c"Hello World\00" } }
+WithString string = {(String){"hello world"}, .str.buffer[0] = 'H', .str.buffer[6] = 'W'};
+
+struct LargeArray {
+  int arr[4096];
+};
+struct WithLargeArray {
+  LargeArray arr;
+};
+// CHECK: @large = global { { <{ [11 x i32], [4085 x i32] }> } } { { <{ [11 x i32], [4085 x i32] }> } { <{ [11 x i32], [4085 x i32] }> <{ [11 x i32] [i32 1, i32 2, i32 3, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 10], [4085 x i32] zeroinitializer }> } }
+WithLargeArray large = {(LargeArray){1, 2, 3}, .arr.arr[10] = 10};
Index: lib/CodeGen/CGExprConstant.cpp
===
--- lib/CodeGen/CGExprConstant.cpp
+++ lib/CodeGen/CGExprConstant.cpp
@@ -22,6 +22,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
@@ -30,148 +32,559 @@
 using namespace CodeGen;
 
 //===--===//
-//ConstStructBuilder
+//ConstantBuilder
 //===--===//
 
 namespace {
 class ConstExprEmitter;
+
+struct ConstantBuilderUtils {
+  CodeGenModule 
+
+  ConstantBuilderUtils(CodeGenModule ) : CGM(CGM) {}
+
+  CharUnits getAlignment(const llvm::Constant *C) const {
+return CharUnits::fromQuantity(
+CGM.getDataLayout().getABITypeAlignment(C->getType()));
+  }
+
+  CharUnits getSize(llvm::Type *Ty) const {
+return CharUnits::fromQuantity(CGM.getDataLayout().getTypeAllocSize(Ty));
+  }
+
+  CharUnits getSize(const llvm::Constant *C) const {
+return getSize(C->getType());
+  }
+
+  llvm::Constant *getPadding(CharUnits PadSize) const {
+llvm::Type *Ty = CGM.Int8Ty;
+if (PadSize > CharUnits::One())
+  Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
+return llvm::UndefValue::get(Ty);
+  }
+
+  llvm::Constant *getZeroes(CharUnits ZeroSize) const {
+llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+return llvm::ConstantAggregateZero::get(Ty);
+  }
+};
+
+/// Incremental builder for an llvm::Constant* holding a structure constant.
+class ConstantBuilder : private ConstantBuilderUtils {
+  llvm::SmallVector Elems;
+  llvm::SmallVector Offsets;
+  CharUnits Size = CharUnits::Zero();
+
+  /// This is true only if laying out Elems in order as the elements of a
+  /// non-packed LLVM struct will give the correct layout.
+  bool NaturalLayout = true;
+
+  bool split(size_t Index, CharUnits Hint);
+  Optional splitAt(CharUnits Pos);
+
+  static llvm::Constant *buildFrom(CodeGenModule ,
+   ArrayRef Elems,
+   ArrayRef Offsets,
+   CharUnits 

[PATCH] D63369: [AST] Fixed extraneous warnings for binary conditional operator

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/AST/Expr.cpp:2351-2352
   return false;
 if (!Exp->getLHS())
   return true;
 return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);

Looks like whoever wrote this expected to handle binary conditional operators 
here. Maybe delete this check since it's impossible for a ternary conditional 
operator?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63369



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

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprCXX.cpp:1867-1868
 
-  if (Dtor->isVirtual()) {
+  if (Dtor->isVirtual() &&
+  !(Dtor->hasAttr() || RD->hasAttr())) {
 CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,

yamauchi wrote:
> rsmith wrote:
> > Can we use `getDevirtualizedMethod` here instead? That catches a few other 
> > cases that just checking for `final` will miss, and disables this for cases 
> > where it will currently do the wrong thing (eg, `-fapple-kext`). See 
> > `EmitCXXMemberOrOperatorMemberCallExpr` for where we do this for other 
> > virtual calls.
> > 
> > As a particularly terrible example:
> > 
> > ```
> > struct A { virtual ~A() final = 0; };
> > void evil(A *p) { delete p; }
> > ```
> > 
> > is (amazingly) valid, and must not be devirtualized because there is no 
> > requirement that the destructor of `A` have a definition in this program. 
> > (It would, however, be correct to optimize out the entire `delete` 
> > expression in this case, on the basis that `p` must always be `nullptr` 
> > whenever it's reached. But that doesn't really seem worthwhile.)
> Updated. Hopefully this does the right thing.
Please can you add that test to your testcases too (eg, with a `CHECK-NOT` to 
ensure we don't reference the destructor of `A`)? It's subtle enough that I 
could imagine us getting that wrong in the future.



Comment at: lib/CodeGen/CGExprCXX.cpp:1887
+Dtor = DevirtualizedDtor;
+Ptr = CGF.EmitPointerWithAlignment(Inner);
+  } else {

In this case we'll emit the inner expression (including its side-effects) twice.

The simplest thing to do would be to just drop this `else if` case for now and 
add a FIXME.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62825#1542662 , @rjmccall wrote:

> In D62825#1542639 , @rsmith wrote:
>
> > In my view, the mistake was specifying `nullptr_t` to have the same size 
> > and alignment as `void*`; it should instead be an empty type. Only 
> > confusion results from making it "look like" a pointer type rather than 
> > just being an empty tag type.
>
>
> Perhaps, but that's clearly unfixable without breaking ABI, whereas the 
> object-representation issue is fixable by, at most, requiring a few stores 
> that might be difficult to eliminate in some fanciful situations.


Requiring initialization of, or assignment to, an object of type nullptr_t to 
actually store a null pointer value is also an ABI break. (Eg, `void f() { 
decltype(nullptr) n; g(); }` does not initialize `n` today in GCC, Clang, or 
EDG, but would need to do so under the new rule.)

In any case, I've started a discussion on the core reflector. We'll see where 
that goes.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62825#1542597 , @rjmccall wrote:

> In D62825#1542374 , @rsmith wrote:
>
> > In D62825#1542309 , @rjmccall 
> > wrote:
> >
> > > In D62825#1542301 , @rsmith 
> > > wrote:
> > >
> > > > In D62825#1542247 , @rjmccall 
> > > > wrote:
> > > >
> > > > > In what sense is the bit-pattern of a null pointer indeterminate?
> > > >
> > > >
> > > > The problem is not null pointers, it's `nullptr_t`, which is required 
> > > > to have the same size and alignment as `void*` but which comprises only 
> > > > padding bits. (Loads of `nullptr_t` are not even permitted to touch 
> > > > memory...).
> > >
> > >
> > > I mean, I know this is C++ and the committee loves tying itself in knots 
> > > to make the language unnecessarily unusable, but surely the semantics of 
> > > bitcasting an r-value of type `nullptr_t` are intended to be equivalent 
> > > to bitcasting an r-value of type `void*` that happens to be a null 
> > > pointer.
> >
> >
> > I don't follow -- why would they be? `bit_cast` reads the object 
> > representation, which for `nullptr_t` is likely to be uninitialized, 
> > because the type contains only padding bits. (Note that there is formally 
> > no such thing as "bitcasting an rvalue". `bit_cast` takes an lvalue, and 
> > reinterprets its storage.)
>
>
> I agree that the problem is that the object representation of `nullptr_t` is 
> wrong, but it seems absurd to me that we're going to bake in an absurd 
> special case (from the user's perspective) to `bit_cast` because we consider 
> that representation unfixable.


Note that `bit_cast` supports casting not just from fundamental types but also 
from aggregates (which might contain a `nullptr_t`). At runtime, we're going to 
`memcpy` from the source object, which will leave the bits in the destination 
corresponding to `nullptr_t` subobjects in the source uninitialized (if they 
were in fact uninitialized in the source object, which they might be). It would 
be unreasonable to initialize the bits in the `bit_cast` result during 
compile-time evaluation but not during runtime evaluation -- compile-time 
evaluation is supposed to diagnose cases that would be undefined at runtime, 
not define them.

In my view, the mistake was specifying `nullptr_t` to have the same size and 
alignment as `void*`; it should instead be an empty type. Only confusion 
results from making it "look like" a pointer type rather than just being an 
empty tag type.


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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:5360
+  // FIXME: Its possible under the C++ standard for 'char' to not be 8 bits, 
but
+  // we don't support a host or target where that is the case. Still, we should
+  // use a more generic type in case we ever do.

A `static_assert(std::numeric_limits::digits >= 8);` would be 
nice.



Comment at: clang/lib/AST/ExprConstant.cpp:5461-5469
+case APValue::LValue: {
+  LValue LVal;
+  LVal.setFrom(Info.Ctx, Val);
+  APValue RVal;
+  if (!handleLValueToRValueConversion(Info, BCE, Ty.withConst(),
+  LVal, RVal))
+return false;

This looks wrong: bitcasting a pointer should not dereference the pointer and 
store its pointee! (Likewise for a reference member.) But I think this should 
actually simply be unreachable: we already checked for pointers and reference 
members in `checkBitCastConstexprEligibilityType`.

(However, I think it currently *is* reached, because there's also some cases 
where the operand of the bitcast is an lvalue, and the resulting lvalue gets 
misinterpreted as a value of the underlying type, landing us here. See below.)



Comment at: clang/lib/AST/ExprConstant.cpp:5761-5764
+for (FieldDecl *FD : Record->fields()) {
+  if (!checkBitCastConstexprEligibilityType(Loc, FD->getType(), Info, Ctx))
+return note(0, FD->getType(), FD->getBeginLoc());
+}

The spec for `bit_cast` also asks us to check for members of reference type. 
That can't happen because a type with reference members can never be 
trivially-copyable, but please include an assert here to demonstrate to a 
reader that we've thought about and handled that case.



Comment at: clang/lib/AST/ExprConstant.cpp:7098-7099
   case CK_BitCast:
+// CK_BitCast originating from a __builtin_bit_cast have different 
constexpr
+// semantics. This is only reachable when bit casting to nullptr_t.
+if (isa(E))

I think this is reversed from the way I'd think about it: casts to `void*` are 
modeled as bit-casts, and so need special handling here. (Theoretically it'd be 
preferable to call the base class function for all other kinds of bitcast we 
encounter, but it turns out to not matter because only bitcasts to `nullptr_t` 
are ever evaluatable currently. In future we might want to support bit-casts 
between integers and pointers (for constant folding only), and then it might 
matter.)



Comment at: clang/lib/Sema/SemaCast.cpp:2801-2802
+void CastOperation::CheckBitCast() {
+  if (isPlaceholder())
+SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
+

erik.pilkington wrote:
> rsmith wrote:
> > Should we be performing the default lvalue conversions here too? Or maybe 
> > materializing a temporary? (Are we expecting a glvalue or prvalue as the 
> > operand of `bit_cast`? It seems unnecessarily complex for the AST 
> > representation to allow either.)
> Okay, new patch filters the expression through DefaultLvalueConversion (which 
> deals with placeholders too).
Hmm, sorry, on reflection I think I've led you down the wrong path here.

I think we really do want the operand of the bitcast here to be a glvalue. In 
the case where the operand is of class type, it will be an lvalue even after 
`DefaultLvalueConversion`, because there's no such thing as a 
`CK_LValueToRValue` conversion on class type in C++. (For example, this is why 
you currently need the code in `BitCastReader` that deals with 
`APValue::LValue` -- when bitcasting a class type, you get an lvalue denoting 
the object rather than the object itself.)

Also, forming an lvalue-to-rvalue conversion here is at least theoretically 
wrong, because that conversion (notionally) discards the values of padding bits 
in the "from" object, whereas `std::bit_cast` requires those values to be 
preserved into the destination if they are defined in the source object.

Perhaps we should also have different cast kinds for an lvalue-to-rvalue 
bitcast (this new thing) versus an rvalue-to-rvalue bitcast (`CK_BitCast`).



Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:100-112
+void test_array(int ()[2]) {
+  // CHECK-LABEL: define void @_Z10test_arrayRA2_i
+  __builtin_bit_cast(unsigned long, ary);
+
+  // CHECK: [[ARY_PTR:%.*]] = alloca [2 x i32]*, align 8
+  // CHECK-NEXT: [[TEMP]] = alloca [2 x i32], align 8
+  // store ary into ARY_PTR

If we remove the `DefaultLvalueConversion` from building the bitcast 
expression, we should be able to avoid the unnecessary extra alloca here, and 
instead `memcpy` directly from `ary` to the `unsigned long`. (This is the most 
important testcase in this file, since it's the only one that gives 
`__builtin_bit_cast` an lvalue operand, which is what `std::bit_cast` will do.)

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprCXX.cpp:1871
+ CGF.CGM.getLangOpts().AppleKext)))
+  Dtor = DevirtualizedDtor;
+else {

`Dtor` could be the destructor for a type derived from `ElementPtr`. We'll 
either need to somehow emit a cast to the correct derived type here, or just 
abort the devirtualization and emit a virtual call in that case. (The latter is 
what `EmitCXXMemberOrOperatorMemberCallExpr` does -- see its checks on 
`getCXXRecord(Base)` and `getCXXRecord(Inner)`.)

Eg, for:

```
struct SomethingElse { virtual void f(); };
struct A {
  virtual ~A();
};
struct B : SomethingElse, A {
  ~B() final;
};
void f(B *p) {
  delete (A*)p;
}
```

... `Ptr` will be a pointer to an `A` subobject of a `B` object, but 
`getDevirtualizedMethod` will still be able to work out that `B::~B` is the 
right thing to call here (by looking through the cast and finding that the 
pointer must actually point to a `B` object whose destructor is `final`). We 
need to either convert `Ptr` back from an `A*` to a `B*` or just not 
devirtualize this call.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

(You might argue that it's ridiculous to require that `nullptr_t` have the same 
size and alignment as `void*` but not have the same storage representation as a 
null `void*`. I'd agree, and I've raised this in committee before, but without 
success)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62825#1542309 , @rjmccall wrote:

> In D62825#1542301 , @rsmith wrote:
>
> > In D62825#1542247 , @rjmccall 
> > wrote:
> >
> > > In what sense is the bit-pattern of a null pointer indeterminate?
> >
> >
> > The problem is not null pointers, it's `nullptr_t`, which is required to 
> > have the same size and alignment as `void*` but which comprises only 
> > padding bits. (Loads of `nullptr_t` are not even permitted to touch 
> > memory...).
>
>
> I mean, I know this is C++ and the committee loves tying itself in knots to 
> make the language unnecessarily unusable, but surely the semantics of 
> bitcasting an r-value of type `nullptr_t` are intended to be equivalent to 
> bitcasting an r-value of type `void*` that happens to be a null pointer.


I don't follow -- why would they be? `bit_cast` reads the object 
representation, which for `nullptr_t` is likely to be uninitialized, because 
the type contains only padding bits. (Note that there is formally no such thing 
as "bitcasting an rvalue". `bit_cast` takes an lvalue, and reinterprets its 
storage.)

`nullptr_t` is modeled roughly as if:

  struct alignas(void*) nullptr_t {
template operator T*() { return nullptr; }
  }

and as with that struct type,`bit_cast` of `nullptr_t` to some other type 
should (presumably) result in an uninitialized value.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL363295: C++ DR712 and others: handle non-odr-use resulting 
from an lvalue-to-rvalue… (authored by rsmith, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63157?vs=204589=204599#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63157

Files:
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.h
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/CXX/basic/basic.def.odr/p2.cpp
  cfe/trunk/test/CXX/drs/dr20xx.cpp
  cfe/trunk/test/CXX/drs/dr21xx.cpp
  cfe/trunk/test/CXX/drs/dr23xx.cpp
  cfe/trunk/test/CXX/drs/dr6xx.cpp
  cfe/trunk/test/CXX/drs/dr7xx.cpp
  cfe/trunk/test/CodeGenCXX/no-odr-use.cpp
  cfe/trunk/www/cxx_dr_status.html

Index: cfe/trunk/lib/CodeGen/CGDecl.cpp
===
--- cfe/trunk/lib/CodeGen/CGDecl.cpp
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp
@@ -1077,17 +1077,16 @@
   return constant;
 }
 
-static Address createUnnamedGlobalFrom(CodeGenModule , const VarDecl ,
-   CGBuilderTy ,
-   llvm::Constant *Constant,
-   CharUnits Align) {
+Address CodeGenModule::createUnnamedGlobalFrom(const VarDecl ,
+   llvm::Constant *Constant,
+   CharUnits Align) {
   auto FunctionName = [&](const DeclContext *DC) -> std::string {
 if (const auto *FD = dyn_cast(DC)) {
   if (const auto *CC = dyn_cast(FD))
 return CC->getNameAsString();
   if (const auto *CD = dyn_cast(FD))
 return CD->getNameAsString();
-  return CGM.getMangledName(FD);
+  return getMangledName(FD);
 } else if (const auto *OM = dyn_cast(DC)) {
   return OM->getNameAsString();
 } else if (isa(DC)) {
@@ -1099,22 +1098,39 @@
 }
   };
 
-  auto *Ty = Constant->getType();
-  bool isConstant = true;
-  llvm::GlobalVariable *InsertBefore = nullptr;
-  unsigned AS = CGM.getContext().getTargetAddressSpace(
-  CGM.getStringLiteralAddressSpace());
-  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
-  CGM.getModule(), Ty, isConstant, llvm::GlobalValue::PrivateLinkage,
-  Constant,
-  "__const." + FunctionName(D.getParentFunctionOrMethod()) + "." +
-  D.getName(),
-  InsertBefore, llvm::GlobalValue::NotThreadLocal, AS);
-  GV->setAlignment(Align.getQuantity());
-  GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
-
-  Address SrcPtr = Address(GV, Align);
-  llvm::Type *BP = llvm::PointerType::getInt8PtrTy(CGM.getLLVMContext(), AS);
+  // Form a simple per-variable cache of these values in case we find we
+  // want to reuse them.
+  llvm::GlobalVariable * = InitializerConstants[];
+  if (!CacheEntry || CacheEntry->getInitializer() != Constant) {
+auto *Ty = Constant->getType();
+bool isConstant = true;
+llvm::GlobalVariable *InsertBefore = nullptr;
+unsigned AS =
+getContext().getTargetAddressSpace(getStringLiteralAddressSpace());
+llvm::GlobalVariable *GV = new llvm::GlobalVariable(
+getModule(), Ty, isConstant, llvm::GlobalValue::PrivateLinkage,
+Constant,
+"__const." + FunctionName(D.getParentFunctionOrMethod()) + "." +
+D.getName(),
+InsertBefore, llvm::GlobalValue::NotThreadLocal, AS);
+GV->setAlignment(Align.getQuantity());
+GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+CacheEntry = GV;
+  } else if (CacheEntry->getAlignment() < Align.getQuantity()) {
+CacheEntry->setAlignment(Align.getQuantity());
+  }
+
+  return Address(CacheEntry, Align);
+}
+
+static Address createUnnamedGlobalForMemcpyFrom(CodeGenModule ,
+const VarDecl ,
+CGBuilderTy ,
+llvm::Constant *Constant,
+CharUnits Align) {
+  Address SrcPtr = CGM.createUnnamedGlobalFrom(D, Constant, Align);
+  llvm::Type *BP = llvm::PointerType::getInt8PtrTy(CGM.getLLVMContext(),
+   SrcPtr.getAddressSpace());
   if (SrcPtr.getType() != BP)
 SrcPtr = Builder.CreateBitCast(SrcPtr, BP);
   return SrcPtr;
@@ -1197,10 +1213,10 @@
   }
 
   // Copy from a global.
-  Builder.CreateMemCpy(
-  Loc,
-  createUnnamedGlobalFrom(CGM, D, Builder, constant, Loc.getAlignment()),
-  SizeVal, isVolatile);
+  Builder.CreateMemCpy(Loc,
+   createUnnamedGlobalForMemcpyFrom(
+   CGM, D, Builder, constant, 

[PATCH] D63164: [HIP] Add option to force lambda nameing following ODR in HIP/CUDA.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think this is the wrong way to handle this issue. We need to give lambdas a 
mangling if they occur in functions for which there can be definitions in 
multiple translation units. In regular C++ code, that's inline functions and 
function template specializations, so that's what we're currently checking for. 
CUDA adds more cases (in particular, `__host__ __device__` functions, plus 
anything else that can be emitted for multiple targets), so we should 
additionally check for those cases when determining whether to number lambdas. 
I don't see any need for a flag to control this behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63164



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

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62825#1542247 , @rjmccall wrote:

> In what sense is the bit-pattern of a null pointer indeterminate?


The problem is not null pointers, it's `nullptr_t`, which is required to have 
the same size and alignment as `void*` but which comprises only padding bits. 
(Loads of `nullptr_t` are not even permitted to touch memory...).


Repository:
  rC Clang

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

https://reviews.llvm.org/D62825



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

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExpr.cpp:1429
+/// for instance if a block or lambda or a member of a local class uses a
+/// const int variable or constexpr variable from an enclosing function.
 CodeGenFunction::ConstantEmission

rjmccall wrote:
> Isn't the old comment correct here?  This is mandatory because of the 
> enclosing-local-scope issues; that might be an "optimization" in the 
> language, but it's not an optimization at the IRGen level because Sema 
> literally is forcing us to do it. 
In the absence of this code, we'd emit the variable as a global constant from 
`EmitDeclRefLValue` in the enclosing-local-scope case (as we now do in the 
more-complex cases), so this should never be necessary for correct code 
generation any more.



Comment at: lib/Sema/SemaExpr.cpp:15808
+namespace {
+// Helper to copy the template arguments from a DeclRefExpr or MemberExpr.
+class CopiedTemplateArgs {

rjmccall wrote:
> This is really cute; I might steal this idea.  That said, it's probably worth 
> explaining the lifetime mechanics here so that non-experts can understand how 
> this works.
Done. I also added a `[[clang::lifetimebound]]` attribute so we'll get a 
warning if this is misused.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63157



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

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith updated this revision to Diff 204589.
rsmith marked 2 inline comments as done.
rsmith added a comment.

Updated comment, fixed typo, added use of `[[clang::lifetimebound]]`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63157

Files:
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenModule.h
  lib/Sema/SemaExpr.cpp
  test/CXX/basic/basic.def.odr/p2.cpp
  test/CXX/drs/dr20xx.cpp
  test/CXX/drs/dr21xx.cpp
  test/CXX/drs/dr23xx.cpp
  test/CXX/drs/dr6xx.cpp
  test/CXX/drs/dr7xx.cpp
  test/CodeGenCXX/no-odr-use.cpp
  www/cxx_dr_status.html

Index: www/cxx_dr_status.html
===
--- www/cxx_dr_status.html
+++ www/cxx_dr_status.html
@@ -4219,7 +4219,7 @@
 http://wg21.link/cwg696;>696
 C++11
 Use of block-scope constants in local classes
-Unknown
+Yes
   
   
 http://wg21.link/cwg697;>697
@@ -4315,7 +4315,7 @@
 http://wg21.link/cwg712;>712
 CD3
 Are integer constant operands of a conditional-expression used?
-Unknown
+Partial
   
   
 http://wg21.link/cwg713;>713
@@ -12313,7 +12313,7 @@
 http://wg21.link/cwg2083;>2083
 DR
 Incorrect cases of odr-use
-Unknown
+Partial
   
   
 http://wg21.link/cwg2084;>2084
@@ -12433,7 +12433,7 @@
 http://wg21.link/cwg2103;>2103
 DR
 Lvalue-to-rvalue conversion is irrelevant in odr-use of a reference
-Unknown
+Yes
   
   
 http://wg21.link/cwg2104;>2104
@@ -12835,7 +12835,7 @@
 http://wg21.link/cwg2170;>2170
 DR
 Unclear definition of odr-use for arrays
-Unknown
+SVN
   
   
 http://wg21.link/cwg2171;>2171
@@ -13933,7 +13933,7 @@
 http://wg21.link/cwg2353;>2353
 DR
 Potential results of a member access expression for a static data member
-Unknown
+SVN
   
   
 http://wg21.link/cwg2354;>2354
Index: test/CodeGenCXX/no-odr-use.cpp
===
--- /dev/null
+++ test/CodeGenCXX/no-odr-use.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple x86_64-linux-gnu %s | FileCheck %s
+
+// CHECK: @__const._Z1fi.a = private unnamed_addr constant {{.*}} { i32 1, [2 x i32] [i32 2, i32 3], [3 x i32] [i32 4, i32 5, i32 6] }
+
+struct A { int x, y[2]; int arr[3]; };
+// CHECK-LABEL: define i32 @_Z1fi(
+int f(int i) {
+  // CHECK: call void {{.*}}memcpy{{.*}}({{.*}}, {{.*}} @__const._Z1fi.a
+  constexpr A a = {1, 2, 3, 4, 5, 6};
+
+  // CHECK-LABEL: define {{.*}}@"_ZZ1fiENK3$_0clEiM1Ai"(
+  return [] (int n, int A::*p) {
+// CHECK: br i1
+return (n >= 0
+  // CHECK: getelementptr inbounds [3 x i32], [3 x i32]* getelementptr inbounds ({{.*}} @__const._Z1fi.a, i32 0, i32 2), i64 0, i64 %
+  ? a.arr[n]
+  // CHECK: br i1
+  : (n == -1
+// CHECK: getelementptr inbounds i8, i8* bitcast ({{.*}} @__const._Z1fi.a to i8*), i64 %
+// CHECK: bitcast i8* %{{.*}} to i32*
+// CHECK: load i32
+? a.*p
+// CHECK: getelementptr inbounds [2 x i32], [2 x i32]* getelementptr inbounds ({{.*}} @__const._Z1fi.a, i32 0, i32 1), i64 0, i64 %
+// CHECK: load i32
+: a.y[2 - n]));
+  }(i, ::x);
+}
Index: test/CXX/drs/dr7xx.cpp
===
--- test/CXX/drs/dr7xx.cpp
+++ test/CXX/drs/dr7xx.cpp
@@ -17,6 +17,42 @@
   }
 }
 
+namespace dr712 { // dr712: partial
+  void use(int);
+  void f() {
+const int a = 0; // expected-note 5{{here}}
+struct X {
+  void g(bool cond) {
+use(a);
+use((a));
+use(cond ? a : a);
+use((cond, a)); // expected-warning 2{{unused}} FIXME: should only warn once
+
+(void)a; // FIXME: expected-error {{declared in enclosing}}
+(void)(a); // FIXME: expected-error {{declared in enclosing}}
+(void)(cond ? a : a); // FIXME: expected-error 2{{declared in enclosing}}
+(void)(cond, a); // FIXME: expected-error {{declared in enclosing}} expected-warning {{unused}}
+  }
+};
+  }
+
+#if __cplusplus >= 201103L
+  void g() {
+struct A { int n; };
+constexpr A a = {0}; // expected-note 2{{here}}
+struct X {
+  void g(bool cond) {
+use(a.n);
+use(a.*::n);
+
+(void)a.n; // FIXME: expected-error {{declared in enclosing}}
+(void)(a.*::n); // FIXME: expected-error {{declared in enclosing}}
+  }
+};
+  }
+#endif
+}
+
 namespace dr727 { // dr727: partial
   struct A {
 template struct C; // expected-note 6{{here}}
Index: test/CXX/drs/dr6xx.cpp
===
--- test/CXX/drs/dr6xx.cpp
+++ test/CXX/drs/dr6xx.cpp
@@ -1132,3 +1132,20 @@
 template void f(int*); // expected-error {{ambiguous}}
   }
 }
+
+namespace dr696 { // dr696: yes
+  void f(const int*);
+  void g() {
+const int N = 10; // expected-note 1+{{here}}
+struct A {
+   

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62988#1540359 , @rjmccall wrote:

> For ARC, you could bzero the union member; this is what how we tell people to 
> initialize malloc'ed memory as well, since there's no default-constructor 
> concept that they can invoke for such cases.
>  Our immediate need for this attribute is that we have some code that wants 
> to adopt non-trivial annotations in unions, with no intention of ever copying 
> or destroying them as aggregates.


OK, so we'd be looking at something like this when using the attribute:

  typedef struct S {
int is_b;
union {
  float f;
  __attribute__((non_trivial_union_member)) id b;
};
  } S;
  S make() {
S s = {0, 0.0f};
return s;
  }
  void set_f(S *s, float f) {
if (s->is_b) s->b = 0;
s->is_b = 0;
s->f = f;
  }
  void set_b(S *s, id b) {
if (!s->is_b) bzero(s, sizeof(S));
s->is_b = 1;
s->b = b;
  }
  void destroy_s(S *s) {
if (s->is_b) s->b = 0;
  }

versus the same code written without the attribute, which might be:

  typedef struct S {
int is_b;
union {
  float f;
  void *b;
};
  } S;
  S make() {
S s = {0, 0.0f};
return s;
  }
  void set_f(S *s, float f) {
if (s->is_b) (__bridge_transfer id)s->b;
s->is_b = 0;
s->f = f;
  }
  void set_b(S *s, id b) {
if (s->is_b) (__bridge_transfer id)s->b;
s->is_b = 1;
s->b = (__bridge_retained void*)b;
  }
  void destroy_s(S *s) {
if (s->is_b) (__bridge_transfer id)s->b;
  }

To me, using the attribute here doesn't seem worthwhile; the first form of this 
code seems scary since important reference counting actions are hidden behind 
what appear to be dead stores, and the second form (while verbose) is at least 
explicit about what's going on. If you still think this is a useful feature, so 
be it, but we should at least be explicit in Clang's ARC documentation about 
how to correctly make use of it.

I'm not sure whether we should support this as a way of disabling the union 
triviality features in general, or only the restriction on lifetime types, but 
either way, I think the support of this attribute should not be dependent on 
the language mode; I think adding a C-only extension for this does a disservice 
to our users.

> Of course a better solution would be to make any C code that would copy or 
> destroy the aggregate type illegal, but that seems like a big project.  But 
> maybe there'd be no need for this attribute in the long term if we eventually 
> do have that support.

Yeah, maybe so; adopting the C++11 "unrestricted unions" behavior for 
non-trivial types in unions in C does seem to make a certain amount of sense.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988



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

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62988#1539230 , @ahatanak wrote:

> In D62988#1538577 , @rsmith wrote:
>
> > How do you write correct (non-leaking, non-double-freeing, 
> > non-releasing-invalid-pointers) code with this attribute? For example, 
> > suppose I have a `__strong` union member: does storing to it release the 
> > old value (which might be a different union member)? If so, how do you work 
> > around that? 
> > https://clang.llvm.org/docs/AutomaticReferenceCounting.html#ownership-qualified-fields-of-structs-and-unions
> >  should be updated to say what happens. If manual reference counting code 
> > is required to make any use of this feature correct (which seems 
> > superficially likely), is this a better programming model than 
> > `__unsafe_unretained`?
>
>
> I should first make it clear that this attribute is a dangerous feature that 
> makes it easier for users to write incorrect code if they aren't careful.
>
> To write correct code, users have to manually (I don't mean 
> manual-retain-release, this is compiled with ARC) do assignments to the 
> fields that are annotated with the attribute to patch up the code generated 
> by the compiler since the compiler isn't generating the kind of special 
> functions it generates for non-trivial structs. For example:
>
>   union U1 {
> id __weak __attribute__((non_trivial_union_member)) a;
> id __attribute__((non_trivial_union_member)) b;
>   };
>
>   id getObj(int);
>  
>   void foo() {
> union U1 u1 = { .b = 0}; // zero-initialize 'b'.
> u1.b = getObj(1); // assign to __strong field 'b'.
> u1.b = getObj(2); // retain and assign another object to 'b' and release 
> the previously referenced object.


This is what I expected and what I was worried about. I could be missing 
something, but this approach appears to me to cause this feature to not be 
useful in many cases. If a store to a union member now first reads from (and 
releases) that union member, then you cannot change the active union member to 
a member with lifetime type, because that would trigger a load of an inactive 
union member with undefined behavior. For example, consider:

  union U {
float f;
__attribute__((non_trivial_union_member)) id b;
  };
  void f() {
union U u = {.f = 1.0f};
u.b = getObj(1); // undefined behavior: tries to release existing value of 
u.b, which is not active union member; the implied `[u.b release]` is going to 
do Very Bad Things.

In C, storing to a union member is the way you change the active member, and I 
can't off-hand think of any sensible workaround for this problem. But maybe I 
overlooked it: can you demonstrate how you would correctly write the last line 
of the above example? If that can't be done reasonably, I do not think we 
should add this attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988



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

[PATCH] D63175: [MS] Pretend constexpr variable template specializations are inline

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/AST/ASTContext.cpp:9809
+  // variable template specializations inline.
+  if (isa(VD) && VD->isConstexpr())
+return GVA_DiscardableODR;

It'd be nice to include a note here that this is strictly non-conforming (since 
another TU could be relying on this TU to provide the definition), but that we 
don't expect that to happen in practice for variable template specializations 
declared `constexpr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63175



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

[PATCH] D63161: Devirtualize destructor of final class.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprCXX.cpp:1867-1868
 
-  if (Dtor->isVirtual()) {
+  if (Dtor->isVirtual() &&
+  !(Dtor->hasAttr() || RD->hasAttr())) {
 CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,

Can we use `getDevirtualizedMethod` here instead? That catches a few other 
cases that just checking for `final` will miss, and disables this for cases 
where it will currently do the wrong thing (eg, `-fapple-kext`). See 
`EmitCXXMemberOrOperatorMemberCallExpr` for where we do this for other virtual 
calls.

As a particularly terrible example:

```
struct A { virtual ~A() final = 0; };
void evil(A *p) { delete p; }
```

is (amazingly) valid, and must not be devirtualized because there is no 
requirement that the destructor of `A` have a definition in this program. (It 
would, however, be correct to optimize out the entire `delete` expression in 
this case, on the basis that `p` must always be `nullptr` whenever it's 
reached. But that doesn't really seem worthwhile.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63161



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

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith created this revision.
rsmith added a reviewer: rjmccall.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

When a variable is named in a context where we can't directly emit a
reference to it (because we don't know for sure that it's going to be
defined, or it's from an enclosing function and not captured, or the
reference might not "work" for some reason), we emit a copy of the
variable as a global and use that for the known-to-be-read-only access.


Repository:
  rC Clang

https://reviews.llvm.org/D63157

Files:
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenModule.h
  lib/Sema/SemaExpr.cpp
  test/CXX/basic/basic.def.odr/p2.cpp
  test/CXX/drs/dr20xx.cpp
  test/CXX/drs/dr21xx.cpp
  test/CXX/drs/dr23xx.cpp
  test/CXX/drs/dr6xx.cpp
  test/CXX/drs/dr7xx.cpp
  test/CodeGenCXX/no-odr-use.cpp
  www/cxx_dr_status.html

Index: www/cxx_dr_status.html
===
--- www/cxx_dr_status.html
+++ www/cxx_dr_status.html
@@ -4219,7 +4219,7 @@
 http://wg21.link/cwg696;>696
 C++11
 Use of block-scope constants in local classes
-Unknown
+Yes
   
   
 http://wg21.link/cwg697;>697
@@ -4315,7 +4315,7 @@
 http://wg21.link/cwg712;>712
 CD3
 Are integer constant operands of a conditional-expression used?
-Unknown
+Partial
   
   
 http://wg21.link/cwg713;>713
@@ -12313,7 +12313,7 @@
 http://wg21.link/cwg2083;>2083
 DR
 Incorrect cases of odr-use
-Unknown
+Partial
   
   
 http://wg21.link/cwg2084;>2084
@@ -12433,7 +12433,7 @@
 http://wg21.link/cwg2103;>2103
 DR
 Lvalue-to-rvalue conversion is irrelevant in odr-use of a reference
-Unknown
+Yes
   
   
 http://wg21.link/cwg2104;>2104
@@ -12835,7 +12835,7 @@
 http://wg21.link/cwg2170;>2170
 DR
 Unclear definition of odr-use for arrays
-Unknown
+SVN
   
   
 http://wg21.link/cwg2171;>2171
@@ -13933,7 +13933,7 @@
 http://wg21.link/cwg2353;>2353
 DR
 Potential results of a member access expression for a static data member
-Unknown
+SVN
   
   
 http://wg21.link/cwg2354;>2354
Index: test/CodeGenCXX/no-odr-use.cpp
===
--- /dev/null
+++ test/CodeGenCXX/no-odr-use.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple x86_64-linux-gnu %s | FileCheck %s
+
+// CHECK: @__const._Z1fi.a = private unnamed_addr constant {{.*}} { i32 1, [2 x i32] [i32 2, i32 3], [3 x i32] [i32 4, i32 5, i32 6] }
+
+struct A { int x, y[2]; int arr[3]; };
+// CHECK-LABEL: define i32 @_Z1fi(
+int f(int i) {
+  // CHECK: call void {{.*}}memcpy{{.*}}({{.*}}, {{.*}} @__const._Z1fi.a
+  constexpr A a = {1, 2, 3, 4, 5, 6};
+
+  // CHECK-LABEL: define {{.*}}@"_ZZ1fiENK3$_0clEiM1Ai"(
+  return [] (int n, int A::*p) {
+// CHECK: br i1
+return (n >= 0
+  // CHECK: getelementptr inbounds [3 x i32], [3 x i32]* getelementptr inbounds ({{.*}} @__const._Z1fi.a, i32 0, i32 2), i64 0, i64 %
+  ? a.arr[n]
+  // CHECK: br i1
+  : (n == -1
+// CHECK: getelementptr inbounds i8, i8* bitcast ({{.*}} @__const._Z1fi.a to i8*), i64 %
+// CHECK: bitcast i8* %{{.*}} to i32*
+// CHECK: load i32
+? a.*p
+// CHECK: getelementptr inbounds [2 x i32], [2 x i32]* getelementptr inbounds ({{.*}} @__const._Z1fi.a, i32 0, i32 1), i64 0, i64 %
+// CHECK: load i32
+: a.y[2 - n]));
+  }(i, ::x);
+}
Index: test/CXX/drs/dr7xx.cpp
===
--- test/CXX/drs/dr7xx.cpp
+++ test/CXX/drs/dr7xx.cpp
@@ -17,6 +17,42 @@
   }
 }
 
+namespace dr712 { // dr712: partial
+  void use(int);
+  void f() {
+const int a = 0; // expected-note 5{{here}}
+struct X {
+  void g(bool cond) {
+use(a);
+use((a));
+use(cond ? a : a);
+use((cond, a)); // expected-warning 2{{unused}} FIXME: should only warn once
+
+(void)a; // FIXME: expected-error {{declared in enclosing}}
+(void)(a); // FIXME: expected-error {{declared in enclosing}}
+(void)(cond ? a : a); // FIXME: expected-error 2{{declared in enclosing}}
+(void)(cond, a); // FIXME: expected-error {{declared in enclosing}} expected-warning {{unused}}
+  }
+};
+  }
+
+#if __cplusplus >= 201103L
+  void g() {
+struct A { int n; };
+constexpr A a = {0}; // expected-note 2{{here}}
+struct X {
+  void g(bool cond) {
+use(a.n);
+use(a.*::n);
+
+(void)a.n; // FIXME: expected-error {{declared in enclosing}}
+(void)(a.*::n); // FIXME: expected-error {{declared in enclosing}}
+  }
+};
+  }
+#endif
+}
+
 namespace dr727 { // dr727: partial
   struct A {
 template struct C; // expected-note 6{{here}}
Index: test/CXX/drs/dr6xx.cpp
===
--- test/CXX/drs/dr6xx.cpp
+++ 

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

How do you write correct (non-leaking, non-double-freeing, 
non-releasing-invalid-pointers) code with this attribute? For example, suppose 
I have a `__strong` union member: does storing to it release the old value 
(which might be a different union member)? If so, how do you work around that? 
https://clang.llvm.org/docs/AutomaticReferenceCounting.html#ownership-qualified-fields-of-structs-and-unions
 should be updated to say what happens. If manual reference counting code is 
required to make any use of this feature correct (which seems superficially 
likely), is this a better programming model than `__unsafe_unretained`?

Unions with non-trivial members are only permitted in C++11 onwards; should we 
allow the attribute in C++98 mode? But more than that, unions with non-trivial 
members require user-provided special members in C++11 onwards, meaning that a 
union using this attribute in C would have a different calling convention than 
the "natural" equivalent in C++. So I'm also wondering if we should allow this 
in all language modes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988



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

[PATCH] D63072: [clang] Fixing incorrect implicit deduction guides (PR41549)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Thanks!


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

https://reviews.llvm.org/D63072



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

[PATCH] D50360: [Concepts] Requires Expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/DeclCXX.h:2051
+/// \code
+/// template requires requires (T t) { {t++} -> Regular };
+/// \endcode

The semicolon here is in the wrong place (should be before the final `}`, not 
after).



Comment at: include/clang/AST/DeclTemplate.h:1185
   bool wereArgumentsSpecified() const {
-return ArgsAsWritten == nullptr;
+return ArgsAsWritten != nullptr;
   }

Please move this bugfix earlier in the patch series.



Comment at: include/clang/AST/ExprCXX.h:4634
+  llvm::SmallVector Requirements;
+  SourceLocation RBraceLoc;
+

Reorder this next to `RequiresKWLoc`. `SourceLocation`s are 4 bytes, whereas 
for most platforms we care about, pointers are size-8 and align-8, so the 
reordering will save 8 bytes of padding.



Comment at: include/clang/AST/ExprCXX.h:4645
+
+  void setLocalParameters(ArrayRef LocalParameters);
+  ArrayRef getLocalParameters() const { return LocalParameters; 
}

Please avoid adding setters to `Expr` subclasses if possible. We want the AST 
to be immutable. (Befriend `ASTReaderStmt` instead.)



Comment at: include/clang/AST/ExprCXX.h:4674
+  llvm::SmallVector LocalParameters;
+  llvm::SmallVector Requirements;
+  SourceLocation RBraceLoc;

riccibruno wrote:
> Can you tail-allocate them ?
Using a `SmallVector` here is a bug; the destructor is not run on `Expr` nodes 
so this will leak memory. Please do change to using tail allocation here.



Comment at: include/clang/Basic/DiagnosticParseKinds.td:752
+def err_requires_expr_simple_requirement_unexpected_tok : Error<
+  "unexpected %0 after expression. Did you intend to use a compound "
+  "requirement? (with '{' '}' around the expression)">;

`. Did` -> `; did` so this fits the general diagnostic pattern regardless of 
whether the diagnostic renderer capitalizes the diagnostic.



Comment at: include/clang/Basic/DiagnosticParseKinds.td:753
+  "unexpected %0 after expression. Did you intend to use a compound "
+  "requirement? (with '{' '}' around the expression)">;
 

Move `?` to the end.



Comment at: lib/AST/ExprCXX.cpp:32-35
 #include "clang/Sema/Template.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "clang/Sema/Sema.h"
+#include "clang/Sema/Overload.h"

These `#include`s are all unacceptable. `AST` is layered below `Sema`, and 
cannot depend on `Sema` headers and classes.



Comment at: lib/AST/ExprConstant.cpp:9953-9955
+  if (E->isValueDependent()) {
+return Error(E);
+  }

It is a logic error for the expression evaluator to encounter a value-dependent 
expression, and we assert on the way into the evaluator if we would encounter 
one. You don't need to check this here.



Comment at: lib/AST/StmtProfile.cpp:1325
+  //expression. It is equivalent to the simple-requirement x++; [...]
+  // We therefore do not profile isSimple() here.
+  ID.AddBoolean(ExprReq->getNoexceptLoc().isValid());

We don't /need to/ profile `isSimple`, but we still could. (This "is equivalent 
to" doesn't override the general ODR requirement that you spell the expression 
with the same token sequence.)

Do we mangle simple and compound requirements the same way? (Has a mangling for 
requires-expressions even been proposed on the Itanium ABI list yet?)



Comment at: lib/Parse/ParseExprCXX.cpp:3097
+Braces.consumeClose();
+return ExprError();
+  }

Recovering by producing an (always-satisfied) `RequiresExpr` with no 
requirements would seem reasonable here.



Comment at: lib/Parse/ParseExprCXX.cpp:3113
+switch (Tok.getKind()) {
+case tok::kw_typename: {
+  // Type requirement

This is incorrect. A //simple-requirement// can begin with the `typename` 
keyword. (Eg, `requires { typename T::type() + 1; }`)

The right way to handle this is to call `TryAnnotateTypeOrScopeToken()` when 
you see `tok::kw_typename`, and then detect a `type-requirement` as a 
`tok::annot_typename` followed by a `tok::semi`. (By following that approach, 
you can also handle cases where the `typename` is missing.) You'll need to deal 
specially with the case where the //nested-name-specifier// is omitted, since 
in that case the `typename` keyword does not form part of a 
//typename-specifier//; in that case, after `TryAnnotateTypeOrScopeToken()` 
you'll have a `tok::kw_typename`, `tok::identifier`, `tok::semi` sequence or a 
`tok::kw_typename`, `tok::annot_template_id`, `tok::semi` sequence to detect 
and special-case.



Comment at: lib/Parse/ParseExprCXX.cpp:3125
+  SS.isInvalid()) {
+Braces.skipToEnd();
+Actions.ActOnExitRequiresExpr();

[PATCH] D63072: [clang] Fixing incorrect implicit deduction guides (PR41549)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:503
+
+umm<> m(1);
+

Should this be using CTAD?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63072



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