[Sema][MSVC] warn at dynamic_cast when /GR- is given

[lebedev.ri]

clang/test/SemaCXX/ms_dynamic_cast.cpp

16

I'm not sure it makes sense to talk about MSVC/clang-cl flags when normal clang is used.
Either the diag should only be used in MSVC compat mode,
or it should talk about -fno-rtti in non-MSVC compat mode.

[zequanwu]

clang/test/SemaCXX/ms_dynamic_cast.cpp

16

I am not sure either... But -fno-rtti-data is only used when clang-cl driver translate /GR- to -fno-rtti-data.

[hans]

clang/test/SemaCXX/ms_dynamic_cast.cpp

16

Instead of "should not use" maybe the warning could say something which suggests it won't work, like "dynamic_cast will not work since RTTI data is disabled" or something like that.

I'm not sure what to do about the name of the option. -fno-rtti-data was added to support clang-cl's /GR- option, but it could also be used with regular Clang. Maybe the warning doesn't need to mention the name of the flag? Or it could mention both? Or maybe it could figure out if it's in clang-cl or regular clang mode?

Also, should it also warn about typeid()?

[zequanwu]

address comments.

[hans]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7436

The other warning names spell rtti in lower-case, so I'd suggest that here too for consistency.

clang/lib/Sema/SemaCast.cpp

895

Is the isIgnored() check necessary here? I think in general we call Diag() anyway, and if the warning is ignored, it will be ignored.

The reason we sometimes check isIgnored() explicitly is to avoid expensive computation when possible, but that's not the case here.

900

getLangOpts().MSVCCompat can be true both with clang-cl and regular clang, it just depends on teh -fms-compatibility flag.

Driver::IsCLMode() is the sure way to check for clang-cl, but I don't think that information is forwarded to the compiler frontend.

One slightly hacky way to check, and in this case maybe it makes sense, is to look at the TextDiagnosticFormat. If that's MSVC, it's very likely the driver was clang-cl.

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

1 ↗

(On Diff #287835)

When using %clang_cl, the source file should always come after a "--", otherwise if for example the source file is "/Users/foo/src/test.cc" the filename can get interpreted as a command-line option.

But tests outside of Driver/ generally invoke cc1 directly, with %clang_cc1 and passing the appropriate flags. I'd suggest doing that here too. (And in the other test.)

[zequanwu]

Address cmments.

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

1 ↗

(On Diff #287835)

This test is for testing in clang-cl. Another test is for clang. If I use %clang_cc1, /GR- can not be passed.

[hans]

clang/lib/Sema/SemaCast.cpp

895

I'm not sure isMSVC is the best name (it's not clear what aspect is MSVC exactly -- in this case it's the diagnostics format).

It's possible to target MSVC both with clang-cl and with regular clang.

For example, one could use

clang-cl /c /tmp/a.cpp

or

clang -c /tmp/a.cpp -target i686-pc-windows-msvc19.11.0 -fms-extensions

My understanding is that the purpose of "isMSVC" here is to try and detect if we're using clang-cl or clang so that the diagnostic can say "/GR-" or "-fno-rtti-data". So maybe it's better to call it "isClangCL" or something like that.

Also, I don't think we should check "isMSVC" in the if-statement below. We want the warning to fire both when using clang and clang-cl: as long as -fno-rtti-data or /GR- is used, the warning makes sense.

So I think the code could be more like:

if (!Self.getLangOpts().RTTIData && !DestPointee->isVoidType()) {
  bool isClangCL = ...;
  Self.Diag(...) << isClangCL;
}

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

1 ↗

(On Diff #287835)

But clang-cl is just a driver mode, all it does is pass flags to the cc1 invocation. So normally we test the driver separately (in Driver/) and the compiler (cc1) separately.

In this case the warning lives in the compiler (cc1) so it makes sense to target that directly with the test. If you grep through clang/test/Sema and clang/test/SemaCXX, you'll see that %clang_cl is not use in any test.

You should be able to run cc1 with and without "-fdiagnostics-format msvc" and -fno-rtti-data to test the new warnings.

[zequanwu]

clang/lib/Sema/SemaCast.cpp

895

MSVC will warn even if the DestPointee is void type. What I thought is if invoked by clang-cl warn regardless of DeskPointee type. If invoked by clang, warn if it's not void type.
https://godbolt.org/z/475q5v. I noticed MSVC won't warn at typeid if /GR- is given. Probably I should remove the warning in typeid.

[zequanwu]

address comments.

[hans]

clang/lib/Sema/SemaCast.cpp

895

If it's true the casting to void* doesn't need RTTI data (I think it is, but would be good to verify), then it doesn't make sense to warn. We don't have to follow MSVC's behavior when it doesn't make sense :)

Similar reasoning for typeid() - I assume it won't work with /GR- also with MSVC, so warning about it probably makes sense.

[zequanwu]

clang/lib/Sema/SemaCast.cpp

895

In clang, I believe that dynamic_cast to void* doesn't use RTTI data: https://godbolt.org/z/Kbr7Mq
Looks like MSVC only warns if the operand of typeid is not pointer: https://godbolt.org/z/chcMcn

[hans]

clang/lib/Sema/SemaCast.cpp

895

When targeting Windows, dynamic_cast to void* is implemented with in a runtime function, RTCastToVoid: https://godbolt.org/z/Kecr7z
I wonder if that uses RTTI data internally though...

For typeid() I guess it would also warn on references? Maybe we should do the same.

[zequanwu]

clang/lib/Sema/SemaCast.cpp

895

Couldn't find if __RTCastToVoid uses RTTI data internally.

For typeid(), it also warn on references. But the behavior is a bit weird (https://godbolt.org/z/jn4Pjx). Seems like it warns only when dereferencing a pointer or argument is a reference.

[hans]

clang/lib/Sema/SemaCast.cpp

895

Okay, maybe it's safest to warn about dynamic_cast also for void*, like MSVC does.

For typeid() maybe we should be conservative like Clang is with -fno-rtti, and always warn.

But I'm a little bit surprised about this, because I'd imagine typeid(int) could work also with -fno-rtti... if you're curious maybe it would be interesting to dig deeper into how this works.

[zequanwu]

warn at dynamic_cast<void*> like MSVC in clang-cl, but not in clang.

[zequanwu]

clang/lib/Sema/SemaCast.cpp

895

It might just because typeid is a rtti-only thing when introduced, https://en.cppreference.com/w/cpp/types. gcc also gives error for this.

[hans]

Okay, almost there..

clang/lib/Sema/SemaCast.cpp

897

I don't think the if-statement is necessary. I thought we concluded we want to warn even for void*? Also note that "isMSVC" here is only checking the driver mode, i.e. just the user interface to the compiler. The user could still be compiling in MSVC mode.

clang/lib/Sema/SemaExprCXX.cpp

649 ↗

(On Diff #289603)

s/RTTI/RTTI data/
(the "RTTI disabled" case is on line 646)

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

19 ↗

(On Diff #289603)

Is the cast to void necessary? Same comment for the next file.

[zequanwu]

address comment.

clang/lib/Sema/SemaCast.cpp

897

My bad. I thought you meant to use the previous version, so I reverted this region.
Like we concluded, we want to warn even for void*. This only applies to clang-cl, not clang, right? This is the purpose of this if. If it's in clang-cl, warn regardless of destination type. If it's in clang, don't warn for void*, like line 887 which doesn't emit error for void* destination type.

If RTTI is false, RTTIData is also false (https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/CompilerInvocation.cpp#L2870). There is a test https://github.com/llvm/llvm-project/blob/master/clang/test/SemaCXX/no-rtti.cpp#L28, which should not be warned, right?

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

19 ↗

(On Diff #289603)

Yes, to suppress the warning of unused expression.

[hans]

clang/lib/Sema/SemaCast.cpp

897

Like we concluded, we want to warn even for void*. This only applies to clang-cl, not clang, right?

Oh, right, we need to do the "even for void*" part only when using the Microsoft ABI. It's not about clang or clang-cl, those are just different user interfaces really, but about what platform (processor, abi, etc.) the compiler is targeting. It's possible to target Windows with regular clang, not just clang-cl.

Inside Sema, you can check Context.getTargetInfo().getCXXABI().isMicrosoft() to see if the microsoft abi is being targeted.

Apologies for my confusing comments here.

clang/test/SemaCXX/ms_no_dynamic_cast.cpp

19 ↗

(On Diff #289603)

Ah, okay. Didn't realize that warning was on by default :)

[zequanwu]

address comment.

[hans]

LGTM

But please add a -triple parameter to the test files, and check the dynamic_cast<void*> behavior in each case before committing.

[zequanwu]

reopen the diff and update.

[hans]

lgtm

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_invalid_declarator_in_block : Error<
   "definition or redeclaration of %0 not allowed inside a block">;
 def err_not_tag_in_scope : Error<
   "no %select{struct|interface|union|class|enum}0 named %1 in %2">;
 
 def err_no_typeid_with_fno_rtti : Error<
   "use of typeid requires -frtti">;
 def err_no_dynamic_cast_with_fno_rtti : Error<
   "use of dynamic_cast requires -frtti">;
+def warn_no_dynamic_cast_with_no_GR: Warning<
+  "should not use dynamic_cast with /GR-">;
 
 def err_cannot_form_pointer_to_member_of_reference_type : Error<
   "cannot form a pointer-to-member to member %0 of reference type %1">;
 def err_incomplete_object_call : Error<
   "incomplete type in call to object of type %0">;
 
 def warn_condition_is_assignment : Warning<"using the result of an "
   "assignment as a condition without parentheses">,

clang/lib/Sema/SemaCast.cpp

void CastOperation::CheckDynamicCast() {
   // As an exception, dynamic_cast to void* is available because it doesn't
   // use RTTI.
   if (!Self.getLangOpts().RTTI && !DestPointee->isVoidType()) {
     Self.Diag(OpRange.getBegin(), diag::err_no_dynamic_cast_with_fno_rtti);
     SrcExpr = ExprError();
     return;
   }
 
+  // MSVC warns when dynamic_cast is used with /GR-.
+  if (!Self.getLangOpts().RTTIData) {
+    Self.Diag(OpRange.getBegin(), diag::warn_no_dynamic_cast_with_no_GR);
+  }
+
   // Done. Everything else is run-time checks.
   Kind = CK_Dynamic;
 }
 
 /// CheckConstCast - Check that a const_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.11 for details. const_cast is typically used in code
 /// like this:
 /// const char *str = "literal";
 /// legacy_function(const_cast\<char*\>(str));
 void CastOperation::CheckConstCast() {
   CheckNoDerefRAII NoderefCheck(*this);

clang/test/SemaCXX/ms_dynamic_cast.cpp

+// RUN:  %clang_cc1 %s -fno-rtti-data -fsyntax-only -verify
+
+class B {
+public:
+    virtual ~B() = default;
+};
+
+class D1 : public B {
+public:
+    ~D1() = default;
+
+};
+
+void  f() {
+    B *b = new D1();
+    auto d = dynamic_cast<D1 *>(b); // expected-warning{{should not use dynamic_cast with /GR-}}
+}
 No newline at end of file