[lldb] Remove limit on max memory read size #105765
Conversation
`memory read` will return an error if you try to read more than 1k bytes in a single command, instructing you to set `target.max-memory-read-size` or use `--force` if you intended to read more than that. This is a safeguard for a command where people are being explicit about how much memory they would like lldb to read (either to display, or save to a file) and is an annoyance every time you need to read more than a small amount. If someone confuses the --count argument with the start address, lldb may begin dumping gigabytes of data but I'd rather that behavior than requiring everyone to special-case their way around a common use case.
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) Changes
I don't want to remove the setting because many people have added (much larger) default max read sizes to their ~/.lldbinit files after hitting this behavior. Another option would be to stop reading/using the value in Target.cpp, but I see no harm in leaving the setting if someone really does prefer to have a small cap on their memory read size. Full diff: https://github.com/llvm/llvm-project/pull/105765.diff 1 Files Affected:
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 7bb5bd53688b14..f553d92f7c64f6 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -102,7 +102,7 @@ let Definition = "target" in {
DefaultUnsignedValue<1024>,
Desc<"Maximum number of characters to show when using %s in summary strings.">;
def MaxMemReadSize: Property<"max-memory-read-size", "UInt64">,
- DefaultUnsignedValue<1024>,
+ DefaultUnsignedValue<4294967295>,
Desc<"Maximum number of bytes that 'memory read' will fetch before --force must be specified.">;
def BreakpointUseAvoidList: Property<"breakpoints-use-platform-avoid-list", "Boolean">,
DefaultTrue,
|
|
(the setting is a UInt64, but Target.cpp reads it and returns a uint32_t, so I only did UINT32_MAX value) |
|
lol my hubris is on display, the PR testing hit a test failure in ... TestMemoryReadMaximumSize.py which I wrote & merged earlier today, and assumes there is a limit of 1024. ;) |
| @@ -102,7 +102,7 @@ let Definition = "target" in { | |||
| DefaultUnsignedValue<1024>, | |||
| Desc<"Maximum number of characters to show when using %s in summary strings.">; | |||
| def MaxMemReadSize: Property<"max-memory-read-size", "UInt64">, | |||
| DefaultUnsignedValue<1024>, | |||
| DefaultUnsignedValue<4294967295>, | |||
There was a problem hiding this comment.
suggestion: Either make it hex, or instead of providing a default value here, initialize the CommandOption to UINT32_MAX
|
Updated patch to a hex constant in the .td file as per Ismail's suggestion, fixed the new TestMemoryReadMaximumSize.py test to work with the new default. |
|
both of the linux PR tests failed in Unwind/trap_frame_sym_ctx.test, I could have sworn the only test failure I had before was in TestMemoryReadMaximumSize.py, I don't see how this PR breaks that test case. Hmm. |
I believe that failure is unrelated to your PR since another user have the exact same issue in theirs: #105757. The issue seems to be resolved by #105695. |
thanks! I want to leave this PR open until next week in case @clayborg has an opinion (this is def his orig code), but I won't look into that fail any further. |
|
@clayborg what do you think about removing the memory limit from memory read? I know this is code you originally wrote, and I might not be thinking of a way for someone to do a huge memory read unintentionally. But it seems like this cap just gets in people's way more than anything else. (I have complaints about our other safety limits like the 1024 byte c-string formatter cap which is really easy to hit with long strings, but those are more debatable ;) |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/4279 Here is the relevant piece of the build log for the reference |
This used to timeout (#101162) now it's aborting (#105765 (comment))
memory readwill return an error if you try to read more than 1k bytes in a single command, instructing you to settarget.max-memory-read-sizeor use--forceif you intended to read more than that. This is a safeguard for a command where people are being explicit about how much memory they would like lldb to read (either to display, or save to a file) and is an annoyance every time you need to read more than a small amount. If someone confuses the --count argument with the start address, lldb may begin dumping gigabytes of data but I'd rather that behavior than requiring everyone to special-case their way around a common use case.I don't want to remove the setting because many people have added (much larger) default max read sizes to their ~/.lldbinit files after hitting this behavior. Another option would be to stop reading/using the value in Target.cpp, but I see no harm in leaving the setting if someone really does prefer to have a small cap on their memory read size.