Page MenuHomePhabricator

clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (330 w, 3 d)

Recent Activity

Thu, Jan 21

clayborg added a comment to D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH.

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?

Thu, Jan 21, 2:27 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

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.

Thu, Jan 21, 9:04 AM · Restricted Project

Wed, Jan 20

clayborg added a comment to D94997: [lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided..

D92164 was intended for fixing the "settings set" issue, however, it revealed some deadlocks and data races, and had to be reverted temporarily. Currently, I'm working on those multithreading issues and will submit a patch as soon as possible.

Wed, Jan 20, 2:33 PM · Restricted Project
clayborg added a comment to D94997: [lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided..

Thanks for review and your input, @clayborg

So I agree we need to fix the "settings set" issue as it surfaces a quirk in the order and number of targets we create. The main questions is if we care that we don't auto create a target when "launchCommands" are used and what the right solution is for lldb-vscode going forward.

Please correct me if I'm wrong, but my understanding is: from lldb-vscode launch.json configuration we generally should create a single target/process which user will debug.

Wed, Jan 20, 2:31 PM · Restricted Project

Tue, Jan 19

clayborg added a comment to D94997: [lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided..

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?

Tue, Jan 19, 2:35 PM · Restricted Project
clayborg requested changes to D94997: [lldb][lldb-vscode] Updated implementation of 'launch' and 'attach' requests to not create auxiliary target in case "launchCommands" and "attachCommands" are provided..

We have iterated on this patch and talked about the ramifications prior to posting this public patch. I will start with a little history:

Tue, Jan 19, 2:00 PM · Restricted Project

Fri, Jan 15

clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Looks like harbormaster is finally clean. Any more issues or is this good to go?

Fri, Jan 15, 11:41 AM · Restricted Project
clayborg accepted D94672: Implement vAttachOrWait.

LGTM. Pavel?

Fri, Jan 15, 11:29 AM · Restricted Project

Thu, Jan 14

clayborg added inline comments to D94672: Implement vAttachOrWait.
Thu, Jan 14, 12:33 PM · Restricted Project

Wed, Jan 13

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

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.

Wed, Jan 13, 11:29 PM · Restricted Project
clayborg accepted D94629: [LLDB] MinidumpParser: Prefer executable module even at higher address.

LGTM!

Wed, Jan 13, 2:22 PM · Restricted Project

Mon, Jan 11

clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

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?

Mon, Jan 11, 6:29 PM · Restricted Project
clayborg added inline comments to D93874: [process] fix exec support on Linux.
Mon, Jan 11, 3:37 PM · Restricted Project
clayborg added a comment to D93874: [process] fix exec support on Linux.

I still think we should add a Process::ProcessDidExec() as mentioned in the inline comments.

Mon, Jan 11, 3:00 PM · Restricted Project
clayborg accepted D93951: [vscode] Improve runInTerminal and support linux.

Just one rename of an instance variable and this is good to go! Much cleaner without the directory and multiple files.

Mon, Jan 11, 2:53 PM · Restricted Project

Fri, Jan 8

clayborg added a comment to D93951: [vscode] Improve runInTerminal and support linux.

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
Fri, Jan 8, 9:51 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

looks good to me too. When you get around to the wait times & intervals I'd argue for not doing that as a GDBRemote specific addition, as Greg was suggesting above. There's nothing gdb-remote specific about how long you want the debug agent to wait around for some process to show up.

Fri, Jan 8, 9:08 PM · Restricted Project

Wed, Jan 6

clayborg added a comment to D93951: [vscode] Improve runInTerminal and support linux.

And a test for current working directory

Wed, Jan 6, 5:35 PM · Restricted Project
clayborg added a comment to D93951: [vscode] Improve runInTerminal and support linux.

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

Wed, Jan 6, 5:35 PM · Restricted Project
clayborg requested changes to D93951: [vscode] Improve runInTerminal and support linux.

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?

Wed, Jan 6, 5:34 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

The really confusing thing is the error message for x64 debian > LLVM-Unit.DebugInfo/DWARF/_/DebugInfoDWARFTests::DWARFDebugFrame.UnwindTable_DW_CFA_expression:

Wed, Jan 6, 4:36 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Phabricator is still saying there are unit tests failing. Are these up-to-date reports, or is Phabricator stale?

Wed, Jan 6, 4:13 PM · Restricted Project
clayborg accepted D93895: Implement vAttachWait in lldb-server.

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.

Wed, Jan 6, 4:11 PM · Restricted Project

Tue, Jan 5

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Found the test suite issue on

Tue, Jan 5, 9:29 PM · Restricted Project
clayborg added a reviewer for D93895: Implement vAttachWait in lldb-server: jasonmolenda.
Tue, Jan 5, 2:46 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

I think I get your point. If we pass the extra options in the packet, the validation on older lldb-server versions will reject the message.

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'll defer to you guys, since you're a lot more knowledgeable on lldb than I am :)

I do have another possible solution we could consider: can we create a new message, similar to qVAttachOrWaitSupported, that queries if these extra options are supported or not. Something like qVAttachWaitExtraOptionsSupported. We might even consider returning something akin to a "version number", so if we support more options in the future we can bump up the number.

Let me know what you think (again, I'm fine doing it either way).

Tue, Jan 5, 2:44 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

Here lies the problem that I mentioned above. I would like to avoid having to launch lldb-server with any arguments so that we continue to work with older lldb-servers.

So maybe we just rely on defaults for now and avoid having to add any new arguments to "process attach" and to the launching of the lldb-server? Let me know your thoughts.

Sorry, I don't think I fully understand this point. Do you mean we shouldn't add 'waitfor-interval' and 'waitfor-duration' since older lldb-server versions might not recognize the packet format?

Tue, Jan 5, 2:29 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

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.

Tue, Jan 5, 2:09 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

So it might be nice to add support for vAttachOrWait, along with the query packet, in this patch if you have the time?

Hi @clayborg, I saw that implementing vAttachOrWait was already 90% done, so I did just that :) The current patch as-is supports vAttachOrWait. My question was more on the lines of if keeping the query for vAttachOrWait made sense now that it's implemented on Linux. Are there be other targets where it isn't implemented?

Tue, Jan 5, 2:06 PM · Restricted Project
clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Update to main branch from today.

Tue, Jan 5, 1:53 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

Question: why does lldb queries if vAttachOrWait is supported, but not vAttachWait? Does it make sense to keep this query? Or should I remove it?

Tue, Jan 5, 12:05 PM · Restricted Project

Mon, Jan 4

clayborg accepted D94033: [lldb-vscode] improve modules request.

LGTM

Mon, Jan 4, 3:04 PM · Restricted Project
clayborg added a comment to D93895: Implement vAttachWait in lldb-server.

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.

Mon, Jan 4, 2:19 PM · Restricted Project
clayborg added a comment to D93874: [process] fix exec support on Linux.

Walter and I identified this at work, definitely want Jim to chime in on this.

Mon, Jan 4, 1:03 PM · Restricted Project
clayborg requested changes to D93951: [vscode] Improve runInTerminal and support linux.

Great stuff! See inline comments.

Mon, Jan 4, 12:49 PM · Restricted Project

Dec 16 2020

clayborg added inline comments to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..
Dec 16 2020, 3:30 PM · Restricted Project
clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fixed remaining error message and used error handling build into the dump options.

Dec 16 2020, 3:29 PM · Restricted Project

Dec 15 2020

clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Looks fine aside from the two outstanding comments re. error messages.

Dec 15 2020, 11:14 AM · Restricted Project
clayborg added inline comments to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..
Dec 15 2020, 11:12 AM · Restricted Project

Dec 14 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fix one last missed comment fix.

Dec 14 2020, 11:34 PM · Restricted Project

Dec 11 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fixed more jhenderson comments. Combined many common cases that were possible after making OperandType to string function.

Dec 11 2020, 2:13 PM · Restricted Project

Dec 10 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Update to fix more of jhenderson's issues.

Dec 10 2020, 7:03 PM · Restricted Project

Dec 9 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fixed issue found by jhenderson.

Dec 9 2020, 5:35 PM · Restricted Project
clayborg accepted D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.
Dec 9 2020, 4:18 PM · Restricted Project
clayborg added a comment to D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.

The point is if you produce a binary with ld64 with or without local symbols, with or without debug info (STABS debug map) -- at link time -- it will produce the same UUID.

I understand that part. I'm wondering what the use case is where this is important. In my experience, people build with debug info and then strip (which gives you binaries with and without debug info and with matching UUID even with the elf scheme), instead of building once with debug info and once without. So my question above is: Do you do two separate builds, one with debug info and one without, instead of using strip? If so, why?

Dec 9 2020, 4:17 PM · Restricted Project
clayborg added a comment to D92537: [lld-macho] Implement -object_path_lto.

If I understood @clayborg's earlier comments, LLDB will match nonzero STABS values with the mtime, so a content hash would result in a mismatch. But a zero STABS value will match any file (at the given path), so I guess we could have a flag to enable that behavior in order to get reproducible builds?

Dec 9 2020, 4:07 PM · Restricted Project
clayborg added a comment to D92537: [lld-macho] Implement -object_path_lto.

FWIW we're very interested in having deterministic build outputs (in the senses of https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html), and putting file mtimes in the linker output defeats that. Could we put content hashes in the mtime instead? (We could make the compiler produce a content hash of its .o output at compile time so that we could just read the content off out the .o file instead of computing it if computing it is a perf issue.)

Dec 9 2020, 3:43 PM · Restricted Project

Dec 8 2020

clayborg added a comment to D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.

So we are hashing only what ld64 is hashing? Do the RFC 4122 changes make us differ from ld64 if we don't switch hashing algorithms?

No, we're hashing the object paths in the debug info too. But that should be addressed in a separate diff...

Dec 8 2020, 2:35 PM · Restricted Project
clayborg added a comment to D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.

Is stripped builds and non-stripped builds having the same UUID something that's useful in practice? I would've expected that everyone builds a debug binary with symbols, and then strips it after building. What's the use case for links with and without debug info and wanting the same UUID?

We have cases where the ELF build ID changes on us for binaries when we have stripped debug info. Debuggers really want to match a stripped binary to a non stripped binary.

This is where I don't follow. My mental model is:

  1. Build with symbols, upload that binary to some symbol server
  2. Run strip
  3. Ship the stripped binary to production (users, data centers, whatever)

strip doesn't change the UUID, it leaves it in place unchanged (...right?). So stripped and non-stripped binary always have a matching UUID in this case. Having a strip-invariant hash algorithm matters if you do something like:

  1. Build with symbols, upload that binary to some symbol server
  2. Independently build the same binary at the same revision, but without symbols
  3. Ship the binary from 2 to production

In that case, if you want the binaries in 1 and 2 to have the same UUID you do need a hashing algorithm like the one you describe. Is the second workflow what you're doing? Or is there something else I'm missing?

Dec 8 2020, 2:32 PM · Restricted Project
clayborg accepted D92537: [lld-macho] Implement -object_path_lto.
Dec 8 2020, 2:15 PM · Restricted Project

Dec 7 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Update to be based off of the "main" branch.

Dec 7 2020, 4:04 PM · Restricted Project
clayborg added a comment to D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.

Thanks! There's two mostly independent questions here.

  1. Choice of hash function. The patch changes the hash function but doesn't modify what parts of the binary get hashed:

So if we change the UUID to be unique on each build, we can probably guarantee that Apple won't ever use lld for their builds. Not sure how important this is. This also means that if a UUID is generated by ld64 for a stripped or non-stripped binary, it will end up being the same, which is nice.

That's not what this patch is doing. It changes only which hash function we use, not which ranges of the binary we hash.

Dec 7 2020, 4:02 PM · Restricted Project
clayborg added a comment to D92537: [lld-macho] Implement -object_path_lto.

@clayborg was hoping to get your opinion on my approach to modtime in this diff

Dec 7 2020, 3:09 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

One week ping.

Dec 7 2020, 1:47 PM · Restricted Project
clayborg added a comment to D92736: [lld/mac] Use xxhash instead of MD5 for computing the UUID.

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 7 2020, 1:41 PM · Restricted Project

Dec 2 2020

clayborg added inline comments to D92452: [lldb] Treat remote macOS debugging like any other remote darwin platform.
Dec 2 2020, 4:17 PM · Restricted Project

Dec 1 2020

clayborg accepted D89257: [lld-macho] Emit STABS symbols for debugging, and drop debug sections.
Dec 1 2020, 2:38 PM · Restricted Project
clayborg accepted D92366: [lld-macho] Flesh out STABS implementation.
Dec 1 2020, 2:36 PM · Restricted Project
clayborg accepted D92430: [lld-macho] Add isCodeSection().
Dec 1 2020, 2:36 PM · Restricted Project

Nov 30 2020

clayborg requested changes to D89257: [lld-macho] Emit STABS symbols for debugging, and drop debug sections.

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".

Nov 30 2020, 11:20 PM · Restricted Project
clayborg added inline comments to D92366: [lld-macho] Flesh out STABS implementation.
Nov 30 2020, 11:05 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Post holiday ping! I'd love to get this in so I can get unwind info into GSYM with a follow up patch.

Nov 30 2020, 2:07 PM · Restricted Project
clayborg accepted D86292: [LLDB][RISCV] Add RISC-V ArchSpec and rv32/rv64 variant detection.
Nov 30 2020, 2:02 PM · Restricted Project

Nov 18 2020

clayborg accepted D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.

Thanks for chiming in Pavel. LGTM.

Nov 18 2020, 1:53 PM · Restricted Project

Nov 17 2020

clayborg requested changes to D91679: [trace][intel-pt] Implement trace start and trace stop.

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.

Nov 17 2020, 9:31 PM · Restricted Project
clayborg added a comment to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.

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 17 2020, 9:01 PM · Restricted Project

Nov 16 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fixed issues found by grimar.

Nov 16 2020, 1:27 PM · Restricted Project

Nov 15 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Fixed issues found by MaskRay.

Nov 15 2020, 12:44 PM · Restricted Project
clayborg added inline comments to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..
Nov 15 2020, 12:44 PM · Restricted Project

Nov 13 2020

clayborg updated the diff for D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

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 13 2020, 12:03 PM · Restricted Project

Nov 12 2020

clayborg added inline comments to D89285: [lld-macho] Emit local symbols in symtab; record metadata in LC_DYSYMTAB.
Nov 12 2020, 2:31 PM · Restricted Project
clayborg accepted D91130: [crashlog] Implement parser for JSON encoded crashlogs.

Thanks for adding the extra tests!

Nov 12 2020, 2:31 PM · Restricted Project
clayborg accepted D91318: [lld-macho] Add archive name and file modtime to STABS output.

LTGM!

Nov 12 2020, 2:28 PM · Restricted Project

Nov 11 2020

clayborg added a comment to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.

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...

Nov 11 2020, 3:43 PM · Restricted Project
clayborg added inline comments to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.
Nov 11 2020, 3:14 PM · Restricted Project
clayborg added a comment to D91238: Recognize __builtin_debugtrap on arm64, advance pc past it so users can continue easily.

Yeah, I don't know where it's best to centralize the logic of (1) recognize a builtin_debugtrap instruction, and (2) set the pc so we step past it, know how to step past it when the user asks to resume control. debugserver (and lldb-server I'm sure) are already lying a *little* with a normal x86 user breakpoint - when you execute 0xcc at addr 0x100, the stop pc is 0x101 and someone is rolling that back to 0x100 to report the breakpoint hit (debugserver does this, I'm sure lldb-server does the same). So we're just adding more lies into the remote stub.

You could argue that the stub should always set $pc to be the instruction that hit the breakpoint/undefined instruction (so on x86, back up the pc by 1) and lldb should look at the breakpoint list / instruction bytes and decide if this was a user breakpoint or a builtin_trap of some kind. And when it's a builtin_debugtrap, and the user continues/steps/nexts, lldb could advance $pc past the instruction and then do the nexting.

I don't have much of an opinion about where this knowledge and pc adjusting should happen. Jim might though.

Nov 11 2020, 2:33 PM · Restricted Project
clayborg added a comment to D91130: [crashlog] Implement parser for JSON encoded crashlogs.

You could also make a fakey dsymForUUID script like we do in some other corefile tests. Compile a C file with foo(), bar(), baz(), substitute the addresses of those symbols from the binary into the crashlog.json, substitute the UUID of the compiled program in the crashlog.json, then the fakey-dsymForUUID finds the binary, lldb loads it at the binary at the correct offset. It's a lot of futzing around, but it would be possible.

Nov 11 2020, 2:26 PM · Restricted Project
clayborg added a comment to D89285: [lld-macho] Emit local symbols in symtab; record metadata in LC_DYSYMTAB.

Given that Darwin has fully moved to DWARF at this point, why STABS?

Nov 11 2020, 2:15 PM · Restricted Project
clayborg accepted D89285: [lld-macho] Emit local symbols in symtab; record metadata in LC_DYSYMTAB.

LGTM as long as local symbols always have names (see inlined comment).

Nov 11 2020, 2:12 PM · Restricted Project
clayborg added a comment to D91130: [crashlog] Implement parser for JSON encoded crashlogs.

JSON crash logs are almost here??? I remember asking for those back when I was at Apple! Great news.

Yep! 🥳

Nov 11 2020, 2:04 PM · Restricted Project
clayborg added a comment to D91238: Recognize __builtin_debugtrap on arm64, advance pc past it so users can continue easily.

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 11 2020, 1:59 PM · Restricted Project

Nov 10 2020

clayborg added a comment to D91130: [crashlog] Implement parser for JSON encoded crashlogs.

JSON crash logs are almost here??? I remember asking for those back when I was at Apple! Great news.

Nov 10 2020, 4:41 PM · Restricted Project
clayborg accepted D90490: [intel-pt][trace] Implement a "get supported trace type" packet.

LGTM with the AAAAAAAA error string stuff included so we can get a nice string error message. Pavel, do you have anything?

Nov 10 2020, 4:35 PM · Restricted Project
clayborg added a comment to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.

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 10 2020, 4:32 PM · Restricted Project

Nov 9 2020

clayborg requested changes to D89257: [lld-macho] Emit STABS symbols for debugging, and drop debug sections.
Nov 9 2020, 11:07 PM · Restricted Project
clayborg requested changes to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.

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.

Nov 9 2020, 4:11 PM · Restricted Project
clayborg added a comment to D90490: [intel-pt][trace] Implement a "get supported trace type" packet.

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 9 2020, 3:24 PM · Restricted Project

Nov 5 2020

clayborg requested changes to D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands.
Nov 5 2020, 5:06 PM · Restricted Project
clayborg added inline comments to D90490: [intel-pt][trace] Implement a "get supported trace type" packet.
Nov 5 2020, 4:40 PM · Restricted Project
clayborg accepted D89283: [trace][intel-pt] Implement the basic decoding functionality.

I am good too.

Nov 5 2020, 3:39 PM · Restricted Project, Restricted Project

Nov 2 2020

clayborg added inline comments to D90664: [crashlog] Move crash log parsing into its own class.
Nov 2 2020, 8:58 PM · Restricted Project
clayborg accepted D90665: [crashlog] Modularize parser.
Nov 2 2020, 8:57 PM · Restricted Project
clayborg accepted D90664: [crashlog] Move crash log parsing into its own class.

Looks good, just fix typo I mentioned in inlined comments.

Nov 2 2020, 8:56 PM · Restricted Project
clayborg added a comment to D90490: [intel-pt][trace] Implement a "get supported trace type" packet.

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.

Nov 2 2020, 8:16 PM · Restricted Project
clayborg accepted D70885: [lldb] Use explicit lldb commands on tests.
Nov 2 2020, 3:45 PM · Restricted Project
clayborg accepted D86292: [LLDB][RISCV] Add RISC-V ArchSpec and rv32/rv64 variant detection.
Nov 2 2020, 3:44 PM · Restricted Project
clayborg requested changes to D90490: [intel-pt][trace] Implement a "get supported trace type" packet.

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.

Nov 2 2020, 2:21 PM · Restricted Project

Oct 27 2020

clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

If anyone knows anyone that has a lot of experience with unwind information, please feel free to add them as reviewers.

Oct 27 2020, 4:03 PM · Restricted Project
clayborg added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Friendly ping

Oct 27 2020, 4:02 PM · Restricted Project

Oct 26 2020

clayborg accepted D87172: Check if debug line sequences are starting after the first code segment.

Just remove the braces as suggested in inline comments. Pavel?

Oct 26 2020, 2:23 PM · Restricted Project