Page MenuHomePhabricator

clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (260 w, 5 d)

Recent Activity

Wed, Sep 18

clayborg added a comment to D67589: Fix crash on SBCommandReturnObject & assignment.

We couldn't use PointerIntPair due to the our abi stability promise (http://lldb.llvm.org/resources/sbapi.html).

Thanks for this document! I did not know it. But isn't the document trying to keep ABI compatibility despite it is not explicitly stated there? Or otherwise why are there so many restrictions.

Wed, Sep 18, 11:06 AM · Restricted Project

Tue, Sep 17

clayborg added a comment to D67506: GSYM: add encoding and decoding to FunctionInfo.

So it seems that "constexpr" not needing to be captured is not consistent across all builds. The attempt to fix with 372144 then caused some buildbots that have warnings as errors to fail since they would warn that you shouldn't capture a constexpr and that became an error. Windows buildbots require the capture to be there or they fail.

Tue, Sep 17, 1:44 PM
clayborg committed rGf71ececda203: Fix buildbots. (authored by clayborg).
Fix buildbots.
Tue, Sep 17, 1:34 PM
clayborg committed rGc6b156cbb83a: GSYM: Add the llvm::gsym::Header header class with tests (authored by clayborg).
GSYM: Add the llvm::gsym::Header header class with tests
Tue, Sep 17, 10:45 AM
clayborg added inline comments to D67666: GSYM: Add the llvm::gsym::Header header class with tests.
Tue, Sep 17, 10:33 AM · Restricted Project
clayborg created D67666: GSYM: Add the llvm::gsym::Header header class with tests.
Tue, Sep 17, 9:26 AM · Restricted Project
clayborg committed rGb52650d57f86: GSYM: add encoding and decoding to FunctionInfo (authored by clayborg).
GSYM: add encoding and decoding to FunctionInfo
Tue, Sep 17, 9:19 AM
clayborg closed D67506: GSYM: add encoding and decoding to FunctionInfo.

$ svn commit
Sending include/llvm/DebugInfo/GSYM/FunctionInfo.h
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ...done
Committing transaction...
Committed revision 372135.

Tue, Sep 17, 9:19 AM
clayborg added a comment to D67506: GSYM: add encoding and decoding to FunctionInfo.

Any more issues to resolve or does this look good now?

Tue, Sep 17, 8:52 AM
clayborg added inline comments to D67589: Fix crash on SBCommandReturnObject & assignment.
Tue, Sep 17, 8:51 AM · Restricted Project
clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1672210, @kwk wrote:

So my point of this whole question is: What makes a symbol unique in the sense that it shouldn't be added to the symtab if it is already there?

A symbol name is not unique because you can have multiple (static) functions with the same (mangled) name in one module. An address is not unique as well because you can have symbol aliases, which will have the same address (and we want to keep both names to resolve name breakpoints correctly for instance).

The name+address combination (my original suggestion) should be sufficiently unique for the purposes we care about. Theoretically, if you want, you could include some additional items in the uniqueness "key" like symbol type etc. (to rule out the perverse case of somebody setting a "file" symbol to conflict with some other function symbol), but I don't think that is really necessary.

Tue, Sep 17, 8:43 AM · Restricted Project, Restricted Project
clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1672210, @kwk wrote:

@clayborg thank you for this explanation. My patch for minidebuginfo is already done in D66791 . But this one here I was asked to pull out as a separate patch. For good reasons as we see. I wonder how this extra parameter SymbolMapType of yours does help. In the end I have to identify duplicates. But if no symbol with the same name should be added then why do I care about where the symbol is coming from?

Tue, Sep 17, 8:43 AM · Restricted Project, Restricted Project

Mon, Sep 16

clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1671463, @kwk wrote:

@clayborg what address is it exactly to store here std::map<lldb::addr_t, ContString> SymbolMapType;? As an example dc_symbol.GetAddress().GetFileAddress() is something that would work but as soon as we have minidebuginfo, then we might end having the same symbol coming from two different object files and so their address would still be different. Also do you want me to keep this map in ObjectFileELF?

Mon, Sep 16, 10:02 AM · Restricted Project, Restricted Project
clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1671463, @kwk wrote:

@clayborg what address is it exactly to store here std::map<lldb::addr_t, ContString> SymbolMapType;? As an example dc_symbol.GetAddress().GetFileAddress() is something that would work but as soon as we have minidebuginfo, then we might end having the same symbol coming from two different object files and so their address would still be different. Also do you want me to keep this map in ObjectFileELF?

Mon, Sep 16, 9:42 AM · Restricted Project, Restricted Project
clayborg updated the diff for D67506: GSYM: add encoding and decoding to FunctionInfo.
  • Change "enum InfoType" to "enum InfoType : uint32_t"
Mon, Sep 16, 7:43 AM
clayborg added inline comments to D67589: Fix crash on SBCommandReturnObject & assignment.
Mon, Sep 16, 7:28 AM · Restricted Project
clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1671128, @kwk wrote:
In D67390#1667270, @kwk wrote:

@labath how shall we go about this? We do have the situation that when you lookup a symbol you might find it twice if it is in .dynsym and in .symtab. Shall I adjust the test expectation to that or change my implementation?

That's a good question (and another reason why I wanted this to be a separate patch). Since only two tests broke it does not seem like having some symbols twice does much harm. OTOH, having an identical symbol twice does seem like asking for trouble down the line. One possibility would be to restrict this merging to the gnu_debugdata case only. Another option would be to make the merging code smarter and avoid adding the symbol a second time if it has the same name and address. That would have the advantage of having the symbol just once in the common case, while still preserving the full information (in case the symbol tables were munged independently of the gnu_debugdata thingy).

Overall, I guess I would prefer the last solution (inserting only different symbols) unless that turns out to be difficult. In that case, I think just restricting this to gnu_debugdata is fine.

The crucial line is the following line in ObjectFileELF::ParseSymbols():

symtab->AddSymbol(dc_symbol);

If I change that to:

symtab->FindSymbolsByNameAndAddress(dc_symbol.GetName(), dc_symbol.GetAddress(), indexVector)
if (indexVector.empty()) {
  symtab->AddSymbol(dc_symbol);
}
Mon, Sep 16, 7:14 AM · Restricted Project, Restricted Project
clayborg added a comment to D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections.
In D67390#1667270, @kwk wrote:

@labath how shall we go about this? We do have the situation that when you lookup a symbol you might find it twice if it is in .dynsym and in .symtab. Shall I adjust the test expectation to that or change my implementation?

That's a good question (and another reason why I wanted this to be a separate patch). Since only two tests broke it does not seem like having some symbols twice does much harm.

Mon, Sep 16, 7:09 AM · Restricted Project, Restricted Project

Fri, Sep 13

clayborg accepted D67569: [lldb-vscode] correctly handle multiple sourceMap entries.
Fri, Sep 13, 4:27 PM
clayborg updated the diff for D67506: GSYM: add encoding and decoding to FunctionInfo.
  • Fix two operator bool calls to use "bool(<object>)" instead of "<object>.operator bool()"
Fri, Sep 13, 10:40 AM

Thu, Sep 12

clayborg committed rGd44d9e8cda0c: [NFC] Fix file header filename to be Range.h (authored by clayborg).
[NFC] Fix file header filename to be Range.h
Thu, Sep 12, 3:22 PM
clayborg updated the diff for D67506: GSYM: add encoding and decoding to FunctionInfo.
  • Fix comment from /< to /
  • change "sizeof(uint32_t)" to "4"
Thu, Sep 12, 3:08 PM
clayborg accepted D67472: [Target] Move InferiorCall to Process.

lgtm. Jim?

Thu, Sep 12, 2:25 PM · Restricted Project, Restricted Project
clayborg updated the diff for D67506: GSYM: add encoding and decoding to FunctionInfo.
  • Updates license on FunctionInfo.cpp
Thu, Sep 12, 2:25 PM
clayborg added a comment to D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans.

Added Jason Molenda who owns the unwind stuff so he might be able to comment and help us with this patch.

Thu, Sep 12, 10:30 AM · Restricted Project, Restricted Project
clayborg added a reviewer for D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans: jasonmolenda.
Thu, Sep 12, 10:30 AM · Restricted Project, Restricted Project
clayborg added a comment to D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans.

My opinion is we need to be able to ask both the SymbolFile and ObjectFile for unwind plans in the API, but we can always just ask the SymbolFile for the unwind plan and the default SymbolFile virtual function can just defer to the ObjectFile. We also need to say "I need an async unwind plan" or "I need an sync unwind plan". Since we always have a symbol file, we fall back to SymbolFileSymtab if we don't have DWARF or PDB.

Thu, Sep 12, 10:30 AM · Restricted Project, Restricted Project
clayborg created D67506: GSYM: add encoding and decoding to FunctionInfo.
Thu, Sep 12, 9:33 AM

Wed, Sep 11

clayborg committed rG7fcc2c2b5a9e: Add a LineTable class to GSYM and test it. (authored by clayborg).
Add a LineTable class to GSYM and test it.
Wed, Sep 11, 1:50 PM
clayborg closed D66602: Add a LineTable class to GSYM and test it..

$ svn commit
Sending include/llvm/DebugInfo/GSYM/FunctionInfo.h
Adding include/llvm/DebugInfo/GSYM/LineTable.h
Sending lib/DebugInfo/GSYM/CMakeLists.txt
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Adding lib/DebugInfo/GSYM/LineTable.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ......done
Committing transaction...
Committed revision 371657.

Wed, Sep 11, 1:49 PM
clayborg updated the diff for D66602: Add a LineTable class to GSYM and test it..

Switch LineTable and InlineInfo inside of FunctionInfo to be Optional<T>.

Wed, Sep 11, 11:13 AM

Tue, Sep 10

clayborg updated the diff for D66602: Add a LineTable class to GSYM and test it..
  • Fixed license on LineTable.h/.cpp
  • Fixed camel case on encode_special
  • Remote LTOC_ prefix from LineTableOpCode enumerators
Tue, Sep 10, 6:32 PM
clayborg added inline comments to D66602: Add a LineTable class to GSYM and test it..
Tue, Sep 10, 6:32 PM

Mon, Sep 9

clayborg committed rG4f68c226a561: Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz (authored by clayborg).
Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz
Mon, Sep 9, 2:48 PM
clayborg added a comment to D67370: Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz.

$ git llvm push
Pushing 1 monorepo commit:

f54a49a8133 Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz

Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py
Sending lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
Transmitting file data ..done
Committing transaction...
Committed revision 371457.
Committed f54a49a8133 to svn.

Mon, Sep 9, 2:48 PM · Restricted Project
clayborg created D67370: Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz.
Mon, Sep 9, 1:39 PM · Restricted Project
clayborg added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

LGTM. Test seems about as good as we can do. Pavel, you ok with this now?

Mon, Sep 9, 9:54 AM · Restricted Project
clayborg added a comment to D66602: Add a LineTable class to GSYM and test it..

Does anyone have a chance to look at this? Full documentation on the line table encoding is in the LineTable.h header file in the comments.

Mon, Sep 9, 9:54 AM
clayborg added a comment to D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans.

At the object file level I would love to see much less of this specific unwind info making it into the ObjectFile interface. Why can't we just ask the ObjectFile to provide an UnwindPlan for a given address. There is so much complexity within the UnwindPlan where it is managing all these specific unwind plans. IMHO we should just ask the ObjectFile for an UnwindPlan and let the ObjectFile decide which one is best. Right now UnwindPlan manages a ton of different kinds of unwind alternatives and UnwindPlan has all sorts of knowledge about each specific unwind plan type (ARM compact unwind, Apple compact unwind, EH frame, .debug_frame, arch default, and more). We seem to be adding to this complexity here by making all ObjectFile instances having to know how to create each different type when UnwindPlan already has the all the abstraction we need in UnwindPlan::Row.

Mon, Sep 9, 7:51 AM · Restricted Project, Restricted Project

Fri, Sep 6

clayborg added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

This looks like a good approach to me. Pavel?

Even without tests? Just to clarify my last comment, I could not find a way to use the command "target modules dump as" to ensure we won't have a regression in the future. Apparently, with or without the extra calls, that command's output looks the same.

Fri, Sep 6, 8:28 AM · Restricted Project
clayborg added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

This looks like a good approach to me. Pavel?

Fri, Sep 6, 7:35 AM · Restricted Project
clayborg accepted D67239: [Core] Remove use of ClangASTContext in DumpDataExtractor.
Fri, Sep 6, 7:31 AM · Restricted Project, Restricted Project
clayborg added inline comments to D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding.
Fri, Sep 6, 7:25 AM · Restricted Project
clayborg added a comment to D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding.

So made a "sbt" command in a Python module as a way to get a backtrace when our backtracing fails because Android almost never makes it back to the first frame for any stacks. This code just grabs the current SP, looks up the memory region using the SP, and then it uses this as the address range (current SP back to the base of the stack) to scan for any pointers with an address aligned address that falls into executable sections. What it doesn't do, is to try and backup one instruction from the potential RA values to see if there is a branch to the current function. This avoids having to know anything about stack parameter sizes and just scan the stack for all executable memory region pointers, and then ask each architecture to backup one instruction from each proposed RA and see if there is a function call to the current function. For ARM this is easy: backup by 4 for ARM and 2 or 4 for Thumb and see if there is a branch and link. For x64, it would be trickier, but it might be as easy as backup up by 1 + address size and looking for a callq. If we can successfully do this, we could really improve stack backtracing for all architectures and OSs. Thoughts?

Fri, Sep 6, 7:22 AM · Restricted Project
clayborg added inline comments to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Fri, Sep 6, 7:08 AM

Thu, Sep 5

clayborg updated the diff for D66602: Add a LineTable class to GSYM and test it..

Add full error handling to encoding and decoding in the same way that I did it in https://reviews.llvm.org/D66600.

Thu, Sep 5, 11:53 AM
clayborg retitled D66602: Add a LineTable class to GSYM and test it. from Add a LineTable class and test it. to Add a LineTable class to GSYM and test it..
Thu, Sep 5, 11:50 AM

Wed, Sep 4

clayborg added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Thanks for the clarification Greg.

Functionally, this patch seems fine, but I am still wondering if we shouldn't make this more efficient. std::set is not the most memory-efficient structure, and so creating a std::set entry just to store one bit seems wasteful, particularly as there is already a container which uses the context as a key (m_decl_ctx_to_die). Could just shove this bit into the m_decl_ctx_to_die map (probably by changing it to something functionally equivalent to map<DeclContext, pair<bool, vector<DWARFDie>>>)? Given that the sole "raison d´être" of this map is to enable the ParseDeclsForContext functionality, I don't think that having that flag be stored there should be awkward. Quite the opposite, it would remove the awkward back-and-forth between the SymbolFileDWARF and the DWARFASTParsers (symbol file calls ForEachDIEInDeclContext, passing it a callback; then ast parser calls the callback; finally the callback immediately re-enters the parser via GetDeclForUIDFromDWARF) -- instead we could just have the SymbolFileDWARF do ast_parser->PleaseParseAllDiesInThisContextIfTheyHaventBeenParsedAlready(ctx) and have everything happen inside the parser.

So, overall, it seems to me that this way, we could improve the code readability while reducing the time, and avoiding a bigger memory increase. In fact, since we now will not be using the list of dies after they have been parsed once, we could even free up the vector<DWARFDie> after the parsing is complete, and thereby reduce the memory footprint as well. That would be a win-win-win scenario. :)

WDYT?

Wed, Sep 4, 11:01 AM · Restricted Project
clayborg committed rG7d0a545ee65e: Add encode and decode methods to InlineInfo and document encoding format to the… (authored by clayborg).
Add encode and decode methods to InlineInfo and document encoding format to the…
Wed, Sep 4, 10:33 AM
clayborg closed D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Wed, Sep 4, 10:33 AM
clayborg added a comment to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.

$ svn commit
Sending include/llvm/DebugInfo/GSYM/InlineInfo.h
Sending include/llvm/DebugInfo/GSYM/Range.h
Sending lib/DebugInfo/GSYM/InlineInfo.cpp
Sending lib/DebugInfo/GSYM/Range.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data .....done
Committing transaction...
Committed revision 370936.

Wed, Sep 4, 10:32 AM
clayborg added inline comments to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Wed, Sep 4, 10:29 AM

Tue, Sep 3

clayborg accepted D67001: [lldb] Limit the amount of zeroes we use for padding when printing small floats.
Tue, Sep 3, 8:03 AM · Restricted Project, Restricted Project
clayborg added a comment to D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance.

Hey guys,

This change is more for me to get to know what you think. I've noticed that the GetDeclForUIDFromDWARF() calls inside SymbolFileDWARF::ParseDeclsForContext (https://github.com/llvm/llvm-project/blob/ef82098a800178a1f973abb8af86eaa690a29734/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1216) often end up having no side effect besides generating a llvm::DenseMap::find invocation (https://github.com/llvm/llvm-project/blob/f07b4aff06d83c6ad25d95f456fbc12b2d2a0a0c/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L3322), especially when we evaluate multiple expressions inside the same scope. So I was wondering if there's a way to improve that. Do you guys think we could do something like what this change proposes, or am I missing something important? Thanks!

Tue, Sep 3, 7:10 AM · Restricted Project

Fri, Aug 30

clayborg added a comment to D67018: [lldb][NFC] Add basic test for GUI command.

Seems like revision 370054 sets the screen size quite wide. Just checking that this change will be used for this test?

Fri, Aug 30, 1:37 PM · Restricted Project, Restricted Project
clayborg added a comment to D67018: [lldb][NFC] Add basic test for GUI command.

It is great to have tests. The main issue I would see is some of the text might not render correctly if the screen size is too small. Can we control the size (char width and height) of the screen in pexpect tests?

Fri, Aug 30, 1:32 PM · Restricted Project, Restricted Project
clayborg accepted D66863: [lldb] Unify target checking in CommandObject.
Fri, Aug 30, 1:22 PM · Restricted Project, Restricted Project
clayborg updated the diff for D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.

Added error handling to InlineInfo::encode and InlineInfo::decode with full testing.

Fri, Aug 30, 11:27 AM
clayborg added a comment to D66934: [ARM64] Simplify RegisterInfos_arm64.h with macro based RegisterInfo array.

Just a few nits around DEFINE_GPR64 and how the "alt" names are specified. See inline comments.

Fri, Aug 30, 9:02 AM · Restricted Project
clayborg requested changes to D66863: [lldb] Unify target checking in CommandObject.

Just a few string reflows and fix a typo

Fri, Aug 30, 8:53 AM · Restricted Project, Restricted Project

Mon, Aug 26

clayborg added a comment to D66689: [Platform/Android] Read the adb server from an env variable if set.

Added Pavel since he previously worked on Android LLDB at Google. Pavel, please add any additional reviewers to this patch that you think might be interested.

Mon, Aug 26, 1:27 PM · Restricted Project
clayborg added a reviewer for D66689: [Platform/Android] Read the adb server from an env variable if set: labath.
Mon, Aug 26, 1:27 PM · Restricted Project
clayborg added a comment to D66689: [Platform/Android] Read the adb server from an env variable if set.

Should we do this via a "settings set" setting instead of an environment variable? This would make it easier to test running a test suite kind of situation.

Mon, Aug 26, 1:27 PM · Restricted Project
clayborg added a comment to D66689: [Platform/Android] Read the adb server from an env variable if set.

Also is this the server that runs on the host machine where LLDB resides, or remotely on the device? If this is something that would be done at "platform connect" time, each platform can have its own unique options and arguments to "platform connect". Might be better to just add them as options to the "platform connect" command. Something like:

Mon, Aug 26, 1:27 PM · Restricted Project
clayborg updated the diff for D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
  • use llvm_unreachable when encoding InlineInfo objects. We don't want them emitted into the GSYM file and taking up space if they aren't valid.
  • improve comment about terminating child InlineInfo object siblings.
Mon, Aug 26, 11:38 AM
clayborg added inline comments to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Mon, Aug 26, 11:26 AM
clayborg added a comment to D66654: 2/2: Process formatters in reverse-chronological order.

What about having the first one that matched being the one that is used and avoid errors all together? We do this with the regex alias stuff already. With regular expressions people are sure to make ones that conflict with each other.

Mon, Aug 26, 11:19 AM · Restricted Project, Restricted Project
clayborg accepted D66745: DWARFExpression: Simplify class interface.
Mon, Aug 26, 11:11 AM · Restricted Project

Aug 22 2019

clayborg updated the diff for D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.

Don't clear the inline info when decodeAll returns false so that we can see what was actually decoded. Not sure how great of an error we can return other than "not enough data". I am open to suggestions?

Aug 22 2019, 3:26 PM
clayborg accepted D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Now that obj2yaml is a library, we might be able to make some real DWARF to exercise this? I can custom make any DWARF you want as I have a DWARF generator.

Aug 22 2019, 3:25 PM · Restricted Project, Restricted Project, Restricted Project
clayborg added inline comments to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Aug 22 2019, 2:34 PM
clayborg added a comment to D65952: SymbolVendor: Have plugins return symbol files directly.

So I am confused. Are we keeping SymbolVendor around for locating symbols files or are we getting rid of it entirely?

Well... right now my idea was to keep it around as an class with just some static methods for the purposes of locating symbol files, though I'm open to other ideas too. After this patch the SymbolVendor class will contain just one method (FindPlugin), but I think that in the future it could be used to host functionality that is common for different symbol vendors. Here, I'm mainly thinking of Symbols/LocateSymbolFile.cpp (formerly Host/Symbols.cpp), which contains a bunch of functionality for searching for symbol files (so it could fall under the symbol vendor mandate), and it is currently being called by other symbol vendors to do their job. However, it is also being called from placed other than symbol vendors, and so more refactoring would be needed for that to fit in neatly.

Aug 22 2019, 11:15 AM
clayborg added a comment to D66546: Extend FindTypes w/ CompilerContext to allow filtering by language.

Sounds good about LanguageSet being cheap to pass by value. Are there any paths that will call this over and over where we still will be calculating the LangaugeSet over and over in a type system? We might benefit from using llvm::once and a static LanguageSet in the static functions that return LanguageSets if that is the case?

Aug 22 2019, 10:56 AM · Restricted Project
clayborg updated the diff for D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
  • document arguments to "static bool decodeAll(...)" in InlineInfo.cpp
  • If decodeAll() returns false, clear the InlineInfo
Aug 22 2019, 10:48 AM
clayborg added inline comments to D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Aug 22 2019, 10:46 AM
clayborg added a comment to D66546: Extend FindTypes w/ CompilerContext to allow filtering by language.

Looks good overall. Just a question of it we want to return "const LanguageSet &" to avoid copies. Also switch static functions that currently return "LanguageSet" over to use static variables and llvm::once to init them and then return "const LanguageSet &".

Aug 22 2019, 10:38 AM · Restricted Project
clayborg created D66602: Add a LineTable class to GSYM and test it..
Aug 22 2019, 10:23 AM
clayborg created D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Aug 22 2019, 9:41 AM
clayborg retitled D66600: Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format from Add encode and decode methods to InlineInfo and document encoding format. to Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
Aug 22 2019, 9:41 AM

Aug 21 2019

clayborg committed rGbf9ee07afa3f: Add FileWriter to GSYM and encode/decode functions to AddressRange and… (authored by clayborg).
Add FileWriter to GSYM and encode/decode functions to AddressRange and…
Aug 21 2019, 2:52 PM

Aug 19 2019

clayborg requested changes to D66447: Add char8_t support (C++20).
Aug 19 2019, 3:19 PM · Restricted Project, Restricted Project
clayborg added a comment to D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges.

Let's ask the underlying question: Which of the above use-cases does a GSYM writer need, and why isn't the StringRef case sufficient?

Aug 19 2019, 1:39 PM · Restricted Project
clayborg added a comment to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Pavel, any comments on the testing code? LGTM.

Aug 19 2019, 1:30 PM · Restricted Project, Restricted Project, Restricted Project
clayborg added inline comments to D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges.
Aug 19 2019, 1:28 PM · Restricted Project
clayborg updated the diff for D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges.
  • remove function name from doxygen comments
  • change @param to \param
  • fix typos
Aug 19 2019, 1:27 PM · Restricted Project
clayborg updated the diff for D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges.
  • added full FileWriter doxygen comments
  • renamed writeCStr to writeNullTerminated
  • update all DataExtractor offsets to be uint64_t to match recent DataExtractor changes
Aug 19 2019, 10:08 AM · Restricted Project
clayborg added a comment to D63828: Add FileWriter to GSYM and encode/decode functions to AddressRange and AddressRanges.

Can anyone take a look please? Another week, no comments.

Aug 19 2019, 8:28 AM · Restricted Project
clayborg added a comment to D66345: [lldb][NFC] Allow for-range iterating over StringList.

The other thing to think about is the SB layer must be thread safe. StringList is not thread safe right now, but it probably should be. We might need to keep the internal version of StringList and make sure it is thread safe. That will ensure that our buildbots always run clean.

Aug 19 2019, 7:27 AM · Restricted Project
clayborg added a comment to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Needs a test, but looks good.

Sure, I agree. Though, it's not total clear to me how we could do that.

I imagined something like the following.

TEST_F(DWARFASTParserClangTests,
       TestGetDIEForDeclContextReturnsOnlyMatchingEntries) {
  auto ast = ClangASTContext(...);
  auto ast_parser = DWARFASTParserClang(ast);
  CompilerDeclContext decl1(nullptr, (void *)1LL);
  CompilerDeclContext decl2(nullptr, (void *)2LL);
  CompilerDeclContext decl3(nullptr, (void *)2LL);
  CompilerDeclContext decl4(nullptr, (void *)3LL);
  ast_parser->LinkDeclContextToDIE(decl1, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl2, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl3, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl4, DWARFDIE());

  auto decl_ctx_die_list = ast_parser->GetDIEForDeclContext(decl2);
  EXPECT_EQ(2u, decl_ctx_die_list.size());
  EXPECT_EQ(decl2.GetOpaqueDeclContext(),
            decl_ctx_die_list[0].GetDeclContext().GetOpaqueDeclContext());
  EXPECT_EQ(decl3.GetOpaqueDeclContext(),
            decl_ctx_die_list[1].GetDeclContext().GetOpaqueDeclContext());
}

But DWARFASTParserClang::LinkDeclContextToDIE is protected. Do you have any ideas to overcome that?

Aug 19 2019, 7:17 AM · Restricted Project, Restricted Project, Restricted Project

Aug 16 2019

clayborg requested changes to D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context.

Needs a test, but looks good.

Aug 16 2019, 11:23 AM · Restricted Project, Restricted Project, Restricted Project
clayborg added a comment to D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB.

Why are we not just using ObjectFileJIT? I am guessing these breakpoint expressions create one of these by compiling the breakpoint expression and JIT'ing it just like any other expression. If this is the case, then why do we need to create a ObjectFileTrampoline? Seems like we could add .cfi directives to the assembly we use for the breakpoint condition function so that we can unwind out of it?

Aug 16 2019, 11:23 AM · Restricted Project

Aug 15 2019

clayborg accepted D66241: stop-hooks don't fire on "step-out".

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

Aug 15 2019, 7:10 AM · Restricted Project, Restricted Project

Aug 14 2019

clayborg added inline comments to D66241: stop-hooks don't fire on "step-out".
Aug 14 2019, 3:50 PM · Restricted Project, Restricted Project
clayborg added inline comments to D66241: stop-hooks don't fire on "step-out".
Aug 14 2019, 3:00 PM · Restricted Project, Restricted Project
clayborg added a comment to D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex.

Looks fine to me. A few nits.

Aug 14 2019, 2:58 PM · Restricted Project, Restricted Project

Aug 13 2019

clayborg accepted D62570: Use LLVM's debug line parser in LLDB.
Aug 13 2019, 12:36 PM · Restricted Project, Restricted Project
clayborg added a comment to D62570: Use LLVM's debug line parser in LLDB.

I agree with Adrian, this isn't something that needs to hold up this patch, but I do think it is worth doing soon in LLVM. The line table parsing code just needs to take a std::function (or llvm equivalent) that gets called like a lambda when a row is to be pushed. The current LLVM parsing code just has this callback create its own line tables as is currently done. We would just create our own line tables using our callback. So it won't take much work at all in llvm to make this happen and then be able to adopt it in LLDB.

Aug 13 2019, 10:42 AM · Restricted Project, Restricted Project
clayborg added a comment to D62570: Use LLVM's debug line parser in LLDB.

Thanks for running the perf tests, that will be great to keep handy somewhere. Might be good to check this test in somewhere. I still think that making changes to the llvm line parser might be a good idea where we can get a callback when a row is to be pushed with the current line table state. This would allow symbolication clients that need just one address, like in atos kind of cases, to not have to store an array of these things and then search them. It would also allow us to just populate our line table as we see fit without the need to have complete LLVM tables and then convert them. Shouldn't be very hard to modify the code in llvm. Let me know what you think.

Aug 13 2019, 9:58 AM · Restricted Project, Restricted Project
clayborg accepted D65739: [API] Have SBCommandReturnObject::GetOutput/Error return "" instead of nullptr.
Aug 13 2019, 9:54 AM · Restricted Project

Aug 12 2019

clayborg added a comment to D66102: [Symbol] Decouple clang from CompilerType.

lgtm

Aug 12 2019, 2:20 PM · Restricted Project, Restricted Project