Use reference to DWARFContext instead of pointers? Apply to entire patch and good to go

The suggestion just avoids the Clear() call at the expense of a branch. No big deal. Fine either way.

LGTM as long as the test suite is happy

LGTM

We can get rid of the m_dwarf_data all together. As Pavel suggested, we used to not mmap the entire file, and now we do most of the time. The Section::GetSectionData() will ref count the mmap data in the data extractor and use the mmap already, so we should only support grabbing the contents via the Section::GetSectionData() and don't propagate this legacy code in the new DWARFContext.

A unit test would be nice so we catch future issues.

I haven't used it in a long time. I can add it back temporarily if I ever need to.

Most of these logs seem useful. See inline comments.

In D59433#1431488, @zturner wrote:

"MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name?

Must fix logic error as mentioned in inlined comments.

As long as there is not a large performance regress when parsing large DWARF files this looks good to me. Abbreviation declarations are small, so I don't expect one.

In D59235#1425443, @zturner wrote:

In D59235#1425436, @clayborg wrote:

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

If no one is depending on type_sp, then ok to delete. Should be easy to test by removing it and seeing what breaks (if anything).

What part of parsing DWARF64 wasn't working? I think the SPARC compiler was the only one producing DWARF64. Be a shame to lose the support. What is the plan here after this patch?

Since unwinding is Jason's thing, he should give the ok for this patch.

In D58347#1424745, @labath wrote:

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

Adrian's suggestion was right, much simpler test case now!

As long as we are still seeing all argument values, I check the first 10 or so calls and it looked like this was the case, then I am good with this.

LGTM!

Fix zturner's suggestion.

Strong ownership is needed for this class IMHO because it hands out pointers to native data

Do we want this in LLVM instead of lldb?

lgtm

In D58792#1415629, @zturner wrote:

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

If this is new API I guess it is ok to have them all be const. I was mostly worried that you wouldn't be able to call a non const function from a const function. Do we have any IsValid() calls that are const? I believe we do. If so, how are we calling IsValid() if it is const from a non const conversion operator?

In D58792#1414964, @labath wrote:

In D58792#1414191, @shafik wrote:

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
2. give up on constness and just have a non-const operator bool everywhere
3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that

I think we mostly wanted to catch these in the test suite. Probably best to convert to lldbassert

So it affects Apple directly!

LLDB_CONFIGURATION_BUILD_AND_INTEGRATION doesn't have them. So you can change these to

Sounds good.

Actually on re-reading the patch, we are detecting if anything wasn't set and setting it. Makes sense.

Still issues from what I see. We need to know if something was specified or not and with your change we don't know the difference between the "none" state (which is current enum = unknown and string = "unknown") and the just not specified case (which is current enum = unknown and string = <empty>)

In D58653#1411285, @aprantl wrote:

@clayborg I recently ran into a similar issue and I think that perhaps adding explicit llvm::Triple::Any{Vendor|OS|...} enumerators to llvm::Triple to make this distinction explicit would be the cleanest solution.

In D58678#1410970, @jingham wrote:

I have two questions about this patch.

1. I want some llvm expert to weigh in on whether
   
   m_instructions[i]->IsCall()
   
   always means it returns to the next instruction after the call. That seems obvious, but since this patch depends on that being true, I'd like to know that it is guaranteed.

I would be all for adding a "none" as a valid vendor, os or environment to the llvm triple class. Then all of the unspecified unknown stuff goes away. But I am not sure how likely the LLVM community will like this change since it won't be useful for non LLDB code.

In D58653#1410907, @xiaobai wrote:

In D58653#1410593, @clayborg wrote:

A "specified unknown" is when the user actually typed "unknown" in their triple. IIRC there is no "none" to specify that there is no OS, so we use "specified unknowns" for this case. In this case we expect the enum _and_ the string accessor value for vendor and/or OS to be the string "unknown".

This will work when the user types "armv7" as their triple, but when they type "armv7--linux" the vendor gets implicitly set to "unknown" even though the user didn't specify it.

In D58653#1410184, @labath wrote:

I think this basically defeats the purpose of the IsUnspecifiedUnknown functions, which was to differentiate between foo-unknown-bar and foo--bar triples. That in itself was a pretty big hack, but it seems that there is functionality which depends on this. There was a discussion about this back in december http://lists.llvm.org/pipermail/lldb-dev/2018-December/014437.html, and IIRC it converged to some sort of a conclusion, but I don't think there hasn't been any effort to try to implement it. I'm not sure what exactly it means about the future of this patch, but I'd be cautious about it.

Part of what you said is inconsistent with my understanding. Specifically, if you specify a Triple with the string "foo-unknown-bar" and another Triple with the string "foo--bar" you end up with the same triple. We rely on the llvm::Triple::normalize function to create triples from strings, and there is no way to indicate using this method that a triple has unspecified non-optional parts. We are relying on ArchSpec being created with a Triple object and not a StringRef for this behavior to work.

In D53379#1376627, @echristo wrote:

Hi Greg,

I've had a lot of time to review this (thanks for that) and do apologize for taking so long. I have a couple of concerns about this so bear with me and let's see where we can get:

a) This looks like it's a standalone tool that's being added to llvm, but that really doesn't involve anything coming out of llvm right now?

I seem to remember that we currently expect that there are two cases when it comes to unknowns:

• specified unknowns
• unspecific unknowns

Looks good.

So I question the need for the SectionPart class. Seems like it would be easier to just add extra args to the ReadSectionData calls? Comments?

BTW: I did add a - character after the CHECK before I checked it in

I am definitely ready for this to go in ASAP if we are good on this!

This change was probably due to things changing over time and there may be more of these kinds of cleanups to be had. nice catch.

Just a question of if we need an optional unwind table instance instead of just an instance. I am fine either way.

Yeah localhost can cause problems if someone has a custom mapping in their network config file.

You changed the API to return a pointer which can be NULL so we either need to check all returns for this or change the API to return a reference to a default constructed UnwindTable. I like the latter option, but I will leave that up to you. Since we are moving functionality into symbol files, we might want to opt for the latter to allow us to populate the UnwindTable as needed from any source when ever that source becomes available (like adding a symbol file to a module after the fact).

In D57751#1390345, @labath wrote:

Hi Greg, what do you think of my replies to your CreateModuleFromObjectFile comment?

Just bounds check "index" in parse compile unit and this is good to go

Do we have any callers that call GetNumTemplateArguments with false? If not, remove the argument?

Thanks for the answers. LGTM

Check the comment about m_platform_file

I like the way you did the compile units and the line tables and support file list. It would be nice to change this to do things more lazily. Right now we are parsing all compile unit data into CompUnitData structures and then passing their info along to the lldb_private::CompileUnit when we need to. We can be more lazy, see inlined comments.