[LLDB][Statistics] Add coreFilePath to Target stats #161448
Conversation
…idate it's included in the stats output
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesThis patch adds the coreFilePath, if applicable, to the target statistics for LLDB. My primary motivation here is sanity check user reports when they say I had an issue with a specific corefile and then validating it was in fact that specifies corefile, as right now there is no guaruntee the filename and the process/target name will be the same. Full diff: https://github.com/llvm/llvm-project/pull/161448.diff 3 Files Affected:
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 8ad8d507268e2..480c1637286b3 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -202,6 +202,10 @@ TargetStats::ToJSON(Target &target,
if (process_sp->GetDynamicLoader())
dyld_plugin_name = process_sp->GetDynamicLoader()->GetPluginName();
target_metrics_json.try_emplace("dyldPluginName", dyld_plugin_name);
+
+ if (process_sp->GetCoreFile())
+ target_metrics_json.try_emplace("coreFilePath",
+ process_sp->GetCoreFile().GetPath());
}
target_metrics_json.try_emplace("sourceMapDeduceCount",
m_source_map_deduce_count);
diff --git a/lldb/test/API/functionalities/stats_api/TestStatisticsAPI.py b/lldb/test/API/functionalities/stats_api/TestStatisticsAPI.py
index f06c9ae14bb7a..69d0f183586b4 100644
--- a/lldb/test/API/functionalities/stats_api/TestStatisticsAPI.py
+++ b/lldb/test/API/functionalities/stats_api/TestStatisticsAPI.py
@@ -1,6 +1,8 @@
# Test the SBAPI for GetStatistics()
import json
+import os
+
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -157,3 +159,24 @@ def test_command_stats_force(self):
stats_force.GetAsJSON(stream_force)
debug_stats_force = json.loads(stream_force.GetData())
self.assertEqual(debug_stats_force["totalDebugInfoByteSize"], 445)
+
+ def test_command_stats_coredump(self):
+ """
+ Test to see if the coredump path is included in statistics dump.
+ """
+ yaml_file = "minidump.yaml"
+ minidump_path = self.getBuildArtifact(os.path.basename(yaml_file) + ".dmp")
+ self.yaml2obj(yaml_file, minidump_path)
+ target = self.dbg.CreateTarget(None)
+ process = target.LoadCore(minidump_path)
+ self.assertTrue(process.IsValid())
+
+ stats_options = lldb.SBStatisticsOptions()
+ stats = target.GetStatistics(stats_options)
+ stream = lldb.SBStream()
+ stats.GetAsJSON(stream)
+ debug_stats = json.loads(stream.GetData())
+ self.assertTrue("targets" in debug_stats)
+ target_info = debug_stats["targets"][0]
+ self.assertTrue("coreFilePath" in target_info)
+ self.assertEqual(target_info["coreFilePath"], minidump_path)
diff --git a/lldb/test/API/functionalities/stats_api/minidump.yaml b/lldb/test/API/functionalities/stats_api/minidump.yaml
new file mode 100644
index 0000000000000..d04ca1ae0dc12
--- /dev/null
+++ b/lldb/test/API/functionalities/stats_api/minidump.yaml
@@ -0,0 +1,38 @@
+--- !minidump
+Streams:
+ - Type: SystemInfo
+ Processor Arch: AMD64
+ Processor Level: 6
+ Processor Revision: 15876
+ Number of Processors: 40
+ Platform ID: Linux
+ CSD Version: 'Linux 3.13.0-91-generic'
+ CPU:
+ Vendor ID: GenuineIntel
+ Version Info: 0x00000000
+ Feature Info: 0x00000000
+ - Type: ThreadList
+ Threads:
+ - Thread Id: 0x2896BB
+ Context: 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000700100000000000FFFFFFFF0000FFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B040A812FF7F00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000050D0A75BBA7F00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+ Stack:
+ Start of Memory Range: 0x0
+ Content: ''
+ - Type: Memory64List
+ Memory Ranges:
+ - Start of Memory Range: 0x1000
+ Data Size: 0x100
+ Content : ''
+ - Start of Memory Range: 0x2000
+ Data Size: 0x20
+ Content : ''
+ - Start of Memory Range: 0x3000
+ Data Size: 0x400
+ Content : ''
+ - Start of Memory Range: 0x5000
+ Data Size: 0x500
+ Content : ''
+ - Start of Memory Range: 0x5500
+ Data Size: 0x500
+ Content : ''
+...
|
| if (process_sp->GetCoreFile()) | ||
| target_metrics_json.try_emplace("coreFilePath", | ||
| process_sp->GetCoreFile().GetPath()); |
There was a problem hiding this comment.
Is this going to add the full file path always? Do we want that or just the basename? Can we make sure it's clear here and in the test?
There was a problem hiding this comment.
I follow the same naming as the FileSpec with just Path. It should be a normalized path either relative or full to the coredump, we could specify the full path but I don't see when/how the path would be relative.
There was a problem hiding this comment.
FWIW I don't think we should ever include full paths as that can contain PII such as the name of the home directory.
There was a problem hiding this comment.
FWIW I don't think we should ever include full paths as that can contain PII such as the name of the home directory.
Recommendations on this patch then? I personally prefer the path but I never considered the privacy concerns, which makes sense for Apple. I'd be comfortable compromising with the file name.
There was a problem hiding this comment.
To be clear: we're not collecting these statistics, but if we were this would become an issue.
Using the basename seems like a good compromise. If you somehow really need a path, you can try to obfuscate the home directory but that seems overkill here.
As a concrete example, crash logs on our platform remove everything after /Users/ and before the basename, so something like /Users/jonas/Downloads/a.out becomes /Users/USER/a.out. However, that begs the question of how meaningful that path still is...
This patch adds the coreFilePath, if applicable, to the target statistics for LLDB. My primary motivation here is sanity check user reports when they say I had an issue with a specific corefile and then validating it was in fact that specifies corefile, as right now there is no guaruntee the filename and the process/target name will be the same.