Skip to content

[LLDB][Minidump] Fix bug where we were using the wrong collection for thread stacks #110579

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 1, 2024
Merged

[LLDB][Minidump] Fix bug where we were using the wrong collection for thread stacks #110579

merged 1 commit into from
Oct 1, 2024

Conversation

Jlalond
Copy link
Contributor

@Jlalond Jlalond commented Sep 30, 2024

In my prior two save core API's, I experimented on how to save stacks with the new API. I incorrectly left these in, as the existing m_thread_by_range_end was the correct choice.

I have removed the no-op collection, and moved to use the proper one. It's worth noting this was not caught by testing because we do not verify where the items are contained in the minidump. This would require a test being aware of how minidumps are structured, or adding a textual tool that we can then scan the output of.

@llvmbot llvmbot added the lldb label Sep 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 30, 2024

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

In my prior two save core API's, I experimented on how to save stacks with the new API. I incorrectly left these in, as the existing m_thread_by_range_end was the correct choice.

I have removed the no-op collection, and moved to use the proper one. It's worth noting this was not caught by testing because we do not verify where the items are contained in the minidump. This would require a test being aware of how minidumps are structured, or adding a textual tool that we can then scan the output of.

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

2 Files Affected:

  • (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+1-1)
  • (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (-1)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 3f1e25f730a184..f6c16b6e3d96ae 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -853,7 +853,7 @@ Status MinidumpFileBuilder::AddMemoryList() {
   uint64_t total_size = GetCurrentDataEndOffset();
   auto iterator = all_core_memory_vec.begin();
   while (iterator != all_core_memory_vec.end()) {
-    if (m_saved_stack_ranges.count(iterator->range.start()) > 0) {
+    if (m_thread_by_range_end.count(iterator->range.end()) > 0) {
       // We don't save stacks twice.
       ranges_32.push_back(*iterator);
       total_size +=
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index d5eac9015ac422..a4240f871c8a2f 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -172,7 +172,6 @@ class MinidumpFileBuilder {
   // to duplicate it in the exception data.
   std::unordered_map<lldb::tid_t, llvm::minidump::LocationDescriptor>
       m_tid_to_reg_ctx;
-  std::unordered_set<lldb::addr_t> m_saved_stack_ranges;
   lldb::FileUP m_core_file;
   lldb_private::SaveCoreOptions m_save_core_options;
 };

@Jlalond Jlalond changed the title [LLDB][Minidump] Fix bug where we were using the wrong collection for stacks in mmeory 32 [LLDB][Minidump] Fix bug where we were using the wrong collection for stacks Sep 30, 2024
@Jlalond Jlalond changed the title [LLDB][Minidump] Fix bug where we were using the wrong collection for stacks [LLDB][Minidump] Fix bug where we were using the wrong collection for thread stacks Sep 30, 2024
@jeffreytan81 jeffreytan81 requested a review from clayborg October 1, 2024 00:33
@Jlalond Jlalond merged commit 78ff340 into llvm:main Oct 1, 2024
9 checks passed
@hnrklssn
Copy link
Member

hnrklssn commented Oct 1, 2024

This is my main suspect for this failure in Green Dragon: https://green.lab.llvm.org/job/llvm.org/job/llvm-coverage/64/consoleFull

Relevant test output:

AIL: test_with_run_command_libcpp_dsym (TestDataFormatterGenericOptional.GenericOptionalDataFormatterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1769, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 103, in test_with_run_command_libcpp
    self.do_test_with_run_command(USE_LIBCPP)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 59, in do_test_with_run_command
    self.expect(
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2475, in expect
    self.fail(log_msg)
AssertionError: Ran command:
"frame var numbers"

Got output:
(optional_int_vect) numbers =  Has Value=true  {
  Value = {
    __begin_ = 0x00006000013d4000
    __end_ = 0x00006000013d4010
    __cap_ = 0x00006000013d4010
    __padding1_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\U00000014B\xb5\0\U0000007f\0\0p\U00000013B\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
    __alloc_ = {}
    __padding2_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\U00000014B\xb5\0\U0000007f\0\0p\U00000013B\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
  }
}

Expecting sub string: "(optional_int_vect) numbers =  Has Value=true  {" (was found)
Expecting sub string: "Value = size=4 {" (was not found)
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/lldb-build/bin/clang
======================================================================
FAIL: test_with_run_command_libcpp_dwarf (TestDataFormatterGenericOptional.GenericOptionalDataFormatterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1769, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 103, in test_with_run_command_libcpp
    self.do_test_with_run_command(USE_LIBCPP)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 59, in do_test_with_run_command
    self.expect(
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2475, in expect
    self.fail(log_msg)
AssertionError: Ran command:
"frame var numbers"

Got output:
(optional_int_vect) numbers =  Has Value=true  {
  Value = {
    __begin_ = 0x0000600003704000
    __end_ = 0x0000600003704010
    __cap_ = 0x0000600003704010
    __padding1_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\xc3\U00000010\xbc\0\U0000007f\0\0`\xc3\U00000010\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
    __alloc_ = {}
    __padding2_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\xc3\U00000010\xbc\0\U0000007f\0\0`\xc3\U00000010\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
  }
}

Expecting sub string: "(optional_int_vect) numbers =  Has Value=true  {" (was found)
Expecting sub string: "Value = size=4 {" (was not found)
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/lldb-build/bin/clang
----------------------------------------------------------------------
Ran 6 tests in 13.814s

FAILED (failures=2, skipped=4)

@Michael137
Copy link
Member

This is my main suspect for this failure in Green Dragon: https://green.lab.llvm.org/job/llvm.org/job/llvm-coverage/64/consoleFull

Relevant test output:

AIL: test_with_run_command_libcpp_dsym (TestDataFormatterGenericOptional.GenericOptionalDataFormatterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1769, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 103, in test_with_run_command_libcpp
    self.do_test_with_run_command(USE_LIBCPP)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 59, in do_test_with_run_command
    self.expect(
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2475, in expect
    self.fail(log_msg)
AssertionError: Ran command:
"frame var numbers"

Got output:
(optional_int_vect) numbers =  Has Value=true  {
  Value = {
    __begin_ = 0x00006000013d4000
    __end_ = 0x00006000013d4010
    __cap_ = 0x00006000013d4010
    __padding1_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\U00000014B\xb5\0\U0000007f\0\0p\U00000013B\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
    __alloc_ = {}
    __padding2_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\U00000014B\xb5\0\U0000007f\0\0p\U00000013B\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
  }
}

Expecting sub string: "(optional_int_vect) numbers =  Has Value=true  {" (was found)
Expecting sub string: "Value = size=4 {" (was not found)
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/lldb-build/bin/clang
======================================================================
FAIL: test_with_run_command_libcpp_dwarf (TestDataFormatterGenericOptional.GenericOptionalDataFormatterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1769, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 103, in test_with_run_command_libcpp
    self.do_test_with_run_command(USE_LIBCPP)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py", line 59, in do_test_with_run_command
    self.expect(
  File "/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2475, in expect
    self.fail(log_msg)
AssertionError: Ran command:
"frame var numbers"

Got output:
(optional_int_vect) numbers =  Has Value=true  {
  Value = {
    __begin_ = 0x0000600003704000
    __end_ = 0x0000600003704010
    __cap_ = 0x0000600003704010
    __padding1_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\xc3\U00000010\xbc\0\U0000007f\0\0`\xc3\U00000010\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
    __alloc_ = {}
    __padding2_781_ = (__padding_ = "\U00000001\0\0\0\0\0\0\0*\0\0\0*\0\0\0\U00000001\0\0\0*\0\0\0*\0\0\0\U00000001\U0000007f\0\0\0\xc3\U00000010\xbc\0\U0000007f\0\0`\xc3\U00000010\U00000001\0\0\0\0\U00000001\0\0\0\U00000002\0\0\0\U00000003\0\0\0\U00000004")
  }
}

Expecting sub string: "(optional_int_vect) numbers =  Has Value=true  {" (was found)
Expecting sub string: "Value = size=4 {" (was not found)
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/llvm-coverage/lldb-build/bin/clang
----------------------------------------------------------------------
Ran 6 tests in 13.814s

FAILED (failures=2, skipped=4)

That failure was caused by #110355 and has been reverted earlier today

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
@Jlalond @Sterling-Augustine
[LLDB][Minidump] Fix bug where we were using the wrong collection for…
04948f7 
… thread stacks (llvm#110579)

In my prior two save core API's, I experimented on how to save stacks
with the new API. I incorrectly left these in, as the existing
`m_thread_by_range_end` was the correct choice.

I have removed the no-op collection, and moved to use the proper one.
It's worth noting this was not caught by testing because we do not
verify where the items are contained in the minidump. This would require
a test being aware of how minidumps are structured, or adding a textual
tool that we can then scan the output of.
@Jlalond Jlalond deleted the minidump-stacks-first-bug branch March 7, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffreytan81 jeffreytan81 jeffreytan81 approved these changes

@JDevlieghere JDevlieghere Awaiting requested review from JDevlieghere JDevlieghere is a code owner

@clayborg clayborg Awaiting requested review from clayborg

Assignees
No one assigned
Labels
lldb
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jlalond @llvmbot @hnrklssn @Michael137 @jeffreytan81