This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Sema] Implement GCC -Wcast-function-type
ClosedPublic

Authored by ychen on Mar 2 2021, 9:10 PM.

Details

Summary
Warn when a function pointer is cast to an incompatible function
pointer. In a cast involving function types with a variable argument
list only the types of initial arguments that are provided are
considered. Any parameter of pointer-type matches any other
pointer-type. Any benign differences in integral types are ignored, like
int vs. long on ILP32 targets. Likewise type qualifiers are ignored. The
function type void (*) (void) is special and matches everything, which
can be used to suppress this warning. In a cast involving pointer to
member types this warning warns whenever the type cast is changing the
pointer to member type. This warning is enabled by -Wextra.

Diff Detail

Event Timeline

ychen requested review of this revision.Mar 2 2021, 9:10 PM
ychen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 9:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ychen updated this revision to Diff 332384.Mar 22 2021, 11:53 AM
  • update
rsmith added inline comments.Mar 23 2021, 3:02 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8368

I'd find something like "cast from function type %0 to incompatible function type %1" to be easier to read. But I suppose %0 and %1 are the pointer types, not the function types, so perhaps "cast from %0 to %1 converts to incompatible function type" or something like that?

clang/lib/Sema/SemaCast.cpp
1044–1048

In addition to allowing the cases where the sizes are the same width, should we also allow cases where the promoted types are the same width (eg, int versus short)? What does GCC do?

1050–1055

I think a "same type" check (Context.hasSameUnqualifiedType(SrcType, DestType)) would be more appropriate here than a structural equivalence check.

1067–1081

This can be simplified a bit.

1082–1083

We should also handle the case where the source is of function type and the destination is a reference-to-function type.

Maybe also block pointer types?

1090

Please use auto * here (or const auto * as the linter suggests).

ychen updated this revision to Diff 332840.Mar 23 2021, 6:18 PM
ychen marked 3 inline comments as done.
  • Add code/tests for function -> function pointer and block pointer -> function pointer.
  • Use Context.hasSameUnqualifiedType
  • Style
clang/lib/Sema/SemaCast.cpp
1044–1048

GCC does exactly that. I didn't find a way to do that by looking at TargetInfo. Maybe there is a way for clang?

1050–1055

Agreed.

1082–1083

Yep. Code/tests added for function -> function pointer and block pointer -> function pointer.

ychen marked 2 inline comments as done.Mar 23 2021, 6:18 PM
rsmith accepted this revision.Mar 24 2021, 2:59 PM

Looks good. Some possible improvements to make the diagnostic a bit more precise, but I'd be happy if you want to address some of those as follow-ups after this change.

clang/lib/Sema/SemaCast.cpp
1041

I think we should also treat references like pointers here, and similarly for block pointers and ObjC pointers. Perhaps using hasPointerRepresentation() would make sense?

1044–1048

Hm. The right thing to do would be to call ABIInfo::isPromotableIntegerTypeForABI, but that's defined in CodeGen so we can't call it from here without violating layering. I suppose we could pull it up into the target info in Basic, if you're prepared to do that kind of refactoring, but maybe for now we can skip this check and emit a few warnings that GCC doesn't.

1050–1055

Taking this a bit further: we could use hasSimilarType to allow changes in nested cv-qualifiers. But given that we've already allowed arbitrary differences in pointee types, I suppose this would only affect pointers-to-members, and it might be preferable to allow arbitrary changes between pointer-to-member types here (except in the MS ABI case where there are meaningful differences in pointer-to-member representations between classes).

What does GCC do with differences in pointer-to-member types in function parameter types?

This revision is now accepted and ready to land.Mar 24 2021, 2:59 PM
ychen added a comment.Mar 24 2021, 3:46 PM

Thanks for the insightful suggestions. I'll definitely send a follow-up patch for these (pointerLike arguments, TargetInfo refactoring, promotable integral types).

ychen updated this revision to Diff 333162.Mar 24 2021, 3:57 PM
  • update doc to match diagnostics
ychen edited the summary of this revision. (Show Details)Mar 24 2021, 3:59 PM
This revision was landed with ongoing or failed builds.Mar 24 2021, 4:04 PM
This revision was automatically updated to reflect the committed changes.