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
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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 | |
Comment Actions
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). | |
Comment Actions
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). | |
Comment Actions
So we accurately set the
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?Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
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.
Comment Actions
Going to pursue the approach in http://reviews.llvm.org/D11102 based on the feedback here.
Remove this, we shouldn't need this setting.