samitolvanen 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< ---------------- aaron.ballman wrote: > aaron.ballman wrote: > > samitolvanen 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? > > > > 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? > > > > > > Note that this wouldn't only affect `-fsanitize=kcfi`. We still use > > > `-fsanitize=cfi` in 5.13-6.0 arm64 kernels. We probably don't want to > > > break those builds with ToT Clang either, so we might still need to > > > disable the warning in the relevant stable trees (or backport the fixes > > > there). > > >>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. > 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. Sure, that works for me. I'll update the patch with release notes when I have a chance. 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
