Skip to content

[LLDB][Save Core Options] Custom ranges should follow the same safety checks as everyone else #125323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

Jlalond
Copy link
Contributor

@Jlalond Jlalond commented Feb 1, 2025

I encountered a qMemoryRegionInfo not supported error when capturing a Minidump. This was surprising, and I started looking around I found @jasonmolenda's fix in #115963 and then realized I was not validated anything from the custom ranges.

…se, to avoid empty ranges, or ranges with no permissions
@Jlalond Jlalond requested a review from clayborg February 1, 2025 01:40
@Jlalond Jlalond requested a review from JDevlieghere as a code owner February 1, 2025 01:40
@llvmbot llvmbot added the lldb label Feb 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 1, 2025

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

I encountered a qMemoryRegionInfo not supported error when capturing a Minidump. This was surprising, and I started looking around I found @jasonmolenda's fix in #115963 and then realized I was not validated anything from the custom ranges.


Full diff: https://github.com/llvm/llvm-project/pull/125323.diff

1 Files Affected:

  • (modified) lldb/source/Target/Process.cpp (+2-5)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 89731f798deda8..428f8519b72fd5 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6677,11 +6677,8 @@ static void GetUserSpecifiedCoreFileSaveRanges(Process &process,
 
   for (const auto &range : regions) {
     auto entry = option_ranges.FindEntryThatContains(range.GetRange());
-    if (entry) {
-      ranges.Append(range.GetRange().GetRangeBase(),
-                    range.GetRange().GetByteSize(),
-                    CreateCoreFileMemoryRange(range));
-    }
+    if (entry)
+      AddRegion(range, true, ranges);
   }
 }
 

Copy link

github-actions bot commented Feb 3, 2025

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok for now, but we need to find the bug in lldb-server that causes qMemoryRegionInfo to fail and return a $#00 which means unsupported and causes all future qMemoryRegionInfo to fail.

@Jlalond Jlalond merged commit ce7bca7 into llvm:main Feb 4, 2025
7 checks passed
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
… checks as everyone else (llvm#125323)

I encountered a `qMemoryRegionInfo not supported` error when capturing a
Minidump. This was surprising, and I started looking around I found
@jasonmolenda's fix in llvm#115963 and then realized I was not validated
anything from the custom ranges.
@Jlalond Jlalond deleted the empty-region-savecoreoptions branch March 7, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants