This is all about the decision in "batch" mode for what constitutes a stop by the program that should cause batch mode to return the control to the user.

This looks right.

In rL372016#689059, @JDevlieghere wrote:

This is failing to build on Darwin:

`
 /Volumes/Jonas/llvm/llvm-project/libcxx/include/__config:1091:58: error: function-like macro '_LIBCPP_GLIBC_PREREQ' is not defined
 #  if (defined(__ANDROID__) && __ANDROID_API__ >= 30) || _LIBCPP_GLIBC_PREREQ(2, 30)

Our CMake bot was offline for a while, but I'm pretty sure it'll start showing up here: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/

Approved with that addition.

The help for -f should say what happens if this option isn't provided. Otherwise this looks fine.

Looks right.

LGTM

Excellent!

I don't think CallNoArgNoReturnFunc is the right name. This routine calls a function that takes no arguments but returns a void *.

There's already Utils/StreamGDBRemote.cpp... It might be natural to stick the class in there?

It seems weird to me to have the GDBRemotePacket class live in the repro namespace. There's nothing particularly Reproducer specific about it, and it clearly gets used outside the reproducer as well. Wouldn't it make more sense to have this in Utils?

The code looks fine to me.

In D67472#1668505, @clayborg wrote:

lgtm. Jim?

Also, since in C++ you can't do:

Your description says that if there are bits not belonging to the enum, you will print them after the enum values are listed. But you don't have a test for that. I thought you were going to use the "nonsense" var for that, but then you didn't...

If I've run lldb like:

This seems like the right place to put it, it's clearly something you would do with a process.

This pattern repeated a couple of times north or the one you fixed. Can you get those too?

That was the only test that tested the "command regex" functionality. It was probably called TestFormats.py because somebody copied over a test without changing the file name since that name makes no sense.

Does anybody know what this was supposed to test?

I use -# for running single tests multiple times. This is useful particularly if you have a test that only fails sometimes, you can do:

The whole command flags was a late addition, but we were using it for new commands and "when you touch it" kind of changes. That seems to have stalled the conversion, so thanks for completing this!

LGTM

You can go a long time without actually using the Dummy target, which is why I made it lazily.

Pavel said "we'll just have to bite the bullet and say that our expressions are now (more or less) POSIX conformant"... We should say this somewhere users are likely to find.

I don't think you need to go back and cover all the ones already added, but could we start adding tests for new lldb/llvm data formatters as we add them? That way we can keep them useful as people change llvm.

In D66241#1631426, @clayborg wrote:

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

Does this seem clearer? We use the language of Private and Public StopInfo's in a bunch of places, even though at present that's more about "How" you get them than actual separate entities. But still, this seems like it follows the language we are using elsewhere.

Do you think I should put more verbiage in the comment, or would this have been clear if you were actually working on the code?

I think we need to fix the behavior for "", other than that, this looks fine to me.

Thanks for pointing that out.

Fixed the inverted call order.

In D56010#1623762, @amccarth wrote:

It looks like the code changes landed (probably as part of another patch) but not the tests. I'll look and see if the tests should to be revived.

Given that we do some work to find executables (like search along the path for "ls" -> /bin/ls") which work might not end up with the result you expected, printing the full path seems useful.

Thanks for doing this. Looks okay to me just one typo... But I don't know CMake well enough to comment on substance...

That would be fine. In the swift REPL, we increment the line number so that it appears that all the expressions are one contiguous source file. But I think for the expression parser treating each expression as a separate file is a better fiction.

In D65646#1612436, @aprantl wrote:

I have a question based on my half-knowledge about the expression evaluator: I thought that we already wrote out a temporary file for the expression in order to support expr -g? Is this orthogonal or do we now have to ways of pretending there is a source file backing up the expression?

Okay, if clang-format won't undo this, then I'm fine with writing them this way.

IIRC both GetPath and ResolvePath return the number of characters it would take to actually resolve the path. 128 is actually pretty short, so you should check the result and malloc a buffer of the return + 1 if 128 ends up not being enough.

If you just want the path and don't need the FileSpec, you can call the static SBFileSpec::ResolvePath.

What happens if you clang-format your reformatted version of the setter? Do we need clang-format off for them?

If we don't want to forget that there was specific useful info, we could gate this patch on implementing the "reproducer generate <FEATURE>" feature. That should be easy to add if we're clear this this is the right design for reproducers. But I don't think that "bugreport unwind" was all that much used, so it might be better to design this at leisure.

The thing the "bugreport unwind" adds is that it runs a handful of commands that gather data useful in diagnosing unwind problems. That's useful when the information you need might not be output by the session in which the bug appeared.

It might be good to see if you can trigger the bug more directly (for instance by getting the SBType for the method and pulling out its arguments?) The reason that a breakpoint triggers this at present is that lldb reads the debug info for the function to find the prologue extents so it can more accurately push the breakpoint past the prologue. But that's not really desirable, it is a lot of work just for setting a breakpoint, and if I ever figure out how not to do that, I will. In which case, this test will no longer test what you think it tests.

It worries me a little bit that we are making it harder and harder to figure out "where does the option for "-t" get stored once this CommandObject's options have been parsed. Can you show the steps I would have to go through to get from "-f" to OptionEnumSettingsSet::Force or whatever.

This seems okay to me. I would also check with Jason. I don't know who coordinates running tests remotely - or even if they can run in parallel now. But that's the one bit I don't understand well enough to say yea or nay on.

It seems to me part of the goal of Alex's efforts is to make it possible for a given lldb to have or not have support for any particular type system. In some future day they might even be live pluggable, so that which ones get loaded would be a user choice. In that future, it is never an error to not have a given type system. And even if lldb has builtin code to support a given type system, I may not want to pay the cost to construct it. If I'm debugging ObjC code in a program that also supports swift, I might want to tell lldb not to bother with swift types.

Yeah, I didn't want to bother with formatting in case there were suggestions on content. I checked in a version that's correctly truncated.

There isn't an SB API for "settings set" yet. I've put off doing that because the "settings" subsystem is currently unfinished. In the original design you would be specify qualifiers in the setting hierarchy, like:

In D65109#1598695, @sgraenitz wrote:

We discussed this and came to an agreement only a few hours before in the team meeting

After all, LLVM is open-source and has a community. Making preemptive decisions in internal team meetings sounds concerning.

As far as we know, Greg was the only user of the Xcode project outside of Apple, and he gave the thumbs up.

I was a user of the Xcode project before Apple.

I don't think removing it was especially controversial

That's right. And still, it would be appreciated to give folks in other time zones at least a chance to take part. Thks

Sure, this is okay.

In D65165#1598553, @labath wrote:

Thanks for responding quickly. LGTM, with one comment..

I agree with Greg, having one function that can do any of the combinations of stdout & stderr seems more convenient.

Actually, I don't want this change as is. Some logs - like the expression and step logs - are laid out for readability, and LLDB_LOG automatically adds the file & function which will make them much harder to read.

IIUC, LLDB_LOG already adds the file and function. A bunch of these logs are also adding FUNCTION, so that's probably going to come out twice now. You should probably remove the duplicates.

Sounds reasonable.

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.