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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:895
+  if (!Self.getLangOpts().RTTIData) {
+    bool isMSVC = Self.getDiagnostics().getDiagnosticOptions().getFormat() ==
+                  DiagnosticOptions::MSVC;
----------------
hans wrote:
> zequanwu wrote:
> > hans wrote:
> > > zequanwu wrote:
> > > > hans wrote:
> > > > > zequanwu wrote:
> > > > > > hans wrote:
> > > > > > > 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;
> > > > > > > }
> > > > > > > ```
> > > > > > 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.
> > > > > 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.
> > > > 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
> > > > 
> > > 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.
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86369