[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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 ?

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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);

[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

ravitheja added inline comments. Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393 + llvm::SmallVector parts = { + src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)}; + labath wrote: > Isn't the drop-back

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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: >

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

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, +

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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\" :

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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.

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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

[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

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"?

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

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

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

ravitheja added inline comments. Comment at: include/lldb/API/SBTraceOptions.h:38 + /// They should be formatted as a JSON Array. + void setTraceParams(lldb::SBStream ); + clayborg wrote: > This should probably be: > > ``` > void

[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

ravitheja added inline comments. Comment at: include/lldb/API/SBProcess.h:269 + lldb::user_id_t StartTrace(SBTraceOptions , lldb::SBError , + lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID); + clayborg wrote: > Should the thread be