- User Since
- Sep 23 2014, 10:11 AM (251 w, 2 d)
We also need to test to ensure this doesn't regress parsing speed. Need a large C++ project, like LLDB with debug clang and debug llvm, and something that forces all line tables to be parsed (not just the prologues like when setting a breakpoint) and see how things compare between the two methods.
Tue, Jul 16
So in the actual line table parsing code it seems like we are creating a line table in LLVM format, complete with sequences and a bunch of LLVM data structures, and then copying the results over into LLDB and then throwing away the LLVM line table. It would be nicer to modify the LLVM line table code to be able to get a callback when a row was pushed (if it isn't already in that format) and expose that so clients, like LLDB, can just get a callback and only store the data structure we care about. This would allow us to share the llvm line table parsing code and re-use it in LLDB, but not require twice the memory and overhead. Symbolication tools could also take advantage of this by not storing any rows except the closest matching row.
Mon, Jul 15
Fri, Jul 12
I need some feedback here. Is the general consensus that we need to modify raw_ostream to support seeking or continue with the current FileWriter?
Thu, Jul 11
Probably best to have someone else review cmake changes, as I am not very familiar with all the intricacies of all the settings. I will add some people with more cmake experience to the reviewers. Jonas and Stefan, feel free to add more cmake experts here.
Wed, Jul 10
Just wondering if we want to cache the llvm::DWARFContext in the LLDB DWARFContext. See inline comments.
Ping for comments
never mind, I missed that you had!
ok, so do you want to abandon this patch?
I agree with all comments. After those few things are fixed, this looks good.
Mon, Jul 8
Do we really expect users to actually write code where variables start with '$'? We will need to make some clear rules of the lookup order if so. I would suggest the following order
- look for real variables by exact name match first
- look for expression global variables next
- look for expression results next ($<digit>)
- look for registers
I am happy to do what is needed. Anyone have any comments on my last comments? We just need a solid direction that everyone thinks we should follow.
Wed, Jul 3
The main issue I have with this is the name of the function we are adding to LanguageRuntime. See inlined comments.
Tue, Jul 2
So what is the verdict on FileWriter? Do we want real error handling like is currently coded, or just be able to check sometime later and find out that something went wrong? If we just want to check that something is wrong, we don't need to maintain a llvm::Error, we can just check the stream at any point and return a generic error that means nothing. If this is what we do, I am not sure I see the point of adding the error handling. The current solution calls hasError() which checks if the llvm::Error contains a ErrorInfoBase pointer so the call is pretty efficient. And the first thing to cause the error will stay in the error object.
Mon, Jul 1
Jim, you ok with doing the symbol context scope refactoring as a separate diff?
Don't #include <iostream>
Fri, Jun 28
A few nits, but nothing structural. Fix those and this will be good to go.
Update this patch to compile after r364634.
Thu, Jun 27
Added llvm::Error as a member variable of FileWriter. Each method that does anything with the stream will check if there is an error and return if the FileWriter has encountered an error. Added:
To allow clients to check errors after a series of method calls on FileWriter objects. The error must be checked or taken before the FileWriter object destructs.
Use reinterpret_cast instead of C casts.
Switch FileWriter::writeCStr to use llvm::StringRef. Addressed other review comments.
Wed, Jun 26
Jim: do you have any other solutions that could make this work?
Switched FileWriter::writeData() to use a llvm::ArrayRef<uint8_t> instead of pointer and size.
Address other review comments.
Fixed build bots as they showed an iterator was being used even if it was the end of a collection.
few nits, but looks good.
Tue, Jun 25
Mon, Jun 24
It would be great to get this moving. I have figured out how the rest of the patches will be broken up to make them really simple to test and get in, but need this one to go in first.
DataExtractor is a copy of the one from LLDB from a while back and changes have been made to adapt it to llvm. DataExtractor was designed so that you can have one of them (like for .debug_info or any other DWARF section) and use this same extractor from multiple threads. This is why it is currently stateless.
Thu, Jun 20
Wed, Jun 19
my fix assumes we would have zero sized symbols first in the m_file_addr_to_index map, and the symbols with the same size would be ordered after the ones with zero size. Also, I am assuming when we do:
const FileRangeToIndexMap::Entry *entry = m_file_addr_to_index.FindEntryStartsAt(file_addr);
that it finds the first symbol that starts with "file_addr". If this isn't the case we might need to be able to back up a few symbols to ensure we get consistent results
So I am fine with symbols having zero size being in the symbol table. I would be fine not changing anything in the sorting and leaving some symbols with zero size, we just need to fix:
Symbol *Symtab::FindSymbolAtFileAddress(addr_t file_addr);
Jun 18 2019
Marked things as done.
Converted AddressRange objects into a class so we can ensure consistent and correct results when using accessors.
Jun 17 2019
Marking inline comments as done and mentioning that I did add tests for FunctionInfo operator < as well for cases where things have inline info and line entries
- Fixed all header includes to abide by https://llvm.org/docs/CodingStandards.html#include-style
- Fixed inline comments
I am concerned that our mapping from DIERef to lldb::user_id_t won't work for all cases now that we are/have expanded the DIERef class (including as we add the DWO field). I voiced this concern in https://reviews.llvm.org/D63428. Let me know what you think.
I am concerned with increasing the size of DIERef objects. If we go to 12 bytes per DIERef, and we mostly store these in NameToDIE in "lldb_private::UniqueCStringMap<DIERef> m_map;" maps, which contain a std::vector of UniqueCStringMap::Entry objects which are:
Just questioning if we want to increase the size of the DIERef struct.
Jun 14 2019
Added Jason and Jim. Not sure if all libdispatch stuff is in llvm.org? Either Jim or Jason should ok this just in case they still use this in Apple specific repos.
I would rather see this stuff just moved into NativeProcessWindows (or some class in the windows lldb-server plug-in) and have all code in ProcessWindow.cpp and ProcessDebugger.cpp go away once lldb-server works on windows? The goal is to get rid of the ProcessWindows.cpp once lldb-server is running. Better to not maintain two different plug-ins (one for native and one for remote). The remote version will always work less well in that case.
Address review comments:
- make all comment block doxygen blocks
Jun 13 2019
uint32_t llvm::gsym::StringTable::find(StringRef Str) const;
to return optional:
llvm::Optional<uint32_t> llvm::gsym::StringTable::find(StringRef Str) const;
Let compiler hoist "cu_language == eLanguageTypeObjC || cu_language == eLanguageTypeObjC_plus_plus" by inlining it into if statement so it is more readable.
Jun 11 2019
I don't see much I would change here as long as this works and gets tested by the generic GDB remote protocol testing? Any others have comments?
Remove commented out code.
Changed to use ConstString in AddMangled which simplifies the logic quite a bit. I verified performance didn't regress.
Jun 10 2019
Started the split up with: https://reviews.llvm.org/D63104
Still want to resolve getting files from a DWARFUnit a bit better. See inlined comment.