This sounds like a good idea, but I'd like to get another set of eyes on the llvm part.

Ok, this is something we can talk about. dwarf parsing is a big bottle neck, so spending some time to optimize that (and even sacrifice some readability/mantainability/etc. if needed) is worthwhile.

This is fixing the same issue that I tried to fix with D84402, but then failed to get back to it when the discussion about the best approach started getting long. While I do have some reservations about this approach, it is definitive improvement than the status quo, so I won't start bikeshedding the design (but do see the inline comments)..

In D87675#2273831, @dmantipov wrote:

Use std::pair rather than an ad-hoc two-member trivial structures. Since std::pair provides the default constructor to initialize both members, no extra C++ glue is required for emplace_back()'ing the pairs into containers.

In D87590#2273721, @haampie wrote:

Also note that the changes are not likely to be backported to 11 even: https://reviews.llvm.org/rG31e5f7120bdd2f76337686d9d169b1c00e6ee69c#942622.

@tstellar, who is (was) the release manager for 10.0, can make a definitive call, but our usual modus operandi is to have one point release for each major version. I don't know if this is a sufficiently major issue to break from that (and it definitely is risky).

Using emplacement instead of construction is a good idea, where it makes sense. However, I am not convinced that adding constructors to otherwise-trivial structures is worth it. These structures are usually trivially copyable and/or movable and so an emplacement does not really save much.

(Sorry about the delay.) Given the current requirements, I think this patch is fine (excellent even).

This looks fine (sorry about the delay, I've been OOO).

I'm not married to the current way we process commands, but I do value their consistency (both between different commands, and between a command and their documentation). This would make script behave differently than expr even though they have identical modes of operation (raw input, special handling of empty argument, etc.), and differently from its documentation. The help script command contains this:

Invoke the script interpreter with provided code and display any results.  Start the interactive
interpreter if no code is supplied.  Expects 'raw' input (see 'help raw-input'.)

I'm not sure how this is patch coded, but probably after the argument are added, this additional snippet will appear:

Important Note: Because this command takes 'raw' input, if you use any command options you must use
 ' -- ' between the end of the command options and the beginning of the raw input.

help raw-input says this:

<raw-input> -- Free-form text passed to a command without prior interpretation, allowing spaces
               without requiring quotes.  To pass arguments and free form text put two dashes ' -- '
               between the last argument and any raw input.

If we do want to change that, we should at least make sure all of these things reflect that. And yeah, that should be a separate patch (maybe a small RFC even).

In D86670#2244876, @clayborg wrote:

In D86670#2244092, @labath wrote:

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target. "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

I'm pretty sure the json changes should get a separate review. The json owner (let's call him @sammccall) might have a different idea on how to do this thing...

I'd like to also pull in Jason for this, since this is really the trickiest part of the whole patchset.

There are things I'd want to change here, but I think this is patch is fundamentally ok. Before going into that, I think we should first sort out the dependant patch.

Looks good.

In D86996#2252561, @JDevlieghere wrote:

In D86996#2252520, @JDevlieghere wrote:

In D86996#2252246, @labath wrote:

I agree that silly, but maybe the same fix should then be applied to the expr command (and any other command with similar behavior).

Sure, we can use the same trick in other places that use OptionsWithRaw. I'll do that in a separate patch.

Actually, after discussion this with @teemperor offline, for the expo command we can't use that trick because a valid option might also be a valid expression. expr --flag would parse correctly if flag was an option, but it might also be an expression that decrements --flag.

I don't know anything about Sphinx. It might be useful to mention in the patch description the motivation for this change.

In D86792#2252243, @DavidSpickett wrote:

Combined two functions into one single run command
and check message function.

With some odd kwargs, but I think it helps to be able
to put the expected lines on the end.

In D86996#2251508, @JDevlieghere wrote:

because I thought it would be silly to require you to type script --language lua -- to launch the interactive Lua interpreter.

This is great, thanks for doing this. Just a couple of minor comments inline.

Yay

Thanks, this looks good to me now.

I like this. And a big thank-you for writing the tests. We don't usually bother to write self-tests for the test infrastructure, but we definitely should be doing that. The place is slightly weird, but I think it will do for now -- it's easy to move this around if we find a better place for it.

I'm going to respond to the rest of your (very insightful) comment later. So far, I'm just responding to this:

This isn't exactly layout related, but there is the question of covariant methods. If a method is covariant, then its return type must be complete.

We already import all the base classes of a derived class (at least in full import). But, we do not import all the derived classes during the import of the base. Is this what you do in LLDB to support covariant return types?

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

In D85705#2243536, @wallace wrote:

• I'm still using StructuredData for the parsing. There's a chance that once we release this feature users will want support for other formats besides JSON, so for now I prefer to ask for JSON input but parse in a format-agnostic way. If eventually no one needs any other format, we can switch to JSON-only parsing.

This is going to derail this patch somewhat (sorry), but since this is an important long-standing open issue, I think we should discuss it in more detail.

This looks like it could work, though only barely. @skipIfWindows checks for target system, but we would really need this to check the host OS (for remote testing, although I don't know if anyone does that on windows these days). However, in conjuction with @skipIfRemote it should do the right thing. In any case, this is small enough a change to try it out. Just be sure to check the windows bot afterwards.

In D86667#2243781, @mib wrote:

In D86667#2242566, @jingham wrote:

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

I think platform shell --shell sounds/looks repetitive so I opted for -i|--interpreter instead, as in Command-line Interpreter.

In D86616#2238946, @ldionne wrote:

LGTM, but I'm not an owner for any of the projects touched by this change.

Looks good.

If the format is going to be json, why not use llvm::json from llvm/Support/JSON.h? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense.

I am wondering what should the platform classes which do not implement this feature do (although most of them could implemented I don't think you need to implement all of them -- PlatformGdbRemote in particular can be a bit tricky). It seems to me like it would be better for them to bail out if a non-standard shell is requested instead of silently executing a command the user did not intend.

In D86436#2234110, @davide wrote:

@labath something I noticed when finding this (and related bugs) is that frame var carries a decent diagnostic

(int *) l_125 = <empty constant data>

and the expression parser returns something not particularly useful:

(lldb) p l_125 
error: <lldb wrapper prefix>:43:31: no member named 'l_125' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::l_125;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'l_125'
l_125

From my testing infrastructure/fuzzing perspective the two are indistinguishable, as the script I've written chokes on both, but it would be better from an ergonomics point of view if p would return something meaningful, if possible (even if there's a bug in lldb). Do you think it's worth filing a PR? (also, cc: @teemperor for ideas as he spent a fair amount of time working on the expression parser)

Sorry, I missed this patch. Modulo the inline comments and the missing windows support and tests, I think this patch is ok-ish. However, I am still not convinced of its usefulness. Why would we be doing something (particularly a thing which will be hard to do in a cross-platform manner, and will very likely border on, or downright cross into, undefined behavior territory), if we get that from vscode for free?

Thanks for the clarification. The new version looks great.

Though these extra messages are often superfluous in general, I don't think that's the case for this particular message. This macro is only used in the expect and match checks. These are the most common way of asserting something right now, so I think it's reasonable to spend some effort to make the errors it produces clear and actionable. The current state is pretty far from ideal.

Sounds good. FWIW, I wouldn't be opposed to some CLI command which would dump some low-level details of the register context, similar to how we have the "target modules dump" command tree for Module objects...

This looks good, though it would be nice to add a test for it. It looks like TestGdbRemoteExpeditedRegisters.py has a lot of functionality that could be reused for this purpose.

I like this a lot. LGTM with some small fixes inline.

Sorry about the delay. This looks good to me now.

In D86348#2231094, @aprantl wrote:

This looks nice! I'm somewhat suspicious that the new test doesn't specifically test the union case of the old test, but I'm assuming that would still work and your simpler tests covers the same code?

If I correctly understand what this is doing, then I don't think it's a good idea. The base of an (elf) shared library does not have to be mapped executable. These are the mappings I get for a trivial hello world program (no mmapping of libraries or anything) on my linux machine:

00400000-00401000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r-xp 00001000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00403000-00404000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00404000-00405000 rw-p 00003000 fd:01 2838574                            /tmp/l/a.out
020fb000-0211c000 rw-p 00000000 00:00 0                                  [heap]
7fe4f5d87000-7fe4f5da9000 r--p 00000000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932                    /lib64/libc-2.31.so
...

Here, the correct base of a.out is 0x00400000 and the libc base is 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 0x7fe4f5da9000, respectively.

I actually think tests relying on this should be converted to c++ unit tests. The logic to compile and link against liblldb takes up a considerable chunk of our test buildsystem. Why recreate that logic, when cmake knows how to do that already? TestPluginCommands might be more involved, but converting api/command-return-object should be trivial using the api unit test framework we already have (unittests/API)...

thanks

Actually, keep the old const_value test, but give it a bitfield-specific name. Testing bitfields+const_values is still interesting.

In D86261#2229341, @clayborg wrote:

Hopefully this should be good to go, let me know if anyone has any issues.

LGTM, modulo the defensive check. Incidentally, I was just looking at a DW_AT_const_value bug, but I think it's a different issue than this one.

If you start to need to make more changes for your project, we'll need to have a bigger discussion, but right now, this seems ok.

Also, +@markmentovai, in case he has any thoughts on this.

This has been a feature we've been missing for a while. Thanks for implementing it.

I looked at the gdb-remote docs, and I could find precedents for both returning error (or OK) for "empty" responses, and prefixing the responses with a random character. So I think adding the M character is fine too.