This is an archive of the discontinued LLVM Phabricator instance.

[clang] trigger -Wcast-qual on functional casts
AcceptedPublic

Authored by sousajo on Apr 13 2023, 4:05 PM.

Details

Summary

-Wcast-qual does not trigger on the following code in Clang, but does in GCC.

const auto i = 42;
using T = int*;
auto p = T(&i);

The expected behavior is that a functional cast should trigger
the warning the same as the equivalent C cast because
the meaning is the same, and nothing about the functional cast
makes it easier to recognize that a const_cast is occurring.

Fixes https://github.com/llvm/llvm-project/issues/62083

Diff Detail

Event Timeline

sousajo created this revision.Apr 13 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 4:05 PM
sousajo requested review of this revision.Apr 13 2023, 4:05 PM
sousajo edited the summary of this revision. (Show Details)

Thank you for putting this PR, I was actually going to do this myself but I probably might not get to it soon.

clang/test/SemaCXX/warn-cast-qual.cpp
218

I would actually like to see each test above replicated for the functional cast. I realize this is a lot of work but we want to be sure that the diagnostic is really doing the equivalent check for both functional and non-functional cast.

I am not sure if we just do them right below each non-functional case of split out the functional cast cases into their own functions.

@aaron.ballman please let me know if you disagree here.

sousajo updated this revision to Diff 513579.Apr 14 2023, 7:15 AM

Add more tests

sousajo added inline comments.Apr 14 2023, 7:17 AM
clang/test/SemaCXX/warn-cast-qual.cpp
218

I replicated them on each of them functions

sousajo updated this revision to Diff 513583.Apr 14 2023, 7:20 AM
  • get rid of whitespace unrelated changes

@shafik does it look better now?

shafik accepted this revision.Apr 18 2023, 9:10 AM

LGTM

This revision is now accepted and ready to land.Apr 18 2023, 9:10 AM

can someone land it for me pls? Jorge Pinto Sousa <jorge.pinto.sousa@proton.me>

kind ping ^^

can someone land it for me pls? Jorge Pinto Sousa <jorge.pinto.sousa@proton.me>

I've landed on your behalf, and I took the liberty to add a release note when landing it.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 4:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank you for pointing this out!

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank you for pointing this out!

Actually, that's going to be more involved than a quick fix. I think I'll revert the patch and reopen this review so that @sousajo can investigate the proper fix.

This revision is now accepted and ready to land.Apr 21 2023, 10:35 AM

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

Thank you for looking into it! I didn't think about remove_const, remove_cv, etc. when considering test cases, sorry about that!

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

Thank you for looking into it! I didn't think about remove_const, remove_cv, etc. when considering test cases, sorry about that!

No worries :) Ill add those and try to craft a fix. If I am really lost I also let you know ^^

Thanks Aaron and Jorge for the quick revert and replies! :-)

have been sick, and could not advance much except I added the tests to replicate the issue. Any ideas on how to proceed here?

have been sick, and could not advance much except I added the tests to replicate the issue. Any ideas on how to proceed here?

Sorry to hear you've been sick, but thank you for your patience while I thought about this further. Two points worth observing:

  1. GCC diagnoses the problematic code the same way your patch does: https://godbolt.org/z/44GnM9jzE
  2. Clang and GCC both diagnose the notionally equivalent code using a C-style cast: https://godbolt.org/z/bbTPYb5rE

Based on that, it seems defensible for us to diagnose that code by re-landing the patch. However, I wonder why the changes broke anything. We build with GCC all the time. Is -Wcast-qual only enabled for bots building with Clang?

However, I also feel like this behavior is somewhat user-hostile because it seems reasonable to expect the developer is well aware that they're casting away the const qualifier when using something named remove_const_t to specify the cast destination type, so it seems potentially reasonable to silence the diagnostic in those cases (consistently, for both function- and c-style casts). It seems to be a reasonably popular approach to casting away const in the wild: https://sourcegraph.com/search?q=context:global+%5Cb%5Bstd::%5D%3Fremove_const%5B_t%5D%3F%5C%3C.*%5C%3E%5B::type%5D%3F%5C%28.*%5C%29&patternType=regexp&sm=1&groupBy=group so this might allow more folks to opt into -Wcast-qual. But implementing that would be tricky because we'd have to look through the cast expression to see whether it came from a type trait, and we typically do not want the frontend to be inspecting AST nodes by name (like looking for particular identifiers and changing behavior based on that), and we'd be introducing another divergence between the Clang and GCC behaviors.