- User Since
- Sep 23 2014, 10:11 AM (168 w, 1 d)
This was left over from before we mmap'ed the entire object file into memory. Removing it is fine as the backing DataBufferSP for the object file will be mmaped or not depending on where the file was loaded from and if the section isn't compressed, we will just hand out a shared slice of the object file data.
No worries then. No need to make a new enum if this is just two places and they aren't all setting the same flags.
Sounds good,. So the solution will be:
- Section::GetFileSize() will return the size in bytes of the section data as it appears in the file
- Section::GetByteSize() will return the size in bytes for when this section is loaded into process memory (we might consider renaming this to "GetLoadSize()" then?)
- Getting section data might return more data that GetByteSize() if it needs to be decompressed and decompression will happen automatically
Mon, Dec 11
We should have a dedicated API that actually searches for types using a lldb_private::RegularExpression. Why do we even try to create a regular expression in SymbolFilePDB::FindTypes()? Was it used in testing? No API in LLDB currently expects this and all other API in LLDB has a separate function call that uses lldb_private::RegularExpression. We can add one if needed to the lldb_private::SymbolFile API. I don't like API that magically tries to look at something and tries to compile a regular expression on each invocation. Can we change this?
I think GetFileSize() should remain the number of bytes of the section on disk and we should add new API if we need to figure out the decompressed size. Or maybe when we get bytes from a compressed section we are expected to always just get the raw bytes, then we check of the section is compressed, and if so, then we call another API on ObjectFile to decompress the data. So I would prefer GetFileSize() to return the file size of the section size in the file and not the decompressed size. Is there a way to make this work?
Is the lldb_private::Process we have an exit status in the class itself. The first person to set the exit status wins and no one can set it twice. Doesn't look like what we are doing here. I am not able to tell what actually fixes things here?
Anything that launches a process should use the ProcessLaunchInfo. Nice patch.
Tue, Dec 5
Looks fine to me, but a test would be nice as Zach suggested. I am guessing before we made way too many sections. Should be easy to conjure up a test and verify the same sections is used.
I don't believe I am the right guy to review this.
Seems wrong to remove the const on structs that don't need to change in order to make the write happen. Can't we just quiet the warnings with a const_cast inside the function call?
One really nice way we can get a lot of testing of the DWARF to clang::ASTContext conversion is to:
1 - compile a source file with clang and dumps the AST for a specific type as the compiler knows it
2 - using the .o file with debug info from step 1, load it into LLDB and have the DWARF to clang::ASTContext conversion happen and dump the AST info for the type
3 - compare the two and look for differences
Wed, Nov 29
Please undo all renaming from offset to file_offset. The "offset" you get from a DIE should always be the absolute .debug_info offset. No need to say file_offset. Patch will be easier to read after spurious renames are removed.
Take a look how the LLVM DWARF parser handles its units. It makes a DWARFUnit base class that the compile unit inherits from. That can then be used for type units and compile units.
Why would we text scrape when we can test the API?
Few nits, but nothing that would hold up the patch. Looks good.
Tue, Nov 28
ok, this is fine then. Just need to test somehow.
No one is relying on the 16 bytes of zeroes that I know of. UUID::IsValid() is the test that everyone uses to tell if the UUID is valid or not. I still prefer to just set the length to zero as this does allow us to have a UUID of all zeroes if needed.
You can abandon https://reviews.llvm.org/D40537 and just do the fixes I suggested in that change here: init m_num_uuid_bytes to zero in constructor and in Clear() and change UUID::IsValid() to just return "m_num_uuid_bytes > 0". I agree we should relax the UUID length.
A better solution would be to initialize UUID::m_num_uuid_bytes with zero and only set it to a valid value if we set bytes into it. Then UUID::IsValid() becomes easy:
I am not sure I follow this patch. We are adding a FileSpec whose path is just the basename of the current ELF file? What do we do with that? Do we look in certain directories to try and find this file? How this this basename added to the list end up getting found in the end?
Mon, Nov 27
ok, sounds like the abi_tag stuff is meant to just affect the mangled named with no code gen implications. Jim should ok this, but I am ok if he is.
DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() seems broken, see inlined comments.
See inline comments.
Seems like it would be cleaner to leave DWARFDebugInfoEntry and DWARFDie alone and just make clients deal with accepting DW_TAG_partial_unit when and where they need to. I don't like the idea of not telling the truth.
Just move the eSectionTypeDWARFGNUDebugAltLink enumerator to the end of the enum and this is good to go.
It would be much easier to read this patch if there were no renames from "*offset" to "*file_offset". Can we remove these extra renames where it isn't needed?
Won't hold up the checkin for the cleaner while loop, but feel free to fix that and checkin if it works.
This change should be fine. Watch buildbots for changes. If they were clean they should stay clean, if they had some fails, then only those fails should continue failing.
Jim will need to ok this. A few concerns follow. Most of the time when we don't get the DWARF -> AST conversion right, it can mean that we might code gen incorrect code. Is there not enough information for the GNU abi_tag in the DWARF to actually re-create these classes correctly in the AST? My first inclination would be to fix that if possible. If there is info missing from the DWARF such that we can't re-create the AST correctly, then it seems that this approach will help. Can you clarify what the GNU abi_tag stuff does? Will it change how code would be generated? If we have things in the AST that aren't correct, but have been fixed by correcting the "asm" tag, will we JIT up bad code?
Wed, Nov 22
Tue, Nov 21
Mon, Nov 20
I can add a test that does this if needed.
Sun, Nov 19
Feel free to remove any unused code. No need for review on dead code removal. So just remove the code, don't add #if 0
See inline comments.
Good change in the header file. I am not sure I like the destruct this object in place and replace with new version... If this is commonly done and acceptable form of C++ I would be ok with it, but I agree with Pavel, it seems a little bit off the books.
Fri, Nov 17
Thu, Nov 16
This change also stops extra calls to the Die.find() when a new Die wasn't fetched using Die.getAttributeValueAsReferencedDie as it was always calling Die.Find() even with the old Die...
Wed, Nov 15
I had suggested in D38142 that we have ObjectFileELF check the file type of the file and only map with write abilities if the ELF file is an object file since that is the only time we need relocations. If we can pass a flag through to indicate how to map, would that provide any additional performance improvements?
Tue, Nov 14
The "--set-pc-to-entry" change is fine. No need for review when removing unused variables or clang formatting, but it would be nice to do those commits separately.
Please clang format and check in without any changes to the functionality and then make a patch that only has your functional changes.
Before working on a file for a diff you will submit, clang format it first, and then check it in. No need for review on clang format only changes.
I believe we normally just clang format the changes. This is done with:
Nov 13 2017
First change looks good. Second one we can probably avoid doing anything in Value::AppendDataToHostBuffer and return 0. No need to copy data over itself.
Nov 10 2017
I just saw Jim's email for this, Please comment in this bug so we can all see the info in one location. Accepting pending the outcome of the discussion that Jim start in email that I am pasting below:
Nov 9 2017
I see your point. But if we do ask a template parameter for its type, I would like to be able to get it's type somehow. Seems wrong to leave this out. I know it doesn't mirror clang, but we should do the right thing here. At least at the SB API layer. I am fine with leaving the new TypeSystem calls as you added them.
Just a few changes.
- I would like the see the SBType returned for the integer template types as it is what I would expect to happen.
- we should add default implementations for stuff in TypeSystem and not require all languages to override everything. I know this isn't the way things were done in the past, but we should fix that to avoid bloat in all TypeSystem subclasses.
If no tests currently fail due to this, then we need to add some.
Looks fine to me as long as you got a clean test suite run. Be sure to keep an eye on the buildbots after this goes in.
Nov 8 2017
Nov 7 2017
If it isn't expensive to copy, then we should probably just return by value.
Looks fine. Seems like you should use your a const reference in a few places and this will be good to go?
Nov 6 2017
Are we planning on getting shared libraries working on windows? Or this is just an expression parser with shared libraries bug? Be nice to file a bug and mention it if it is something we are planning on fixing.
Do we want to add an option to our build system to try LLD where it is supported? Doesn't need to be part of this patch, but it would be great to be able to try it out on ELF based systems.
Nov 2 2017
Another option would be to add a URL version of these functions:
Nov 1 2017
No objections from me as I don't use these categories.
And yes, the memory savings are quite large as well when sharing directories.
The main reason for two strings is for searching efficiency. Most people don't set breakpoints using full paths, they give the basename:
(lldb) b main.c:12
I was unhappy when we went over two pointers for a FileSpec when m_syntax was added due to the extra size. Anything we can do to make this smaller would be great, so the type on the enum would work, but as you say the alignment will nullify that. The two ConstString members contain a pointer which isn't aligned so we can't use any bits from the low end of the pointer. Are there any classes that take advantage of high bits in pointers? Most if not all OS's don't use the entire 64 bit address space... It would be great to get lldb_private::FileSpec down to just 2 pointers again.
Oct 31 2017
A few general things: don't modify FileSpec, we have many of these objects and we can't increase their size without directly affecting memory usage. FileSpec objects represent one file on disk, not multiple. We should make a new class that contains a FileSpec and a regex, but not put it into FileSpec.
Oct 26 2017
Each lldb.SBValue has accessors for the stuff in an execution context:
The test suite can be run remotely if ds2 supports the "lldb-server platform" packets. I'll be happy to help you get this going Stephane. Ping me internally if interested.
Testing would include a test that dynamically loads a shared library and sets a breakpoint it in. We have these tests, but I am guessing they are disabled on windows probably because they might use "dlopen(...)" which is probably not available on windows? I know we have attach tests as well. Many tests are disabled on windows, but probably shouldn't be. So I would start looking for those tests. The code will need to be window-ized with #ifdefs and such, but it shouldn't be hard. The shared libraries tests will dynamically open a shared library and then verify we can hit a breakpoint in that shared library (a hint that the DYLD worked). So these tests don't need to be windows specific.