Page MenuHomePhabricator

Add -Wcast-calling-convention to warn when casting away calling conventions
ClosedPublic

Authored by rnk on Feb 17 2016, 1:25 PM.

Details

Summary

This only warns on casts of the address of a function defined in the
current TU. In this case, the fix is likely to be local and the warning
useful.

Here are some things we could experiment with in the future:

  • Fire on declarations as well as definitions
  • Limit the warning to non-void function prototypes
  • Limit the warning to mismatches of caller and callee cleanup CCs

This warning is currently off by default while we study its usefulness.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 48234.Feb 17 2016, 1:25 PM
rnk retitled this revision from to Add -Wcast-calling-convention to warn when casting away calling conventions.
rnk updated this object.
rnk added reviewers: thakis, rtrieu.
rnk added a subscriber: cfe-commits.
thakis edited edge metadata.Feb 17 2016, 1:40 PM

Nice!

include/clang/Basic/DiagnosticGroups.td
294 ↗(On Diff #48234)

nit: since this isn't referenced in this file, consider using an unnamed inline InGroup<DiagGroup<"cast-calling-convention"> in the other file instead

include/clang/Basic/DiagnosticSemaKinds.td
6515 ↗(On Diff #48234)

Maybe add ", likely to abort at runtime" to communicate the gravity of the situation better.

6516 ↗(On Diff #48234)

I'd build a large codebase of your choice with this enabled and see how it does. If it does well I'd land it default-on. (I spent some time enabling DefaultIgnore warnings that nobody turned on after they landed recently, and I'd prefer to not having to do this for this warning too :-) )

lib/Sema/SemaCast.cpp
1762 ↗(On Diff #48234)

If it's an inline function in a header, would we want to warn? Should this check if the body's SourceLocation is from the main file?

1775 ↗(On Diff #48234)

Consider doing something like r164858 did to see if there's a define for the calling convention (WINAPI or what have you) and suggest that instead

rtrieu added inline comments.Feb 23 2016, 3:52 PM
lib/Sema/SemaCast.cpp
1729 ↗(On Diff #48234)

Capitalize Diagnose like the other Diagnose functions in the file.

1752–1753 ↗(On Diff #48234)

Which UnaryOperator are you attempting to strip off here? This one will strip off every UnaryOperator.

1769–1773 ↗(On Diff #48234)

Usually in Sema, we uses a SmallVector backed ostream for making custom strings.

SmallString<64> CCAttrText;
llvm::raw_svector_ostream OS(CCAttrText);
OS << "some string";
rnk updated this revision to Diff 56802.May 10 2016, 1:44 PM
rnk marked 3 inline comments as done.
rnk edited edge metadata.
  • address comments
rnk added inline comments.May 10 2016, 1:49 PM
include/clang/Basic/DiagnosticGroups.td
294 ↗(On Diff #48234)

Sure. I dono, I never really liked that style since it makes it harder to add it to a group later.

include/clang/Basic/DiagnosticSemaKinds.td
6516 ↗(On Diff #48234)

My attempt to bait you into doing this work has failed. :) I'd still like to land it as ignored-by-default so I can run this warning across Chromium with a try job instead of doing it locally on my workstation. Sound good?

lib/Sema/SemaCast.cpp
1752–1753 ↗(On Diff #48234)

Oops, added a test for that.

1762 ↗(On Diff #48234)

Yeah, we do want to warn even if it's from a header. If it were defined elsewhere with a different convention, that'd be an ODR violation.

1775 ↗(On Diff #48234)

Done, that was fun. :)

thakis accepted this revision.May 10 2016, 1:55 PM
thakis edited edge metadata.
This revision is now accepted and ready to land.May 10 2016, 1:55 PM
This revision was automatically updated to reflect the committed changes.