In D122975#3500176, @labath wrote:

or rethink the do-tasks-while-you-wait idea.

I'm afraid I had to revert this, as it was causing hangs in TestMultipleDebuggers.py.

Sure, that's great.

@clayborg, @shafik, what do you make of this version?

Heh, well.. there definitely are, though a lot of things that come to mind right now are not really suitable for a first-time contributor. Is there any specific are that you would like to know (or that you know already)?

Looks good. Thanks for the patch. Do you need me to commit this for you?

Yeah, I guess that what I am saying.

Awesome. Thanks for your patience.

Just my opinion, but I wouldn't like to complicate the logging architecture with file-only log statements and cross-channel dependencies. If you want to cross-reference some other logging channel with the executed commands then I'd say you should enable both channels. We can definitely talk about making the command feature more prominently in the log stream.

There were probably easier ways to test this, but it's nice to have the core file nonetheless. LGTM, assuming the core is of a reasonable size.

In D122974#3482540, @llunak wrote:

In D122974#3481852, @labath wrote:

In D122974#3481269, @llunak wrote:

But what if I don't want StringMap to compute the hash at all? There's still that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] that (and LLDB's index cache could be modified to store it). Which means it can be useful for StringMap to allow accepting precomputed hash, and then what purpose will that HashedStringRef have?

I think that David's point was that you would use a (probably static) StringMap method to produce the hash object, and then you use the hash object to select the correct map, and also to insert the key in the map. Something like:

...

That should only produce one hash computation, right?

Yes, it would, but that's not my point. If StringMap insists on computing the hash itself and doesn't allow outside code to provide it, then that prevents the possible LLDB optimization saving the remaing 10-15% startup CPU cost by not computing the hash at all. If StringMap allows that, then this HashedStringRef idea can be easily circumvented by passing the hash directly, so it seems rather pointless. And the relevant LLDB code from this change is so small that creating HashedStringRef just for that seems like an overkill.

In D122974#3481269, @llunak wrote:

In D122974#3480686, @dblaikie wrote:

In D122974#3424654, @JDevlieghere wrote:

...

struct HashedStringRef {
  const llvm::StringRef str;
  const unsigned full_hash;
  const uint8_t hash; 
  HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), hash(hash(str)) {}
}

...

The external code shouldn't be able to create their own (ctor private/protected, etc) - the only way to get one is to call StringMap to produce one.

But what if I don't want StringMap to compute the hash at all? There's still that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] that (and LLDB's index cache could be modified to store it). Which means it can be useful for StringMap to allow accepting precomputed hash, and then what purpose will that HashedStringRef have?

In D124606#3480164, @aaronpuchert wrote:

Fair enough, but don't we want to enforce LF or CRLF, respectively?

Sure, but is the version control system the right tool to do that? I think it'd be better to have the test itself confirm the consistency of the data (or convert it into the right format at runtime) and not rely on magical conversions within the VC tool.

I believe (based on some similar issues I was solving in lldb) that -text instead of binary is sufficient to prevent git from fidlling with the contents, while maintaining the ability to diff the files.

In D124198#3468039, @clayborg wrote:

So there is real benefit to doing these checks IMHO as it can help us get back on track in live debug sessions. Core file sessions results will vary depending on it we actually have the original object files or not, many times we do not have this info, in which case we would just enable RA search and we might be able to unwind the PC, but not other registers if we can't actually find the prologue or if we don't have stack frame details from breakpad

Here's the updated version. I'll place my new findings into the patch
description.

One could add a unit test for the CFD overload, though, as this appears to be the only callsite (or other callsites were equally untested and broken), we could also remove it altogether.

Yeah, this was very surprising.

For future reference, it's better to generate a full diff when uploading a patch manually. Here are some ways to do that.

FWIW, I don't think that this feature, as it is implemented right now, has any chance of working on windows. Windows does not have the equivalent of .symtab (only .dynsym), so we cannot use that as a poor man's debug info substitute -- the debug info is either there -- or it isn't.

In D124370#3472394, @clayborg wrote:

So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.

in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.

So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

Woohoo

Thanks, for your patience. Overall, I'd say this looks pretty clean now -- much cleaner than I originally thought it could be. I don't have any further comments here.

Yes, that's pretty much what I had in mind. Thanks.

The patch description could definitely use more details about the motivation for the change, and a description of how it works. Apart from the fact that it uses the raSearch feature I introduced a while back, I don't know much about how it works (and given the planned changes, I am not going to try to understand that), but I think I can provide some details/thoughts about the motivation.

In D124113#3464287, @philnik wrote:

LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

In D124110#3463380, @clayborg wrote:

I might suggest we rename things like suggested in my inline comment, and then have the "SymbolFile.h" class just include the extra needed header file? Many of the changes in this diff would just go away.

remove the stray binary

• address review comments

Yeah, I suppose we can do that. If this turns out to be more widespread, then I'd rather disable the tidy check (or perhaps the clang warning) for the unittest files instead.

seems reasonable

Yes, I think that's exactly what I had in mind. Basically, the idea is to structure things such that the ondemand class can sit between the actual symbol file class and the outside world, but that it does not (and cannot) interfere with any of the interactions that happen inside a symbol file class. If it works, I think that kind of a setup would be much cleaner/understandable. As it stands now, one has to consider the possibility that any action inside the real symbol file can reenter the ondemand instance, which makes it harder to reason about.

Thanks.

This is great. Thanks for doing that.

In D123020#3442738, @llunak wrote:

Based on that 'introduce' comment I expect the part that you're not aware of is that all 4 of those simple commits I pushed directly changed code that had been written by me. So I still think I was following the guidelines, and I got an explicit review for all changes where I had any uncertainty, but as I said if it's expected that I'll get explicit review even for simple changes in code I'm familiar with, I can do that.

I am trying to get rid of the friendship declaration and the forwarding of protected methods. I have no issue with making GetCompileUnitAtIndex virtual -- that is pretty much a necessity for this feature (*). However, SetCompileUnitAtIndex seems like an internal implementation detail of a SymbolFile instance (that's why it's protected), and it would be better if the new class didn't have to touch it. I wouldn't call the new class the "real" symbol file. I don't see it as any more real as any other subclass. The way I'd look at it is that the new class just contains some helpful implementation that can be useful to a specific symbol file class. An individual symbol file class may choose to use it, but it could also implement the pure SymbolFile interface is a completely different way, if it knows how.

I'm looking at the SymbolFileOnDemand friendship and the forwarding of protected methods (mostly dealing with compile unit lists). Does this by any chance have something to do with the fact that there are now two compile unit lists (one in the actual symbol file, and one in the wrapping ondemand class? Would it be possible to avoid that by making SymbolFile a stateless interface (just methods, no member variables), and having a putting the member variables and other utility functions into a separate class that the real symbol files would inherit from? Then only SymbolFileOnDemand would inherit from SymbolFile directly, and it would only add the state that it needs (the bool flag, and the actual symbol file pointer).

Cool. Thanks.

In D123020#3437246, @llunak wrote:

In D123020#3426867, @JDevlieghere wrote:

FWIW the official policy is outlined here: https://llvm.org/docs/CodeReview.html

I'm aware of it, but as far as I can judge I was following it. Even reading it now again I see nothing that I would understand as mandating review for everything.

It does say "patches that meet likely-community-consensus requirements can be committed prior to an explicit review" and "where there is any uncertainty, a patch should be reviewed prior to being committed".
It can be hard to judge what is a likely-community-consensus without being an active member of the community, which is why it's safer to go down the pre-commit review path.

I think this is actually a wider problem. I also ran into this recently (with triples), but didn't have time to create a patch yet. I don't think that None as the "actual" value should ever be considered a match (except when the pattern is None, I guess), regardless of whether it is a debug info variant, triple, or anything else. (None as a "pattern" should of course continue to match anything.)

The reason for the funny /*override*/ thingy in the mock classes is that it is impossible (in the gmock version that we use) to add the override keyword to the methods overridden by the MOCK macros. Then, having override keywords on hand-written methods triggered -Winconsistent-missing-override.

Yes, that's the way we usually deal with this.

The gcc behavior is quite annoying here...

This is great. I've been bothered by this for quite a while, but I didn't realize the fix is that easy.

Looks good. Thanks for your patience.

Note that this patch could have been shorter if I simply changed LoadModuleAtAddress() to always pass false for notify (all callers currently call ModulesDidLoad() afterwards) and added a note to LoadModuleAtAddress() docs about it, but I don't know if that's a good design choice.

Reopening, since the last attempt was quite some time ago, and the rebase was
non-trivial.

In D122975#3426731, @llunak wrote:

In D122975#3426575, @labath wrote:

I'd like to understand what is the precise thing that you're trying to optimise. If there is like a single hot piece of code that we want to optimise, then we might be able to come up with an different approach (a'la PrefetchModuleSpecs) to achieve the same thing.

The scenario is a repeated lldb start with index cache enabled (and so everything necessary cached), the debugged app is debug build of LibreOffice Calc (~100 medium-sized C++ libraries). 90% of the CPU time is spent in LoadFromCache() calls, because 70% of the CPU time is spent in ConstString (and that's after my ConstString optimizations). I don't think there's any other way to make it faster other than parallelizing it, although parallelizing only LoadFromCache() should be enough and I'd expect that to be limited enough to be safe if you expect the lldb codebase is not generally thread-safe. In this patch I parallelized higher because it was simple to do it up there, and I don't know how to easily get at all the LoadFromCache() calls (especially the one called from SymbolFileDWARFDebugMap is relatively deep). If you know how to parallelize only that, that works for me too.

cool. thanks.