In D67390#1672210, @kwk wrote:

So my point of this whole question is: What makes a symbol unique in the sense that it shouldn't be added to the symtab if it is already there?

In D67390#1667270, @kwk wrote:

@labath how shall we go about this? We do have the situation that when you lookup a symbol you might find it twice if it is in .dynsym and in .symtab. Shall I adjust the test expectation to that or change my implementation?

+ Jim for SB API.

lgtm

Sorry for the delay, I was OOO. I haven't yet read through all of the discussion, but I wanted to plug into this part.

cool

*Maybe* this is fine for now, but I also don't like moving the gdb-remote reproducer stuff out of the gdb plugin. If you want an inspection command to access those (which sounds like a useful thing btw), then the right way to handle that would be to have some sort of an abstraction/plugin mechanism for various reproducers. For instance, the reproducers could register themselves somewhere (if they don't do that already), so we get a list of all of them, similar to how we handle "plugins" now. Then the inspection command could iterate over that list and ask each reproducer component to do its thing. This may be clunky, or it may not, depending on what exactly this "thing" is.

This looks like a really useful feature. The code seems fine, but I am wondering if we should really bail out when encountering a zero enumerator. It is not uncommon to use a special enumerator to mean "none of the above". Lldb does that occasionally (eEmulateInstructionOptionNone), and other APIs do that too (PROT_NONE, PROT_READ, PROT_WRITE, PROT_EXEC in mmap(2) for instance). I am guessing this practice is even more common for "class" enums, as those can't be implicitly constructed from integer constants.

This looks great now. Thank you for your patience. Just make sure to deal with the breakages from the other patch first. If the fix is non trivial, or if it's going to take a while, then please revert to unbreak the bots.

It broke on linux too. You did run the tests before committing, did you?

The yaml object file is pretty impressive, but I am wondering if it is really necessary. Have you looked if it would be possible to reuse the existing TestFile::fromYaml functionality? I think it should be possible to reuse that by just inserting some MachO yaml blurbs around the dwarf yaml (elf yaml2obj doesn't support embedding dwarf yaml, but that's just because nobody enabled that yet).

In D67347#1664640, @aleksandr.urakov wrote:

It's a good point, thank you! I had the same thoughts when I done it, but I'm not completely sure. The problem is that an object file can't be completely responsible for choosing an unwind plan, because some plans are produced by symbol files (and an object file knows nothing about that, if I understand right). So we can move only a part of the plan-choosing-functionality from FuncUnwinders to ObjectFiles, but will it be better? In that case both FuncUnwinders and ObjectFiles will be responsible for managing plans, won't it add an additional complexity to the solution?

looks fine to me

lgtm. The reason I requested this to be put separately, is because it is implemented in a way that kicks in even without minidebuginfo. I think this is fine, because entries can be removed from the symtab even without the whole minidebuginfo business. While this format will likely be the only real user of these partial symtabs, in theory, there isn't anything stopping someone from removing symtab entries independently of that. While unlikely, I see no harm in supporting that, if it does not incur any extra maintenance costs.

I believe error handling is important. And even more important is testing of that error handling -- as I'm trying to use llvm dwarf parser in lldb I keep finding new ways to crash it with invalid input.

I think this looks great. Some operations may get less efficient with this, but OTOH, some will get actually faster. And it is definitely the most understandable solution out of everything we had so far.

In D67230#1661120, @serge-sans-paille wrote:

@davezarzycki I tend to agree with you. We can just remove 35:44 and just keep the hostname.

Thanks Jonas. This looks fine to me, but someone who actually works on VFS (/me looks at @sammccall) will need to ack that.

• s/FoundHeuristically/RaSearch
• max_iterations:=256
• tweak the handling of "own frame size" instead of having the unwinder ask for it, it is now embedded directly into the unwind plan. This automatically makes it possible to have different frame sizes at different points in the function, and also reduces the api surface a bit. The "parameter size" is still in a separate function, because that belongs to a different frame/function/module.

Thanks for the quick reply. My responses are inline:

Thank you for adding the tests, and for adding the xz detection to lit in particular. We're getting _reaally_ close now. I have a bunch more comments, including highlighting some you seems to have missed from earlier, but they are all just about polishing. The only somewhat larger comment is about the minidebuginfo-set-and-hit-breakpoint.test test. I'd like to understand what exactly is the scenario you are intending to test there, because I have a feeling it's not testing what you want it to test...

Sounds like a good idea, particularly as gethostbyname is not thread safe. It seems getaddrinfo is already used in lldb, so it should be supported by all OSs that we care about.

Thanks. The code looks fine to me. Losing the ability to unit test is a bit of a pity, but since test was pretty icky to begin with, I can't say I'm going to miss it too much...
One way we could possibly test this is with a full scale test via the "target modules dump ast" command. So, the idea would be to run to process to some point, evaluate some expressions, run "target modules dump ast" and check the things are in the output (or that they are *not* there).

Rebase on master. Should be ready for review now.

In D67090#1659064, @MaskRay wrote:

In D67090#1657187, @manojgupta wrote:

Thanks for working on it. Have you tested if Linux perf can symbolize the perf traces using the split debug files especially with large binaries?

Meanwhile, I'll try the patch on Chrome OS to verify that gdb and perf still work.

I've learned that @manojgupta actually has a different use case: symbolization without requiring access to the stripped binary.

Thanks. The new version seems fine to me. I'll leave it for the windows folks to have the final say on this..

Though I am not very familiar VFS, this seems like the most intuitive solution out of everything that we had so far. But... shouldn't you also check that the directory you're chdir-ing into "exists" before you actually change the cwd? Also, the chdir operation should probably follow the usual semantics of a non-absolute chdir path being treated as relative to the previous cwd. And lastly :), what is the initial cwd value? Since previously the class shared the cwd with the underlying filesystem, one option might be to fetch the initial cwd from there (though I don't know if that is actually desired). Another option might be to just pick the first directory in the yaml file or something...

In D67022#1658057, @clayborg wrote:

Great idea, but this idea gave me the idea to use "m_decl_ctx_to_die" as is, and just remove the entry once we have parse all decls. Then we free the memory and don't need a bit. If there is no entry in the m_decl_ctx_to_die map, then ForEachDIEInDeclContext will just not iterate at all?

In D67090#1657449, @MaskRay wrote:

@labath to confirm whether it works on lldb if the separate debug file (created by --only-keep-debug) has no program header.

I played with some executable and it appears to work.

Any thoughts on this one?

I think we're getting pretty close. I have a couple of extra comments inline, but they're all pretty minor, I hope. I'd just like to add some more tests, per our discussion on the IRC, and I have some doubts about what exactly is the test you've written actually testing, which I'd like to clarify...

We always need more tests. I've now added more tests for various boundary conditions.

add more tests

lgtm

Thanks for the clarification Greg.

I remember seeing this inefficiency when looking at this class some time ago, but it was not clear to me what should be done about that. This is still an improvement, as it will reduce the time by 50% on average, but it is still going to be O(n). If this is really a performance bottleneck, then we will need to come up with some better data structure for storing this, or put some restrictions on the kind of entries that we can have in the map...

Looks fine to me. I believe these bots will be a very useful addition. I have one random idea you can consider if you'd like: I believe there is a way to mark (I think it involves setting flunkOnFailure=False) a particular step so that it is still run, but it does not fail the overall build. That way, you might be able to mark these bots "stable" and get breakage notifications if the build breaks, but still leave open the possibility for manual inspection of test results.

I am not very familiar with this code, but I don't see a reason why what you're doing could not work. However, it looks like the implementation of it could be done in a better way. For instance, this function seems to be the only caller of GetDIEForDeclContext. So, we could replace it with something like ast_parser->ParseAllDiesForContext(decl_ctx). This would avoid materializing the std::vector, and the ast parser could store the list of processed dies in a more intelligent (memory friendly) fashion.

Hm... "summarizing" might not have been the best word here. I have "summarized" what *I* said over IRC, but I did not mean to imply that Jan agreed with my position (and he didn't, not yet anyway..)

Summarizing our discussion on IRC:
While I think that this version of the patch is definitely better than the previous ones, I still feel that the two container solution is unnecessarily complicated. I think using llvm::MapVector or even std::vector would be sufficient. While this may be optimal in terms of performance (not memory), I don't think that is really important in this case. The only place where the std::list solution is better than a std::vector is the removal of an arbitrary element in the list. However, I would stipulate that this is a very uncommon operation. The most common operations are lookup (which are linear in any case), and insertion (constant).

Rename the function to _get_bool_config_skip_if_decorator