[PATCH] D136790: [Clang][Sema] Add -Wincompatible-function-pointer-types-strict

Wed Nov 2 12:15:49 PDT 2022

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8217
+  err_typecheck_convert_incompatible_function_pointer.Text>,
+  InGroup<DiagGroup<"incompatible-function-pointer-types-strict">>, DefaultIgnore;
 def ext_typecheck_convert_discards_qualifiers : ExtWarn<
----------------
nathanchance wrote:
> nathanchance wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > samitolvanen wrote:
> > > > > nathanchance wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > samitolvanen wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > We don't typically add new off-by-default warnings because we have evidence that users don't enable them enough to be worth adding them. Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users? I don't think we have any examples of doing this in the code base, so I believe this would be breaking new ground (and thus is worth thinking about more, perhaps it's a bad idea for some reason).
> > > > > > > > > Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users?
> > > > > > > > 
> > > > > > > > I can look into it, but I'm not sure if there's a convenient way to do that. An alternative to this could be to enable the warning by default, but only actually perform the check if CFI is enabled. This way non-CFI users would never see the warning because this really isn't a concern for them, but CFI users would still get the warning without explicitly enabling it. Or do you think this behavior would be confusing?
> > > > > > > Heh, sorry for not being clear, that's actually somewhat along the lines of how I envisioned you'd implement it (we don't have a way in tablegen to tie `DefaultIgnore` to some language options or a target).
> > > > > > > 
> > > > > > > I was thinking the diagnostic would be left as `DefaultIgnore` in the .td file, but we'd take note in the driver that CFI was enabled and pass `-Wincompatible-function-pointer-types-strict` to the `-cc1` invocation. If the user wrote `-Wno-incompatible-function-pointer-types-strict` at the driver level, that would come *after* the automatically generated flag for cc1 and would still disable the diagnostic in the cc1 invocation (because the last flag wins). WDYT?
> > > > > > I do not think that is unreasonable behavior in the generic case but specifically for the Linux kernel, kCFI is disabled by default (`CONFIG_CFI_CLANG` has to be specifically enabled) but we want to see this warning regardless of the value of that configuration so that driver authors who test with clang and automated testers are more likely to find them. As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?
> > > > > > I was thinking the diagnostic would be left as `DefaultIgnore` in the .td file, but we'd take note in the driver that CFI was enabled and pass `-Wincompatible-function-pointer-types-strict` to the `-cc1` invocation.
> > > > > 
> > > > > Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.
> > > > > As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?
> > > > 
> > > > It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> > > > Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.
> > > 
> > > Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.
> > > 
> > > If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).
> > > It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> > 
> > Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)
> > Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.
> 
> Yes, we do not have as much freedom with pushing changes into Linux mainline, as they first have to soak in the -next tree, so that will require at least a week. I assume @kees can help us out with getting that patch to Linus in a timely manner though so that is probably not as much of a concern.
> 
> > If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).
> 
> I have sent patches to fix all the instances of this that I see in the Linux kernel as of this morning:
> 
> https://github.com/ClangBuiltLinux/linux/issues/1750#issuecomment-1300972689
> 
> I have to track all of those so I know when it is possible to enable this warning in `KBUILD_CFLAGS` so I could remind Sami to enable it for CFI builds by default once that is done?
>>It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)

Phew, I'm glad I wasn't missing something subtle. :-D

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8217
+  err_typecheck_convert_incompatible_function_pointer.Text>,
+  InGroup<DiagGroup<"incompatible-function-pointer-types-strict">>, DefaultIgnore;
 def ext_typecheck_convert_discards_qualifiers : ExtWarn<
----------------
aaron.ballman wrote:
> nathanchance wrote:
> > nathanchance wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > samitolvanen wrote:
> > > > > > nathanchance wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > samitolvanen wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > We don't typically add new off-by-default warnings because we have evidence that users don't enable them enough to be worth adding them. Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users? I don't think we have any examples of doing this in the code base, so I believe this would be breaking new ground (and thus is worth thinking about more, perhaps it's a bad idea for some reason).
> > > > > > > > > > Is there a way we can enable this warning by default for CFI compilation units (or when the cfi sanitizer is enabled) so that it's only off by default for non-CFI users?
> > > > > > > > > 
> > > > > > > > > I can look into it, but I'm not sure if there's a convenient way to do that. An alternative to this could be to enable the warning by default, but only actually perform the check if CFI is enabled. This way non-CFI users would never see the warning because this really isn't a concern for them, but CFI users would still get the warning without explicitly enabling it. Or do you think this behavior would be confusing?
> > > > > > > > Heh, sorry for not being clear, that's actually somewhat along the lines of how I envisioned you'd implement it (we don't have a way in tablegen to tie `DefaultIgnore` to some language options or a target).
> > > > > > > > 
> > > > > > > > I was thinking the diagnostic would be left as `DefaultIgnore` in the .td file, but we'd take note in the driver that CFI was enabled and pass `-Wincompatible-function-pointer-types-strict` to the `-cc1` invocation. If the user wrote `-Wno-incompatible-function-pointer-types-strict` at the driver level, that would come *after* the automatically generated flag for cc1 and would still disable the diagnostic in the cc1 invocation (because the last flag wins). WDYT?
> > > > > > > I do not think that is unreasonable behavior in the generic case but specifically for the Linux kernel, kCFI is disabled by default (`CONFIG_CFI_CLANG` has to be specifically enabled) but we want to see this warning regardless of the value of that configuration so that driver authors who test with clang and automated testers are more likely to find them. As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?
> > > > > > > I was thinking the diagnostic would be left as `DefaultIgnore` in the .td file, but we'd take note in the driver that CFI was enabled and pass `-Wincompatible-function-pointer-types-strict` to the `-cc1` invocation.
> > > > > > 
> > > > > > Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.
> > > > > > As a result, if the warning is not going to be default on, we would still need to specifically enable it in our CFLAGS. I suppose your suggestion would help out folks using CFI in production though so maybe it is still worth considering doing?
> > > > > 
> > > > > It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> > > > > Thanks for clarifying, that sounds reasonable. Of course, enabling it by default with CFI would still mean that we'll have to either explicitly disable this in the kernel (or fix all the existing warnings) before submitting the Clang change.
> > > > 
> > > > Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.
> > > > 
> > > > If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).
> > > > It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> > > 
> > > Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)
> > > Er, is there a chicken and egg problem here? I was imagining that you could land the changes in Clang and disable the flag in the Linux kernel around roughly the same time (we can time when we land things to make life easier for you) so it wouldn't be particularly disruptive.
> > 
> > Yes, we do not have as much freedom with pushing changes into Linux mainline, as they first have to soak in the -next tree, so that will require at least a week. I assume @kees can help us out with getting that patch to Linus in a timely manner though so that is probably not as much of a concern.
> > 
> > > If it would be disruptive, another approach would be to leave it off by default everywhere, get the Linux kernel CFI warning free, then enable it for CFI builds by default. But that seems riskier to me (we are more likely to forget to come turn the warning on by default later).
> > 
> > I have sent patches to fix all the instances of this that I see in the Linux kernel as of this morning:
> > 
> > https://github.com/ClangBuiltLinux/linux/issues/1750#issuecomment-1300972689
> > 
> > I have to track all of those so I know when it is possible to enable this warning in `KBUILD_CFLAGS` so I could remind Sami to enable it for CFI builds by default once that is done?
> >>It sounds like either approach will require you to specifically enable it in your CFLAGS, right? Either it's off-by-default for everyone, or it's only on-by-default for CFI enabled builds (which the Linux kernel disables by default).
> > Right, I was only mentioning our use case will necessitate it being always enabled so conditionally enabling it would be extra work that wouldn't benefit us but obviously, the world does not revolve around our one project :)
> 
> Phew, I'm glad I wasn't missing something subtle. :-D
> I have to track all of those so I know when it is possible to enable this warning in KBUILD_CFLAGS so I could remind Sami to enable it for CFI builds by default once that is done?

Okay, that seems reasonable enough to me (assuming @samitolvanen agrees, of course). If that's the route we're going, then I think this patch is basically ready to go as-is (the diagnostic is already off-by-default here) and the only thing left is the release note.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136790