[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.

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 inlined comments.



================
Comment at: lldb/source/Target/Trace.cpp:59
+  "plugin": <<<plugin_schema>>>,
+  "triple": string, // llvm-triple
+  "processes": [
----------------
Do we want an optional "trace-file" here in case trace files can contain all data for all processes?


================
Comment at: lldb/source/Target/Trace.cpp:62
+    {
+      "pid": integer,
+      "threads": [
----------------
Do we want an optional "trace-file" here in case trace files can contain all data for a single process?




================
Comment at: lldb/source/Target/Trace.cpp:66
+          "tid": integer,
+          "traceFile": string // path to trace file relative to the settings file
+        }
----------------
Do all trace files have one file per thread? Should this be "trace-file" instead of "traceFile"? 

Might make sense to make this optional and also include an optional traceFile at the process level and possibly one at the root level? See inline comments above


================
Comment at: lldb/source/Target/Trace.cpp:71
+        {
+          "file": string, // path to trace file relative to the settings file
+          "loadAddress": string, // address in hex or decimal form
----------------
This should probably be optional as we won't always have the file copied into the trace directory?

Do we want a "system-path" for the original path here?


================
Comment at: lldb/source/Target/Trace.cpp:72
+          "file": string, // path to trace file relative to the settings file
+          "loadAddress": string, // address in hex or decimal form
+          "uuid"?: string,
----------------
"load-address" instead of "loadAddress" and should be an integer instead of a string.


================
Comment at: lldb/source/Target/Trace.cpp:114-123
+bool Trace::ExtractDictionaryFromArray(const StructuredData::Array &array,
+                                       size_t index,
+                                       StructuredData::Dictionary *&dict,
+                                       Status &error) {
+  StructuredData::ObjectSP item = array.GetItemAtIndex(index);
+  dict = item->GetAsDictionary();
+  if (!dict)
----------------
This entire function should go into the StructuredData::Array class for re-use


================
Comment at: lldb/source/Target/Trace.cpp:125-133
+bool Trace::ExtractOptionalStringFromDictionary(
+    const StructuredData::Dictionary &dict, const char *field, StringRef &value,
+    Status &error) {
+  if (!dict.HasKey(field))
+    return true;
+  if (dict.GetValueForKeyAsString(field, value))
+    return true;
----------------
This entire function should go into StructuredData::Dictionary so other code can re-use this.


================
Comment at: lldb/source/Target/Trace.cpp:139
+  lldb::tid_t tid;
+  if (!thread.GetValueForKeyAsInteger("tid", tid))
+    return SetMissingFieldError(error, "tid", "integer", thread);
----------------
Seems like we should modify GetValueForKeyAsInteger to take a pointer to an error as the 3rd parameter here? Then that can return an appropriate error in case there is a "tid" entry, but it isn't an integer.




================
Comment at: lldb/source/Target/Trace.cpp:151
+  StructuredData::Array *threads = nullptr;
+  if (!process.GetValueForKeyAsArray("threads", threads))
+    return SetMissingFieldError(error, "threads", "array", process);
----------------
Seems like we should modify GetValueForKeyAsArray to take an pointer to an error as the 3rd parameter here? Then that can return an appropriate error in case there is a "threads" entry, but it isn't an array.


================
Comment at: lldb/source/Target/Trace.cpp:156
+    StructuredData::Dictionary *thread = nullptr;
+    if (!ExtractDictionaryFromArray(*threads, i, thread, error))
+      return false;
----------------
This functionality should be moved into StructuredData::Array::ExtractItemAsArray(...)


================
Comment at: lldb/source/Target/Trace.cpp:168
+  StringRef load_address_str;
+  if (!module.GetValueForKeyAsString("loadAddress", load_address_str))
+    return SetMissingFieldError(error, "loadAddress", "string", module);
----------------
"load-address" might be a better identifier and it is also weird to store an address as a string. Should be an integer.


================
Comment at: lldb/source/Target/Trace.cpp:193-194
+
+  ModuleSP module_sp =
+      target_sp->GetOrCreateModule(module_spec, /*notify*/ false, &error);
+  return error.Success();
----------------
This will need to be able to fail gracefully here. We might not always have the module file on disk. We might need to create a Placeholder module like the ProcessMinidump does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705