Thanks for all the reviews. I'll probably take a break with this now. I'll need to integrate the existing stuff into our internal infrastructure before adding more features.

Well.. I would say that the most user-facing aspect is the /action/ that happens after the user pressed the appropriate key. But at the end of the day, that doesn't matter that much -- we can (and do) test non-user-facing interfaces as well. But those tests make most sense when there is some complex logic in the code. Making sure that a table does not change by keeping another copy of the table is the very definition of a change-detector test (https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html). Like, it may have some value while you're doing the refactoring, but that value quickly drops to zero (or even becomes negative) after you complete it.

• add some explanatory comments
• make the function return a new environment

Thanks for fixing this. I assume that the type is already being added to the type list somewhere else -- it'd be nice to say, for future archaeologists' sake, where that actually happens.

The main reason I wanted to avoid a separate library is because I could not imagine how would it fit into the general library landscape in lldb. It's also moderately strange to have a library for just a single function, but that is something I can live with.

I am still not sure this is the right solution, but a) I don't know what *is* the right solution; and (b) I don't think this makes the situation worse; so I am hitting accept.

This is what I hand in mind. Thanks.

I can't help the feeling that the boilerplate introduced by the enum compression is just not worth the benefit it brings, but otherwise this looks ok apart from the two inline comments.

I am definitely not sad to see lldbBase go away, but I am wondering if we really need a separate library for this functionality (function).

Thanks for the explanation. This makes things a lot clearer (though it doesn't convince me that this is the right approach).

I don't think we actually disagree here. I'm aware of your use case (let's call it "dynamic mode") for appending to a buffer. I agree it's useful and I don't want to change that. What I want to remove is the other mode (non-dynamic). Presently, this is the only mode, but we're not actually making a good use of it. All of the existing use cases can be implemented with a "dynamic" buffer. And the code would be much simpler since there is only one mode to support -- one in which the data encoder owns the buffer it is writing to and can resize it at will.

I just went ahead and did that in a52af6d3714.

I don't have a problem with incomplete patches. I (and llvm, in general) actually strongly incremental development.

All of the changes seem fine, but it's not clear to me what is the value of the new test. To me, it just seems like a change-detector forcing the two versions of the keybindings to stay in sync. The need for the friend declarations and everything also enforces the feeling that this test is not checking what it should be. Did you actually run into some kind of a problem/bug here that you think this test would have prevented?

In D112759#3172488, @noajshu wrote:

Regarding the output behavior, Elfutils debuginfod-find prints the path of the fetched/cached artifact file to stdout. By default, llvm-debuginfod-find does the same. I've also added a command line flag llvm-debuginfod-find --dump which dumps the contents of the artifact to stdout instead of its file path.
If you can only print the path to the cached file, you have to read the stdout and then use that path as the argument to another program (e.g. cat). This is easy to do in a full shell using xargs, or a subshell or variable, but inconvenient in LLVM Lit scripts which use a sequence of LLVM commands chained by pipes.

Does this output behavior of llvm-debuginfod-find and the --dump flag seem reasonable to you? If it's more clear we could change the name to --print-contents or something else. Thanks!

I am somewhat confused by this deduplication logic. I've read https://reviews.llvm.org/D113930#3136404, so I understand why the duplication is happening, but I don't understand _should_ it be happening. IIUC, SymbolFileNativePDB::ParseTypes tries to force the completion of all types. If it succeeds, then there should be no need to add anything else to that type. In fact, adding methods to an already-complete type sounds like a pretty bad idea.

It seems to me that the DataEncoder class is a lot more complicated than what is necessary for our existing use cases. All of them involve creating a new buffer (either empty, or with some pre-existing content), modifying it, and passing it on. A lot of the DataEncoder complexity (IsDynamic, UpdateMemberVariables, ...) stems from the fact that it wants to support writing into unowned buffers, functionality that we don't even use.

I like this feature, but I gotta say that a map feels a bit wasteful as well, given that the last access specifier is only needed for the short duration of time while the class definition is being parsed.

Looks good. Thanks for fixing this up.

I don't really know what to make of this, but adding Jim for the thread plan changes.

It would be better if Jason reviewed this, but my first thoughts after seeing this are:

fix typo

In D114675#3160513, @mgorny wrote:

Well, that's what happens with the core file I have and what you're asking for is really above my paygrade.

I don't think we should be putting this into the DWARFAttribute file. It is substantially higher-level than the rest of the file, and the DWARFAttribute file is destined to be merged with the llvm implementation at some point. Is there a reason for not putting it into DWARFASTParser (like the other patch)?

What is the reason for these differences? The test in question appears to be fairly hermetic (in-tree compiler and linker), so it sounds like something is going wrong inside lldb...

In D114675#3157622, @mgorny wrote:

In D114675#3157595, @labath wrote:

I don't think this is the right solution to this problem.

If it's not, then the expression parser should probably be changed for consistency.

I don't think we'd want to do that (*), but we could definitely make sure that the memory read command produces a meaningful error message instead of a bunch of zeroes. But lets wait to see what Jim thinks about that.

I don't think this is the right solution to this problem.

You need to construct issue appropriately nested ValueCheck objects. Something like this:

children.insert(0,ValueCheck(children=[ValueCheck(value=i), ValueCheck(value=i+1), ValueCheck(i + 2)])

If that doesn't work then that's probably a bug in the ValueCheck matching implementation, as it was definitely written with that mind.

I still like the idea from the inline comment, but mainly clicking request changes to get this off my queue.

I guess this slipped off my radar.... What's the state of this? Are you still interested in the patch? If yes, we can try again after rebasing.

Getting this off my queue. I think the change is fine, but I can also live with the original comment.

Thanks for the review. I'm going to wait until the US folks get back before submitting this.

In D114509#3151035, @mgorny wrote:

That said, I wonder if we should use some formatter (e.g. black) for Python files. While I can't really say I like black's coding style, it's quite popular and using some formatter would be consistent with using clang-format for C++ code.

feedback David

In D114509#3151461, @DavidSpickett wrote:

qemu also does not support macOS

Do you mean qemu-user here?

Yes, definitely.

In D114008#3153211, @danilashtefan wrote:

As we discussed offline, I add the print tests, since SBValue.GetValue() doesn't work for complex structures

In D114288#3152709, @clayborg wrote:

Do all .dwo files claim the main executable that references them is their owning module?

The short answer is "yes".

I've fixed this in rGc0e3bb4d4ba306.

Ed, I don't suppose you by any chance remember more details about that patch? I couldn't find a way to reproduce this with a modern-day qemu, but I also couldn't find any evidence that this ever was an issue, so it is possible (though not very likely, I hope) that I am missing something...

Address Michał's feedback.

thanks

In D114450#3149281, @wallace wrote:

That said, I'm more worried about the strange interactions between libc++ and libstdc++ formatters I reported.

yes, I'm trying to install libc++ on my machine now

I didn't realize that _M_engaged has type bool. That means the max value this could have is 255, which isn't nearly as bad as 0xffffffff. Nonetheless, this is a good change.

I don't believe this solution is correct.

I'm seeing some very funky behavior after this patch. My machine has both libc++ and libstdc++, so both versions of this test run normally, and then libstdc++ fails. If I run just the libstdc++ version then it passes just fine. If I change the test order so that the libstdc++ version is run first, then the *libc++* version fails instead...

Revert the PyInit_lldb changes. They fail on windows due to declspec(dllexport)
mistmatches, and this function is kinda special anyway.