- User Since
- Sep 23 2014, 10:11 AM (178 w, 2 h)
Fri, Feb 16
I look forward to this being checked in
Looks good. Self describing and concise.
Thu, Feb 15
Ok, marking as needs changes.
Looks good if you are not concerned with my inline comment about using bitfields and endianess
Think if we want to cache the configuration in case we start using this a lot more in the test suite. Doesn't need to be done now.
Mon, Feb 12
Mon, Feb 5
Fri, Feb 2
You would check in the YAML code for the NetBSD ELF file so that the test case can make it into an ELF file, run the test, verify things, and then cleanup the created ELF file.
Probably take a ELF file that is NetBSD and obj2yaml it. The test would run yaml2obj on it and then test that things are recognized correctly via the SB interfaces (check triple is correct)?
Question: if you have a global entry point, is there ever any reason to stop at these? Is there anything you can debug in the global entry point? Does a global entry point always forward on to a local entry point? Does the local entry point always exists and is it the only thing that can be debugged? If so, then it sounds like we should mark the global entry point as a trampoline by setting the symbol type to lldb::eSymbolTypeTrampoline. Then all stepping logic will automatically just go through this code.
Makes sense for me, but make sure Pavel is ok with this as well before committing.
Many file systems are case insensitive so that is probably why it works on some computers.
Thu, Feb 1
Looks good! Not sure if we need the PPC specific comment in Architecture.h, but I won't hold up this patch for that. Might be nice to move the PPC comment into ArchitecturePPC64.h.
I am pretty sure that the ack that gets sent should happen in response to getting an ACK. Note that LLDB send an ACK and then waits for an ACK. This helps us see if anything is there. So yes, this code can always just respond to an unsolicited ACK with an ACK.
It would be nice if we can try and disable the redeclaration lookups from clang when in debugger mode. I can't remember if we already have such a flag in the compiler options. If we do, we should try to just disable this when in debug expression mode and see how the test suite fares. Unless we are able to lookup the name within the AST that is currently being built so we can re-find the exact same variable declaration and hand it back, disabling this seems to be the correct thing to do for debug expression. Comments?
Tue, Jan 30
Mon, Jan 29
There would be no spinning an instance, it would be call the API in python. No extra process, done in process.
See inlined comment
Fri, Jan 26
Added Jim Ingham so he can chime in on this and see if he agrees with my solution
This seems like functionality that should go into an architecture plug-in. The class is currently "lldb_private::Architecture". For an example see the ARM version:
Thu, Jan 25
Thanks for iterating on this. This will be great.
Wed, Jan 24
The funny thing is this is only 981 characters long... Not sure what the right cutoff would be...
This is part of the problem, but not the entire thing.. We had a mangled name:
This code is making debugging of large C++ apps so slow it is unusable...
Looks fine to me.
Tue, Jan 23
Mon, Jan 22
Sounds good then.
And yes, if the write memory can only write fewer byte than requested, it won't be an error if at least some bytes were written
I am not sure we can say for sure that a breakpoint intersected with the write here? I would rather just have an error saying something like "only %u of %u bytes in section %s were written". Extra credit for checking if there are overlapping breakpoints and appending "(try disabling all breakpoints and loading again)" to the error message.
This will need a test, preferable covering all of the cases:
- breakpoint right at start with extra data after
- breakpoint right at start with no extra data after
- breakpoint in middle of ranges with data before and after
- breakpoint at end of range with no data after
- multiple breakpoints tests with two breakpoints in a row with no data between with no data before first or after last
- multiple breakpoints tests with two breakpoints in a row with no data between with data before first and after last
- multiple breakpoints tests with two discontiguous breakpoints data between them with data before first and after last
I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed.
Jan 19 2018
Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around.
Jan 17 2018
Are we going to test each unix call that can fail with EINTR? Seems a bit overkill.
This is a very common unix thing.
My objection to the BeginWriteMemoryBatch and EndWriteMemoryBatch is users must know to emit these calls when they really just want to call process.WriteMemory() and not worry about how it is done. Much like writing to read only code when setting breakpoints, we don't require the user to know that they must check the memory permissions first, set permissions to be writable and then revert them back after writing to a memory location.
Looks fine to me. Wait for Pavel to give it the OK since he did more of the lldb-server testing stuff.
Looks nice. Only nit is we probably don't need the m_endian member variable. See inlined comment.
Very nice overall. See inlined comments. Big issues are:
- GDBRemoteCommunication::WriteEscapedBinary() is not needed as StreamGDBRemote::PutEscapedBytes() does this already
- Remove the batch start and end functions in process if possible and have ProcessGDBRemote::DoWriteMemory() just "do the right thing"
- Can we actually cache the results of the qXfer:memory-map for the entire process lifetime?
- Remove the new ProcessGDBRemote::GetMemoryMapRegion() and fold into GDBRemoteCommunicationClient::GetMemoryRegionInfo() as needed
Jan 12 2018
Very cool and close. It would be nice to function correctly without asserts, see inlined comment.
Jan 11 2018
Yeah, the threading was added later. It started out as single threaded and didn't have this problem.
We will need to pass the mutex down into any call that needs to let the mutex go. At least that is the only way I could see this working... But then all functions that do anything lazily will need this same treatment. Lot of trouble for the logging.
Jan 10 2018
And I do agree with Jim that we don't want to have to mangle the typename to see if it matches, that is too much work.
Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there any way that a DIE is marked up with an extra attribute when the asm name is explicitly set? It would be great to know this from the DWARF so we don't have to end up setting the ASM name for every single possibly affected DIE...
Sounds like finding a clang expert to clarify what Jim last asked for is the way forward. Do a source control "blame" command and see who worked on the code in the area of clang and maybe add them as reviewers so they can comment? I agree with Jim that this sounds like a good fix, but we should be careful. Is there no other way to detect this special "asm" name? No extra DWARF attributes?
Tough to call on this one. Yes the function is dumping simple stuff, but it is using m_arch, m_file and m_objname. It could cause crashes in multi-threaded environments. Is the deadlock caused by an A/B lock issue?
As long as:
Fix comment spacing as mentioned in inline comments and this is good to go.
Jan 9 2018
I like the overall direction this patch is taking, just a few fixes.
Looks fine. Set the breakpoint using the list of names and delete the breakpoint if you get no locations and this will be good to go.
Jan 8 2018
Detecting x86_64h might be tricky on different platforms. Two things I can think of: explicitly compile using x86_64h and when you try to run it it might fail. I believe that will work for Darwin, but not sure about other OSs (linux, Android). Mach-o has the x86_64h CPU type/subtyype, but I am not sure how ELF does this since they only really have the machine type and it isn't enough to represent x86_64h. The other way would be to start up the process (if it launches on all platforms) and look for a register by name that is only available if haswell support is available on the system. I believe all of our targets do correctly detect their AVX regs, so just stopping and looking for the register by name, and end the test as an expected failure or success if the register isn't available.
Jan 3 2018
Dec 21 2017
Thanks for all the revisions. Looks good.
Dec 20 2017
If you load a exe file that has a PDB file, people can currently run:
Looks good, thanks for the making the changes. This will make future tweaks to reading of file data in object files just a single change in ObjectFile.cpp.
Dec 19 2017
This is exposing a bug where if we have options like:
Environment vars might be set by using --env, so those need to go into "inferior_envp" first. If we are launching, we will copy only the host environment vars that haven't been already set manually (they don't already exist in the inferior_envp).
So we can also specify extra environment variable using the "--env" option with debugserver and your current fix will break that.
The existing code for the "--forward-env" does this:
See inlined comment. Quick fix and this will be good to go.
So the main question is what do we expect to happen by default. I kind of agree that if we launch the inferior directly when launching I would expect the environment to be passed along. Jason: since we always just launch debugserver for Xcode in the mode that doesn't expect environment variables to be passed along, I think changing the default behavior would be a good idea and it would only affect internal Apple customers. What do you think? We might need to add a "--no-forward-env" option in case anyone doesn't want this behavior just in case if we do switch the default.
Thanks for the explanation. Good to go.
Dec 18 2017
I am ok either way, but I do want to make sure Jason gets input before we do anything. He might be out for a few weeks over the break, so there might be a delay.
ok as long as we don't want to return const reference when returning Environment values in getters and setters.
We already have an option for this named "--forward-env". It might be better to fix this by adding the "--forward-env" argument to the debugserver launch during testing? When we launch through LLDB, it forwards all environment variables manually. Maybe we add a "--forward-env" option to lldb-server too? Not sure what the right thing to do here is. iOS, tvOS and watchOS won't be affected since we never launch directly and all env vars are manually set by LLDB. Jason, any comments on this one?
Dec 15 2017
Dec 14 2017
Move #include of "llvm/Object/Decompressor.h" into CPP file and this is good to go.