Added a test. PTAL.

Added a test and fixed some minor issues that I found while adding the test.

We are still waiting for discussion on where to put this cache eviction / memory management functionality to better serve existing user via different API layers. While we try to make progress on that, it'll be great if we expose the minimum interface change so that we could reduce the our memory footprint by implementing cache eviction out of LLVM source tree. So, I extracted the minimum changes and sent out https://reviews.llvm.org/D90006 for review. PTAL. Thank you.

In D78950#2086166, @dblaikie wrote:

In D78950#2086146, @netforce wrote:

FYI, I haven't abandoned this patch. I have been waiting for more opinions including dblaikie@'s.

I'll be on leave from work for the next 6 weeks. So, once I get back from it, I'll revisit the comments then and try to figure out how to move forward.

Thank you!

Yep, it's on my list of things to review! Sorry for the delay.

FYI, I haven't abandoned this patch. I have been waiting for more opinions including dblaikie@'s.

Sorry I mistakenly dropped the CMakeLists.txt files in the previous patch. Added them.

In D78950#2033767, @labath wrote:

(sorry about the delay... the usual blurb about being busy, not being qualified etc.)

Yes, that sounds like a reasonable design to me. I think it's definitely worth creating a patch to see how it looks like in practice.

In D78950#2007001, @ikudrin wrote:

Is it possible to add some tests for the LRU logic?

Reflected the discussion we've had so far. Made minimum change to the lower level API (DWARFContext, DWARFDebugLine, and DWARFUnit) to make it possible for the high level API and symbolization tool to implement caching. Added a higher level API (DWARFCachedDIContext) that implements DIContext with LRU caching so that llvm-symbolizer could benefit from the caching without code change. It is also a step toward decoupling DWARFContext from DIContext in the future which would be desirable for other use of DWARFContext (e.g. LLDB). Finally, added a unit test for the LRU logic.

Hi Pavel,

Hi Pavel,

In D78950#2022293, @labath wrote:

First off, let me say that I don't feel qualified to set the direction here, nor I am fully familiar with all of these interfaces. However, since I am already involved in here, I am going to say something anyway. :)

The way I would imagine this working ideally is that there would be two layers. The lower layer would provide more explicit access to the debug info, and it would provide it's users with the ability to manually manage the memory usage. It would be the responsibility of the users to not shoot themselves in the foot if they use it.

The second layer would offer a higher level view of the debug info and it would manage the memory (semi)automatically. Being high level it would/could/should also abstract away the difference between the different debug info formats.

The first layer would roughly correspond to the current DWARFUnit, DWARFContext, etc. classes, with one important difference DWARFContext would not implement DIContext -- it would be a standalone class. The second layer would correspond to the DIContext class, and any of the (new) helper classes it needs to manage the memory and perform the abstractions.

The main advantage I see in that is the breaking of the is-a relationship between DWARFContext and DIContext. Without it the interfaces seems rather shoot-footy because one can happily play around with the lower-level DWARFContext apis (which don't manage memory), and then accidentally call some higher level method which comes from DIContext , which does the management and will then cause the debug info to disappear from under you.

As I said, I don't know how easy would be to reach this state, nor whether it would be an acceptable state for other stakeholders...

This is a friendly ping. Could you please let me know which direction we should go? Thank you.

In D78950#2009668, @labath wrote:

(adding more debug info folks)

In D78950#2008973, @netforce wrote:

Do you think this is better in terms of keeping the API flexible enough?

Umm... probably?

I don't have a comprehensive overview of the users of the DWARF parser either, but I'm sure there are users that want to access it through the lower level APIs. Lldb will most likely be one of those users -- even after it starts using llvm parsers completely I think it's fairly unlikely it would use APIs like getLocalsForAddress, but rather do something custom. However, all of that is vapourware, so its hard to reason about that.

It may be more interesting to look at other (real) users. One such user which comes to mind is (llvm-)dsymutil, as it accesses dwarf in a fairly complex way. Jonas, what do you think?

In D78950#2007530, @labath wrote:

In D78950#2006434, @aprantl wrote:

Do you think this is always a win, i.e., also for LLDB, or should this be done in a higher layer?

Whether this is a win or not will definitely depend on the usage patterns, and it's very hard to know that for sure before trying it out on a specific use case. My gut feeling would be that this wouldn't help lldb's memory usage much because lldb already stores so much other state, but it may have a big (negative) impact on cpu usage. However, given that this functionality can be turned off (thought that could be better documented) I don't think we need to burden ourselves with that too much with what would happen with lldb in particular.

That said, I'm not really sure what to make of this patch. What was they criterion for choosing where to place the referCompileUnit calls ? Right, now they seem to be present on a couple of high-level APIs, but those functions are definitely not the only way to access dwarf units. Indeed, once you start thinking about the lower level APIs, things start to get a lot more fuzzier. And dangerous -- for instance, this makes it very easy to turn a perfectly valid DWARFDie object into a landmine if some operation happens to "garbage-collect" the DWARFDebugInfoEntry object that it refers to.

I think that example would definitely speak for a separate layer with a high level api, which offers only a limited (and more controlled) way of accessing the information in the dwarf file. How feasible is that in the current situation -- I don't know.

In D78950#2006767, @netforce wrote:

Although I'm not very familiar with LLDB, if this hurts the performance of it, it can opt out of using the caching by giving negative LRUSize when creating DWARFContext.

I think it's important to note here that lldb does not use (most of) llvm dwarf parsing code right now -- it has it's own semi-forked versions of a lot of stuff. However, we are looking into making it reuse more (all?) of llvm dwarf parsing code, which is why it is important to ensure the interface stays flexible enough.

In D78950#2007001, @ikudrin wrote:

Is it possible to add some tests for the LRU logic?

In D78950#2006434, @aprantl wrote:

Do you think this is always a win, i.e., also for LLDB, or should this be done in a higher layer?