There's a copy paste error in the first header. Other than that, this is fine, thanks for adding the entries!

BTW, I had to fix this patch (cd9e5c32302cd3b34b796683eedb072c6a1cfdc1) to build on macOS. uint64_t and size_t are differently spelled (though I think otherwise equivalent.) One is "long long unsigned int", the other "long unsigned int". I have no idea why that's true, but std::min refuses to compare a size_t and a unit64_t. Anyway, I fixed this by casting one of the two sides of the comparison. But this was causing problems because we have an api (ReadImageData) that takes a uint64_t for the offset and a size_t for the size. That seems a little weird to me, why are these different types?

In D72748#1822443, @clayborg wrote:

So I did a bunch of original IOHandler. And there are many complex cases for sure. One thing to be aware of is that if you won't use editline() and we call fgets() in the default implementation, there is no way to cancel this IIRC. So it might be worth trying this without editline support to make sure things don't deadlock. If the test suite is happy, then this looks worth trying, though with all the complexities I don't think we can guarantee this doesn't cause issues in some unexpected way. The main things that worry me are:

• REPL issues since the REPL and (lldb) prompt switch between themselves in a tricky way where they swap IOHandlers
• running from python script directly when no IOHandlers are pushed because no command interpreter is being run and all the fallout from the cases (HandleCommand that results in a python breakpoint callback etc)
• the process IO handler that does STDIO for a process
• when no editline, we use fgets() that can't be canceled

I agree it should be TypeSystemClang, since that's how we name all the plugins. Other that that this seems like a great change. We should do the same for swift to keep things consistent, though that's not relevant to this patch.

This is a great cleanup. Thanks for doing it.

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

In D70238#1808135, @labath wrote:

Greg, Jim, any thoughts on pidless processes?

In D71801#1794758, @labath wrote:

+Jim, for his thoughts on debugger+interpreter relationship

I think this is the time to step back and discuss the relationship between debugger and script interpreter contexts...

Hey, Mark... The test you added is failing on the Debian bots, e.g.:

I committed this (2a42a5a2f4144cd99812ad0d230480f94a1d1c92) but I forgot to attribute the patch to Mark in the commit message. My apologies!

This looks fine to me. We should let Greg have a final chance to weigh in and then I'll check it in.

Changed the test case from locking.c - using pthreads directly - to locking.cpp using std::thread & Co.

In D71440#1783197, @labath wrote:

This looks like a tricky bug. Thanks for tracking it down.

I have two questions though: :D

• could you use c++11 locking primitives in the test? pthreads do not work on windows

In D71372#1782549, @clayborg wrote:

In D71372#1782538, @jingham wrote:

In D71372#1782536, @clayborg wrote:

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

[0x00000000 - 0x100000000) ---

no permisssions are represented as "---". Then you can take the end address and look it up, and continue. So as long as we aren't getting bogus values back, we are good.

Then what does the bool return mean?

If there is no memory map info in the process plug-in at all, then false will be returned.

In D71372#1782536, @clayborg wrote:

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

[0x00000000 - 0x100000000) ---

no permisssions are represented as "---". Then you can take the end address and look it up, and continue. So as long as we aren't getting bogus values back, we are good.

In D71372#1782512, @mossberg wrote:

Screenshots with new error messages.

(I artificially forced this branch to execute, still unable to make it execute at runtime)

I replaced my 0x22 example with 0x200000000, which is unmapped *and* outside the low 32 bit range. GetLoadAddressPermissions still seems to succeed.

In D71372#1781316, @labath wrote:

I'm not sure how easy it is to do that here, but it would certainly be nice to include these error messages in the actual error output from the "finish" command so that they are visible even without logging enabled.

Please do print out the return address in the log. That will be helpful.

Would you mind adding a mention on the Projects page? This was a neat idea and realistically if it just sits in git, in a few months no one will remember it's there.

Adrian and I talked about this some more. Apparently the idea was that you have some type Foo and you want to look for some error state in instances of that type (Foo::a + Foo::b < 10). So you add a Type Validator for Foo that does this check, and every time lldb prints a variable of type Foo, it will run the Validator on it, and if it fails validation, then the printer will print an ! at the beginning of the printing, and also there's an SB API to get whether the Value passed the validator.

And check the Accept checkbox...

This looks fine to me. So long as there wasn't much strict type-based matching done in the Hardcoded Summary providers, then this shouldn't cause the format matching to get slower.

This looks fine, sorry for leaving it too long in the In tray...

This seems reasonable.

In D71236#1776404, @aprantl wrote:

I'm assuming you already checked that this isn't used in swift-lldb either? Otherwise this obviously LGTM.

If you make the frame format such that it is easy to pull the mangled & demangled names out of the format output (e.g. #${function.mangled-name}# ???${function.name}) then you could check that the two names are different, and for extra credit you could use "lang c++ demangle" to demangle it and match it against the regular mangled name...

One comment on a part you didn't change, but left a C++ specific track in the generic ValueObject code.

In D70883#1771686, @aadsm wrote:

Fair enough, I haven't seen evidence of this (haven't searched for it) but I imagine IDEs need to ignore this as well otherwise they just barf if they're expecting Content-Length and a wild print appears. The notion of stdout of SBDebugger is the SBCommandReturnObject which doesn't print right away (only when the command finishes executing) and from my previous tests .flush() doesn't help with this. I believe this is the reason why people opt to use print instead in their custom commands. But I don't think it's a reasonable assumption (or api contract) to tell people they can't use print or it breaks lldb-vscode.

I'm going to fix this in lldb-vscode itself to wrap all stdout in a proper DAP console response.

I don't think it is wise to use shortened command names like "br" in a test if you aren't testing shortest command string. We don't guarantee that we will keep all currently unique shortened command names unique in perpetuity...

In other places, I've used "XList" to mean the container that manages the things contained, and "XCollection" to be a random possibly unrelated collection of items. It doesn't make any sense to have a collection of breakpoints from more than one target, so the collection class could ensure that, whereas a std::vector is just a random assortment. But I'm not sure that adds enough value to warrant adding a collection container here. std::vector is fine.

Ack, sorry about that! This looks fine, thanks for chasing it down.

Looks right, thanks for catching this.

LGTM

There's a PP above each instance of this code explaining why it's okay if we don't get the API lock, since that's a bit of a tricky point. So if you want to add a comment, it would be better to say "see explanation above".

In D70134#1743075, @clayborg wrote:

In D70134#1742647, @jingham wrote:

This is fine.

I wondered a bit about whether it would be generally useful to add the 'dylibs that have to be copied' to the SBLaunchInfo? It has some other platform'y like things. I'm not strongly promoting the idea, just thought I'd float it.

You can currently do this by setting the platform file spec for a SBModule. If that is set, then the target will upload the module to that path. The main executable always goes to the remote working directory if its platform file spec isn't set, but any module can set this manually. The register shared libraries code sets the platform path.

It looks like all the continue routines in lldbvscode_testcase.py use self.vscode.wait_for_stopped() after setting the target going. Would that fix the problem here? Without knowing anything about the protocol VSCode is using, I can't say that's right. But if launch is asynchronous, as it looks like it is, it seems reasonable to test whether wait_for_stopped() also works for launch. Be weird if it didn't...

This is fine.

In D69704#1742494, @labath wrote:

I think this is fine.

In D69704#1741588, @jingham wrote:

If, for instance, we decided to use .pcm files as a serialization form for type info you would need the same sort of check for modules backed by these pcm's - since the clang in lldb and the clang building the pcm would have to match. BTW I am NOT suggesting that would be a good idea, and this will probably always be a no-op for anything but Swift.

Hm... If I understand modules correctly (and I admit I know very little about them), I think this actually illustrates the my reservations about this. Assuming we were fetching type info from a pcm file etc., there is no relationship between a module (shared library) and a pcm file... I would imagine a module can reference multiple pcm files, and each one could be theoretically built with a different compiler. That then brings up a question of what does it mean for lldb to be "compatible" with the module as a whole...

In D69704#1732484, @labath wrote:

The part that bothers me here is that this assumes that the entirety of a module (or an least the portion of it written in the same language) uses the same version of the language. Is that true for swift? Because it definitely isn't true for c++, as you can easily mix compile units built with different -std flags (and most of the time it will work). So it's not clear to me how this would work for c/c++, or any other language that offers some kind of a version-independent ABI, thus, enabling one to combine different versions in a single module. Nevertheless, since this is how swift works, it seems reasonable to have some way to access this information. However, it's not clear to me if this is the best way to do that...

I don't think either version is right.

Why not have the FindFunctions lambda take a FunctionSP? It would be easy to get the Function name out of the function in the lambda, and that would make the function more general (and also match what the Foreach does...

As long as target.GetREPL (..., true) returns a sensible error for the Dummy target, this is fine. Actually having the dummy target run the REPL seems like a bad idea, since that's a pretty heavy-weight activity and the dummy target is supposed to be just for priming settings for future targets.

In D69425#1731155, @poya wrote:

No commit access, so would need some assistance getting this change into the repo.

In Swift you have to ask the runtime for most of the layout details of objects so getting a const result object, stepping, then asking it to reevaluate itself would lead to passing the runtime incorrect data and potentially undoing a correct type decision that you had made when you fetched the result on the first stop. That's what it sounded like from Davide's description, but I haven't looked at it more deeply yet.

LGTM, thanks!

Addressed review comments.

Ack, yes. That's definitely wrong. Could you write a test for this to make sure we don't break it in the future. This should fail if you make one target, set a breakpoint with the -D option, then make a second target and check that it has the breakpoint, so it should be pretty easy to write the test? That way I won't break it again...

I have no objection to documenting the aliases we have. Having the ability to define shortcuts for common operations was one of the key pieces that allowed the straight command set to be laid out in an orderly fashion, which I think is great for discoverability. It's really hard to do the latter and not have some commands be overly long to type. This gives us the best of both worlds.

Except for the comment comment this looks fine.

As Greg said, iOS (and macOS as well, though less directly) have the notion of bundleID. At present, lldb doesn't directly use/figure out the bundle ID, though it could either from the binary itself or from debugserver, which does have to know that. As far as I know we always also have the path, the bundle ID is inferred from the App package the binary path points to. So that complication should not be present on iOS.

IIUC, the only external change in this patch is that before if you implemented your Python command using a class with an __call__ method, it would have to be the signature that took exe_ctx. Since this is now switching off of the number of arguments (less self) you could make an __call__ form that didn't take the extra argument. I certainly want to discourage that, since the form without the exe_ctx will do the wrong thing in some contexts (e.g. if the command is used in breakpoint callbacks or stop-hooks). It looks like that would be easy to prevent, you know that you've found call right above where you check. So I think it would be better to explicitly disallow callforms that don't take the exe_ctx.

Addressed Pavel's comments.

I added ScriptInterpreter::GetNumArgumentsForCallable, so I can base all the decisions on how to call the function on whether I was passed a function with 3 or 4 arguments. That's cleaner. I also plumbed an error through the callback setting, so I can report an error if you pass me a function with the wrong number of arguments, or provide extra_args when you are passing a function that doesn't take 4 arguments.

In D68671#1704803, @clayborg wrote:

Looks good overall. A few questions:

• should we do a global "s/extra_args/args/" edit in this patch? Not sure what "extra_" is adding? Can we remove?

I have no memory of adding the 'addr' parameter.

One of the oddities of the Search -> Search callback stuff is that the Searcher's Filter might be narrower than the level at which the Search Callback wants to be invoked. The former is whatever the user actually wants limit the search to which is really undetermined, whereas the latter is the level that is convenient for implementing the callback, so there's no necessary linkage. That means that every time the callback finds a match, it has to check with the filter to see if the match falls within the filter. This parameter was supposed to be a way for the searcher to tell the callback that it's search scope exactly matched the filter, so the callback didn't need to check.

Yes, I don't think we need to support this anymore.