[PATCH] D134831: [Clang][Sema] Add -Wcast-function-type-strict

Thu Sep 29 15:29:34 PDT 2022

samitolvanen added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8686
+def warn_cast_function_type_strict : Warning<warn_cast_function_type.Text>,
+  InGroup<CastFunctionTypeStrict>, DefaultIgnore;
 def err_cast_pointer_to_non_pointer_int : Error<
----------------
aaron.ballman wrote:
> nathanchance wrote:
> > kees wrote:
> > > aaron.ballman wrote:
> > > > samitolvanen wrote:
> > > > > nickdesaulniers wrote:
> > > > > > I don't think we need a new group for a warning that only contains one diagnostic kind?
> > > > > > I don't think we need a new group for a warning that only contains one diagnostic kind?
> > > > > 
> > > > > I might have misunderstood something, but we seem to have plenty of groups with only one diagnostic kind. Is there a way to add a new warning flag without adding a diagnostic group? Most users of `-Wcast-function-type` wouldn't want to enable the stricter version, so I would prefer to keep this separate.
> > > > > 
> > > > > I did notice that some warnings don't define the group in DiagnosticGroups.td, but specify it directly here. For example, `InGroup<DiagGroup<"argument-outside-range">>`. I'm not sure if there are any benefits in doing so.
> > > > Typically we only define a group in DiagnosticGroups.td when the group is going to be used by more than one diagnostic, otherwise we prefer using `InGroup<DiagGroup<"whatever">>` to form one-off diagnostic groups.
> > > > 
> > > > However, in this case, I am wondering if we want to add `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users who enable `-Wcast-function-type` get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?
> > > > However, in this case, I am wondering if we want to add `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users who enable `-Wcast-function-type` get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?
> > > 
> > > I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.
> > > 
> > > cc @nathanchance
> > > > However, in this case, I am wondering if we want to add `CastFunctionTypeStrict` to be a subgroup of `CastFunctionType` so that users who enable `-Wcast-function-type` get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?
> > > 
> > > I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.
> > > 
> > > cc @nathanchance
> > 
> > Right, it would be quite noisy but we have already built an escape hatch for this type of scenario. We can just do what we have done for other warnings and disable it for "normal" builds to avoid disrupting the configurations with `-Werror` enabled (especially since we are not the only ones testing with tip of tree LLVM) then turn it on for `W=1` so that the build bots will catch new instances of the warnings while we clean up the other ones. I think such a diff would just look like:
> > 
> > ```
> > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > index 6ae482158bc4..52bd7df84fd6 100644
> > --- a/scripts/Makefile.extrawarn
> > +++ b/scripts/Makefile.extrawarn
> > @@ -64,6 +64,7 @@ KBUILD_CFLAGS += -Wno-sign-compare
> >  KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
> >  KBUILD_CFLAGS += -Wno-tautological-constant-out-of-range-compare
> >  KBUILD_CFLAGS += $(call cc-disable-warning, unaligned-access)
> > +KBUILD_CFLAGS += $(call cc-disable-warning, cast-function-type-strict)
> >  endif
> > 
> >  endif
> > ```
> > 
> > then that can just be reverted when we have all the instances cleaned up. It would be nice to get a game plan together for tackling all these because they appear to be quite numerous.
> Do we think this will be onerously chatty for C code bases in general? My intuition is that it will be reasonable -- folks who have enabled this warning want to know when they're type casting function pointers in odd ways. Looking at some test cases, I actually think this will behave more like users expect. e.g., https://godbolt.org/z/cWGecK1KG
> Do we think this will be onerously chatty for C code bases in general? My intuition is that it will be reasonable -- folks who have enabled this warning want to know when they're type casting function pointers in odd ways.

This produces 1500+ new warnings in the kernel, but yes, I believe these are warnings we expected to see already with `-Wcast-function-type`. Nathan's suggestion to disable the strict warning without `W=1` sounds reasonable to me.

================
Comment at: clang/test/Sema/warn-cast-function-type-strict.c:1
+// RUN: %clang_cc1 -x c %s -fsyntax-only -Wcast-function-type-strict -triple x86_64-- -verify
+
----------------
aaron.ballman wrote:
> Do we need to use the triple here, or can we make this test target agnostic?
I don't think it's needed. This was copied from the `-Wcast-function-type` test.

================
Comment at: clang/test/Sema/warn-cast-function-type-strict.c:30
+  g = (f7 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}} */
+}
----------------
aaron.ballman wrote:
> Some other test cases I think we should try out:
> ```
> typedef int (f8)(int *);
> typedef int (f9)(const int);
> typedef int (f10)(int);
> 
> int foo(int array[static 12]);
> int bar(int i);
> const int baz(int i);
> 
> f8 *h = (f8 *)foo; // Should be okay, types are the same after adjustment
> f9 *i = (f9 *)bar; // Should be okay, types are the same after adjustment
> f10 *j = (f10 *)baz; // Should be okay, types are the same after adjustment
> ```
The first two seem to be OK, the last one does produce a warning here:
```
cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134831