Re: [Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

I totally agree with you, I didn't think about creating custom callbacks =P. I'll refactor the code accordingly. Thanks, man Il giorno ven 2 ott 2020 alle ore 01:51 Pavel Labath ha scritto: > On 01/10/2020 20:57, Walter wrote: > >> - I am surprised that it was not necessary to create a special

Re: [Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

On 01/10/2020 20:57, Walter wrote: >> - I am surprised that it was not necessary to create a special process > plugin for this purpose. I have a feeling one will be necessary sooner > or later because of the need to customize the step/continue/etc. flows. > Currently, this will probably produce

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. In D85705#2305994 , @labath wrote: > I have two high-level remarks/questions about this: > > - I am surprised that it was not necessary to create a special process plugin > for this purpose. I have a feeling one will be

Re: [Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

> - I am surprised that it was not necessary to create a special process plugin for this purpose. I have a feeling one will be necessary sooner or later because of the need to customize the step/continue/etc. flows. Currently, this will probably produce very bizarre if one tries to execute those

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

labath added a comment. I have two high-level remarks/questions about this: - I am surprised that it was not necessary to create a special process plugin for this purpose. I have a feeling one will be necessary sooner or later because of the need to customize the step/continue/etc. flows.

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace marked an inline comment as done. wallace added inline comments. Comment at: lldb/source/Target/TraceSettingsParser.cpp:128 +std::string plugin_schema(GetPluginSchema()); +plugin_schema = std::regex_replace(plugin_schema, std::regex("\n"), "\n "); +

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

MaskRay added inline comments. Comment at: lldb/source/Target/TraceSettingsParser.cpp:123 + if (schema.empty()) { +std::ostringstream schema_builder; +schema_builder << "{\n \"trace\": "; This requires `sstream`. Fixed. Comment at:

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG74c93956e1c1: Add a Trace plug-in to LLDB to add process trace

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 292922. wallace added a comment. Based on the discussion from D87335 , I'm moving all the JSON utilities as non-member functions to the settings parser file. Eventually this can be refactored if that patch moves forward.

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg accepted this revision. clayborg added a comment. This looks good to me. We will need to wait until the JSON changes from https://reviews.llvm.org/D87335 to make it in though in case there are any modifications to that patch. Walter will put up the RFC for trace plug-ins and if that

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 290603. wallace added a comment. - Moved the JSON changes to another diff. - Addressed all the comments so far. As a side note, I'll work on a RFC for the LLDB mailing list to get feedback on the overall design of the feature, as @labath suggested. However,

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

labath added a subscriber: sammccall. labath added a comment. I'm pretty sure the json changes should get a separate review. The json owner (let's call him @sammccall) might have a different idea on how to do this thing... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 289301. wallace added a comment. fix header c++ declaration Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/ https://reviews.llvm.org/D85705 Files: lldb/include/lldb/Core/PluginManager.h

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

aprantl added inline comments. Comment at: lldb/source/Commands/CommandObjectTrace.h:1 +//===-- CommandObjectTrace.h --===// +// Can you add `-*- C++ -*-` markers the the `.h` files you are adding? Otherwise tools

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 289285. wallace added a comment. fix nit. Waiting for an additional approval Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/ https://reviews.llvm.org/D85705 Files:

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg accepted this revision. clayborg added a comment. LGTM now. Probably should get an ok from one more person. Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp:1 +//===-- TraceIntelPTSettingsParser.cpp

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 288788. wallace added a comment. - Removed whitespace only changes. - Simplified the way a SettingsParser is created, now there's no need to passinga a pointer around. - Fixed the forward declaration in lldb-private-interfaces. This should address all

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. A few simple changes from inlined comments. Other than that looks good. Should get another OK from others. Also not sure if the JSON stuff in llvm needs to be done separately? Comment at: lldb/include/lldb/Target/Trace.h:115 + /// implementation,

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 288747. wallace added a comment. fix typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/ https://reviews.llvm.org/D85705 Files: lldb/include/lldb/Core/PluginManager.h

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 288739. wallace added a comment. Herald added subscribers: llvm-commits, danielkiss, hiraditya. Herald added a project: LLVM. Addressed all comments. - Wwitched to using llvm::json instead of StructuredData. Fortunately, the logic is pretty much the same. -

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace added a comment. You changed my mind. I'll move to Support/JSON.h Comment at: lldb/test/API/commands/trace/intelpt-trace/trace.json:11 + }, + "triple": "x86_64-*-linux", + "processes": [ labath wrote: > What if one of the processes is 64-bit and the

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

labath added a comment. In D85705#2243536 , @wallace wrote: > - I'm still using StructuredData for the parsing. There's a chance that once > we release this feature users will want support for other formats besides > JSON, so for now I prefer to ask for

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 288496. wallace added a comment. Addressed comments - Now using StringRef correctly whenever possible. - Revert back to using "traceFile" only inside the thread section. That's how intel-pt works at the moment and, as Greg suggested, we can change the

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. In D85705#2241249 , @labath wrote: > If the format is going to be json, why not use llvm::json from > `llvm/Support/JSON.h`? That's what

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

labath added a comment. If the format is going to be json, why not use llvm::json from `llvm/Support/JSON.h`? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace added a comment. Thanks for the review! Those are very useful comments. So, I'll split the parsing out from the Trace object. Regarding a possible inconsistent state, you are right. That could happen. The targets, modules and processes used in the parsing are all created there, and it

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

JDevlieghere added a comment. In D85705#2240249 , @wallace wrote: > Given that this feature is intended to be used by non debugger developers, I > want to stick to using JSON as a default input format, as it's more > ubiquitous than YAML and I prefer to

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 288145. wallace added a comment. - As error messaging is important when parsing, I create a new set of functions in StructuredData for parsing that return an llvm::Expected. This allowed me to move the helper functions for parsing from the Trace file to

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace added a comment. Given that this feature is intended to be used by non debugger developers, I want to stick to using JSON as a default input format, as it's more ubiquitous than YAML and I prefer to make this decision based on the user experience than on the technical solution. I like

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

JDevlieghere added a comment. In D85705#2237506 , @clayborg wrote: > In D85705#2237397 , @JDevlieghere > wrote: > >> A large part of this patch is concerned with parsing which worries me from a >> maintenance

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. In D85705#2237397 , @JDevlieghere wrote: > A large part of this patch is concerned with parsing which worries me from a > maintenance perspective. Did you consider using Yaml I/O > ? While

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. After speaking with Walter a bit, key names in the JSON should be camel case. Many languages (Swift and JavaScript) can auto import JSON and create objects and use the key names as member variable names, and if they are camelCase, then no name conversion needs to

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Looking better. The main thing we need to do it modify StructuredData classes a bit by moving a lot of the static functions we are using here into the appropriate classes. See

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace added a comment. That's a very good idea! I'll revisit this patch then. I was not fond of doing the manual parsing but I hadn't found a more automated way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

JDevlieghere added a comment. A large part of this patch is concerned with parsing which worries me from a maintenance perspective. Did you consider using Yaml I/O ? While I'm not a particularly big fan of the format, the benefits of being able to

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 287527. wallace added a comment. - Added a command that shows the schema of a given trace plugin. - Added a test for such command Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 287518. wallace added a comment. - Addressed comments. - Added schemas for both the base class and the implementation plugins. - Added two tests checking the error messages along with the schema when parsing failed. - Made some general clean up of the code

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace marked 12 inline comments as done. wallace added inline comments. Comment at: lldb/source/Target/Trace.cpp:109 + StringRef load_address_str; + if (!module->GetValueForKeyAsString("loadAddress", load_address_str)) +return SetMissingFieldError(error, "loadAddress",

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Lots of little things regarding the encoding and format of the JSON. Comment at: lldb/source/Target/Trace.cpp:37 + std::errc::invalid_argument, + "no

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace updated this revision to Diff 287079. wallace added a comment. - Added libipt as a dependency to build the new intel pt plugin. It's merely a copy paste of what the old intel pt plugin does. - Added a test with a sample trace.json definition file. It includes a simple a.out object

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

wallace added a comment. Thanks for doing this. There are some basic bits to make this that I was unaware of, so this saves a lot of time for me :) I'll take over this patch next week and fill it with intel-pt stuff, I already have a good amount of code that is quite mergeable with this patch.

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

aprantl added a comment. I only added a few non-substantial inline comments. Comment at: lldb/include/lldb/Target/Trace.h:39 +public: + ~Trace() override = default; + I'm just curious: What's the purpose of this? Is the destructor = 0 in the base class and

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg updated this revision to Diff 285177. clayborg added a comment. Fix inline comment issues from vsk. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85705/new/ https://reviews.llvm.org/D85705 Files: lldb/include/lldb/Core/PluginManager.h

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

vsk added a comment. In D85705#2211607 , @clayborg wrote: > In D85705#2211073 , @vsk wrote: > >> This looks very cool, thanks @clayborg! I think using JSON to describe the >> trace data (what kind of trace is

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. In D85705#2211073 , @vsk wrote: > This looks very cool, thanks @clayborg! I think using JSON to describe the > trace data (what kind of trace is this, what's in it, etc.) sounds reasonable. > >> For "trace load", I get the

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

vsk added a comment. This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable. > For "trace load", I get the plugin for the JSON file by matching it up with > the "name" field in the JSON, but I

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

clayborg added a comment. The idea for the JSON file is that it must have a "name" (or maybe "plug-in"?) to identify which trace plug-in and a "trace-file" at the very least: { "name": "intel-pt", "trace-file": "/tmp/trace-info" } The "name" field would match the name of the