[PATCH] D40218: [Clang] Add __builtin_launder

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 22 inline comments as done.
EricWF 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:
+  //
----------------
rsmith wrote:
> 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.
Apparently I misread the specification. The blanket wording in [[res.on.functions](http://eel.is/c++draft/res.on.functions#2.5)]  already prohibits this.

Though it's worth noting that GCC doesn't enforce this requirement (nor the one about function pointers).


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1451
+    return false;
+  llvm::DenseSet<const Decl *> Seen;
+  return TypeRequiresBuiltinLaunderImp(CGM.getContext(), Ty, Seen);
----------------
rsmith wrote:
> Would `SmallPtrSet` be a better choice here?
I have no idea. I'm ignorant to the internals of both containers. Since you asked the question I'll assume it is and make the change.
If you would like, I can do further investigation upon request.


================
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);
----------------
rsmith wrote:
> 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".
Ack.

I think I've implemented what you requested. Could you please verify?


================
Comment at: lib/Sema/SemaChecking.cpp:935
   return true;
+
 }
----------------
rsmith wrote:
> Did you mean to add this blank line?
Oh Richard, you're too kind. Of course I didn't mean to do that.


================
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
----------------
rsmith wrote:
> 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]],
> ```
If you're referring to the behavior of discarding value names, Clang CC1 only does that if `-discard-value-names` is explicitly passed, regardless of how the compiler is built, and so the names should be stable. (The Clang driver still conditionally passes `-discard-value-names` in release builds).

Are there other IR changes you have in mind?

None the less, I'll simply this test as requested. 


================
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
----------------
rsmith wrote:
> This is likewise likely to fail with a release build of clang.
I just verified that they pass in both release and debug builds. I previously changed Clang and LLVM to make the label names consistent between the two configurations for the purposes of testing.

I'll still attempt to clean unnecessary checks up.


================
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), "");
----------------
rsmith wrote:
> `&i` doesn't what?
That's a great question. Not sure what I was up to when I wrote that. I tried a couple of things I though I might have been thinking, but they all passed.

Removing the comment.


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

https://reviews.llvm.org/D40218