clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Wed, Apr 18

clayborg added a comment to D45700: Improve LLDB's handling of non-local minidumps.

LGTM

Wed, Apr 18, 5:45 PM · Restricted Project
clayborg accepted D45628: [LLDB] Support compressed debug info sections (.zdebug*).

Very nice!

Wed, Apr 18, 9:07 AM · Restricted Project

Tue, Apr 17

clayborg added a comment to D45703: Use a utility function to speed up "process load image" for POSIX Platforms.

I am fine if we don't want to do the general solution now. LGTM if all is well in the test suite.

Tue, Apr 17, 11:24 AM
clayborg added a comment to D45703: Use a utility function to speed up "process load image" for POSIX Platforms.

I don't think doing this is necessary when we have only one customer, but if we are going to be designing an general purpose storage facility then I don't think we should be using strings (and particularly not ConstStrings) as the lookup keys.

I would propose going for the the token based approach where each customer has a "token" (it can be just a char) and then you use the address of that token as the key.

Tue, Apr 17, 9:38 AM
clayborg added inline comments to D45703: Use a utility function to speed up "process load image" for POSIX Platforms.
Tue, Apr 17, 7:22 AM

Mon, Apr 16

clayborg added a comment to D45700: Improve LLDB's handling of non-local minidumps.

Looks good. I questions if we want PlaceholderModule to be available for all symbolicators/core dump plugins. See inlined comments.

Mon, Apr 16, 4:10 PM · Restricted Project

Fri, Apr 13

clayborg added a comment to D45628: [LLDB] Support compressed debug info sections (.zdebug*).

We should test both ways: using normal DWARF section names with SHF_COMPRESSED, and with ".zdebug" prefixed section names.

Fri, Apr 13, 2:57 PM · Restricted Project
clayborg added a comment to D45628: [LLDB] Support compressed debug info sections (.zdebug*).

If this is for current and future debugging, it would be nice to change the tool to just use the normal .debug prefixes and just specify SHF_COMPRESSED???

Fri, Apr 13, 10:41 AM · Restricted Project
clayborg added inline comments to D45628: [LLDB] Support compressed debug info sections (.zdebug*).
Fri, Apr 13, 10:36 AM · Restricted Project
clayborg added a comment to D45592: Allow relative file paths when settings source breakpoints.

Didn't update the diffs, but I did fix the things Davide requested before submission

Fri, Apr 13, 8:10 AM
clayborg added a comment to D45554: Make sure deleting all breakpoints clears their sites first.

There is an ownership cycle between BreakpointSite::m_owners and BreakpointLocation::m_bp_site_sp.
We should probably make m_owners a collection of weak references.
But currently most of the code just works it around by calling Breakpoint::ClearAllBreakpointSites() before deleting a breakpoint.

Fri, Apr 13, 7:52 AM

Thu, Apr 12

clayborg updated the diff for D45592: Allow relative file paths when settings source breakpoints.

Fixed the breakpoint verification in lldbutil to make sure the file name ends with the right thing

Thu, Apr 12, 3:56 PM
clayborg added inline comments to D45592: Allow relative file paths when settings source breakpoints.
Thu, Apr 12, 3:51 PM
clayborg accepted D45586: Prevent deadlock in OS Plugins.
Thu, Apr 12, 2:18 PM
clayborg created D45592: Allow relative file paths when settings source breakpoints.
Thu, Apr 12, 2:12 PM

Wed, Apr 11

clayborg accepted D45497: Don't assume the backing thread shares a Protocol ID.
Wed, Apr 11, 10:39 PM
clayborg added a comment to D44668: [demangler] Add a "partial" demangling API for LLDB.

Looks good to me, but probably need someone that works on LLVM fulltime to give the final ok

Wed, Apr 11, 10:51 AM

Tue, Apr 10

clayborg requested changes to D45497: Don't assume the backing thread shares a Protocol ID.
Tue, Apr 10, 11:44 AM

Mon, Apr 9

clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

For DWZ files, it seems types can be contained in external DWARF files and are referred to using new forms. The right way to fix this in LLDB it to load each of these external files as a separate DWZ file (no changes needed to anything) and then teach the SymbolFileDWARF that refers to these files to import the types. If we have 24 shared libraries that use a single DWZ, we shouldn't be loading it more than once and inlining the DWARF, we should be using the AST importer to import the type from that file into the AST for the current DWARF file. We do have done this with -gmodules already, so DWZ shouldn't be that different. Let me know if I am missing anything.

Mon, Apr 9, 6:52 PM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Found the DWZ stuff:
http://www.dwarfstd.org/ShowIssue.php?issue=120604.1&type=open

Mon, Apr 9, 6:47 PM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Is there a link to some documentation that explains what DWZ does? I found a bunch of blurbs and man pages, but nothing useful. Nothing is found when I look for "DWZ" in either the DWARF 4 or DWARF 5 spec.

Mon, Apr 9, 6:45 PM

Wed, Apr 4

clayborg added a comment to D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit.

I have only one general question and otherwise I'm fine with this: Does this bring LLDB's DWARF parser closer to the structure of LLVM's? We still want to long-term replace LLDB's DWARF parser with LLVM's so every refactoring should aim at making their interfaces more similar.

Wed, Apr 4, 8:12 PM
clayborg added a comment to D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit.

ping

Wed, Apr 4, 9:18 AM

Mon, Apr 2

clayborg accepted D44998: ObjectFileELF: Add support for arbitrarily named code sections.
Mon, Apr 2, 1:24 PM · Restricted Project
clayborg added a comment to D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit.

No real functional change, just fixing layering and moving code. Also a renaming from GetCompileUnitDIEOnly to GetUnitDIEOnly.

Mon, Apr 2, 10:13 AM
clayborg added a comment to D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit.

Yes this is just moving all ivars from DWARFCompileUnit to DWARFUnit, moving all functions that used those accessors to DWARFUnit and remove the indirection through DWARFCompileUnit that was in DWARFUnit using:

Mon, Apr 2, 10:12 AM
clayborg created D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit.
Mon, Apr 2, 9:45 AM
clayborg requested changes to D44998: ObjectFileELF: Add support for arbitrarily named code sections.
Mon, Apr 2, 8:32 AM · Restricted Project
clayborg accepted D44613: Support template template parameters.

Sorry for the delay.

Mon, Apr 2, 8:21 AM

Thu, Mar 29

clayborg requested changes to D44752: Implement TypeAndOrName::TypeAndOrName(const CompilerType &).

We probably need a test then.

Thu, Mar 29, 10:55 AM
clayborg added a comment to D44752: Implement TypeAndOrName::TypeAndOrName(const CompilerType &).

Can you add this to the patch for the rust plug-in then?

Thu, Mar 29, 10:40 AM
clayborg added a comment to D44752: Implement TypeAndOrName::TypeAndOrName(const CompilerType &).

Should this be removed then? If not one was using it, we should remove it from the header file.

Thu, Mar 29, 9:21 AM
clayborg accepted D44907: Remove Scalar::Cast.

Very nice. Thanks for doing this. Someone a while back had converted Scalar to use APInt and APFloat and never got around to all of these details.

Thu, Mar 29, 9:19 AM

Wed, Mar 28

clayborg added a comment to D44998: ObjectFileELF: Add support for arbitrarily named code sections.

Quick test case where we have an ELF file with a .text section and with another executable section with something in it that we can set breakpoints in. Easiest to make an ELF file that fits the bill, and then run obj2yaml on it. You check that into a test case directory and have the test case run yaml2obj on it and run the test.

Wed, Mar 28, 1:49 PM · Restricted Project

Tue, Mar 27

clayborg added a comment to D44437: Avoid GEP when creating a breakpoint.

I would remove all of the "need_adjust_break_address" and just always call that function in the architecture? Or only call it if m_skip_prologue is true? Seems like this logic should always happen. Seems like magic to only adjust the breakpoint address if the prologue byte size is zero or we aren't skipping the prologue. Feel free to pass any extra args down into AdjustBreakpointAddress() if needed (like m_skip_prologue or prologue_byte_size).

What if we remove the two identical blocks:

Adjust_Block:

if (m_skip_prologue && break_addr.IsValid()) {
  const uint32_t prologue_byte_size =
      sc.function->GetPrologueByteSize();
  if (prologue_byte_size)
    break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
  else
    need_adjust_break_address = true;
}else {
  need_adjust_break_address = true;
}

and create a new local function in BreakpointResolverName, like:

BreakpointResolverName::AdjustBreakpointAddress(SymbolContext &context, bool m_skip_prologue, Address break_addr){
}

so, inside the function, I would place the block I mentioned above and remove the need_adjust_break_address = true;.

Why not always do this and let the arch worry about the details of if it needs to be adjusted? Also, the AdjustBreakpointAddress(...) should probably just take a SymbolContext, not just a symbol. Why?
Because you might have stripped your symbol table and you might only have a function from debug info...

Do you mean put the Adjust_Block inside the arch->AdjustBreakpointAddress(? If so, what if arch is null? I don't know if this may happen.

Tue, Mar 27, 12:41 PM
clayborg accepted D44922: gdb-remote: Fix checksum verification for messages with escape chars.
Tue, Mar 27, 9:35 AM

Mar 23 2018

clayborg accepted D44693: Correctly handle float division in Scalar.
Mar 23 2018, 1:29 PM

Mar 22 2018

clayborg added a comment to D44739: [SymbolFileDWARF] Replace FixedFormSizes with llvm::dwarf::getFixedFormByteSize.

I would like to not lose any performance if possible.

Mar 22 2018, 9:47 AM

Mar 21 2018

clayborg added a comment to D44693: Correctly handle float division in Scalar.

Bonus points for catching any other conversion errors that don't match the current C/C++ specs. The unit tests should be an easy place to test these kinds of things.

Mar 21 2018, 10:58 AM
clayborg added a comment to D44693: Correctly handle float division in Scalar.

Seems like a unit test would work well in this case.

Mar 21 2018, 9:44 AM
clayborg requested changes to D44693: Correctly handle float division in Scalar.

Actually, we should add a test for this to ensure this doesn't regress.

Mar 21 2018, 9:41 AM
clayborg accepted D44693: Correctly handle float division in Scalar.
Mar 21 2018, 9:34 AM
clayborg added a comment to D44739: [SymbolFileDWARF] Replace FixedFormSizes with llvm::dwarf::getFixedFormByteSize.

Looks fine. It would be nice to verify that there are no performance regressions due to this before making this change. Can you do some timings to make sure? Do anything that indexes a large C++ DWARF codebase, like clang, and make sure we don't regress by a significant amount.

Mar 21 2018, 9:26 AM

Mar 20 2018

clayborg added inline comments to D44668: [demangler] Add a "partial" demangling API for LLDB.
Mar 20 2018, 1:09 PM
clayborg added a comment to D44668: [demangler] Add a "partial" demangling API for LLDB.

Seems like it might be nice to replace all "char *Buf, size_t *N" arguments, where "char *" is returned with a llvm::raw_ostream and let the user use what ever backing / memory allocation scheme they want instead of forcing re-allocations? Probably best to include a person that has contributed to this file in the LLVM community so they can comment on the changes here.

Mar 20 2018, 7:20 AM
clayborg added a comment to D44668: [demangler] Add a "partial" demangling API for LLDB.

Great stuff, thanks for doing this BTW

Mar 20 2018, 7:20 AM

Mar 19 2018

clayborg added inline comments to D44613: Support template template parameters.
Mar 19 2018, 10:27 AM
clayborg updated subscribers of D44526: [dotest] Clean up test folder clean-up.

I would be nice to have the option to nuke each test build directory if the test passed. Can be an option that we specify. That way, the only folders left over could be the tests that are failing. The options doesn't need to default to true, but if anyone is in that code and would know how to do that, that would be a great option to have. This option could also clean up the log directory and remove any logs for tests that succeeded or were skipped. I alway have to go and nuke these files manually.

Mar 19 2018, 10:17 AM

Mar 16 2018

clayborg added a comment to D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context..

Much better. Make sure Jim is ok with this as I am ok with it if he is.

Mar 16 2018, 12:04 PM

Mar 14 2018

clayborg added a comment to D44502: Fix a bug in "target.source-map" where we would resolve unmapped paths incorrectly.

Committed revision 327600.

Mar 14 2018, 10:17 PM
clayborg added a comment to D44502: Fix a bug in "target.source-map" where we would resolve unmapped paths incorrectly.

The DWARF file that this produces has a DW_AT_comp_dir set to ".". This is similar to how some builds work at Facebook when using Buck to build things.

Mar 14 2018, 4:27 PM
clayborg created D44502: Fix a bug in "target.source-map" where we would resolve unmapped paths incorrectly.
Mar 14 2018, 4:22 PM

Mar 13 2018

clayborg requested changes to D44437: Avoid GEP when creating a breakpoint.

I would remove all of the "need_adjust_break_address" and just always call that function in the architecture? Or only call it if m_skip_prologue is true? Seems like this logic should always happen. Seems like magic to only adjust the breakpoint address if the prologue byte size is zero or we aren't skipping the prologue. Feel free to pass any extra args down into AdjustBreakpointAddress() if needed (like m_skip_prologue or prologue_byte_size).

Mar 13 2018, 10:18 AM

Mar 12 2018

clayborg added inline comments to D44342: Introduce a setting to disable Spotlight while running the test suite.
Mar 12 2018, 1:00 PM
clayborg accepted D42892: DWZ 02/11: Move the codebase to use: DWARFCompileUnit -> DWARFUnit.
Mar 12 2018, 10:33 AM
clayborg added a comment to D44342: Introduce a setting to disable Spotlight while running the test suite.

I would rather see this done in a more generic fashion. Any platform should be able to enable/disable external symbol lookups. Maybe make this setting:

Mar 12 2018, 8:52 AM
clayborg added a comment to D40474: DWZ 05/07: Main functionality.

Too many changes to give the whole thing the OK to checkin. I will worry about each one as an individual patch. 02/11 next.

Mar 12 2018, 8:45 AM
clayborg accepted D40466: DWZ 01/11: DWARFUnit split out of DWARFCompileUnit.

This is fine. Lets start with this and then worry about each next patch. 02/11 next.

Mar 12 2018, 8:45 AM

Mar 8 2018

clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

I personally don't think having a new debug info flavour is a good idea. Tests written specifically to test this functionality will be easier to maintain and debug when they break. And keeping adding new flavours for every compiler option is not a viable long term strategy.

Here I disagree, from GDB testsuite experience I find more wider testing always better as it will better catch bugs even in unrelated new functionalities being implemented in the future. One can never guess all the combinations in advance - if one could then one needs no regression testing at all. There are more than enough CPU cycles available for testing. I find rather a serious problem racy==unreliable test results which waste human efforts investigating the results and with more parallel testing (make -j) and virtualized infrastructure the racy results happen even more often. As an example one racy testcase I idenitifed is:

packages/Python/lldbsuite/test/functionalities/signal/handle-segv/TestHandleSegv.py
line 33: AssertionError: 4 != 5 (eStateLaunching != eStateStopped)

I think at least these test are racy:

BreakpointAfterJoinTestCase-test
 CreateDuringStepTestCase-test_step_inst_with
 HandleSegvTestCase-test_inferior_handle_sigsegv
 ReturnValueTestCase-test_vector_values
 TestTargetSymbolsBuildidSymlink-test_target_symbols_buildid
 ThreadSpecificBreakPlusConditionTestCase-test_python
 ThreadSpecificBreakTestCase-test_python
 ThreadStateTestCase-test_process_interrupt

I'm not necessarily convinced having another column in the matrix is something that can substitute targeted testing.

Mar 8 2018, 9:46 AM

Mar 6 2018

clayborg added a comment to D44166: [SymbolFilePDB] Add missing Char16 and Char32 types in a few places.

Test?

Mar 6 2018, 10:47 AM
clayborg added a comment to D44101: Fix some tests for PPC64le architecture.

If you need to force something to use a global entry point you might be able to switch this test to use a shared library that contains that function. This would then require that the global entry point is used?

Mar 6 2018, 10:26 AM
clayborg added inline comments to D44101: Fix some tests for PPC64le architecture.
Mar 6 2018, 7:35 AM
clayborg accepted D44015: Fix std unique pointer not printing..
Mar 6 2018, 7:28 AM
clayborg added a comment to D44139: Update selected thread after loading mach core.

Change looks fine to me.

Mar 6 2018, 7:26 AM

Mar 5 2018

clayborg accepted D43771: [LLDB][PPC64] Fix TestGdbRemoteAuxvSupport.
Mar 5 2018, 10:28 AM
clayborg added a comment to D44056: [lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel.

Yes we want to avoid ever using this function. It only leads to bad things happening.

Mar 5 2018, 8:38 AM
clayborg added inline comments to D44074: ObjectFileMachO: split CreateSections mega-function into more manageable chunks.
Mar 5 2018, 8:34 AM
clayborg accepted D44074: ObjectFileMachO: split CreateSections mega-function into more manageable chunks.

Looks fine. Would just move stuff from header into .cpp file for the SegmentParsingContext class. I'll leave that up to you.

Mar 5 2018, 7:47 AM
clayborg added a comment to D42955: Make Module::GetSectionList output consistent.

AS for the ELF example where only debug info is around, it might not be running into issues if it doesn't contain a symbol table. If it does contain a symbol table, hopefully it is using the unified section table, otherwise if might have symbol's whose addresses have sections from the stripped ELF file and it might not have the section contents that it needs to back it. In that case we might fail to disassemble the symbol's code.

I made a trivial experiment:

g++ a.cc
objcopy a.out --only-keep-debug a.sym #a.sym contains a symtab
strip a.out # a.out does not contain a symtab
(lldb) target create "a.out"
Current executable set to 'a.out' (x86_64).
(lldb) b main
Breakpoint 1: no locations (pending).
WARNING:  Unable to resolve breakpoint to any actual locations.
(lldb) target symbols add a.sym
symbol file '/tmp/X/a.sym' has been added to '/tmp/X/a.out'
1 location added to breakpoint 1
#symbol resolution seems fine
(lldb) disassemble -n main
a.out`main:
a.out[0x69b] <+0>:  pushq  %rbp
a.out[0x69c] <+1>:  movq   %rsp, %rbp
a.out[0x69f] <+4>:  callq  0x690                     ; foo()
a.out[0x6a4] <+9>:  addl   $0x2a, %eax
a.out[0x6a7] <+12>: popq   %rbp
a.out[0x6a8] <+13>: retq
# so does disassembling

The part that is not clear to me is, if the dsym object file should absorb the sections from the main object file, then what is the purpose of the "unified section list" in the module? I can see why we need a unified list, and I think it's a good idea, but having an ObjectFile containing the merged list seems to defeat it. From a separation of concerns POV, it would be much clearer if each ObjectFile contained only it's own sections, and then some other entity (Module) held the combined data(sections) of the individual object files. Then, most clients should operate on the unified view of the module, but if anyone ever had a reason, he could always look directly at a specific ObjectFile, and see what's contained there.

Mar 5 2018, 7:37 AM

Mar 2 2018

clayborg added a comment to D42955: Make Module::GetSectionList output consistent.

AS for the ELF example where only debug info is around, it might not be running into issues if it doesn't contain a symbol table. If it does contain a symbol table, hopefully it is using the unified section table, otherwise if might have symbol's whose addresses have sections from the stripped ELF file and it might not have the section contents that it needs to back it. In that case we might fail to disassemble the symbol's code.

Mar 2 2018, 3:50 PM
clayborg added a comment to D42955: Make Module::GetSectionList output consistent.

The dSYM file is a mach-o file that contains symbols only, It is because the dSYM file (stand alone debug info file) has all of the section definitions from the main executable, but no section content for everything except the DWARF debug info. The DWARF only exists in the dSYM file. The dSYM file also has all of the symbols as it gets made before the executable is stripped. Many symbols refer to a section by section index, so that is why we must have the section definitions in the dSYM file. So the dSYM wants the sections from the main executable so it can access text and data if needed since symbol refer to these and someone might ask a symbol from a dSYM file what its instructions are. So the dSYM uses the real sections from the main executable file instead of its own.

I see, thanks for the explanation. The code makes more sense now.

But now I have another question. Is this an intentional design decision that we want to preserve; or just how things happen to work now, and should be cleaned up in the future (to achieve the "ObjectFile contains only own sections" invariant)?

Mar 2 2018, 3:48 PM
clayborg added a comment to D42955: Make Module::GetSectionList output consistent.

I can try to split the patch up a bit if you think it will make things easier. I'm not sure how much I will be able to do that, as the patch is not that big, it just needs to touch a bunch of files for the changed interfaces.

Looks fine, a bit hard to tell exactly what is going on so I will accept as long as the following things are still true:

  • each lldb_private::ObjectFile has its own section list that perfectly mirrors exactly what is in that object file and that file alone

This part is not completely true. It is true for ObjectFileELF/COFF/JIT, and is enforced by the fact that the ObjectFile::GetSections does not even have access to the unified section list. However, it is *not* true for ObjectFileMachO (but that is not because of this patch). This is the problem I've had when trying to refactor this (and it's the reason the ObjectFileMachO<->SymbolVendorMacOSX interface is a bit weird). It seems that the ObjectFileMachO sometimes takes a section from the unified list and adds it to it's own list (or at least it appears to be doing that). I don't know enough about MachO to understand why is it doing that, or how to fix it. I've highlighted the places in the code where I think this is happening. Any suggestions would be welcome here.

Mar 2 2018, 3:21 PM
clayborg added a comment to D44042: Ensure that trailing characters aren't included in PECOFF section names.

Will we have the same problem with coff_symbol?

Mar 2 2018, 2:38 PM
clayborg added a comment to D44042: Ensure that trailing characters aren't included in PECOFF section names.

Add test as well.

Mar 2 2018, 2:30 PM
clayborg requested changes to D44042: Ensure that trailing characters aren't included in PECOFF section names.
Mar 2 2018, 2:30 PM
clayborg accepted D42955: Make Module::GetSectionList output consistent.

Looks fine, a bit hard to tell exactly what is going on so I will accept as long as the following things are still true:

  • each lldb_private::ObjectFile has its own section list that perfectly mirrors exactly what is in that object file and that file alone
  • The lldb_private::Module hands out a unified section list that is populated by the symbol vendor where it uses one or more object files to create the unified section list
Mar 2 2018, 2:24 PM

Mar 1 2018

clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Ping for my last comment

Mar 1 2018, 8:25 AM
clayborg added a comment to D43857: Speed up TestWatchpointMultipleThreads.

very nice!

Mar 1 2018, 7:39 AM

Feb 27 2018

clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Would everyone be ok with a solution where we leave the .debug_types tests in as a flavor, but we check the binary after creating it to ensure it has a .debug_types section. If it doesn't, then we skip the test (or pass it?), and if it does, then we go ahead and test it? The only overhead that is added then is some building. Many of the tests don't have types that go into the .debug_types section, so this would be an easy way for us to test .debug_types when it is present and just build and skip if it doesn't. No maintenance needed on any tests, we will just detect and enable if needed?

Feb 27 2018, 9:05 AM
clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

I agree with Pavel, thanks for taking the time to get this done right and listening to the feedback that was offered. The patch is better for it.

Feb 27 2018, 7:13 AM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

I am afraid of the opposite: we test what we think we need to test and our simple tests cases don't adequately test the feature we are adding. I can certainly limit the testing to very simple test cases with a one test for a class and one for a enum, but that wouldn't have caught the issues I ran into with static variables in classes that I fixed before submitting this. My point is it is hard to figure out what a compiler, or debugger might hose up without testing all possible cases. Please chime in with opinions as I will go with the flow on this.

I don't think anyone here is suggesting to test less. The question we are asking is if running all 1000 or so tests doesn't just giv a false sense of security (and makes the testing process longer). A specially crafted test can trigger more corner cases and make it easier to debug future failures that a generic test. If the case of a static variable in a class is interesting, then maybe a test where you have two static variables from the same class defined in two different compilation units (and maybe referenced from a third one) is interesting as well. I'm pretty sure we don't have a test like that right now. Another interesting case which would not be tested in the "separate flavour" mode is the mixed-flags case: have part of your module compiled with type units, part without (and maybe a third part with type units and fission)..

Running the entire dotest test suite in -fdebug-types-section is certainly a good way to catch problems, but it's not the way to write regression tests. FWIW, the way I plan to test the new accelerator tables when I get to the lldb part is to run dotest with the new flags locally during development, use the failures to identify interesting test vectors, and then write regular tests to trigger these.

Feb 27 2018, 7:11 AM

Feb 26 2018

clayborg updated the diff for D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Address Adrian's comments.

Feb 26 2018, 3:29 PM
clayborg updated the diff for D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

clang format
fixed comments
removed white space
fixed functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py failures

Feb 26 2018, 2:13 PM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Note, I have (though very very occasionally) seen a dsymutil bug that broke line tables. So it would be good to keep a few "set a breakpoint and run to it" tests just to sanity check this. But most tests that just run somewhere and print a backtrace or view an integer variable or such-like do not need to run all variants.

The thing here is that I'm not sure everything can be captured by a single "debug info variant" dimension. The current dimensions kinda make sense, as in they alter the way pretty much all of debug info is accessed (we can think of "dwarf" as the basic one, "dsym" and "dwo" are similar in that they place everything in a separate file, I'm not sure about gmodules, but that's because I have no idea on how it works). However, I'm not sure the same goes for type units. For one they are not completely mutually exclusive with the existing variants, so you can have type-units+regular dwarf and type units+dwo (maybe even type units+dsym?). But obviously we can't add another "type unit" dimension and do a cartesian product....

Feb 26 2018, 1:16 PM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

I am afraid of the opposite: we test what we think we need to test and our simple tests cases don't adequately test the feature we are adding. I can certainly limit the testing to very simple test cases with a one test for a class and one for a enum, but that wouldn't have caught the issues I ran into with static variables in classes that I fixed before submitting this. My point is it is hard to figure out what a compiler, or debugger might hose up without testing all possible cases. Please chime in with opinions as I will go with the flow on this.

Feb 26 2018, 1:02 PM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

So, I tried this out on my machine, and got just one failure:

======================================================================
FAIL: test_with_run_command_dwarf_type_units (TestInlinedBreakpoints.InlinedBreakpointsTestCase)
   Test 'b basic_types.cpp:176' does break (where int.cpp includes basic_type.cpp).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1827, in dwarf_type_units_test_method
    return attrvalue(self)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py", line 24, in test_with_run_command
    self.inlined_breakpoints()
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py", line 46, in inlined_breakpoints
    self, "basic_type.cpp", self.line, num_expected_locations=0)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 376, in run_break_set_by_file_and_line
    num_locations=num_expected_locations)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 572, in check_breakpoint_result
    out_num_locations))
AssertionError: False is not True : Expecting 0 locations, got 1.

I saw this too. When I ran _any_ of the variants manually, they all failed to set any breakpoints. Not sure this test is testing what it claims to be testing.

Feb 26 2018, 11:38 AM
clayborg added inline comments to D43771: [LLDB][PPC64] Fix TestGdbRemoteAuxvSupport.
Feb 26 2018, 11:26 AM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

So, I tried this out on my machine, and got just one failure:

======================================================================
FAIL: test_with_run_command_dwarf_type_units (TestInlinedBreakpoints.InlinedBreakpointsTestCase)
   Test 'b basic_types.cpp:176' does break (where int.cpp includes basic_type.cpp).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1827, in dwarf_type_units_test_method
    return attrvalue(self)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py", line 24, in test_with_run_command
    self.inlined_breakpoints()
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py", line 46, in inlined_breakpoints
    self, "basic_type.cpp", self.line, num_expected_locations=0)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 376, in run_break_set_by_file_and_line
    num_locations=num_expected_locations)
  File "/usr/local/google/home/labath/ll/lldb/packages/Python/lldbsuite/test/lldbutil.py", line 572, in check_breakpoint_result
    out_num_locations))
AssertionError: False is not True : Expecting 0 locations, got 1.
Feb 26 2018, 11:01 AM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

This commit has no tests. It should have many.

Feb 26 2018, 10:05 AM
clayborg accepted D43767: [LLDB][PPC64] Fixed issues with expedited registers.
Feb 26 2018, 9:42 AM
clayborg updated the diff for D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Updated to top of tree sources and verified it passes all tests on linux.

Feb 26 2018, 9:02 AM

Feb 23 2018

clayborg added a comment to D43698: Plug errno in TCPSocket::Connect().

Yeah, never seen that before. Are you sure CreateSocket is doing the right thing and checking for the correct return value? That is the only way I can see this causing issues.

Feb 23 2018, 2:59 PM · Restricted Project
clayborg added a comment to D39967: Disable breakpoints before writing memory and re-enable after..
In D39967#1017732, @ted wrote:

Traditionally, remote gdbservers handle memory operations that overlap a software breakpoint. Writes get the new value saved off, and reads get the breakpoint opcode replaced by the saved value.

Feb 23 2018, 1:20 PM
clayborg added a comment to D39967: Disable breakpoints before writing memory and re-enable after..

If any of our lldb_private::Process subclasses handle this already, then we need a new virtual function on lldb_private::Process like:

Feb 23 2018, 11:58 AM
clayborg added a comment to D39967: Disable breakpoints before writing memory and re-enable after..

So the question still remains: does this need to be done when debugging with debugserver or lldb-server? I can't remember if memory write works around breakpoint opcodes in debugserver and/or lldb-server. If it does, then this code is only needed for targets that don't work around software breakpoints.

Feb 23 2018, 11:52 AM
clayborg added inline comments to D39967: Disable breakpoints before writing memory and re-enable after..
Feb 23 2018, 7:27 AM
clayborg added a comment to D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line.

I am good with this as long as Jim and Pavel are.

Feb 23 2018, 7:19 AM
clayborg added a comment to D43647: Generate most of the target properties from a central specification..

I like Pavel's template syntax. It's not clear to me how to best implement the registering of the properties this way though. I think we want to avoid static intializers. Given how TargetProperties::TargetProperties() is implemented, I could imagine we could just make the properties members of the class, we just need some way to generate a for-each-property loop to handle the initialization. I'll think about this some more.

Feb 23 2018, 7:14 AM

Feb 22 2018

clayborg added inline comments to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
Feb 22 2018, 8:53 AM
clayborg added a comment to D43506: Fix a couple of more tests to not create files in the source tree.

Pavel: I would vote for the same thing, if the test runs fine, nuke the directory, if it fails for any reason, leave it around so we can do post mortem. Might be nice to copy the logs over into the build directory if things fail so it is really easy to just go into the directory that contains all failed test and each log file would be right where it needs to be. Right now, you go look in the log directory, then you need to go find the build directory and put the two together.

Feb 22 2018, 8:32 AM