[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D106035#2885174 , @nealsid wrote:

> In D106035#2879939 , @teemperor 
> wrote:
>
>> I actually expected after the RFC that we would remove all the non-wchar 
>> code, but this seems also fine. I think this LGTM in general, but I feel a 
>> bit nervous about touching stuff that depends so much on OS/environment. 
>> What OS/environment did you test this patch on? Would be nice to have this 
>> tested on a few setups before landing.
>
> This was my mistake, sorry.  I originally went the route in this patch and 
> ran into some errors testing, so I switched to what I detailed in the RFC.  
> But then I found the problem (I was using narrow chars for the GetCharacter 
> callback when that actually isn't supported). Overall I think it is best to 
> use narrow char and string rather than wide-char and wstring because that's 
> more consistent with the rest of LLVM.

I actually like this approach even better, so I'm glad this turned out the way 
it did!

> Regarding platforms, I tested on OS X Big Sur and Monterey, and Debian Linux 
> inside a VM.  Jan was able to build on Fedora.  I'm happy to test on more 
> platforms - FreeBSD, NetBSD perhaps?

I think this is more than enough to test on before landing so this LGTM. FWIW, 
we do have a release branching in about 10 days or so, so you might want to 
wait with landing this just directly after that branch was made so that this 
spends a bit more time on ToT where we can find issues before going into a 
release. (I assume you don't care whether this makes it into the current 
release or not, but please correct me if i'm wrong).

>> Also I'm kinda curious if you found any docs/examples that explain whether 
>> mixing the wchar/char functions like we do now is actually supported in 
>> libedit? IIUC we call now `el_wset` and `el_set` on the same libedit 
>> instance. It feels like the `wchar` support in the FreeBSD port was some 
>> kind of parallel implementation to the normal `char` support, so I'm 
>> surprised that we can just mix those functions and everything still works 
>> fine (at least on my Linux machine this seems to work).
>
> Yeah, the original source converts the parameters and calls el_w* functions 
> when the narrow-char functions are called.  This is also true on FreeBSD: 
> https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359

Cool, thanks for finding that out!

Anyway, I think this LGTM. Thanks for doing it! Also I can't recall if you have 
commit access, so please let me know if I should land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106035

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

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus created this revision.
apoos-maximus requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

lldb attach operation fails when the ptrace() call fails on a traced
process. The error reporting in this scenario could be improved by
including what went wrong and what could be done by the user to resolve
the situation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106226

Files:
  lldb/source/Commands/CommandObjectProcess.cpp


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,9 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage("Could not attach to process. If your uid matches 
the uid of the target process, \n" 
+   "check the setting of 
/proc/sys/kernel/yama/ptrace_scope, \n" 
+   "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,9 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage("Could not attach to process. If your uid matches the uid of the target process, \n" 
+   "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n" 
+   "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus added reviewers: jingham, rupprecht, davide.
apoos-maximus added a comment.

This patch is supposed to fix this Bug :  39166 

I compiled the code and used the binary with local/remote debugging scenarios. 
On attach failure this is the output it produces :-

  $ ./lldb -p 46503
  (lldb) process attach --pid 46503
  Could not attach to process. If your uid matches the uid of the target 
process, 
  check the setting of /proc/sys/kernel/yama/ptrace_scope, 
  or try again as the root user.
  error: attach failed: Operation not permitted
  (lldb) 

My queries :-

- Is AppendMessage() really the right function to use in an error scenario ? or 
the additional message should just be appended 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() ?


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

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus updated this revision to Diff 359584.
apoos-maximus added a comment.

changed the code-styling to comply with pre-merge checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

Files:
  lldb/source/Commands/CommandObjectProcess.cpp


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,10 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the 
target process, \n" 
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n" 
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,10 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the target process, \n" 
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n" 
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus updated this revision to Diff 359585.
apoos-maximus added a comment.

realigned patch against main branch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

Files:
  lldb/source/Commands/CommandObjectProcess.cpp


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,10 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the 
target process, \n" 
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n" 
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,10 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the target process, \n" 
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n" 
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106226: [lldb] Improve error message when "lldb attach" fails

[APOORV SACHAN via Phabricator via lldb-commits]

apoos-maximus updated this revision to Diff 359586.
apoos-maximus added a comment.

reformatted code to comply with pre-merge checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106226

Files:
  lldb/source/Commands/CommandObjectProcess.cpp


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,11 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the "
+  "target process, \n"
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n"
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())


Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -409,6 +409,11 @@
   }
 } else {
   result.AppendErrorWithFormat("attach failed: %s\n", error.AsCString());
+  result.AppendMessage(
+  "Could not attach to process. If your uid matches the uid of the "
+  "target process, \n"
+  "check the setting of /proc/sys/kernel/yama/ptrace_scope, \n"
+  "or try again as the root user. \n");
 }
 
 if (!result.Succeeded())
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits