[Lldb-commits] [lldb] [lldb] Change the two remaining SInt64 settings in Target to uint (PR #105460)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Wed Aug 21 16:25:16 PDT 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/105460
>From 5021ef81d8c18a06cbb6d7595845bacd7e99b423 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 20 Aug 2024 18:29:24 -0700
Subject: [PATCH 1/2] [lldb] Change the two remaining SInt64 settings in Target
to uint
TargetProperties.td had a few settings listed as signed integral
values, but the Target.cpp methods reading those values were reading
them as unsigned. e.g. target.max-memory-read-size, some accesses
of target.max-children-count, still today, previously
target.max-string-summary-length.
After Jonas' change to use templates to read these values in
https://reviews.llvm.org/D149774, when the code tried to fetch these
values, we'd eventually end up calling OptionValue::GetAsUInt64
which checks that the value is actually a UInt64 before returning
it; finding that it was an SInt64, it would drop the user setting
and return the default value. This manifested as a bug that
target.max-memory-read-size is never used for memory read.
target.max-children-count is less straightforward, where one read
of that setting was fetching it as an int64_t, the other as a
uint64_t.
I suspect all of these settings were originally marked as SInt64
so a user could do -1 for "infinite", getting it static_cast to a
UINT64_MAX value along the way. I can't find any documentation for
this behavior, but it seems like something Greg would have done.
We've partially lost that behavior already via
https://github.com/llvm/llvm-project/pull/72233 for
target.max-string-summary-length, and this further removes it.
We're still fetching UInt64's and returning them as uint32_t's
but I'm not overly pressed about someone setting a count/size
limit over 4GB.
I added a simple API test for the memory read setting limit.
More generally speaking, I think our limits on string / children /
bytes are far too low. I don't think people should need to set these
as a regular part of using the commands. I know the danger is
always printing an uninitialized char* and reading 16MB before we
hit a nul byte, or having some ValueObject chain that is following
a circular linked list and never terminates, or dumping 4MB of
memory to console when someone confuses size/count and start address.
But I think I'd prefer much higher limits on all of these, and
having people be annoyed in those cases instead of annoyed when
lldb won't print a paragraph-length c-string, or reading more than
1k of memory.
---
lldb/source/Target/Target.cpp | 2 +-
lldb/source/Target/TargetProperties.td | 4 +--
.../functionalities/memory/big-read/Makefile | 3 ++
.../big-read/TestMemoryReadMaximumSize.py | 31 +++++++++++++++++++
.../functionalities/memory/big-read/main.c | 9 ++++++
5 files changed, 46 insertions(+), 3 deletions(-)
create mode 100644 lldb/test/API/functionalities/memory/big-read/Makefile
create mode 100644 lldb/test/API/functionalities/memory/big-read/TestMemoryReadMaximumSize.py
create mode 100644 lldb/test/API/functionalities/memory/big-read/main.c
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 5a5d689e03fbc0..260974bddedf3a 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -4609,7 +4609,7 @@ uint32_t TargetProperties::GetMaxZeroPaddingInFloatFormat() const {
uint32_t TargetProperties::GetMaximumNumberOfChildrenToDisplay() const {
const uint32_t idx = ePropertyMaxChildrenCount;
- return GetPropertyAtIndexAs<int64_t>(
+ return GetPropertyAtIndexAs<uint64_t>(
idx, g_target_properties[idx].default_uint_value);
}
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 421252aa4aea26..7bb5bd53688b14 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -92,7 +92,7 @@ let Definition = "target" in {
def MaxZeroPaddingInFloatFormat: Property<"max-zero-padding-in-float-format", "UInt64">,
DefaultUnsignedValue<6>,
Desc<"The maximum number of zeroes to insert when displaying a very small float before falling back to scientific notation.">;
- def MaxChildrenCount: Property<"max-children-count", "SInt64">,
+ def MaxChildrenCount: Property<"max-children-count", "UInt64">,
DefaultUnsignedValue<256>,
Desc<"Maximum number of children to expand in any level of depth.">;
def MaxChildrenDepth: Property<"max-children-depth", "UInt64">,
@@ -101,7 +101,7 @@ let Definition = "target" in {
def MaxSummaryLength: Property<"max-string-summary-length", "UInt64">,
DefaultUnsignedValue<1024>,
Desc<"Maximum number of characters to show when using %s in summary strings.">;
- def MaxMemReadSize: Property<"max-memory-read-size", "SInt64">,
+ def MaxMemReadSize: Property<"max-memory-read-size", "UInt64">,
DefaultUnsignedValue<1024>,
Desc<"Maximum number of bytes that 'memory read' will fetch before --force must be specified.">;
def BreakpointUseAvoidList: Property<"breakpoints-use-platform-avoid-list", "Boolean">,
diff --git a/lldb/test/API/functionalities/memory/big-read/Makefile b/lldb/test/API/functionalities/memory/big-read/Makefile
new file mode 100644
index 00000000000000..10495940055b63
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/big-read/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/memory/big-read/TestMemoryReadMaximumSize.py b/lldb/test/API/functionalities/memory/big-read/TestMemoryReadMaximumSize.py
new file mode 100644
index 00000000000000..259fde71a63626
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/big-read/TestMemoryReadMaximumSize.py
@@ -0,0 +1,31 @@
+"""
+Test the maximum memory read setting.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestMemoryReadMaximumSize(TestBase):
+ def test_memory_read_max_setting(self):
+ """Test the target.max-memory-read-size setting."""
+ self.build()
+ (
+ self.target,
+ self.process,
+ self.thread,
+ self.bp,
+ ) = lldbutil.run_to_source_breakpoint(
+ self, "breakpoint here", lldb.SBFileSpec("main.c")
+ )
+ self.assertTrue(self.bp.IsValid())
+
+ self.expect(
+ "mem rea -f x -s 4 -c 2048 `&c`",
+ error=True,
+ substrs=["Normally, 'memory read' will not read over 1024 bytes of data"],
+ )
+ self.runCmd("settings set target.max-memory-read-size `2048 * sizeof(int)`")
+ self.expect("mem rea -f x -s 4 -c 2048 `&c`", substrs=["feed"])
diff --git a/lldb/test/API/functionalities/memory/big-read/main.c b/lldb/test/API/functionalities/memory/big-read/main.c
new file mode 100644
index 00000000000000..a9143a50d093b8
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/big-read/main.c
@@ -0,0 +1,9 @@
+#include <string.h>
+int main() {
+ int c[2048];
+ memset(c, 0, 2048 * sizeof(int));
+
+ c[2047] = 0xfeed;
+
+ return c[2047]; // breakpoint here
+}
>From 9bf5fc7d7c4e7b17598dbe882c91bc29ab1a121a Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Wed, 21 Aug 2024 16:24:38 -0700
Subject: [PATCH 2/2] Update TestDataFormatterGenericForwardList.py to
recognize new unsigned type for target.max-children-count
---
.../forward_list/TestDataFormatterGenericForwardList.py | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py
index 072a580afe24e4..185a24cf6dce30 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py
@@ -2,7 +2,6 @@
Test lldb data formatter subsystem.
"""
-
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -51,7 +50,7 @@ def do_test(self, stdlib_type):
self.expect(
"settings show target.max-children-count",
matching=True,
- substrs=["target.max-children-count (int) = 256"],
+ substrs=["target.max-children-count (unsigned) = 256"],
)
self.expect(
@@ -132,7 +131,7 @@ def do_test_ptr_and_ref(self, stdlib_type):
self.expect(
"settings show target.max-children-count",
matching=True,
- substrs=["target.max-children-count (int) = 256"],
+ substrs=["target.max-children-count (unsigned) = 256"],
)
self.expect(
More information about the lldb-commits
mailing list