Page MenuHomePhabricator

Proposal to make rtti errors more generic.
ClosedPublic

Authored by Sunil_Srivastava on May 23 2018, 3:16 PM.

Details

Summary

This patch changes the wording of two errors to make them more generic.

An attempt to use dynamic_cast while rtti is disabled, curently emits the error:

cannot use dynamic_cast with -fno-rtti

and a similar one for typeid.

This patch proposes to change that to:

use of dynamic_cast requires enabling RTTI

For targets where the default mode of RTTI is disabled, the current error
message is confusing because the user never used the -fno-rtti option.

This proposal is motivated by the PS4 compiler, whose default is to have RTTI
disabled. However, it is just as clear as the existing diagnostic, and it
may be applicable to other llvm compilers having the have the same default as the
PS4 compiler. It is also more appropriate in cases where the spelling of the
argument (to disable RTTI) is something other than -fno-rtti (for
example, /GR- is the switch to disable RTTI for cl).

Diff Detail

Repository
rC Clang

Event Timeline

filcab added a subscriber: filcab.May 25 2018, 6:51 AM
filcab added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6612

I'd prefer to have the way to enable RTTI mentioned in the message. Could we have something like ToolChain::getRTTIMode() and have a "RTTI was on/of" or "RTTI defaulted to on/off"? That way we'd be able to have a message similar to the current one (mentioning "you passed -fno-rtti") on platforms that default to RTTI=on, and have your updated message (possibly with a mention of "use -frtti") on platforms that default to RTTI=off.

(This is a minor usability comment about this patch, I don't consider it a blocker or anything)

probinson added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6612

If the options are spelled differently for clang-cl and we had a way to retrieve the appropriate spellings, providing the option to use in the diagnostic does seem like a nice touch.

include/clang/Basic/DiagnosticSemaKinds.td
6612

The idea of suggestion as to how-to-turn-on-rtti is appealing, but it is not trivial.

First, clang-cl does not give this warning at all, so the issue is moot for clang-cl.

For unix-line command-line, if the default RTTI mode in ENABLED (the unknown-linux)
then this warning appears only if the user gives -fno-rtti options, so again we do
not need to say anything more.

The only cases left are compilers a where the default RTTI mode is DISABLED.
Here an addendum like '[-frtti]' may make sense. AFAIK, PS4 is the only compiler
with this default, but there may be other such private compilers.

So should we append '[-frtti]' if Target.getTriple().isPS4() is true?

wristow added a subscriber: wristow.Jun 4 2018, 2:44 PM
wristow added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6612

So should we append '[-frtti]' if Target.getTriple().isPS4() is true?

Personally, I'd be OK with producing a suggestion of how to enable RTTI based on the PS4 triple. But I'd also be OK with not enhancing this diagnostic to suggest how to turn on RTTI (that is, going with the patch as originally proposed here).

If clang-cl produced a warning when a dynamic_cast or typeid construct was encountered in /GR- mode, then I'd feel it's worth complicating the code to provide a target-sensitive way for advising the user how to turn RTTI on. But clang-cl doesn't produce a warning in that case, so the effort to add the framework for producing a target-sensitive warning doesn't seem worth it to me.

Improving clang-cl to produce a diagnostic in this /GR- situation seems like a good idea, but separate from this proposed patch. If that work gets done at some point, then it would be natural to revisit this diagnostic at that time.

probinson added inline comments.Jun 5 2018, 9:59 AM
include/clang/Basic/DiagnosticSemaKinds.td
6612

If clang-cl is not a consideration, then I think the easiest and clearest way to do this is simply to say requires -frtti without hair-splitting which targets default which way.

wristow added inline comments.Jun 5 2018, 11:12 AM
include/clang/Basic/DiagnosticSemaKinds.td
6612

Saying requires -frtti makes good sense to me.

Changed as per Paul's suggestion.

include/clang/Basic/DiagnosticSemaKinds.td
6612

Done

This revision is now accepted and ready to land.Jun 6 2018, 9:25 AM
This revision was automatically updated to reflect the committed changes.