Page MenuHomePhabricator

Fix ObjC++ types Class and id being defined in C and C++ expressions.
Needs RevisionPublic

Authored by paulherman on Aug 5 2015, 4:22 PM.

Details

Summary

This allows using "Class" and "id" as identifiers in C and C++ expressions. This was because ObjC++ runtime was available by default. It now selects the default language when evaluating an expression from the current frame compile unit.

Diff Detail

Event Timeline

paulherman updated this revision to Diff 31417.Aug 5 2015, 4:22 PM
paulherman retitled this revision from to Fix ObjC++ types Class and id being defined in C and C++ expressions..
paulherman updated this object.
paulherman added a subscriber: lldb-commits.
paulherman updated this revision to Diff 31419.Aug 5 2015, 4:26 PM

Fix ObjC++ types Class and id being defined in C and C++ expressions.

Ignore previous commit included by mistake.

dawn added a subscriber: dawn.Aug 10 2015, 8:52 PM
spyffe edited edge metadata.Aug 11 2015, 8:46 AM

It looks like this patch does not use "language" to determine which language the expression parser runs in, but rather to allow lookups specifically of "id" and "Class" if the current frame is not Objective-C++. Is that correct?

spyffe requested changes to this revision.Aug 11 2015, 9:14 AM
spyffe edited edge metadata.

Ooh, I'm reading the patch at the beginning of ClangUserExpression::Evaluate again and it does look like this patch sets language based on the language of the containing frame, and that affects what language the expression is parsed in.

I don't think it's appropriate to force the expression parser out of Objective-C or C++ mode in non-ObjC frames, because it's entirely possible for a user to want to run Objective-C (or C++ code) from frames in a different language. For example, it is often the case that a user will want to call [NSApplication sharedApplication] in an app, even if they are in C/C++ code.

For the same reason I think the idea of a target-level language isn't the right approach. Rather, I think there should be a setting that says "force all expressions to this language by default" that the user can set if they're debugging a process that prefers a particular language. By default, this setting would be Objective-C++ – but for cases where the user is expecting to deal mostly with pure C++ binaries, that can be switched to C++. It might even be something that could be set on a per-platform basis.

Be aware (as I usually warn) that you will get a broken expression parser if you try to force it to use only C. The expression parser uses references internally to allow you to use local variables without dereferencing pointers. We want to get Clang support for a "C-language reference" of some sort eventually, but until that happens you need to at least enable C++.

This revision now requires changes to proceed.Aug 11 2015, 9:14 AM

The idea is that, as I user, I do not expect the identifiers "Class" and "id" to not be available - I don't think I've seen a warning or notice about that when evaluating expressions.

I believe that setting the language based on the current frame is a good guess. I think evaluating something in the language of the current frame is more common than evaluating something that is in ObjC++ and the current frame is C++.

The idea about target-level language is that it only selects a specific language (i.e. not the default ObjC++) if all the compile units are in the same language. I believe this to be right since a debugger is not a REPL, hence we shouldn't expect the users to evaluate something in ObjC++ if their binary doesn't contain anything related to that language.

dawn added a comment.Aug 14 2015, 12:14 PM

Hi Sean,

Ooh, I'm reading the patch at the beginning of ClangUserExpression::Evaluate again and it does look like this patch sets language based on the language of the containing frame, and that affects what language the expression is parsed in.

Yep - and I agree that this is a good thing.

I don't think it's appropriate to force the expression parser out of Objective-C or C++ mode in non-ObjC frames ...

But the parsing of ObjC conflicts with other languages. This is a real problem.

For the same reason I think the idea of a target-level language isn't the right approach.

I agree here.

Rather, I think there should be a setting that says "force all expressions to this language by default" that the user can set if they're debugging a process that prefers a particular language.

We have such a setting with target.language.

By default, this setting would be Objective-C++

I disagree here - the default is (and should be) unknown.

The idea is that, as I user, I do not expect the identifiers "Class" and "id" to not be available - I don't think I've seen a warning or notice about that when evaluating expressions.

I believe that setting the language based on the current frame is a good guess. I think evaluating something in the language of the current frame is more common than evaluating something that is in ObjC++ and the current frame is C++.

I'm totally with Paul on this, and we've discussed this same issue in http://reviews.llvm.org/D11482, http://reviews.llvm.org/D11173 and http://reviews.llvm.org/D11102. It was Jim's belief that you would be in favor of using the frame too, but here it sounds like you *do* want to always be able to eval ObjC, which contradicts Jim and Greg. I believe the approach I took in http://reviews.llvm.org/D11102 is exactly what was requested from Greg and Jim. If you disagree, please talk to them, because Paul and I are stuck between two conflicting desires here, and we can't move forward. Note that my patch at http://reviews.llvm.org/D11102 still awaits your review.

Thanks,
-Dawn

dawn added a comment.Aug 14 2015, 12:49 PM

Hi Paul,

Some background... I basically tried the same thing in my original http://reviews.llvm.org/D11102 and http://reviews.llvm.org/D11173. In light of Sean's objections in http://reviews.llvm.org/D11173, I created http://reviews.llvm.org/D11482 which used an option to control this behavior, but Greg and Jim objected to that, so I took their comments and revised http://reviews.llvm.org/D11102 to what I believed the consensus was.

You should read the comments in http://reviews.llvm.org/D11482 as they apply here as well.

My revision of http://reviews.llvm.org/D11102 is what I believe the consensus was, so perhaps you would like to abandon this patch in favor of that one?

Thanks,
-Dawn

Is Objective C++ even a thing on non-Apple platforms? I dont' think we
should be forcing a default *anything* to a language that only exists on a
subset of supported platforms. But even ignoring that, I don't think
preferential treatment should be given to any particular language.

If there is going to be a setting, then perhaps the setting should be
something like:

lldb.expression-eval-mode = {ObjC++, C++, current-frame}

and the default value of the setting should be "current-frame".

spyffe added a subscriber: spyffe.Aug 14 2015, 1:27 PM

I totally agree that Objective-C shouldn’t be part of the default on non-Apple platforms.
The way I imagine this working is that your current frame’s language gets “upgraded” – for the Clang expression parser, at least to something that supports C++, but for other languages perhaps something else.
So, supposing e.g. Go gets its own expression parser:

[on Apple]

(current frame -> expression language)
C -> ObjC++
C++ -> ObjC++
ObjC -> ObjC++
Go -> Go)

[on Linux]
C -> C++
C++ -> C++
ObjC -> ObjC++
Go -> Go

The dependency of the Clang expression parser on C++ is an implementation detail, so we should upgrade C/ObjC to C++/ObjC++ internally.
The only settable preference here is whether Objective-C is globally desirable. Maybe it would be best to just have a simple global setting:

lldb.force-objc-in-clang-expressions = true (on Apple), false (elsewhere)

Sean

This patch does exactly that. It detects the language of the frame and upgrades it according to the rules you said (I think I might've missed ObjC -> ObjC++, but that can be added).

Regarding the global setting of ObjC, I believe it is not helpful. What I mean is, when debugging something in ObjC the frame is probably already ObjC.

Paul

dawn added a comment.Aug 14 2015, 6:02 PM

See inline comments.

source/Commands/CommandObjectExpression.cpp
308

Why was this removed? Doesn't this break the intent of the target.language setting? That is:

if expression --language option is set, use it,
else if target.language option is set, use it,
else default to the language of the frame.

Please keep this or preserve the above behavior another way.

source/Expression/ClangUserExpression.cpp
1063

If the user explicitly wanted the language to be C (either by expression --language or target.language settings), we should not give them C++11.

sivachandra resigned from this revision.Oct 19 2015, 10:33 AM
sivachandra removed a reviewer: sivachandra.