Page MenuHomePhabricator

[Expr] Check the language before ignoring Objective C keywords
ClosedPublic

Authored by aleksandr.urakov on Nov 23 2018, 1:45 AM.

Details

Summary

This patch adds the check of the language before ignoring names like id or Class, which are reserved in Objective C, but allowed in C++. It is needed to make it possible to evaluate expressions in a C++ program containing names like id or Class.

There was a discussion of this at the end of October 2018 in lldb-dev.

Diff Detail

Repository
rL LLVM

Event Timeline

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
401–404 ↗(On Diff #175090)

Is it still necessary? We can do here something like #ifdef __APPLE__, but then the test will fail on Apple platforms. Can we somehow specify a platform requirement in the test?

labath added a subscriber: labath.Nov 26 2018, 2:02 AM
labath added inline comments.
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
401–404 ↗(On Diff #175090)

Instead of ifdef it would be better to do a target->GetArchitecture().getTriple().getVendor() == llvm::Triple::Apple and then @skipIfDarwin in the test. But it would be certainly better to remove this altogether :)

aleksandr.urakov marked an inline comment as done.
aleksandr.urakov added inline comments.
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
401–404 ↗(On Diff #175090)

Yes, your variant is better! I'll do it in the way you have described if there will be objections against removing it at all.

clayborg requested changes to this revision.Nov 26 2018, 7:15 AM
clayborg added a subscriber: clayborg.
clayborg added inline comments.
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
404 ↗(On Diff #175090)

A better way would be to try and grab the Objective C runtime from the process. There are some variants of objective C that might run under non-apple targets:

ProcessSP process_sp = target->GetProcess();
if (process_sp) 
  m_compiler->getLangOpts().ObjC = process_sp->GetLanguageRuntime(eLanguageTypeObjC) != nullptr;

Then C and C++ programs on Mac will be able to use "id" and other reserved words in their expressions again.

This revision now requires changes to proceed.Nov 26 2018, 7:15 AM
aleksandr.urakov marked 2 inline comments as done.
aleksandr.urakov added inline comments.
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
404 ↗(On Diff #175090)

Thanks for the idea! I've updated the patch.

aleksandr.urakov marked an inline comment as done.Dec 3 2018, 6:06 AM

Ping! Can you look at this, please?

Looks fine to me. Jim Ingham should ok this as well.

jingham accepted this revision.Dec 3 2018, 6:51 PM

I think the ideal behavior would be "if we don't have debug info for the frame, choose ObjC++". But if you are in frame with debug info, obey that frame's language (except that we have to use C++ because the expression parser uses C++ references to play some of the games it plays.) The reasoning behind that is because in the first case, you won' have local names and are calling out to global objects and having the widest language available is the most convenient. If we did it that way, we could also make the user's choice of language always hold (except again you can't use "class" anywhere because of the use of C++...).

But I don't see an easy way to implement that behavior. You'd have to plumb through another bit - has debug info - which is orthogonal to the language, but have that affect the language choice.

However, this change is strictly better for non-objc programs, and preserves the same behavior as currently holds when there is ObjC. So I think this change is fine for now.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 4 2018, 1:54 AM
This revision was automatically updated to reflect the committed changes.

The test was broken after this commit. It seems that on MacOS X the process of the compiled test application always contains Objective C Runtime, so the part of the test, which assumes that there is no ObjC option enabled, fails. To fix it I've skip this part for Darwin here: https://reviews.llvm.org/rLLDB348250 Please, tell me if you have any objections on this.