[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -186,7 +186,19 @@ class SymbolFileDWARF : public SymbolFileCommon {
GetMangledNamesForFunction(const std::string _qualified_name,
std::vector _names) override;
- uint64_t GetDebugInfoSize() override;
+ /// Get total currently loaded debug info size or total possible debug info
+ /// size.
+ ///
+ /// For cases like .dwo files, the debug info = skeleton debug info +
+ /// all dwo debug info where .dwo files might not be loaded yet. Calling this
+ /// function by default will NOT force the loading of any .dwo files.
+ ///
+ /// \param load_all_debug_info
+ /// If true, force loading any .dwo files associated and add to the size
+ ///
+ /// \return
+ /// Returns total currently loaded debug info size
clayborg wrote:
We don't need headerdoc on overridden functions. You can remove this.
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
https://github.com/clayborg commented: Just remove `static ProgressManager ();` and inline the code into `static ProgressManager ();` and this is good to go. https://github.com/llvm/llvm-project/pull/81319 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +67,41 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+ProgressManager ::InstanceImpl() {
+ static std::once_flag g_once_flag;
+ static ProgressManager *g_progress_manager = nullptr;
+ std::call_once(g_once_flag, []() {
+// NOTE: known leak to avoid global destructor chain issues.
+g_progress_manager = new ProgressManager();
+ });
+ return *g_progress_manager;
+}
clayborg wrote:
We probably don't need this function and we can just inline the contents into
the `static ProgressManager ::Instance()` right?
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +67,37 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+ProgressManager ::InstanceImpl() {
+ static std::once_flag g_once_flag;
+ static ProgressManager *g_progress_manager = nullptr;
+ std::call_once(g_once_flag, []() {
+// NOTE: known leak to avoid global destructor chain issues.
+g_progress_manager = new ProgressManager();
+ });
+ return *g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_category_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return InstanceImpl(); }
clayborg wrote:
Remove `ProgressManager ::InstanceImpl()` and inline the code
from that function in this function.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -119,6 +120,32 @@ class Progress {
bool m_complete = false;
};
+/// A class used to group progress reports by category. This is done by using a
+/// map that maintains a refcount of each category of progress reports that
have
+/// come in. Keeping track of progress reports this way will be done if a
+/// debugger is listening to the eBroadcastBitProgressByCategory broadcast bit.
+class ProgressManager {
+public:
+ ProgressManager();
+ ~ProgressManager();
+
+ static void Initialize();
+ static void Terminate();
+ // Control the refcount of the progress report category as needed
+ void Increment(std::string category);
+ void Decrement(std::string category);
+
+ // Public accessor for the class instance
+ static ProgressManager ();
+
+private:
+ // Manage the class instance internally using a std::optional
clayborg wrote:
Is this comment out of date? I don't think we are using `std::optional`
anymore
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81319 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
clayborg wrote: I am fine with this patch if we fix the Scalar class to obey the bit width as suggested above. I do worry though that other Scalar values in the expression might get set to 64 bit values all of the time and cause problems with DWARF expressions. Do you have a code snippet of where this is used where you rely on the bit width? Usually `DW_OP_*` opcodes act on a pointer width integer, but some do actually specify a width (some of the deref operations). https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > > I am fine with telling people what to do and giving them a golden path to > > what is easiest for our debuggers. And I will suggest to everyone that they > > use `.debug` and `.dwp`, but if we want to only support this, this leaves > > the downloading of the `.debug` file requiring a rename from `.dwp` to > > `.debug.dwp` in order for it to work for people. So then everything in this > > patch should be supported to allow loading the `.debug` file with a `.dwp` > > like we will encourage people to do. > > Not sure I follow - one of the scenarios mentioned in this patch is > > "lldb loads which is stripped but has .gnu_debuglink pointing to .debug with > skeleton DWARF and needs to find .debug.dwp" > > I don't think we should support that, for instance - we should load > `.dwp` in that case. We currently have this happening here at Facebook all over the place. We have tools that convert DWARF to GSYM and they don't need the stripped executable at all, they > > > It would also be nice if we do have a single `.debug` file that has debug > > info only, it would be nice to allow it and the `.dwp` file to be combined > > into a single file. There is no reason for them to be separate anymore once > > we have `a.out` stripped, it would be nice to only require `a.out.debug` > > which contains the `.dwp` sections inside of it already instead of > > requiring people to have two files needed for debug info. > > Maybe? I figure once you've got to download one file, two isn't a substantial > imposition... - it'd be a bit weird having a DWP file and a .debug file > mashed up together, but can't see any reason it wouldn't work - with the > logic of "check if this program has a cu_index in it, if so, treat it as a > dwp, otherwise look for .dwp, otherwise look for the dwos". > > > > I am fine with telling people what to do and giving them a golden path > > > > to what is easiest for our debuggers. And I will suggest to everyone > > > > that they use `.debug` and `.dwp`, but if we want to only support this, > > > > this leaves the downloading of the `.debug` file requiring a rename > > > > from `.dwp` to `.debug.dwp` in order for it to work for people. So then > > > > everything in this patch should be supported to allow loading the > > > > `.debug` file with a `.dwp` like we will encourage people to do. > > > > > > > > > Not sure I follow - one of the scenarios mentioned in this patch is > > > "lldb loads which is stripped but has .gnu_debuglink pointing to .debug > > > with skeleton DWARF and needs to find .debug.dwp" > > > I don't think we should support that, for instance - we should load > > > `.dwp` in that case. > > > > > > If the client strips the debug info first into "a.out.debug" and then runs > > llvm-dwp, they will end up with a "a.out.debug.dwp". We have clients that > > are doing this already and we want to support them. > > OK, could we fix llvm-dwp to match the behavior, then? If the file has a > .debug extension, strip that and add the .dwp extension. Here people are not using ".debug", but are using ".debuginfo"... Again, nothing is enforced and people are left to use llvm-objcopy + llvm-dwp how ever they want. Getting a solution that does everything might be nice. Any thoughts on modifying llvm-dwp to be able to do all of this and provide some path for people where it can either just create a .dwp file for a given executable _or_ it can create a `.debug` file, strip the original file, and create a `.dwp` file? > > > The compiler and linker drivers are staying out of this and we expect > > people to do this on their own, so this is what we end up with when there > > is no enforcement. > > They aren't doing it on their own though - they're using llvm-dwp and its > defaults (they're passing it a .debug file and getting a .debug.dwp file - > it's the defaults you/we are worried about, and how to make other tools work > well with those defaults). We can change those defaults if they don't work > well/don't create a consistent environment. > > > I am not sure why this is such a sticking point. Lets make the debugger > > work for people. > > As I explained above - my concern is that supporting a wider variety of ways > these files can be named/arranged means more variants that need to be > supported across a variety of tooling (symbolizers and debuggers - not just > LLVM's but binutils, etc too). > > But that's my 2c - if LLDB owners prefer this direction, so be it. Wouldn't > mind hearing some other people's perspectives on the issues around limiting > variation here. I am happy to hear any other opinions as well. I tend to want to make my life easier and ease the support burden I run into everyday where people that know nothing about split DWARF are trying to use it and failing and require tech support to make it work for them. I am happy to suggest a path to follow, in fact I am going to write up the best
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg updated
https://github.com/llvm/llvm-project/pull/81067
>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH 1/5] Add more ways to find the .dwp file.
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
---
lldb/include/lldb/Utility/FileSpecList.h | 4 ++
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 61 +--
.../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
3 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Utility/FileSpecList.h
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b..6eb3bb9971f13 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
const_iterator begin() const { return m_files.begin(); }
const_iterator end() const { return m_files.end(); }
+ llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+ }
+
protected:
collection m_files; ///< A collection of FileSpec objects.
};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a43677..487961fa7437f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap
*SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr ::GetDwpSymbolFile()
{
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+ symfiles.Append(symfile_fspec);
+} else {
+ // If we don't have a separate debug info file, then try stripping the
+ // extension. We main module could be "a.debug" and the .dwp file could
be
+ // "a.dwp" instead of "a.debug.dwp".
+ ConstString filename_no_ext =
+ module_fspec.GetFileNameStrippingExtension();
+ if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+ }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), _filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (!dwp_obj_file)
-return;
- m_dwp_symfile = std::make_shared(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto : symfiles.files()) {
+ module_spec.GetSymbolFileSpec() =
+ FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
+ FileSpec dwp_filespec =
+ PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+ if (FileSystem::Instance().Exists(dwp_filespec)) {
+DataBufferSP dwp_file_data_sp;
+lldb::offset_t dwp_file_data_offset = 0;
+ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+GetObjectFile()->GetModule(), _filespec, 0,
+FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -1419,6 +1419,10 @@ let Command = "statistics dump" in {
def statistics_dump_all: Option<"all-targets", "a">, Group<1>,
Desc<"Include statistics for all targets.">;
def statistics_dump_summary: Option<"summary", "s">, Group<1>,
-Desc<"Dump only high-level summary statistics."
+Desc<"Dump only high-level summary statistics. "
"Exclude targets, modules, breakpoints etc... details.">;
+ def statistics_dump_force: Option<"force", "f">, Group<1>,
clayborg wrote:
How about `--load-all-debug-info`? `--force` doesn't clearly indicate what it
is forcing without reading the help
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -391,7 +392,14 @@ class SymbolFile : public PluginInterface {
/// entire file should be returned. The default implementation of this
/// function will iterate over all sections in a module and add up their
/// debug info only section byte sizes.
- virtual uint64_t GetDebugInfoSize() = 0;
+ ///
+ /// \param load_if_needed
+ /// If true, force loading any symbol files if they are not yet loaded and
+ /// add to the total size
+ ///
+ /// \returns
+ /// Total currently loaded debug info size in bytes
+ virtual uint64_t GetDebugInfoSize(bool load_if_needed = false) = 0;
clayborg wrote:
Maybe `load_all_debug_info` is more clear? It would be nice if this matches the
named inside `StatisticsOptions` as well.
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -186,7 +186,19 @@ class SymbolFileDWARF : public SymbolFileCommon {
GetMangledNamesForFunction(const std::string _qualified_name,
std::vector _names) override;
- uint64_t GetDebugInfoSize() override;
+ /// Get total currently loaded debug info size or total possible debug info
+ /// size.
+ ///
+ /// For cases like .dwo files, the debug info = skeleton debug info +
+ /// all dwo debug info where .dwo files might not be loaded yet. Calling this
+ /// function by default will NOT force the loading of any .dwo files.
+ ///
+ /// \param load_if_needed
+ /// If true, force loading any .dwo files associated and add to the size
+ ///
+ /// \return
+ /// Returns total currently loaded debug info size
clayborg wrote:
no need to duplicate the headerdoc in subclasses. Only SymbolFile.h needs the
headerdoc.
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -132,6 +132,7 @@ struct ConstStringStats {
struct StatisticsOptions {
bool summary_only = false;
+ bool force_loading = false;
clayborg wrote:
Maybe `load_all_debug_info` is more clear?
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -24,6 +24,9 @@ class LLDB_API SBStatisticsOptions {
void SetSummaryOnly(bool b);
bool GetSummaryOnly();
+
+ void SetForceLoading(bool b);
clayborg wrote:
Need headerdoc for these.
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -24,6 +24,9 @@ class LLDB_API SBStatisticsOptions {
void SetSummaryOnly(bool b);
bool GetSummaryOnly();
+
+ void SetForceLoading(bool b);
+ bool GetForceLoading();
clayborg wrote:
The `SetForceLoading` name doesn't clearly indicate to us what we are force
loading just by the API name. How about:
```
/// If set to true, the debugger will load all debug info that is available
/// and report statistics on the total amount. If this is set to false, then
/// only report statistics on the currently loaded debug information.
/// This can avoid loading debug info from separate files just so it can
/// report the total size which can slow down statistics reporting.
void SetReportAllAvailableDebugInfo(bool b);
bool SetReportAllAvailableDebugInfo();
```
This setting would default to `false`.
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > > I am fine with telling people what to do and giving them a golden path to > > what is easiest for our debuggers. And I will suggest to everyone that they > > use `.debug` and `.dwp`, but if we want to only support this, this leaves > > the downloading of the `.debug` file requiring a rename from `.dwp` to > > `.debug.dwp` in order for it to work for people. So then everything in this > > patch should be supported to allow loading the `.debug` file with a `.dwp` > > like we will encourage people to do. > > Not sure I follow - one of the scenarios mentioned in this patch is > > "lldb loads which is stripped but has .gnu_debuglink pointing to .debug with > skeleton DWARF and needs to find .debug.dwp" > > I don't think we should support that, for instance - we should load > `.dwp` in that case. If the client strips the debug info first into "a.out.debug" and then runs llvm-dwp, they will end up with a "a.out.debug.dwp". We have clients that are doing this already and we want to support them. The compiler and linker drivers are staying out of this and we expect people to do this on their own, so this is what we end up with when there is no enforcement. I am not sure why this is such a sticking point. Lets make the debugger work for people. > > > It would also be nice if we do have a single `.debug` file that has debug > > info only, it would be nice to allow it and the `.dwp` file to be combined > > into a single file. There is no reason for them to be separate anymore once > > we have `a.out` stripped, it would be nice to only require `a.out.debug` > > which contains the `.dwp` sections inside of it already instead of > > requiring people to have two files needed for debug info. > > Maybe? I figure once you've got to download one file, two isn't a substantial > imposition... - it'd be a bit weird having a DWP file and a .debug file > mashed up together, but can't see any reason it wouldn't work - with the > logic of "check if this program has a cu_index in it, if so, treat it as a > dwp, otherwise look for .dwp, otherwise look for the dwos". Just more things that can go wrong for clients as they try to use split DWARF. But this fix isn't about that, that was just a tangent. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
@@ -1666,33 +1646,52 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = {};
+std::optional pass_action = {};
+std::optional notify_action = {};
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
- result.AppendError("Invalid argument for command option --stop; must be "
- "true or false.\n");
- return;
+if (!m_options.stop.empty()) {
+ bool success = false;
+ bool value = OptionArgParser::ToBoolean(m_options.stop, false, );
clayborg wrote:
That is fine.
https://github.com/llvm/llvm-project/pull/79901
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
https://github.com/clayborg approved this pull request. Looks good! https://github.com/llvm/llvm-project/pull/81319 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb-dap] Followup fixs for data breakpoints (PR #81680)
@@ -2757,13 +2769,18 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
body.try_emplace("description", error_cstr && error_cstr[0]
? std::string(error_cstr)
: "evaluation failed");
-} else
- addr = llvm::utohexstr(value.GetValueAsUnsigned());
+} else {
+ uint64_t value_as_unsigned = value.GetValueAsUnsigned();
+ if (value_as_unsigned == 0) {
+body.try_emplace("dataId", nullptr);
+body.try_emplace("description",
+ "unable to evaluate expression to an address.");
+ }
+ addr = llvm::utohexstr(value_as_unsigned);
+}
} else {
-auto state = g_dap.target.GetProcess().GetState();
body.try_emplace("dataId", nullptr);
-body.try_emplace("description",
- "variable not found: " + llvm::utostr(state));
+body.try_emplace("description", "variable not found");
clayborg wrote:
Do we want more info shown here like the variable name that we tried to lookup?
https://github.com/llvm/llvm-project/pull/81680
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb-dap] Followup fixs for data breakpoints (PR #81680)
@@ -2757,13 +2769,18 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
body.try_emplace("description", error_cstr && error_cstr[0]
? std::string(error_cstr)
: "evaluation failed");
-} else
- addr = llvm::utohexstr(value.GetValueAsUnsigned());
+} else {
+ uint64_t value_as_unsigned = value.GetValueAsUnsigned();
+ if (value_as_unsigned == 0) {
+body.try_emplace("dataId", nullptr);
+body.try_emplace("description",
+ "unable to evaluate expression to an address.");
+ }
+ addr = llvm::utohexstr(value_as_unsigned);
+}
clayborg wrote:
For any address we come up with, it is probably a good idea to find the memory
region that contains it and verify we have at least some permissions. You can
take any uint64_t address and find the region by doing:
```
uint64_t load_addr = ...;
lldb::SBMemoryRegionInfo region;
lldb::SBError err = g_dap.target.GetProcess().GetMemoryRegionInfo(load_addr,
region);
if (err.Success()) {
if (!(region.IsReadable() || region.IsWritable())) {
body.try_emplace("description", "memory region for address has no
read or write permissions");
}
} else {
body.try_emplace("description", "unable to get memory region info for address
");
}
```
https://github.com/llvm/llvm-project/pull/81680
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb-dap] Followup fixs for data breakpoints (PR #81680)
@@ -2757,13 +2769,18 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
body.try_emplace("description", error_cstr && error_cstr[0]
? std::string(error_cstr)
: "evaluation failed");
-} else
- addr = llvm::utohexstr(value.GetValueAsUnsigned());
+} else {
+ uint64_t value_as_unsigned = value.GetValueAsUnsigned();
+ if (value_as_unsigned == 0) {
+body.try_emplace("dataId", nullptr);
+body.try_emplace("description",
+ "unable to evaluate expression to an address.");
+ }
clayborg wrote:
Avoid this and do the memory region code I show below to verify the address in
somewhere we can access.
https://github.com/llvm/llvm-project/pull/81680
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb-dap] Add support for data breakpoint. (PR #81541)
@@ -2697,58 +2737,41 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
GetUnsigned(arguments, "variablesReference", 0);
llvm::StringRef name = GetString(arguments, "name");
lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments);
- bool is_duplicated_variable_name = name.contains(" @");
+ lldb::SBValue variable = FindVariable(variablesReference, name);
+ std::string addr, size;
- lldb::SBValue variable;
- if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
-// variablesReference is one of our scopes, not an actual variable it is
-// asking for a variable in locals or globals or registers
-int64_t end_idx = top_scope->GetSize();
-// Searching backward so that we choose the variable in closest scope
-// among variables of the same name.
-for (int64_t i = end_idx - 1; i >= 0; --i) {
- lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
- std::string variable_name = CreateUniqueVariableNameForDisplay(
- curr_variable, is_duplicated_variable_name);
- if (variable_name == name) {
-variable = curr_variable;
-break;
- }
-}
+ if (variable.IsValid()) {
+addr = llvm::utohexstr(variable.GetLoadAddress());
+size = llvm::utostr(variable.GetByteSize());
+ } else if (variablesReference == 0 && frame.IsValid()) {
+// Name might be an expression. In this case we assume that name is
composed
+// of the number of bytes to watch and expression, separated by '@':
+// "${size}@${expression}"
clayborg wrote:
Is this format VS Code supported, or just something we are making up?
https://github.com/llvm/llvm-project/pull/81541
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb-dap] Add support for data breakpoint. (PR #81541)
@@ -2697,58 +2737,41 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
GetUnsigned(arguments, "variablesReference", 0);
llvm::StringRef name = GetString(arguments, "name");
lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments);
- bool is_duplicated_variable_name = name.contains(" @");
+ lldb::SBValue variable = FindVariable(variablesReference, name);
+ std::string addr, size;
- lldb::SBValue variable;
- if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
-// variablesReference is one of our scopes, not an actual variable it is
-// asking for a variable in locals or globals or registers
-int64_t end_idx = top_scope->GetSize();
-// Searching backward so that we choose the variable in closest scope
-// among variables of the same name.
-for (int64_t i = end_idx - 1; i >= 0; --i) {
- lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
- std::string variable_name = CreateUniqueVariableNameForDisplay(
- curr_variable, is_duplicated_variable_name);
- if (variable_name == name) {
-variable = curr_variable;
-break;
- }
-}
+ if (variable.IsValid()) {
+addr = llvm::utohexstr(variable.GetLoadAddress());
+size = llvm::utostr(variable.GetByteSize());
clayborg wrote:
Need to check if the byte size is not zero and return an error if it is.
https://github.com/llvm/llvm-project/pull/81541
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb-dap] Add support for data breakpoint. (PR #81541)
@@ -2697,58 +2737,41 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
GetUnsigned(arguments, "variablesReference", 0);
llvm::StringRef name = GetString(arguments, "name");
lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments);
- bool is_duplicated_variable_name = name.contains(" @");
+ lldb::SBValue variable = FindVariable(variablesReference, name);
+ std::string addr, size;
- lldb::SBValue variable;
- if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
-// variablesReference is one of our scopes, not an actual variable it is
-// asking for a variable in locals or globals or registers
-int64_t end_idx = top_scope->GetSize();
-// Searching backward so that we choose the variable in closest scope
-// among variables of the same name.
-for (int64_t i = end_idx - 1; i >= 0; --i) {
- lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
- std::string variable_name = CreateUniqueVariableNameForDisplay(
- curr_variable, is_duplicated_variable_name);
- if (variable_name == name) {
-variable = curr_variable;
-break;
- }
-}
+ if (variable.IsValid()) {
+addr = llvm::utohexstr(variable.GetLoadAddress());
+size = llvm::utostr(variable.GetByteSize());
+ } else if (variablesReference == 0 && frame.IsValid()) {
+// Name might be an expression. In this case we assume that name is
composed
+// of the number of bytes to watch and expression, separated by '@':
+// "${size}@${expression}"
+llvm::StringRef expr;
+std::tie(size, expr) = name.split('@');
+lldb::SBValue value = frame.EvaluateExpression(expr.data());
+if (value.GetError().Fail()) {
+ lldb::SBError error = value.GetError();
+ const char *error_cstr = error.GetCString();
+ body.try_emplace("dataId", nullptr);
+ body.try_emplace("description", error_cstr && error_cstr[0]
+ ? std::string(error_cstr)
+ : "evaluation failed");
+} else
+ addr = llvm::utohexstr(value.GetValueAsUnsigned());
clayborg wrote:
We probably want to check this address by making sure it is valid. What if this
return zero?
https://github.com/llvm/llvm-project/pull/81541
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb-dap] Add support for data breakpoint. (PR #81541)
@@ -561,6 +561,46 @@ void EventThreadFunction() {
}
}
+lldb::SBValue FindVariable(uint64_t variablesReference, llvm::StringRef name) {
+ lldb::SBValue variable;
+ if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
+bool is_duplicated_variable_name = name.contains(" @");
+// variablesReference is one of our scopes, not an actual variable it is
+// asking for a variable in locals or globals or registers
+int64_t end_idx = top_scope->GetSize();
+// Searching backward so that we choose the variable in closest scope
+// among variables of the same name.
+for (int64_t i = end_idx - 1; i >= 0; --i) {
+ lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
+ std::string variable_name = CreateUniqueVariableNameForDisplay(
+ curr_variable, is_duplicated_variable_name);
clayborg wrote:
Do we want to check the name that might include the "foo @ main.cpp:12" or just
look for "foo"? Also, some members of a struct be anonymous unions, we probably
want to make sure we can do the right thing with those
https://github.com/llvm/llvm-project/pull/81541
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb-dap] Add support for data breakpoint. (PR #81541)
@@ -2697,58 +2737,41 @@ void request_dataBreakpointInfo(const
llvm::json::Object ) {
GetUnsigned(arguments, "variablesReference", 0);
llvm::StringRef name = GetString(arguments, "name");
lldb::SBFrame frame = g_dap.GetLLDBFrame(*arguments);
- bool is_duplicated_variable_name = name.contains(" @");
+ lldb::SBValue variable = FindVariable(variablesReference, name);
+ std::string addr, size;
- lldb::SBValue variable;
- if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
-// variablesReference is one of our scopes, not an actual variable it is
-// asking for a variable in locals or globals or registers
-int64_t end_idx = top_scope->GetSize();
-// Searching backward so that we choose the variable in closest scope
-// among variables of the same name.
-for (int64_t i = end_idx - 1; i >= 0; --i) {
- lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
- std::string variable_name = CreateUniqueVariableNameForDisplay(
- curr_variable, is_duplicated_variable_name);
- if (variable_name == name) {
-variable = curr_variable;
-break;
- }
-}
+ if (variable.IsValid()) {
+addr = llvm::utohexstr(variable.GetLoadAddress());
clayborg wrote:
Need some error checking here. `variable.GetLoadAddress()` will only return a
valid address if the variable is actually in memory. So if
`variable.GetLoadAddress()` returns LLDB_INVALID_ADDRESS we need to return an
appropriate error like " does not exist in memory, its location is
" where is the result of `const char
*SBValue::GetLocation()`.
https://github.com/llvm/llvm-project/pull/81541
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
clayborg wrote: What code is taking a scalar and then trying to byte swap it after it has been created? https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
clayborg wrote: > > This uses `DataExtractor::GetMaxU64` which already does this under the > > hood. What does this do that isn't already being done? It may help if you > > add a test case to show what you are trying to fix. > > @clayborg @bulbazord The problem with GetMaxU64 is that it always returns > uint64_t even though actual type was uint32_t, so when byteswap is performed > it becomes invalid integer, to fixed this we need to store correct bitwidth > not higher. i.e. Suppose there is actual 32 bit integer i.e. 0x > `GetMaxU64` will return 0x (prmoted to uint64_t from > uint32_t) and when performing byteswap on this it becomes 0x > which is invalid. So you are saying someone is taking the resulting Scalar and trying to byteswap it? Errors can still happen then in this class for int8_t/uint8_t and int16_t/uint16_t as there are no overloads for these in the `Scalar` class. We only currently have: ``` Scalar(int v); Scalar(unsigned int v); Scalar(long v); Scalar(unsigned long v); Scalar(long long v); Scalar(unsigned long long v); ``` It would suggest if we are going to make a requirement that Scalar will hold the correct bit widths, then we need to stop using "int" and "long" and switch to using the `int*_t` and `uint*_t` typedefs to make things clear: ``` Scalar(int8_t v); Scalar(int16_t v); Scalar(int32_t v); Scalar(int64_t v); Scalar(uint8_t v); Scalar(uint16_t v); Scalar(uint32_t v); Scalar(uint64_t v); ``` Then we have all of the bit widths handled correctly. https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
@@ -1666,33 +1646,52 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = {};
+std::optional pass_action = {};
+std::optional notify_action = {};
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
- result.AppendError("Invalid argument for command option --stop; must be "
- "true or false.\n");
- return;
+if (!m_options.stop.empty()) {
+ bool success = false;
+ bool value = OptionArgParser::ToBoolean(m_options.stop, false, );
clayborg wrote:
Why don't we do this error checking in
`CommandObjectProcessHandle::CommandOptions::SetOptionValue(...)`? We can
return an error from there instead of making it successfully though option
parsing only to look closer at the option values and return an error later in
this function?
Also, I see many places use this version of `OptionArgParser::ToBoolean(...)`,
and there is a ton of duplicated code like:
- call `OptionArgParser::ToBoolean(...)`
- check value of success
- make error string and everyone picks their own "hey this isn't a valid
boolean value (like Jim already mentioned below).
Since there are already so many uses of the current and only version of
`OptionArgParser::ToBoolean(...)`, maybe we make an overload of this:
```
llvm::Expected OptionArgParser::ToBoolean(llvm::StringRef ref);
```
Then in the `CommandObjectProcessHandle::CommandOptions::SetOptionValue(...)
function we can do:
```
case 's':
if (auto ExpectedBool = OptionArgParser::ToBoolean(option_arg)) {
stop = *ExpectedBool;
else
error = Status(ExpectedBool.takeError());
```
And the new `OptionArgParser::ToBoolean()` overload would return an error as
Jim wants below:
```
"Invalid value "%s"
```
Where %s gets substitued with "option_arg.str().c_str()". And then the help
would help people figure this out.
https://github.com/llvm/llvm-project/pull/79901
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -301,7 +301,8 @@ class ProcessGDBRemote : public Process, using FlashRange = FlashRangeVector::Entry; FlashRangeVector m_erased_flash_ranges; - bool m_vfork_in_progress; + // Number of vfork in process. + int m_vfork_in_progress; clayborg wrote: Do we really want a number that can go below zero? Doesn't make sense. I would store this as a `uint32_t` and never let it get decremented if it is zero. https://github.com/llvm/llvm-project/pull/81564 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,45 @@
+#include
+#include
+#include
+#include
+
+int call_vfork() {
+ printf("Before vfork\n");
+
+ pid_t child_pid = vfork();
+
+ if (child_pid == -1) {
+// Error handling
+perror("vfork");
+return 1;
+ } else if (child_pid == 0) {
+// This code is executed by the child process
+printf("Child process\n");
+_exit(0); // Exit the child process
+ } else {
+// This code is executed by the parent process
+printf("Parent process\n");
+ }
+
+ printf("After vfork\n");
+ return 0;
+}
+
+void worker_thread() { call_vfork(); }
+
+void create_threads(int num_threads) {
+ std::vector threads;
+ for (int i = 0; i < num_threads; ++i) {
+threads.emplace_back(std::thread(worker_thread));
+ }
+ printf("Created %d threads, joining...\n",
+ num_threads); // end_of_create_threads
+ for (auto : threads) {
+thread.join();
+ }
+}
+
+int main() {
+ int num_threads = 5; // break here
+ create_threads(num_threads);
+}
clayborg wrote:
Now reap the children you spawned:
```
std::lock_guard Lock(g_child_pids_mutex);
for (pid_t child_pid: g_child_pids) {
int child_status = 0;
pid_t pid = waitpid(child_pid, _status, 0);
if (child_status != 0)
_exit(1); // This will let our program know that some child processes
didn't exist with a zero exit status.
if (pid != child_pid)
_exit(2); // This will let our program know it didn't succeed
}
```
This will ensure that if we stayed with the parent, then we successfully
resumed all of the child processes and that they didn't crash and that they
exited. If we don't resume the child processes correctly, they can get caught
in limbo if they weren't resumed and this test will deadlock waiting for the
child process to exit. There are some options you can do that allows you to not
hang where instead of zero passed as the last parameter of `waitpid()`, you can
specify `WNOHANG`, but that will get racy. We would poll for a few seconds. We
will want to make sure that the test doesn't just deadlock and cause the test
suite to never complete.
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,45 @@
+#include
+#include
+#include
+#include
+
+int call_vfork() {
+ printf("Before vfork\n");
+
+ pid_t child_pid = vfork();
+
+ if (child_pid == -1) {
+// Error handling
+perror("vfork");
+return 1;
+ } else if (child_pid == 0) {
+// This code is executed by the child process
+printf("Child process\n");
+_exit(0); // Exit the child process
+ } else {
+// This code is executed by the parent process
+printf("Parent process\n");
clayborg wrote:
Add the pid to the global vector:
```
printf("Parent process\n");
std::lock_guard Lock(g_child_pids_mutex);
g_child_pids.append(child_pid);
```
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -5670,8 +5673,8 @@ void ProcessGDBRemote::DidVFork(lldb::pid_t child_pid,
lldb::tid_t child_tid) {
}
void ProcessGDBRemote::DidVForkDone() {
- assert(m_vfork_in_progress);
- m_vfork_in_progress = false;
+ --m_vfork_in_progress;
+ assert(m_vfork_in_progress >= 0);
clayborg wrote:
```
assert(m_vfork_in_progress > 0);
--m_vfork_in_progress;
```
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -5681,7 +5684,10 @@ void ProcessGDBRemote::DidVForkDone() {
void ProcessGDBRemote::DidExec() {
// If we are following children, vfork is finished by exec (rather than
// vforkdone that is submitted for parent).
- if (GetFollowForkMode() == eFollowChild)
-m_vfork_in_progress = false;
+ if (GetFollowForkMode() == eFollowChild) {
+if (m_vfork_in_progress > 0)
+ --m_vfork_in_progress;
+assert(m_vfork_in_progress >= 0);
clayborg wrote:
This assert does nothing as you won't decrement it if it is not `> 0`. Remove
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,31 @@
+"""
+Make sure that the concurrent vfork() from multiple threads works correctly.
+"""
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class TestConcurrentVFork(TestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipIfWindows
+def test_vfork_follow_parent(self):
+self.build()
+lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.cpp")
+)
+self.runCmd("settings set target.process.follow-fork-mode parent")
+self.expect("continue", substrs=["exited with status = 0"])
+
+@skipIfWindows
+def test_vfork_follow_child(self):
+self.build()
+lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.cpp")
+)
+self.runCmd("settings set target.process.follow-fork-mode child")
+self.expect("continue", substrs=["exited with status = 0"])
clayborg wrote:
Can we check for a output from our process that matches "Child process" to
verify it worked as expected?
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,45 @@ +#include +#include +#include +#include + clayborg wrote: Might be a good idea to create a global `std::vector` and a mutex to protect it. And any child processes that get forked get added to this vector. So maybe add: ``` std::mutex g_child_pids_mutex; std::vector g_child_pids; ``` I will comment below where to use it. https://github.com/llvm/llvm-project/pull/81564 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -120,15 +120,25 @@ bool NativeThreadLinux::GetStopReason(ThreadStopInfo
_info,
case eStateCrashed:
case eStateExited:
case eStateSuspended:
- case eStateUnloaded:
+ case eStateUnloaded: {
if (log)
LogThreadStopInfo(*log, m_stop_info, "m_stop_info in thread:");
stop_info = m_stop_info;
description = m_stop_description;
if (log)
LogThreadStopInfo(*log, stop_info, "returned stop_info:");
+// Include child process PID/TID for forks.
+// Client expects " " format.
+if (stop_info.reason == eStopReasonFork ||
+stop_info.reason == eStopReasonVFork) {
+ description = std::to_string(stop_info.details.fork.child_pid);
+ description += " ";
+ description += std::to_string(stop_info.details.fork.child_tid);
clayborg wrote:
Why are we adding it here and not changing the original "m_stop_description" to
contain it? We want everyone to get all of the information.
If this is a human readable string, we should probably print out something like:
```
child-pid = 12345, child-tid = 345645
```
instead of two magic numbers separate by a space.
Or is this output used in the GDB remote packets somehow? Don't we need to add
the child pid and tid to the JSON stop info for the other threads in a
key/value pair encoding?
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,31 @@
+"""
+Make sure that the concurrent vfork() from multiple threads works correctly.
+"""
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class TestConcurrentVFork(TestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipIfWindows
+def test_vfork_follow_parent(self):
+self.build()
+lldbutil.run_to_source_breakpoint(
+self, "// break here", lldb.SBFileSpec("main.cpp")
+)
+self.runCmd("settings set target.process.follow-fork-mode parent")
+self.expect("continue", substrs=["exited with status = 0"])
clayborg wrote:
Can we check for a output from our process that matches "Parent process" to
verify it worked as expected?
https://github.com/llvm/llvm-project/pull/81564
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
@@ -0,0 +1,31 @@ +""" +Make sure that the concurrent vfork() from multiple threads works correctly. +""" + + +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.lldbtest import * +from lldbsuite.test.decorators import * + + +class TestConcurrentVFork(TestBase): +NO_DEBUG_INFO_TESTCASE = True + +@skipIfWindows +def test_vfork_follow_parent(self): clayborg wrote: Can you document what you are testing for here a bit more? I know what you are doing, but others might not. https://github.com/llvm/llvm-project/pull/81564 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
https://github.com/clayborg requested changes to this pull request. https://github.com/llvm/llvm-project/pull/81564 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix lldb crash while handling concurrent vfork() (PR #81564)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81564 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add settings and code that limits the number of progress events. (PR #75769)
clayborg wrote: What is the status of this PR? I think we will want this to make sure we don't spam too many individual progress events. It can also be adpated for the new categories after it goes in. https://github.com/llvm/llvm-project/pull/75769 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: Any chance we can get this in? https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
clayborg wrote: What is the problem here? I tried to understand the description, but don't know what the issue is? Can we test this? https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
Works for me as long as all clients check the optional and use it correctly. It
allows for multi-thread safety so that is good. Not sure if std::optional is
actually threadsafe though, but the timing required to make this fail would be
pretty low percentage. If it were me I would make it bullet proof as just leak
the map as when we tear down the process, any allocated memory will just go
poof anyway and it will actually speed up the process teardown process as no
work will need to be done to clear this map, though it should really be empty
anyway.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: I am fine with telling people what to do and giving them a golden path to what is easiest for our debuggers. And I will suggest to everyone that they use `.debug` and `.dwp`, but if we want to only support this, this leaves the downloading of the `.debug` file requiring a rename from `.dwp` to `.debug.dwp` in order for it to work for people. So then everything in this patch should be supported to allow loading the `.debug` file with a `.dwp` like we will encourage people to do. It would also be nice if we do have a ".debug" file that has debug info only, it would be nice to allow it and the ".dwp" file to be combined into a single file. There is no reason for them to be separate anymore once we have `a.out` stripped, it would be nice to only require "a.out.debug" which contains the ".dwp" sections inside of it already instead of requiring people to have two files needed for debug info. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
But wether you leak it or not it won't affect the program since it is exiting
already when this gets cleaned up if it is left as is above. So up to you guys.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
It is fine to start with this and switch over eventually if we run into
problems. Not sure what percentage of people will listen to the category
progresses vs the individual ones. So it will affect only the people using the
category ones.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
https://github.com/clayborg approved this pull request. Ok, LGTM. https://github.com/llvm/llvm-project/pull/81314 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
I also want to say that I don't mind the need to call
`ProgressManager::Initialize()` before using parts of this class. I am mostly
looking out for shut down crashes of the the LLDB library where the main thread
exits first while other threads are not all terminated, which does happen. All
of the places we have the comment:
```
// NOTE: intentional leak to avoid issues with C++ destructor chain
```
Was from tracking down shut down crashers in LLDB.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
Another reason for using the `std::once` is if no one tries to listen to the
eBroadcastBitProgressCategory, then nothing will need to be created. If not we
are always making an instance. Granted this object isn't very expensive to
create, but it is extra work that might not be needed.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
This can cause shut down crashes in LLDB where extra threads call a progress
API.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
> We have other subsystems that work exactly like what Chelsea did here, such
> as the Diagnostics and the FileSystem. They rely on the requirement that you
> need to call `::Initialize ` before doing anything with it. It's true that
> this will crash if you forget to `Initialize`/`Terminate` but that's true for
> the `Debugger` too. This is an establishes pattern and has the advantage that
> it can be tested in unittest by initializing and terminating the subsystem
> between tests.
This is fine as long as you always check the optional then and we aren't doing
that. So this can and will crash LLDB if extra threads call these APIs which is
never good. So the API, in order to be safe, should be:
```
std::optional ProgressManager::InstanceImpl()
```
And then every user needs to check this optional. Otherwise, this:
```
ProgressManager ::Instance() { return *InstanceImpl(); }
```
Will crash if Terminate has been called right? the `std:optional::operator
*`docs say "The behavior is undefined if *this does not contain a value."
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
clayborg wrote: None of what I suggested was mandatory. I was mainly trying to make sure things don't go away on us without us knowing about it. If we have crashes it is always hard to tell if simple heap corruption is the issue or if we are having lifetime issues. If you are not concerned about lifetime issues then you can ignore the `std::weak_ptr` suggestion, but if you are, this would be a good way to tell as we can check `std::weak_ptr::expired()`. Running a lldb with libgmalloc is a good way to watch for pointer lifetime issues and things being freed if someone ever has a repro case that they add to a bug. https://github.com/llvm/llvm-project/pull/81314 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
+
+void ProgressManager::Increment(std::string title) {
+ std::lock_guard lock(m_progress_map_mutex);
+ auto pair = m_progress_map.insert(std::pair(title, 1));
+
+ // If pair.first is not empty after insertion it means that that
+ // category was entered for the first time and should not be incremented
+ if (!pair.first->first().empty())
clayborg wrote:
This returns a `std::pair` where the bool designates if the
instertion took place. So this should be:
```
if (!pair.second)
```
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
clayborg wrote:
The global destructor chain will still cause this to be destroyed when the main
thread exits. Which is ok if we return `std::optional` from
`ProgressManager::Instance()`. More below in `ProgressManager
::Instance()` comments.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -119,6 +120,32 @@ class Progress {
bool m_complete = false;
};
+/// A class used to group progress reports by category. This is done by using a
+/// map that maintains a refcount of each category of progress reports that
have
+/// come in. Keeping track of progress reports this way will be done if a
+/// debugger is listening to the eBroadcastBitProgressByCategory broadcast bit.
+class ProgressManager {
+public:
+ ProgressManager();
+ ~ProgressManager();
+
+ static void Initialize();
+ static void Terminate();
+ // Control the refcount of the progress report category as needed
+ void Increment(std::string);
+ void Decrement(std::string);
clayborg wrote:
```
void Increment(std::string category);
void Decrement(std::string category);
```
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
clayborg wrote:
We can avoid the initialize/terminate if we just want to create this object on
the first call to `ProgressManager::InstanceImpl()`. More comments below as to
why
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -119,6 +120,32 @@ class Progress {
bool m_complete = false;
};
+/// A class used to group progress reports by category. This is done by using a
+/// map that maintains a refcount of each category of progress reports that
have
+/// come in. Keeping track of progress reports this way will be done if a
+/// debugger is listening to the eBroadcastBitProgressByCategory broadcast bit.
+class ProgressManager {
+public:
+ ProgressManager();
+ ~ProgressManager();
+
+ static void Initialize();
+ static void Terminate();
+ // Control the refcount of the progress report category as needed
+ void Increment(std::string);
+ void Decrement(std::string);
+
+ // Public accessor for the class instance
+ static ProgressManager ();
+
+private:
+ // Manage the class instance internally using a std::optional
+ static std::optional ();
+
+ llvm::StringMap m_progress_map;
clayborg wrote:
maybe `m_category_map`?
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81319 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
m_debugger_id);
}
}
+
+void ProgressManager::Initialize() {
+ lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+ InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+ lldbassert(InstanceImpl() &&
+ "A progress report manager has already been terminated.");
+ InstanceImpl().reset();
+}
+
+std::optional ::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager ::Instance() { return *InstanceImpl(); }
clayborg wrote:
If we want to use a static variable in `std::optional
::InstanceImpl()`, then we need to return a
`std::optional` here and people need to check it and not use
it if the optional isn't valid. If we always want to return a "ProgressManager
&" here, then we need `InstanceImpl()` to look like:
```
ProgressManager ::InstanceImpl() {
static std::once g_once_flag;
static ProgressManager *g_progress_manager = nullptr;
std::call_once(std::call_once(g_once_flag, {
// NOTE: known leak to avoid global destructor chain issues.
g_progress_manager = new ProgressManager();
});
return *g_progress_manager;
}
```
And then we don't need `ProgressManager::Initialize()` or
`ProgressManager::Terminate()`.
The problem is when the process exits, the main thread exists and calls the C++
global destructor chain and if any threads are still running and call any of
these functions that require the global instance of `ProgressManager`, it can
crash the process.
https://github.com/llvm/llvm-project/pull/81319
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)
https://github.com/clayborg requested changes to this pull request. https://github.com/llvm/llvm-project/pull/81319 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > Will this now work with .dwp files not having UUID? I actually added a test for this and it works just fine if the main executable (`a.out`) has a UUID and the debug info file (`a.out.debug`) also has the UUID, but the .dwp file doesn't, we _are_ able to load the .dwp file just fine. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
@@ -121,8 +135,10 @@ class ValueObjectPrinter {
private:
bool ShouldShowName() const;
- ValueObject *m_orig_valobj;
- ValueObject *m_valobj;
+ ValueObject _orig_valobj;
+ ValueObject *m_cached_valobj; /// Cache the current "most specialized" value.
clayborg wrote:
I wonder if we should start using `std::weak_ptr` instead of raw
pointers. Someone is storing a shared pointer to the object somewhere in the
value object tree, so having a weak pointer could help us to catch these things
as we can ask `if (m_cached_valobj_wp.expired() == true)` and this would tell
us that this was assigned to a valid object at one point, but it has been
freed. The expired call will return false if the weak pointer was never set or
if the weak pointer was cleared.
https://github.com/llvm/llvm-project/pull/81314
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
@@ -177,7 +177,10 @@ class CommandObjectFrameDiagnose : public
CommandObjectParsed {
DumpValueObjectOptions options;
options.SetDeclPrintingHelper(helper);
-ValueObjectPrinter printer(valobj_sp.get(), (),
+// We've already handled the case where the value object sp is null, so
+// this is just to make sure future changes don't skip that:
+assert(valobj_sp.get() && "Must have a valid ValueObject to print");
+ValueObjectPrinter printer(*(valobj_sp.get()), (),
clayborg wrote:
Can this just be:
```
ValueObjectPrinter printer(*valobj_sp, (),
```
https://github.com/llvm/llvm-project/pull/81314
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81314 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
@@ -18,39 +18,35 @@
using namespace lldb;
using namespace lldb_private;
-ValueObjectPrinter::ValueObjectPrinter(ValueObject *valobj, Stream *s) {
- if (valobj) {
-DumpValueObjectOptions options(*valobj);
-Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
- } else {
-DumpValueObjectOptions options;
-Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
- }
+ValueObjectPrinter::ValueObjectPrinter(ValueObject , Stream *s)
clayborg wrote:
Would it make sense to pass in a "ValueObjectSP" here instead? Could some of
our crashes be due to someone creating a ValueObjectPrinter and then somehow
the ValueObject gets freed behind out backs?
https://github.com/llvm/llvm-project/pull/81314
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
https://github.com/clayborg commented: See inlined comments, but maybe we need to start storing a `std::weak_ptr` instead of raw pointers to things, and possibly always passsing a `ValueObjectSP` object into the `ValueObjectPrinter` so it holds a strong reference to the value object while printing. I could see dead pointers causing problems and crashes. It would be nice to also not crash, right now we do things like: ``` assert(valobj); valobj->CrashNow(); ``` If we use `std::weak_ptr` instead of raw pointers or references, we can then call the `bool std::weak_ptr::expired() const` function to see if the weak pointer was valid before, but the main shared pointer keeping it alive has been freed. This will allow us to detect issues where our backing object has been freed. https://github.com/llvm/llvm-project/pull/81314 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
@@ -177,7 +177,10 @@ class CommandObjectFrameDiagnose : public
CommandObjectParsed {
DumpValueObjectOptions options;
options.SetDeclPrintingHelper(helper);
-ValueObjectPrinter printer(valobj_sp.get(), (),
+// We've already handled the case where the value object sp is null, so
+// this is just to make sure future changes don't skip that:
+assert(valobj_sp.get() && "Must have a valid ValueObject to print");
clayborg wrote:
It is fine to assert here, but we should not crash if valobj_sp is NULL. I
would rather log or report an error that crash if possible.
https://github.com/llvm/llvm-project/pull/81314
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg updated
https://github.com/llvm/llvm-project/pull/81067
>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH 1/4] Add more ways to find the .dwp file.
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
---
lldb/include/lldb/Utility/FileSpecList.h | 4 ++
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 61 +--
.../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
3 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Utility/FileSpecList.h
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b..6eb3bb9971f13 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
const_iterator begin() const { return m_files.begin(); }
const_iterator end() const { return m_files.end(); }
+ llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+ }
+
protected:
collection m_files; ///< A collection of FileSpec objects.
};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a43677..487961fa7437f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap
*SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr ::GetDwpSymbolFile()
{
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+ symfiles.Append(symfile_fspec);
+} else {
+ // If we don't have a separate debug info file, then try stripping the
+ // extension. We main module could be "a.debug" and the .dwp file could
be
+ // "a.dwp" instead of "a.debug.dwp".
+ ConstString filename_no_ext =
+ module_fspec.GetFileNameStrippingExtension();
+ if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+ }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), _filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (!dwp_obj_file)
-return;
- m_dwp_symfile = std::make_shared(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto : symfiles.files()) {
+ module_spec.GetSymbolFileSpec() =
+ FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
+ FileSpec dwp_filespec =
+ PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+ if (FileSystem::Instance().Exists(dwp_filespec)) {
+DataBufferSP dwp_file_data_sp;
+lldb::offset_t dwp_file_data_offset = 0;
+ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+GetObjectFile()->GetModule(), _filespec, 0,
+FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg updated
https://github.com/llvm/llvm-project/pull/81067
>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH 1/3] Add more ways to find the .dwp file.
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
---
lldb/include/lldb/Utility/FileSpecList.h | 4 ++
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 61 +--
.../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
3 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Utility/FileSpecList.h
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b6..6eb3bb9971f13a 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
const_iterator begin() const { return m_files.begin(); }
const_iterator end() const { return m_files.end(); }
+ llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+ }
+
protected:
collection m_files; ///< A collection of FileSpec objects.
};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a436778..487961fa7437fb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap
*SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr ::GetDwpSymbolFile()
{
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+ symfiles.Append(symfile_fspec);
+} else {
+ // If we don't have a separate debug info file, then try stripping the
+ // extension. We main module could be "a.debug" and the .dwp file could
be
+ // "a.dwp" instead of "a.debug.dwp".
+ ConstString filename_no_ext =
+ module_fspec.GetFileNameStrippingExtension();
+ if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+ }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), _filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (!dwp_obj_file)
-return;
- m_dwp_symfile = std::make_shared(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto : symfiles.files()) {
+ module_spec.GetSymbolFileSpec() =
+ FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
+ FileSpec dwp_filespec =
+ PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+ if (FileSystem::Instance().Exists(dwp_filespec)) {
+DataBufferSP dwp_file_data_sp;
+lldb::offset_t dwp_file_data_offset = 0;
+ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+GetObjectFile()->GetModule(), _filespec, 0,
+FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg updated
https://github.com/llvm/llvm-project/pull/81067
>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH 1/2] Add more ways to find the .dwp file.
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
---
lldb/include/lldb/Utility/FileSpecList.h | 4 ++
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 61 +--
.../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
3 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Utility/FileSpecList.h
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b6..6eb3bb9971f13a 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
const_iterator begin() const { return m_files.begin(); }
const_iterator end() const { return m_files.end(); }
+ llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+ }
+
protected:
collection m_files; ///< A collection of FileSpec objects.
};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a436778..487961fa7437fb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap
*SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr ::GetDwpSymbolFile()
{
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+ symfiles.Append(symfile_fspec);
+} else {
+ // If we don't have a separate debug info file, then try stripping the
+ // extension. We main module could be "a.debug" and the .dwp file could
be
+ // "a.dwp" instead of "a.debug.dwp".
+ ConstString filename_no_ext =
+ module_fspec.GetFileNameStrippingExtension();
+ if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+ }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), _filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (!dwp_obj_file)
-return;
- m_dwp_symfile = std::make_shared(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto : symfiles.files()) {
+ module_spec.GetSymbolFileSpec() =
+ FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
+ FileSpec dwp_filespec =
+ PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+ if (FileSystem::Instance().Exists(dwp_filespec)) {
+DataBufferSP dwp_file_data_sp;
+lldb::offset_t dwp_file_data_offset = 0;
+ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+GetObjectFile()->GetModule(), _filespec, 0,
+FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > FWIW, I think we should be opinionated (& consistent with whatever gdb does, > if it has some precedent here - if it is less opinionated, then maybe we have > to be accepting) when it comes to whether x.debug goes with x.dwp or > x.debug.dwp - we shouldn't support both unless there's some prior precedent > that's unavoidable to support. It'd be better for everyone if there was one > option we encourage people to follow. (so I think we shouldn't support (2) > and (3) if we can help it - we should pick one) There currently is no enforcement on any of this from the compiler or linker driver side and everyone must do the stripping of the executable and creating the .dwp file manually. Since there is no actual enforcement, it seems like we should support both IMHO. More on this below... > I'm not sure I understand the "lldb loads .debug and needs to find .dwp" > case. the .debug file usually only has the debug info, not the executable > code, so you can't load it directly, right? (see the documentation in > https://sourceware.org/gdb/current/onlinedocs/gdb.html/Separate-Debug-Files.html > - `objcopy --only-keep-debug foo foo.debug`). If we have a build system that creates: - `a.out` which is stripped and shipped for distribution - `a.out.debug` which contains debug info only with the skeleton units, address table, line tables, accelerator tables - `a.out.dwp` or `a.out.debug.dwp` which contains the .dwo files for `a.out.debug` We might request to download debug info for this build using a GNU build ID or some other build number for say a symbolication server which wants to use LLDB. If we get this debug info, we might only download `a.out.debug` and the associated `.dwp` file (either `a.out.dwp` or `a.out.debug.dwp`). It would be a shame to require that we download the `a.out` binary as well just so that we can load this into a debugger and get the .dwp file to load automatically. With this change people _can_ load `a.out.debug` into LLDB and peruse the debug information and perform symbolication without having to download the original `a.out`, a file which is not required for symbolication. If we downloaded `a.out.debug` + `a.out.dwp`, unless a user knows to rename `a.out.dwp` to `a.out.debug.dwp`, or create a symlink, then they won't get any info from the .dwo files in the .dwp file. If we don't fix this, then people will get confused and not know how to fix things and get full debug info to get loaded. No one besides debugger, compiler, and linker engineers and very few other people actually know what split DWARF does and or means and if they can't get things to work, they give up or waste time figuring out the magic things that need to happen to make us able to load the right debug info. So I prefer to allow things to work more seamlessly without trying to enforce something that is left up to users to do in the first place. I am someone that often responds to peoples posts when I am on call and I need to help people figure out how to load their debug symbols correrctl. With split DWARF it is even worse because if they get the debug info file with the skeleton compile units but the .dwp file isn't named consistent with GDB conventions, then they can set file and line breakpoints and hit them, but as soon as they stop there, they get no variable information because the .dwo file doesn't get loaded. They don't know what a .dwo or a .dwp file is or why it is needed. We get a few posts a week with people trying to use split DWARF symbols and not being able to get things to work. So IMHO there is no harm in making all scenarios work without having to require some naming convention that isn't actually enforced by _any_ compiler or linker driver, and when things don't work, we put the pressure on the user to find some wiki somewhere that says "here is how you were supposed to do it" and figure out how to fix the issue themselves or by seeking help from us. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > > > Will this now work with .dwp files not having UUID? > > > > > > No. If binairies have UUIDs (GNU build IDs), they need to match right now. > > That is larger fix that involves adding a "enum UUIDFlavor" to the UUIDs so > > we can ensure we aren't comparing two different things. > > What Alexander is talking about is if we have a GNU build ID in ``, > > the `.debug` file will have the same UUID, but llvm-dwp currently > > doesn't copy the GNU build ID over into the `.dwp` file. This causes LLDB > > to not allow the .dwp file to be loaded. The problem is if the .dwp file > > doesn't have a UUID, it will make one up by calculating a CRC of the file > > itself, and then we will compare a GNU build ID from `` to the CRC > > calculated by the `.dwp` file and they won't match. > > @dwblaikie do you know how accurate the DWO ID is? Can we avoid relying on > > matching up the UUID on the .dwp file and solely rely on allowing it to be > > loaded and rely on the DWO IDs matching between the skeleton unit and the > > .dwo unit? If so, there is an easy fix I can make to this patch to solve > > that problem. > > Not sure I follow. For .dwo files path is described in Skeleton CU: > DW_AT_comp_dir/DW_AT_dwo_name. The DWP file can have multiple CUs with > different DWO IDs. Exactly. The question is, if a have one `a.out.dwp` file that is out of date, and one of the .dwo files was recompiled into the `a.out` binary, but the `a.out.dwp` file wasn't recreated and if it was actually allowed to be loaded without comparing the GNU build ID, will the DWO ID be unique if the skeleton unit has a `DW_AT_dwo_name` with "foo.o" that has a DWO ID of `0x123` and if we lookup up "foo.o" in the old .dwp file, will the DWO ID mismatch be enough for us to ensure we don't load the .dwo info. If the answer is yes, which implied the DWO ID is some sort of checksum or signature that always changes when a specific file is rebuilt, then it is ok to not match a UUID on a .dwp file. Seeing as no .dwp files actually have a GNU build ID right now unless people manually add it, it seems like we should allow loading any .dwp regardless of the lack of a GNU build ID and deal with the mismatches from the DWO ID. If the DWO ID is just a hash of the file path or something that isn't guaranteed to be unique with each new build, then we need the UUID in the .dwp file. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
https://github.com/clayborg approved this pull request. https://github.com/llvm/llvm-project/pull/80890 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
clayborg wrote: > @clayborg The idea behind the feature is to get symbols for things that are > relevant to the user. Right now, that's only hooked up for images that appear > in the stack trace, but there are certainly other places this would be > useful. So yeah, I absolutely expect this to apply to other things in the > future as well. > > We currently check the setting right before downloading, so in a generic > place that would be shared by all the other places that could trigger a > download. Therefore I think the "mode" should be generic. If we have > different triggers we could have separate settings to turn those things off > (e.g. only download symbols for images in the stack trace, not for address > lookups). So down the line it could look something like this: > > ``` > (lldb) settings set symbols.auto-download background > (lldb) settings set symbols.auto-download-stack-symbols true > (lldb) settings set symbols.auto-download-address-lookup false > ``` > > Let me know if you think it's critical to add > `symbols.auto-download-stack-symbols` now in addition to > `symbols.auto-download` (which I like better than `symbols.download`, thanks > for the suggestion!) . Thanks for the update. Not critical to this patch, just wondering where we are going with this in the future and auto downloading of symbols. https://github.com/llvm/llvm-project/pull/80890 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
clayborg wrote: > The discussions happening here are talking about 2 major things, how to do > the bookkeeping of the map that keeps track of progress reports and where to > do that bookkeeping. I think it makes sense to split up this work into > smaller patches as such: > > 1. Since it's best to do the bookkeeping using the > `Debugger::eBroadcastBitProgressByCategory` bit, I'll add this bit to > `Debugger` and `SBDebugger` in its own patch. > 2. Add a class to manage the map that holds progress reports. > 3. Once that class and the bit are added, use it to perform operations on the > map of progress reports and use the new bit to actual broadcast operations. > > I'll put up a patch to add the bit now and start work on parts 2 and 3. I'm > also going to close this PR since the work is now being split up. Sounds great. Thanks for putting in the work on this! Also, I do have the PR that adds settings and throttles progress events, do we still want that patch? I do believe that will improve our progress events and keep them from overwhelming clients. https://github.com/llvm/llvm-project/pull/81026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][debugger][NFC] Add broadcast bit for category-based progress events. (PR #81169)
https://github.com/clayborg approved this pull request. https://github.com/llvm/llvm-project/pull/81169 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
clayborg wrote: > Will this now work with .dwp files not having UUID? No. If binairies have UUIDs (GNU build IDs), they need to match right now. That is larger fix that involves adding a "enum UUIDFlavor" to the UUIDs so we can ensure we aren't comparing two different things. What Alexander is talking about is if we have a GNU build ID in ``, the `.debug` file will have the same UUID, but llvm-dwp currently doesn't copy the GNU build ID over into the `.dwp` file. This causes LLDB to not allow the .dwp file to be loaded. The problem is if the .dwp file doesn't have a UUID, it will make one up by calculating a CRC of the file itself, and then we will compare a GNU build ID from `` to the CRC calculated by the `.dwp` file and they won't match. @dwblaikie do you know how accurate the DWO ID is? Can we avoid relying on matching up the UUID on the .dwp file and solely rely on allowing it to be loaded and rely on the DWO IDs matching between the skeleton unit and the .dwo unit? If so, there is an easy fix I can make to this patch to solve that problem. https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81067 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add more ways to find the .dwp file. (PR #81067)
https://github.com/clayborg created
https://github.com/llvm/llvm-project/pull/81067
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
>From 3c2f6039cf0e253d78b5193098b311028daaea72 Mon Sep 17 00:00:00 2001
From: Greg Clayton
Date: Wed, 7 Feb 2024 16:43:50 -0800
Subject: [PATCH] Add more ways to find the .dwp file.
When using split DWARF we can run into many different ways to store debug info:
- lldb loads "" which contains skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".dwp"
- lldb loads "" which is stripped but has .gnu_debuglink pointing to
".debug" with skeleton DWARF and needs to find ".debug.dwp"
- lldb loads ".debug" and needs to find ".dwp
Previously we only handled the first two cases. This patch adds support for the
latter two.
---
lldb/include/lldb/Utility/FileSpecList.h | 4 ++
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 61 +--
.../DWARF/x86/dwp-separate-debug-file.cpp | 30 +
3 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/lldb/include/lldb/Utility/FileSpecList.h
b/lldb/include/lldb/Utility/FileSpecList.h
index 49edc667ddd5b6..6eb3bb9971f13a 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -238,6 +238,10 @@ class FileSpecList {
const_iterator begin() const { return m_files.begin(); }
const_iterator end() const { return m_files.end(); }
+ llvm::iterator_range files() const {
+return llvm::make_range(begin(), end());
+ }
+
protected:
collection m_files; ///< A collection of FileSpec objects.
};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 781f5c5a436778..487961fa7437fb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4349,26 +4349,53 @@ SymbolFileDWARFDebugMap
*SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr ::GetDwpSymbolFile()
{
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+// Create a list of files to try and append .dwp to
+FileSpecList symfiles;
+// Append the module's object file path.
+const FileSpec module_fspec = m_objfile_sp->GetModule()->GetFileSpec();
+symfiles.Append(module_fspec);
+// Append the object file for this SymbolFile only if it is different from
+// the module's file path. Our main module could be "a.out", our symbol
file
+// could be "a.debug" and our ".dwp" file might be "a.debug.dwp" instead of
+// "a.out.dwp".
+const FileSpec symfile_fspec(m_objfile_sp->GetFileSpec());
+if (symfile_fspec != module_fspec) {
+ symfiles.Append(symfile_fspec);
+} else {
+ // If we don't have a separate debug info file, then try stripping the
+ // extension. We main module could be "a.debug" and the .dwp file could
be
+ // "a.dwp" instead of "a.debug.dwp".
+ ConstString filename_no_ext =
+ module_fspec.GetFileNameStrippingExtension();
+ if (filename_no_ext != module_fspec.GetFilename()) {
+FileSpec module_spec_no_ext(module_fspec);
+module_spec_no_ext.SetFilename(filename_no_ext);
+symfiles.Append(module_spec_no_ext);
+ }
+}
+
+FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
-module_spec.GetSymbolFileSpec() =
-FileSpec(m_objfile_sp->GetModule()->GetFileSpec().GetPath() + ".dwp");
-
module_spec.GetUUID() = m_objfile_sp->GetUUID();
-FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
-FileSpec dwp_filespec =
-PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
-if (FileSystem::Instance().Exists(dwp_filespec)) {
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), _filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (!dwp_obj_file)
-return;
- m_dwp_symfile = std::make_shared(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
+for (const auto : symfiles.files()) {
+ module_spec.GetSymbolFileSpec() =
+
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
clayborg wrote: > > A few questions I have: > > > > * do we really want each progress to select if it should be coalesced as a > > `Progress` constructor arguments? Or do we want a global setting on how > > progress events should be delivered? > > My understanding is that the constructor conveys whether something is an > "aggregate" progress event or not and they're broadcast differently so that > the listener can decide how they want to receive these "aggregate" events. My idea is the user decides how to listen to the events by which `SBDebugger::eBroadcastBit` (`SBDebugger::eBroadcastBitProgress` or `SBDebugger::eBroadcastBitProgressByCategory`) they listen to, and we will deliver the events accordingly. > > > * The current code adds a way to increment and decrement refcounts of > > ongoing progress categories (titles), but doesn't do anything with them, > > what do we envision happening with these ref counts? > > With the previous idea in mind, you need the refcount in case there are > overlapping events: > > ``` > eBroadcastBitProgress [foo] > eBroadcastBitProgress [foo] > eBroadcastBitProgressByCategory [foo ] > ---> (time) > ``` > > So basically if you have two events `foo` that overlap as shown above, if > you're listening for `eBroadcastBitProgress` you get two events. If you're > listening for `eBroadcastBitProgressByCategory`. The refcount is used to make > sure you don't broadcast an events for `eBroadcastBitProgressByCategory` when > the first foo ends. Yeah, with category, we will see "Indexing symbol table" and then we might have both "a.out" and "b.out" details from two different categories needing to be displayed. The debugger event that gets sent for `eBroadcastBitProgressByCategory` might need some new accessors to account for this. https://github.com/llvm/llvm-project/pull/81026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -99,6 +105,10 @@ class Progress {
private:
void ReportProgress();
static std::atomic g_id;
+ static std::atomic g_refcount;
+ /// Map that tracks each progress object and if we've seen its start and stop
+ /// events
+ static std::unordered_map g_map;
clayborg wrote:
Correct. Because we have to just in case the debugger pointer was set in the
Progress, so we have to track it in each debugger just in case.
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -9,26 +9,34 @@
#include "lldb/Core/Progress.h"
#include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
#include
using namespace lldb;
using namespace lldb_private;
std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
clayborg wrote:
yes, might not be needed. Still needs to be threadsafe though if we do move it
into the Debugger.h/.cpp
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -99,6 +105,10 @@ class Progress {
private:
void ReportProgress();
static std::atomic g_id;
+ static std::atomic g_refcount;
+ /// Map that tracks each progress object and if we've seen its start and stop
+ /// events
+ static std::unordered_map g_map;
clayborg wrote:
> We spoke about this a little offline but at the moment although Progress
> reports hold on to debugger instances, these instances are null by default
> and most Progress reports don't give a debugger instance in their
> instantiation so we wouldn't be able to have a debugger instance hold on to
> their own map of ongoing progress reports.
>
> @clayborg Having a progress report holding on to a debugger instance is a
> good idea, but if we want to use a map to keep track of ongoing reports we
> need to know what will own the map? Do we want the map to be owned by a
> debugger instance or could the progress class hold on to its own map of
> ongoing reports? In the case of the former we would then want to enforce that
> progress reports have valid debugger instances and in the latter we could
> remove the `debugger` field from the reports altogether.
The main question is if we want all progress reports to follow the currently
selected mode (coalesce or individual) or if we want them to be able to pick
which one they are. I would vote to have a global setting that controls if we
deliver things, but would love to hear what others think.
Each Progress instance _can_ have a debugger, but most don't, and those will
get delivered to all debugger instances. So I would say that this map should
live in each debugger object because `Progress` objects _can_ have a debugger
set, and if they do, they will only get delivered to _that_ debugger.
So my suggestion here is to:
- not change anything in Progress.h/cpp
- move the progress categorization code into each debugger object
- in `PrivateReportProgress` we check if anyone is listening to the new
`eBroadcastBitProgressByCategory` bit, and then and _only_ then do we do any of
the category counting.
So the code in `PrivateReportProgress` will now look something like:
```
static void PrivateReportProgress(Debugger , uint64_t progress_id,
std::string title, std::string details,
uint64_t completed, uint64_t total,
bool is_debugger_specific) {
// Only deliver progress events if we have any progress listeners.
if
(debugger.GetBroadcaster().EventTypeHasListeners(Debugger::eBroadcastBitProgressByCategory))
PrivateReportProgressByCategory(debugger, progress_id,
is_debugger_specific, completed, total, is_debugger_specific);
const uint32_t event_type = Debugger::eBroadcastBitProgress;
if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
return;
EventSP event_sp(new Event(
event_type,
new ProgressEventData(progress_id, std::move(title), std::move(details),
completed, total, is_debugger_specific)));
debugger.GetBroadcaster().BroadcastEvent(event_sp);
}
```
Then we write a new `PrivateReportProgressByCategory()` function that will do
all this book keeping and coordingate with the debugger which will own the
category ref count map
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
https://github.com/clayborg edited https://github.com/llvm/llvm-project/pull/81026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -9,26 +9,34 @@
#include "lldb/Core/Progress.h"
#include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
#include
using namespace lldb;
using namespace lldb_private;
std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
clayborg wrote:
When something isn't just an integer, like the map here, we can run into issues
with this in a multi-threaded environment. Why? These variables are part of the
global C++ constructor / destructor chain. If any thread uses these after the
main thread shuts down it can cause a crash. So for global objects we typically
will create a pointer to one and leak it with a note. See
`lldb/source/Core/Debugger.cpp` and look for `g_debugger_list_mutex_ptr` and
other variables right next to them and then see `Debugger::Initialize()`.
This map also need to be threadsafe, so it needs a mutex. Might be a good idea
to create a class to contain these and use an accessor to provide access to it.
The solution below will make something that is thread safe _and_ also something
that will survive the global destructor chain being called and letting other
threads still be able to use Progress objects without crashing the process:
```
namespace {
class ProgressCategoryMap {
std::mutex m_mutex;
std::unordered_map m_map;
void Increment(std::string title) {
std::lock_guard lock(m_mutex);
auto pair = g_map.emplace(title, 1); // No need to use g_refcount as it is
just a constant 1
// pair.first will be true if insertion happened.
if (pair.first == false)
++pair.second; // Key already in map, increment the ref count.
}
void Decrement(std::string title) {
std::lock_guard lock(m_mutex);
auto pos = g_map.find(title);
if (pos == g_map.end())
return;
if (--pos->second == 0)
g_map.erase(pos->second); // Decremented to zero
}
};
// Now make a global accessor to get a threadsafe copy of ProgressCategoryMap
ProgressCategoryMap *GetProgressCategoryMap() {
static std::once_flag g_once;
static ProgressCategoryMap *g_category_map_ptr = nullptr;
std::call_once(g_once, []{
// NOTE: intentional leak to avoid issues with C++ destructor chain
g_category_map_ptr = new ProgressCategoryMap();
});
return g_category_map_ptr;
}
} // anonymous namespace
```
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -9,26 +9,34 @@ #include "lldb/Core/Progress.h" #include "lldb/Core/Debugger.h" -#include "lldb/Utility/StreamString.h" #include using namespace lldb; using namespace lldb_private; std::atomic Progress::g_id(0); +std::atomic Progress::g_refcount(1); clayborg wrote: This isn't needed. This value doesn't change and is always 1, so you can either make it a constexpr in the class description, or it isn't really needed at all actually. https://github.com/llvm/llvm-project/pull/81026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -55,6 +56,10 @@ namespace lldb_private {
class Progress {
public:
+ enum {
+eProgressLinearReports,
+eProgressCoalecseReports,
+ };
clayborg wrote:
Should we add a new setting for this? Something like:
```
(lldb) settings set progress-report [individual|grouped]
```
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -38,6 +46,13 @@ Progress::~Progress() {
std::lock_guard guard(m_mutex);
if (!m_completed)
m_completed = m_total;
+
+ if (m_type == Progress::eProgressCoalecseReports) {
+--g_map.at(m_title);
+if (g_map.at(m_title) == 0)
+ g_map.erase(m_title);
clayborg wrote:
Use the threadsafe `ProgressCategoryMap` here:
```
GetProgressCategoryMap()->Decrement(title);
```
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
https://github.com/clayborg commented: So I added comments on fixes to the current implementation in inline comments. A few questions I have: - do we really want each progress to select if it should be coalesced as a `Progress` constructor arguments? Or do we want a global setting on how progress events should be delivered? - The current code adds a way to increment and decrement refcounts of ongoing progress categories (titles), but doesn't do anything with them, what do we envision happening with these ref counts? But overall I would think we would want to keep track of this at a higher layer than in this class. Like in the code that would be delivering the progress events. If we do it that way, then we could have a global setting that controls if users want events delivered by category or individually. We actually won't need a setting for this as we can just see who is listening to the process events. Anyone listening to `SBDebugger::eBroadcastBitProgress` would get progress events delivered on a one by one basis, and anyone listening to a new progress bit like `SBDebugger::eBroadcastBitProgressByCategory` would just get different events delivered and all of this ref counting by title code from here would be moved into Debugger.cpp where the other progress events are being handled already. https://github.com/llvm/llvm-project/pull/81026 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Add groundwork to keep track of progress reports (PR #81026)
@@ -9,26 +9,34 @@
#include "lldb/Core/Progress.h"
#include "lldb/Core/Debugger.h"
-#include "lldb/Utility/StreamString.h"
#include
using namespace lldb;
using namespace lldb_private;
std::atomic Progress::g_id(0);
+std::atomic Progress::g_refcount(1);
+std::unordered_map Progress::g_map = {};
Progress::Progress(std::string title, std::string details,
std::optional total,
- lldb_private::Debugger *debugger)
+ lldb_private::Debugger *debugger, bool type)
: m_title(title), m_details(details), m_id(++g_id), m_completed(0),
- m_total(Progress::kNonDeterministicTotal) {
+ m_total(Progress::kNonDeterministicTotal), m_type(type) {
if (total)
m_total = *total;
if (debugger)
m_debugger_id = debugger->GetID();
std::lock_guard guard(m_mutex);
+
+ if (m_type == Progress::eProgressCoalecseReports) {
+g_map.emplace(title, g_refcount);
+
+if (g_map.at(title) >= 1)
+ ++g_map.at(title);
clayborg wrote:
Use the threadsafe `ProgressCategoryMap` here:
```
GetProgressCategoryMap()->Increment(title);
```
https://github.com/llvm/llvm-project/pull/81026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
clayborg wrote: Does this setting only apply to the downloading of symbols that are auto downloaded for stack traces? Or does it apply to any other areas that auto download stuff? Do we want a setting to control the auto downloading of symbols to clarify? Something like: ``` (lldb) settings set symbols.auto-download-stack-symbols true ``` Right now your setting seems to be targeting stack auto symbol downloads, but this setting is pretty generic and could apply to other things in the future like auto downloading symbols when an address lookup happens on a module, finding the real definition for a type when we know a type lives in a shared library (like ObjC and C++ vtables can easily tell us). https://github.com/llvm/llvm-project/pull/80890 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)
clayborg wrote: > Should the Doxygen comment of GetStackFrameCount warn that this is an > expensive operation? > https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Thread.html#afc54feef950a58b625bbb198dc4cf57c It might be nice to add a "std::optional max_frame_count" to this function to allow it to stop when it hits "max_frame_count". Like: ``` /// Get the number of frames in a thread. /// /// If \a max_frame_count is valid, return a number that is less than or equal /// to max_frame_count, else calculate the true number of frames. Calculating /// the total number of frames can be expensive. virtual uint32_t lldb_private::Thread::GetStackFrameCount(std::optional max_frame_count); ``` https://github.com/llvm/llvm-project/pull/80918 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
@@ -5,10 +5,11 @@ let Definition = "modulelist" in {
Global,
DefaultTrue,
Desc<"Control the use of external tools and repositories to locate symbol
files. Directories listed in target.debug-file-search-paths and directory of
the executable are always checked first for separate debug info files. Then
depending on this setting: On macOS, Spotlight would be also used to locate a
matching .dSYM bundle based on the UUID of the executable. On NetBSD, directory
/usr/libdata/debug would be also searched. On platforms other than NetBSD
directory /usr/lib/debug would be also searched. If all other methods fail
there may be symbol-locator plugins that, if configured properly, will also
attempt to acquire symbols. The debuginfod plugin defaults to the
DEGUFINFOD_URLS environment variable which is configurable through the
'plugin.symbol-locator.debuginfod.server_urls' setting.">;
- def EnableBackgroundLookup: Property<"enable-background-lookup", "Boolean">,
clayborg wrote:
Should we leave this setting around and adapt it to use the new setting with
the correct defaults set? If someone has a `settings set
enable-background-lookup true` in their ~/.lldbinit file, this will cause the
.lldbinit file to stop executing lines if this setting fails to be set. This is
kind of like a backward compatibility in our API kind of thing. We can make
this setting as deprecated and tell people to use the `download` setting
instead, but it would be nice if it still worked.
If this was used for dsymForUUID only and there weren't many customers it could
be ok to remove, but I would err on the side of caution.
https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
@@ -47,6 +47,26 @@ class UUID;
class VariableList;
struct ModuleFunctionSearchOptions;
+static constexpr OptionEnumValueElement g_download_enum_values[] = {
+{
+lldb::eSymbolDownloadOff,
+"off",
+"Disable downloading symbols.",
+},
+{
+lldb::eSymbolDownloadBackground,
+"background",
+"Download symbols in the background for images as they appear in the "
+"backtrace.",
clayborg wrote:
Is this truly only for backtraces? Wouldn't an address lookup in a binary also
qualify as a way to force debug info to be loaded?
https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
@@ -5,10 +5,11 @@ let Definition = "modulelist" in {
Global,
DefaultTrue,
Desc<"Control the use of external tools and repositories to locate symbol
files. Directories listed in target.debug-file-search-paths and directory of
the executable are always checked first for separate debug info files. Then
depending on this setting: On macOS, Spotlight would be also used to locate a
matching .dSYM bundle based on the UUID of the executable. On NetBSD, directory
/usr/libdata/debug would be also searched. On platforms other than NetBSD
directory /usr/lib/debug would be also searched. If all other methods fail
there may be symbol-locator plugins that, if configured properly, will also
attempt to acquire symbols. The debuginfod plugin defaults to the
DEGUFINFOD_URLS environment variable which is configurable through the
'plugin.symbol-locator.debuginfod.server_urls' setting.">;
- def EnableBackgroundLookup: Property<"enable-background-lookup", "Boolean">,
+ def Download: Property<"download", "Enum">,
Global,
-DefaultFalse,
-Desc<"On macOS, enable calling dsymForUUID (or an equivalent
script/binary) in the background to locate symbol files that weren't found.">;
+DefaultEnumValue<"eSymbolDownloadOff">,
+EnumValues<"OptionEnumValues(g_download_enum_values)">,
+Desc<"On macOS, lazily download symbols with dsymForUUID (or an equivalent
script/binary) as images appear in the current backtrace.">;
clayborg wrote:
Is it truly only for backtraces? Seems like an address lookup in a shared
library might be a good place to also get the symbols.
https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)
@@ -608,11 +608,10 @@ static bool Evaluate_DW_OP_entry_value(std::vector
,
StackFrameSP parent_frame = nullptr;
addr_t return_pc = LLDB_INVALID_ADDRESS;
uint32_t current_frame_idx = current_frame->GetFrameIndex();
- uint32_t num_frames = thread->GetStackFrameCount();
- for (uint32_t parent_frame_idx = current_frame_idx + 1;
- parent_frame_idx < num_frames; ++parent_frame_idx) {
+
+ for (uint32_t parent_frame_idx = current_frame_idx + 1;;parent_frame_idx++) {
clayborg wrote:
yeah, I would second that suggestion to make the code easier to read.
https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
@@ -184,8 +219,12 @@ void TargetStats::IncreaseSourceMapDeduceCount() {
bool DebuggerStats::g_collecting_stats = false;
-llvm::json::Value DebuggerStats::ReportStatistics(Debugger ,
- Target *target) {
+llvm::json::Value DebuggerStats::ReportStatistics(
+Debugger , Target *target,
+const lldb_private::StatisticsOptions ) {
+
+ bool summary_only = options.summary_only;
clayborg wrote:
`const bool ...`
https://github.com/llvm/llvm-project/pull/80745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
