[LLVM][Casting.h] Don't create a temporary while casting.

[bzcheeseman]

Summary

C-style casting can create a temporary when compiled by a C++ compiler, which was emitting a warning casting a reference to another reference. We can't use C++-style casting directly because it doesn't always work with incomplete types. In order to support the current use-cases, for references we switch to pointer space to perform the cast.

[bzcheeseman]

@qiongsiwu1 or @uabelho please let me know if this fixes the issue you saw. I don't think I see the warning emitted anywhere I was able to repro.

[qiongsiwu1]

In D125482#3509800, @bzcheeseman wrote:

@qiongsiwu1 or @uabelho please let me know if this fixes the issue you saw. I don't think I see the warning emitted anywhere I was able to repro.

Thanks for the fast response @bzcheeseman ! I've tested this patch locally on a ppc machine. The warning is no longer there and all lit tests pass.

[bzcheeseman]

In D125482#3509880, @qiongsiwu1 wrote:

In D125482#3509800, @bzcheeseman wrote:

@qiongsiwu1 or @uabelho please let me know if this fixes the issue you saw. I don't think I see the warning emitted anywhere I was able to repro.

Thanks for the fast response @bzcheeseman ! I've tested this patch locally on a ppc machine. The warning is no longer there and all lit tests pass.

Great to hear! Once I get all green in CI I'll land this patch.

[Jake-Egan]

Hi, we're also getting the original error on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/1255/steps/6/logs/stdio

I applied your patch here and it seems to fix the original error, but there is a new one:

/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:69:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(D);
  ^~~~~~~~~~~~~~~~ ~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:70:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(BD);
  ^~~~~~~~~~~~~~~~ ~~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:71:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyDerivedType>(BD);
  ^~~~~~~~~~~~~~~~~~~ ~~

[bzcheeseman]

In D125482#3510066, @Jake-Egan wrote:

Hi, we're also getting the original error on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/1255/steps/6/logs/stdio

I applied your patch here and it seems to fix the original error, but there is a new one:

/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:69:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(D);
  ^~~~~~~~~~~~~~~~ ~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:70:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(BD);
  ^~~~~~~~~~~~~~~~ ~~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:71:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyDerivedType>(BD);
  ^~~~~~~~~~~~~~~~~~~ ~~

This commit went in last night it looks like to fix that: 52328dafda13711653fd15aa4db34db350471a1f

[Jake-Egan]

In D125482#3510072, @bzcheeseman wrote:

In D125482#3510066, @Jake-Egan wrote:

Hi, we're also getting the original error on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/1255/steps/6/logs/stdio

I applied your patch here and it seems to fix the original error, but there is a new one:

/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:69:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(D);
  ^~~~~~~~~~~~~~~~ ~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:70:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(BD);
  ^~~~~~~~~~~~~~~~ ~~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:71:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyDerivedType>(BD);
  ^~~~~~~~~~~~~~~~~~~ ~~

This commit went in last night it looks like to fix that: 52328dafda13711653fd15aa4db34db350471a1f

Oh my mistake, my branch was only up to the other commit. Thank you

[bzcheeseman]

In D125482#3510080, @Jake-Egan wrote:

In D125482#3510072, @bzcheeseman wrote:

In D125482#3510066, @Jake-Egan wrote:

Hi, we're also getting the original error on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/1255/steps/6/logs/stdio

I applied your patch here and it seems to fix the original error, but there is a new one:

/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:69:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(D);
  ^~~~~~~~~~~~~~~~ ~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:70:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyBaseType>(BD);
  ^~~~~~~~~~~~~~~~ ~~
/llvm-project/llvm/unittests/Support/ExtensibleRTTITest.cpp:71:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  cast<MyDerivedType>(BD);
  ^~~~~~~~~~~~~~~~~~~ ~~

This commit went in last night it looks like to fix that: 52328dafda13711653fd15aa4db34db350471a1f

Oh my mistake, my branch was only up to the other commit. Thank you

No worries! Thanks for bringing it up and repro-ing the fix :)

[qiongsiwu1]

LGTM! Thanks!

[bzcheeseman]

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

[qiongsiwu1]

In D125482#3510322, @bzcheeseman wrote:

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

Good call - no I did not run the whole test suite. I did run ninja check-all. Let me give the test suite a try as well.

[bzcheeseman]

In D125482#3510327, @qiongsiwu1 wrote:

In D125482#3510322, @bzcheeseman wrote:

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

Good call - no I did not run the whole test suite. I did run ninja check-all. Let me give the test suite a try as well.

That's what I meant. I think build kite is running ninja check-clang-tools check-bolt check-mlir check-flang check-all check-lld check-openmp check-clang check-polly check-libc check-llvm

build kite is being flaky right now so I'd love a clean bill of health before landing to unblock folks.

[qiongsiwu1]

In D125482#3510328, @bzcheeseman wrote:

In D125482#3510327, @qiongsiwu1 wrote:

In D125482#3510322, @bzcheeseman wrote:

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

Good call - no I did not run the whole test suite. I did run ninja check-all. Let me give the test suite a try as well.

That's what I meant. I think build kite is running ninja check-clang-tools check-bolt check-mlir check-flang check-all check-lld check-openmp check-clang check-polly check-libc check-llvm

build kite is being flaky right now so I'd love a clean bill of health before landing to unblock folks.

Ah I see. I tested check-all, but I did not build flang/openmp/polly/libc for my local testing. Let me give those a go instead of running the "test-suite".

[bzcheeseman]

In D125482#3510332, @qiongsiwu1 wrote:

In D125482#3510328, @bzcheeseman wrote:

In D125482#3510327, @qiongsiwu1 wrote:

In D125482#3510322, @bzcheeseman wrote:

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

Good call - no I did not run the whole test suite. I did run ninja check-all. Let me give the test suite a try as well.

That's what I meant. I think build kite is running ninja check-clang-tools check-bolt check-mlir check-flang check-all check-lld check-openmp check-clang check-polly check-libc check-llvm

build kite is being flaky right now so I'd love a clean bill of health before landing to unblock folks.

Ah I see. I tested check-all, but I did not build flang/openmp/polly/libc for my local testing. Let me give those a go instead of running the "test-suite".

Many thanks - I assume your build machine is better than mine :)

I got check-clang and check-llvm on my machine and both are all good (which is usually a good sign)

[qiongsiwu1]

In D125482#3510333, @bzcheeseman wrote:

In D125482#3510332, @qiongsiwu1 wrote:

In D125482#3510328, @bzcheeseman wrote:

In D125482#3510327, @qiongsiwu1 wrote:

In D125482#3510322, @bzcheeseman wrote:

@qiongsiwu1 I assume you ran the whole test suite with no problems? My machine is rather slow so running check-all takes ~many hours for me

Good call - no I did not run the whole test suite. I did run ninja check-all. Let me give the test suite a try as well.

That's what I meant. I think build kite is running ninja check-clang-tools check-bolt check-mlir check-flang check-all check-lld check-openmp check-clang check-polly check-libc check-llvm

build kite is being flaky right now so I'd love a clean bill of health before landing to unblock folks.

Ah I see. I tested check-all, but I did not build flang/openmp/polly/libc for my local testing. Let me give those a go instead of running the "test-suite".

Many thanks - I assume your build machine is better than mine :)

I got check-clang and check-llvm on my machine and both are all good (which is usually a good sign)

No problem! The ppc buildbot (e.g. https://lab.llvm.org/buildbot/#/builders/57/builds/17829/steps/6/logs/stdio) is only testing clang;llvm;clang-tools-extra;lld, and that was what included in my runs. Those were green!

My local OpenMP tests are having some hiccups but it does not seem to have anything to do with this patch. Bolt is not supported on PPC. polly, mlir and flang checks are green. So I am pretty confident that we are good for Linux on Power. @Jake-Egan can comment more on on AIX but it seems we are good there as well.

[Jake-Egan]

check-all is green on AIX.

[bzcheeseman]

Finally got green on Debian build (seems like check-all on build kite only works at night?) https://buildkite.com/llvm-project/premerge-checks/builds/92657#5c473a5a-6e3d-4247-a307-30df72a6482d

I'll land this to unblock folks, thanks all for helping test!

llvm/include/llvm/Support/Casting.h

template <class To, class From, class SimpleFrom> struct cast_convert_val {
   static typename cast_retty<To, From>::ret_type doit(const From &Val) {
     return cast_convert_val<To, SimpleFrom,
                             typename simplify_type<SimpleFrom>::SimpleType>::
         doit(simplify_type<From>::getSimplifiedValue(const_cast<From &>(Val)));
   }
 };
 
 template <class To, class FromTy> struct cast_convert_val<To, FromTy, FromTy> {
-  // This _is_ a simple type, just cast it.
+  // If it's a reference, switch to a pointer to do the cast and then deref it.
   static typename cast_retty<To, FromTy>::ret_type doit(const FromTy &Val) {
-    typename cast_retty<To, FromTy>::ret_type Res2 =
-        (typename cast_retty<To, FromTy>::ret_type) const_cast<FromTy &>(Val);
-    return Res2;
+    return *(std::remove_reference_t<typename cast_retty<To, FromTy>::ret_type>
+                 *)&const_cast<FromTy &>(Val);
+  }
+};
+
+template <class To, class FromTy>
+struct cast_convert_val<To, FromTy *, FromTy *> {
+  // If it's a pointer, we can use c-style casting directly.
+  static typename cast_retty<To, FromTy *>::ret_type doit(const FromTy *Val) {
+    return (typename cast_retty<To, FromTy *>::ret_type) const_cast<FromTy *>(
+        Val);
   }
 };
 
 //===----------------------------------------------------------------------===//
 // is_simple_type
 //===----------------------------------------------------------------------===//
 
 template <class X> struct is_simple_type {

llvm/unittests/Support/Casting.cpp

TEST(CastingTest, unique_dyn_cast) {
   ASSERT_EQ(nullptr, B);
 
   // Converting from B to D is valid, D should have a value and B should be
   // nullptr;
   D = unique_dyn_cast<derived>(NewB);
   ASSERT_EQ(OrigD, D.get());
   ASSERT_EQ(nullptr, NewB);
 
-  // Converting between unrelated types should fail.  The original value should
-  // remain unchanged and it should return nullptr.
-  //
-  // Note that this is a very contrived test - most of the time we want a cast
-  // like this to emit a compiler error.
+  // This is a very contrived test, casting between completely unrelated types
+  // should generally fail to compile. See the classof shenanigans we have in
+  // the definition of `foo` above.
   auto F = unique_dyn_cast<foo>(D);
   ASSERT_EQ(nullptr, F);
   ASSERT_EQ(OrigD, D.get());
 
   // All of the above should also hold for temporaries.
   auto D2 = unique_dyn_cast<derived>(newd());
   EXPECT_NE(nullptr, D2);
 
   auto B2 = unique_dyn_cast<derived>(newb());
   EXPECT_NE(nullptr, B2);
 
   auto B3 = unique_dyn_cast<base>(newb());
   EXPECT_NE(nullptr, B3);
 
+  // This is a very contrived test, casting between completely unrelated types
+  // should generally fail to compile. See the classof shenanigans we have in
+  // the definition of `foo` above.
   auto F2 = unique_dyn_cast<foo>(newb());
   EXPECT_EQ(nullptr, F2);
 }
 
 // These lines are errors...
 // foo *F20 = cast<foo>(B2);  // Yields const foo*
 // foo &F21 = cast<foo>(B3);  // Yields const foo&
 // foo *F22 = cast<foo>(B4);  // Yields const foo*