Ping...
On 11/08/17 17:55, Bernd Edlinger wrote: > Ping... > > for the C++ part of this patch: > > https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html > > > Thanks > Bernd. > >> On 10/10/17 00:30, Bernd Edlinger wrote: >>> On 10/09/17 20:34, Martin Sebor wrote: >>>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote: >>>>> On 10/09/17 18:44, Martin Sebor wrote: >>>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote: >>>>>>> Hi! >>>>>>> >>>>>>> I think I have now something useful, it has a few more heuristics >>>>>>> added, to reduce the number of false-positives so that it >>>>>>> is able to find real bugs, for instance in openssl it triggers >>>>>>> at a function cast which has already a TODO on it. >>>>>>> >>>>>>> The heuristics are: >>>>>>> - handle void (*)(void) as a wild-card function type. >>>>>>> - ignore volatile, const qualifiers on parameters/return. >>>>>>> - handle any pointers as equivalent. >>>>>>> - handle integral types, enums, and booleans of same precision >>>>>>> and signedness as equivalent. >>>>>>> - stop parameter validation at the first "...". >>>>>> >>>>>> These sound quite reasonable to me. I have a reservation about >>>>>> just one of them, and some comments about other aspects of the >>>>>> warning. Sorry if this seems like a lot. I'm hoping you'll >>>>>> find the feedback constructive. >>>>>> >>>>>> I don't think using void(*)(void) to suppress the warning is >>>>>> a robust solution because it's not safe to call a function that >>>>>> takes arguments through such a pointer (especially not if one >>>>>> or more of the arguments is a pointer). Depending on the ABI, >>>>>> calling a function that expects arguments with none could also >>>>>> mess up the stack as the callee may pop arguments that were >>>>>> never passed to it. >>>>>> >>>>> >>>>> This is of course only a heuristic, and if there is no warning >>>>> that does not mean any guarantee that there can't be a problem >>>>> at runtime. The heuristic is only meant to separate the >>>>> bad from the very bad type-cast. In my personal opinion there >>>>> is not a single good type cast. >>>> >>>> I agree. Since the warning uses one kind of a cast as an escape >>>> mechanism from the checking it should be one whose result can >>>> the most likely be used to call the function without undefined >>>> behavior. >>>> >>>> Since it's possible to call any function through a pointer to >>>> a function with no arguments (simply by providing arguments of >>>> matching types) it's a reasonable candidate. >>>> >>>> On the other hand, since it is not safe to call an arbitrary >>>> function through void (*)(void), it's not as good a candidate. >>>> >>>> Another reason why I think a protoype-less function is a good >>>> choice is because the alias and ifunc attributes already use it >>>> as an escape mechanism from their type incompatibility warning. >>>> >>> >>> I know of pre-existing code-bases where a type-cast to type: >>> void (*) (void); >>> >>> .. is already used as a generic function pointer: libffi and >>> libgo, I would not want to break these. >>> >>> Actually when I have a type: >>> X (*) (...); >>> >>> I would like to make sure that the warning checks that >>> only functions returning X are assigned. >>> >>> and for X (*) (Y, ....); >>> >>> I would like to check that anything returning X with >>> first argument of type Y is assigned. >>> >>> There are code bases where such a scheme is used. >>> For instance one that I myself maintain: the OPC/UA AnsiC Stack, >>> where I have this type definition: >>> >>> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint >>> hEndpoint, ...); >>> >>> And this plays well together with this warning, because only >>> functions are assigned that match up to the ...); >>> Afterwards this pointer is cast back to the original signature, >>> so everything is perfectly fine. >>> >>> Regarding the cast from pointer to member to function, I see also a >>> warning without -Wpedantic: >>> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)« >>> [-Wpmf-conversions] >>> F *pf = (F*)&S::foo; >>> ^~~ >>> >>> And this one is even default-enabled, so I think that should be >>> more than sufficient. >>> >>> I also changed the heuristic, so that your example with the enum should >>> now work. I did not add it to the test case, because it would >>> break with -fshort-enums :( >>> >>> Attached I have an updated patch that extends this warning to the >>> pointer-to-member function cast, and relaxes the heuristic on the >>> benign integral type differences a bit further. >>> >>> >>> Is it OK for trunk after bootstrap and reg-testing? >>> >>> >>> Thanks >>> Bernd. >>>
