Page MenuHomePhabricator

[Clang] Add __builtin_launder
AcceptedPublic

Authored by EricWF on Nov 18 2017, 3:03 PM.

Details

Summary

This patch adds __builtin_launder, which is required to implement std::launder. Additionally GCC provides __builtin_launder, so thing brings Clang in-line with GCC.

I'm not exactly sure what magic __builtin_launder requires, but based on previous discussions this patch applies a @llvm.invariant.group.barrier. As noted in previous discussions, this may not be enough to correctly handle vtables.

Diff Detail

Event Timeline

EricWF created this revision.Nov 18 2017, 3:03 PM
EricWF updated this revision to Diff 123484.Nov 18 2017, 3:05 PM
  • Remove incorrect FIXME comment.
majnemer edited edge metadata.Nov 19 2017, 12:20 PM

A test with restrict and __restrict might be interesting.

jroelofs added inline comments.
include/clang/Basic/Builtins.def
501

GCC's is type-generic:

#include <type_traits>

bool is_type_generic() {
  int v;
  return std::is_same<decltype(__builtin_launder(&v)),
                      decltype(&v)>::value;
}

However this BUILTIN line says that this one is not (yeah, I see it's being fixed up in Sema later in this patch). I think you need to use custom type checking here via t.

EricWF added inline comments.Nov 19 2017, 2:12 PM
include/clang/Basic/Builtins.def
501

The t is specified. It's in the third argument.

jroelofs added inline comments.Nov 19 2017, 2:13 PM
include/clang/Basic/Builtins.def
501

ohhh. duh. sorry.

EricWF updated this revision to Diff 123516.Nov 19 2017, 2:23 PM
  • Fix argument initialization.
  • Make constexpr.
EricWF updated this revision to Diff 123627.Nov 20 2017, 11:20 AM
  • Improve quality of tests.
  • Format code.
rsmith added inline comments.Dec 7 2017, 3:02 PM
lib/CodeGen/CGBuiltin.cpp
1674

It would be nice to avoid this for types that contain no const subobjects / reference subobjects / vptrs. I think we can also omit this entirely if -fstrict-vtable-ptrs is disabled, since in that case we don't generate any invariant.group metadata.

I'd be OK with the former being left to a future change, but the latter should be part of this change so we don't generate unnecessarily-inefficient code in the default mode for uses of std::launder.

lib/Sema/SemaChecking.cpp
890–894

Please also check that the pointee type is an object type -- per [ptr.launder]p3, "the program is ill-formed if T is a function type or cv void", and we don't want our builtin to need to deal with such cases.

EricWF marked 2 inline comments as done.Jan 12 2018, 5:23 PM
EricWF added inline comments.
lib/CodeGen/CGBuiltin.cpp
1674

I'll put it in this change set.

EricWF updated this revision to Diff 129740.Jan 12 2018, 5:25 PM
EricWF marked an inline comment as done.
  • Address inline comments about missing diagnostics for void pointers and function pointers.
  • Address inline comments about only enabling when -fstrict-vtable-pointers is specified, and only on types with vtables.
EricWF updated this revision to Diff 129742.Jan 12 2018, 5:49 PM
  • Improve diagnostic handling.
EricWF updated this revision to Diff 133290.Feb 7 2018, 1:17 PM
  • Rebase off master.
rsmith added inline comments.Feb 23 2018, 2:44 PM
lib/CodeGen/CGBuiltin.cpp
1947–1948

I think you also need to catch class types that contain dynamic classes as subobjects.

lib/Sema/SemaChecking.cpp
890

Might be a bit clearer to use llvm::Optional<unsigned> as the type of this.

test/CodeGenCXX/builtin-launder.cpp
94–97

I would note that this means adding optimizations for those cases later is an LTO ABI break. That's probably OK, but just something we're going to need to remember.

EricWF marked an inline comment as done.Mar 21 2018, 1:25 PM
EricWF added inline comments.
lib/CodeGen/CGBuiltin.cpp
1947–1948

Only by-value subobjects, or also reference subobjects?

EricWF marked 2 inline comments as done.Mar 21 2018, 1:46 PM
EricWF added inline comments.
test/CodeGenCXX/builtin-launder.cpp
94–97

I added your note almost verbatim to the test case.

EricWF updated this revision to Diff 139363.Mar 21 2018, 1:48 PM
EricWF marked an inline comment as done.
  • Launder types with subobjects of dynamic class type.
  • Improve diagnostic selection using llvm::Optional<unsigned>.
  • Add comment about LTO ABI concerns to test file.
  • Merge with master.
rsmith accepted this revision.May 14 2018, 11:34 AM

LGTM with a fix (and test) for pointer-to-array-of-dynamic-class-type handling.

lib/CodeGen/CGBuiltin.cpp
1326–1328

I think you also need to recurse through array types (use Ty->getBaseElementTypeUnsafe()->getAsCXXRecordDecl() or similar).

This revision is now accepted and ready to land.May 14 2018, 11:34 AM

Also we probably want to hold off on landing this until PR37458 is fixed, otherwise std::launder will start miscompiling code.

amharc added a subscriber: amharc.May 16 2018, 12:28 AM
EricWF updated this revision to Diff 147639.May 18 2018, 7:37 PM
  • Handle array types as requested.
  • Attempt to handle incomplete class types. They are always considered in need of laundering. Including class templates who's instantiation hasn't been forced yet. @rsmith is this the correct thing to do?

Bump! I don't see anything here that seems questionable (and this has extensive testing), but I presume @rsmith should be the final one to approve.

Hi,

Is there any news on this code review ? Is it ready to land ?

Cheers,
Romain

EricWF updated this revision to Diff 174109.Wed, Nov 14, 3:16 PM

Merge with upstream.

Is there any news on this code review ? Is it ready to land ?

I think so, but there were some changes regarding incomplete types since Richard last looked at it.
I wanted him to sign off on the new behavior.

lib/CodeGen/CGBuiltin.cpp
1037

@rsmith I believe this changed since you approved the patch.

We talked about this offline, but could you give it a thumbs up?

rsmith added inline comments.Mon, Dec 3, 1:09 PM
lib/CodeGen/CGBuiltin.cpp
1425–1426

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.

1437–1438

Delete this commented-out code.

1451

Would SmallPtrSet be a better choice here?

lib/Sema/SemaChecking.cpp
888–890

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

955

Did you mean to add this blank line?

test/CodeGen/builtins.c
404–409

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]],
test/CodeGenCXX/builtin-launder.cpp
17

This is likewise likely to fail with a release build of clang.

test/SemaCXX/builtins.cpp
120

&i doesn't what?

EricWF marked 22 inline comments as done.Sun, Dec 9, 1:35 PM
EricWF added inline comments.
lib/CodeGen/CGBuiltin.cpp
1425–1426

Apparently I misread the specification. The blanket wording in [res.on.functions] already prohibits this.

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

1451

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.

lib/Sema/SemaChecking.cpp
888–890

Ack.

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

955

Oh Richard, you're too kind. Of course I didn't mean to do that.

test/CodeGen/builtins.c
404–409

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.

test/CodeGenCXX/builtin-launder.cpp
17

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.

test/SemaCXX/builtins.cpp
120

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.

EricWF updated this revision to Diff 177445.Sun, Dec 9, 1:40 PM
EricWF marked 6 inline comments as done.

Address review comments.

@rsmith I think this is good to go. Just need your input on one change.