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
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
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
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
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)
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
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
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
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
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
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
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
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
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,
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
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
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
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
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
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
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
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,
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
34 matches
Mail list logo
