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.

In D94997#2509175, @tatyana-krasnukha wrote:

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.

In D94997#2510144, @serhiy.redko wrote:

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.

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:

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

LGTM. Pavel?

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!

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.

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

In D93895#2488249, @jingham wrote:

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.

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:

In D89845#2481493, @jhenderson wrote:

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

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.

Found the test suite issue on

In D93895#2480578, @augusto2112 wrote:

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

In D93895#2480554, @augusto2112 wrote:

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?

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.

In D93895#2480324, @augusto2112 wrote:

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?

Update to main branch from today.

In D93895#2479845, @augusto2112 wrote:

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?

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.

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

In D89845#2454273, @jhenderson wrote:

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

Fix one last missed comment fix.

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

Update to fix more of jhenderson's issues.

Fixed issue found by jhenderson.

In D92736#2442831, @thakis wrote:

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?

In D92537#2444027, @int3 wrote:

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?

In D92537#2443306, @thakis wrote:

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

In D92736#2439511, @int3 wrote:

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

In D92736#2439612, @thakis wrote:

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?

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

In D92736#2438432, @thakis wrote:

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.

In D92537#2438256, @int3 wrote:

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

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.

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.

Thanks for chiming in Pavel. LGTM.

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?

Fixed issues found by grimar.

Fixed issues found by MaskRay.

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.

Thanks for adding the extra tests!

LTGM!

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

In D91238#2389877, @jasonmolenda wrote:

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.

In D91130#2389831, @jasonmolenda wrote:

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.

In D89285#2332489, @compnerd wrote:

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

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

In D91130#2387692, @JDevlieghere wrote:

In D91130#2387390, @clayborg wrote:

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

Yep! 🥳

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.

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.

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

I am good too.

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.

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

Friendly ping

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