[PATCH] D40218: [Clang] Add __builtin_launder

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

rsmith added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1425-1426
+
+  // FIXME: We either have an incomplete class type, or we have a class template
+  // whose instantiation has not been forced. Example:
+  //
----------------
I think it's a bug that `launder` doesn't require `T` to be a complete type. Can you file an LWG issue?

We should also decide whether we want to proactively fix this issue (require the type to be complete from the `Sema` checking of the builtin and assert that it's defined here) or not.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1437-1438
+
+  // if (!Seen.insert(Record).second)
+  //  return false;
+  for (FieldDecl *F : Record->fields()) {
----------------
Delete this commented-out code.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1451
+    return false;
+  llvm::DenseSet<const Decl *> Seen;
+  return TypeRequiresBuiltinLaunderImp(CGM.getContext(), Ty, Seen);
----------------
Would `SmallPtrSet` be a better choice here?


================
Comment at: lib/Sema/SemaChecking.cpp:885-887
+  // Don't perform LValue conversions since they may strip things like the
+  // restrict qualifier
+  ExprResult Arg = S.DefaultFunctionArrayConversion(OrigArg);
----------------
Instead of performing some of the conversions here and some of them as part of initialization, I think it'd be more obvious to compute the builtin's parameter type here (which is the type of the argument if it's not of array [or function] type, and the decayed type of the argument otherwise), and do the decay and lvalue-to-rvalue conversion as part of the parameter initialization below.

The current code arrangement (and especially this comment) leaves a reader thinking "but you *need* an lvalue-to-rvalue conversion if the argument is an lvalue".


================
Comment at: lib/Sema/SemaChecking.cpp:935
   return true;
+
 }
----------------
Did you mean to add this blank line?


================
Comment at: test/CodeGen/builtins.c:404-409
+  // CHECK: entry
+  // CHECK-NEXT: %p.addr = alloca i32*
+  // CHECK-NEXT: %d = alloca i32*
+  // CHECK-NEXT: store i32* %p, i32** %p.addr, align 8
+  // CHECK-NEXT: [[TMP:%.*]] = load i32*, i32** %p.addr
+  // CHECK-NEXT: store i32* [[TMP]], i32** %d
----------------
This test is not robust against minor IR differences such as variable or basic block names changing (some of these change in a release build), and is testing things that are not related to this builtin (eg, that we produce an alloca for a function parameter and its relative order to an alloca for a local variable).

I would remove everything here other than the load and the store, and add an explicit check that we don't generate a launder call:

```
  // CHECK: [[TMP:%.*]] = load i32*,
  // CHECK-NOT: @llvm.launder
  // CHECK: store i32* [[TMP]],
```


================
Comment at: test/CodeGenCXX/builtin-launder.cpp:16
+extern "C" void test_builtin_launder_virtual_fn(TestVirtualFn *p) {
+  // CHECK: entry
+  // CHECK-NEXT: %p.addr = alloca [[TYPE:%.*]], align 8
----------------
This is likewise likely to fail with a release build of clang.


================
Comment at: test/SemaCXX/builtins.cpp:120
+  constexpr int i = 42;
+  // FIXME: Should this work? Since `&i` doesn't.
+  static_assert(test_in_constexpr(i), "");
----------------
`&i` doesn't what?


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

https://reviews.llvm.org/D40218