User Details
- User Since
- Sep 23 2014, 10:11 AM (443 w, 2 d)
Today
Must be relocations causing the stuff to not match up to the llvm-dwarfdump output.
FYI: This might be because we are using a .o file and relocations are being applied internally!?
Fix the error message to check for the exact error message and this is good to go.
Yesterday
Is there no test case catching this?
Fri, Mar 17
LGTM
Mon, Mar 13
Fri, Mar 10
Looks good to me.
Thu, Mar 9
I would suggest checking the google stadia patch for the L1 and L2 caches:
One other optimization we can do is if we read from the process memory and it returns that is read zero bytes, right now we add the range we were trying to read into the m_invalid_ranges member variable. So lets say we were trying to read the range [0x1000-0x2000) on a mac. We will fail to read this due to __PAGEZERO, but I believe we currently add this range to the m_invalid_ranges. But we could ask about this memory region from the process and realize we can actually add [0x0-0x100000000) to the m_invalid_ranges. That might help avoid multiple bad reads from a large area that isn't mapped.
Wed, Mar 8
There was a fix that was never submitted for Google Stadia for the memory cache here:
Thanks for the changes! LGTM. Just one missed EXPECT_THAT_EXPECTED, but accepted.
Tue, Mar 7
Looks good. Only questions is if we can add a C++ unit test for this file and test the new Symbol::FromJSON() and test all error conditions.
Mon, Mar 6
This is a resubmission of https://reviews.llvm.org/D143793. I added fixes for the buildbots
Add fixes for buildbots where some compilers don't need a std::move(), but some do.
Failed to "arc diff" to update an older patch.
If these files can be used as the only source of information (without a stripped executable), we really should include a serialized SectionList in the JSON that can be loaded into ObjectFileJSON. This would be very useful for easily creating unit tests.
Just change HandleDestroryCallback to a member function and this is good to go.
Thu, Mar 2
I like this idea of this, but I would like to see this be a bit more complete. One idea is to remove the ObjectFileJSON::Symbol definition and just use lldb_private::Symbol objects and allow these objects to construct encode and decode from JSON. Then we have the ability to re-create any symbols we need in full fidelity. But that not be the goal in your case, but I don't think it would hurt to use the lldb_private::Symbol as the object we serialize and deserialize from JSON as ObjectFileJSON::Symbol just can't reproduce the full depth of the symbols we want.
Thu, Feb 23
Sorry for delay, I have been on vacation for the last two weeks. I will be back next Monday and get to this soon. Feel free to ping again next week if I don't get to it!
Address review comments:
- createSegment now can return an error if the segment size is too small
- added code to detect segment sizes that are too small to contain even a single function info
- added test code to test for small segment sizes
Feb 10 2023
If anyone doesn't know the GSYM code that I added as reviewers, feel free to resign. I just did some blame lines on GSYM code to search for people that have made some fixes to the code since there aren't a lot of people that know the GSYM code!
Feb 9 2023
Feb 8 2023
Looks good to me. Pavel?
Feb 7 2023
Very clean patch now, just a few nits about asserts!
If I am reading the code for this patch correctly, we need the BatonSP stuff because we have differing callback bytes for public vs private APIs. If we switch to using a common callback type of:
typedef void (*DebuggerDestroyCallback)(lldb::user_id_t &debugger_id, void *baton);
(we can define this both internally and leave the current "SBDebuggerDestroyCallback" in SBDefines.h alone), then we can avoid having to add any of the BatonSP stuff and just store a "void * m_destroy_callback_baton;" in Debugger.h.
Feb 6 2023
Changes look fine to me. I would like someone that specializes in the expression parser to give the final ok though.
Feb 2 2023
Jan 31 2023
- My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.
Jan 30 2023
So looks like this function needs to be fixed:
llvm::Optional<DIERef> DWARFBaseDIE::GetDIERef() const { if (!IsValid()) return llvm::None;
Looks good to me. Pavel?
Jan 27 2023
Updated to latest sources and fixed issues from a recently added test for simple template types that showed some issues in type lookup. We previously would allow results from a lookup like "Foo" to match a type that was in the accelerator ables as "Foo", but for a type that was really "Foo<int>". This latest addition to the patch fixes these lookups from incorrectly succeeding.
Much better, thanks for making sure we don't end up indexing real skeleton units as they cause problems if they do get indexed.
Jan 26 2023
If the only time clang is not emitting .debug_aranges correctly is when we use -g1, then fixing that issue (by either fixing or removing .debug_arnages) would be fine by me. That being said, we have a clang that has been producing bad .debug_aranges, so unless we fix LLDB it won't fix the issue for binaries already out in the wild.
As long as there are no regressions in the test suite this looks good to me
We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:
uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file virtual uint32_t GetFileIndex() { return m_file_index; }
Jan 19 2023
Gotcha, makes sense now. Thanks for the extra info. We do need to make sure the m_symbol_file values match and return an empty TypeSP if they don't. Once that is done, this is good to go.
We don't allow types to be copied into another SymbolFile. There is a strict rule in LLDB that SymbolFile objects only create types from their own data. There are many reasons we don't want this:
- Type objects have a m_symbol_file which points to the SymbolFile that created it. This pointer can and will go bad if the original symbol file is freed.
- Type objects have a symbol context member variable ( "SymbolContextScope *m_context;"), same issue as above
- Type objects might have a valid encoding type member variable ("Type *m_encoding_type;") that is non NULL that points to a type that is owned by the original symbol file
- Type objects might have a encoding User ID and encoding type that point to the original symbol file ( lldb::user_id_t m_encoding_uid = LLDB_INVALID_UID; EncodingDataType m_encoding_uid_type = eEncodingInvalid;) and the user_id_t only makes sense in the original encoding type.
Change looks good. It would be nice to test this if possible.
Update: There are some new failures after someone recently added new tests that involve simple template types. I need to fix my new lookup routine to do the right thing in light of these new tests. The main issue is currently if you have a "foo<int>", the accelerator tables and DWARF contain "foo" only. This causes the expression parser to be able to lookup "foo" as it is parsing an expression that has "auto a = foo<int>()" as it will actually return a specialized "foo<int>" as the result since the accelerator table will point to the "foo<int>" type. We need to outlaw these raw lookups from working. Worse yet, if there are multiple instantiations of "foo<T>", it will return all of them and cause LLDB to crash later in some AST copying code.
Jan 17 2023
Jan 16 2023
Looks fine to me as is seems performant. I will let owners of the DWARF linker approve for good.
Jan 13 2023
Sorry for the delay!
New changes looks great, thanks for tackling this. Just a few comments that need to be updated and this will be good to go as long as the test suite passes just fine!
Jan 11 2023
Looks good!
I mean we can not just subtract something, any number, from any address unless we have fixed size opcodes. If we do this for x86, you can get complete garbage with no hope of ever getting back on track and this disassembly just won't make sense at all and will be useless. I thought my x86 example spelled out why it is bad to backup. If we start disassembling in the middle of an opcode, we can attempt to disassemble immediate values that are encoded into the middle of an opcode. Since x86 instructions can be 1 byte to 15 bytes, we might disassembly garbage and never align up to real opcodes.
There have also been some bugs in .debug_aranges and some folks want to get rid of .debug_aranges all together and rely only upon the DW_TAG_compile_unit's DW_AT_ranges. Tons of details in this patch:
SymbolVendorELF.cpp is using this as well:
dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);
They must have copied and pasted some code. So not sure this is safe enough as is to implement. There are also a few places that try to check for a dSYM file using this kind of method, might be nice to add a method, that is done right, that detects if we really have a dSYM or not in ObjectFile.h/.cpp. Wasm also sets the file type to debug info and so does breakpad.
Storing raw pointers in DieToTypePtr may cause use-after-frees to occur, since there are no guarantees that the shared pointers that owns the underlying pointer to the type are kept around as long as the map. Change the map to store a shared pointer instead.
Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:
TypeSP type_sp(... /* create type here*/ ...); dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list