This is an archive of the discontinued LLVM Phabricator instance.

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

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
6626

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

6627

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
1765

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?

1778

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
1732

Capitalize Diagnose like the other Diagnose functions in the file.

1755–1756

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

1772–1776

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
6627

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
1755–1756

Oops, added a test for that.

1765

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.

1778

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.