ravitheja updated this revision to Diff 93850.
ravitheja added a comment.
Changes for review.
https://reviews.llvm.org/D29581
Files:
include/lldb/API/LLDB.h
include/lldb/API/SBDefines.h
include/lldb/API/SBError.h
include/lldb/API/SBProcess.h
include/lldb/API/SBStructuredData.h
inclu
ravitheja updated this revision to Diff 95930.
ravitheja added a comment.
New diff with requested changes.
https://reviews.llvm.org/D29581
Files:
include/lldb/API/LLDB.h
include/lldb/API/SBDefines.h
include/lldb/API/SBError.h
include/lldb/API/SBProcess.h
include/lldb/API/SBStructuredD
ravitheja marked 7 inline comments as done.
ravitheja added inline comments.
Comment at: include/lldb/API/SBProcess.h:251
+ /// supported. To obtain the actual used trace options please
+ /// use the GetTraceConfig API. For the custom parameters, only
+ /// the par
ravitheja updated this revision to Diff 96682.
ravitheja added a comment.
Fixing few header file inclusions.
https://reviews.llvm.org/D29581
Files:
include/lldb/API/LLDB.h
include/lldb/API/SBDefines.h
include/lldb/API/SBError.h
include/lldb/API/SBProcess.h
include/lldb/API/SBStructure
ravitheja added inline comments.
Comment at: docs/lldb-gdb-remote.txt:212
//--
+// QTrace:1:type:;
+//
clayborg wrote:
> Should we make all these new packets JSON based to start with? "jTrace"?
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
ravitheja added a comment.
> TraceOptions opt;
> opt.setType(...);
> opt.setBufferSize(...);
> opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor,
> this could be a one-liner
> std::future result = std::async(std::launch::async, [&] {
> return client.StartT
ravitheja added a comment.
Well nothings preventing me from doing so, I have the changes for that were
suggested to me for this revision. I thought I would first upload them, so that
people can look at the parallel while I upload the linux server code and Unit
tests.
https://reviews.llvm.org/
ravitheja updated this revision to Diff 99716.
ravitheja added a comment.
Adding unit tests and making the packets fully JSON.
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/NativeProce
ravitheja added a comment.
Sorry I had to update, since the Error class has been changed to Status.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
ravitheja added inline comments.
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:352
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+return Status("Not implemented");
+ }
krytarowski wrote:
> This is Linuxism. Not ever
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3293
+ if (custom_params_sp) {
+if (custom_params_sp->GetType() !=
StructuredData::Type::eTypeDictionary) {
+ error.SetErrorString("Invalid Conf
ravitheja updated this revision to Diff 99870.
ravitheja added a comment.
Updates from last 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/NativeProcessProtocol.h
include/lldb
ravitheja added inline comments.
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35
ravitheja updated this revision to Diff 99891.
ravitheja added a comment.
Modifying string literals in Unit tests.
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/NativeProcessProtocol.h
ravitheja updated this revision to Diff 100051.
ravitheja added a comment.
Running clang-format.
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/NativeProcessProtocol.h
include/lldb/Ta
ravitheja added a comment.
Hello, the reason I switched to allocating with new (std::nothrow)
uint8_t[byte_count]; was because the byte_count is actually user defined and I
do want to catch cases where lldb is not able to allocate the requested buffer
size.
https://reviews.llvm.org/D32585
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+ // -
+ static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3009-3010
+ errno = 0;
+ m_mmap_base =
+ mmap(NULL, (metabufsize + page_size), PROT_WRITE, MAP_SHARED, m_fd, 0);
+ if (m_mmap_base == MAP_FAILED) {
zturner wr
ravitheja updated this revision to Diff 102540.
ravitheja marked 29 inline comments as done.
ravitheja added a comment.
Changes suggested in previous round of feedback.
https://reviews.llvm.org/D33674
Files:
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Host/linux/Support.h
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847
+
+Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) {
+ Status ret_error;
labath wrote:
> You are calling this function with uid =
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:611
+
+if (ProcessorTraceMonitor::GetProcessTraceID() != LLDB_INVALID_UID) {
+ auto traceMonitor = ProcessorTraceMonitor::Create(
labath wrote:
> Every call
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+error.SetError(FileNotFound, eErrorTypePOSIX);
+return error;
labath wrote:
> eErrorTypePOSI
ravitheja added a comment.
Just to make things clear, I will explain a use case
Suppose if we are debugging an application where the main thread spawns a
second new thread ->
int main() {
int i = 0; // user starts tracing on main thread -> gets traceid 1
. // Some statements
i++;
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+ // same process user id.
+ llvm::DenseSet m_pt_traced_thread_group;
};
labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > I am confused about the purpose
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+error.SetError(FileNotFound, eErrorTypePOSIX);
+return error;
labath wrote:
> ravitheja wrot
ravitheja added a comment.
Although a bit confusing, there is more flexibility for the user.I must also
point out that the trace buffer available is not unlimited and there can be
situations where a user might simultaneously want to trace newly spawned
threads with a smaller buffer and trace an
ravitheja added a comment.
> Start tracing the whole process
> Ok. Traceid = 1
> Start tracing thread 47
> Error. Thread 47 is already traced. (???)
The error is because its already being traced, so if someone wants the trace
log, its already available and the user need not do anything extra.
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+ llvm::DenseMap
+ m_processor_trace_monitor;
labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > I'd like to downgrade these to unique pointers to Pro
ravitheja added a comment.
In https://reviews.llvm.org/D33674#783893, @labath wrote:
> In https://reviews.llvm.org/D33674#783861, @ravitheja wrote:
>
> > Although a bit confusing, there is more flexibility for the user.I must
> > also point out that the trace buffer available is not unlimited an
ravitheja updated this revision to Diff 103355.
ravitheja added a comment.
Changes for last feedback.
https://reviews.llvm.org/D33674
Files:
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Host/linux/Support.h
source/Host/linux/Support.cpp
source/Plugins/Process/Linux/CMak
ravitheja marked 5 inline comments as done.
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+ Status error;
labath wrote:
> I'd like this work to be done in the destruc
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:79
+ // Trace id of trace instance corresponding to the process.
+ static lldb::user_id_t m_pt_process_traceid;
+
labath wrote:
> labath wrote:
> > Instead of global vari
ravitheja updated this revision to Diff 103943.
ravitheja marked 6 inline comments as done.
ravitheja added a comment.
More changes from past feedback.
https://reviews.llvm.org/D33674
Files:
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Host/linux/Support.h
source/Host/lin
ravitheja added a comment.
Yes you can start looking at it. The thing with munmap stuff is the following,
you basically don't want to give the user an opportunity to have an
uninitialized or instances which have been destroyed but not deleted.
So I removed the Destroy function from the public sp
ravitheja added inline comments.
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+ llvm::SmallVector, 2> parts = {
+ src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+
labath wrote:
> Isn't the drop-back expression equiv
ravitheja added a comment.
>> Regarding the testing strategy, we have tests at two levels, one at the SB
>> API level and the other at the tool level.
>
> Cool, are you going to put some of those tests in this patch?
The problem with uploading tests is that they need minimum Broadwell machine
ravitheja added a comment.
In https://reviews.llvm.org/D33674#790653, @labath wrote:
> In https://reviews.llvm.org/D33674#790595, @ravitheja wrote:
>
> > Yes you can start looking at it. The thing with munmap stuff is the
> > following, you basically don't want to give the user an opportunity to
ravitheja updated this revision to Diff 104160.
ravitheja added a comment.
Removing Destroy() function.
https://reviews.llvm.org/D33674
Files:
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Host/linux/Support.h
source/Host/linux/Support.cpp
source/Plugins/Process/Linux/CM
ravitheja added a comment.
Yeah that would be broken, as long as we make the error packet more than 3
bytes, then it won't work with the older lldb clients.
https://reviews.llvm.org/D34945
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
h
ravitheja updated this revision to Diff 105150.
ravitheja added a comment.
changes from feedback.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClie
ravitheja marked 4 inline comments as done.
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:110
+ packet += ";";
+ packet += std::string(error.AsCString());
+ return SendPacketNoLock(packet);
lab
ravitheja added a comment.
With this patch, I see the extra packet to query from server is probably
unnecessary. I mean the error code is still there and it should be sufficient
for the client to detect an error. As long as it does not mistake it for
something else it should not be a problem ?
ravitheja updated this revision to Diff 105272.
ravitheja added a comment.
Updating Doc and changing feature to be enabled by client.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/g
ravitheja updated this revision to Diff 105273.
ravitheja added a comment.
Forgot this in the doc.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationCli
ravitheja updated this revision to Diff 105380.
ravitheja added a comment.
Support for Hex encoded strings and more error checking.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb
ravitheja updated this revision to Diff 105395.
ravitheja added a comment.
Correcting mistakes.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:32-35
+ RegisterPacketHandler(
+ StringExtractorGDBRemote::eServerPacketType_QEnableErrorStrings,
+ [this](StringExtractorGDBRemote packet, Status &error, b
ravitheja added a comment.
Uploaded SVN Revision Number - 307773 to fix Android Builder.
https://reviews.llvm.org/D34945
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
ravitheja updated this revision to Diff 90461.
ravitheja added a comment.
Herald added a subscriber: mgorny.
Changes according to the comments.
https://reviews.llvm.org/D29581
Files:
include/lldb/API/LLDB.h
include/lldb/API/SBDefines.h
include/lldb/API/SBError.h
include/lldb/API/SBProce
ravitheja added inline comments.
Comment at: include/lldb/API/SBProcess.h:269
+ lldb::user_id_t StartTrace(SBTraceOptions &sboptions, lldb::SBError &sberror,
+ lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
+
clayborg wrote:
> Shoul
ravitheja added inline comments.
Comment at: include/lldb/API/SBTraceOptions.h:38
+ /// They should be formatted as a JSON Array.
+ void setTraceParams(lldb::SBStream ¶ms);
+
clayborg wrote:
> This should probably be:
>
> ```
> void setTraceParams(lldb::SBStru
ravitheja added inline comments.
Comment at: scripts/interface/SBTrace.i:12
+
+class LLDB_API SBTrace {
+public:
clayborg wrote:
> Do we want something in here that explains what kind of trace buffer data you
> will be getting? I am thinking that even though we
55 matches
Mail list logo
