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.

nice!

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.

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?

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.

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.

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.

The main issue I have with this is the name of the function we are adding to LanguageRuntime. See inlined comments.

In D63828#1567764, @MaskRay wrote:

In D63828#1567323, @clayborg wrote:

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. Without the error checking, FileWriter seems to just forward IO operations to the underlying std::ostream. I wonder if it would be easier to review the other components of GSYM by inlining those FileWriter use sites.

Others may have different opinions but my feeling is that FileWriter does lots of duplicate work that has been done in raw_ostream. It tries to catch errors but I don't see how errors are handled. It may become clearer when other components of GSYM are checked in. Before that, without concrete use cases, it may be difficult to suggest what FileWriter should do.

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.

Jim, you ok with doing the symbol context scope refactoring as a separate diff?

Don't #include <iostream>

In D63828#1563337, @MaskRay wrote:

It currently uses std::ostream over llvm::raw_ostream because llvm::raw_ostream doesn't support seeking which is required when encoding and decoding GSYM data.

There is raw_fd_ostream::seek. You just need to make sure raw_fd_ostream::supportsSeeking() returns true when you open it.

A few nits, but nothing structural. Fix those and this will be good to go.

Update this patch to compile after r364634.

In D63240#1562527, @jingham wrote:

This looks good to me. I wonder if the SymbolContextScope -> Language calculation that you do in IsRuntimeSupportValue should really be done in SymbolContextScope? If that's going to be the policy for going from SymbolContextScope, maybe centralize it there?

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:

llvm::Error FileWriter::takeError();

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.

In D63828#1561085, @aprantl wrote:

JF is currently working on making BitStream error-safe over in https://reviews.llvm.org/D63518. Perhaps there are some patterns that are worth copying.

Switch FileWriter::writeCStr to use llvm::StringRef. Addressed other review comments.

In D63828#1559914, @jakehehrlich wrote:

How invasive would it be to change this to not use an ostream but to instead use an in memory buffer?

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.

lgtm

few nits, but looks good.

In D63540#1556989, @labath wrote:

In D63540#1550858, @clayborg wrote:

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);

To ignore zero sized symbols when it finds them _if_ there is another symbol that has a size for that address. Wouldn't that fix the issue here?

You are assuming here that the symbols have size zero at the time we are performing the lookup. If I understand correctly what is going on, the problem here is that the code munging the symbol table (InitAddressIndexes), will set these symbols to have non-zero size. This is what this patch is trying to avoid.

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.

friendly ping

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);

Marked things as done.

Converted AddressRange objects into a class so we can ensure consistent and correct results when using accessors.

In D63491#1548400, @labath wrote:

Ok, sounds good. In that case, I think I can squeeze that change into the previous patch in this series.

In D63491#1548238, @labath wrote:

I made this depend on the DIERef patch instead of the other way around because the cleaner separation between dwo identifiers and compile unit offsets implemented in that patch makes it easier to implement this. It would still be possible to implement it the other way around, but the logic would be more complex, and it would have to be redone anyway once the DIERef patch lands.

Another possibility would be to just remove the unit_offset field from the DIERef class, and have the manual index keep using DIERefs. Once we introduce the dwo identifier, the unit offset is strictly optional, as the unit is uniquely identified by the other coordinates.

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.

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

In D63165#1540606, @Hui wrote:

In D63165#1539118, @amccarth wrote:

Sorry for the stupid question, but ...

What exactly is meant here by "Native"? How is a NativeProcessWindows different from ProcessWindows?

The Native*** classes are meant to be used from lldb-server. They look somewhat similar to their non-native counterpart because they still do debugging, but they're a lot dumber, because they only deal with basic process control, and none of the fancy symbolic stuff that you'd need debug info for.

They differ in APIs but most of them have common implementations. The APIs from native process classes are more easy to apply process/thread control.
Hope the native and non-native ones can be merged. The similar thing to the RegisterContext and NativeRegisterContext classes.

The other thing is that using "native" classes can avoid linking a lot of unnecessary lldb libs (LLDB plugins or whatever comes with the plugins) to lldb-server.
The nativeprocesswindows could just be a pass-through to processwindows plugin, but the usage is a sort of strange since the
lldb-server needs to initialize the plugin, create a target, and create a instance just like what lldb does. This means literally
there will be two lldb debuggers, one on host and the other one on remote. It is doable, but not that applicable.

Change:

uint32_t llvm::gsym::StringTable::find(StringRef Str) const;

to return optional:

llvm::Optional<uint32_t> llvm::gsym::StringTable::find(StringRef Str) const;

as suggested.

Let compiler hoist "cu_language == eLanguageTypeObjC || cu_language == eLanguageTypeObjC_plus_plus" by inlining it into if statement so it is more readable.

In D63171#1539049, @aprantl wrote:

I suppose one could compile Objective-C code on Linux using GCC.

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.

In D62756#1526996, @labath wrote:

Being consistent definitely sounds like a good idea. Since this does change behavior somewhat, I'm wondering whether it would make sense to add a test here. The thing that's not clear to me is whether this change in behavior is only theoretical, or if it can also happen in a real-world scenario. We can always hand-construct an DIE which will have a linkage name, but no "normal" name, but I don't see how would this ever happen in practice. How did you initially discover this inconsistency?

nice

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.

In D58678#1530031, @lanza wrote:

@clayborg Seems like this still steps into the call if the call is the last instruction in the range. ThreadPlanStepRange::SetNextBranchBreakpoint checks if (last_index - pc_index > 1) before setting the breakpoint. So if last_index == pc_index and pc points to call then the thread plan will resort to single stepping and thus go through all the same machinery. Obviously, this isn't a problem as this just leads to using the same functionality that it used prior to this patch, but you miss out on the optimization you're aiming for.