User Details
- User Since
- Sep 23 2014, 10:11 AM (330 w, 3 d)
Thu, Jan 21
I think the only difference is the triple is supposed to be a complete triple and the arch can be just "arm64" and like you added, we let the currently selected platform help fill it out? I don't remember why this was added. Nothing in the source history?
Can I get one more approval so I can commit? This is blocking my unwind in GSYM patch that I would like to post next.
Wed, Jan 20
Tue, Jan 19
Another solution would be to not auto create a target if the "program" argument is missing from the launch config and document this in the "attachCommands" or "launchCommands"? Or is "program" a required launch config argument?
We have iterated on this patch and talked about the ramifications prior to posting this public patch. I will start with a little history:
Fri, Jan 15
Looks like harbormaster is finally clean. Any more issues or is this good to go?
LGTM. Pavel?
Thu, Jan 14
Wed, Jan 13
Fixed inline issues found by MaskRay and fixed the issue found by ASAN that was making the debian buildbot fail. Thanks Shoaib for the ASAN suggestion.
LGTM!
Mon, Jan 11
So I setup a CentOS 8 linux server today and ran the failing tests and they succeed. I am out of options here. I would like to submit this as I need it to get other work submitted. Is there any way to see how the debian x86 harbormaster build is setup? Maybe it is enabling some extra things?
I still think we should add a Process::ProcessDidExec() as mentioned in the inline comments.
Just one rename of an instance variable and this is good to go! Much cleaner without the directory and multiple files.
Fri, Jan 8
Can we still try to use just one file? Isn't this file both read and write? The flow would be:
- create 1 file using mkfifo(...) and pass it down as argument
- launcher lldb-vscode will write pid or error JSON back into the stream, and will start to wait for some data from the same fifo file
- normal lldb-vscode will write some JSON to fifo file to indicate it has attached after it has attached
- launcher lldb-vscode reads that it attached and then does the exec
Wed, Jan 6
And a test for current working directory
We should also add a test that makes sure that env vars set in the launch config are received in the program that runs in the terminal
So I like that we are using files in the file system for things, but we are using 3 of them and that seems excessive. See inlined comments and see if we can get down to using just one? Also, what is stopping us from being able to support windows here? Just mkfifo?
The really confusing thing is the error message for x64 debian > LLVM-Unit.DebugInfo/DWARF/_/DebugInfoDWARFTests::DWARFDebugFrame.UnwindTable_DW_CFA_expression:
LGTM. Nice and simple. We can do another patch for vAttachOrWait as it will be very simple modifications and very similar to this patch. I am find avoiding all of the polling interval and duration settings for now.
Tue, Jan 5
Found the test suite issue on
Another option would be to have lldb-server check for environment variables for default values for --waitfor-interval and --waitfor-duration. If they are set, they become the new default values. Of course a user can launch the lldb-server manually with the options set a different way and then attach with "process connect ..." if required. But this would provide an alternate way for users to control the polling and timeout. I would just hate for us to add these new flags and have some people not be able to debug because their GDB server gets launched (because they replaced the lldb-server) and doesn't support the flags.
Update to main branch from today.
Mon, Jan 4
LGTM
Patch looks good, just a question of what timeout to use by default and possible adding an option to lldb-server in case users want faster or slower polling.
Walter and I identified this at work, definitely want Jim to chime in on this.
Great stuff! See inline comments.
Dec 16 2020
Fixed remaining error message and used error handling build into the dump options.
Dec 15 2020
Dec 14 2020
Fix one last missed comment fix.
Dec 11 2020
Fixed more jhenderson comments. Combined many common cases that were possible after making OperandType to string function.
Dec 10 2020
Update to fix more of jhenderson's issues.
Dec 9 2020
Fixed issue found by jhenderson.
Dec 8 2020
Dec 7 2020
Update to be based off of the "main" branch.
One week ping.
So the UUID that ld64 writes out is the MD5 hash of only parts of the binary that do not have debug info paths in them. So you can build the same binary a /tmp/a or in ~/sources/a and get the exact same UUID for the resulting binary.
Dec 2 2020
Dec 1 2020
Nov 30 2020
So marking as needing changes because the size of the N_FUN entries must be correct. dsymutil creates an address map when remapping symbols with ranges and if the size of the N_FUN stabs entry is too large, it will cause major problems when linking the DWARF. We should also find a rock solid way to identify N_FUN entries if possible instead of relying on the section name being "__text".
Post holiday ping! I'd love to get this in so I can get unwind info into GSYM with a follow up patch.
Nov 18 2020
Thanks for chiming in Pavel. LGTM.
Nov 17 2020
So I think I get the gist of what you were trying to go for with this patch. I stopped inline comments after I got the idea.
LGTM. A few inline nits, but nothing major. I only question if "RecordedProcess" is the right name for the base class for a core file/trace file, but don't have much of a better name. Maybe SerializedProcess? SavedProcess? Don't we usually have Pavel in on these reviews?
Nov 16 2020
Fixed issues found by grimar.
Nov 15 2020
Fixed issues found by MaskRay.
Nov 13 2020
Updated the opcode value accessors to use errors instead of asserts. This helped find some issues with the usage of this class and made its use cleaner.
Nov 12 2020
Thanks for adding the extra tests!
LTGM!
Nov 11 2020
Walter and I discussed some issues offline and came to the conclusion we need to use CommandObjectProxy and get rid of CommandObjectDelegate. This will simplify the patch and use the pre-existing class that already does what we needed CommandObjectDelegate to do...
LGTM as long as local symbols always have names (see inlined comment).
Another possible way to do this would be to make a new stop reason in LLDB. debugserver currently can return a "reason" whose values are one of "trap", "breakpoint", "watchpoint", "exception", or "exec". What if we added "debug-trap"? Then back in LLDB we could add "eStopReasonDebugTrap" to "lldb::StopReason". Then the ABI plug-in or some arch specific plug-in could do something in response to this on stop, or on continue. x86 would know that is doesn't need to do anything because the PC is already past the trap instructions. But ARM64 it could advance the PC when the process goes to start running again so it skips the trap instruction. I would presonally like to see the program tell the truth when you hit a "brk #0xf000" and show the PC being on this instruction, and then continue from it correctly when you resume. Your solution is much easier though, so either way works.
Nov 10 2020
JSON crash logs are almost here??? I remember asking for those back when I was at Apple! Great news.
LGTM with the AAAAAAAA error string stuff included so we can get a nice string error message. Pavel, do you have anything?
Still wondering if we need to do caching in TracePluginCommandDelegate. How many times do we fetch the command object if we type "thread trace start"? I don't know how much performance is truly needed here at the expense of maintaining the caching code.
Nov 9 2020
So I am not sure I like the caching that we are doing in TracePluginCommandDelegate in the CommandObjectTraceStart class. See inline comments, but it might be better to cache the command object SP in lldb_private::Process and add accessors.
So down to just one question about the packet definition about if we should have an error code in JSON or if an error code even makes sense. Other than that this LGTM
Nov 5 2020
I am good too.
Nov 2 2020
Looks good, just fix typo I mentioned in inlined comments.
See inlined comments. Just some questions on wether we are going to reuse the other existing "tTrace*" packets, or if we are going to make new ones.
So I know Intel had gone and done some tracing support in an external kind of way before. This patch is preparing to use this functionality by asking for the supported trace types. I am not a fan of a global enumeration in lldb-enumerations.h controlling the tracing type and having to have GDB servers have to respond using these LLDB specific enumeration values. It would be nice if we can query the GDB server for the supported trace types and have them respond with a string for the tracing type. See my inlined comments for what was thinking. I also think that a GDB server should respond with a single kind of supported tracing, see inlined comments for more details.
Oct 27 2020
If anyone knows anyone that has a lot of experience with unwind information, please feel free to add them as reviewers.
Friendly ping
Oct 26 2020
Just remove the braces as suggested in inline comments. Pavel?