Also naming quibble...

I think you have to protect against your dyn_cast failing.

It's a little weird that the CommandObjectExpression is telling the REPL what its options should be, it would be more natural to go the other way. Maybe have the repl_sp provide a properly set up set of EvaluateExpressionOptions and then copy them over to this CommandObjectExpression execution?

I want to keep this commit simple so I'm leaving Adrian's substantial comments for a future commit.

I forgot to sort the project file before making the diff. I'll do that on commit.

Yes, looks fine.

In D58838#1415646, @aprantl wrote:

In D58838#1415637, @zturner wrote:

Yea it would be nice if we could remove all of the LLDB_CONFIGURATION_xxx macros and just use either the LLVM ones or standard ones such as NDEBUG

I generally agree with this, but we need to decide what to do on a case-by-case basis. So far I found four categories of LLDB_CONFIGURATION_DEBUG

1. assertions that should just be "assert"
2. expensive checks that should be guarded by LLVM_ENABLE_EXPENSIVE_CHECKS instead
3. Consistency checks that assume that the debug info is 100% accurate/complete and that may fail in the real world
4. additional logging
   
   there may be other cases that I haven't found yet.

In D58792#1415645, @clayborg wrote:

In D58792#1415629, @zturner wrote:

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

yes. Xcode. For Swift, Xcode runs the LLDBRPC.framework in process which runs lldb-rpc-server out of process with the DYLD_FRAMEWORK_PATH set to the directory that contains the LLDB.framework that lldb-rpc-server wants to run with. API is very important and without it this would not have been possible. So API is very important.

That being said, at some point we should make a lldb2 namespace and make a liblldb2.so and cull out all of the fluff we are keeping in there and get down to the exact API we want. We can then change things to be const where needed. remove redundant calls. In the name of backward compatibility we have functions that didn't return an error which now do so we have two versions of similar cal, one with error reporting and one without. Or the 5 flavors of CreateTarget in SBDebugger. There are many others. So at some point it would be nice to go through and reduce and improve the API. That will take time though cause if we do it, we should get it right.

In D58792#1415629, @zturner wrote:

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

This is fine by me, though Greg I think still had an outstanding question. If he's satisfied, I am.

Excellent, thanks! I didn't think to look there.

This seems reasonable to me. Any Host implementation can live off source/Host/common/Symbols.cpp, and only needs to do anything if it wants to augment that behavior. Seen that way, it is not defining API's that are a required step in porting to a new platform. So the API definitions don't need to be specially called out as being required of a Host implementation. And the implementation is still in source/Host so it's clear that this does provide host specific functionality. So that seems fine to me.

I chose not to do it that way because I don't think it will ever be the case the msgSend_stret will NOT be a struct return convention so the test isn't relevant (and is a bit confusing) there. And we may at some point need to do some other work that depends on the flavor of msgSend, so I wanted to keep the distinction.

I'm fine with leaving the reporting as is. This really only happens in fairly restricted situations (only hardware breakpoints) and neither way of reporting the failure seems much better to me, so we needn't over-polish it.

One comment seems to have wandered out of place.

I don't remember why this was inserted as separate pass. Maybe we were still early on learning what SWIG could do? That was a long time ago now...

I have two questions about this patch.

In D58678#1410861, @zturner wrote:

Since we are stepping over we can safely ignore these calls since they will return to the next instruction

What if the call throws an exception?

There aren't many more platforms (OpenVMS?) that are likely to show up and need support for lldb, so maybe this is making too much of the matter. But having there be a well defined set of places where you need to look when porting lldb to a new host platform seems a useful design to me. The fact that we weren't strict enough and allowed Host platform dependencies to creep in where they don't belong doesn't argue we should just abandon trying to keep the places where a port sockets into lldb regular.

Utility is supposed to be a bunch of stand-alone utility files & headers that we gather together for convenience's sake.

Is this the right way to do it? I don't know why the other was UNSUPPORTED: linux. The only other XFAIL for linux I could see spelled it -linux-?

Pavel, I think the new behavior w.r.t. launching is better. When the process plugins are managing events to handle a launch or attach, they really don't want random stop hooks getting in their way. On MacOS, if you set a stop hook before launch you always got one stop message at a place you didn't cause the target to stop, which I think was just confusing.

Trying to guess how thread list output is going to look is indeed a losing game.

I wasn't using the right %lldb when running the tests. That provides the 'echo-comment-commands false'. Run correctly you don't need the command in the test files.

BTW, I still think that we should add an explicit way to indicate that stop hooks should continue. But the current intended behavior was regular. If any stop hook caused the target to proceed, it would terminate at that point and all the other stop hooks would be jettisoned. But if the stop hook gets (incorrectly) run in sync mode, we can no longer implement that policy.

Ah, I see what happened.

I haven't gotten a chance to revisit the stop hooks in a long time. They definitely need some love.

That code seems amazingly good to me ;-)

You are using a mix of llvm & lldb naming conventions for local variables and arguments and the ivars of UserIDResolver (lots of "Uid", etc...) Probably better to stick with the lldb convention.

LGTM.

Might be clearer to do:

LGTM

Thanks for adding the ObjC tests! I had two trivial requests for the test comments to be a little clearer, but even without that this is fine.

That looks good. Could you add a test for this setting to the ./functionalities/jitloader_gdb/TestJITLoaderGDB.py test? I wouldn't test that the default has any particular behavior because that might change over time. But test that if you turn it on, you do get load notifications, and if you turn it off you don't. Thanks!

It would be better to have the setting be an enum of "on/off/default", and then have the somebody - the current DynamicLoader plugin seems the best somebody - provide the default value if the setting hasn't been explicitly set. That way on any platform one could turn the loading on and off, which seems useful, and we wouldn't have to have a Darwin specific setting that will cease being applicable when the Darwin default switches.

In D57552#1381094, @ted wrote:

In D57552#1381046, @jingham wrote:

In D57552#1380782, @ted wrote:

FYI, @jingham , this is also an issue with DYLD plugins, if the file in the link map starts with ".". "image search-paths add . /path/to/my/libraries" fails in this case, because "./library.so" gets changed to "library.so".

This should fix that issue as well, since image search-paths are also PathMappingLists. I don't see any tests for this, however. Do you have something handy you could turn into a test?

I don't think this will fix it, because the problem happens in the initial FileSpec, before we get to search-paths.

From source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp, DYLDRendezvous::ReadSOEntryFromMemory():

std::string file_path = ReadStringFromMemory(entry.path_addr);
entry.file_spec.SetFile(file_path, false, FileSpec::Style::native);

FileSpec::SetFile normalizes the path, which will remove a leading "./", so "image search-paths add . /path/to/my/lib", which would work with "./mylib.so", won't work with "mylib.so".

I think I can write a test that uses LD_LIBRARY_PATH and a subdirectory so the target can load "./mylib.so", but LLDB won't be able to see it without a search-path.

Sure, that's clearer.