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

[Greg Clayton via Phabricator via lldb-commits]

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 trace plug-in matches the specified type: \"%s\"",
+      type.str().c_str());
----------------
Dump schema here as part of the error?


================
Comment at: lldb/source/Target/Trace.cpp:53
+  error.SetErrorStringWithFormat(
+      "structured data is missing the \"%s\" \"%s\" field: %s", field, type,
+      object_stream.GetData());
----------------
dump schema when we have an error?


================
Comment at: lldb/source/Target/Trace.cpp:65
+  error.SetErrorStringWithFormat("structured data is expected to be \"%s\": %s",
+                                 type, object_stream.GetData());
+  return false;
----------------
dump schema?



================
Comment at: lldb/source/Target/Trace.cpp:105-106
+  StringRef file_path;
+  if (!module->GetValueForKeyAsString("filePath", file_path))
+    return SetMissingFieldError(error, "filePath", "string", *module);
+
----------------
rename "filePath" to just "path"?


================
Comment at: lldb/source/Target/Trace.cpp:109
+  StringRef load_address_str;
+  if (!module->GetValueForKeyAsString("loadAddress", load_address_str))
+    return SetMissingFieldError(error, "loadAddress", "string", *module);
----------------
clayborg wrote:
> does JSON typically use camel case? Should this be "load-address"?
Should load address be encoded as an integer to begin with? I know it is less readable as an integer since we can't use hex numbers It would simplify the logic here. If we want to use strings, we should make a helper function that decodes an address from a key's value in a dictionary so we can re-use elsewhere.


================
Comment at: lldb/source/Target/Trace.cpp:109-110
+  StringRef load_address_str;
+  if (!module->GetValueForKeyAsString("loadAddress", load_address_str))
+    return SetMissingFieldError(error, "loadAddress", "string", *module);
+  addr_t load_address;
----------------
does JSON typically use camel case? Should this be "load-address"?


================
Comment at: lldb/source/Target/Trace.cpp:116-119
+  std::string full_path = m_info_dir;
+  full_path += "/";
+  full_path += file_path;
+  FileSpec file_spec(full_path);
----------------
We shouldn't assume that "file_path" is relative. This code should be something like:

```
FileSpec file_spec(file_path);
if (file_spec.IsRelative())
  file_spec.PrependPathComponent(m_info_dir);
```


================
Comment at: lldb/source/Target/Trace.cpp:127
+
+  ModuleSP module_sp(new Module(file_spec, ArchSpec()));
+  bool changed = true;
----------------
We should store a target triple as a mandatory key/value pair in the top level trace JSON file and access via a getter. Then we should also fill out a ModuleSpec with a path, UUID (optional) and architecture to make sure we don't end up getting the wrong file:
 
```
ModuleSpec module_spec;
module_spec.GetFileSpec() = file_spec;
module_spec.GetArchitecture() = Trace::GetArchitecture(); // This would be a new accessor that will hand out a architecture for the "triple" setting in the JSON trace settings
StringRef uuid_str;
if (module->GetValueForKeyAsString("uuid", uuid_str))
  module_spec.GetUUID().SetFromStringRef(uuid_str);
lldb::ModuleSP module_sp = target_sp->GetOrCreateModule(module_spec, false /* notify */, &error);
```

We wouldn't want to accidentally load "/usr/lib/libc.so" on a different machine with the wrong architecture since "libc.so" can exist on many systems.




================
Comment at: lldb/source/Target/Trace.cpp:131
+                            changed);
+  target_sp->GetImages().Append(module_sp);
+  return true;
----------------
Remove this since we create the module above already in the target in my inline comment code changes.


================
Comment at: lldb/source/Target/Trace.cpp:181
+
+bool Trace::ParseProcesses(Debugger &debugger, StructuredData::Dictionary &dict,
+                           Status &error) {
----------------
This should be more generic like:

```
Trace::ParseSettings(...)
```

We will add more top level key/value pairs that follow a structured data schema which we should make available through a command eventually, so lets not limit this to just processes. We might add architecture or target triple, and much more in the future.


================
Comment at: lldb/source/Target/Trace.cpp:198
+    return false;
+  return InitializeInstance(debugger, error);
+}
----------------
Maybe we should confine all plug-in specific settings to a key/value pair where the key is "arguments" or "plug-in-arguments" and the value is a dictionary. This will help ensure that no plug-in specific settings can ever conflict.
```
{ 
  "type": "intel-pt",
  "processes": [...],
  ....  (other settings that are for this "lldb_private::Trace::ParseSettings(...)")
  "arguments": {
      ... (plug-in specific arguments go here)
  }
}
```

Or maybe a structure like:

```
{
  "plug-in": {
    "name": "intel-pt",
    "arguments: {
      ... (TraceIntelPT plug-in specific arguments go here)
    }
  }
  "processes": [...],
  "triple": "armv7-apple-ios",
  ....  (other settings that are for this "lldb_private::Trace::ParseSettings(...)")
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705