Page MenuHomePhabricator

[Clang][Diagnostic] Add hidden-reinterpret-cast diagnostic warning
Needs ReviewPublic

Authored by TWeaver on Oct 18 2022, 3:33 AM.

Details

Summary

There's an argument to be made that C++ functional casts are just as dangerous as old style C casts. They have the ability to perform reinterpret casts, causing potentially dangerous bugs in software. On top of this, C++ functional style casts have the same syntactic 'look' as a constructor call. This has the potential, especially when dealing with template code, for a hidden reinterpret cast to be embedded in code without an author realising.

This patch adds a new diagnostic and warning flag (-Whidden-reinterpret-cast) that will fire whenever a C++ functional style cast fails a static cast check but succeeds a reinterpret cast check. This should help identify unintentional reinterpret casts in a codebase.

This work was partly inspired by the following blog post:
https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/

The template test in this change takes inspiration from the first example in said blogpost.

Diff Detail

Event Timeline

TWeaver created this revision.Oct 18 2022, 3:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 3:33 AM
TWeaver requested review of this revision.Oct 18 2022, 3:33 AM
TWeaver edited the summary of this revision. (Show Details)Oct 18 2022, 3:35 AM
TWeaver edited the summary of this revision. (Show Details)Oct 18 2022, 3:39 AM
TWeaver added a subscriber: Restricted Project.Oct 18 2022, 3:42 AM
n-omer added a subscriber: n-omer.Oct 18 2022, 4:02 AM
inclyc added a subscriber: inclyc.Oct 18 2022, 4:10 AM
TWeaver updated this revision to Diff 468894.Oct 19 2022, 6:43 AM

Fixed pre-merge issues:
updated hidden-reinterpret-test types to long longs to fix windows clang compile error.
ran clang-format over changed lines in SemaCast.cpp

Thank you for the patch!

We don't typically add new diagnostics that are ignored by default because we have significant evidence that users don't enable them enough to warrant adding them.

In this case, I don't think we should enable this by default because it's effectively a "you're using the language feature" kind of diagnostic rather than pointing out inherently incorrect code. However, if there are situations where it points out undefined behavior were weren't diagnosing and we can limit the diagnostic to those cases, I think that would be great to enable by default. Alternatively, I think this would make sense as a clang-tidy check rather than part of the compiler itself as it feels more like a stylistic diagnostic than a correctness one.

Looks like this and more is already being caught by clang-tidy's google-readability-casting check.

TWeaver added a comment.EditedThu, Nov 24, 6:00 AM

Hello all,

First of all, thank you so much for your reviews, comments and time.

Secondly, I apologise for the late reply.

I've be thinking about your statements and I have to somewhat agree. This doesn't catch undefined behaviour, at least by its self. It may help catch undefined behaviour as a result of 'bad-code', but it's not going to catch UB as a result of the cast in every case. The clang-tidy checks are fine but they don't actually drill down to the functionality I'm looking for, aka, "this functional cast is a reinterpret_cast - beware!" (missing from this patch is a suggestion to write 'reinterpret_cast<T>(...)' instead).

Also, I'm aware that there's a warning for picking up old style c casts in the compiler, enabled by providing '-Wold-style-cast' on the command line. Are old style c casts not also language features? If that's the case why is its diagnostic embedded in the compiler and not in clang-tidy?

I ask this because there's an argument to be made that C++ style functional casts are just as dangerous as old-style casts. They can produce reinterpret casts when used in the right (or wrong) context. Would it be possible to at least add a diagnostic for detecting any functional style casts instead?

$ clang.exe .\old-style-test.cpp -Wold-style-cast
.\old-style-test.cpp:3:11: warning: use of old-style cast [-Wold-style-cast]
  int y = (double)x;
          ^  

Thanks again for your time