Just reverse if statement as noted in inlined comments and this is good to go.

yep

Ping

In D54454#1296392, @zturner wrote:

BTW, I will have to see if it's possible to write a test for this. Even when I compiled and built a program with DWARF on Linux, the target variable Pi<double> example didn't "just work" for me, because FindGlobalVariables wasn't returning the variable. So I think this part actually needs to be fixed in the DWARF plugin, which I'm not equipped to fix. I can try something that is not a variable template, such as the Foo<int>::StaticMember example, but if that also doesn't work, then there might not be a good way to write a general purpose test for this until this is fixed in the DWARF plugin.

I can definitely add a test in the native pdb plugin though, and technically that runs everywhere (although it would only test target variable and not frame variable, maybe that's ok though?).

Anything that takes a process shared pointer should be per process. A long time ago these plug-ins didn't take process, and as time went on they did take a process:

Since we only handle . and -> this general idea seems ok to me. Do we even need a regex then? Maybe just search for first of ".-"?

nice!

Friendly ping

Added to the lit tests to ensure the function size and inline function sizes are present

In D54217#1291834, @dblaikie wrote:

In D54217#1291820, @clayborg wrote:

yeah, an easy code search indeed kicked up the tests. I will add one. Sorry about that

No worries - still curious where you were looking for the tests initially, perhaps we can do something to make them more discoverable?

Looks like I can piggy back on llvm/test/tools/llvm-dwarfdump/X86/statistics.ll

yeah, an easy code search indeed kicked up the tests. I will add one. Sorry about that

Very close, just use the layout type as mentioned in the inline comment.

So we need to do this everywhere for any string value that gets put into an object. Otherwise we are just waiting for a crash from any string value. Values of variables, summaries from variables, so many other places where we get the string value from the API in LLDB can fail. I am fine with just making a static helper function that does this for us:

In D54217#1291170, @vsk wrote:

Looks reasonable to me, but it'd help to check in a .o for a test.

So sounds like we are ok with the idea of this patch. Any comments on the patch itself?

In D54217#1290632, @vsk wrote:

In D54217#1290630, @clayborg wrote:

In D54217#1290504, @vsk wrote:

In D54217#1290474, @aprantl wrote:

Thanks. There seems to be some overlap with

[llvm-dev] RFC: Adding a code size analysis tool
https://lists.llvm.org/pipermail/llvm-dev/2018-September/126501.html

how was that discussion resolved?

I think the response was generally positive but I haven't had the time to push things into tree. @clayborg, have you tried out the tool here: https://github.com/vedantk/llvm-project ? The RFC has some usage instructions.

If you just want the ratio of inlined to not-inlined bytes, this patch is a simpler way to do it. But if you need something more fine-grained (i.e. *which* functions were inlined more compared to a baseline, by how much, etc.), it might be worth checking out. Happy to have someone else run with the code.

This current patch is indeed for an overview of inlined to not-inlined bytes. I have other patches to --statistics that will display more output coming. Breaking down exact sizes of inlined functions by count and size will be part of this future output. But this current one is just for a quick breakdown. Like you said in the URL provided, it is nice to know that say 30% of your code is inlined. If you are expecting less inlining and try a new option, this can be a quick way to see results in a very concise kind of way.

Sure, I have no objections. As you're thinking about future patches, though, I'd just suggest that you take a look at what can be salvaged/shared from my size tool prototype. There's some neat stuff there, like displaying full inlining trees. There's also an option to treat a batch of dSYMs as one unit, so you can compare all of the dsyms from one build of an OS to another, and see which symbols grew the most, were inlined the most, etc. If that's out of scope for what you need, that's fine too.

In D54217#1290504, @vsk wrote:

In D54217#1290474, @aprantl wrote:

Thanks. There seems to be some overlap with

[llvm-dev] RFC: Adding a code size analysis tool
https://lists.llvm.org/pipermail/llvm-dev/2018-September/126501.html

how was that discussion resolved?

I think the response was generally positive but I haven't had the time to push things into tree. @clayborg, have you tried out the tool here: https://github.com/vedantk/llvm-project ? The RFC has some usage instructions.

If you just want the ratio of inlined to not-inlined bytes, this patch is a simpler way to do it. But if you need something more fine-grained (i.e. *which* functions were inlined more compared to a baseline, by how much, etc.), it might be worth checking out. Happy to have someone else run with the code.

Just rename MakeVSCodeFrameID to MakeDAPFrameID, or rename all MakeDAPXXX functions you added to be MakeVSCodeXXX.

In D53599#1289691, @lanza wrote:

Am I missing some line of code or does lldb-vscode not ever map from the DAP breakpoint ID back to the LLDB BreakpointLocation and Breakpoint IDs? I wrote the equivalent functions but there doesn't seem to be anywhere that maps in the inverse direction.

In D53599#1288214, @lanza wrote:

@clayborg We can either store a dap-frame-to-lldb-frame map as well as a lldb-frame-to-dap-frame map or we can truncate the thread count down to a much smaller count of bits. I think the former is an ugly idea to just let a pair of gigantic maps grow endlessly over the lifetime of a program. E.G. for debugging some game that could be running for a long duration. For the latter, I haven't seen a thousand thread program before and if you're using a thousand threads you probably have much bigger problems than your GUI debugger. How about a split with 10 or so bits for the thread ID and the remaining 22 bits for the frame ID?

Fix all of Jonas' issues.

Fixes:

• Take care of issues found by Jonas
• Rename variables to be camel case
• Remove commented out code

In D53379#1287702, @lemo wrote:

Greg, what do you have in mind as the next step? LLDB integration?

A great way to understand and see the file format is to use to convert some DWARF:

$ llvm-gsymutil -dwarf /tmp/a.out

Then dump it:

$ llvm-gsymutil -verbose /tmp/a.out.gsym

README.md has complete file details now for anyone wanting to understand the entire file format.

So I believe this is in a good shape now. It matches the current GSYM format we have here at Facebook which is "version 1". I would love to get this in without any changes to the format so that it matches the current format we have been using for a year now. Then we can iterate and move to version 2 and make changes. Thoughts?

Thanks for making the changes!

LGTM. lldbassert to check that AST in "enum_type" is the same as "this" since we now can after switching to CompilerType as arg instead of opaque clang type pointer.

So it depends on what code was retrieving the symbol table from the object file. Can you detail where this was happening?

Just a few nits, but _very_ close.

In D53530#1284267, @aprantl wrote:

I didn't realize that the string int [] is produced by ValueObject itself; I think this makes this option more palatable to me. I'll give it a try.

So, it turns out it isn't. The only way to get the length into the typename is to hack ClangASTContext::GetTypeName to replace the training "[]" of what clang returns as the typename with a different string. The main problem with this is that this will only work for outermost types.

Something that has been requested in the past is to support C structs with trailing array members, such as

struct variable_size {
  unsigned length;
  int __attribute__((size=.length)) elements[]; // I just made up this attribute, but you get the basic idea.
};

in a similar fashion. When printing such a struct, there's no way of safely injecting the size into array type string any more.
If we dynamically created the correctly-sized array type instead, this would work just fine.

I haven't yet understood the motivation behind why overriding GetNumChildren/GetTypeName/GetChildAtIndex is preferable over creating a dynamic type in the language runtime. Is there something that I need to know?

Looks good. Thanks for making the changes.

The only other thing you would need to change to get the usability back in check when doing things in GetNumChildren() would be to have the function that gets the typename take on optional execution context for dynamic types. The ValueObject can easily pass its execution context when getting the typename. Anyone who doesn't would continue to get the "int []" as the typename which isn't really lying either way. Thoughts?

Solution is fine. As long as we don't have to duplicate the work everywhere that needs a ranges offset.

Just one question about extracting the value for DW_AT_ranges. It would be nice if we just took care of extracting the value so the form value was more useful

Updated README.md to contain full details on how line tables and inline info are encoded.

Looks fine to me. Make sure no one else has any objections.

Fixed an issue with removing auto that was causing a crasher when parsing DWARF

• Added full InlineInfo tests
• Cleaned up the API for parsing InlineInfo. Prior to this an offset reference was needed to be supplied when parsing, but the user need not worry about that so I cleaned up the API and make private decoding calls and made the API correct

• Added lookup tests to unit tests
  • test function info lookup with valid and invalid addresses
  • test that we can infer the size for zero sized symbols by taking delta between current entry and next entry

• Fixed new StringRef decoding of Breakpad files
• Added a breakpad to GSYM unit test
• Removed the mutable BinaryStreamReader ivar from GsymReader and create a BinaryStreamReader instance on the fly when we need to decode information

In D53379#1278175, @zturner wrote:

That actually makes it more thread safe too since you wouldn’t have multiple threads sharing the same offset

In D53379#1271305, @lemo wrote:

+Eric Christopher for a DWARF expert's perspective

Hi Greg, this is great stuff. I’m going to take a closer look at the implementation, in the meantime here are a few high level comments:

Important requirements

These observations describe areas which would prevent or significantly limit the use of GSYM format for crash processing. I’m not suggesting that these are blockers for checking in the initial implementation although some, if we all agree to incorporate, might be harder to retrofit later on.

• Full debug info fidelity: this is one of our key requirements for the Breakpad processor replacement. For example our developers have been asking for things like the ability to extract the arguments/locals values. It would be fine if we can use GSYM as a first level index which would enable use to pick a subset of the full debug information (having to fetch the full ELF/DWARF/PDB files would defeat the value of using GSYM)
  • CFI (must have, w/o this we won’t be able to do even basic stack walks, right?)
  • “Accelerator” indexes pointing to subsets of the real debug information
  • Encoding the full debug info (does this even make sense since we’d end up with yet another debug info format)

Most of Zachary's fixes are in this path. Few major differences:

• Updated the README.md
  • be more specific about the string table encoding
  • specify address info offsets are offsets relative to the start of the GSYM header
• Update GsymReader
  • Switched over to using BinaryStreamReader and removed DataRef class
  • Use llvm::ArrayRef instead of pointers for AddrOffsets, AddrInfoOffsets, and Files (file table)
• Switch to DenseMap and StringMap where applicable
• Removed use of std::shared_ptr for string tables and file tables
• Made GsymCreator thread safe and cleaned up multi-threaded code
• Removed segmenting code until we get all classes in the right shape. Will commit this back after first checkin once format is locked down
• Added unit tests

My issue with adding "exact_match" is it renders a few of the arguments useless:

In D53368#1276152, @aleksandr.urakov wrote:

Yes, I'll implement it tomorrow (I'm already OOO now), thanks. But is it really necessary to check the number of symbols added if we must to calculate / finalize the symtab after getting it from object file anyway? May be just always do it after creation and processing by the symbol file? For each symtab it will be done just once because of caching.

The current SymbolFile::FindTypes(...) in was designed with type base name only due to how DWARF stores it variables. It has a "const CompilerDeclContext *parent_decl_ctx" which can be specified in order to limit what we find. So we might be able to think of this as a type lookup by basename. It might be handy to add another type lookup that does lookups by fully qualified name only. and leave the current infrastructure alone? A default implementation can be added that returns false for all symbol file unless they support name lookup. Also the current implementation allows you to under specify a type. For code like:

struct Pt { char x; char y; };
namespace a {
  struct Pt { short x; short y; };
  namespace b {
    struct Pt { int x; int y; };
  }
}

We can find all of them using:

(lldb) type lookup Pt
struct Pt {
    short x;
    short y;
}
struct Pt {
    int x;
    int y;
}
struct Pt {
    char x;
    char y;
}

Very close, just down to making the SymbolVendor::GetSymtab() call symtab.CalculateSymbolSizes() and symtab.Finalize().

As long as Swig is happy and the ABI doesn't change I am ok with this. Will we see the variables better when debugging? Or is this solely so the SymbolContextItem type doesn't disappear from the debug info?

Zach: yes the module mutex needs to be recursive. Plenty of places where the symbol file stuff can call other symbol file APIs. To avoid A/B locking issues, the lldb_private::Module, lldb_private::ObjectFile and lldb_private::SymbolVendor and lldb_private::SymbolFile all use the module mutex.

Very close. Just a bit of cleanup now that we changed SymbolFilePDB where we don't seem to need m_public_symbols anymore. See inlined comments and let me know what you think

This won't work when we have many frames. If you get a stack trace that blows the stack, you can easily have 300K stack frames. Thread index ID can get quite high too. There are also no lifetimes on the frame IDs we use, so we never get told to free a frame ID when the IDE is done with one. So we either have to switch to using a map that can map between frame ID and lldb::SBFrame or let the map just grow and grow, or something else. This isn't sufficient.

Kotlin reduces down to Java no? Or is there a case where it compiles directly to native code? If so there are two valid use cases for Kotlin: one as Java and one as Native. We would need to distinguish between the two

Parsing a type shouldn't need an execution context and we shouldn't be re-parsing a type over and over for each frame. We should be encoding the array expression somewhere that we can access it when we need to get the number of children using the current execution context.

Shouldn't the JavaASTContext be used here instead of the ClangASTContext?

LGTM

Just fix the assert to use lldbassert so we don't crash as mentioned in the inline comment and this is good to go.

All symbol tables are currently extracted from the object files via ObjectFile::GetSymtab(). Are symbols only in the PDB file? If so I would vote to add a "virtual void SymbolVendor::AddSymbols(Symtab *symtab)" and a "virtual void SymbolFile::AddSymbols(Symtab *symtab)" where we take the symbol table that comes from the object file and we can add symbols to it if the symbol file has symbols it wants to add to the object file's symbol table. All symbol queries go through the lldb_private::Symtab class anyway. Care must be taken to watch out for symbols that might already exist from an ObjectFile's symbol table to ensure don't have duplicates.

See inlined comments

Since this is duplicated in so many spots, can we make a helper function to do this to ensure no one gets it wrong. We might be able to pass in the "guard" and "stop_locker" as reference variables and modify them in the helper function.

I rushed gettting this ready for LLVM and definitely didn't convert all variables names correctly. I will fix these. Thanks for the suggestions on the LLVM classes that will do stuff better. I know I need to replace the StringTable class with something in LLVM, the DataRef class with something in LLVM and more. I will fix all these issues an update soon

Looks good. Nice cleanup.

See inlined comment.

m_has_children was used to represent what was in the DWARF in the byte that follows the DW_TAG. m_empty_children was used for DIEs that said they had children but actually only contain a single NULL tag. It is fine to not differentiate between the two.

LGTM

It would be great if we can detect this when all of the test files are loaded and emit an error instead of waiting for results to be overwritten

See inlined comments.

Thanks for making the changes. Looks good!

Down to just modifying the DWARFFormValue constructor to be able to take a CU only. Looks good.

A few things inlined. Very close. DumpAttribute will need to take a DWARFFormValue in order to dump the value correctly.

See inline comments.

So any string in the debugger can contain invalid UTF8. This can easily happen for C string summaries or char array summaries from lldb::SBValue objects when a variable isn't yet initialized. It would rather us sanitize any strings that can contain invalid UTF8 and just change any invalid UTF8 sequences into either "\\xXX" where XX is the byte. Notice the extra backslash then the string would show up as \xXX when it gets displayed. Or we can use \uXXXX (notice just a single backslash) which would send the character over as is, just not as invalid UTF8. Best option would be to convert valid UTF8 sequences over to use \uXXXX format (single backslash to correctly encode the character as unicode), and invalid UTF8 sequences to send each invalid byte as \\xXX (double backslash to just display the non printable character correctly. One vote for using the first \\xXX format is it will display correctly in a variable view. If we encode invalid UTF8 sequences over as the actual characters themselves, then the GUI might show something unexpected, or have issues displaying the invalid UTF8.

For space savings it seems like we would want to support the DW_RLE_base_address and DW_RLE_offset_pair pretty soon.

Just add a switch statement when handling the encodings and a lldbassert as mentioned in inlined comments and this will be good to go.