[PATCH] D88712: [CGBuiltin] Respect asm labels and redefine_extname for builtins with specialized emitting

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

rsmith added a comment.

I worry that we're chasing after the implementation details of GCC here. It 
seems self-contradictory to say all three of these things at once:

1. The frontend function `foo` has known, builtin semantics X. (Eg, we'll use 
those semantics in the frontend.)
2. The symbol `foo` has known, builtin semantics X. (Eg, we'll synthesize calls 
to `foo` from the middle-end.)
3. It's not correct to lower a call to the frontend function `foo` to the 
symbol `foo`.

So, from a principled standpoint, which of the above do we not consider to be 
correct in this situation?

- If we don't consider (1) to be correct, then we need to stop treating `foo` 
as a builtin everywhere -- we shouldn't be constant-evaluating calls to it, in 
particular. That's problematic in principle, because the `asm` label might be 
added after we've already seen the first declaration and added a `BuiltinAttr` 
to it. If we want to go in this direction, I think we should require a build 
that attaches an `asm` label to a builtin to also pass `-fno-builtin-foo` 
rather than trying to un-builtin a builtin function after the fact.
- If we don't consider (2) to be correct, then we need to stop the middle-end 
from inserting calls to the symbol `foo`, and get it to use the new symbol 
instead. That'd be the "teach the target libcall info about this" approach, 
which (as you point out) would be quite complex.
- The status quo is that we don't consider (3) to be correct. If we take this 
path, I suppose we could say that we make a best effort to use the renamed 
symbol, but provide no guarantees. That seems unprincipled and will likely 
continue to cause problems down the line, but maybe this gets us closest to the 
observed GCC behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88712

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

[PATCH] D87565: [Sema] Improve const_cast conformance to N4261

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaCast.cpp:1817-1819
+  NeedToMaterializeTemporary =
+  SrcType->isRecordType() || SrcType->isArrayType() ||
+  SrcType->isFunctionPointerType() || SrcType->isMemberPointerType();

It looks like GCC allowing a cast from `T*` to `T*&&` is just a bug in their 
implementation. Consider:

```
using T = int*;
T & = const_cast(T{}); // GCC accepts
T & = const_cast(T()); // GCC rejects
```

... and the same behavior seems to show up for all scalar types: they permit 
`const_cast` from `T{}` but not from `T()` when `T` is `int`, `int*`,  This 
doesn't seem like good behavior to follow. I think we should either implement 
the current direction of 1965 (that is, only allow classes and arrays here) or 
leave the current behavior (following the standard as-is) alone.


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

https://reviews.llvm.org/D87565

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

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

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

rsmith added a comment.

Reverse ping: I have a patch implementing class type non-type template 
parameters that's blocked on this landing. If you won't have time to address 
@martong's comments soon, do you mind if I take this over and land it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63640

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

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

rsmith updated this revision to Diff 298268.
rsmith marked 3 inline comments as done.
rsmith added a comment.

- Addressing review feedback from @aaron.ballman.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89212

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-weak.c
  clang/test/Sema/init.c

Index: clang/test/Sema/init.c
===
--- clang/test/Sema/init.c
+++ clang/test/Sema/init.c
@@ -155,7 +155,7 @@
 int PR4386_a = ((void *) PR4386_bar) != 0;
 int PR4386_b = ((void *) PR4386_foo) != 0; // expected-error{{initializer element is not a compile-time constant}}
 int PR4386_c = ((void *) PR4386_zed) != 0;
-int PR4386_zed() __attribute((weak));
+int PR4386_zed() __attribute((weak)); // expected-warning{{'PR4386_zed' declared weak after its first use}} expected-note {{attribute}}
 
 //  (derived from SPEC vortex benchmark)
 typedef char strty[10];
Index: clang/test/Sema/attr-weak.c
===
--- clang/test/Sema/attr-weak.c
+++ clang/test/Sema/attr-weak.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-unknown-macosx10.3.0 -DMACOS
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-linux-gnu -DLINUX
 
 extern int f0() __attribute__((weak));
 extern int g0 __attribute__((weak));
@@ -25,3 +26,55 @@
 
 static void pr14946_f();
 void pr14946_f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
+
+void some_function();
+
+void pr47663_a();
+void pr47663_b();
+static void pr47663_c();
+void pr47663_d();
+void pr47663_e(); // expected-warning {{declared weak after its first use}}
+void pr47663_f(); // expected-note {{possible target}}
+void pr47663_g();
+int pr47663_h;
+void pr47663_z() __attribute__((weak));
+void use() {
+  int arr_a[_a ? 1 : -1];
+  int arr_b[_b ? 1 : -1];
+  int arr_c[_c ? 1 : -1];
+  int arr_d[_d ? 1 : -1];
+  int arr_e[_e ? 1 : -1];
+  int arr_f[_f ? 1 : -1];
+  int arr_g[_g ? 1 : -1];
+  int arr_h[_h ? 1 : -1]; // expected-warning {{will always evaluate to 'true'}}
+  int arr_z[_z ? -1 : 1];
+}
+void pr47663_a() __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_b() __attribute__((weak_import)); // expected-warning {{declared weak after its first use}} expected-note {{'weak_import' attribute here}}
+#ifdef LINUX
+static void pr47663_c() __attribute__((weakref, alias("might_not_exist"))); // expected-warning {{declared weak after its first use}} expected-note {{'weakref' attribute here}}
+#endif
+#ifdef MACOS
+void pr47663_d() __attribute__((availability(macos, introduced=10.4))); // expected-warning {{declared weak after its first use}} expected-note {{'availability' attribute here}}
+#endif
+#pragma weak pr47663_e // expected-note {{pragma 'weak' here}}
+
+// FIXME: This should warn; see PR47796. But it actually creates a bogus
+// overload set. When this is fixed, ensure we produce the 'declared weak after
+// its first use' warning.
+#pragma weak pr47663_f = some_function // expected-note {{possible target}}
+void q() { pr47663_f; } // expected-error {{overloaded}}
+
+#pragma clang attribute push (__attribute__((weak)), apply_to = function) // expected-note {{'weak' attribute here}}
+void pr47663_g(); // expected-warning {{declared weak after its first use}}
+#pragma clang attribute pop
+extern int pr47663_h __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_z() __attribute__((weak_import));
+
+// 'weak' on a definition means something different from 'weak' on a
+// declaration. Don't warn in that case.
+void weak_def_after_use();
+extern int weak_def_after_use_v;
+void use_wdau() { weak_def_after_use(); weak_def_after_use_v = 0; }
+__attribute__((weak)) void weak_def_after_use() {}
+__attribute__((weak)) int weak_def_after_use_v;
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8154,6 +8154,9 @@
   W.setUsed(true);
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
 IdentifierInfo *NDId = ND->getIdentifier();
+// FIXME (PR47796): Check for a previous declaration with the target name
+// here, and build a redeclaration of it. Check whether the previous
+// declaration is used and warn if so.
 NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
 NewD->addAttr(
 AliasAttr::CreateImplicit(Context, NDId->getName(), W.getLocation()));

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

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



Comment at: clang/lib/Sema/SemaDecl.cpp:6435-6436
+Attr *WeakA = nullptr;
+for (Attr *A : VD->getAttrs()) {
+  if (!isa(A))
+continue;

aaron.ballman wrote:
> Ah, it's too bad that `Decl::specific_attrs()` doesn't accept a pack of 
> attributes...
I took a brief look at fixing that, but it's not a completely natural extension 
because we would want to use a different `value_type` for the iterator if 
there's more than one kind under consideration. Probably not worth it for only 
one user.



Comment at: clang/lib/Sema/SemaDecl.cpp:18288
 
-  if (PrevDecl) {
-PrevDecl->addAttr(WeakAttr::CreateImplicit(Context, PragmaLoc, 
AttributeCommonInfo::AS_Pragma));
+  if (NamedDecl *PrevDecl =
+  LookupSingleName(TUScope, Name, NameLoc, LookupOrdinaryName)) {

aaron.ballman wrote:
> Same request for `const` here as above.
Can't make this `const`; this function modifies the declaration by adding an 
attribute :) But I can make `VD` below const.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89212

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

[PATCH] D88498: [FPEnv] Evaluate initializers in constant rounding mode

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

In D88498#2329845 , @sepavloff wrote:

> - Reverted check to the previous version, in which it applied to C++ file 
> level variables also.

This presumably reintroduces the misbehavior for

  double d;
  double e = (fesetround(...), d = some calculation, fesetround(...default...), 
d);

in which `some calculation` will be treated as being in the default rounding 
mode, right?

> - Added workaround for constexpr functions. Now they are parsed with constant 
> rounding mode, which allows to use them with option `-frounding-math`.

This is inappropriate. When a `constexpr` function is invoked at runtime, it 
should behave exactly like any other function. Marking a function as 
`constexpr` should not cause it to round differently when used outside of 
constant expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88498

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

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

rsmith added a comment.

In D89212#2326771 , @rjmccall wrote:

> Patch looks good to me.  I think we're running some internal tests; do you 
> want to wait for those?

More testing and validation would be nice; I don't think this patch is urgent 
so I'm happy to wait.

In my internal testing, I found one more false positive: the protocol buffer 
compiler's support for weak imports results in code like this:

  // generated .h file
  extern T foo;
  inline T *get() { return  }



  // generated .cc file
  __attribute__((weak)) extern T foo;
  // more uses of foo

This is intentional: the idea is that if someone includes the `.h` file and 
invokes the inline function, they get a strong link-time dependency on the 
symbol `foo` and need to link against that proto library. But if not, then 
there is no such dependency, and the library is optional. However, this 
functionality of the protocol buffer compiler is deprecated and being phased 
out (and suppressing the warning in this one case should be straightforward) so 
I'm not worried about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89212

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

[PATCH] D89360: Treat constant contexts as being in the default rounding mode.

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

rsmith added inline comments.



Comment at: clang/test/SemaCXX/rounding-math.cpp:9
+
+constexpr int f(int n) { return int(n * (1.0 / 3.0)); }
+

rsmith wrote:
> sepavloff wrote:
> > This code requires additional solution. The function body is built using 
> > dynamic rounding mode, which breaks its constexprness. We can avoid this 
> > kind of errors if we assume that body of a constexpr function is parsed 
> > using constant rounding mode (in this example it is the default mode). It 
> > makes parsing constexpr function different from non-constexpr ones, but 
> > enables convenient use:
> > ```
> > constexpr int add(float x, float y) { return x + y; }
> > 
> > #pragma STDC FENV_ROUND FE_UPWARD
> > int a2 = add(2.0F, 0x1.02p0F);
> > 
> > #pragma STDC FENV_ROUND FE_DOWNWARD
> > int a3 = add(2.0F, 0x1.02p0F);
> > ```
> > If calls of constexpr functions are processes with FP options acting in the 
> > call site, a call to constexpr function becomes equivalent to execution of 
> > statements of its body.
> I don't understand what you mean by "breaks its constexprness". Using a 
> dynamic rounding mode for the body of the function is exactly what we want 
> here. When the function is called in a manifestly constant evaluated context, 
> we should use the default rounding mode, and when it's called at runtime, we 
> use the appropriate dynamic rounding mode; if we try to constant evaluate it 
> outside of a manifestly constant evaluated constant, we deem it non-constant.
> 
> When `constexpr` function is called at runtime, it's a completely normal 
> function with normal semantics. It would be wrong to use the default rounding 
> mode when parsing its body.
To be clear: in your example with `add`, both `a2` and `a3` should be 
initialized to the same value, which should be rounded with the default 
rounding mode. Per ISO18661, `FENV_ROUND` only affects operations in its 
lexical scope, not functions called within those operations, and per the 
regular C++ rules, `constexpr` functions behave like regular functions, not 
like macros.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89360

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

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

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2165
+assert(getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0 &&
+   "Expected `this` pointer without address space attribute.");
+

jdoerfert wrote:
> I'm unsure why this assertion has to hold and more importantly why we need 
> it. @arsenm do you?
Even if the `this` pointer is always an address-space-0 pointer for now, it 
would be more consistent with how we handle `Attribute::NonNull` elsewhere to 
skip adding the `NonNull` attribute for non-address-space-0 pointers rather 
than asserting here.



Comment at: clang/lib/CodeGen/CGCall.cpp:2174
+  }
+
   unsigned ArgNo = 0;

jdoerfert wrote:
> rsmith wrote:
> > CJ-Johnson wrote:
> > > jdoerfert wrote:
> > > > Even if null pointer is valid we should place dereferenceable.
> > > > 
> > > > We also could never place nonnull and let the middle-end make the 
> > > > dereferenceable -> nonnull deduction, though I don't see why we can't 
> > > > just add nonnull here.
> > > I re-ran ninja check after making this fix and addressed the 
> > > newly-affected tests. So the patch is fully up to date :)
> > The LLVM LangRef says that in address space 0, `dereferenceable` implies 
> > `nonnull`, so I don't think we can emit `dereferenceable` in 
> > `NullPointerIsValid` mode, and we'd need to use `dereferenceable_or_null` 
> > instead. (Perhaps the LangRef is wrong, though, and the 
> > `null_pointer_is_valid` function attribute overrides that determination.)
> > (Perhaps the LangRef is wrong, though, and the null_pointer_is_valid 
> > function attribute overrides that determination.)
> 
> This is the case. IMHO, dereferenceable has to be correct here, regardless of 
> the mode. You could access the object in the function entry, which we would 
> use to deduce dereferenceable, and if the `NullPointerIsValid` is not 
> translated to the function attribute also to `nonnull`. 
OK, if the LangRef is wrong about this, then I agree we should emit 
`dereferenceable` unconditionally. Thanks for clarifying.


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

https://reviews.llvm.org/D17993

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

[PATCH] D89360: Treat constant contexts as being in the default rounding mode.

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

rsmith added a comment.

In D89360#2329856 , @sepavloff wrote:

> I would propose to consider solution in D88498 
> . It tries to fix the real reason of the 
> malfunction - using dynamic rounding mode for evaluation of global variable 
> initializers.

That is not the real reason for the malfunction. If you narrowly look at C, you 
can convince yourself otherwise, but that's only because global variable 
initializers are the only place where we need to evaluate floating-point 
constant expressions in C. In C++, this problem affects all contexts where 
constant evaluation might happen (array bounds, bit-field widths, and a whole 
host of others).

D88498 's approach also can't correctly handle 
`constexpr` functions, for which we want different behavior depending on 
whether the *caller* is a manifestly constant evaluated context.




Comment at: clang/test/SemaCXX/rounding-math.cpp:9
+
+constexpr int f(int n) { return int(n * (1.0 / 3.0)); }
+

sepavloff wrote:
> This code requires additional solution. The function body is built using 
> dynamic rounding mode, which breaks its constexprness. We can avoid this kind 
> of errors if we assume that body of a constexpr function is parsed using 
> constant rounding mode (in this example it is the default mode). It makes 
> parsing constexpr function different from non-constexpr ones, but enables 
> convenient use:
> ```
> constexpr int add(float x, float y) { return x + y; }
> 
> #pragma STDC FENV_ROUND FE_UPWARD
> int a2 = add(2.0F, 0x1.02p0F);
> 
> #pragma STDC FENV_ROUND FE_DOWNWARD
> int a3 = add(2.0F, 0x1.02p0F);
> ```
> If calls of constexpr functions are processes with FP options acting in the 
> call site, a call to constexpr function becomes equivalent to execution of 
> statements of its body.
I don't understand what you mean by "breaks its constexprness". Using a dynamic 
rounding mode for the body of the function is exactly what we want here. When 
the function is called in a manifestly constant evaluated context, we should 
use the default rounding mode, and when it's called at runtime, we use the 
appropriate dynamic rounding mode; if we try to constant evaluate it outside of 
a manifestly constant evaluated constant, we deem it non-constant.

When `constexpr` function is called at runtime, it's a completely normal 
function with normal semantics. It would be wrong to use the default rounding 
mode when parsing its body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89360

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

rsmith added a comment.

FYI: I posted a patch to implement the "manifestly constant evaluated" rule as 
https://reviews.llvm.org/D89360. It looks like the stricter rule doesn't work 
in practice; far too much C++ code uses `-frounding-math` and expects to be 
able to still include things like standard library headers that define 
`constexpr` functions and variables that involve floating-point rounding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528

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

[PATCH] D89360: Treat constant contexts as being in the default rounding mode.

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

rsmith created this revision.
rsmith added reviewers: rjmccall, mibintc, sepavloff.
Herald added a project: clang.
rsmith requested review of this revision.

This addresses a regression where pretty much all C++ compilations using
-frounding-math now fail, due to rounding being performed in constexpr
function definitions in the standard library.

This follows the "manifestly constant evaluated" approach described in
https://reviews.llvm.org/D87528#2270676 -- evaluations that are required
to succeed at compile time are permitted even in regions with dynamic
rounding modes, as are (unfortunately) the evaluation of the
initializers of local variables of const integral types.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89360

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/rounding-math.cpp


Index: clang/test/SemaCXX/rounding-math.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/rounding-math.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -triple x86_64-linux -verify=norounding %s
+// RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math
+// rounding-no-diagnostics
+
+#define fold(x) (__builtin_constant_p(x) ? (x) : (x))
+
+constexpr double a = 1.0 / 3.0;
+
+constexpr int f(int n) { return int(n * (1.0 / 3.0)); }
+
+using T = int[f(3)];
+using T = int[1];
+
+struct Bitfield { unsigned int n : 1; };
+
+void f(Bitfield ) {
+  b.n = int(6 * (1.0 / 3.0)); // norounding-warning {{changes value from 2 to 
0}}
+}
+
+const int k = 3 * (1.0 / 3.0);
+static_assert(k == 1, "");
+
+void g() {
+  // FIXME: Constant-evaluating this initializer is surprising, and violates
+  // the recommended practice in C++ [expr.const]p12:
+  //
+  //   Implementations should provide consistent results of floating-point
+  //   evaluations, irrespective of whether the evaluation is performed during
+  //   translation or during program execution.
+  const int k = 3 * (1.0 / 3.0);
+  static_assert(k == 1, "");
+}
+
+int *h() {
+  return new int[int(-3 * (1.0 / 3.0))]; // norounding-error {{too large}}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -2528,6 +2528,11 @@
 /// Check if the given evaluation result is allowed for constant evaluation.
 static bool checkFloatingPointResult(EvalInfo , const Expr *E,
  APFloat::opStatus St) {
+  // In a constant context, assume that any dynamic rounding mode or FP
+  // exception state matches the default floating-point environment.
+  if (Info.InConstantContext)
+return true;
+
   FPOptions FPO = E->getFPFeaturesInEffect(Info.Ctx.getLangOpts());
   if ((St & APFloat::opInexact) &&
   FPO.getRoundingMode() == llvm::RoundingMode::Dynamic) {


Index: clang/test/SemaCXX/rounding-math.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/rounding-math.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -triple x86_64-linux -verify=norounding %s
+// RUN: %clang_cc1 -triple x86_64-linux -verify=rounding %s -frounding-math
+// rounding-no-diagnostics
+
+#define fold(x) (__builtin_constant_p(x) ? (x) : (x))
+
+constexpr double a = 1.0 / 3.0;
+
+constexpr int f(int n) { return int(n * (1.0 / 3.0)); }
+
+using T = int[f(3)];
+using T = int[1];
+
+struct Bitfield { unsigned int n : 1; };
+
+void f(Bitfield ) {
+  b.n = int(6 * (1.0 / 3.0)); // norounding-warning {{changes value from 2 to 0}}
+}
+
+const int k = 3 * (1.0 / 3.0);
+static_assert(k == 1, "");
+
+void g() {
+  // FIXME: Constant-evaluating this initializer is surprising, and violates
+  // the recommended practice in C++ [expr.const]p12:
+  //
+  //   Implementations should provide consistent results of floating-point
+  //   evaluations, irrespective of whether the evaluation is performed during
+  //   translation or during program execution.
+  const int k = 3 * (1.0 / 3.0);
+  static_assert(k == 1, "");
+}
+
+int *h() {
+  return new int[int(-3 * (1.0 / 3.0))]; // norounding-error {{too large}}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -2528,6 +2528,11 @@
 /// Check if the given evaluation result is allowed for constant evaluation.
 static bool checkFloatingPointResult(EvalInfo , const Expr *E,
  APFloat::opStatus St) {
+  // In a constant context, assume that any dynamic rounding mode or FP
+  // exception state matches the default floating-point environment.
+  if (Info.InConstantContext)
+return true;
+
   FPOptions FPO = E->getFPFeaturesInEffect(Info.Ctx.getLangOpts());
   if ((St & APFloat::opInexact) &&
   FPO.getRoundingMode() == llvm::RoundingMode::Dynamic) {
___
cfe-commits mailing 

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

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

- Don't warn if we see a weak definition for a declaration that's already


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89212

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-weak.c
  clang/test/Sema/init.c

Index: clang/test/Sema/init.c
===
--- clang/test/Sema/init.c
+++ clang/test/Sema/init.c
@@ -155,7 +155,7 @@
 int PR4386_a = ((void *) PR4386_bar) != 0;
 int PR4386_b = ((void *) PR4386_foo) != 0; // expected-error{{initializer element is not a compile-time constant}}
 int PR4386_c = ((void *) PR4386_zed) != 0;
-int PR4386_zed() __attribute((weak));
+int PR4386_zed() __attribute((weak)); // expected-warning{{'PR4386_zed' declared weak after its first use}} expected-note {{attribute}}
 
 //  (derived from SPEC vortex benchmark)
 typedef char strty[10];
Index: clang/test/Sema/attr-weak.c
===
--- clang/test/Sema/attr-weak.c
+++ clang/test/Sema/attr-weak.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-unknown-macosx10.3.0 -DMACOS
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-linux-gnu -DLINUX
 
 extern int f0() __attribute__((weak));
 extern int g0 __attribute__((weak));
@@ -25,3 +26,55 @@
 
 static void pr14946_f();
 void pr14946_f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
+
+void some_function();
+
+void pr47663_a();
+void pr47663_b();
+static void pr47663_c();
+void pr47663_d();
+void pr47663_e(); // expected-warning {{declared weak after its first use}}
+void pr47663_f(); // expected-note {{possible target}}
+void pr47663_g();
+int pr47663_h;
+void pr47663_z() __attribute__((weak));
+void use() {
+  int arr_a[_a ? 1 : -1];
+  int arr_b[_b ? 1 : -1];
+  int arr_c[_c ? 1 : -1];
+  int arr_d[_d ? 1 : -1];
+  int arr_e[_e ? 1 : -1];
+  int arr_f[_f ? 1 : -1];
+  int arr_g[_g ? 1 : -1];
+  int arr_h[_h ? 1 : -1]; // expected-warning {{will always evaluate to 'true'}}
+  int arr_z[_z ? -1 : 1];
+}
+void pr47663_a() __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_b() __attribute__((weak_import)); // expected-warning {{declared weak after its first use}} expected-note {{'weak_import' attribute here}}
+#ifdef LINUX
+static void pr47663_c() __attribute__((weakref, alias("might_not_exist"))); // expected-warning {{declared weak after its first use}} expected-note {{'weakref' attribute here}}
+#endif
+#ifdef MACOS
+void pr47663_d() __attribute__((availability(macos, introduced=10.4))); // expected-warning {{declared weak after its first use}} expected-note {{'availability' attribute here}}
+#endif
+#pragma weak pr47663_e // expected-note {{pragma 'weak' here}}
+
+// FIXME: This should warn; see PR47796. But it actually creates a bogus
+// overload set. When this is fixed, ensure we produce the 'declared weak after
+// its first use' warning.
+#pragma weak pr47663_f = some_function // expected-note {{possible target}}
+void q() { pr47663_f; } // expected-error {{overloaded}}
+
+#pragma clang attribute push (__attribute__((weak)), apply_to = function) // expected-note {{'weak' attribute here}}
+void pr47663_g(); // expected-warning {{declared weak after its first use}}
+#pragma clang attribute pop
+extern int pr47663_h __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_z() __attribute__((weak_import));
+
+// 'weak' on a definition means something different from 'weak' on a
+// declaration. Don't warn in that case.
+void weak_def_after_use();
+extern int weak_def_after_use_v;
+void use_wdau() { weak_def_after_use(); weak_def_after_use_v = 0; }
+__attribute__((weak)) void weak_def_after_use() {}
+__attribute__((weak)) int weak_def_after_use_v;
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8154,6 +8154,9 @@
   W.setUsed(true);
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
 IdentifierInfo *NDId = ND->getIdentifier();
+// FIXME (PR47796): Check for a previous declaration with the target name
+// here, and build a redeclaration of it. Check whether the previous
+// declaration is used and warn if so.
 NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
 NewD->addAttr(
 AliasAttr::CreateImplicit(Context, NDId->getName(), W.getLocation()));
Index: 

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

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

rsmith added a comment.

In D76620#2324858 , @erichkeane wrote:

> The feature that this supports is a part of the SYCL 2020 Provisional Spec, I 
> thought that was sufficient.  We'll look into an RFC to re-submit in the 
> future.

Does that cover only functionality that can be implemented in terms of this 
builtin, or the builtin itself? If so, what is the functionality that needs to 
be supported? That information will be a good starting point for the RFC.

In D76620#2326499 , @keryell wrote:

> Otherwise, just removing a feature used for almost 6 months will cause some 
> kind of forking pain...

Well, there have at least not been any LLVM releases with this feature yet. 
(We're on the cusp of releasing LLVM 11, which would be the first release with 
this functionality.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76620

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

[PATCH] D88712: [CGBuiltin] Respect asm labels and redefine_extname for builtins with specialized emitting

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

rsmith added a comment.

In D88712#2325823 , @MaskRay wrote:

> In D88712#2324105 , @rsmith wrote:
>
>> What are the expected semantics in this case? Is this:
>>
>> - the function is still the builtin, but if it ends up being a libcall, call 
>> a function with a different asm name, or
>> - the function is not considered to be the builtin any more, or
>> - something else?
>>
>> I think this patch is approximately the first thing, but it's also cutting 
>> off emission for cases where we wouldn't emit a libcall. Should we make that 
>> distinction?
>
> Yes, we do the first one. I mentioned the limitation in the description. This 
> happens with some functions like `abs` which clang has customized emit 
> without using a library call.

What should happen for a case where the LLVM mid-level optimizers insert a 
libcall (for example, inventing a call to `memcpy`), and `memcpy` was renamed 
at the source level to another name? Do we still call the original name?

Generally I wonder if we should instead be renaming the function in LLVM's 
`TargetLibraryInfo`, rather than getting the frontend to try to set up a 
situation where LLVM is unlikely to generate a call to the "wrong" name for the 
builtin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88712

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' to 'this' pointer arguments.

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

A lot of the test changes here seem to be over-constraining the existing tests. 
Tests that don't care about `nonnull` / `dereferenceable` `this` pointers 
should generally not be checking for that. Instead of looking for `nonnull 
dereferenceable(...)`, I'd prefer to see `{{[^,]*}}` or similar to skip any 
parameter attributes.

I would also like to see some tests that directly cover the new functionality: 
specifically, tests that check that the argument to `dereferenceable` is 
correct, including cases such as classes with virtual bases, virtual function 
calls, calls through pointers to member functions, and a check that we don't 
emit the attributes under `-fno-delete-null-pointer-checks`.




Comment at: clang/lib/CodeGen/CGCall.cpp:2174
+  }
+
   unsigned ArgNo = 0;

CJ-Johnson wrote:
> jdoerfert wrote:
> > Even if null pointer is valid we should place dereferenceable.
> > 
> > We also could never place nonnull and let the middle-end make the 
> > dereferenceable -> nonnull deduction, though I don't see why we can't just 
> > add nonnull here.
> I re-ran ninja check after making this fix and addressed the newly-affected 
> tests. So the patch is fully up to date :)
The LLVM LangRef says that in address space 0, `dereferenceable` implies 
`nonnull`, so I don't think we can emit `dereferenceable` in 
`NullPointerIsValid` mode, and we'd need to use `dereferenceable_or_null` 
instead. (Perhaps the LangRef is wrong, though, and the `null_pointer_is_valid` 
function attribute overrides that determination.)



Comment at: clang/test/CodeGenCXX/array-default-argument.cpp:22-24
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1BC1E1A([[TEMPORARY:.*]])
   // CHECK-EH:  unwind label %[[A_AND_PARTIAL_ARRAY_LPAD:.*]]
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1AD1Ev([[TEMPORARY:.*]])

This has changed the meaning of the test. Previously we were checking the 
argument bound in the first call is also passed to the second and third calls; 
now we're not checking the call arguments at all, because we re-bind 
`TEMPORARY` in each `CHECK` line.



Comment at: 
clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp:126
 //
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* %[[RIGHT]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* nonnull %[[RIGHT]])
 // CHECK: ret

Why does this call get `nonnull` but not `dereferenceable`? Seems like we 
should be able to use at least `dereferenceable(sizeof(Right))` here -- but I 
think we could actually be more aggressive and pass 
`dereferenceable(sizeof(ChildOverride) - offsetof(ChildOverride, ))`.


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

https://reviews.llvm.org/D17993

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

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

rsmith added a comment.

In D89212#2324127 , @rjmccall wrote:

> This seems like a good idea in the abstract; we'll need to figure out what 
> the practical consequences are.

Looks like it triggers warnings in Abseil at least; there, the code looks like 
this:

  // spinlock.h
  void AbslInternalSpinLockDelay();
  inline void lock(...) {
AbslInternalSpinLockDelay();
  }



  // spinlock.cc
  __attribute__((weak)) void AbslInternalSpinLockDelay() { ... }

... and adding the `weak` attribute to the declaration would change the meaning 
of the program (we want an `external` reference, not an `extern_weak` 
reference).

Perhaps we should only warn if the function is not defined in the same 
translation unit? If it is defined, then I think its address can never be null, 
and CodeGen will emit it with the correct linkage. We still have the problem 
that `==` comparisons against pointers to the function can incorrectly evaluate 
to `false`, but I think that's really a problem with aliases rather than with 
weakness. (C++ requires that `void f(), g(); bool b = f == g;` initializes `b` 
to `false` even though the only correct answer is that we don't know; we refuse 
to evaluate the comparison if either `f` or `g` is weak, but I think that's 
only really addressing the problem that they could both be null, not that they 
could have the same non-null address, since the latter problem isn't specific 
to weak declarations. I think the constant evaluator is being 
overly-conservative, and could evaluate `f == g` to `false` if `f` and `g` are 
both weak but at least one of them is defined locally, under the 
language-guaranteed assumption that distinct functions have different 
addresses. And perhaps we should have a way of declaring a function as 
potentially aliasing another function without actually defining the function as 
an alias, as a way to turn off that assumption independent of weakness?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89212

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

[PATCH] D88712: [CGBuiltin] Respect asm labels and redefine_extname for builtins with specialized emitting

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

rsmith added a comment.

What are the expected semantics in this case? Is this:

- the function is still the builtin, but if it ends up being a libcall, call a 
function with a different asm name, or
- the function is not considered to be the builtin any more, or
- something else?

I think this patch is approximately the first thing, but it's also cutting off 
emission for cases where we wouldn't emit a libcall. Should we make that 
distinction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88712

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

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

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

rsmith created this revision.
rsmith added reviewers: rjmccall, aaron.ballman.
Herald added a project: clang.
rsmith requested review of this revision.

Such code will not work in general; we might have already used the
non-weakness, for example when constant-evaluating an address comparison
Thisor when generating code for the declaration.

Modern versions of GCC reject some such cases, but silently accept
others (based, it appears, on whether the weakness of the declaration
was already inspected). It doesn't seem feasible for us to exactly
follow their behavior, and it might be problematic to start rejecting
cases that GCC accepts and that work in practice, so only warn on this
rather than rejecting, at least for now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89212

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-weak.c
  clang/test/Sema/init.c

Index: clang/test/Sema/init.c
===
--- clang/test/Sema/init.c
+++ clang/test/Sema/init.c
@@ -155,7 +155,7 @@
 int PR4386_a = ((void *) PR4386_bar) != 0;
 int PR4386_b = ((void *) PR4386_foo) != 0; // expected-error{{initializer element is not a compile-time constant}}
 int PR4386_c = ((void *) PR4386_zed) != 0;
-int PR4386_zed() __attribute((weak));
+int PR4386_zed() __attribute((weak)); // expected-warning{{'PR4386_zed' declared weak after its first use}} expected-note {{attribute}}
 
 //  (derived from SPEC vortex benchmark)
 typedef char strty[10];
Index: clang/test/Sema/attr-weak.c
===
--- clang/test/Sema/attr-weak.c
+++ clang/test/Sema/attr-weak.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-unknown-macosx10.3.0 -DMACOS
+// RUN: %clang_cc1 -verify -fsyntax-only %s -triple x86_64-linux-gnu -DLINUX
 
 extern int f0() __attribute__((weak));
 extern int g0 __attribute__((weak));
@@ -25,3 +26,47 @@
 
 static void pr14946_f();
 void pr14946_f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
+
+void some_function();
+
+void pr47663_a();
+void pr47663_b();
+static void pr47663_c();
+void pr47663_d();
+void pr47663_e(); // expected-warning {{declared weak after its first use}}
+void pr47663_f(); // expected-note {{possible target}}
+void pr47663_g();
+int pr47663_h;
+void pr47663_z() __attribute__((weak));
+void use() {
+  int arr_a[_a ? 1 : -1];
+  int arr_b[_b ? 1 : -1];
+  int arr_c[_c ? 1 : -1];
+  int arr_d[_d ? 1 : -1];
+  int arr_e[_e ? 1 : -1];
+  int arr_f[_f ? 1 : -1];
+  int arr_g[_g ? 1 : -1];
+  int arr_h[_h ? 1 : -1]; // expected-warning {{will always evaluate to 'true'}}
+  int arr_z[_z ? -1 : 1];
+}
+void pr47663_a() __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_b() __attribute__((weak_import)); // expected-warning {{declared weak after its first use}} expected-note {{'weak_import' attribute here}}
+#ifdef LINUX
+static void pr47663_c() __attribute__((weakref, alias("might_not_exist"))); // expected-warning {{declared weak after its first use}} expected-note {{'weakref' attribute here}}
+#endif
+#ifdef MACOS
+void pr47663_d() __attribute__((availability(macos, introduced=10.4))); // expected-warning {{declared weak after its first use}} expected-note {{'availability' attribute here}}
+#endif
+#pragma weak pr47663_e // expected-note {{pragma 'weak' here}}
+
+// FIXME: This should warn; see PR47796. But it actually creates a bogus
+// overload set. When this is fixed, ensure we produce the 'declared weak after
+// its first use' warning.
+#pragma weak pr47663_f = some_function // expected-note {{possible target}}
+void q() { pr47663_f; } // expected-error {{overloaded}}
+
+#pragma clang attribute push (__attribute__((weak)), apply_to = function) // expected-note {{'weak' attribute here}}
+void pr47663_g(); // expected-warning {{declared weak after its first use}}
+#pragma clang attribute pop
+int pr47663_h __attribute__((weak)); // expected-warning {{declared weak after its first use}} expected-note {{'weak' attribute here}}
+void pr47663_z() __attribute__((weak_import));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8151,6 +8151,9 @@
   W.setUsed(true);
   if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
 IdentifierInfo *NDId = ND->getIdentifier();
+// FIXME (PR47796): Check for a previous declaration with the target name
+// here, and build a redeclaration of it. Check whether the previous
+// declaration is used and warn if so.
 NamedDecl *NewD = 

[PATCH] D88498: [FPEnv] Evaluate initializers in constant rounding mode

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

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:2290
+  // rounding mode.
+  if (VD->isFileVarDecl() || VD->isConstexpr() ||
+  (!getLangOpts().CPlusPlus && VD->isStaticLocal())) {

mibintc wrote:
> sepavloff wrote:
> > sepavloff wrote:
> > > rsmith wrote:
> > > > It's far from clear to me that this is correct in C++. In principle, 
> > > > for a dynamic initializer, the rounding mode could have been set by an 
> > > > earlier initializer.
> > > > 
> > > > Perhaps we can make an argument that, due to the permission to 
> > > > interleave initializers from different TUs, every dynamic initializer 
> > > > must leave the program in the default rounding mode, but I don't think 
> > > > even that makes this approach correct, because an initializer could do 
> > > > this:
> > > > 
> > > > ```
> > > > double d;
> > > > double e = (fesetround(...), d = some calculation, 
> > > > fesetround(...default...), d);
> > > > ```
> > > > 
> > > > I think we can only do this in C and will need something different for 
> > > > C++.
> > > > 
> > > > (I think this also has problems in C++ due to constexpr functions: it's 
> > > > not the case that all floating point operations that will be evaluated 
> > > > as part of the initializer lexically appear within it.)
> > > I changed the code to confine it with C and constexpr only. Hopefully 
> > > this would be enough to enable build of SPECS attempted by @mibintc 
> > > (https://reviews.llvm.org/D87528#2295015). However in long-term 
> > > perspective we should return to this code.
> > > 
> > > The intent was to align behavior of C and C++. If an initializer is valid 
> > > in C, then it should produce the same code in C++. If the source code like
> > > ```
> > > float appx_coeffs_fp32[3 * 256] = {
> > > SEGMENT_BIAS + 1.4426950216,
> > > …
> > > ```
> > > produces compact table in C mode and huge initialization code in C++, it 
> > > would be strange from user viewpoint and would not give any advantage.
> > > 
> > > C in C2x presents pretty consistent model, provided that `#pragma STDC 
> > > FENV_ROUND FE_DYNAMIC` does not set constant rounding mode. Initializers 
> > > for variables with static allocation are always evaluated in constant 
> > > rounding mode and user can chose the mode using pragma FENV_ROUND.
> > > 
> > > When extending this model to C++ we must solve the problem of dynamic 
> > > initialization. It obviously occurs in runtime rounding mode, so changing 
> > > between static and dynamic initialization may change semantics. If 
> > > however dynamic initialization of global variable occurs in constant 
> > > rounding mode (changing FP control modes in initializers without 
> > > restoring them is UB), static and dynamic initialization would be 
> > > semantically equivalent.
> > > 
> > > We cannot apply the same rule to local static variables, as they are 
> > > treated differently in C and C++. So the code:
> > > ```
> > > float func_01(float x) {
> > >   #pragma STDC FENV_ACCESS ON
> > >   static float val = 1.0/3.0;
> > >   return val;
> > > }
> > > ```
> > > Would be compiled differently in C and C++.
> > > 
> > Additional note:
> > 
> > If initialization is dynamic and constant rounding mode is not default, the 
> > body of initializer is executed with dynamic rounding mode set to the 
> > constant mode. That is, the code:
> > ```
> > #pragma STDC FENV_ROUND FE_UPWARD
> > float var = some_init_expr;
> > ```
> > generates code similar to:
> > ```
> > float var = []()->float {
> >   #pragma STDC FENV_ROUND FE_UPWARD
> >   return some_init_expr;
> > }
> > ```
> > 
> > So initializers of global variable must conform to:
> > 
> > 1. If the initialization is dynamic and dynamic rounding mode is changed in 
> > the initializer, it must be restored upon finishing the initializer.
> > 2. The initializer is evaluated in constant rounding mode. If the 
> > initialization is dynamic, the initializing code is executed in dynamic 
> > rounding mode set to the constant rounding mode.
> > 
> > These seems enough to provide compatibility with C and the same semantics 
> > for static and dynamic initialization.
> @rsmith Serge changed the patch to confine it with C and constexpr only, is 
> that adequate to move forward with this patch, and we can return to C++ at 
> some point down the road?
This still seems inappropriate for the `constexpr` case -- we shouldn't have 
different behavior based on whether an expression appears directly in the 
initializer or in a `constexpr` function invoked by that initializer. (It also 
violates the C++ standard's recommendation that floating-point evaluation 
during translation and at runtime produce the same value.) Please see the 
discussion in D87528. Let's continue the conversation there; we shouldn't be 
splitting this discussion across two separate review threads.


Repository:
  rG LLVM Github Monorepo

CHANGES 

[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain

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

rsmith added a comment.

This seems like it would break the opposite situation: a user places a custom 
clang and the corresponding libc++ somewhere (perhaps in `$HOME`, or perhaps 
they run Clang from the build tree). Right now we use the custom clang and its 
corresponding libc++, but with this change, I would expect we'd instead pick up 
the SDK version of libc++. Is that what we want?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88984

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

[PATCH] D85144: [clang] Improve Dumping of APValues

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

rsmith added a comment.

Ideally we should be tree-dumping the `LValue` representation rather than 
pretty-printing it (and similarly for member pointers and label address 
differences). The problem with doing so is that we can't interpret the 
`LValuePathEntry`s without knowing the type of the lvalue base. However, we 
could fix that: `LValuePathEntry` has spare bits. As a result of PR8256, we 
require the size *in bits* of an array type to fit into 64 bits, so the maximum 
possible array index is 2^61 (and the pointer representation is a `Decl*`, 
which has 3 spare low-order bits). So we could track which kind of path entry 
it is with no additional storage cost, which would remove the need to have an 
`ASTContext` and a `QualType` in order to be able to correctly dump an 
`APValue` in `LValue` form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85144

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

[PATCH] D88643: [NFC] Correct name of profile function to Profile in APValue

[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.

Good idea. (My use for this in class type non-type template parameters doesn't 
directly use `APValue`s as values in a folding set, but that seems like a 
reasonable use case for this functionality.)




Comment at: clang/include/clang/AST/APValue.h:366-367
   /// Profile this value. There is no guarantee that values of different
-  /// types will not produce the same profiled value, so the type should
-  /// typically also be profiled if it's not implied by the context.
-  void profile(llvm::FoldingSetNodeID ) const;
+  /// types will not produce the same Profiled value, so the type should
+  /// typically also be Profiled if it's not implied by the context.
+  void Profile(llvm::FoldingSetNodeID ) const;

These ones shouldn't be capitalized :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88643

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

[PATCH] D87853: [SemaTemplate] Stop passing insertion position around during VarTemplate instantiation

[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/D87853/new/

https://reviews.llvm.org/D87853

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

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

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

rsmith added a comment.

In D54222#2313686 , @JonasToth wrote:

> I am confused now, but richard knows this stuff very well! The patches you 
> referenced showed only cases where the static was defined in an inline-header 
> definition (and templates). Does this make a difference on the semantics?
> This should be clear before we continue.

Local static variables in inline functions and function templates are required 
by the C++ language semantics to result in exactly one variable in the whole 
program. In easy / typical cases, that works. However, this fails in at least 
the following cases:

- Under the MS ABI, DLLs behave more like separate programs sharing an address 
space than like a single program, at least by default, so by default each DLL 
will see a different static local variable (and similarly for class statics and 
other vague linkage entities). Marking the class or function as 
`__declspec(dllexport)` / `__declspec(dllimport)` as appropriate would be 
sufficient to make the static variable behave properly if it appears on the DLL 
interface; if you don't mark it suitably, you broke the DLL import/export rules 
.
- On POSIX systems, `dlopen` with `RTLD_LOCAL` (which is, unfortunately, the 
default for many systems), or linking a shared library with `-Bsymbolic`, can 
result in symbols being duplicated across the program. That's sort of 
fundamental -- that's what those options are supposed to do -- but they break 
C++ language semantics in various ways, this being one of them. (The idea that 
`dlopen` with `RTLD_LOCAL` creates a local copy of the specified shared library 
doesn't work, because -- due to C++'s heavy reliance on vague linkage 
definitions in interfaces -- the shared library will in general also contain 
symbols that are not part of that library, such as instantiations of someone 
else's templates or copies of someone else's local static variables, inline 
functions, type info, etc., and so you can also end up with local copies of 
those other things that aren't part of the shared library and that are expected 
to be unique across the program.) The solution is, don't do that -- it's not 
safe to use those options for shared libraries that either expose or use a 
vague linkage interface (which is more or less any modern C++ interface) from a 
different library and for which symbol uniqueness across the boundary matters 
(which is very hard to falsify in general).

I'm not sure whether we can locally distinguish the first case from harmless 
cases where the exposed state is not on a DLL boundary. We can detect instances 
of the second case by looking for `dlopen` calls in C++ code that use 
`RTLD_LOCAL` and warning on them (but we presumably can't detect `-Bsymbolic` 
from `clang-tidy`, because it's a linker flag).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54222

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

[PATCH] D87962: [clang] Change the multi-character character constants from extension to implementation-defined.

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

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:109
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
+def ext_four_char_character_literal : Warning<
   "multi-character character constant">, InGroup;

aaron.ballman wrote:
> One potential reason why we don't want to warn on this by default is that 
> four char codes were quite popular back in the Mac Classic days.
Please also rename these diagnostics from ext_ to warn_.


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

https://reviews.llvm.org/D87962

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

[PATCH] D88446: docs: add documentation describing API Notes

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

rsmith accepted this revision.
rsmith added a comment.

Thanks, looks good to me.




Comment at: clang/docs/APINotes.rst:250-252
+  Note that the type is *not* parsed in the context where it will be used,
+  which means that macros are not available and nullability must be applied
+  explicitly (even in an ``NS_ASSUME_NONNULL_BEGIN`` section).

Please update this text similarly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88446

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

[PATCH] D88518: Recognize setjmp and friends as builtins even if jmp_buf is not declared yet.

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

rsmith added a comment.

In D88518#2308085 , @mstorsjo wrote:

> This broke use of setjmp for mingw on x86_64.

Thanks, should be fixed in rG8fb2a235b0f22dedba72b8b559ba33171a8dcd09 
. Now that 
we properly recognize `_setjmp` even if it has a weird signature, the 
MSVC-specific check for exactly one argument was breaking the MinGW case. I've 
removed that check and instead we only apply the custom MSVC-specific codegen 
rule if the declaration happens to be of the right form (but we apply the 
`returns_twice` attribute regardless).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88518

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

[PATCH] D88446: docs: add documentation describing API Notes

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

rsmith added inline comments.



Comment at: clang/docs/APINotes.rst:233-235
+  Note that the type is *not* parsed in the context where it will be used,
+  which means that macros are not available and nullability must be applied
+  explicitly (even in an ``NS_ASSUME_NONNULL_BEGIN`` section).

compnerd wrote:
> rsmith wrote:
> > So what context is it parsed in? Below you have an `NSArray *` example; how 
> > do we do the lookup for `NSArray`?
> A separate buffer is constructed where the annotations are processed.  During 
> semantic analysis, the requested APINotes are processed and the attributes 
> specified are applied to the declaration.
I've not been able to work out what the rule is, based on this documentation. 
The name `NSArray` isn't predeclared, so how is it found? How can a user ensure 
their types are visible to this parsing process? Are these things parsed as if 
they appear at the global scope in a translation unit in which the 
corresponding framework has been imported? (And if so, why wouldn't macros be 
available there too?) Whatever the rule is, I'd like to see it documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88446

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

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

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

rsmith added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst:7-8
+Detects functions defined in headers that return the address of a static
+local variable. These are not guaranteed to have the same addresss across
+shared libraries and could be problematic for plugins.
+

The C++ language rules do guarantee that such static locals have the same 
address throughout the entire program. Should this warning check the symbol 
visibility / `dllexport`edness? People are going to learn the wrong thing if we 
give them this diagnostic without any further explanation.

If the concern is plugins loaded with `RTLD_LOCAL`, sharing C++ interfaces 
across an `RTLD_LOCAL` boundary is pretty fundamentally broken, and this is far 
from the only way in which it goes wrong. It might be better to warn on all 
non-`RTLD_GLOBAL` calls to `dlopen` from C++ code...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54222

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

[PATCH] D87962: [clang] Change the multi-character character constants from extension to implementation-defined.

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

rsmith added a comment.

In D87962#2306043 , @aaron.ballman 
wrote:

> That doesn't sound like the right approach to me -- Remarks are usually for 
> reporting backend decision-making to the frontend for things like 
> optimization passes.

To be clear: that's how they happen to most visibly be used, but the more 
general idea is that Remarks are for purely informational messages. One test 
for whether a diagnostic could reasonably be a remark is: can we imagine anyone 
ever reasonably wanting to promote it to an error as part of the flags for some 
project? If so, then use of a remark is inappropriate. That's certainly the 
case here: it's entirely reasonable to want to be able to reject use of 
non-portable functionality such as this. So I agree, this should not be a 
remark.

> Btw, it looks like when you generated this patch, it was generated against 
> the previous diff and not trunk, so the diff view is misleading. When you 
> update the patch, can you be sure to diff against the trunk?

Please also ensure you upload a diff with full context 
.


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

https://reviews.llvm.org/D87962

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

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[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.

Looks great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63640

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

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

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

rsmith added inline comments.



Comment at: clang-tools-extra/clangd/test/check.test:1
+# RUN: clangd -log=verbose -check 2>&1 | FileCheck -strict-whitespace %s
+

This test implicitly parses a source file that `#include`s standard library 
headers, and fails if those headers aren't available; this causes the test to 
fail in some build environments. Would it be possible to make this test work in 
freestanding environments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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

[PATCH] D88518: Recognize setjmp and friends as builtins even if jmp_buf is not declared yet.

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1c604a9f5fd6: Recognize setjmp and friends as builtins even 
if jmp_buf is not declared yet. (authored by rsmith).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88518

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/setjmp.c
  clang/test/Sema/builtin-setjmp.c
  clang/test/Sema/implicit-builtin-decl.c

Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -54,13 +54,12 @@
 
 void snprintf() { }
 
-// PR8316 & PR40692
-void longjmp(); // expected-warning{{declaration of built-in function 'longjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+void longjmp();
 
 extern float fmaxf(float, float);
 
 struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+void sigsetjmp(struct __jmp_buf_tag[1], int);
 
 // PR40692
 void pthread_create(); // no warning expected
Index: clang/test/Sema/builtin-setjmp.c
===
--- clang/test/Sema/builtin-setjmp.c
+++ clang/test/Sema/builtin-setjmp.c
@@ -1,10 +1,42 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_JMP_BUF %s
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DWRONG_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DRIGHT_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DONLY_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_SETJMP %s -ast-dump 2>&1 | FileCheck %s
 
 #ifdef NO_JMP_BUF
-extern long setjmp(long *);   // expected-warning {{declaration of built-in function 'setjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
-#else
+// This happens in some versions of glibc: the declaration of __sigsetjmp
+// precedes the declaration of sigjmp_buf.
+extern long setjmp(long *); // Can't check, so we trust that this is the right type
+// FIXME: We could still diagnose the missing `jmp_buf` at the point of the call.
+// expected-no-diagnostics
+#elif WRONG_JMP_BUF
 typedef long jmp_buf;
-extern int setjmp(char);  // expected-warning@8 {{incompatible redeclaration of library function 'setjmp'}}
-  // expected-note@8{{'setjmp' is a builtin with type 'int (jmp_buf)' (aka 'int (long)')}}
+extern int setjmp(char); // expected-warning {{incompatible redeclaration of library function 'setjmp'}}
+ // expected-note@-1 {{'setjmp' is a builtin with type 'int (jmp_buf)' (aka 'int (long)')}}
+#elif RIGHT_JMP_BUF
+typedef long jmp_buf;
+extern int setjmp(long); // OK, right type.
+// expected-no-diagnostics
+#elif ONLY_JMP_BUF
+typedef int *jmp_buf;
 #endif
+
+void use() {
+  setjmp(0);
+  #ifdef NO_SETJMP
+  // expected-warning@-2 {{implicit declaration of function 'setjmp' is invalid in C99}}
+  #elif ONLY_JMP_BUF
+  // expected-warning@-4 {{implicitly declaring library function 'setjmp' with type 'int (jmp_buf)' (aka 'int (int *)')}}
+  // expected-note@-5 {{include the header  or explicitly provide a declaration for 'setjmp'}}
+  #endif
+
+  #ifdef NO_SETJMP
+  // In this case, the regular AST dump doesn't dump the implicit declaration of 'setjmp'.
+  #pragma clang __debug dump setjmp
+  #endif
+}
+
+// CHECK: FunctionDecl {{.*}} used setjmp
+// CHECK: BuiltinAttr {{.*}} Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} Implicit
Index: clang/test/CodeGen/setjmp.c
===
--- /dev/null
+++ clang/test/CodeGen/setjmp.c
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+struct __jmp_buf_tag { int n; };
+int setjmp(struct __jmp_buf_tag*);
+int sigsetjmp(struct __jmp_buf_tag*, int);
+int _setjmp(struct __jmp_buf_tag*);
+int __sigsetjmp(struct __jmp_buf_tag*, int);
+
+typedef struct __jmp_buf_tag jmp_buf[1];
+typedef struct __jmp_buf_tag sigjmp_buf[1];
+
+#ifdef __cplusplus
+}
+#endif
+
+void f() {
+  jmp_buf jb;
+  // CHECK: call {{.*}}@setjmp(
+  setjmp(jb);
+  

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

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

rsmith added a comment.

In D67678#1856229 , @rsmith wrote:

> In D67678#1836953 , @dexonsmith 
> wrote:
>
>> In D67678#1836922 , @rsmith wrote:
>>
>>> If there's no timeline to update the macOS SDK, then perhaps we could add a 
>>> hack to Clang to allow these conversions only in limited contexts (the 
>>> specific parts of the macOS SDK that are relying on them). Do you know how 
>>> many such places there might be? If it's just a few functions, we could 
>>> match against the function names, or depending on what `SIMD_CFUNC` expands 
>>> to, perhaps we could match that. Failing that, we could set a 
>>> platform-specific default or similar.
>>
>> A hack like this sounds pragmatic to me.  An initial look suggests it's a 
>> small number of frameworks; I suspect many functions, but we could 
>> potentially match on partial file path or `SIMD_CFUNC`.  We need a deeper 
>> audit across our internal stack to be sure, though.  I don't think we'll 
>> have bandwidth for that until ~late February.
>
> OK, that seems fine to me. I don't want this to miss another release if we 
> can avoid it, but that should give plenty of time to iterate on something 
> that keeps your SDK building :)

@dexonsmith Ping, I'd like to get this done for the Clang 12 release; what do 
we need to do to make sure we're not causing problems for the macOS SDK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67678

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2300013 , @rjmccall wrote:

> Being permissive about recognizing builtins when the expected signature 
> requires a type that lookup can't find seems completely reasonable.  We don't 
> really want to force library functions to take the custom-typechecking path 
> just because we want to infer an attribute for them.

Proposed fix in https://reviews.llvm.org/D88518.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D88518: Recognize setjmp and friends as builtins even if jmp_buf is not declared yet.

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

rsmith created this revision.
rsmith added reviewers: tambre, rjmccall.
Herald added a project: clang.
rsmith requested review of this revision.

This happens in glibc's headers. It's important that we recognize these
functions so that we can mark them as returns_twice.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88518

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/setjmp.c
  clang/test/Sema/builtin-setjmp.c
  clang/test/Sema/implicit-builtin-decl.c

Index: clang/test/Sema/implicit-builtin-decl.c
===
--- clang/test/Sema/implicit-builtin-decl.c
+++ clang/test/Sema/implicit-builtin-decl.c
@@ -54,13 +54,12 @@
 
 void snprintf() { }
 
-// PR8316 & PR40692
-void longjmp(); // expected-warning{{declaration of built-in function 'longjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+void longjmp();
 
 extern float fmaxf(float, float);
 
 struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
+void sigsetjmp(struct __jmp_buf_tag[1], int);
 
 // PR40692
 void pthread_create(); // no warning expected
Index: clang/test/Sema/builtin-setjmp.c
===
--- clang/test/Sema/builtin-setjmp.c
+++ clang/test/Sema/builtin-setjmp.c
@@ -1,10 +1,42 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_JMP_BUF %s
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DWRONG_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DRIGHT_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DONLY_JMP_BUF %s -ast-dump | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -DNO_SETJMP %s -ast-dump 2>&1 | FileCheck %s
 
 #ifdef NO_JMP_BUF
-extern long setjmp(long *);   // expected-warning {{declaration of built-in function 'setjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header .}}
-#else
+// This happens in some versions of glibc: the declaration of __sigsetjmp
+// precedes the declaration of sigjmp_buf.
+extern long setjmp(long *); // Can't check, so we trust that this is the right type
+// FIXME: We could still diagnose the missing `jmp_buf` at the point of the call.
+// expected-no-diagnostics
+#elif WRONG_JMP_BUF
 typedef long jmp_buf;
-extern int setjmp(char);  // expected-warning@8 {{incompatible redeclaration of library function 'setjmp'}}
-  // expected-note@8{{'setjmp' is a builtin with type 'int (jmp_buf)' (aka 'int (long)')}}
+extern int setjmp(char); // expected-warning {{incompatible redeclaration of library function 'setjmp'}}
+ // expected-note@-1 {{'setjmp' is a builtin with type 'int (jmp_buf)' (aka 'int (long)')}}
+#elif RIGHT_JMP_BUF
+typedef long jmp_buf;
+extern int setjmp(long); // OK, right type.
+// expected-no-diagnostics
+#elif ONLY_JMP_BUF
+typedef int *jmp_buf;
 #endif
+
+void use() {
+  setjmp(0);
+  #ifdef NO_SETJMP
+  // expected-warning@-2 {{implicit declaration of function 'setjmp' is invalid in C99}}
+  #elif ONLY_JMP_BUF
+  // expected-warning@-4 {{implicitly declaring library function 'setjmp' with type 'int (jmp_buf)' (aka 'int (int *)')}}
+  // expected-note@-5 {{include the header  or explicitly provide a declaration for 'setjmp'}}
+  #endif
+
+  #ifdef NO_SETJMP
+  // In this case, the regular AST dump doesn't dump the implicit declaration of 'setjmp'.
+  #pragma clang __debug dump setjmp
+  #endif
+}
+
+// CHECK: FunctionDecl {{.*}} used setjmp
+// CHECK: BuiltinAttr {{.*}} Implicit
+// CHECK: ReturnsTwiceAttr {{.*}} Implicit
Index: clang/test/CodeGen/setjmp.c
===
--- /dev/null
+++ clang/test/CodeGen/setjmp.c
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+struct __jmp_buf_tag { int n; };
+int setjmp(struct __jmp_buf_tag*);
+int sigsetjmp(struct __jmp_buf_tag*, int);
+int _setjmp(struct __jmp_buf_tag*);
+int __sigsetjmp(struct __jmp_buf_tag*, int);
+
+typedef struct __jmp_buf_tag jmp_buf[1];
+typedef struct __jmp_buf_tag sigjmp_buf[1];
+
+#ifdef __cplusplus
+}
+#endif
+
+void f() {
+  jmp_buf jb;
+  // CHECK: call {{.*}}@setjmp(
+  setjmp(jb);
+  // 

[PATCH] D88446: docs: add documentation describing API Notes

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

rsmith added a comment.

Broadly, it seems reasonable to me for Clang to support this. I have no major 
concerns with the overall approach here, and it seems like you already have 
sufficient implementation experience with this approach to know that it's going 
to work out well in practice. I'm happy that all of the points in 
http://clang.llvm.org/get_involved.html for Clang extensions are covered -- or 
will be covered by the work to implement this.




Comment at: clang/docs/APINotes.rst:5-12
+**The Problem:** You have headers you want to use, but you also want to add
+extra information to some of the APIs. You don't want to put that information
+in the headers themselves --- perhaps because you want to keep them clean for
+other clients, or perhaps because they're from some open source project and you
+don't want to modify them at all.
+
+**Incomplete solution:** Redeclare all the interesting APIs in your own header

"API" should generally be used only as an uncountable noun. If you mean 
something more concrete here, such as "a function, method, type, ...", then 
saying that would be better, since "API" can be interpreted as meaning an 
individual such entity or a collection of such entities, depending on the 
reader.



Comment at: clang/docs/APINotes.rst:45
+Clang will search for API notes files next to module maps only when passed the
+``-fapinotes-modules`` option.
+

Can we add a hyphen between `api` and `notes` here? `-fapinotes` is a little 
hard to read.



Comment at: clang/docs/APINotes.rst:187-190
+  - "N"onnull (``_Nonnull``)
+  - "O"ptional (``_Nullable``)
+  - "U"nspecified (``_Null_unspecified``)
+  - "S"calar (deprecated)

Is it important that these are single letters? Spelling out the name in full 
(as you do for other enumerated values like `MethodKind` and `PropertyKind`) 
would seem a little more readable. (We could accept the single-letter forms as 
aliases.)



Comment at: clang/docs/APINotes.rst:233-235
+  Note that the type is *not* parsed in the context where it will be used,
+  which means that macros are not available and nullability must be applied
+  explicitly (even in an ``NS_ASSUME_NONNULL_BEGIN`` section).

So what context is it parsed in? Below you have an `NSArray *` example; how do 
we do the lookup for `NSArray`?



Comment at: clang/docs/APINotes.rst:273
+  Used for ``NSError`` code enums. The value is the name of the associated 
domain
+  ``NSString`` constant; an empty string ("") means the enum is a normal enum
+  rather than an error code.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88446

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

[PATCH] D88445: Use "default member initializer" instead of "in-class initializer" for diagnostics

[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/D88445/new/

https://reviews.llvm.org/D88445

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

[PATCH] D88498: [FPEnv] Evaluate initializers in constant rounding mode

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

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



Comment at: clang/lib/Parse/ParseDecl.cpp:2290
+  // rounding mode.
+  if (VD->isFileVarDecl() || VD->isConstexpr() ||
+  (!getLangOpts().CPlusPlus && VD->isStaticLocal())) {

It's far from clear to me that this is correct in C++. In principle, for a 
dynamic initializer, the rounding mode could have been set by an earlier 
initializer.

Perhaps we can make an argument that, due to the permission to interleave 
initializers from different TUs, every dynamic initializer must leave the 
program in the default rounding mode, but I don't think even that makes this 
approach correct, because an initializer could do this:

```
double d;
double e = (fesetround(...), d = some calculation, fesetround(...default...), 
d);
```

I think we can only do this in C and will need something different for C++.

(I think this also has problems in C++ due to constexpr functions: it's not the 
case that all floating point operations that will be evaluated as part of the 
initializer lexically appear within it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88498

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

We've hit a fairly subtle miscompile caused by this patch.

glibc's setjmp.h looks like this (irrelevant parts removed):

  struct __jmp_buf_tag { /*...*/ };
  extern int __sigsetjmp(struct __jmp_buf_tag __env[1], int);
  typedef struct __jmp_buf_tag sigjmp_buf[1];
  #define sigsetjmp __sigsetjmp

This worked fine with the old approach. But with the new approach, we decide 
the declaration of `__sigsetjmp` is not a builtin, because at its point of 
declaration, we can't compute the "proper" type because `sigjmp_buf` has not 
been declared yet. As a result, we don't add a `BuiltinAttr` to `__sigsetjmp`, 
but much more critically, we don't add a `ReturnsTwiceAttr`, which results in 
miscompiles in calls to this function. (I think `sigsetjmp` is the only 
affected function with glibc. `jmp_buf` is declared prior to `__setjmp` and 
friends.)

I suppose we don't actually care what the parameter types for `__sigsetjmp` 
are, and it would be fine (and much safer) to treat any function with that name 
as a builtin, like we used to. Perhaps we should have a way of marking builtins 
as "the given type is what we expect / what we will implicitly declare, but 
it's OK if it doesn't actually match"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D68364: Implement C++20's P0784 (More constexpr containers)

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

rsmith added a comment.

In D68364#2299336 , @ldionne wrote:

> In D68364#2294569 , @jwakely wrote:
>
>> I don't think your implementation is valid. I think P0784 only allows 
>> new-expressions and calls to `std::allocator::allocate` in constexpr 
>> functions, not calls to `operator new`.
>
> Hmm, right. It's actually impossible to implement `__libcpp_allocate` in a 
> constexpr-friendly manner, since it's just allocating bytes.

Right, we need to rely on implementation magic. I wrote up a document proposing 
how to do this 

 a year or so ago, and shared it with some GCC folks (I don't remember whether 
it was mailed to the GCC mailing list or just to Jason or what) suggesting that 
we follow option 3 of that doc -- in particular, this permits calls to 
`operator new` "//transitively// within a member of `std::allocator`" 
(emphasis mine), precisely so that stdlib implementations can pull out helper 
functions as libc++ does, and don't need to branch on `is_constant_evaluated()` 
for this at all. That said, this is all vendor extension territory, and GCC is 
of course under no obligation to follow my proposal, and indeed seems not to 
have followed this detail of it. So be it.

But... there's no question of "valid" here :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68364

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

[PATCH] D88333: Better diagnostics for anonymous bit-fields with attributes or an initializer

[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!




Comment at: clang/lib/Parse/ParseDecl.cpp:4126
 /// struct-declarator: declarator[opt] ':' constant-expression
-if (Tok.isNot(tok::colon)) {
+if (Tok.isNot(tok::colon) && !isCXX11AttributeSpecifier()) {
   // Don't parse FOO:BAR as if it were a typo for FOO::BAR.

I think this change is redundant now.



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2317
+  // current grammar rules as of C++20.
+  if (Tok.isNot(tok::colon) && !isCXX11AttributeSpecifier())
 ParseDeclarator(DeclaratorInfo);

Similarly here, I think the check for an attribute specifier is now redundant.


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

https://reviews.llvm.org/D88333

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

[PATCH] D88445: Use "default member initializer" instead of "in-class initializer" for diagnostics

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

rsmith added a comment.

Thanks!




Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:864-867
+  "default member initialization of non-static data member is a C++11 "
+  "extension">, InGroup;
 def warn_cxx98_compat_nonstatic_member_init : Warning<
+  "default member initialization of non-static data members is incompatible "

Consider "initialization of" -> "initializer for", so we use the standard term 
in full.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8506-8524
+def err_default_member_initializer_non_const : Error<
   "non-const static data member must be initialized out of line">;
-def err_in_class_initializer_volatile : Error<
+def err_default_member_initializer_volatile : Error<
   "static const volatile data member must be initialized out of line">;
-def err_in_class_initializer_bad_type : Error<
+def err_default_member_initializer_bad_type : Error<
   "static data member of type %0 must be initialized out of line">;
+def ext_default_member_initializer_float_type : ExtWarn<

For a static member, it's just an initializer, not a default member 
initializer. I think "in-class initializer" is probably the best we can say 
here.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8533-8536
+def ext_default_member_initializer_non_constant : Extension<
+  "default member initializer for static data member is not a constant "
+  "expression; folding it to a constant is a GNU extension">,
+  InGroup;

Likewise, this case is an initializer for a static member, and is not a default 
member initializer.


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

https://reviews.llvm.org/D88445

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:2446
+  fpOptions.isFPConstrained()) {
+Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+return false;

FFDiag



Comment at: clang/lib/AST/ExprConstant.cpp:2677
+  if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained() &&
+  llvm::APFloatBase::opOK != status) {
+Info.FFDiag(E, diag::note_constexpr_float_arithmetic_strict);

Nit: do these checks in the opposite order; we don't need to compute 
`FPOptions` if the calculation was exact. (I assume we don't get an OK status 
if the result was rounded?)



Comment at: clang/lib/AST/ExprConstant.cpp:2827
 
-  if (!handleFloatFloatBinOp(Info, E, LHSFloat, Opcode,
- RHSElt.getFloat())) {
+  if (!handleFloatFloatBinOp(Info, dyn_cast(E), LHSFloat,
+ Opcode, RHSElt.getFloat())) {

Change this function to take a `BinaryOperator` too rather than using a cast 
here.



Comment at: clang/lib/AST/ExprConstant.cpp:4158
 } else if (RHS.isFloat()) {
+  const BinaryOperator *BE = dyn_cast(E);
+  const FPOptions fpOptions = BE->getFPFeaturesInEffect(

Change the `E` member to be a `BinaryOperator*` rather than using a cast here.



Comment at: clang/lib/AST/ExprConstant.cpp:12242
+// but this doesn't affect constant folding since NaN are
+// not constant expressions.
+

We can form NaNs in constant evaluation using `__builtin_nan()`, and that's 
exposed to the user via `std::numeric_limits`, so I think we do need to check 
for this.



Comment at: clang/lib/AST/ExprConstant.cpp:13290
   case Builtin::BI__builtin_fabsf128:
+// The c lang ref says that "fabs raises no floating-point exceptions,
+// even if x is a signaling NaN. The returned value is independent of





Comment at: clang/lib/AST/ExprConstant.cpp:13347
 bool FloatExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
+  // In C lang ref, the unary - raises no floating point exceptions,
+  // even if the operand is signalling.





Comment at: clang/lib/AST/ExprConstant.cpp:13383
+  return ST && DT && DT->isRealFloatingType() && ST->isRealFloatingType() &&
+ ST->getKind() <= DT->getKind();
+}

I don't think this works in general. There is no "wideness" order between PPC 
double-double and IEEE float128.



Comment at: clang/lib/AST/ExprConstant.cpp:13406
+if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained() &&
+!isWideningFPConversion(E->getType(), SubExpr->getType())) {
+  // In C lang ref, footnote, cast may raise inexact exception.

Can we check whether the value is representable, not only the type? Eg, it 
would seem preferable to accept `float x = 1.0;`



Comment at: clang/lib/AST/ExprConstant.cpp:13407
+!isWideningFPConversion(E->getType(), SubExpr->getType())) {
+  // In C lang ref, footnote, cast may raise inexact exception.
+  // Cast may also raise invalid.

It would be useful to say which C standard and which footnote, eg "C11 footnote 
123"



Comment at: clang/lib/AST/ExprConstant.cpp:13197-13198
   assert(E->isRValue() && E->getType()->isRealFloatingType());
+  if (Info.isStrictFP)
+return false;
   return FloatExprEvaluator(Info, Result).Visit(E);

mibintc wrote:
> rsmith wrote:
> > I think we should be able to evaluate (for example) `constexpr float f = 
> > 1.0f;` even in a strict FP context. I think only floating point operations 
> > that depend on the rounding mode should be disabled, not all floating-point 
> > evaluation. Perhaps we should propagate the `FPOptions` into 
> > `handleFloatFloatBinOp` and perform the check there, along with any other 
> > places that care (I think we probably have some builtins that we can 
> > constant-evaluate that care about rounding modes.)
> > 
> > You also need to produce a diagnostic when you treat an expression as 
> > non-constant -- please read the comment at the top of the file for details.
> > I think we should be able to evaluate (for example) `constexpr float f = 
> > 1.0f;` even in a strict FP context. I think only floating point operations 
> > that depend on the rounding mode should be disabled, not all floating-point 
> > evaluation. Perhaps we should propagate the `FPOptions` into 
> > `handleFloatFloatBinOp` and perform the check there, along with any other 
> > places that care (I think we probably have some builtins that we can 
> > constant-evaluate that care about rounding modes.)
> > 
> > You also need to produce a diagnostic when you treat an expression as 
> > non-constant -- please read the comment 

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

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

rsmith added inline comments.



Comment at: clang/include/clang/AST/APValue.h:651-653
+  /// The following functions are used as part of initialization. during
+  /// Deserialization and Importing. Reserve the space then write element
+  /// directly in place as after importing/deserialization then.

Maybe something like this?



Comment at: clang/include/clang/AST/APValue.h:613
+  /// in place as after importing/deserializating then.
+  void reserveVector(unsigned N) {
+assert(isVector() && "Invalid accessor");

aaron.ballman wrote:
> `ReserveVector`
This function changes the size of the vector, so `Reserve` doesn't seem right 
to me. How about `setVectorUninit`? (Generally including `Uninit` in the names 
of the setters that leave things uninitialized would be useful.)

Also, instead of exposing `GetInternal` functions, how about adding functions 
that return a `MutableArrayRef` holding the array:

```
MutableArrayRef setVectorUninit(unsigned N);
MutableArrayRef setLValueUninit(LValueBase B, const CharUnits 
, unsigned Size, bool OnePastTheEnd, bool IsNullPtr);
MutableArrayRef MakeMemberPointerUninit(const ValueDecl 
*Member, bool IsDerivedMember, unsigned Size);
```

(It would be nice if `MakeMemberPointer` weren't so inconsistent with 
everything else this class does. But this whole interface is a mess.)



Comment at: clang/lib/AST/APValue.cpp:779
 
-void APValue::MakeMemberPointer(const ValueDecl *Member, bool IsDerivedMember,
-ArrayRef Path) {
+void APValue::MakeEmptyMemberPointer(const ValueDecl *Member,
+ bool IsDerivedMember, unsigned Size) {

Similarly maybe mentioning `Uninit` here would be useful.



Comment at: clang/lib/AST/ASTImporter.cpp:6680-6687
 
-  APValue::ValueKind Kind = E->getResultAPValueKind();
-  if (Kind == APValue::Int || Kind == APValue::Float ||
-  Kind == APValue::FixedPoint || Kind == APValue::ComplexFloat ||
-  Kind == APValue::ComplexInt)
-return ConstantExpr::Create(Importer.getToContext(), ToSubExpr,
-E->getAPValueResult());
-  return ConstantExpr::Create(Importer.getToContext(), ToSubExpr);
+  // APValue::ValueKind Kind = E->getResultAPValueKind();
+  // if (Kind == APValue::Int || Kind == APValue::Float ||
+  // Kind == APValue::FixedPoint || Kind == APValue::ComplexFloat ||
+  // Kind == APValue::ComplexInt)
+  //   return ConstantExpr::Create(Importer.getToContext(), ToSubExpr,
+  //   E->getAPValueResult());

Delete this commented-out code, please.



Comment at: clang/lib/AST/ASTImporter.cpp:9010
+  ToPath[Idx] =
+  cast(const_cast(ImpDecl.get()));
+}

We want the path in an `APValue` to be canonical, but importing a canonical 
decl might result in a non-canonical decl.



Comment at: clang/lib/AST/ASTImporter.cpp:9030-9031
+} else {
+  FromElemTy =
+  FromValue.getLValueBase().get()->getType();
+  llvm::Expected ImpDecl =

If you're intentionally not handling `DynamicAllocLValue` here (because those 
should always be transient), a comment to that effect would be useful.



Comment at: clang/lib/AST/ASTImporter.cpp:9071
+  for (unsigned LoopIdx = 0; LoopIdx < PathLength; LoopIdx++) {
+if (FromElemTy->getAs()) {
+  const Decl *FromDecl =





Comment at: clang/lib/Serialization/ASTReader.cpp:8973-8978
+const llvm::fltSemantics  = llvm::APFloatBase::EnumToSemantics(
+static_cast(asImpl().readUInt32()));
 llvm::APFloat First = readAPFloat(FloatSema1);
-const llvm::fltSemantics  = readAPFloatSemantics(*this);
-return APValue(std::move(First), readAPFloat(FloatSema2));
-  }
-  case APValue::LValue:
-  case APValue::Vector:
-  case APValue::Array:
-  case APValue::Struct:
-  case APValue::Union:
-  case APValue::MemberPointer:
-  case APValue::AddrLabelDiff:
-// TODO : Handle all these APValue::ValueKind.
-return APValue();
+const llvm::fltSemantics  = llvm::APFloatBase::EnumToSemantics(
+static_cast(asImpl().readUInt32()));
+return APValue(std::move(First), asImpl().readAPFloat(FloatSema2));

You can assume here (and assert in the writer) that both floats in a 
`ComplexFloat` have the same `fltSemantics`.



Comment at: clang/lib/Serialization/ASTReader.cpp:9027
+for (unsigned LoopIdx = 0; LoopIdx < PathSize; LoopIdx++)
+  PathArray[LoopIdx] = asImpl().readDeclAs();
+return Result;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63640

___
cfe-commits mailing list

[PATCH] D86649: Fix for assertion failure on PR46865

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

I think the assertion is correct: if we reach this point, then we have a 
`DeclRefExpr` that is not value-dependent and refers to a declaration whose 
initializer is value-dependent. That should never happen if the variable's type 
satisfies the requirements for use in a constant expression, which is what 
`mightBeUsableInConstantExpressions` checks.

The bug here appears to be that the dependency bits on a `DeclRefExpr` are 
computed too early when it refers to a templated variable -- before we 
instantiate the initializer -- and don't get "fixed" later when the initializer 
is instantiated. `DoMarkVarDeclReferenced` would be the right place to handle 
this -- after we trigger instantiation of the initializer, we should recompute 
the dependence bits on the `DeclRefExpr`, because they can depend on the 
initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86649

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

[PATCH] D88333: Better diagnostics for anonymous bit-fields with attributes or an initializer

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

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:875-876
   "C++ standards before C++20">, InGroup, DefaultIgnore;
+def err_anon_bitfield_member_init : Error<
+  "anonymous bit-field cannot have an in-class initializer">;
 def err_incomplete_array_member_init: Error<

Please retain the diagnostic wording using proper standard terminology; the 
other diagnostics say "in-class initializer" because they predate the existence 
of the standard terminology and haven't been fixed yet. (Fixing them -- and 
renaming the corresponding functions throughout Clang -- would be great if you 
feel so inclined.)



Comment at: clang/lib/Parse/ParseDecl.cpp:4127-4129
+  // Anonymous bit-fields cannot specify attributes; the attributes
+  // appertain to the type specifier for the bit-field instead. Provide a
+  // kinder parsing error than if we just let parsing happen organically.

I think this will regress our diagnostics for this (probably more common) case:

```
struct X { 
  int a, [[attr]] b;
};
```

Instead, how about we unconditionally `DiagnoseAndSkipCXX11Attributes()` before 
and after we parse GNU attributes in the `if (!FirstDeclarator)` check up 
above? (Aside: we should probably be better about handling mixed sequences of 
GNU and C++11 attributes in general.)



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2311-2313
+  //   identifier attribute-specifier-seq[opt] ':' constant-expression
+  //   brace-or-equal-initializer[opt]
+  //   ':' constant-expression

Please mention that this is a proposed bugfix, not the standard grammar.



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2318-2320
+// Anonymous bit-fields cannot specify attributes; the attributes appertain
+// to the type specifier for the bit-field instead. Provide a kinder
+// parsing error than if we just let parsing happen organically.

(Same comment as for the C side of things.)


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

https://reviews.llvm.org/D88333

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

[PATCH] D87561: [Sema] List conversion validate character array

[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, only nits here. Please feel free to submit after addressing them, or 
request another round of review if you prefer.




Comment at: clang/lib/Sema/SemaInit.cpp:3115-3118
+bool Sema::IsStringInit(Expr *Init, const ArrayType *AT) {
+  return ::IsStringInit(Init, AT, Context) == SIF_None;
+}
+

Consider moving this up to around line 144 (just after the definition of 
`::IsStringInit`).



Comment at: clang/lib/Sema/SemaOverload.cpp:4988
+
+if (const auto *AT = S.Context.getAsArrayType(ToType))
+  if (S.IsStringInit(From->getInit(0), AT)) {

Nit: the `if` body is long, please add braces.



Comment at: clang/test/CXX/drs/dr14xx.cpp:338-340
+void f1(int);
+void f1(std::initializer_list);
+void g1() { f1({42}); }

It would be useful to also test that the right overload is used here, and for 
`g2`.



Comment at: clang/test/CXX/drs/dr14xx.cpp:424-431
+f({"abc"});
+f({((("abc")))});
+f({L"abc"});
+#if __cplusplus >= 202002L
+f({u8"abc"});
+#endif
+f({uR"(abc)"});

Please test that the correct overload is called in each of these cases 
(especially considering the unintuitive outcome of these calls).



Comment at: clang/test/CXX/drs/dr14xx.cpp:411-414
+  void f(const char[4]);
+  void f(const wchar_t[4]);
+  void f(const char16_t[4]);
+  void f(const char32_t[4]);

Mordante wrote:
> rsmith wrote:
> > rsmith wrote:
> > > Mordante wrote:
> > > > rsmith wrote:
> > > > > These should presumably be references to arrays, rather than arrays, 
> > > > > or the parameter type is as if you wrote (for example) `void f(const 
> > > > > char *)`, which shouldn't get the special treatment here.
> > > > > 
> > > > > [over.ics.list]p4 mentions this in its footnote:
> > > > > 
> > > > > "Otherwise, if the parameter type is a character array [Footnote: 
> > > > > Since there are no parameters of array type, this will only occur as 
> > > > > the referenced type of a reference parameter.] and the initializer 
> > > > > list has a single element that is an appropriately-typed 
> > > > > string-literal (9.4.3), the implicit conversion sequence is the 
> > > > > identity conversion."
> > > > Ah I missed that footnote. I reread the standard and can you confirm 
> > > > some cases?
> > > > ```
> > > > namespace A { 
> > > >   void f(const char(&)[4]);
> > > >   void g() { f({"abc"}); }
> > > > }
> > > > namespace B { 
> > > >   void f(const char(&&)[4]);
> > > >   void g() { f({"abc"}); }
> > > > } 
> > > > ```
> > > > both should work and with P0388 the array without bounds will also work.
> > > > 
> > > > I ask since this is my interpretation of the standard, but it seems 
> > > > there's a difference between implementations and `void f(const 
> > > > char(&&)[4]);` fails for "abc" but works for "ab".
> > > > It seems ICC and consider "abc" an lvalue in this case and not when 
> > > > using "ab".
> > > > 
> > > > Here's a gotbolt link with the examples https://godbolt.org/z/r1TKfx
> > > That's a really interesting example :)
> > > 
> > > The initializer is a list containing an lvalue of type `const char[4]`. 
> > > Per [dcl.init.list]/3.9 and /3.10, the behavior depends on whether the 
> > > referenced type is reference-related to `const char[4]` -- if so, then 
> > > the reference can only bind directly and a `&&` reference will be 
> > > invalid, because it's binding an rvalue reference to an lvalue, and if 
> > > not, then we create an array temporary and the `&&` binding is fine.
> > > 
> > > Per [dcl.init.ref]/4, `const char[???]` is reference-related to `const 
> > > char[4]` if they are similar types, and per [conv.qual]/2, the types are 
> > > similar if `???` is omitted or `4`, and not similar otherwise.
> > > 
> > > So:
> > > * `const char (&)[4] = {"abc"}` is ill-formed (types are the same, 
> > > binds rvalue reference to lvalue)
> > > * `const char (&&)[] = {"abc"}` is ill-formed (types are similar, binds 
> > > rvalue reference to lvalue)
> > > * `const char (&)[5] = {"abc"}` is OK (types are not similar, creates 
> > > temporary)
> > > 
> > > That seems like a very surprising outcome to me. Perhaps we should 
> > > probably ignore array bounds entirely when determining whether two types 
> > > are reference-related. I'll take this to CWG for discussion.
> > I think that's only an answer to half of your question. The other half is 
> > that [over.ics.list]p4 does not apply (directly) to either of your 
> > testcases, because the "parameter type" is a reference type, not an array 
> > type. For:
> > 
> > ```
> > namespace A { 
> >   void f(const char(&)[4]);
> >   void g() { f({"abc"}); }
> > }
> > ```
> > 
> > ... we reach [over.ics.list]p9, which says to use the rules in 
> > [over.ics.ref]. Those rules say that 

[PATCH] D88333: Correctly parse attributes on the declaration of an anonymous bit-field

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

rsmith added a comment.

I think we established rough consensus in CWG (at least, no one objected) that 
this is a grammar bug -- there's nothing for these attributes to appertain to, 
so they shouldn't be permitted. Unless CWG indicates it wants to change 
direction here I think we should continue to reject.


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

https://reviews.llvm.org/D88333

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

[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

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

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:2148
+CommonElementType == nullptr && !NumInitElts) {
   const ArrayType *AT = CGM.getContext().getAsArrayType(DestType);
   CommonElementType = CGM.getTypes().ConvertType(AT->getElementType());

rjmccall wrote:
> `AT` is now just `ArrayTy`, and I think you can just check `!Filler && 
> !NumInitElts`.
`Filler` is null if and only if `NumElements == NumInitElts`, and `NumInitElts 
<= NumElements`, so this condition reduces to `if (!NumElements)`. 
`EmitArrayConstant` responds to that case by creating a `ConstantAggregateZero` 
of its destination type. So it seems to me that we probably don't need this 
special case at all.


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

https://reviews.llvm.org/D88236

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

[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

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

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/pr47636.cpp:2
+// RUN: %clang_cc1 -o - -emit-llvm -triple x86_64-linux-pc %s | FileCheck %s
+int(&_rvref)[] {1,2,3,4};
+// CHECK: @_ZGR10intu_rvref_ = internal global [4 x i32] [i32 1, i32 2, i32 3, 
i32 4]

The AST we generate for this is:

```
`-VarDecl 0x106e7630 <:1:1, col:29> col:7 intu_rvref 'int (&&)[4]' 
listinit
  `-ExprWithCleanups 0x106e7908  'int []':'int []' xvalue
`-MaterializeTemporaryExpr 0x106e78a8  'int []':'int []' 
xvalue extended by Var 0x106e7630 'intu_rvref' 'int (&&)[4]'
  `-InitListExpr 0x106e77c0  'int [4]'
|-IntegerLiteral 0x106e76e0  'int' 1
|-IntegerLiteral 0x106e7700  'int' 2
|-IntegerLiteral 0x106e7720  'int' 3
`-IntegerLiteral 0x106e7740  'int' 4
```

... which looks surprising to me -- `MaterializeTemporaryExpr` corresponds to 
allocating storage for an object of type `T`, so `T` being incomplete is at 
least surprising, and it would seem more consistent to complete the type of the 
MTE in the same way we would complete the type of a corresponding `VarDecl`.

We also crash while processing

```
constexpr int f() { int (&_rvref)[] {1,2,3,4}; return intu_rvref[0]; }
```

... due to the same unexpected AST representation.

I don't know if we would need this `CodeGen` change in other cases; it seems 
like if we ever supported constant initialization of a class with a flexible 
array member, we'd want to do something like this. For now at least, we refuse 
to constant-evaluate things such as `struct A { int n; int arr[]; } a = {1, 2, 
3};` to an `APValue` and use the lowering-from-`Expr` path in `CGExprConstant` 
for such cases instead. So I suspect if we change the AST representation to use 
a complete type here, the code change here will be unreachable at least in the 
short term.

On the other hand, if we did choose to support constant initialization of 
flexible array members, it might be natural to treat objects of incomplete 
array type analogously to flexible array members, giving them an incomplete 
type whose size is determined from its initializer, and with that viewpoint the 
AST representation we're currently using wouldn't really be wrong. But I'm 
inclined to think that `MaterializeTemporaryExpr` should mirror the behavior of 
a `VarDecl`: the array bound should be filled in in the AST representation 
based on the initializer.


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

https://reviews.llvm.org/D88236

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

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

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

rsmith added a comment.

In D87808#2288186 , @dblaikie wrote:

> In D87808#2286664 , @rsmith wrote:
>
>> But I think this code should give the same outcome either way, because a 
>> class with any constructor other than a default/copy/move constructor must 
>> have a user-declared constructor of some kind, and so will never have an 
>> implicit default constructor.
>
> Hmm, trying to parse this. So if we're in the subtle case of having an 
> implicit default constructor (and no other (necessarily user-declared, as you 
> say) constructors) - if it's not been declared, then this function will 
> return false. If it has been declared, it'll return false... hmm, nope, then 
> it'll return true.

Hmm, yes, you're right, if the implicit default constructor would not be 
trivial, we could get different answers depending on whether we triggered its 
implicit declaration or not. And for:

  struct A { A(); };
  struct X { A a; };
  struct Y { A a; };
  void f(X x) {}
  void f(decltype(Y()) y) {}

... I see we get different debug information for `X` and `Y`, and it doesn't 
look like this patch will fix that, but querying 
`hasTrivialDefaultConstructor()` would.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300
+  bool hasCtor = false;
+  for (const auto *Ctor : RD->ctors()) {
 if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
   return false;
+if (!Ctor->isCopyOrMoveConstructor())
+  hasCtor = true;
+  }

dblaikie wrote:
> rsmith wrote:
> > This looks pretty similar to:
> > 
> > ```
> > return RD->hasUserDeclaredConstructor() && 
> > !RD->hasTrivialDefaultConstructor();
> > ```
> > 
> > (other than its treatment of user-declared copy or move constructors), but 
> > I have to admit I don't really understand what the "at least one 
> > constructor and no trivial or constexpr constructors" rule aims to achieve, 
> > so it's hard to know if this is the right interpretation. The rule as 
> > written in the comment above is presumably not exactly right -- all classes 
> > have at least one constructor, and we're not excluding classes with trivial 
> > copy or move constructors, only those with trivial default constructors.
> > 
> > I wonder if the intent would be better captured by "at least one non-inline 
> > constructor" (that is, assuming all declared functions are defined, there 
> > is at least one translation unit in which a constructor is defined that can 
> > be used as a home for the class info).
> So the general goal is to detect any type where the construction of that type 
> somewhere must invoke a constructor that will be IR-generated.
> 
> Move and copy constructors are ignored because the assumption is they must be 
> moving/copying from some other object, which must've been constructed, 
> ultimately, by a non-move/copy constructor.
> 
> Ideally this would be usable even for inline ctors - even if the ctor calls 
> get optimized away later[^1], they'd still allow us to reduce the number of 
> places the type is emitted to only those places that call the ctor.
> 
> 
> 
> [^1] actually, the way that should work doesn't seem to be working right now 
> (eg:
> type.cpp
> ```
> struct t1 { t1() { } };
> void f1(void*);
> int main() {
>   f1(new t1());
> }
> ```
> type2.cpp
> ```
> struct t1 { t1() { } };
> void f1(void* v) {
>   delete (t1*)v;
> }
> ```
> build: `clang++ type.cpp -g -Xclang -fuse-ctor-homing type2.cpp && 
> llvm-dwarfdump a.out`
> -> definition of "t1" in the DWARF
> build with optimizations: `clang++ -O3 type.cpp -g -Xclang -fuse-ctor-homing 
> type2.cpp && llvm-dwarfdump a.out`
> -> missing definition of "t1"
> `type.cpp` is chosen as a home for `t1` because it calls a user-defined ctor, 
> but then that ctor gets optimized away and there's no other mention of `t1` 
> in `type.cpp` so the type is dropped entirely. This could happen even with a 
> non-inline definition - under LTO the ctor could get optimized away (though 
> this would be safe in FullLTO - the other references to `t1` would be made to 
> refer to the definition and keep it alive - but in ThinLTO the TU defining 
> the ctor might be imported and nothing else - leaving the type unused/dropped)
> To fix this we should put 'homed' types in the retained types list so they 
> are preserved even if all other code/references to the type are dropped. I 
> think I implemented this homed type pinning for explicit template 
> specialization definitions, because they have no code attachment point, so 
> similar logic could be used for ctor homing. (vtable homing /might/ benefit 
> from this with aggressive/whole program devirtualization? Not sure - harder 
> to actually optimize away all the references to a type, but possible maybe?)
Oh, I see, that's clever and very neat.

So the intent is to identify types for which we know that either no instances 
are ever created, or some constructor 

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

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

rsmith added a comment.

In D87808#2280197 , @dblaikie wrote:

> @rsmith What's the deal with these anonymous structs/unions? Why do they have 
> copy/move constructors (are those technically called from the enclosing 
> class's copy/move constructors?) but no default constructor to be called from 
> the other ctors of the enclosing class?

Let's use the class `G` in the example. `G::G` directly initializes the field 
`g_`, so it can't call any kind of constructor for the anonymous struct member. 
Instead, anonymous structs and unions are "expanded" when building the 
constructor initializer lists for all cases other than a copy or move 
constructor (no initialization action is taken for union members by default, 
though -- not unless they have a default member initializer or an explicit 
mem-initializer). So the default constructor of an anonymous struct or union is 
never really used for anything[*].

But now consider the copy or move constructor for a type like this:

  struct X {
Y y;
union { ... };
  };

That copy or move constructor needs to build a member initializer list that (a) 
calls the `Y` copy/move constructor for `y`, and (b) performs a bytewise copy 
for the anonymous union. We can't express this in the "expanded" form, because 
we wouldn't know which union member to initialize.

So for the non-copy/move case, we build `CXXCtorInitializers` for expanded 
members, and in the copy/move case, we build `CXXCtorInitializers` calling the 
copy/move constructors for the anonymous structs/unions themselves.

[*]: It's not entirely true that the default constructor is never used for 
anything. When we look up special members for the anonymous struct / union 
(looking for the copy or move constructor) we can trigger the implicit 
declaration of the default constructor. And it's actually even possible to 
*call* that default constructor, if you're tricksy enough: 
https://godbolt.org/z/Tq56bz

In D87808#2282223 , @rnk wrote:

> Maybe the issue is that this code is running into the lazy implicit special 
> member declaration optimization. Maybe the class in question has an implicit, 
> trivial, default constructor, but we there is no CXXConstructorDecl present 
> in the ctors list for the loop to find.

That seems very likely. The existing check:

  if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
return false;

is skating on thin ice in this regard: a class with an implicit default 
constructor might or might not have had that default constructor implicitly 
declared. But I think this code should give the same outcome either way, 
because a class with any constructor other than a default/copy/move constructor 
must have a user-declared constructor of some kind, and so will never have an 
implicit default constructor. (Inherited constructors introduce some 
complications here, but we don't do lazy constructor declaration for classes 
with inherited constructors, so I think that's OK too.)




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300
+  bool hasCtor = false;
+  for (const auto *Ctor : RD->ctors()) {
 if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
   return false;
+if (!Ctor->isCopyOrMoveConstructor())
+  hasCtor = true;
+  }

This looks pretty similar to:

```
return RD->hasUserDeclaredConstructor() && !RD->hasTrivialDefaultConstructor();
```

(other than its treatment of user-declared copy or move constructors), but I 
have to admit I don't really understand what the "at least one constructor and 
no trivial or constexpr constructors" rule aims to achieve, so it's hard to 
know if this is the right interpretation. The rule as written in the comment 
above is presumably not exactly right -- all classes have at least one 
constructor, and we're not excluding classes with trivial copy or move 
constructors, only those with trivial default constructors.

I wonder if the intent would be better captured by "at least one non-inline 
constructor" (that is, assuming all declared functions are defined, there is at 
least one translation unit in which a constructor is defined that can be used 
as a home for the class info).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87808

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

[PATCH] D87080: [AST] Reduce the size of TemplateArgumentLocInfo.

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

rsmith added inline comments.



Comment at: clang/include/clang/AST/TemplateBase.h:421
 
-public:
-  constexpr TemplateArgumentLocInfo() : Template({nullptr, nullptr, 0, 0}) {}
-
-  TemplateArgumentLocInfo(TypeSourceInfo *TInfo) : Declarator(TInfo) {}
+  T *getTemplate() const {
+assert(Pointer.is());

If you're going to add more uses of the name `T`, we should rename it to 
something more descriptive. Maybe `TemplateTemplateArgLocInfo` or something 
like that?



Comment at: clang/include/clang/AST/TemplateBase.h:435-441
+T *Template = new (Ctx) T;
+Template->Qualifier = QualifierLoc.getNestedNameSpecifier();
+Template->QualifierLocData = QualifierLoc.getOpaqueData();
+Template->TemplateNameLoc = TemplateNameLoc.getRawEncoding();
+Template->EllipsisLoc = EllipsisLoc.getRawEncoding();
+Pointer = Template;
   }

I think this is just about complex enough to be worth moving to the .cpp file.



Comment at: clang/include/clang/AST/TemplateBase.h:444
   TypeSourceInfo *getAsTypeSourceInfo() const {
-return Declarator;
+assert(Pointer.is());
+return Pointer.get();

This is redundant (here and below): `get` does this assert for you.



Comment at: clang/include/clang/AST/TemplateBase.h:43
+// the dependency.
+template <> struct PointerLikeTypeTraits {
+  static inline void *getAsVoidPointer(clang::Expr *P) { return P; }

hokein wrote:
> sammccall wrote:
> > At first glance this is unsafe: you could have two different definitions of 
> > the same specialization in different files.
> > 
> > In fact it's OK, the default one can now never be instantiated, because 
> > Expr.h includes this file and so anyone that can see the definition can see 
> > this specialization.
> > 
> > But this is subtle/fragile: at least it needs to be spelled out explicitly 
> > in the comment.
> > I'd also suggest adding a static assert below the definition of Expr that 
> > it is compatible with this specialization (because it is sufficiently 
> > aligned).
> > 
> > (I can't think of a better alternative - use of PointerUnion is a win, 
> > partly *because* it validates the alignment)
> yes, exactly. 
> 
> do you have a better idea on how the static_assert should look like? The way 
> I can think of is to add a new flag in this specialization, and use 
> `static_assert(PointerLikeTypeTraits::flag, "...")`.
This may be included from `Expr.h` right now, but really doesn't seem like the 
right header to hold this specialization. Perhaps we should consider adding 
something like an `AST/ForwardDecls.h`, containing forward declarations and 
specializations such as this one, and include that from `Expr.h` and from here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87080

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

[PATCH] D87561: [Sema] List conversion validate character array

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

rsmith added inline comments.



Comment at: clang/test/CXX/drs/dr14xx.cpp:411-414
+  void f(const char[4]);
+  void f(const wchar_t[4]);
+  void f(const char16_t[4]);
+  void f(const char32_t[4]);

rsmith wrote:
> Mordante wrote:
> > rsmith wrote:
> > > These should presumably be references to arrays, rather than arrays, or 
> > > the parameter type is as if you wrote (for example) `void f(const char 
> > > *)`, which shouldn't get the special treatment here.
> > > 
> > > [over.ics.list]p4 mentions this in its footnote:
> > > 
> > > "Otherwise, if the parameter type is a character array [Footnote: Since 
> > > there are no parameters of array type, this will only occur as the 
> > > referenced type of a reference parameter.] and the initializer list has a 
> > > single element that is an appropriately-typed string-literal (9.4.3), the 
> > > implicit conversion sequence is the identity conversion."
> > Ah I missed that footnote. I reread the standard and can you confirm some 
> > cases?
> > ```
> > namespace A { 
> >   void f(const char(&)[4]);
> >   void g() { f({"abc"}); }
> > }
> > namespace B { 
> >   void f(const char(&&)[4]);
> >   void g() { f({"abc"}); }
> > } 
> > ```
> > both should work and with P0388 the array without bounds will also work.
> > 
> > I ask since this is my interpretation of the standard, but it seems there's 
> > a difference between implementations and `void f(const char(&&)[4]);` fails 
> > for "abc" but works for "ab".
> > It seems ICC and consider "abc" an lvalue in this case and not when using 
> > "ab".
> > 
> > Here's a gotbolt link with the examples https://godbolt.org/z/r1TKfx
> That's a really interesting example :)
> 
> The initializer is a list containing an lvalue of type `const char[4]`. Per 
> [dcl.init.list]/3.9 and /3.10, the behavior depends on whether the referenced 
> type is reference-related to `const char[4]` -- if so, then the reference can 
> only bind directly and a `&&` reference will be invalid, because it's binding 
> an rvalue reference to an lvalue, and if not, then we create an array 
> temporary and the `&&` binding is fine.
> 
> Per [dcl.init.ref]/4, `const char[???]` is reference-related to `const 
> char[4]` if they are similar types, and per [conv.qual]/2, the types are 
> similar if `???` is omitted or `4`, and not similar otherwise.
> 
> So:
> * `const char (&)[4] = {"abc"}` is ill-formed (types are the same, binds 
> rvalue reference to lvalue)
> * `const char (&&)[] = {"abc"}` is ill-formed (types are similar, binds 
> rvalue reference to lvalue)
> * `const char (&)[5] = {"abc"}` is OK (types are not similar, creates 
> temporary)
> 
> That seems like a very surprising outcome to me. Perhaps we should probably 
> ignore array bounds entirely when determining whether two types are 
> reference-related. I'll take this to CWG for discussion.
I think that's only an answer to half of your question. The other half is that 
[over.ics.list]p4 does not apply (directly) to either of your testcases, 
because the "parameter type" is a reference type, not an array type. For:

```
namespace A { 
  void f(const char(&)[4]);
  void g() { f({"abc"}); }
}
```

... we reach [over.ics.list]p9, which says to use the rules in [over.ics.ref]. 
Those rules say that if the reference binds directly to an argument expression 
(ignore the "expression" here; this is very old wording that predates braced 
initializers), then we form an identity conversion. So that's what happens in 
this case.

For

```
namespace B { 
  void f(const char(&&)[4]);
  void g() { f({"abc"}); }
}
```

the same thing happens, but now [over.ics.ref]p3 says "an implicit conversion 
sequence cannot be formed if it requires binding an lvalue reference other than 
a reference to a non-volatile const type to an rvalue or binding an rvalue 
reference to an lvalue other than a function lvalue", so the candidate is not 
viable.

If the array bound were omitted or were not 4, then the reference would not 
bind directly, and we would instead consider initializing a temporary. 
[over.ics.ref]p2 says "When a parameter of reference type is not bound directly 
to an argument expression, the conversion sequence is the one required to 
convert the argument expression to the referenced type according to 12.4.4.2.", 
which takes us back around to [over.ics.list] with the "parameter type" now 
being the array type. That's how we can reach [over.ics.list]p4 and consider 
string literal -> array initialization.

So I think that, according to the current rules, for

```
void f(const char (&&)[4]); // #1
void f(const char (&&)[5]); // #2
```

... a call to `f({"abc"})` remarkably calls #2, because #1 is not viable (would 
bind rvalue reference to lvalue string literal), but #2 is, just like `const 
char (&)[4] = {"abc"};` is ill-formed but `const char (&)[5] = {"abc"};` is 
valid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D87561: [Sema] List conversion validate character array

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

rsmith added inline comments.



Comment at: clang/test/CXX/drs/dr14xx.cpp:411-414
+  void f(const char[4]);
+  void f(const wchar_t[4]);
+  void f(const char16_t[4]);
+  void f(const char32_t[4]);

Mordante wrote:
> rsmith wrote:
> > These should presumably be references to arrays, rather than arrays, or the 
> > parameter type is as if you wrote (for example) `void f(const char *)`, 
> > which shouldn't get the special treatment here.
> > 
> > [over.ics.list]p4 mentions this in its footnote:
> > 
> > "Otherwise, if the parameter type is a character array [Footnote: Since 
> > there are no parameters of array type, this will only occur as the 
> > referenced type of a reference parameter.] and the initializer list has a 
> > single element that is an appropriately-typed string-literal (9.4.3), the 
> > implicit conversion sequence is the identity conversion."
> Ah I missed that footnote. I reread the standard and can you confirm some 
> cases?
> ```
> namespace A { 
>   void f(const char(&)[4]);
>   void g() { f({"abc"}); }
> }
> namespace B { 
>   void f(const char(&&)[4]);
>   void g() { f({"abc"}); }
> } 
> ```
> both should work and with P0388 the array without bounds will also work.
> 
> I ask since this is my interpretation of the standard, but it seems there's a 
> difference between implementations and `void f(const char(&&)[4]);` fails for 
> "abc" but works for "ab".
> It seems ICC and consider "abc" an lvalue in this case and not when using 
> "ab".
> 
> Here's a gotbolt link with the examples https://godbolt.org/z/r1TKfx
That's a really interesting example :)

The initializer is a list containing an lvalue of type `const char[4]`. Per 
[dcl.init.list]/3.9 and /3.10, the behavior depends on whether the referenced 
type is reference-related to `const char[4]` -- if so, then the reference can 
only bind directly and a `&&` reference will be invalid, because it's binding 
an rvalue reference to an lvalue, and if not, then we create an array temporary 
and the `&&` binding is fine.

Per [dcl.init.ref]/4, `const char[???]` is reference-related to `const char[4]` 
if they are similar types, and per [conv.qual]/2, the types are similar if 
`???` is omitted or `4`, and not similar otherwise.

So:
* `const char (&)[4] = {"abc"}` is ill-formed (types are the same, binds 
rvalue reference to lvalue)
* `const char (&&)[] = {"abc"}` is ill-formed (types are similar, binds rvalue 
reference to lvalue)
* `const char (&)[5] = {"abc"}` is OK (types are not similar, creates 
temporary)

That seems like a very surprising outcome to me. Perhaps we should probably 
ignore array bounds entirely when determining whether two types are 
reference-related. I'll take this to CWG for discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87561

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

[PATCH] D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers

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

rsmith added inline comments.



Comment at: clang/include/clang/AST/Type.h:4478
 
-  DecltypeType(Expr *E, QualType underlyingType, QualType can = QualType());
+  const ASTContext 
+  DecltypeType(Expr *E, QualType underlyingType, const ASTContext ,

We shouldn't store the context here. To provide it to the `Profile` 
implementation, use a `ContextualFoldingSet`.



Comment at: clang/include/clang/AST/Type.h:4499-4508
 /// Internal representation of canonical, dependent
 /// decltype(expr) types.
 ///
 /// This class is used internally by the ASTContext to manage
 /// canonical, dependent types, only. Clients will only see instances
 /// of this class via DecltypeType nodes.
+class DependentDecltypeType : public DecltypeType {

Can we remove this class now?



Comment at: clang/lib/AST/ASTContext.cpp:5342-5347
 /// Unlike many "get" functions, we don't unique DecltypeType
 /// nodes. This would never be helpful, since each such type has its own
 /// expression, and would not give a significant memory saving, since there
 /// is an Expr tree under each such type.
+/// The instantiation-dependent-but-not-type-dependent DecltypeType node is an
+/// exception, we unique it for forming correct substitutions in name mangling.

Maybe fold these two paragraphs together

> Unlike many "get" functions, we don't unique DecltypeType nodes unless 
> they are instantiation-dependent. [...] 



Comment at: clang/test/CodeGenCXX/mangle.cpp:805
 
-  // CHECK-LABEL: define weak_odr void 
@_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f(decltype(sizeof(1)));

hokein wrote:
> rsmith wrote:
> > rsmith wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > GCC mangles this as `_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE`, so 
> > > > > we're breaking compat here :-\
> > > > > 
> > > > > And in fact we're just incorrect. 
> > > > > https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:
> > > > > 
> > > > > > If the operand expression of decltype is not 
> > > > > > instantiation-dependent then the resulting type is encoded directly.
> > > > > 
> > > > > Here, instantiation-dependent *is* explicitly defined as 
> > > > > "type-dependent or value-dependent or ...". And therefore these cases 
> > > > > must not be "encoded directly", including the motivating case 
> > > > > (decltype(N) where N is a typed constant whose initializer is 
> > > > > value-dependent).
> > > > > 
> > > > > We could consider not two but *three* types of decltypetypes:
> > > > >  - decltype(type-dependent), which is sugar for a canonical 
> > > > > DependentDeclTypeType
> > > > >  - decltype(instantiation-but-not-type-dependent), which is still 
> > > > > sugar for a concrete canonical type (per C++17) but mangles using the 
> > > > > expr (per cxxabi)
> > > > >  - decltype(non-dependent), which is sugar for a concrete canonical 
> > > > > type and mangles directly
> > > > > 
> > > > > This only works if it's OK for mangling to depend on non-canonical 
> > > > > type details - I don't know whether this is the case. @rsmith - any 
> > > > > hints here?
> > > > Hmm, my naive reading of the mangle code matches what I described.
> > > > 
> > > > The big comment in ItaniumMangle talks about related issues: 
> > > > https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572
> > > > 
> > > > I don't really understand what's going on here, sorry.
> > > Looks like we need the single-step-desugaring loop below the big comment 
> > > you quoted to stop when it hits a `decltype` type. That's presumably 
> > > where we step from the instantiation-dependent-but-not-type-dependent 
> > > `decltype` node to its desugared type.
> > Also... the FIXME from that comment will apply here too. Given:
> > ```
> > template void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
> > template void f(unsigned long, unsigned long);
> > ```
> > we currently (correctly, as far as I can see) use a substitution for the 
> > second parameter type, but with the change here, I don't think we will do 
> > so any more. We could fix that by deduplicating `DecltypeType`s when 
> > they're instantiation dependent but not dependent; that's what we do for 
> > `ConstantArrayType`'s size expression, for example. If we do that, we'll 
> > need to store the actual expression on the `DecltypeTypeLoc`, since we 
> > can't rely on the `DecltypeType` having the right expression any more (only 
> > an expression that's equivalent to it).
> Thanks for the hints and explanations.
> 
> > we currently (correctly, as far as I can see) use a substitution for the 
> > second parameter type, but with the change here, I don't think we will do 
> > so any more.
> 
> thanks for the example, yeah, the behavior was changed with the prior change 

[PATCH] D68364: Implement C++20's P0784 (More constexpr containers)

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

rsmith added inline comments.



Comment at: libcxx/include/version:254
 // # define __cpp_lib_concepts 201806L
+# define __cpp_lib_constexpr_dynamic_alloc  201907L
 // # define __cpp_lib_constexpr_misc   201811L

ldionne wrote:
> rsmith wrote:
> > ldionne wrote:
> > > rsmith wrote:
> > > > Should this be conditioned on compiler support being available?
> > > So.. I've decided not to do that in this patch so far.
> > > 
> > > The support for constexpr allocation was checked into Clang about a year 
> > > ago, right? I actually expect this to be a slightly contentious point, 
> > > but I'd like to assume that we're using a reasonably recent Clang. I 
> > > don't see a strong point for being able to use new libc++ headers with an 
> > > old Clang anyway, since vendors usually release the two together. IOW, 
> > > supporting this would add complexity for virtually no benefit. I do agree 
> > > it's a slightly more aggressive stance than we've had so far, but this 
> > > sort of reasonable assumption makes it so much easier to write stuff for 
> > > libc++.
> > OK, just a few thoughts then I'm going to bow out of this; this seems like 
> > a policy decision for the libc++ maintainers to make.
> > 
> > In favor of dropping support for new libc++ + old clang: we generally don't 
> > permit version skew between different components of LLVM. It seems 
> > reasonable to expect all wanted parts of a particular LLVM release to be 
> > built together.
> > 
> > Against dropping support for new libc++ + old clang: we do support 
> > installing more than one version of LLVM (and in particular more than one 
> > version of Clang) on the same system, but because libc++ defaults to being 
> > installed in `/usr/include/c++/v1`, we don't seem to encourage installing 
> > more than one version of libc++, so -- even assuming we only support the 
> > *newest* version of libc++ going into `/usr/include/c++/v1` -- new versions 
> > of libc++ need to work with old versions of Clang.
> > 
> > I think (largely by accident) Clang will prefer a libc++ installed into 
> > `/usr/lib/clang/$VER/include` over one from `/usr/include/c++/v1`. If we 
> > switched to installing libc++ there, I don't see any technical barrier to 
> > version-locking them, though I'm not sure what story that leaves for use of 
> > libc++ with GCC and other compilers. It seems worth noting that this is 
> > exactly what libstdc++ does in order to need to support only one version of 
> > GCC.
> > OK, just a few thoughts then I'm going to bow out of this; this seems like 
> > a policy decision for the libc++ maintainers to make.
> 
> Hear hear!
> 
> 
> > I think (largely by accident) Clang will prefer a libc++ installed into 
> > `/usr/lib/clang/$VER/include` over one from `/usr/include/c++/v1`. If we 
> > switched to installing libc++ there, I don't see any technical barrier to 
> > version-locking them, though I'm not sure what story that leaves for use of 
> > libc++ with GCC and other compilers. It seems worth noting that this is 
> > exactly what libstdc++ does in order to need to support only one version of 
> > GCC.
> 
> I think it would be great to do that. Honestly, this is a huge simplification 
> and makes implementing new features much easier. Also, I think it's 
> reasonable to support not-trunk compilers, like 6 months old or something 
> like that. But not several years back.
> 
> One last question: do you know what controls where the libc++ headers are 
> installed as part of the LLVM distribution?
Install paths are set in `libcxx/include/CMakeLists.txt` for the headers and in 
`libcxx/src/CMakeLists.txt` for the libraries (search for `install(`) based on 
cmake variables LIBCXX_INSTALL_HEADER_PREFIX, LIBCXX_INSTALL_PREFIX, and 
LIBCXX_INSTALL_LIBRARY_DIR.

It would probably make sense to put the libc++ headers into somewhere like 
`/usr/lib/clang/$VER/include/c++` instead of directly in 
`/usr/lib/clang/$VER/include`; that'll need some changes to the clang driver to 
make sure we look there. But of course that's doable if the two are 
version-locked :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68364

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

[PATCH] D87853: [Sema] Update specialization iterator after template argument deduction.

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

rsmith added a comment.

Thanks, this is pretty subtle. Here's a small C++20 testcase that invalidates 
`InsertPos` while matching partial specializations:

  template constexpr bool v = true;
  template requires v constexpr bool v = true;
  template<> constexpr bool v = true;
  int k = v;




Comment at: clang/lib/Sema/SemaTemplate.cpp:4485
   Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
   Converted, TemplateNameLoc, InsertPos /*, LateAttrs, StartingScope*/);
   if (!Decl)

All the handling of `InsertPos` in `BuildVarTemplateInstantiation` and below 
looks wrong to me, with the same bug that this code had. We end up in 
`VisitVarTemplateSpecializationDecl` which does this:

```
  // Do substitution on the type of the declaration
  TypeSourceInfo *DI =
  SemaRef.SubstType(D->getTypeSourceInfo(), TemplateArgs,
D->getTypeSpecStartLoc(), D->getDeclName());
  if (!DI)
return nullptr;

  if (DI->getType()->isFunctionType()) {
SemaRef.Diag(D->getLocation(), diag::err_variable_instantiates_to_function)
<< D->isStaticDataMember() << DI->getType();
return nullptr;
  }

  // Build the instantiated declaration
  VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
  SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
  VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
  Var->setTemplateArgsInfo(TemplateArgsInfo);
  if (InsertPos)
VarTemplate->AddSpecialization(Var, InsertPos);
```

But the call to `SubstType` can absolutely invalidate `InsertPos`; here's a 
test case for a crash caused by that:

```
template T v;
template decltype(v) v;
template<> int v;
int k = v;
```

So I think we should remove the `InsertPos` parameter from this entire cluster 
of functions. No use of it is correct.

With `InsertPos` removed, we still need to know whether to call 
`AddSpecialization` or not. We should in principle do that if and only if 
`!PrevDecl`, but the call to `VisitVarTemplateSpecializationDecl` in 
`InstantiateVariableDefinition` doesn't pass in a `PrevDecl` even though it has 
one (`OldVar`). I wonder if we can change `InstantiateVariableDefinition` to 
pass `OldVar` in as the `PrevDecl` and remove its call to `MergeVarDecl` and 
probably also its call to `InstantiateVariableInitializer` -- since 
`BuildVariableInstantiation` should then do all of that for us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87853

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

[PATCH] D77598: Integral template argument suffix and cast printing

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

rsmith added inline comments.



Comment at: clang/lib/AST/TemplateBase.cpp:71-72
 
   if (T->isBooleanType() && !Policy.MSVCFormatting) {
 Out << (Val.getBoolValue() ? "true" : "false");
   } else if (T->isCharType()) {

reikdas wrote:
> rsmith wrote:
> > rsmith wrote:
> > > It looks like `MSVCFormatting` wants `bool` values to be printed as `0` 
> > > and `1`, and this patch presumably changes that (along with the printing 
> > > of other builtin types). I wonder if this is a problem in practice (eg, 
> > > if such things are used as keys for debug info or similar)...
> > Do we need to suppress printing the suffixes below in `MSVCFormatting` mode 
> > too?
> @rsmith The tests pass, so that is reassuring at least. Is there any other 
> way to find out whether we need to suppress printing the suffixes for other 
> types in MSVCFormatting mode?
@rnk Can you take a look at this? Per 
rG60103383f097b6580ecb4519eeb87defdb7c05c9 and PR21528 it seems like there 
might be specific requirements for how template arguments are formatted for 
CodeView support; we presumably need to make sure we still satisfy those 
requirements.



Comment at: clang/lib/Sema/SemaTemplate.cpp:6825-6831
+// Note: this renders CanonParamType non-canonical, but since every
+// instantiation of this argument will be wrapped in an AutoType (since
+// Param->getType() will always be an AutoType for this template), there
+// should be no difference in how arguments are distinguished.
+if (Param->getType()->getAs())
+  CanonParamType = Context.getAutoType(CanonParamType,
+   AutoTypeKeyword::Auto, false, 
false);

I'm uncomfortable with this approach. It's at least theoretically important 
that we canonicalize the types of template arguments so that template 
instantiations don't depend on how their arguments were spelled. Also, I don't 
think this addresses the whole problem: whether we need the type for a template 
argument depends not only on whether there was a deduced type in the parameter, 
but also whether the template was selected from an overload set. That's 
information that whoever is asking to print the template argument should know, 
but the template argument itself doesn't know in general.

So I'd like us to approach this a different way: change the calls into the 
template argument printing code to explicitly pass in a flag that says whether 
the type of the template argument is known from context. We should pass that in 
as `false` when:
1) dumping template arguments
2) the corresponding parameter contains a deduced type
3) the template arguments are for a `DeclRefExpr` that `hadMultipleCandidates()`


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

https://reviews.llvm.org/D77598

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

[PATCH] D68364: Implement C++20's P0784 (More constexpr containers)

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

rsmith added inline comments.



Comment at: libcxx/include/version:254
 // # define __cpp_lib_concepts 201806L
+# define __cpp_lib_constexpr_dynamic_alloc  201907L
 // # define __cpp_lib_constexpr_misc   201811L

ldionne wrote:
> rsmith wrote:
> > Should this be conditioned on compiler support being available?
> So.. I've decided not to do that in this patch so far.
> 
> The support for constexpr allocation was checked into Clang about a year ago, 
> right? I actually expect this to be a slightly contentious point, but I'd 
> like to assume that we're using a reasonably recent Clang. I don't see a 
> strong point for being able to use new libc++ headers with an old Clang 
> anyway, since vendors usually release the two together. IOW, supporting this 
> would add complexity for virtually no benefit. I do agree it's a slightly 
> more aggressive stance than we've had so far, but this sort of reasonable 
> assumption makes it so much easier to write stuff for libc++.
OK, just a few thoughts then I'm going to bow out of this; this seems like a 
policy decision for the libc++ maintainers to make.

In favor of dropping support for new libc++ + old clang: we generally don't 
permit version skew between different components of LLVM. It seems reasonable 
to expect all wanted parts of a particular LLVM release to be built together.

Against dropping support for new libc++ + old clang: we do support installing 
more than one version of LLVM (and in particular more than one version of 
Clang) on the same system, but because libc++ defaults to being installed in 
`/usr/include/c++/v1`, we don't seem to encourage installing more than one 
version of libc++, so -- even assuming we only support the *newest* version of 
libc++ going into `/usr/include/c++/v1` -- new versions of libc++ need to work 
with old versions of Clang.

I think (largely by accident) Clang will prefer a libc++ installed into 
`/usr/lib/clang/$VER/include` over one from `/usr/include/c++/v1`. If we 
switched to installing libc++ there, I don't see any technical barrier to 
version-locking them, though I'm not sure what story that leaves for use of 
libc++ with GCC and other compilers. It seems worth noting that this is exactly 
what libstdc++ does in order to need to support only one version of GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68364

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2280096 , @jrtc27 wrote:

> If someone cares about pthread_create they might wish to follow up on my 
> https://reviews.llvm.org/D58531, which I filed early last year to permit 
> pthread_create to have a proper type in the syntax. It will likely need 
> rebasing, but probably isn't that much work.

Sorry I missed this before! Please feel free to ping patches every ~week if 
they're not getting attention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

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

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

rsmith added inline comments.



Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1223
 /// The number of special type IDs.
 const unsigned NumSpecialTypeIDs = 8;
 

This presumably needs to be increased. Are we missing test coverage that would 
have caught this?



Comment at: clang/lib/AST/ASTContext.cpp:9386-9389
+static QualType DecodeFunctionTypeFromStr(
+const char *, bool IsNested, bool IsNoReturn, bool IsNoThrow,
+bool ForceEmptyTy, const ASTContext ,
+ASTContext::GetBuiltinTypeError , unsigned *IntegerConstantArgs);

Our convention for such helper functions is to pass the reference to the 
associated class (`ASTContext` in this case) as the first parameter. (I 
appreciate that `DecodeTypeFromStr` violates this convention.)



Comment at: clang/lib/AST/ASTContext.cpp:9677-9679
+Type = DecodeFunctionTypeFromStr(
+Str, /*IsNested=*/true, /*IsNoReturn=*/false,
+/*IsNoThrow=*/false, /*ForceEmptyTy=*/false, Context, Error, nullptr);

OK. We may eventually want a way to specify noreturn and nothrow here, because 
they're both part of the type in at least some cases, but this seems fine for 
the immediate future.



Comment at: clang/lib/AST/ASTContext.cpp:9739
+  if (TypeStr[0] == ')') {
+assert(IsNested && "unmatched ')' found at end of type list");
+Error = ASTContext::GE_Missing_type;

Maybe "missing return type in builtin function type" or similar would be a more 
useful assert message here?



Comment at: clang/lib/AST/ASTContext.cpp:9756
+  while (TypeStr[0] && TypeStr[0] != '.' && TypeStr[0] != ')') {
+QualType Ty = DecodeTypeFromStr(TypeStr, Context, Error, RequiresICE, 
true);
+if (Error != ASTContext::GE_None)

Please `assert(!RequiresICE || !IsNested)` here; we shouldn't require 
parameters of function pointer parameters to be ICEs.



Comment at: clang/lib/AST/ASTContext.cpp:9765-9767
 // Do array -> pointer decay.  The builtin should use the decayed type.
 if (Ty->isArrayType())
+  Ty = Context.getArrayDecayedType(Ty);

Should we do function -> pointer decay here too, so you can use `(...)` instead 
of `(...)*` in `Builtins.def`?



Comment at: clang/lib/AST/ASTContext.cpp:9775-9776
+
+  if (ForceEmptyTy)
 return {};
 

Do we need this special case and its associated flag? Can we make 
`GetBuiltinType` directly return `QualType();`for `_GetExceptionInfo` instead?



Comment at: clang/lib/AST/ASTContext.cpp:9778-9779
 
   assert((TypeStr[0] != '.' || TypeStr[1] == 0) &&
  "'.' should only occur at end of builtin type list!");
 

This will assert on `(.)`.



Comment at: clang/lib/Sema/SemaDecl.cpp:1971-1972
 return "ucontext.h";
+  case ASTContext::GE_Missing_pthread:
+return "pthread.h";
   }

Could we use `GE_Missing_type` here instead? The `LIBBUILTIN` already lists 
"pthread.h" as the corresponding header, so that seems sufficient. I think we 
only need the special cases above to handle the non-`LIBBUILTIN` cases such as 
`__builtin_fprintf` that nonetheless require a header inclusion (in that case 
to provide `FILE`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58531

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

[PATCH] D68364: Implement C++20's P0784 (More constexpr containers)

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

rsmith added a comment.

The mechanism by which this interacts with Clang looks good to me. I've not 
done any detailed analysis to check all the functions made `constexpr` by P0784 
are handled by this patch, though.




Comment at: libcxx/include/version:254
 // # define __cpp_lib_concepts 201806L
+# define __cpp_lib_constexpr_dynamic_alloc  201907L
 // # define __cpp_lib_constexpr_misc   201811L

Should this be conditioned on compiler support being available?



Comment at: 
libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.members/allocate_hint.pass.cpp:87
+#if TEST_STD_VER > 17
+static_assert(test());
+#endif

I really like this approach to testing this change :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68364

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:2683
   }
+  if (const BinaryOperator *BE = dyn_cast(E)) {
+if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {

`E` here is only intended for use in determining diagnostic locations, and 
isn't intended to necessarily be the expression being evaluated. Instead of 
assuming that this is the right expression to inspect, please either query the 
FP features in the caller and pass them into here or change this function to 
take a `const BinaryOperator*`.



Comment at: clang/lib/AST/ExprConstant.cpp:2685
+if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+  Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+  return false;

This should be an `FFDiag` not a `CCEDiag` because we want to suppress all 
constant folding, not only treating the value as a core constant expression. 
Also we should check this before checking for a NaN result, because if both 
happen at once, this is the diagnostic we want to produce.



Comment at: clang/lib/AST/ExprConstant.cpp:13283-13286
+if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+  Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+  return false;
+}

Hm, does `fabs` depend on the floating-point environment? Is the concern the 
interaction with signaling NaNs?



Comment at: clang/lib/AST/ExprConstant.cpp:13372-13376
+  if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+// In C lang ref, footnote, cast may raise inexact exception.
+Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+return false;
+  }

Is it feasible to only reject cases that would actually be inexact, rather than 
disallowing all conversions to floating-point types? It would seem preferable 
to allow at least widening FP conversions and complex -> real, since they never 
depend on the FP environment (right?).



Comment at: clang/test/CodeGen/pragma-fenv_access.c:1
+// xxx: %clang_cc1 -ffp-exception-behavior=strict -frounding-math -triple 
%itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffp-exception-behavior=strict -triple %itanium_abi_triple 
-emit-llvm %s -o - | FileCheck %s

Is this RUN line intentionally disabled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528

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

[PATCH] D87561: [Sema] List conversion validate character array

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:4989
+if (ToType->isArrayType() && ToType->isCharType() &&
+isa(From->getInit(0))) {
   InitializedEntity Entity =

Mordante wrote:
> rsmith wrote:
> > This is too narrow a check in two ways: we should allow parenthesized 
> > string literals here, and we should allow `ObjCEncodeExpr`.
> Actually it seems the code isn't behaving properly at all. It seems the 
> conversion is done by
> https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaOverload.cpp#L5174
> resulting in an array-to-pointer conversion instead of an identity conversion.
> 
> It can solve it by manually removing the decay like:
> ```
> if (const auto *DT = dyn_cast(ToType))
>   if (const auto *AT = S.Context.getAsArrayType(DT->getOriginalType()))
> if (S.IsStringInit(From->getInit(0), AT) {
>...
> ```
> This code works and results in an identity conversion. But it feels a bit odd 
> to manually peel away the array-to-pointer decay. Is this the best solution 
> or do you have a better suggestions?
>   
> 
I think this is a bug in your testcases. I'll comment below.



Comment at: clang/test/CXX/drs/dr14xx.cpp:411-414
+  void f(const char[4]);
+  void f(const wchar_t[4]);
+  void f(const char16_t[4]);
+  void f(const char32_t[4]);

These should presumably be references to arrays, rather than arrays, or the 
parameter type is as if you wrote (for example) `void f(const char *)`, which 
shouldn't get the special treatment here.

[over.ics.list]p4 mentions this in its footnote:

"Otherwise, if the parameter type is a character array [Footnote: Since there 
are no parameters of array type, this will only occur as the referenced type of 
a reference parameter.] and the initializer list has a single element that is 
an appropriately-typed string-literal (9.4.3), the implicit conversion sequence 
is the identity conversion."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87561

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

[PATCH] D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers

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

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/mangle.cpp:805
 
-  // CHECK-LABEL: define weak_odr void 
@_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f(decltype(sizeof(1)));

rsmith wrote:
> sammccall wrote:
> > sammccall wrote:
> > > GCC mangles this as `_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE`, so we're 
> > > breaking compat here :-\
> > > 
> > > And in fact we're just incorrect. 
> > > https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:
> > > 
> > > > If the operand expression of decltype is not instantiation-dependent 
> > > > then the resulting type is encoded directly.
> > > 
> > > Here, instantiation-dependent *is* explicitly defined as "type-dependent 
> > > or value-dependent or ...". And therefore these cases must not be 
> > > "encoded directly", including the motivating case (decltype(N) where N is 
> > > a typed constant whose initializer is value-dependent).
> > > 
> > > We could consider not two but *three* types of decltypetypes:
> > >  - decltype(type-dependent), which is sugar for a canonical 
> > > DependentDeclTypeType
> > >  - decltype(instantiation-but-not-type-dependent), which is still sugar 
> > > for a concrete canonical type (per C++17) but mangles using the expr (per 
> > > cxxabi)
> > >  - decltype(non-dependent), which is sugar for a concrete canonical type 
> > > and mangles directly
> > > 
> > > This only works if it's OK for mangling to depend on non-canonical type 
> > > details - I don't know whether this is the case. @rsmith - any hints here?
> > Hmm, my naive reading of the mangle code matches what I described.
> > 
> > The big comment in ItaniumMangle talks about related issues: 
> > https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572
> > 
> > I don't really understand what's going on here, sorry.
> Looks like we need the single-step-desugaring loop below the big comment you 
> quoted to stop when it hits a `decltype` type. That's presumably where we 
> step from the instantiation-dependent-but-not-type-dependent `decltype` node 
> to its desugared type.
Also... the FIXME from that comment will apply here too. Given:
```
template void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
template void f(unsigned long, unsigned long);
```
we currently (correctly, as far as I can see) use a substitution for the second 
parameter type, but with the change here, I don't think we will do so any more. 
We could fix that by deduplicating `DecltypeType`s when they're instantiation 
dependent but not dependent; that's what we do for `ConstantArrayType`'s size 
expression, for example. If we do that, we'll need to store the actual 
expression on the `DecltypeTypeLoc`, since we can't rely on the `DecltypeType` 
having the right expression any more (only an expression that's equivalent to 
it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87349

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

[PATCH] D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers

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

rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/mangle-exprs.cpp:218
   template void a(decltype(noexcept(int(;
-  // CHECK: void @_ZN5test51aIiEEvDTnxcvT__EE(
+  // CHECK: void @_ZN5test51aIiEEvb
 }

As with the other case, the expression here is instantiation-dependent so 
should be mangled.



Comment at: clang/test/CodeGenCXX/mangle.cpp:805
 
-  // CHECK-LABEL: define weak_odr void 
@_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f(decltype(sizeof(1)));

sammccall wrote:
> sammccall wrote:
> > GCC mangles this as `_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE`, so we're 
> > breaking compat here :-\
> > 
> > And in fact we're just incorrect. 
> > https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:
> > 
> > > If the operand expression of decltype is not instantiation-dependent then 
> > > the resulting type is encoded directly.
> > 
> > Here, instantiation-dependent *is* explicitly defined as "type-dependent or 
> > value-dependent or ...". And therefore these cases must not be "encoded 
> > directly", including the motivating case (decltype(N) where N is a typed 
> > constant whose initializer is value-dependent).
> > 
> > We could consider not two but *three* types of decltypetypes:
> >  - decltype(type-dependent), which is sugar for a canonical 
> > DependentDeclTypeType
> >  - decltype(instantiation-but-not-type-dependent), which is still sugar for 
> > a concrete canonical type (per C++17) but mangles using the expr (per 
> > cxxabi)
> >  - decltype(non-dependent), which is sugar for a concrete canonical type 
> > and mangles directly
> > 
> > This only works if it's OK for mangling to depend on non-canonical type 
> > details - I don't know whether this is the case. @rsmith - any hints here?
> Hmm, my naive reading of the mangle code matches what I described.
> 
> The big comment in ItaniumMangle talks about related issues: 
> https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572
> 
> I don't really understand what's going on here, sorry.
Looks like we need the single-step-desugaring loop below the big comment you 
quoted to stop when it hits a `decltype` type. That's presumably where we step 
from the instantiation-dependent-but-not-type-dependent `decltype` node to its 
desugared type.



Comment at: clang/test/SemaTemplate/dependent-expr.cpp:132
   }
-  template void f(); // expected-note {{instantiation of}}
+  template void f();
 

sammccall wrote:
> Took me a while to understand this, I think this is an improvement as we now 
> diagnose when we see the template rather than when we see the instantiation.
Right. We treat statement-expressions in a template as always being value- and 
instantiation-dependent, in part to match GCC and in part to avoid needing to 
define value- and instantiation-dependence for all statements and block-scope 
declarations, so decltype(statement expression) would have been a dependent 
type before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87349

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

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



Comment at: clang/lib/AST/ExprConstant.cpp:775-778
+/// Strict floating point is enabled, this inhibits
+/// floating ponit constant folding.
+bool isStrictFP;
+

This is a property of the operation being constant-evaluated, not of the 
`EvalInfo`. The way you compute it below will only pick up the right value if 
the top-level expression in the evaluation happens to be a constrained 
floating-point evaluation.



Comment at: clang/lib/AST/ExprConstant.cpp:13197-13198
   assert(E->isRValue() && E->getType()->isRealFloatingType());
+  if (Info.isStrictFP)
+return false;
   return FloatExprEvaluator(Info, Result).Visit(E);

I think we should be able to evaluate (for example) `constexpr float f = 1.0f;` 
even in a strict FP context. I think only floating point operations that depend 
on the rounding mode should be disabled, not all floating-point evaluation. 
Perhaps we should propagate the `FPOptions` into `handleFloatFloatBinOp` and 
perform the check there, along with any other places that care (I think we 
probably have some builtins that we can constant-evaluate that care about 
rounding modes.)

You also need to produce a diagnostic when you treat an expression as 
non-constant -- please read the comment at the top of the file for details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith accepted this revision.
rsmith added a comment.

Thanks! This looks good to me (subject to Aaron's comment being addressed). 
Please wait a couple of days for any more comments from the other reviewers.




Comment at: clang/lib/Sema/SemaDecl.cpp:9689
+   Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+  NewFD->addAttr(BuiltinAttr::CreateImplicit(Context, BuiltinID));
+}

Please can you add a `// FIXME` here that we should probably only recognize 
this as a builtin in the scope where the MS headers actually declare it, rather 
than in every scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

rsmith added a comment.

In D87528#2270561 , @rjmccall wrote:

> Richard, what do you think the right rule is for when to use the special 
> constant-evaluation rules for this feature in C++?  The C standard says that 
> constant expressions should use the default rules rather than considering the 
> dynamic environment, but C can get away with that because constant 
> expressions are restricted to a narrow set of syntactic situations.

Right, C has a lot of leeway here because floating-point operations aren't 
allowed in integer constant expressions in C; the only place FP operations are 
guaranteed to be evaluated during translation is in the initializers of 
variables of static storage duration, which are all notionally initialized 
before the program begins executing (and thus before a non-default FP 
environment can be entered) anyway. Needing to deal with (for example) 
floating-point operations in array bounds is a novel C++ problem.

I think the broadest set of places we could consider assuming the default FP 
environment is manifestly constant evaluated 
 contexts; that's probably the most 
natural category in C++ for this kind of thing. That is the same condition 
under which `std::is_constant_evaluated()` returns `true`. (The "manifestly 
constant evaluated" property is already tracked within the constant evaluator 
by the `InConstantContext` flag.) And your list:

> - initializers for objects of static storage duration if they would be 
> constant initializers if they weren't covered by the pragma; this should 
> roughly align C and C++ semantics for things like `static const` within a 
> function
> - any `constexpr` / `constinit` context, and it should be ill-formed to use 
> the pragma in such a context; that is, it's ignored from outside and 
> prohibited (directly) inside

... roughly matches "manifestly constant evaluated". (Aside: I don't think we 
need to, or should, prohibit the pragma inside `constexpr` functions. We'll try 
to reject such functions anyway if they can't produce constant expressions, so 
a special case rule seems redundant, and such functions can legitimately 
contain code only intended for use in non-constant contexts. Disallowing it in 
`consteval` functions might be OK, but we don't disallow calls to 
non-`constexpr` functions from `consteval` functions, for which the same 
concerns would apply.)

"Manifestly constant evaluated" describes the set of expressions that an 
implementation is required to reduce to a constant, and covers two sets of 
cases:

- expressions for which the program is ill-formed if they're not constant: 
template arguments, array bounds, enumerators, case labels, `consteval` 
function calls, `constexpr` and `constinit` variables, concepts, constexpr if
- expressions whose semantics are potentially affected by constantness, and for 
which an implementation is required to guarantee to commit to the constant 
semantics whenever it can: when checking to see if a variable with 
static/thread storage duration has static initialization, or whether an 
automatic variable of reference or const-qualified integral type is usable in 
constant expressions

If we go that way, there'd be at least one class of surprising cases. Here's an 
example of it:

  void f() {
  #pragma STDC FENV_ACCESS ON
fesetround(FE_DOWNWARD);
CONST bool b = (1.0 / 3.0) * 3.0 < 1.0;
if (b) g();
fesetround(FE_TONEAREST);
  }

Under the "manifestly constant evaluated" rule, with `-DCONST=`, this would 
call `g()`, but with `-DCONST=const`, it would *not* call `g()`, because the 
initializer of `b` would be manifestly constant evaluated. That's both 
surprising and different from the behavior in C. (The surprise isn't novel; 
C++'s `std::is_constant_evaluated()` has the same surprising semantics. It's 
not completely unreasonable to claim that C++ says that reference and `const` 
integral variables are implicitly `constexpr` whenever they can be, though 
that's not how the rule is expressed.) Basing this off "manifestly constant 
evaluated" would also be C-incompatible in at least one other way: we'd treat 
FP operations in an array bound as being in the default FP environment in C++ 
if that makes the overall evaluation constant, but in C they always result in a 
VLA.

So I suppose the question is, are we comfortable with all that, or do we want 
to use a different rule?

There's another uninventive rule at the opposite end of the spectrum. Prior 
discussion in the C++ committee, before we had "manifestly constant evaluated" 
and associated machinery, seemed to be converging on this model:

1. constrained FP operations are always treated as non-constant in all 
contexts, and
2. in a C++ translation unit, the initial state for `FENV_ACCESS` is always 
`OFF`.

That approach has the advantage of being both simple and compatible with all 
code we currently support, as well as probably 

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

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

rsmith added a comment.

> @rsmith asked "I don't see changes to the constant evaluator". The semantic 
> rules for enabling this pragma require that strict or precise semantics be in 
> effect., see SemaAttr.cpp And the floating point constant evaluation doesn't 
> occur when strict or precise

What do you mean "the floating point constant evaluation doesn't occur when 
strict or precise"? I don't see any code to do that in `ExprConstant.cpp`, 
neither in trunk nor in this patch. The constant evaluator certainly is still 
invoked when we're in strict or precise mode. I would expect the evaluator 
would need to check for FP strictness whenever it encounters a floating-point 
operator (eg, here: 
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L13340),
 and treat such cases as non-constant.

I'd like to see a C++ test for something like this:

  float myAdd() {
  #pragma STDC FENV_ACCESS ON
  static double v = 1.0 / 3.0;
  return v;
  }

If that generates a runtime guarded initialization for `v`, then I'd be a bit 
more convinced we're getting this right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528

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

[PATCH] D86048: [AST][RecoveryExpr] Popagate the error-bit from a VarDecl's initializer to DeclRefExpr.

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

rsmith added a comment.

In D86048#2254607 , @sammccall wrote:

> The crux is we're forcing `decltype(N)` to be a (unique) dependent type, 
> which feels wrong.
> [...]
> There are three behaviors:
>
> - standard up to C++14: traversing the expr looking for a template param to 
> be lexically included (this is my reading of the standard)

FWIW, "involves a template parameter" is exactly the same phrasing that 
[temp.over.link] uses to refer to instantiation-dependence; that's why Clang 
uses instantiation-dependence in this case at the moment.

> - what clang actually does: check instantiation dependence, which I think 
> pulls in too many cases
> - standard after http://wg21.link/cwg2064: check type dependence
>
> I think it's probably OK to adopt the C++17 behavior for all versions (if I'm 
> right that the current behavior is a bug).
> @rsmith It's your DR, what do you think :-)

Let's try it and see what happens. I think this will reveal a collection of 
cases where we don't properly handle 
instantiation-dependent-but-not-type-dependent constructs (such as, if I 
remember correctly, the types of non-type template parameters), but we should 
be fixing those bugs anyway :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86048

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

[PATCH] D87565: [Sema] Improve const_cast conformance to N4261

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

rsmith added a comment.

For what it's worth, the most recent thoughts from CWG on this question are 
from 2015 (prior to guaranteed copy elision, which probably changes the 
answer): http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1965. That 
direction would permit `const_cast` to rvalue reference type to materialize a 
temporary for arrays and classes but not scalar types (which seems just as 
arbitrary as the current set of rules to me).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87565

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

[PATCH] D87565: [Sema] Improve const_cast conformance to N4261

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

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



Comment at: clang/lib/Sema/SemaCast.cpp:1781-1782
 //   type T2 using the cast const_cast; and
 //-- if T1 is a class type, a prvalue of type T1 can be explicitly
 //   converted to an xvalue of type T2 using the cast const_cast.
 

This is the bullet that causes class pvalues and non-class prvalues to be 
treated differently by `const_cast` to an rvalue reference type, and it still 
exists unchanged in the latest C++ draft. As far as I can tell, the change here 
is not in line with the wording for N4261 / CWG issue 330, which Clang does 
implement.

(Now, I think the change you're suggesting here makes a lot of sense, but we 
should get agreement in WG21 to fix the language rules first.)



Comment at: clang/lib/Sema/SemaCast.cpp:1797-1801
+  // C++17 [expr.const.cast]p3
+  // For two similar types T1 and T2, a prvalue of type T1 may be 
explicitly
+  // converted to the type T2 using a const_cast if, considering the
+  // cv-decompositions of both types, each P1i is the same as P2i for all 
i.
+  // The result of a const_cast refers to the original entity.

In (for example) `const_cast(0);`, `T1` is `int`, and `T2` is "rvalue 
reference to `int`". `T1` and `T2` are not similar types, because they have 
different types `U` (for `T1`, `U` is `int`; for `T2`, `U` is "rvalue reference 
to `int`"). So this doesn't apply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87565

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

[PATCH] D87566: [Sema] Permit conversions to arrays of unknown bound

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

rsmith added a comment.

I made a start on this a while back and found a bunch more places that needed 
updates: 
https://github.com/zygoloid/llvm-project/commit/c23440979ac4f07ce38657ce0e69d010d2afa83e

Feel free to either take that patch as a basis or as inspiration for the 
remaining changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87566

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

[PATCH] D87561: [Sema] List conversion validate character array

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:4988
+
+if (ToType->isArrayType() && ToType->isCharType() &&
+isa(From->getInit(0))) {

`isCharType` is too narrow a check here. We also need to check for all the 
other types that can be initialized from a string literal (`wchar_t`, 
`char16_t`, ... -- including `unsigned short` in some cases). These details are 
handled by 
[`IsStringInit`](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaInit.cpp#L136).
 Maybe it's worth exposing that as a `bool Sema::isStringInit` function or 
similar to use from here?



Comment at: clang/lib/Sema/SemaOverload.cpp:4989
+if (ToType->isArrayType() && ToType->isCharType() &&
+isa(From->getInit(0))) {
   InitializedEntity Entity =

This is too narrow a check in two ways: we should allow parenthesized string 
literals here, and we should allow `ObjCEncodeExpr`.



Comment at: clang/lib/Sema/SemaOverload.cpp:4991-4992
   InitializedEntity Entity =
-InitializedEntity::InitializeParameter(S.Context, ToType,
-   /*Consumed=*/false);
+  InitializedEntity::InitializeParameter(S.Context, ToType,
+ /*Consumed=*/false);
   if (S.CanPerformCopyInitialization(Entity, From)) {

Phabricator thinks you converted spaces to tabs here. Please can you check and 
convert back if necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87561

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

[PATCH] D87563: [Sema] Improve overload resolution

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

rsmith added a comment.

Looks like this is an implementation of DR1307. Please also add a test to 
`test/CXX/drs/dr13xx.cpp`.




Comment at: clang/include/clang/Sema/Overload.h:548-549
+
+/// The std::initializer_list expression to convert from.
+const InitListExpr *StandardInitializerListFrom{nullptr};
+

Storing the `IntListExpr` here doesn't seem like it captures the necessary 
information. Consider:

```
void f2(std::pair (&&)[]); // #1
void f2(std::string (&&)[]); // #2
void g2() { f2({"foo","bar"}); } // should call #2
```

Here, the `InitListExpr` in both cases has two elements. We don't store the 
converted `InitListExpr`, because we don't -- and aren't allowed to -- build 
the converted `InitListExpr` until we've chosen the right overload. But I think 
what we're supposed to do in this case is to notice that calling #1 would 
initialize only one element of the array (one pair), whereas calling #2 would 
initialize two elements of the array, so we should call #2 here.

In effect, what you need to do is to compute the effective array bound for the 
case where we're initializing an array of unknown bound, and prefer the 
conversion sequence with the lower effective array bound. `InitListChecker` 
already works this out internally, see 
[here](https://github.com/llvm/llvm-project/blob/c0bcd11068fc13e45b253c6c315882097f94c121/clang/lib/Sema/SemaInit.cpp#L1937),
 but currently only does that when actually doing the conversion (when not in 
`VerifyOnly` mode); you'd need to adjust that.

Given that the code that needs this (the P0388 part) is dead code for now, 
perhaps the best thing would be to do only the array bound comparison in this 
patch, and we can compute the proper array bound for said comparison in a 
future patch.



Comment at: clang/lib/Sema/SemaOverload.cpp:3690-3703
+class CompareListInitializationSequences {
+  // List-initialization sequence L1 is a better conversion sequence than
+  // list-initialization sequence L2 if:
+  // ...
+  // - C++17:
+  //   L1 converts to type "array of N1 T", L2 converts to type "array of N2 
T",
+  //   and N1 is smaller than N2.,

I think this would be simpler if structured a little differently:

* Rename `CompareListInitializationSequences::Conversion` to 
`ListInitializationSequence`.
* Make `CompareListInitializationSequences::Compare` a static member of 
`ListInitializationSequence`.
* Move the element type comparison into `...::Compare` and remove the 
`CompareListInitializationSequences` class, inlining the (now very simple) 
constructor into its only user.



Comment at: clang/lib/Sema/SemaOverload.cpp:3856-3861
 if (ICS1.isStdInitializerListElement() &&
 !ICS2.isStdInitializerListElement())
   return ImplicitConversionSequence::Better;
 if (!ICS1.isStdInitializerListElement() &&
 ICS2.isStdInitializerListElement())
   return ImplicitConversionSequence::Worse;

Should this be part of `CompareListInitializationSequences` too?



Comment at: clang/lib/Sema/SemaOverload.cpp:5150-5156
+} else if (ToType->isConstantArrayType()) {
+  // Has the initializer list exactly N elements or fewer than N elements?
+  uint64_t Size = S.getASTContext()
+  .getAsConstantArrayType(ToType)
+  ->getSize()
+  .getZExtValue();
+  if (From->getNumInits() > Size)

Simplify, and we may as well avoid assuming that the size fits in 64 bits since 
it's easy to do so here (even though there are lots of other places where we 
make that assumption).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87563

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

[PATCH] D84637: [AST] Enhance the const expression evaluator to support error-dependent exprs.

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

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4583
   if (!InitE)
 return getDefaultInitValue(VD->getType(), Val);
 

The initializer might also be null because the variable is type-dependent (eg, 
`X x;`), in which case assuming default-init is wrong. We 
should check for that and treat it like a value-dependent initializer.



Comment at: clang/lib/AST/ExprConstant.cpp:4961
 }
+if (IS->getCond()->isValueDependent())
+  return EvaluateDependentExpr(IS->getCond(), Info);

hokein wrote:
> The `if` stmt (the same to `while`, `for`) is tricky -- if the condition is 
> value-dependent, then we don't know which branch we should run into.
> 
> - returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the end of 
> the function");
> - returning a ESR_Failed may lead to a bogus diagnostic ("never produce a 
> constexpr"); 
> 
> I guess what we want is to stop the evaluation, and indicate that we hit a 
> value-dependent expression and we don't know how to evaluate it, but still 
> treat the constexpr function as potential constexpr (but no extra diagnostics 
> being emitted), but the current `EvalStmtResult` is not sufficient, maybe we 
> need a new enum.
We should only produce the "never produce a constant expression" diagnostic if 
we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here should 
work.



Comment at: clang/lib/AST/ExprConstant.cpp:4961
 }
+if (IS->getCond()->isValueDependent())
+  return EvaluateDependentExpr(IS->getCond(), Info);

rsmith wrote:
> hokein wrote:
> > The `if` stmt (the same to `while`, `for`) is tricky -- if the condition is 
> > value-dependent, then we don't know which branch we should run into.
> > 
> > - returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the end 
> > of the function");
> > - returning a ESR_Failed may lead to a bogus diagnostic ("never produce a 
> > constexpr"); 
> > 
> > I guess what we want is to stop the evaluation, and indicate that we hit a 
> > value-dependent expression and we don't know how to evaluate it, but still 
> > treat the constexpr function as potential constexpr (but no extra 
> > diagnostics being emitted), but the current `EvalStmtResult` is not 
> > sufficient, maybe we need a new enum.
> We should only produce the "never produce a constant expression" diagnostic 
> if we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here 
> should work.
Should this check live in EvaluateCond instead?



Comment at: clang/lib/AST/ExprConstant.cpp:5053
 FullExpressionRAII IncScope(Info);
 if (!EvaluateIgnoredValue(Info, FS->getInc()) || !IncScope.destroy())
   return ESR_Failed;

Missing value dependence check here.



Comment at: clang/lib/AST/ExprConstant.cpp:7896
   QualType Type = Inner->getType();
-
+  if (Inner->isValueDependent())
+return EvaluateDependentExpr(Inner, Info);

How does this happen? I would expect the dependence of the subexpression to be 
reflected in the MaterializeTemporaryExpr.



Comment at: clang/lib/AST/ExprConstant.cpp:8440
 } else {
+  if (SubExpr->isValueDependent())
+return EvaluateDependentExpr(SubExpr, Info);

How does this happen?



Comment at: clang/lib/AST/ExprConstant.cpp:9145
   } else if (Init) {
+if (Init->isValueDependent())
+  return EvaluateDependentExpr(Init, Info);

How does this happen? Do we not propagate value-dependence from initializers to 
new-expressions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84637

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added inline comments.



Comment at: clang/lib/Headers/intrin.h:435
 #if defined(__i386__) || defined(__x86_64__)
-static __inline__ void __DEFAULT_FN_ATTRS
-__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n) {
+void __DEFAULT_FN_ATTRS __movsb(unsigned char *__dst,
+unsigned char const *__src, size_t __n) {

The functions with inline definitions should still be `static inline` so that 
we don't emit them as strong external defintiions.



Comment at: clang/lib/Sema/SemaDecl.cpp:9672-9673
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+const auto *LinkageDecl =
+dyn_cast(NewFD->getDeclContext());
+

This will give the wrong answer for
```
extern "C" {
namespace X {
void __builtin_foo();
}
}
```
... which does have C language linkage. Instead, please call 
`FunctionDecl::getLanguageLinkage()`, which knows how to handle these cases.



Comment at: clang/lib/Sema/SemaDecl.cpp:9690-9691
+if (!Error && !BuiltinType.isNull()) {
+  // We want noexcept declarations to match. Create an identical
+  // function type, but remove the exception spec.
+  const FunctionProtoType *Type =

Please use `ASTContext::hasSameTypeIgnoringExceptionSpec` instead.



Comment at: clang/lib/Serialization/ASTReader.cpp:914
+  return II.hadMacroDefinition() || II.isPoisoned() ||
+ II.getObjCOrBuiltinID() || II.hasRevertedTokenIDToIdentifier() ||
  (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&

We now consider `getObjCOrBuiltinID()` here for the `IsModule` case, where we 
didn't before. Is that an intentional change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D86508: [clang] improve GCC-compat for C90 __builtin_ functions

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

rsmith added a comment.

It seems to me that adding new `__builtin_*` functions for GCC compatibility 
and adding new `LIBBUILTIN`s are unrelated changes, and it might be clearer to 
split them up into two commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86508

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

[PATCH] D86508: [clang] improve GCC-compat for C90 __builtin_ functions

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

rsmith added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:2370-2398
+* ``bcmp``
 * ``memchr``
-* ``memcmp`` (and its deprecated BSD / POSIX alias ``bcmp``)
+* ``memcmp``
+* ``memcpy``
+* ``memmove``
+* ``memset``
+* ``strcat``

aaron.ballman wrote:
> Can you mention the deprecation issue here?
Do we really provide constant evaluation support for all of these functions?



Comment at: clang/docs/LanguageExtensions.rst:2381
+* ``strcspn``
+* ``strerror``
 * ``strlen``

Do we really provide `__builtin_strerror`? I don't see it below.



Comment at: clang/docs/LanguageExtensions.rst:2425-2435
-Clang provides constant expression evaluation support for builtin forms of the
-following functions from the C standard library headers
- and :
-
-* ``memcpy``
-* ``memmove``
-* ``wmemcpy``

You've moved this from the section where we describe the constant evaluation 
rules for these functions to a section where we describe the constant 
evaluation rules for string functions; those rules are quite different.



Comment at: clang/docs/LanguageExtensions.rst:2427-2428
+
+Clang provides support for builtins forms of the following functions from the C
+standard library header :
+

We should say how these "builtin forms" are named and what they're for: that we 
provide a `__builtin_`-prefixed version of each of these functions that has the 
same signature and semantics as the function from the C standard library but 
doesn't require a header to be included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86508

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

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

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

rsmith added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:1105-
+  /// The likelihood of a branch being taken.
+  enum Likelihood {
+LH_None, ///< No attribute set.
+LH_Likely,   ///< Branch has the [[likely]] attribute.
+LH_Unlikely, ///< Branch has the [[unlikely]] attribute.
+LH_Conflict  ///< Both branches of the IfStmt have the same attribute.
+  };

I'd suggest you order these in increasing order of likeliness: unlikely, none, 
likely. Then you can determine what branch weights to use by a three-way 
comparison of the likeliness of the `then` branch and the likeliness of the 
`else` branch.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

Mordante wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > Sema doesn't care about any of this; can you move this code to CodeGen 
> > > and remove the code that stores likelihood data in the AST?
> > FWIW, I asked for it to be moved out of CodeGen and into Sema because the 
> > original implementation in CodeGen was doing a fair amount of work trying 
> > to interrogate the AST for this information. Now that we've switched the 
> > design to only be on the substatement of an if/else statement (rather than 
> > an arbitrary statement), it may be that CodeGen is a better place for this 
> > again (and if so, I'm sorry for the churn).
> At the moment the Sema cares about it to generate diagnostics about 
> conflicting annotations:
> ```
> if(i) [[ likely]] {}
> else [[likely]] {}
> ```
> This diagnostic only happens for an if statement, for a switch multiple 
> values can be considered likely.
> Do you prefer to have the diagnostic also in the CodeGen?
It looked to me like you'd reached agreement to remove that diagnostic. Are you 
intending to keep it?

If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so that 
`CodeGen` and the warning here can both easily call it.


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

https://reviews.llvm.org/D85091

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

[PATCH] D86207: (De-)serialize BindingDecls before DecompositionDecl

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

rsmith added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:585
   D->setLocation(ThisDeclLoc);
   D->setInvalidDecl(Record.readInt());
   if (Record.readInt()) { // hasAttrs

The bug is here: we should not be calling `Decl::setInvalidDecl` here because 
it has invariants, and the `Decl` is incomplete at this point.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1500
   for (unsigned I = 0; I != DD->NumBindings; ++I) {
 BDs[I] = readDeclAs();
 BDs[I]->setDecomposedDecl(DD);

`BindingDecl` might not be fully initialized here: if we enter deserialization 
to deserialize a `BindingDecl`, and then recurse into reading its binding 
expression, and then deserialize the `DecompositionDecl`, we can land here 
before we finish with the `BindingDecl`. If we called something that looked at 
the `Binding` expression on the `BindingDecl`, that'd be a problem.

But the general idea is that deserialization should never invoke AST functions 
with invariants (and generally should set state directly rather than using AST 
member functions in order to avoid accidentally calling a function with an 
invariant). So it shouldn't matter whether we deserialize the 
`DecompositionDecl` or the `BindingDecl`s first.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1509
   BD->Binding = Record.readExpr();
 }
 

Hm, presumably `BindingDecl::Decomp` gets set here because `readExpr` always 
reads an expression that involves the `DecompositionDecl`, which calls 
`setDecomposedDecl`? That seems ... very subtle. If that's the intended way for 
this to work, we should at least add a comment for that (or better, an assert 
that `Decomp` got set), and `BindingDecl::Decomp` should be a regular pointer 
not a `LazyDeclPtr`. (But even then, is this chain of reasoning guaranteed to 
hold even for invalid declarations? Maybe we should be serializing the 
`DecompositionDecl*` and setting `Decomp` here?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86207

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

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

rsmith added a comment.

In D86993#2251198 , @rjmccall wrote:

> Wording looks good.  Should we alsso document our assumptions about what 
> functions exist in the standard library — the functions that we'll always use 
> even in freestanding builds?

Sounds like a good idea to me. Do we have a list?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

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

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

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

rsmith added a comment.

In D85778#2251160 , @zequanwu wrote:

> Hi, this change seems like hits a false positive case in chromium build: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1124085

That's not a false positive. The code is (simplified):

  int RoundDown(int a, long b) { return a & -b; }

... which is implicitly converting an expression of type `long` to `int`, 
losing precision. For example, `RoundDown(-1, 0x1'')` is -4294967296 
prior to the implicit conversion from `long` to `int`. Given that the 
truncation is presumably intentional (0 at least seems like the least-bad 
answer for rounding down 0 to a multiple of 2^32 as a 32-bit integer), you can 
suppress the warning with a cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85778

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

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

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

rsmith added a comment.

In D85778#2248714 , @nikic wrote:

> Just for the record, the additional analysis has a measurable compile-time 
> impact (0.3% at O0): 
> https://llvm-compile-time-tracker.com/compare.php?from=e7f53044e7263cdbbb4fed9abf086b88ba486bba=cff6dda604cb0548bef5e5951dd1e74536110588=instructions

Thanks, it seems very likely that this is caused by `CheckImplicitConversion` 
now calling `CheckExprRange` twice. It looks like that's not only avoidable, 
but there's a (pre-existing) bug here that we can squash at the same time :) 
Should be fixed in rG0ffbbce78de60f4f4d03d6ef97fe2f3bb4275e08 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85778

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

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

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

Add missing word.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

Files:
  clang/docs/Toolchain.rst


Index: clang/docs/Toolchain.rst
===
--- clang/docs/Toolchain.rst
+++ clang/docs/Toolchain.rst
@@ -276,6 +276,19 @@
 `C standard library `_
 implementations.
 
+Clang and LLVM make some assumptions about the behavior of the C standard
+library beyond those required by the C standard:
+
+* ``memcpy(p, q, 0)`` and ``memmove(p, q, 0)`` are expected to have no effect,
+  even if ``p`` or ``q`` is a null or otherwise-invalid (but correctly-aligned)
+  pointer.
+* ``memcpy(p, p, n)`` is expected to leave the contents of the memory pointed
+  to by ``p`` unchanged, but may perform (redundant) loads and stores through
+  the ``n`` bytes pointed to by ``p``.
+
+C standard library implementations that do not guarantee these properties are
+incompatible with Clang and LLVM (and with several other major compilers).
+
 C++ ABI library
 ---
 


Index: clang/docs/Toolchain.rst
===
--- clang/docs/Toolchain.rst
+++ clang/docs/Toolchain.rst
@@ -276,6 +276,19 @@
 `C standard library `_
 implementations.
 
+Clang and LLVM make some assumptions about the behavior of the C standard
+library beyond those required by the C standard:
+
+* ``memcpy(p, q, 0)`` and ``memmove(p, q, 0)`` are expected to have no effect,
+  even if ``p`` or ``q`` is a null or otherwise-invalid (but correctly-aligned)
+  pointer.
+* ``memcpy(p, p, n)`` is expected to leave the contents of the memory pointed
+  to by ``p`` unchanged, but may perform (redundant) loads and stores through
+  the ``n`` bytes pointed to by ``p``.
+
+C standard library implementations that do not guarantee these properties are
+incompatible with Clang and LLVM (and with several other major compilers).
+
 C++ ABI library
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86993: Document Clang's expectations of the C standard library.

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

rsmith created this revision.
rsmith added reviewers: fhahn, rjmccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
rsmith requested review of this revision.

As suggested in https://reviews.llvm.org/D86815


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86993

Files:
  clang/docs/Toolchain.rst


Index: clang/docs/Toolchain.rst
===
--- clang/docs/Toolchain.rst
+++ clang/docs/Toolchain.rst
@@ -276,6 +276,19 @@
 `C standard library `_
 implementations.
 
+Clang and LLVM make some assumptions about the behavior of the C standard
+library beyond those required by the C standard:
+
+* ``memcpy(p, q, 0)`` and ``memmove(p, q, 0)`` are expected to have no effect,
+  even if ``p`` or ``q`` is a null or otherwise-invalid (but correctly-aligned)
+  pointer.
+* ``memcpy(p, p, n)`` is expected to leave the contents of the memory pointed
+  to by ``p`` unchanged, but may perform (redundant) loads and stores through
+  the ``n`` bytes pointed to by ``p``.
+
+C standard library implementations that do not guarantee these properties
+incompatible with Clang and LLVM (and with several other major compilers).
+
 C++ ABI library
 ---
 


Index: clang/docs/Toolchain.rst
===
--- clang/docs/Toolchain.rst
+++ clang/docs/Toolchain.rst
@@ -276,6 +276,19 @@
 `C standard library `_
 implementations.
 
+Clang and LLVM make some assumptions about the behavior of the C standard
+library beyond those required by the C standard:
+
+* ``memcpy(p, q, 0)`` and ``memmove(p, q, 0)`` are expected to have no effect,
+  even if ``p`` or ``q`` is a null or otherwise-invalid (but correctly-aligned)
+  pointer.
+* ``memcpy(p, p, n)`` is expected to leave the contents of the memory pointed
+  to by ``p`` unchanged, but may perform (redundant) loads and stores through
+  the ``n`` bytes pointed to by ``p``.
+
+C standard library implementations that do not guarantee these properties
+incompatible with Clang and LLVM (and with several other major compilers).
+
 C++ ABI library
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86508: [clang] implement+test remaining C90 __builtin_ functions

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

rsmith added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:488
+BUILTIN(__builtin_calloc, "v*zz", "F")
+BUILTIN(__builtin_exit, "vi", "Fr")
 BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")

aaron.ballman wrote:
> Should we be adding `atexit()` as well?
GCC doesn't have a `__builtin_atexit`, so we'd need some reason to invent one.



Comment at: clang/include/clang/Basic/Builtins.def:513
 BUILTIN(__builtin_printf, "icC*.", "Fp:0:")
+BUILTIN(__builtin_putchar, "ii", "F")
+BUILTIN(__builtin_puts, "icC*", "nF")

aaron.ballman wrote:
> Should we also add a builtin for `putc()` (I know that's often a macro, so 
> I'm not certain if it applies)?
Yes, GCC has a `__builtin_putc`, so it'd make sense for us to support that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86508

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

[PATCH] D60939: [Concepts] Delayed Constraint Substitution

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

rsmith added a comment.

I wonder if there's a cleaner way to model this:

Suppose we add a new `Expr` subclass for an expression with delayed template 
argument substitution, which would capture a list of `TemplateArgument`s and an 
inner `Expr*` into which those template arguments have not yet been 
substituted. Then when we transform a trailing requires-clause, we can wrap it 
suitably with that expression node, and similarly when we transform a 
`TypeConstraint` we can wrap the immediately-declared constraint in the partial 
substitution wrapper.

We'd need to teach normalization to pick up the wrapper and turn it into a 
parameter mapping, and we'd need to teach satisfaction checking to actually 
substitute that partially-substituted wrapper, but I think the result would be 
a bit simpler and wouldn't need us to carefully keep track of whether each 
individual constraint is substituted or not. What do you think?




Comment at: include/clang/AST/DeclTemplate.h:70-71
 : private llvm::TrailingObjects {
+  llvm::PointerUnion> 
{
   /// The location of the 'template' keyword.

Please can you introduce a typedef or wrapper type for this? Maybe a `struct 
TemplateHeadConstraints` or similar?



Comment at: include/clang/AST/DeclTemplate.h:96-97
 SourceLocation LAngleLoc, ArrayRef Params,
-SourceLocation RAngleLoc, Expr *RequiresClause);
+SourceLocation RAngleLoc, Expr *RequiresClause,
+TemplateParameterList *InheritedConstraints);
 

Consider passing the union type (whatever it's called) here instead of having 
an `Expr*` argument and a `TemplateParameterList*` argument, at least one of 
which must be null.



Comment at: include/clang/AST/DeclTemplate.h:1276
+  };
+  llvm::PointerIntPair
+  TypeConstraintStatus;

Is it feasible to tail-allocate the pointer? (At least in the case where there 
is no type-constraint, it'd be nice to avoid allocating storage for it; 
allocating it unconditionally along with the `TypeConstraint` seems good enough 
to me.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D60939

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

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

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

rsmith added a comment.

Looking specifically for attributes in the 'then' and 'else' cases of an `if` 
seems like a fine first pass at this, but I think this is the wrong way to 
lower these attributes in the longer term: we should have a uniform treatment 
of them that looks for the most recent prior branch and annotates it with 
weights. We could do that either by generating LLVM intrinsic calls that an 
LLVM pass lowers, or by having the frontend look backwards for the most recent 
branch and annotate it. I suppose it's instructive to consider a case such as:

  void mainloop() noexcept; // probably terminates by calling `exit`
  void f() {
mainloop();
[[unlikely]];
somethingelse();
  }

... where the `[[unlikely]];` should probably cause us to split the 
`somethingelse()` out into a separate basic block that we mark as cold, but 
should not cause `f()` itself or its call to `mainloop()` to be considered 
unlikely -- except in the case where `mainloop()` can be proven to always 
terminate, in which case the implication is that it's unlikely that `f()` is 
invoked. Cases like this probably need the LLVM intrinsic approach to model 
them properly.




Comment at: clang/include/clang/AST/Stmt.h:175-178
+/// The likelihood of taking the ThenStmt.
+/// One of the enumeration values in Stmt::Likelihood.
+unsigned ThenLikelihood : 2;
+

Do we need to store this here? The "then" and "else" cases should be an 
`AttributedStmt` holding the likelihood attribute, so duplicating that info 
here seems redundant.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

Sema doesn't care about any of this; can you move this code to CodeGen and 
remove the code that stores likelihood data in the AST?


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

https://reviews.llvm.org/D85091

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

[PATCH] D86802: [Modules] Don't parse/load explicit module maps if modules are disabled

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

Sorry for not noticing this review earlier.

We can't make this change. Module map files are still useful and still used 
even when modules are disabled -- they power the undeclared inclusion and 
private header diagnostics, and with `-fmodules-local-submodule-visibility` you 
still get modular semantics for modular headers even when `-fmodules` is 
disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86802

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

[PATCH] D86508: [clang] implement+test remaining C90 __builtin_ functions

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

rsmith added a comment.

LGTM

I've not checked all the types are correct (someone should double-check that 
prior to commit), but it looks like GCC provides these `__builtin_*` functions, 
so we should too. The addition of the non-`__builtin_` versions should improve 
our diagnostics but otherwise have no effect.




Comment at: clang/lib/Lex/PPMacroExpansion.cpp:348
   // C++ Standing Document Extensions.
-  if (LangOpts.CPlusPlus)
+  if (getLangOpts().CPlusPlus)
 Ident__has_cpp_attribute =

The changes to this file appear to be independent of the rest of the patch. 
Please go ahead and land this cleanup separately.



Comment at: clang/test/CXX/over/over.oper/over.literal/p7.cpp:10
 
-void puts(const char *);
+int puts(const char *);
 static inline void operator "" _puts(const char *c) {

Doesn't this also need to be `extern "C"` to be recognized as the builtin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86508

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

[PATCH] D86514: Correctly parse LateParsedTemplates in case of multiple dependent modules

[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!




Comment at: clang/lib/Serialization/ASTReader.cpp:8395
+for (unsigned Idx = 0, N = LateParsed.size(); Idx < N;
+ /* In loop */) {
+  FunctionDecl *FD =

Phabricator claims there's a tab here and below. Please double-check you're 
indenting with spaces before commit.


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

https://reviews.llvm.org/D86514

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

[PATCH] D85778: More accurately compute the ranges of possible values for +, -, *, &, %.

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcff6dda604cb: More accurately compute the ranges of possible 
values for +, -, *, , %. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D85778?vs=284883=289079#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85778

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/tautological-constant-compare.c

Index: clang/test/Sema/tautological-constant-compare.c
===
--- clang/test/Sema/tautological-constant-compare.c
+++ clang/test/Sema/tautological-constant-compare.c
@@ -4,12 +4,16 @@
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify=silent -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify=silent -x c++ %s
+
+#ifndef TEST
+// silent-no-diagnostics
+#endif
 
 int value(void);
 
@@ -184,7 +188,6 @@
   if (-32768L >= s)
   return 0;
 #else
-  // expected-no-diagnostics
   if (s == 32767)
 return 0;
   if (s != 32767)
@@ -373,7 +376,6 @@
   if (65535UL >= us) // expected-warning {{comparison 65535 >= 'unsigned short' is always true}}
   return 0;
 #else
-  // expected-no-diagnostics
   if (us == 65535)
   return 0;
   if (us != 65535)
@@ -571,20 +573,100 @@
 
   if ((s & 0xff) < 0) {} // #valuerange1
   if ((s & 0xff) < 1) {}
-  if ((s & -3) < -4) {} // #valuerange2
+  if ((s & -3) < -4) {}
   if ((s & -3) < -3) {}
-  if ((s & -3) < 4u) {} // (true if s non-negative)
-  if ((s & -3) > 4u) {} // (true if s negative)
-  if ((s & -3) == 4u) {} // #valuerange3 (never true)
-  if ((s & -3) == 3u) {}
-  if ((s & -3) == -5u) {} // #valuerange4
+  if ((s & -3) < 4u) {}
+  if ((s & -3) > 4u) {}
+  if ((s & -3) == 4u) {}
+  if ((s & -3) == 3u) {} // FIXME: Impossible.
+  if ((s & -3) == -5u) {}
   if ((s & -3) == -4u) {}
 #if TEST == 2
   // expected-warning@#valuerange1 {{comparison of 8-bit unsigned value < 0 is always false}}
-  // expected-warning@#valuerange2 {{comparison of 3-bit signed value < -4 is always false}}
-  // expected-warning@#valuerange3 {{comparison of 3-bit signed value == 4 is always false}}
-  // expected-warning@#valuerange4 {{comparison of 3-bit signed value == 4294967291 is always false}}
 #endif
 
+  // FIXME: Our bit-level width tracking comes unstuck here: the second of the
+  // conditions below is also tautological, but we can't tell that because we
+  // don't track the actual range, only the bit-width.
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 1) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 2) {}
+  if ((s ? 1 : 0) + (us ? 1 : 0) > 3) {} // #addrange1
+#if TEST == 2
+  // expected-warning@#addrange1 {{comparison of 2-bit unsigned value > 3 is always false}}
+#endif
+
+  // FIXME: The second and third comparisons are also tautological; 0x4000
+  // is the greatest value that multiplying two int16s can produce.
+  if (s * s > 0x3fff) {}
+  if (s * s > 0x4000) {}
+  if (s * s > 0x7ffe) {}
+  if (s * s > 0x7fff) {} // expected-warning {{result of comparison 'int' > 2147483647 is always false}}
+
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be0) {}
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7be1) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7ffe) {} // FIXME
+  if ((s & 0x3ff) * (s & 0x1f) > 0x7fff) {} // #mulrange1
+#if TEST == 2
+  // expected-warning@#mulrange1 {{comparison of 15-bit unsigned value > 32767 is always false}}
+#endif
+
+  if (a.a * a.b > 21) {} // FIXME
+  if (a.a * a.b > 31) {} // #mulrange2
+#if TEST == 2
+  // expected-warning@#mulrange2 {{comparison of 6-bit signed value > 31 is always false}}
+#endif
+
+  if (a.a - (s & 1) < 

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

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

rsmith added a comment.

In D77491#2246948 , @rjmccall wrote:

> I'd still like @rsmith to sign off here as code owner.

Generally, I'm happy with this direction.

What happens for builtins with the "t" (custom typechecking) flag, for which 
the signature is intended to have no meaning? Do we always give them builtin 
semantics, or never, or ... something else? I think it might be reasonable to 
require them to always be declared as taking unspecified arguments -- `()` in C 
and `(...)` in C++, or to simply say that the user cannot declare such 
functions themselves.




Comment at: clang/lib/Headers/intrin.h:148
 void __writemsr(unsigned long, unsigned __int64);
-static __inline__
-void *_AddressOfReturnAddress(void);
-static __inline__
-unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
-static __inline__
-unsigned char _BitScanReverse(unsigned long *_Index, unsigned long _Mask);
+__inline__ void *_AddressOfReturnAddress(void);
+__inline__ unsigned char _BitScanForward(unsigned long *_Index,

Does the `__inline__` here do anything for a builtin function? Can we remove it 
along with the `static`?



Comment at: clang/lib/Headers/intrin.h:200
 void __incgsword(unsigned long);
-static __inline__
-void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__ void __movsq(unsigned long long *, unsigned long long const 
*,
+   size_t);

Why is `static` being removed from some of the functions in this header but not 
others?



Comment at: clang/lib/Sema/SemaDecl.cpp:9668-9694
+  // In C builtins get merged with implicitly lazily created declarations.
+  // In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+  if (getLangOpts().CPlusPlus) {
+if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+  if (unsigned BuiltinID = II->getBuiltinID()) {
+FunctionDecl *D = CreateBuiltin(II, BuiltinID, NewFD->getLocation());
+

I think this needs more refinement:

* You appear to be creating and throwing away a new builtin function 
declaration (plus parameter declarations etc) each time you see a declaration 
with a matching name, even if one was already created. Given that you don't 
actually use `D` for anything other than its type, creating the declaration 
seems redundant and using `ASTContext::GetBuiltinType` would be more 
appropriate.
* There are no checks of which scope the new function is declared in; this 
appears to apply in all scopes, but some builtin names are only reserved in the 
global scope (those beginning with an underscore followed by a lowercase letter 
such as `_bittest`), so that doesn't seem appropriate. The old code in 
`FunctionDecl::getBuiltinID` checked that the declaration is given C language 
linkage (except for `_GetExceptionInfo`, which was permitted to have C++ 
language linkage), and that check still seems appropriate to me.
* The special case permitting `_GetExceptionInfo` to be declared with *any* 
type seems suspect; the old code permitted it to have different language 
linkage, not the wrong type.
* Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't 
have a notion of "compatible types".



Comment at: clang/lib/Serialization/ASTReader.cpp:975
   bool HasRevertedTokenIDToIdentifier = readBit(Bits);
-  bool HasRevertedBuiltin = readBit(Bits);
+  readBit(Bits); // Previously used to indicate reverted builtin.
   bool Poisoned = readBit(Bits);

We don't have any stability guarantees for our AST bitcode format yet; you can 
just remove this bit rather than retaining a placeholder.



Comment at: clang/test/Analysis/bstring.cpp:106
   Derived d;
-  memset(_mem, 0, sizeof(Derived));
+  memset(_mem, 0, sizeof(Derived)); // expected-warning {{'memset' will 
always overflow; destination buffer has size 4, but size argument is 8}}
   clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}

This should not be recognized as a builtin, because the `memset` function is 
not `extern "C"`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D86751: Add new warning for compound punctuation tokens that are split across macro expansions or split by whitespace.

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
rsmith marked an inline comment as done.
Closed by commit rG0e00a95b4fad: Add new warning for compound punctuation 
tokens that are split across macro… (authored by rsmith).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86751

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Headers/altivec.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Parser/compound-token-split.cpp

Index: clang/test/Parser/compound-token-split.cpp
===
--- /dev/null
+++ clang/test/Parser/compound-token-split.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 %s -verify=expected,space -Wcompound-token-split
+// RUN: %clang_cc1 %s -verify=expected,space -Wall
+
+#ifdef LSQUARE
+[ // expected-note {{second '[' token is here}}
+#else
+
+#define VAR(type, name, init) type name = (init)
+
+void f() {
+  VAR(int, x, {}); // #1
+  // expected-warning@#1 {{'(' and '{' tokens introducing statement expression appear in different macro expansion contexts}}
+  // expected-note-re@#1 ^}}'{' token is here}}
+  //
+  // FIXME: It would be nice to suppress this when we already warned about the opening '({'.
+  // expected-warning@#1 {{'}' and ')' tokens terminating statement expression appear in different macro expansion contexts}}
+  // expected-note-re@#1 ^}}')' token is here}}
+  //
+  // expected-error@#1 {{cannot initialize a variable of type 'int' with an rvalue of type 'void'}}
+}
+
+#define RPAREN )
+
+int f2() {
+  int n = ({ 1; }RPAREN; // expected-warning {{'}' and ')' tokens terminating statement expression appear in different macro expansion contexts}} expected-note {{')' token is here}}
+  return n;
+}
+
+[ // expected-warning-re ^}}'[' tokens introducing attribute appear in different source files}}
+#define LSQUARE
+#include __FILE__
+  noreturn ]]  void g();
+
+[[noreturn] ] void h(); // space-warning-re ^}}']' tokens terminating attribute are separated by whitespace}}
+
+struct X {};
+int X:: *p; // space-warning {{'::' and '*' tokens forming pointer to member type are separated by whitespace}}
+
+#endif
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -94,6 +94,9 @@
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
 CHECK-NEXT:  -Wmisleading-indentation
+CHECK-NEXT:  -Wcompound-token-split
+CHECK-NEXT:-Wcompound-token-split-by-macro
+CHECK-NEXT:-Wcompound-token-split-by-space
 
 
 CHECK-NOT:-W
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -227,6 +227,39 @@
   return true;
 }
 
+void Parser::checkCompoundToken(SourceLocation FirstTokLoc,
+tok::TokenKind FirstTokKind, CompoundToken Op) {
+  if (FirstTokLoc.isInvalid())
+return;
+  SourceLocation SecondTokLoc = Tok.getLocation();
+
+  // We expect both tokens to come from the same FileID.
+  FileID FirstID = PP.getSourceManager().getFileID(FirstTokLoc);
+  FileID SecondID = PP.getSourceManager().getFileID(SecondTokLoc);
+  if (FirstID != SecondID) {
+Diag(FirstTokLoc, (FirstTokLoc.isMacroID() || SecondTokLoc.isMacroID())
+  ? diag::warn_compound_token_split_by_macro
+  : diag::warn_compound_token_split_by_file)
+<< (FirstTokKind == Tok.getKind()) << FirstTokKind << Tok.getKind()
+<< static_cast(Op) << SourceRange(FirstTokLoc);
+Diag(SecondTokLoc, diag::note_compound_token_split_second_token_here)
+<< (FirstTokKind == Tok.getKind()) << Tok.getKind()
+<< SourceRange(SecondTokLoc);
+return;
+  }
+
+  // We expect the tokens to abut.
+  if (Tok.hasLeadingSpace()) {
+SourceLocation SpaceLoc = PP.getLocForEndOfToken(FirstTokLoc);
+if (SpaceLoc.isInvalid())
+  SpaceLoc = FirstTokLoc;
+Diag(SpaceLoc, diag::warn_compound_token_split_by_whitespace)
+<< (FirstTokKind == Tok.getKind()) << FirstTokKind << Tok.getKind()
+<< static_cast(Op) << SourceRange(FirstTokLoc, SecondTokLoc);
+return;
+  }
+}
+
 //===--===//
 // Error recovery.
 //===--===//
Index: clang/lib/Parse/ParseStmt.cpp
===
--- 

[PATCH] D86751: Add new warning for compound punctuation tokens that are split across macro expansions or split by whitespace.

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

rsmith marked an inline comment as done.
rsmith added inline comments.
Herald added a subscriber: danielkiss.



Comment at: clang/test/Parser/compound-token-split.cpp:30
+
+[ // expected-warning-re ^}}'[' tokens introducing attribute appear in 
different source files}}
+#define LSQUARE

sbenza wrote:
> Does this count as a "different" source file?
> Or is it just an implementation detail of how clang uses FileIDs ?
:) Yes, this is a consequence of how Clang uses `FileID`s.

I guess it's debatable whether the includer and included file are "different", 
but I'm hoping no-one will be confused by the diagnostic. (I think it's one of 
those "if you ever see this diagnostic in practice you win a small prize" 
cases...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86751

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