Page MenuHomePhabricator

Add target setting to have the language follow the frame's CU.
AbandonedPublic

Authored by dawn on Jul 23 2015, 5:45 PM.

Details

Summary

Add target.language-follows-frame setting.

If the target.language is unknown, default to the language of the selected frame (if known).

Note: this patch depends on http://reviews.llvm.org/D11447.

Diff Detail

Repository
rL LLVM

Event Timeline

dawn updated this revision to Diff 30550.Jul 23 2015, 5:45 PM
dawn retitled this revision from to Add target setting to have the language follow the frame's CU..
dawn updated this object.
dawn added reviewers: clayborg, jingham, spyffe.
dawn set the repository for this revision to rL LLVM.
dawn added a subscriber: lldb-commits.
dawn updated this revision to Diff 30602.Jul 24 2015, 1:32 PM
clayborg requested changes to this revision.Jul 24 2015, 3:41 PM
clayborg edited edge metadata.

I think it is fine to add support for looking at the target.language and using it if set, but I don't think we need a "target.language-follows-frame" setting at all. We should just do the right thing. But the right thing I think it means just asking the frame for its expression language. See inlined comments.

include/lldb/Target/Target.h
175–177

Remove this, we shouldn't need this setting.

1443–1445

This should be on lldb_private::StackFrame and should be:

lldb::LanguageType
StackFrame::GetDefaultExpressionLanguage();
source/Commands/CommandObjectExpression.cpp
303–312

This should be:

if (m_command_options.language != eLanguageTypeUnknown)
    options.SetLanguage(m_command_options.language);
else
{
    StackFrame *frame = exe_ctx.GetFramePtr();
    if (frame)
        options.SetLanguage(frame-> GetDefaultExpressionLanguage());
}
`
source/Target/Target.cpp
391–395

Remove this.

430–434

Remove this.

468–472

Remove this.

2158–2178

This should be moved to the StackFrame as:

lldb::LanguageType
StackFrame::GetDefaultExpressionLanguage();

And it should check if the language is form of C, C++, ObjC, or ObjC ++, then return ObjC++, else return the language.

2998

I would prefer to not have this option as it doesn't really make sense and no one should have to worry about it.

3058

Remove

3516–3522

Remove

This revision now requires changes to proceed.Jul 24 2015, 3:41 PM
dawn added a comment.Jul 24 2015, 6:16 PM

Thank you for your review. I was afraid you might not go for it since you objected to my original frame-based patch for breakpoints, but I had hoped that by having it in an option you would be OK with it. Our users (with mixed Pascal and C++ code) really want this so they don't have to remember to specify the language each time they go up or down the stack and want to view their local variables. Consider this scenario:

Suppose a user hits a breakpoint in a Pascal function called from C++ and sets the target language to Pascal so that they can evaluate their locals using the Pascal FE. Now they go up a frame into C++ and try to see their locals but can't without resetting the target language (or by always specifying the expr --language option). Then they run to a BP in a C++ module, same problem.

Does this help explain why having such a default would be so desirable? It is how all our debuggers and gdb work. I'm curious - how do you get around this problem in Swift?

source/Commands/CommandObjectExpression.cpp
303–312

I would like to have this - see http://reviews.llvm.org/D11102 (you resigned from that review). Is it OK to add? Or did you want to kill the patch entirely? Note that setting the language to the frame means that ObjC can't be evaluated in C++ which Sean is against (see http://reviews.llvm.org/D11173), and there are tests which expect C++ to be available in ObjC (see http://reviews.llvm.org/D11102).

dawn added a comment.Jul 24 2015, 8:21 PM

PS: I hope you won't return ObjC++ if language is unknown for any of the code you write - see inline comment.

PPS: Many thanks again for all your reviews!!

source/Target/Target.cpp
2158–2178

ObjC++ should not be the default, because then the code/user can't know if it really is unknown or is in a ObjC++ context. Having it return unknown is good - the expression code sets the compiler's language options for unknown the same as ObjC++ anyway (see code in ClangExpressionParser::ClangExpressionParser).

So we accurately set the

In D11482#211941, @dawn wrote:

Thank you for your review. I was afraid you might not go for it since you objected to my original frame-based patch for breakpoints, but I had hoped that by having it in an option you would be OK with it. Our users (with mixed Pascal and C++ code) really want this so they don't have to remember to specify the language each time they go up or down the stack and want to view their local variables. Consider this scenario:

Suppose a user hits a breakpoint in a Pascal function called from C++ and sets the target language to Pascal so that they can evaluate their locals using the Pascal FE. Now they go up a frame into C++ and try to see their locals but can't without resetting the target language (or by always specifying the expr --language option). Then they run to a BP in a C++ module, same problem.

Does this help explain why having such a default would be so desirable? It is how all our debuggers and gdb work. I'm curious - how do you get around this problem in Swift?

We know the language of each frame because of DWARF and we "do the right thing" automatically. Having the user have to do anything (set target.language, or any other setting) is the wrong approach.

For Swift, we have modified ClangASTType to contain a pointer union where there is a "clang::ASTContext *" and a "swift::ASTContext *" and still a "void *" that represents the type. All function calls inside the ClangASTType class then have code like:

if (IsClangType())
{
    // clang::ASTContext specific code
}
else if (IsSwiftType())
{
    // clang::ASTContext specific code
}

Then no changes need to be done to any code that displays the types because the API of ClangASTContext takes care of things. 

For expressions we detect the language from the frame (if a language wasn't specified with an option) and we use either the clang based expression parser or the Swift one based on the language.

So any solution must:
- not require intervention from the user in any way (no settings, or any other options on command) when switching frames
- Evaluating expressions can use the current frame's language
- breakpoints, if the language isn't specified, should just do the right thing and set the breakpoint in all languages

Questions:
- So does pascal have a native llvm/clang type system that isn't in the clang namespace?
- Does pascal use types created in a clang::ASTContext or doesn't it have its own type system?
- Does pascal use the clang expression parser or a native one?
dawn requested a review of this revision.EditedJul 27 2015, 11:09 AM
dawn edited edge metadata.

I need guidance due to conflicting directions from Greg and Sean. Sean wants to be able to evaluate ObjC++ by default always, hence this feature was made optional. Greg feels the frame can be used for the language without an option, but that will break the ability to evaluate ObjC++ anytime.

See also:
http://reviews.llvm.org/D11102 - eval in language of frame's CU and add C++ language option to ObjC so tests will still work.
http://reviews.llvm.org/D11173 - rewrote test which tested ObjC types in C++ test case to be ObjC++ so test would still work.

We can either go with this patch modified as per some of Greg's requests (i.e. evaluation language follows frame is optional), or http://reviews.llvm.org/D11102 and http://reviews.llvm.org/D11173 (i.e. evaluation language follows frame by default and tests must be fixed). I would choose this patch because it makes the behavior optional, and everyone is happy.

Please advise. Thank you.

dawn added a comment.Jul 27 2015, 11:22 AM

Questions:

  • So does pascal have a native llvm/clang type system that isn't in the clang namespace?

It uses the clang type system with lots of extensions.

  • Does pascal use types created in a clang::ASTContext or doesn't it have its own type system?

They are created in clang::ASTContext with lots of extensions.

  • Does pascal use the clang expression parser or a native one?

Both at the moment :) we have a modified clang FE as a short-term "minimal parser" until a full-blown pascal FE can be hooked up.

dawn added a comment.Jul 28 2015, 9:36 PM

On Mon, Jul 27, 2015 at 11:24:01AM -0700, Jim Ingham wrote:

Just to be clear... Sean doesn't have a DESIRE to have the expression parser use ObjC++ anytime the language is a C family language. Rather he MUST right now, because the expression parser uses features of C++ to capture values. We could switch to using C++ in C/C++ situations, and ObjC++ in others, but there wasn't sufficient motivation to add that. Sometime when we get some spare cycles we'll try to relax the need for C++, and then we'll truly be able to follow the frame language. For now, we do "Want C -> get ObjC++", "Want ObjC -> get ObjC++" etc... But again, that is not a fundamental choice, it is an implementation necessity.

I see. This is a major pain for Pascal however, because C++ and ObjC/ObjC++ conflict directly with Pascal, so unless the user specifies the language (via target.language or expr --language), they can't evaluate at all. So until you guys can find those "spare cycles", we're forced to maintain private patches.

jingham edited edge metadata.Jul 29 2015, 10:30 AM
jingham added a subscriber: jingham.

I'm not sure it is that hard. For the expression parser, we can let folks ask for whatever they want, and then just give them whatever we need to give them. The mapping is pretty simple, as I said, ask for ObjC, get ObjC++, ask for C++, for now get ObjC++, etc.

I don't know how you guys are doing Pascal expression evaluation? Do you have a Pascal Clang front-end? Do you do some kind of Pascal->C and then submit that to clang? What do you actually need to set here.

Part of the patches you have been submitting have been the breakpoint name chopper's handling of Pascal, but that's independent of the expression parser, and so doesn't so much feature in this discussion.

Jim

dawn added a comment.Jul 30 2015, 1:13 PM

I'm not sure it is that hard. For the expression parser, we can let folks ask for whatever they want, and then just give them whatever we need to give them. The mapping is pretty simple, as I said, ask for ObjC, get ObjC++, ask for C++, for now get ObjC++, etc.

Cool! That can be done in ClangExpressionParser's ctor if you like (see http://reviews.llvm.org/D11102 for an example).

I don't know how you guys are doing Pascal expression evaluation? Do you have a Pascal Clang front-end? Do you do some kind of Pascal->C and then submit that to clang? What do you actually need to set here.

Right now I have a hacked up clang FE until a full-blown Pascal FE can be plugged in. The parsing is controlled by the language options set in the ClangExpressionParser's ctor.

Part of the patches you have been submitting have been the breakpoint name chopper's handling of Pascal, but that's independent of the expression parser, and so doesn't so much feature in this discussion.

Sure, that's fine. This patch was just a proposal as one way to solve the problem via option, and since target.language effects both expressions and breakpoints, it made sense that target.language-follows-frame should as well. You'll notice my initial patch at http://reviews.llvm.org/D11102 affected only expressions and added C++ when OBJC was selected so the tests would still work, assuming that was what was intended. Now that I understand your position better (thanks so much for clarifying), I'll go back to that patch and abandon this one.

dawn abandoned this revision.Jul 30 2015, 1:14 PM

Going to pursue the approach in http://reviews.llvm.org/D11102 based on the feedback here.