DavidSpickett added a comment.

Thanks for looking at this, I've been tripped up by this myself.

> Is AppendMessage() really the right function to use in an error scenario ? or 
> the additional message should just be appended to AppendErrorWithFormat() ?

It changes whether the output goes to stdout or stderr and whether we set the 
return status to failed. So I would merge it into one call to 
`AppendErrorWithFormat`.

> The error object is being returned by the Attach() call on line:399 so, 
> should this message be handled by the Attach() function and not by 
> DoExecute() ?

Go to https://lldb.llvm.org/python_reference/index.html, search for `SBTarget` 
and you'll see a few attach methods there. They call Target Attach so if you 
added it there they'd also get it. Which seems like a good thing to me. (but 
see my other comment about platforms)



================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:416
+          "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n"
+          "or try again as the root user. \n");
     }
----------------
While this is useful for Linux/Unixish things, lldb also runs on Windows as 
well where this message isn't going to make a lot of sense. Adding a `if linux 
...` at this level violates the separation of concerns though.
(yes an experienced user could ignore it but they'll probably at least wonder 
if lldb is confused)

Would it be possible to move this message further down into `Target::Attach` to 
a place where we know the target kind? (that would also answer your question 
about what level it should be at)

If you could return a status from that level with this message then you'd only 
see it where it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

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

Reply via email to