This looks fine to me. Makes it really clear that we need SBTarget::FindFirstTypeForLanguage, etc. But FindFirstType was always a crapshoot anyway...

This looks good in general. I also have a few nits, and you need to fix (and probably test) the behavior for files with spaces in their names.

I wonder if we ought to allow target properties (target as an example) that are only for testing, so they don't print when you do settings list, etc. But the we could have some settings like a "target.testing.dont-read-LC_MAIN" and that would make it easy to automate your hand testing. Kind of like the "experimental" decorator, except you have to know they exist to use them...

The nice thing about the way the ObjCLanguageRuntime::Get was that is was only useable where we decided it was legit to use it, in the actual ObjCLanguageRuntime plugin or its direct users. If you want to keep the convenience cast function in a header in Plugins/ExpressionParser/Clang, that would be fine.

Seems like in most places we're just pulling out basic types or their sizes, which we should certainly be able to do with a TypeSystem.

I think I understand your goal - a worthy one, BTW... But I think this is an unnecessary step along that path.

This seems partly wrong to me. The point is that the Target holds keeps scratch AST contexts for all the languages it supports. They are the central repository for the accumulation of effects of expressions evaluated while that target is alive. For instance, all the user defined types and variables go there. The Target also manages its lifecycle. So all the Scratch AST Contexts are properly owned by the target. For instance, in the swift case if there's a module that we can't import, we have to fall back to making an AST Context for each module we can successfully import and dispatching expressions to the appropriate one of those.

That looks good.

Thanks for writing this up!

That would be cleaner.

Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang...

Okay... Provided we can come up with not too torturous ways to get the ObjC and Swift support out of generic-code, it seems okay to do this as a first step. I just don't want to end up in the state where we have ObjCLanguageRuntime as generic but CPPLanguageRuntime is not.

This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type thing. So it seems like this move is in the right direction, but to be really clean needs to represent LLVMUserExpression in the Plugin Hierarchy.

In D64599#1581620, @labath wrote:

In D64599#1581598, @jingham wrote:

My model for this was that there was a CPPLanguageRuntime.cpp that contains everything you can implement about the CPP runtime that is independent of any particular implementation of the CPP language runtime, and then a plugin instance (in this case the ItaniumABILanguageRuntime) that contains all the bits that are specific to a particular implementation. Then putting the CPPLanguageRuntime.cpp in Target lets you know that this has to only contain the generic parts of the runtime behavior. That still seems to me a useful distinction.

I think that's fine, but it does not conflict the idea of the generic parts of a cpp language runtime being a plugin also. This way the generic parts of lldb would be truly language agnostic. Then if anything wants to work with a c++ runtime, but it does not care which one, it can use CPPLanguageRuntime. And if you need a very specific runtime, you go for the Itanium thingy.

In D64599#1581604, @labath wrote:

What is the indented relationship between CPPLanguage and CPPLanguageRuntime plugins (and generally between any Language and its LanguageRuntime)? Right now you're having the CPPLanguage depend on the CPPLanguageRuntime plugin. There is no reverse dependency, so this may be fine, if that's how we intend things to be. However, it's not clear to me whether that's the right way to organize things. Intuitively, I'd expect the LanguageRuntime to depend on Language, and not the other way around...

My model for this was that there was a CPPLanguageRuntime.cpp that contains everything you can implement about the CPP runtime that is independent of any particular implementation of the CPP language runtime, and then a plugin instance (in this case the ItaniumABILanguageRuntime) that contains all the bits that are specific to a particular implementation. Then putting the CPPLanguageRuntime.cpp in Target lets you know that this has to only contain the generic parts of the runtime behavior. That still seems to me a useful distinction.

Do you know how you are going to do enum option values?

Why did you make this a function of Variable, rather than SymbolContextScope?

LGTM

Sure, NP.

Addressed Greg's comments

This looks good to me. I wonder if the SymbolContextScope -> Language calculation that you do in IsRuntimeSupportValue should really be done in SymbolContextScope? If that's going to be the policy for going from SymbolContextScope, maybe centralize it there?

Addresses Greg's question about what happens when we load a new OS plugin. Indeed we should limit the work we do only to the case where we didn't have an OS plugin, then tried to load one and succeeded. Only then do we need to clear and update the thread list.

In D63240#1561488, @xiaobai wrote:

@jingham: Okay, so I tried to do what you suggested and that solution doesn't work because of ObjC++. After looking into it, it looks like Variable and Function just ask the CompileUnit for the language type instead of determining it themselves meaning that we determining the Variable's language boils down to asking the CompileUnit for its language. That can probably be improved, but I think that just relying on the SymbolContextScope alone isn't yet sufficient. I think it would be worth it to ask the SymbolContextScope before trying all the loaded language runtimes though. Would you be okay with that solution?

In D63240#1560038, @xiaobai wrote:

In D63240#1559994, @jingham wrote:

To be more precise, "frame" is the wrong word to use, Variables have "scopes"... All Variables have a scope, though not every scope is contained in a Function.

But just to back up a little bit.

We're getting into problems, it seems to me, because we're asking ValueObjects questions that should be asked of their containing scope. For instance, if you are in a Function, and want to list the locals and arguments, you want to know what the rules for the Function are for which artificial variables should and should not be printed, not any of its value objects.

For instance, ObjC could decide that it wants to reserve the word "this" in methods of ObjC classes in ObjC++ files, and use it as a should-be-hidden artificial variable that will always store a pointer to C++ object for some evil purposes. Asking the runtime language of this artificial "this" variable whether the word "this" is runtime whitelisted seems like the wrong thing to do. After all, the Variable has a CompilerType that's clearly a C++ object, so you might reasonably expect its runtime language to be C++. But then you would get the wrong answer. You really need to ask the Function "what are your rules for hiding artificial variables".

Getting to the right actor to ask about "which artificial variables to show" was the reason for suggesting that the runtime language of a Variable should be that of its containing scope. But then you get into the weird situation where a C++ Variable is claiming a runtime language of ObjC++, which seems a little odd. Instead, it makes more sense to get the RuntimeLanguage of the current frame, and then iterate over the variables and ask the frame's runtime whether they should be shown or not.

I'm not sure what to do about global variables. Reasoning by analogy, that's something that the language of the CompileUnit which contains the global variables should answer.

If I understood you correctly, your conclusion is that ValueObject::IsRuntimeSupportValue shouldn't really exist and that you should be asking a frame or a compilation unit if a value should be displayed to the user. I think that this is reasonable. That being said, I think that this idea will have some interesting challenges as well. One problem that I'm aware of at the moment is that SBValue has IsRuntimeSupportValue. If I remember correctly, removing from the SBAPI is generally not permitted (something I disagree with in general, but I digress).

In the short term, I'd like to get this patch (or some variation of it) into LLDB so that we can remove ValueObject's dependence on ObjC (after this there's one reference left but I have a good idea of how to remove it). Would you mind if I committed this, or do you feel strongly that we should first refactor further?

In D63240#1559931, @xiaobai wrote:

In D63240#1559913, @jingham wrote:

Shouldn't ValueObjectVariables figure out their runtime language from their defining frame, not their CompilerType? For a ValueObject you get from an expression, you probably can't do that. But we're always talking about hiding locals or args here - i.e. they are all ValueObjectVariables. And it seems to me that in that case getting the RuntimeLanguage from the containing frame is much more useful than from the CompilerType.

Does every variable have an associated frame? I imagine things like global variables wouldn't. I'm not sure if any language or implementation of a language has global variables as compiler/runtime helper variables, but I'm not comfortable making that assumption since one of my goals is more generalized language support.

Shouldn't ValueObjectVariables figure out their runtime language from their defining frame, not their CompilerType? For a ValueObject you get from an expression, you probably can't do that. But we're always talking about hiding locals or args here - i.e. they are all ValueObjectVariables. And it seems to me that in that case getting the RuntimeLanguage from the containing frame is much more useful than from the CompilerType.

In D63622#1553121, @labath wrote:

Seems like a reasonable thing to do, but I don't really know what this code does...

This change makes it clear that SBTarget::FindFirstType should take a language, but that is orthogonal to this change.

In D63363#1545427, @labath wrote:

Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that.

I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it.

I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. Plugins/Process/Utility (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there.

Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me.

This looks fine to me.

In D63181#1539292, @xiaobai wrote:

In D63181#1539291, @JDevlieghere wrote:

Have you considered making just AddExceptionPrecondition virtual? Wouldn't that solve the problem too, without the code duplication of making CreateExceptionBreakpoint virtual?

I don't believe so. As I understand it, you might not have a process yet when you call LanguageRuntime::CreateExceptionBreakpoint from Target, so you won't have an instance of LanguageRuntime. You could create an instance of a LanguageRuntime object with a nullptr process just to invoke it as an instance method, but that seems like the wrong choice to me. All this combined has led me to believe that AddExceptionPrecondition needs to get a callback to a static function from PluginManager.

Pretty much the only actual effect of this patch (besides its changes to the dependency graph) are introducing the possibility for ambiguity here. It seems more logical to do it all as one patch.

This looks fine to me.

I have no problem with the change in general. However, you've introduced the possibility of name collision between convenience type definition in various languages. So it would be good to run through the persistent DECL's for ALL the supported languages and report collisions. Probably also need to add a -l flag to pick a language?

Fine by me.

This deals with my objections so I'm marking as accepted because that's the only way I can see to unblock. Seems like you were still discussing return types, and I don't want to preempt that, however...

In D62755#1525919, @xiaobai wrote:

In D62755#1525890, @aprantl wrote:

I don't yet see the connection between those goals and this patch, but I might be missing something. Would CPPLanguageRuntime need to be anything but a forward declaration in Process.h?

I forgot to address this point. CPPLanguageRuntime would not be a forward declaration in Process.h, nor be referred to anywhere in Process.h

This seems like carrying purity a little too far. You haven't removed the fact that the using code is explicitly dialing up the C++ language runtime, you've just made the call-site a little harder to read.

There should be a test to go along with this.

LGTM

LGTM

The typical trick for doing argument substitution before debugging was roughly to debug:

Then put in a comment saying something like "LLDB ONLY stores history files in .lldb and if you don't like that..." If you are instituting a policy which is not common you should at least document it...

Most other programs write their history files in ~. So we are being a little odd in offering to put them in ~/.lldb, though I agree that is convenient.

I don't think that's quite right. I think the behavior should be:

Why are we calling this "widehistory" because we couldn't write into the .lldb directory? I know that's the way it was but it makes no sense. I guess it would make sense to call it widehistory if edit line was in wchar mode, that way you wouldn't ask a non wchar edit line to read a wchar history file...

In D61921#1503331, @xiaobai wrote:

In D61921#1503282, @jingham wrote:

In D61921#1502502, @labath wrote:

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

+1

Yeah this seems reasonable.

In D61921#1503290, @jingham wrote:

On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift. But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded. A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process. So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones. Not that we didn't need one or the other iterator.

Okay, that makes sense to me. Thanks for clearing that up. If I understand correctly (which I think I do), creating an iterator abstraction to iterate over the process' currently loaded runtimes instead of over every available runtime is the better option here. I'll follow up with that change later today or tomorrow unless somebody believes this is the wrong idea.

In D61921#1502352, @xiaobai wrote:

In D61921#1502338, @jingham wrote:

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

It's fine to do that in a separate patch. If you do that it would be good to back port it to the other iterations over the LanguageRuntimes.

Excellent, sounds good. I definitely intend to backport it to any other iterations over LanguageRuntimes that I find. As for iterating over all available LanguageRuntimes, this patch and my last few have sufficient motivation for doing so, no? Maybe I'm misunderstanding what you mean.

In D61921#1502502, @labath wrote:

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

There is a TypeSystemEnumerateSupportedLanguages that we use so that we don't have to enumerate over all the language in the languages enums. After all the plugin manager knows which languages we have type systems for. If you're going to be doing a lot of this generalization, can you add a similar enumeration for the LanguageRuntimes? There are 40 or so languages in the language enum but we only have a couple of LanguageRuntimes...

Excellent, thanks! Can you check this in or do you need me to?

In D61737#1498329, @jankratochvil wrote:

In D61737#1498297, @jingham wrote:

If you have simple gdb startup commands then translating them into the lldb way should be straightforward, and the burden of learning how lldb works from the command line for these purposes is just not that great and if you are going to use lldb you should just buck up and browse "lldb --help" a bit... If you have complex gdb command lines, you are going to have to rework them anyway to deal with other differences between lldb and gdb. I don't think mixing the two would add enough value to be worth the uglification.

I use GDB to debug LLDB like: gdb -q -ex 'set break pend on' -ex 'set build-id-verbose 0' -ex 'set env ASAN_OPTIONS=alloc_dealloc_mismatch=0:detect_leaks=0' -ex 'set env LD_PRELOAD=/lib64/libasan.so.5' -ex "set env PYTHONPATH=$PWD/lib64/python2.7/site-packages:$PWD/lib64/python2.7/site-packages/lldb" -ex "set env LD_LIBRARY_PATH=$PWD/lib:$PWD/lib64/python2.7/site-packages/lldb" -iex 'set breakpoint pending on' -iex 'b __sanitizer::internal__exit' -ex 'b lldb_private::Module::ReportError' -ex 'b lldb_private::Host::SystemLog' -ex r -ex 'set scheduler-locking on' --args python ../llvm-git/tools/lldb/test/dotest.py --executable $PWD/bin/lldb -C $PWD/bin/clang --log-success -t ../llvm-git/tools/lldb/packages/Python/lldbsuite/test/ -f HelloWatchpointTestCase.test_hello_watchpoint_using_watchpoint_set_dwarf

With appropriate comments, this seems fine.

I really don't want to pollute the lldb driver options with gdb equivalents (or really any duplicate spellings of already present functionality). For the lldb command line, I want us to have the freedom to do the best job of making the lldb options consistent and easy to document, understand and use, and adding in random duplicate options from gdb will only make this harder.

If you really are going to support many languages you need to figure out how to tell folks what really happened with more specificity. For instance, you can use C++ to throw anything, so you could throw an ObjC object from a C++ exception throw. So you need to distinguish between the language of the exception thrown (which is captured by the ValueObject you return) and the language runtime throwing the language. So we need a way to query that. Also, there's no formula reason why you couldn't have two languages throwing an exception at the same time (for instance if a C++ exception is unwinding the stack and the destructor of some ObjC object that is getting destroyed throws an NSException, etc... So there needs to be some way to handle that.

The one outstanding bit of work here is that this change requires that the MSInst "IsCall" function has to mean "will return to the next instruction after call" or we might lose control of the program. It seems obvious that that SHOULD be what it means, but we need to make sure that is always going to be what it means or we risk losing control of the program. Greg is going to follow up on that.

I would rather not clutter up the lldb command driver's options with gdb command flags. That seems like it will make lldb harder to figure out and reduce our freedom to choose reasonable short names for lldb driver options.