Page MenuHomePhabricator

Set the default language to use when evaluating to that of the frame's CU.
ClosedPublic

Authored by dawn on Jul 10 2015, 10:54 AM.

Details

Summary

The main purpose of this patch is to be able to get the frame's context (instead of just the target's) when evaluating, so that the language of the frame's CU can be used to select the compiler and/or compiler options to use when parsing the expression. This allows for modules built with mixed languages to be parsed in the context of their frame.

Other associated changes:

  • Enable C++ language options when language is C.
  • Enable C++ language options when language is C.
  • Disable C++11 language options when language is C++.
  • Add all C and C++ language variants when determining the language options to set.
  • Add test TestMixedLanguages.py to check that the language being used for evaluation is that of the frame.
  • Remove expr --language test for ObjC from TestExprOptions.py (it is redundant).
  • Fix test TestExprOptions.py to check for C++11 instead of C++ since C++ has to be enabled for C.
  • Fix TestPersistentPtrUpdate.py to not require C++11 in C.

Diff Detail

Repository
rL LLVM

Event Timeline

dawn updated this revision to Diff 29454.Jul 10 2015, 10:54 AM
dawn retitled this revision from to Set the compiler's language options using the language of the current frame's CU, and fix ObjC evaluation..
dawn updated this object.
dawn added reviewers: clayborg, spyffe, zturner.
dawn set the repository for this revision to rL LLVM.
dawn added a subscriber: lldb-commits.
clayborg resigned from this revision.Jul 10 2015, 11:08 AM
clayborg removed a reviewer: clayborg.

I will defer to Sean Callanan.

dawn updated this object.Jul 10 2015, 11:12 AM
dawn added a comment.Jul 15 2015, 10:38 AM

Please review? Thanks.

Added more reviewers in the hopes that someone will review this patch. Please?

dawn added a comment.Jul 20 2015, 10:56 PM

See comments added in patch.

source/Commands/CommandObjectExpression.cpp
311 ↗(On Diff #29454)

Ignore this part of the patch - the language will instead be controlled by an option as discussed in http://reviews.llvm.org/D11119. It is useful for testing purposes however, to show where test cases are using expressions outside the scope of the CU's language.

source/Expression/ClangExpressionParser.cpp
219 ↗(On Diff #29454)

Do you want to be able to parse ObjC++ when the language is ObjC? If so, you'll want this.

dawn planned changes to this revision.Jul 28 2015, 4:41 PM

I'll update this patch to what I believe Greg wants in http://reviews.llvm.org/D11482, and merge in http://reviews.llvm.org/D11173 which is also needed in that case.

source/Commands/CommandObjectExpression.cpp
311 ↗(On Diff #29454)

In http://reviews.llvm.org/D11482, Greg says he doesn't want this to be controlled be an option, so this part of the patch stays.

dawn updated this revision to Diff 31098.Jul 30 2015, 6:43 PM
dawn retitled this revision from Set the compiler's language options using the language of the current frame's CU, and fix ObjC evaluation. to Set the default language to use when evaluating to that of the frame's CU..
dawn added a reviewer: clayborg.
dawn removed rL LLVM as the repository for this revision.

This patch has been modified to incorporate the discussions in http://reviews.llvm.org/D11482.

Additional changes to the language options settings were added to take care of the "ask for C++, for now get ObjC++" part (the "ask for ObjC, get ObjC++" part was in the previous patch). Changes to the frame API were also added as suggested by Greg.

dawn added a comment.Aug 6 2015, 1:22 PM

Please review? Thank you.

zturner edited edge metadata.Aug 6 2015, 1:26 PM
zturner added a subscriber: zturner.

Sorry, I don't have much to add here. I guess you need to wait for Sean or
Jim to respond.

clayborg resigned from this revision.Aug 10 2015, 9:57 AM
clayborg removed a reviewer: clayborg.

I will let Jim and Sean handle this one.

dawn added a comment.Aug 15 2015, 10:14 AM

Please review? Thank you.

dawn added a comment.Aug 27 2015, 12:55 PM

Hi guys, I'd still really like to get this into tot. May I commit it please?

clayborg accepted this revision.Aug 27 2015, 12:58 PM
clayborg added a reviewer: clayborg.
clayborg added a subscriber: clayborg.

Looks fine as long as the test suite tests are passing.

This revision is now accepted and ready to land.Aug 27 2015, 12:58 PM

Please wait for the OK from Jim and Sean.

dawn added a comment.Aug 27 2015, 3:20 PM

Please wait for the OK from Jim and Sean.

Jim/Sean, you guys OK with this? All tests pass - we've been running with this patch locally since it was proposed and have had no issues with it.

jingham requested changes to this revision.Aug 27 2015, 4:21 PM
jingham edited edge metadata.

This seems fine. The only things you should fix (see inline comments) are the name of GetDefaultExpressionLanguage and the comment on why the upgrading is necessary. If you do those two things, then this can go in. Sorry for the delay.

include/lldb/Target/StackFrame.h
475–476 ↗(On Diff #31098)

This name seems confusing. This is just the language of the current stack frame. Deciding what language to use for the expression is just one possible use of this information. It might also be useful to have the frame format print the language of the frame you've just stopped in. So this should have a more generic name. How about "GetLanguage".

source/Expression/ClangExpressionParser.cpp
220–228 ↗(On Diff #31098)

The contents of this comment are fine, but just state the reason rather than describing how you found out about the reason...

This revision now requires changes to proceed.Aug 27 2015, 4:21 PM
spyffe edited edge metadata.Aug 27 2015, 4:24 PM

I looked at this with Jim, and once his suggestions are applied this patch is fine. Thanks for working with us on it.
I want to follow up and make non-Apple platforms not force ObjC (it doesn't matter for making the expression parser work).

dawn added a comment.Aug 29 2015, 10:09 AM

Thanks so much for your reviews! I'll post a revised patch Monday.

include/lldb/Target/StackFrame.h
475–476 ↗(On Diff #31098)

Sure, GetLanguage is fine. I went with GetDefaultExpressionLanguage because that is what Greg suggested in http://reviews.llvm.org/D11482.

source/Expression/ClangExpressionParser.cpp
220–228 ↗(On Diff #31098)

will do.

dawn added a comment.Sep 1 2015, 11:10 AM

Just an update to let you know I'm still working on this (and a hundred other things)...

source/Expression/ClangExpressionParser.cpp
227 ↗(On Diff #31098)

In the process of rewriting this patch, I realized I hadn't handled the "Want C -> get ObjC++" case, which, if the switch statement case for C is fixed to handle all the C variants, turns out to be needed. When that is fixed, we get a few new failures, including the test for the expr --language option. I'm trying to figure out how to rework those tests, but with everything enabling ObjC++ now, that's not so easy.

dawn updated this revision to Diff 33721.Sep 1 2015, 1:01 PM
dawn updated this object.
dawn edited edge metadata.
dawn set the repository for this revision to rL LLVM.
dawn added a comment.Sep 2 2015, 11:00 AM

HI Jim/Sean,
Did you guys want to have a final review or can I go ahead and commit? Basically, this patch includes the changes you requested, but it also 1) fixes how the languages are recognized (the switch statement was incomplete) and 2) reworks the language-based test cases to check C/C++03 vs. C++11 (instead of C++ vs. ObjC) since C++ now enables ObjC.

spyffe accepted this revision.Sep 3 2015, 9:38 AM
spyffe edited edge metadata.

Looks good to me. Thanks, Dawn.

This revision was automatically updated to reflect the committed changes.
dawn added a comment.Sep 3 2015, 7:07 PM

I want to follow up and make non-Apple platforms not force ObjC (it doesn't matter for making the expression parser work).

Hi Sean,

I have one question about this that still bugs me... I understand that C++ is needed in C and ObjC because of implementation details of expression evaluation, but why is ObjC needed for C++?

It will be awesome if/when you can follow up on this and remove this requirement. Let me know when that's done and I'll remove the workarounds that force ObjC and give it a try.

Thanks!

jingham added a subscriber: jingham.Sep 4 2015, 8:55 AM

It isn't technically necessary, but on OS X it is very common for people to want to examine global app state from wherever and that is always ObjC, so we'd tick off our users if we forced them to explicitly dial up ObjC in other contexts. For other platforms it would be fine to choose just C++. And if the user explicitly dials up C++ (by passing --language) it should be fine to not add ObjC. But if we're setting it automatically on OS X there's a pretty strong expectation that you can just run an expression and ObjC types will be available.

Jim