LGTM

Very nice!

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

In D45703#1069921, @labath wrote:

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.

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

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

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

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

In D45554#1066730, @eugene wrote:

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.

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

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

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.

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

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.

In D45170#1057067, @aprantl wrote:

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.

ping

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

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:

Sorry for the delay.

We probably need a test then.

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

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

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.

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.

In D44437#1036993, @lbianc wrote:

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.

I would like to not lose any performance if possible.

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.

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

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

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.

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.

Great stuff, thanks for doing this BTW

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.

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

Committed revision 327600.

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.

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

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:

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.

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

In D32167#1026779, @davide wrote:

In D32167#1026762, @jankratochvil wrote:

In D32167#1024546, @labath wrote:

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.

Test?

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?

Change looks fine to me.

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

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

In D42955#1026296, @labath wrote:

In D42955#1026216, @clayborg wrote:

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.

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.

In D42955#1026197, @labath wrote:

In D42955#1026175, @clayborg wrote:

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

In D42955#1026116, @labath wrote:

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.

In D42955#1026061, @clayborg wrote:

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.

Will we have the same problem with coff_symbol?

Add test as well.

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

Ping for my last comment

very nice!

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?

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.

In D32167#1020032, @labath wrote:

In D32167#1019702, @clayborg wrote:

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.

Address Adrian's comments.

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

In D32167#1019701, @labath wrote:

In D32167#1019635, @jingham wrote:

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

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.

In D32167#1019504, @clayborg wrote:

In D32167#1019467, @labath wrote:

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.

In D32167#1019467, @labath wrote:

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.

In D32167#1019399, @davide wrote:

This commit has no tests. It should have many.

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

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.

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.

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

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.

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

In D43647#1016882, @aprantl wrote:

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.

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.