clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (216 w, 3 d)

Recent Activity

Wed, Nov 14

clayborg accepted D53008: Add a check whether or not a str is utf8 prior to emplacing.

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

Wed, Nov 14, 9:52 PM

Tue, Nov 13

clayborg added a comment to D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it..

yep

Tue, Nov 13, 2:50 PM · Restricted Project
clayborg added a comment to D53379: GSYM symbolication format.

Ping

Tue, Nov 13, 11:11 AM
clayborg accepted D54460: Don't keep a global ABI plugin per architecture.
Tue, Nov 13, 7:23 AM

Mon, Nov 12

clayborg added a comment to D54454: Be more permissive in what we consider a variable name..

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?).

Mon, Nov 12, 6:34 PM
clayborg added a comment to D54460: Don't keep a global ABI plugin per architecture.

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:

Mon, Nov 12, 6:29 PM
clayborg added a comment to D54454: Be more permissive in what we consider a variable name..

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 ".-"?

Mon, Nov 12, 4:38 PM
clayborg accepted D54417: Fix a crash when parsing incorrect DWARF.
Mon, Nov 12, 10:15 AM
clayborg accepted D51578: Contiguous .debug_info+.debug_types for D32167.
Mon, Nov 12, 9:56 AM · Restricted Project

Fri, Nov 9

clayborg added a comment to D54333: [CMake] Allow version overrides with -DLLDB_VERSION_MAJOR/MINOR/PATCH/SUFFIX.

nice!

Fri, Nov 9, 1:21 PM
clayborg added a comment to D53379: GSYM symbolication format.

Friendly ping

Fri, Nov 9, 10:09 AM

Thu, Nov 8

clayborg updated the diff for D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

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

Thu, Nov 8, 2:46 PM
clayborg accepted D54241: Fix bug in printing ValueObjects and in PE/COFF Plugin.
Thu, Nov 8, 10:42 AM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

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?

Thu, Nov 8, 10:27 AM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

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

Thu, Nov 8, 10:11 AM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

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

Thu, Nov 8, 9:58 AM
clayborg requested changes to D54241: Fix bug in printing ValueObjects and in PE/COFF Plugin.

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

Thu, Nov 8, 9:50 AM
clayborg requested changes to D53008: Add a check whether or not a str is utf8 prior to emplacing.

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:

Thu, Nov 8, 9:43 AM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
In D54217#1291170, @vsk wrote:

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

Thu, Nov 8, 9:30 AM

Wed, Nov 7

clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

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

Wed, Nov 7, 2:04 PM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
In D54217#1290632, @vsk wrote:
In D54217#1290504, @vsk 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.

Wed, Nov 7, 1:33 PM
clayborg added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
In D54217#1290504, @vsk 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.

Wed, Nov 7, 1:24 PM
clayborg updated the summary of D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
Wed, Nov 7, 11:29 AM
clayborg created D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
Wed, Nov 7, 11:29 AM
clayborg accepted D53599: Adjust some id bit shifts to fit inside 32 bit integers.
Wed, Nov 7, 10:57 AM
clayborg requested changes to D53599: Adjust some id bit shifts to fit inside 32 bit integers.

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

Wed, Nov 7, 7:24 AM
clayborg added a comment to D53599: Adjust some id bit shifts to fit inside 32 bit integers.

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.

Wed, Nov 7, 7:23 AM

Tue, Nov 6

clayborg added inline comments to D53599: Adjust some id bit shifts to fit inside 32 bit integers.
Tue, Nov 6, 2:17 PM
clayborg added a comment to D53599: Adjust some id bit shifts to fit inside 32 bit integers.

@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?

Tue, Nov 6, 10:29 AM
clayborg updated the diff for D53379: GSYM symbolication format.

Fix all of Jonas' issues.

Tue, Nov 6, 10:04 AM

Mon, Nov 5

clayborg updated the diff for D53379: GSYM symbolication format.

Fixes:

  • Take care of issues found by Jonas
  • Rename variables to be camel case
  • Remove commented out code
Mon, Nov 5, 1:56 PM
clayborg added a comment to D53379: GSYM symbolication format.

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

Mon, Nov 5, 1:16 PM
clayborg added a comment to D53379: GSYM symbolication format.

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
Mon, Nov 5, 9:45 AM
clayborg added a comment to D53379: GSYM symbolication format.

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

Mon, Nov 5, 9:33 AM
clayborg added a comment to D53379: GSYM symbolication 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?

Mon, Nov 5, 9:27 AM
clayborg accepted D53530: Fix (and improve) the support for C99 variable length array types.

Thanks for making the changes!

Mon, Nov 5, 7:37 AM · Restricted Project
clayborg added a comment to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

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.

Mon, Nov 5, 7:31 AM
clayborg added a comment to D53368: [Symbol] Search symbols with name and type in a symbol file.

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

Mon, Nov 5, 7:21 AM · Restricted Project
clayborg added a comment to D51578: Contiguous .debug_info+.debug_types for D32167.

Just a few nits, but _very_ close.

Mon, Nov 5, 7:11 AM · Restricted Project

Thu, Nov 1

clayborg added a comment to D53530: Fix (and improve) the support for C99 variable length array types.

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?

Thu, Nov 1, 9:39 PM · Restricted Project
clayborg accepted D53368: [Symbol] Search symbols with name and type in a symbol file.

Looks good. Thanks for making the changes.

Thu, Nov 1, 10:24 AM · Restricted Project
clayborg added a comment to D53530: Fix (and improve) the support for C99 variable length array types.

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?

Thu, Nov 1, 10:19 AM · Restricted Project
clayborg accepted D53929: [LLDB] - Add support for DW_FORM_rnglistx and relative DW_RLE_* entries..

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

Thu, Nov 1, 10:07 AM

Wed, Oct 31

clayborg added a comment to D53929: [LLDB] - Add support for DW_FORM_rnglistx and relative DW_RLE_* entries..

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

Wed, Oct 31, 12:57 PM

Tue, Oct 30

clayborg updated the diff for D53379: GSYM symbolication format.

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

Tue, Oct 30, 4:29 PM
clayborg added a comment to D53506: [ClangASTContext] Extract VTable pointers from C++ objects.

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

Tue, Oct 30, 10:17 AM · Restricted Project
clayborg accepted D53813: [LLDB] - Add support for DW_FORM_addrx[1-4]? forms..
Tue, Oct 30, 10:09 AM

Mon, Oct 29

clayborg added inline comments to D53379: GSYM symbolication format.
Mon, Oct 29, 1:54 PM

Sun, Oct 28

clayborg updated the diff for D53379: GSYM symbolication format.

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

Sun, Oct 28, 3:34 PM
clayborg updated the diff for D53379: GSYM symbolication format.
  • 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
Sun, Oct 28, 9:21 AM

Sat, Oct 27

clayborg updated the diff for D53379: GSYM symbolication format.
  • 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
Sat, Oct 27, 9:47 AM
clayborg updated the diff for D53379: GSYM symbolication format.
  • 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
Sat, Oct 27, 9:00 AM
clayborg added a comment to D53379: GSYM symbolication format.

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

Sat, Oct 27, 7:30 AM
clayborg added inline comments to D53379: GSYM symbolication format.
Sat, Oct 27, 6:59 AM

Fri, Oct 26

clayborg added inline comments to D53379: GSYM symbolication format.
Fri, Oct 26, 4:30 PM
clayborg added inline comments to D53379: GSYM symbolication format.
Fri, Oct 26, 3:12 PM
clayborg added a comment to D53379: GSYM symbolication format.

+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)
Fri, Oct 26, 2:59 PM
clayborg updated the diff for D53379: GSYM symbolication 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
Fri, Oct 26, 2:34 PM
clayborg added inline comments to D53379: GSYM symbolication format.
Fri, Oct 26, 2:28 PM
clayborg added inline comments to D53379: GSYM symbolication format.
Fri, Oct 26, 9:58 AM
clayborg added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

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

Fri, Oct 26, 9:25 AM

Thu, Oct 25

clayborg added a comment to D53368: [Symbol] Search symbols with name and type in a symbol file.

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.

Thu, Oct 25, 11:15 AM · Restricted Project
clayborg added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

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;
}
Thu, Oct 25, 11:10 AM
clayborg requested changes to D53368: [Symbol] Search symbols with name and type in a symbol file.

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

Thu, Oct 25, 10:42 AM · Restricted Project
clayborg added a comment to D53597: Don't type-erase the SymbolContextItem enum.

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?

Thu, Oct 25, 10:34 AM

Wed, Oct 24

clayborg added a comment to D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules().

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.

Wed, Oct 24, 10:50 AM
clayborg accepted D53646: [LLDB] - Parse the DW_LLE_startx_length correctly for DWARF v5 case..
Wed, Oct 24, 10:45 AM
clayborg requested changes to D53368: [Symbol] Search symbols with name and type in a symbol file.

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

Wed, Oct 24, 10:34 AM · Restricted Project

Tue, Oct 23

clayborg requested changes to D53599: Adjust some id bit shifts to fit inside 32 bit integers.

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.

Tue, Oct 23, 3:39 PM
clayborg accepted D53140: [LLDB] - Add support for DW_RLE_base_address and DW_RLE_offset_pair entries (.debug_rnglists).
Tue, Oct 23, 9:31 AM
clayborg added a comment to D53367: [lldb] Kotlin Native naive support..

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

Tue, Oct 23, 9:28 AM
clayborg requested changes to D53530: Fix (and improve) the support for C99 variable length array types.

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.

Tue, Oct 23, 9:21 AM · Restricted Project

Mon, Oct 22

clayborg requested changes to D53367: [lldb] Kotlin Native naive support..

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

Mon, Oct 22, 12:49 PM
clayborg accepted D53436: [LLDB] - Implement the support for the .debug_loclists section..
Mon, Oct 22, 12:49 PM
clayborg accepted D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it..

LGTM

Mon, Oct 22, 12:44 PM · Restricted Project
clayborg added a comment to D53436: [LLDB] - Implement the support for the .debug_loclists section..

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

Mon, Oct 22, 10:54 AM
clayborg requested changes to D53368: [Symbol] Search symbols with name and type in a symbol file.

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.

Mon, Oct 22, 10:44 AM · Restricted Project
clayborg requested changes to D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it..

See inlined comments

Mon, Oct 22, 10:28 AM · Restricted Project
clayborg added a comment to D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected.

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.

Mon, Oct 22, 9:16 AM

Fri, Oct 19

clayborg added a comment to D53379: GSYM symbolication format.

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

Fri, Oct 19, 12:13 PM

Oct 17 2018

clayborg updated subscribers of D53379: GSYM symbolication format.
Oct 17 2018, 2:08 PM
clayborg added a reviewer for D53379: GSYM symbolication format: lemo.
Oct 17 2018, 10:19 AM
clayborg created D53379: GSYM symbolication format.
Oct 17 2018, 9:01 AM

Oct 16 2018

clayborg accepted D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children.

Looks good. Nice cleanup.

Oct 16 2018, 1:21 PM · Restricted Project
clayborg added a comment to D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children.

See inlined comment.

Oct 16 2018, 10:27 AM · Restricted Project
clayborg added a comment to D53321: Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children.

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.

Oct 16 2018, 7:33 AM · Restricted Project
clayborg accepted D53193: [LLDB] - Add support for DW_RLE_start_end entries (.debug_rnglists).

LGTM

Oct 16 2018, 7:20 AM

Oct 15 2018

clayborg added a comment to D53297: [lldbsuite] Make the names of test classes unique.

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

Oct 15 2018, 12:19 PM

Oct 11 2018

clayborg added a comment to D53140: [LLDB] - Add support for DW_RLE_base_address and DW_RLE_offset_pair entries (.debug_rnglists).

See inlined comments.

Oct 11 2018, 9:02 AM
clayborg accepted D52689: [LLDB] - Add support for DW_FORM_implicit_const..

Thanks for making the changes. Looks good!

Oct 11 2018, 8:52 AM
clayborg added a comment to D52689: [LLDB] - Add support for DW_FORM_implicit_const..

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

Oct 11 2018, 7:18 AM

Oct 10 2018

clayborg requested changes to D52689: [LLDB] - Add support for DW_FORM_implicit_const..

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

Oct 10 2018, 9:56 AM

Oct 9 2018

clayborg accepted D53010: Add an alias "var" for "frame var" and "vo" for "frame var -O".
Oct 9 2018, 7:45 AM
clayborg requested changes to D52689: [LLDB] - Add support for DW_FORM_implicit_const..

See inline comments.

Oct 9 2018, 7:43 AM
clayborg requested changes to D53008: Add a check whether or not a str is utf8 prior to emplacing.

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.

Oct 9 2018, 7:23 AM
clayborg accepted D52745: Adjust the comment section of CreateSource to account for lines longer than 60.
Oct 9 2018, 7:12 AM
clayborg accepted D52981: [LLDB] - Add basic support for .debug_rnglists section (DWARF5).
Oct 9 2018, 7:11 AM
clayborg accepted D52672: Set stdout/stdin to binary mode on Windows.
Oct 9 2018, 7:09 AM

Oct 8 2018

clayborg added a comment to D52981: [LLDB] - Add basic support for .debug_rnglists section (DWARF5).

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

Oct 8 2018, 9:34 AM
clayborg requested changes to D52981: [LLDB] - Add basic support for .debug_rnglists section (DWARF5).

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

Oct 8 2018, 9:30 AM