User Details
- User Since
- Sep 23 2014, 10:11 AM (186 w, 5 d)
Wed, Apr 18
LGTM
Very nice!
Tue, Apr 17
I am fine if we don't want to do the general solution now. LGTM if all is well in the test suite.
Mon, Apr 16
Looks good. I questions if we want PlaceholderModule to be available for all symbolicators/core dump plugins. See inlined comments.
Fri, Apr 13
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
Thu, Apr 12
Fixed the breakpoint verification in lldbutil to make sure the file name ends with the right thing
Wed, Apr 11
Looks good to me, but probably need someone that works on LLVM fulltime to give the final ok
Tue, Apr 10
Mon, Apr 9
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.
Wed, Apr 4
ping
Mon, Apr 2
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.
Thu, Mar 29
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.
Wed, Mar 28
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.
Tue, Mar 27
Mar 23 2018
Mar 22 2018
I would like to not lose any performance if possible.
Mar 21 2018
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.
Mar 20 2018
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
Mar 19 2018
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 16 2018
Much better. Make sure Jim is ok with this as I am ok with it if he is.
Mar 14 2018
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.
Mar 13 2018
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 12 2018
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.
Mar 8 2018
Mar 6 2018
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.
Mar 5 2018
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.
Mar 2 2018
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.
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
Mar 1 2018
Ping for my last comment
very nice!
Feb 27 2018
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.
Feb 26 2018
Address Adrian's comments.
clang format
fixed comments
removed white space
fixed functionalities/breakpoint/inlined_breakpoints/TestInlinedBreakpoints.py failures
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.
Updated to top of tree sources and verified it passes all tests on linux.
Feb 23 2018
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.
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.
Feb 22 2018
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.