aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.
sgtm.
https://reviews.llvm.org/D49739
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/
apolyakov updated this revision to Diff 158377.
apolyakov added a comment.
Changed the order of `if` statements to follow llvm coding standards.
https://reviews.llvm.org/D49739
Files:
include/lldb/API/SBTarget.h
lit/lit.cfg
lit/tools/lldb-mi/target/inputs/main.c
lit/tools/lldb-mi/target
aprantl added inline comments.
Comment at: source/API/SBTarget.cpp:1467
+ }
+ const ConstString csFrom(from), csTo(to);
+ if (csFrom && csTo) {
apolyakov wrote:
> aprantl wrote:
> > apolyakov wrote:
> > > aprantl wrote:
> > > > personally I would write this as
apolyakov added inline comments.
Comment at: source/API/SBTarget.cpp:1467
+ }
+ const ConstString csFrom(from), csTo(to);
+ if (csFrom && csTo) {
aprantl wrote:
> apolyakov wrote:
> > aprantl wrote:
> > > personally I would write this as:
> > > ```
> > > if (!
aprantl added inline comments.
Comment at: source/API/SBTarget.cpp:1467
+ }
+ const ConstString csFrom(from), csTo(to);
+ if (csFrom && csTo) {
apolyakov wrote:
> aprantl wrote:
> > personally I would write this as:
> > ```
> > if (!csFrom)
> > return error.
apolyakov added inline comments.
Comment at: source/API/SBTarget.cpp:1467
+ }
+ const ConstString csFrom(from), csTo(to);
+ if (csFrom && csTo) {
aprantl wrote:
> personally I would write this as:
> ```
> if (!csFrom)
> return error.SetErrorString(" path is
aprantl added inline comments.
Comment at: source/API/SBTarget.cpp:1467
+ }
+ const ConstString csFrom(from), csTo(to);
+ if (csFrom && csTo) {
personally I would write this as:
```
if (!csFrom)
return error.SetErrorString(" path is empty");
if (!csTo)
ret
apolyakov updated this revision to Diff 158330.
apolyakov added a comment.
Made error handling more meaningful: added different error messages for cases -
empty path and empty path.
https://reviews.llvm.org/D49739
Files:
include/lldb/API/SBTarget.h
lit/lit.cfg
lit/tools/lldb-mi/target/
clayborg added inline comments.
Comment at: source/API/SBTarget.cpp:1476-1477
+ } else
+error.SetErrorStringWithFormat(" can't be empty "
+ "(SBTarget::%s) failed", __FUNCTION__);
+}
check if csFrom or csTo is empty and give
apolyakov updated this revision to Diff 158319.
apolyakov added a comment.
Added converting from `const char *` to `ConstString`, documented new API.
https://reviews.llvm.org/D49739
Files:
include/lldb/API/SBTarget.h
lit/lit.cfg
lit/tools/lldb-mi/target/inputs/main.c
lit/tools/lldb-mi/t
aprantl added inline comments.
Comment at: source/API/SBTarget.cpp:1468
+ }
+ if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
apolyakov wrote:
> aprantl wrote:
> > apolyakov wrote:
> > > I didn't find nullptr chec
apolyakov added inline comments.
Comment at: source/API/SBTarget.cpp:1468
+ }
+ if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
aprantl wrote:
> apolyakov wrote:
> > I didn't find nullptr check in other API functi
aprantl added a comment.
Seems good otherwise.
Comment at: source/API/SBTarget.cpp:1468
+ }
+ if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
apolyakov wrote:
> I didn't find nullptr check in other API functions
apolyakov added a comment.
OK, thank you for respond. Then, I think, we should wait for review from
@clayborg or @aprantl, and if they accept the patch I'll divide it into two
parts: SB API part and target-select one.
https://reviews.llvm.org/D49739
_
labath added a comment.
Sorry for not responding, I've been a bit busy these days. I've wanted to make
a proof-of-concept to show you that reading the port number from stdout is
possible, but I haven't gotten around to it yet.
However, I am happy with the current mechanism of choosing the port
apolyakov added a comment.
So, do you have any thoughts about this approach letting the debugserver choose
a tcp port and patch overall?
https://reviews.llvm.org/D49739
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org
apolyakov updated this revision to Diff 157512.
apolyakov added a comment.
Now tcp port is choosing by debugserver.
https://reviews.llvm.org/D49739
Files:
include/lldb/API/SBTarget.h
lit/lit.cfg
lit/tools/lldb-mi/target/inputs/main.c
lit/tools/lldb-mi/target/inputs/target-select-so-path
apolyakov added inline comments.
Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11
+def get_free_port():
+s = socket.socket()
+s.bind(('', 0))
+return s.getsockname()[1]
labath wrote:
> apolyakov wrote:
> > labath wrote:
> > >
labath added inline comments.
Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11
+def get_free_port():
+s = socket.socket()
+s.bind(('', 0))
+return s.getsockname()[1]
apolyakov wrote:
> labath wrote:
> > apolyakov wrote:
> > >
apolyakov added inline comments.
Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11
+def get_free_port():
+s = socket.socket()
+s.bind(('', 0))
+return s.getsockname()[1]
labath wrote:
> apolyakov wrote:
> > labath wrote:
> > >
labath added inline comments.
Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11
+def get_free_port():
+s = socket.socket()
+s.bind(('', 0))
+return s.getsockname()[1]
apolyakov wrote:
> labath wrote:
> > This is still racy, bec
apolyakov added inline comments.
Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11
+def get_free_port():
+s = socket.socket()
+s.bind(('', 0))
+return s.getsockname()[1]
labath wrote:
> This is still racy, because the port can
labath added inline comments.
Comment at: lit/lit.cfg:59
-debugserver = lit.util.which('debugserver', lldb_tools_dir)
+if platform.system() in ['Darwin']:
+debugserver = lit.util.which('debugserver', lldb_tools_dir)
apolyakov wrote:
> Do we have `debugserve
apolyakov added inline comments.
Comment at: lit/lit.cfg:59
-debugserver = lit.util.which('debugserver', lldb_tools_dir)
+if platform.system() in ['Darwin']:
+debugserver = lit.util.which('debugserver', lldb_tools_dir)
Do we have `debugserver` only on macOS
apolyakov updated this revision to Diff 157310.
apolyakov added a comment.
Moved test from bash to python, removed unnecessary
`Target::AppendImageSearchPath`.
https://reviews.llvm.org/D49739
Files:
include/lldb/API/SBTarget.h
lit/lit.cfg
lit/tools/lldb-mi/target/inputs/main.c
lit/tool
labath added a comment.
In https://reviews.llvm.org/D49739#1174810, @apolyakov wrote:
> Thanks, I used this `lldb-server gdbserver --pipe 0 localhost:0` and got a
> port chosen by debugserver. But to let lldb-mi know about this port we need
> an additional script, is python a good choice for th
apolyakov added a comment.
Thanks, I used this `lldb-server gdbserver --pipe 0 localhost:0` and got a port
chosen by debugserver. But to let lldb-mi know about this port we need an
additional script, is python a good choice for that?
https://reviews.llvm.org/D49739
_
labath added a comment.
In https://reviews.llvm.org/D49739#1174769, @apolyakov wrote:
> `packages/Python/lldbsuite/test/tools/lldb-mi/signal/TestMiSignal.py` test
> contains `port = 12000 + random.randint(0, 3999)`.
Ok, that's bad. I don't know why the other lldb-mi tests are flaky, but these
apolyakov added a comment.
`packages/Python/lldbsuite/test/tools/lldb-mi/signal/TestMiSignal.py` test
contains `port = 12000 + random.randint(0, 3999)`.
https://reviews.llvm.org/D49739
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http:
labath added a comment.
In https://reviews.llvm.org/D49739#1174744, @apolyakov wrote:
> What do you think about running tests with a hardcoded port number(as it's
> done in a web-services). Doing this, we get rid of additional scripts and
> os-specific things. AFAIK, debugserver even has its ow
apolyakov added a comment.
What do you think about running tests with a hardcoded port number(as it's done
in a web-services). Doing this, we get rid of additional scripts and
os-specific things. AFAIK, debugserver even has its own default port.
P.S. As I saw in the lldb-mi python tests, port n
labath added a comment.
I am sure that particular test is worth trying to implement in lit. That script
of yours is full of operating system specifics, and it's going to be super
flaky.
I'd suggest keeping this as a python test, as there you can abstract the
platform specifics much easier, and
clayborg added inline comments.
Comment at: include/lldb/API/SBTarget.h:275-276
+ void AppendImageSearchPath(const char *from, const char *to);
+
+ void AppendImageSearchPath(const char *from, const char *to,
Remove this first one? Other functions were first
apolyakov created this revision.
apolyakov added reviewers: aprantl, clayborg, labath.
Herald added a subscriber: ki.stfu.
In this patch I suggest a way to deal with the problem started in
https://reviews.llvm.org/D47302 and use it to re-implement MI target-select
command. You are welcome to com
34 matches
Mail list logo
