[LLVM][Casting.h] Fix dyn_cast for std::unique_ptr.

[zuban32]

Summary

Unlike isa<> and cast<>, current implementation of dyn_cast<> fails
to process a std::unique_ptr to a class supporting LLVM RTTI:

// A and B support LLVM RTTI
class A {...}
class B: public A {...}

void foo() {
...

auto V = std::make_unique<A>();
auto VB = dyn_cast<B>(std::move(V));
if (VB)
  ...

...
}

[zuban32]

Added reviewers based on the previous commits to this file, not sure who should be the right one though, so any review is welcome.

[lattner]

Makes sense. thanks!

[bzcheeseman]

llvm/include/llvm/Support/Casting.h

358

I think you want std::forward<std::unique_ptr<From>>(f)? - see here:

https://en.cppreference.com/w/cpp/utility/forward

For example, if used in a wrapper such as the following, the template behaves as described below:
template<class T>
void wrapper(T&& arg)
{
    // arg is always lvalue
    foo(std::forward<T>(arg)); // Forward as lvalue or as rvalue, depending on T
}
If a call to wrapper() passes an rvalue std::string, then T is deduced to std::string (not std::string&, const std::string&, or std::string&&), and std::forward ensures that an rvalue reference is passed to foo.
If a call to wrapper() passes a const lvalue std::string, then T is deduced to const std::string&, and std::forward ensures that a const lvalue reference is passed to foo.
If a call to wrapper() passes a non-const lvalue std::string, then T is deduced to std::string&, and std::forward ensures that a non-const lvalue reference is passed to foo.

[bzcheeseman]

Are there unit tests you could add to this to ensure we don't regress? Clearly we don't have good enough coverage on this.

[zuban32]

Address the comment, add unit test

[zuban32]

Rebase

[zuban32]

Correct merge base

[zuban32]

@bzcheeseman pls take a look if the unit test looks good to you. If so I'll go ahead and merge the change, the buildbot failure seems to be unrelated and fixed by revert commit e08af170736271f022b2cab44d58326356ce1db8

[bzcheeseman]

The test looks fine to me, but I'm now realizing some tricky behavior. If you std::move a unique pointer into a dyn_cast, and casting fails, you now have no pointer anymore (it's been moved already). In the unit test, if BP couldn't be cast to foo I believe you'd see this behavior. I vaguely recall hitting some of this in the past, does it make sense to have UniquePtrCast take unique_ptr<> & rather than unique_ptr<> && and use .release()/.reset()?

[zuban32]

In D147991#4260050, @bzcheeseman wrote:

The test looks fine to me, but I'm now realizing some tricky behavior. If you std::move a unique pointer into a dyn_cast, and casting fails, you now have no pointer anymore (it's been moved already). In the unit test, if BP couldn't be cast to foo I believe you'd see this behavior. I vaguely recall hitting some of this in the past, does it make sense to have UniquePtrCast take unique_ptr<> & rather than unique_ptr<> && and use .release()/.reset()?

Oh, you're right, haven't tried that yet but I'm pretty sure that's what's going to happen. I think we could just make dyn_cast take lvalue reference to unique_ptr instead of an rvalue one, then check it with isa<>, and then either move the pointer further or just return nullptr leaving the original pointer intact. I'll update the patch

[bzcheeseman]

In D147991#4260252, @zuban32 wrote:

Oh, you're right, haven't tried that yet but I'm pretty sure that's what's going to happen. I think we could just make dyn_cast take lvalue reference to unique_ptr instead of an rvalue one, then check it with isa<>, and then either move the pointer further or just return nullptr leaving the original pointer intact. I'll update the patch

Thanks! I think we want dyn_cast to consume its input *if it works* and leave it alone if not. Would you mind also dropping a comment to this effect, please?

[zuban32]

• Add failed cast test

[zuban32]

@bzcheeseman I've updated the test and it passes, i.e. I wasn't able to reproduce the behavior we discussed. It seems modern c++ compilers are able to elide the calls to move constructor in such a code, I tried with gcc 9.4 and Apple aarch64 clang-14 with no success.

[bzcheeseman]

In D147991#4276650, @zuban32 wrote:

@bzcheeseman I've updated the test and it passes, i.e. I wasn't able to reproduce the behavior we discussed. It seems modern c++ compilers are able to elide the calls to move constructor in such a code, I tried with gcc 9.4 and Apple aarch64 clang-14 with no success.

This makes me a bit nervous because it feels like undefined behavior unless there's some part of the standard that allows something like this. Do these tests run under ASan/UBSan? Also, I assume you don't get any warnings about use-after-move?

[bzcheeseman]

@zuban32 Not sure if you were waiting on me (if so, sorry!) - I put some comments inline that would help with the concerns we talked about previously. I'd love to get this landed once the comments are resolved :)

llvm/unittests/Support/Casting.cpp

227

Would it make sense to check something like

base *BP2Ptr = BP2.get();
EXPECT_NE(BP2Ptr, nullptr);
auto DP = dyn_cast<derived_nocast>(std::move(BP2));
EXPECT_EQ(DP.get(), nullptr);
EXPECT_NE(BP2.get(), nullptr);
EXPECT_EQ(BP2.get(), BP2Ptr);

?

That would assuage my concern, I think, as this at least ensures we have the exact behavior we expect on failure.

228

I don't believe this is necessary? I think we want it to be deallocated.

[zuban32]

In D147991#4335333, @bzcheeseman wrote:

@zuban32 Not sure if you were waiting on me (if so, sorry!) - I put some comments inline that would help with the concerns we talked about previously. I'd love to get this landed once the comments are resolved :)

Absolutely not, I would surely ping you it was about that :) I'm sorry, this got retracted by other things. I was planning to run it with the sanitizer to see if something shows up.

[zuban32]

llvm/unittests/Support/Casting.cpp

228

I was just following the chunk above, I think gtest reports some memory issue should we omit the release(). I don't remember why, let me take a look

[bzcheeseman]

In D147991#4339596, @zuban32 wrote:

<snip>
Absolutely not, I would surely ping you it was about that :) I'm sorry, this got retracted by other things. I was planning to run it with the sanitizer to see if something shows up.

Alright great - I also got busy so I wasn't sure if I missed something! Definitely feel free to ping me if needed.

[bzcheeseman]

llvm/unittests/Support/Casting.cpp

228

Ah interesting. Thanks for looking into it!

[zuban32]

• Make it reliable for unsuccessful casting cases, fix the test

[zuban32]

llvm/unittests/Support/Casting.cpp

227

I've changed the behavior to do the moving only after the casting is proved possible, I haven't noticed any changes in the actual behavior though, both the test above and the actual usecase I needed this for.

228

@bzcheeseman sorry for abandoning this for a while, just made the changes we discussed. release() above was needed because asan couldn't understand where the original pointer came from, after allocating it locally the ptrs are deleted just alright.

[zuban32]

@bzcheeseman ping

[bzcheeseman]

Thank you for iterating! LGTM

llvm/include/llvm/Support/Casting.h

static inline CastResultType doCast(std::unique_ptr<From> &&f) {
     return CastResultType((typename CastResultType::element_type *)f.release());
   }
 
   static inline CastResultType castFailed() { return CastResultType(nullptr); }
 
   static inline CastResultType doCastIfPossible(std::unique_ptr<From> &&f) {
     if (!Self::isPossible(f.get()))
       return castFailed();
-    return doCast(std::forward<std::unique_ptr<From> &&>(f));
+    return doCast(std::forward<std::unique_ptr<From>>(f));
   }
 };
 
 /// This cast trait provides std::optional<T> casting. This means that if you
 /// have a value type, you can cast it to another value type and have dyn_cast
 /// return an std::optional<T>.
 template <typename To, typename From, typename Derived = void>
 struct OptionalValueCast

llvm/unittests/Support/Casting.cpp

TEST(CastingTest, dyn_cast) {
   EXPECT_NE(F2, null_foo);
   const foo *F3 = dyn_cast<foo>(B4);
   EXPECT_NE(F3, null_foo);
   // Can't pass null pointer to dyn_cast<>.
   // foo *F4 = dyn_cast<foo>(fub());
   // EXPECT_EQ(F4, null_foo);
   foo *F5 = B1.daz();
   EXPECT_NE(F5, null_foo);
+
+  std::unique_ptr<const bar> BP(B2);
+  auto FP = dyn_cast<foo>(std::move(BP));
+  static_assert(std::is_same_v<std::unique_ptr<const foo>, decltype(FP)>,
+                "Incorrect deduced return type!");
+  EXPECT_NE(FP.get(), null_foo);
+  EXPECT_EQ(BP.get(), nullptr);
+  FP.release();
 }
 
 // All these tests forward to dyn_cast_if_present, so they also provde an
 // effective test for its use cases.
 TEST(CastingTest, dyn_cast_or_null) {
   const foo *F1 = dyn_cast_or_null<foo>(B2);
   EXPECT_NE(F1, null_foo);
   const foo *F2 = dyn_cast_or_null<foo>(B2);
   EXPECT_NE(F2, null_foo);
   const foo *F3 = dyn_cast_or_null<foo>(B4);
   EXPECT_NE(F3, null_foo);
   foo *F4 = dyn_cast_or_null<foo>(fub());
   EXPECT_EQ(F4, null_foo);
   foo *F5 = B1.naz();