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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
