ravitheja updated this revision to Diff 98599.
ravitheja added a comment.
Changes for the feedback recieved in first round of review.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/Nati
ravitheja marked 28 inline comments as done.
ravitheja added inline comments.
Comment at: docs/lldb-gdb-remote.txt:212
//--
+// QTrace:1:type:;
+//
clayborg wrote:
> labath wrote:
> > ravitheja
ravitheja marked 2 inline comments as done.
ravitheja added a comment.
In https://reviews.llvm.org/D32585#740632, @labath wrote:
> I quite like that you have added just the packet plumbing code without an
> concrete implementation. However, that is still a significant amount of
> parsing code t
clayborg added a comment.
So my question is: if we go the jTrace packet route, I would like to not use
GDB remote key/value pairs, and just go full JSON. You can make a new JSON dict
and pass all of the GDB remote key/value pairs inside and then you can add a
"jparams" key whose value is the JS
zturner added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1125
+uint64_t extracted_value;
+value.getAsInteger(16, extracted_value);
+
Should you be checking the return value of `getAsInteger` here?
jingham added a comment.
If you just have symbols, you can't tell whether private symbols were visible
to the current frame or not, but the likelihood is not 0 (I guess it's
something like 1/number of CU's). OTOH, your expression could never have seen
a private symbol from another module, and
jingham added a comment.
But note that the real solution to that problem is implementing some syntax for
"symbol from CU", "symbol from Function" etc, as we've discussed in the past.
Then what we do by default will be less important, since you have a way to
override it.
https://reviews.llvm.
spyffe added a comment.
I also would flip 2 and 3. I'll give this a shot.
https://reviews.llvm.org/D33083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jmajors marked 43 inline comments as done.
jmajors added a comment.
Every comment should either be marked done, with an accompanying code change,
or have a reply comment.
Comment at: unittests/CMakeLists.txt:57
add_subdirectory(Breakpoint)
+add_subdirectory(tools)
add_subdir
jmajors updated this revision to Diff 98673.
jmajors marked an inline comment as done.
jmajors added a comment.
Made changes based on feedback.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unitt
spyffe updated this revision to Diff 98672.
spyffe added a comment.
Improved symbol lookup per Greg and Jim's suggestion, with flipped steps 2 and
3.
Also added testing for the error case.
https://reviews.llvm.org/D33083
Files:
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefil
clayborg added a comment.
The searching code is indeed better and what we now want, but I question of
this code should live in ClangExpressionDeclMap::FindGlobalDataSymbol. A better
home for this might be in SymbolContext::FindGlobalDataSymbol? Then others can
use this functionality. Other clie
jingham added a comment.
I think Greg's right. I can't see anything expression parser specific to how
you do this search. I was a little worried that a lot of the other searches
will return multiple names (maybe sorting to indicate closeness.) So this one
might be confusing. But the API na
zturner added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
This will work on MSVC and presumably clang. I'm not sure about gcc. Is that
sufficient for your needs? Do y
jmajors marked 2 inline comments as done.
jmajors added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
zturner wrote:
> This will work on MSVC and presumably clang. I'm not
jmajors updated this revision to Diff 98675.
jmajors added a comment.
Feedback changes.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.cpp
un
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:98
+ unsigned int register_id;
+ key_str_ref.getAsInteger(10, register_id);
+
Do you need to check for an error here?
Comment at: un
krytarowski added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:200
+
+ return std::stoul(big_endian, nullptr, 16);
+}
Throws exceptions.
https://reviews.llvm.org/D32930
___
lld
clayborg added a comment.
Yeah, we need to document exactly what this is doing: looking for the best
symbol as an expression parser would want it, preferring symbols from the
current module in the SymbolContext, and then looking everywhere. Errors if
multiple exist in current module, or in any
Author: spyffe
Date: Thu May 11 17:08:05 2017
New Revision: 302833
URL: http://llvm.org/viewvc/llvm-project?rev=302833&view=rev
Log:
[DWARF parser] Produce correct template parameter packs
Templates can end in parameter packs, like this
template struct MyStruct
{ /*...*/ };
LLDB does not cu
This revision was automatically updated to reflect the committed changes.
Closed by commit rL302833: [DWARF parser] Produce correct template parameter
packs (authored by spyffe).
Changed prior to commit:
https://reviews.llvm.org/D33025?vs=98388&id=98687#toc
Repository:
rL LLVM
https://revie
jmajors marked 5 inline comments as done.
jmajors added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:200
+
+ return std::stoul(big_endian, nullptr, 16);
+}
krytarowski wrote:
> Throws exceptions.
I didn't change this, because
jmajors updated this revision to Diff 98691.
jmajors marked an inline comment as done.
jmajors added a comment.
More feedback changes.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tool
krytarowski added a comment.
Some headers are UNIX-specific like ``, is it possible to make these
tests compatible with Windows?
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin
krytarowski added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:1
+#include
+#include
For the clarity I would put the copyright notice in all files, including tests.
https://reviews.llvm.org/D32930
__
jmajors marked an inline comment as done.
jmajors added a comment.
I cleaned up the includes.
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jmajors updated this revision to Diff 98702.
jmajors added a comment.
Cleaned up the includes.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.c
Author: spyffe
Date: Thu May 11 18:38:21 2017
New Revision: 302850
URL: http://llvm.org/viewvc/llvm-project?rev=302850&view=rev
Log:
xfail TestClassTemplateParameterPack on gcc to mollify
lldb-x86_64-ubuntu-14.04-cmake.
Modified:
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/class-temp
krytarowski added inline comments.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+ Host::Kill(server_process_info.GetProcessID(), 15);
+}
jmajors wrote:
> labath wrote:
> > This is not portable.
> Is there a p
krytarowski added a comment.
I can build locally with `make thread_inferior`, how to run it?
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
krytarowski added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
jmajors wrote:
> zturner wrote:
> > This will work on MSVC and presumably clang. I'm not sure about gcc. Is
krytarowski added a comment.
If I'm not mistaken LLVM `__builtin_debugtrap` is defined only for X86. It will
also fail on GCC as there is no support for it.
The NetBSD buildbot in LLDB runs GCC 5.x with libstdc++.
https://reviews.llvm.org/D32930
_
krytarowski added a comment.
Personally I would rely on LLDB feature to set software/hardware breakpoint in
tracee, and never simulate debugtrap with some predefined value.
This will ensure portability to more CPUs and operating systems. For example
Sun SPARC CPUs (at least on NetBSD) won't pas
Author: zturner
Date: Fri May 12 00:48:54 2017
New Revision: 302874
URL: http://llvm.org/viewvc/llvm-project?rev=302874&view=rev
Log:
Fix Linux Buildbot.
Modified:
lldb/trunk/source/Plugins/Process/Linux/SingleStepCheck.cpp
Modified: lldb/trunk/source/Plugins/Process/Linux/SingleStepCheck.cp
Author: zturner
Date: Fri May 12 00:49:54 2017
New Revision: 302875
URL: http://llvm.org/viewvc/llvm-project?rev=302875&view=rev
Log:
Update StructuredData::String to return StringRefs.
It was returning const std::string& which was leading to
unnecessary copies all over the place, and preventing
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:61-63
+string name;
+thread_info->GetValueForKeyAsString("name", name);
+string reason;
jmajors wrote:
> zturner wrote:
> > `StringRef name, reason;`
> The
36 matches
Mail list logo
