labath added inline comments.

================
Comment at: lldb/include/lldb/Host/Host.h:231
 
+  /// Check whether a process is translated (Rosetta).
+  /// \arg process_info The info structure for the process queried.
----------------
aprantl wrote:
> davide wrote:
> > aprantl wrote:
> > > Is this supposed to be a generic function call that, e.g, should fire for 
> > > debugging a process in QEMU under Linux, or is it meant to specifically 
> > > check for Rosetta on macOS? The comment makes it sound like it's the 
> > > latter and the API name hints at the former. It would be nice to clarify 
> > > this in the comment.
> > It is meant specifically for Rosetta, but this leaves in `Host`, so I 
> > decided for a generic name.
> > Do you have a better suggestion for the name or the comment ? Happy to go 
> > with it 
> Not awesome, but `IsMacOSRosettaProcess` or `IsMacOSRosettaTranslated`?
> 
> A more serious question: Is Host actually the right place for this function? 
> What happens when I remote-debug a Rosetta process from another Mac or a 
> Linux machine? Is there even a correct abstraction for this use-case? 
> Platform maybe?
This code is only used when debugging on the host. When debugging remotely, you 
either connect to at already running instance of debugserver, or you ask the 
lldb-server-platform process to launch one for you. The platform process then 
should use this function to find the right debugserver to launch, but using a 
Host function is appropriate there, because the platform process is running 
remotely, and so it will the the "remote Host" which is answering the question.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3424
+    if (Host::IsProcessTranslated(process_info)) {
+      FileSpec 
rosetta_debugserver("/Library/Apple/usr/libexec/oah/debugserver");
+      debugserver_launch_info.SetExecutableFile(rosetta_debugserver, false);
----------------
davide wrote:
> labath wrote:
> > I don't think this is a particularly good abstraction, as the debugserver 
> > path is still left here, and the path is definitely os- and arch-specific. 
> > It would be better if the API returned the path to the 
> > debugserver-to-be-used instead.
> > 
> > Bonus points if you can also hide the `DEBUGSERVER_BASENAME` logic from 
> > GDBRemoteCommunication.cpp into the same API.
> hmm, I wonder if this code should live in `GDBRemoteCommunication.cpp`
I think it ended up there because the gdb-server launching code is used from 
both liblldb and lldb-server (the platform version), and GDBRemoteCommunication 
is the greatest common denominator. It's not ideal, but I don't think its the 
worst thing that we have. (At one point I'd like to create a library to house 
code which is shared between liblldb and lldb-server, and then it may be easier 
to organize things better.)

Speaking of that... I think this code should live in GDBRemoteCommunication as 
well. In the situation where one is remote-debugging a mac via 
lldb-server-platform, I assume one would want the platform process to 
automatically spawn the right debugserver for the rosetta thingies.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82813/new/

https://reviews.llvm.org/D82813



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to