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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
