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