- User Since
- Sep 23 2014, 10:11 AM (234 w, 2 d)
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
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.
Mon, Mar 18
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.
Fri, Mar 15
Thu, Mar 14
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.
Tue, Mar 12
Mon, Mar 11
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.
Adrian's suggestion was right, much simpler test case now!
Sun, Mar 10
Thu, Mar 7
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.
Wed, Mar 6
Tue, Mar 5
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?
Fri, Mar 1
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?
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
Wed, Feb 27
Tue, Feb 26
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>)
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.
I seem to remember that we currently expect that there are two cases when it comes to unknowns:
- specified unknowns
- unspecific unknowns
Wed, Feb 20
Tue, Feb 19
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
Feb 19 2019
I am definitely ready for this to go in ASAP if we are good on this!
Feb 14 2019
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.
Feb 13 2019
Just a question of if we need an optional unwind table instance instead of just an instance. I am fine either way.
Feb 12 2019
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).
Feb 11 2019
Feb 8 2019
Feb 7 2019
Feb 6 2019
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?
Feb 5 2019
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.