[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents
DavidSpickett accepted this revision. DavidSpickett added a comment. This revision is now accepted and ready to land. > All the comments etc came from that. Yeah it's a bit of a judgement call so it's a safe bet to add them and see what review thinks. I know for sure there are plenty of tests with way more cryptic expectations that don't have comments :) This LGTM. Comment at: lldb/test/API/commands/memory/write/Makefile:2 +C_SOURCES := main.c +CFLAGS_EXTRAS := -std=c99 + I was gonna say you can drop this but I see a few other tests with it, and it won't harm anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114544/new/ https://reviews.llvm.org/D114544 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
DavidSpickett added a comment. Thanks for splitting the patches. Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:30 + m_option_definition[2] = default_opt_defs[2]; + m_option_definition[3] = default_opt_defs[3]; You could use a std::copy for this and not need to spell out each index. Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66 + 0, + eArgTypeCount, + "The number of total items to display."}, RamNalamothu wrote: > DavidSpickett wrote: > > Not sure if clang-format has done this, or just your preferred style. > > Nothing against either but it makes it difficult to tell if anything has > > changed in this particular part of the diff. > > > > In general we want clang-formatted code but if it makes the diff tricky to > > read it's best done in a follow up change that just does formatting. > The clang-format did it. Will mark this with `clang-format off/on`. My bad, what I meant was that it's fine to format it in general but that in this case it should be avoided to make the diff more readable. But now I see that this is essentially a new function so formatting it all is fine. I now see the change you're making, the body of the array/function isn't actually changing. You can remove the `/* clang-format...*/` and just let it do its thing. That said, what is the reason that you couldn't re-use the option table? I have two thoughts about the function: 1. Do we need to remake the array each time, vs the constexpr array from before. 2. We're returning an ArrayRef to a return value, which probably works in some situations (if you immediately do a copy using the ref, then discard it) but I think it's a lifetime issue overall. I'm assuming that ArrayRef is basically std::array& in a general sense, and if you returned a std::array& to something on the stack, that's a lifetime issue for sure. I looked at some other option groups and they all follow the pattern of returning an array ref to a const array defined outside the function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114448/new/ https://reviews.llvm.org/D114448 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114509: [lldb] Introduce PlatformQemuUser
labath added a comment. Thanks for the review. I'm going to wait until the US folks get back before submitting this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114509/new/ https://reviews.llvm.org/D114509 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 94038c5 - [lldb] Fix 'memory write' to not allow specifying values when writing file contents
Author: Venkata Ramanaiah Nalamothu
Date: 2021-11-26T15:50:36+05:30
New Revision: 94038c570fbc991c03fe68793c576314c231d4ee
URL:
https://github.com/llvm/llvm-project/commit/94038c570fbc991c03fe68793c576314c231d4ee
DIFF:
https://github.com/llvm/llvm-project/commit/94038c570fbc991c03fe68793c576314c231d4ee.diff
LOG: [lldb] Fix 'memory write' to not allow specifying values when writing file
contents
Currently the 'memory write' command allows specifying the values when
writing the file contents to memory but the values are actually ignored. This
patch fixes that by erroring out when values are specified in such cases.
Reviewed By: DavidSpickett
Differential Revision: https://reviews.llvm.org/D114544
Added:
lldb/test/API/commands/memory/write/Makefile
lldb/test/API/commands/memory/write/TestMemoryWrite.py
lldb/test/API/commands/memory/write/file.txt
lldb/test/API/commands/memory/write/main.c
Modified:
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/CommandObject.cpp
Removed:
diff --git a/lldb/source/Commands/CommandObjectMemory.cpp
b/lldb/source/Commands/CommandObjectMemory.cpp
index f27d4bd7e4b26..ff508a939f0be 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1240,6 +1240,7 @@ class CommandObjectMemoryWrite : public
CommandObjectParsed {
// Define the first (and only) variant of this arg.
value_arg.arg_type = eArgTypeValue;
value_arg.arg_repetition = eArgRepeatPlus;
+value_arg.arg_opt_set_association = LLDB_OPT_SET_1;
// There is only one variant this argument could be; put it into the
// argument entry.
@@ -1278,6 +1279,12 @@ class CommandObjectMemoryWrite : public
CommandObjectParsed {
m_cmd_name.c_str());
return false;
}
+ if (argc > 1) {
+result.AppendErrorWithFormat(
+"%s takes only a destination address when writing file
contents.\n",
+m_cmd_name.c_str());
+return false;
+ }
} else if (argc < 2) {
result.AppendErrorWithFormat(
"%s takes a destination address and at least one value.\n",
diff --git a/lldb/source/Interpreter/CommandObject.cpp
b/lldb/source/Interpreter/CommandObject.cpp
index 64b23d04abea3..dcae27ff54790 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -454,6 +454,9 @@ void CommandObject::GetFormattedCommandArguments(Stream
&str,
opt_set_mask == LLDB_OPT_SET_ALL
? m_arguments[i]
: OptSetFiltered(opt_set_mask, m_arguments[i]);
+// This argument is not associated with the current option set, so skip it.
+if (arg_entry.empty())
+ continue;
int num_alternatives = arg_entry.size();
if ((num_alternatives == 2) && IsPairType(arg_entry[0].arg_repetition)) {
diff --git a/lldb/test/API/commands/memory/write/Makefile
b/lldb/test/API/commands/memory/write/Makefile
new file mode 100644
index 0..695335e068c0c
--- /dev/null
+++ b/lldb/test/API/commands/memory/write/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/memory/write/TestMemoryWrite.py
b/lldb/test/API/commands/memory/write/TestMemoryWrite.py
new file mode 100644
index 0..852edf95079f7
--- /dev/null
+++ b/lldb/test/API/commands/memory/write/TestMemoryWrite.py
@@ -0,0 +1,83 @@
+"""
+Test the 'memory write' command.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class MemoryWriteTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set break point at this line.')
+
+def build_run_stop(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Break in main() after the variables are assigned values.
+lldbutil.run_break_set_by_file_and_line(self,
+"main.c",
+self.line,
+num_expected_locations=1,
+loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list",
+STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped', 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+lldbutil.check_breakpoint(self, bpno = 1, expected_h
[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents
This revision was automatically updated to reflect the committed changes.
Closed by commit rG94038c570fbc: [lldb] Fix 'memory write' to not
allow specifying values when writing file… (authored by ramana-nvr).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114544/new/
https://reviews.llvm.org/D114544
Files:
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/CommandObject.cpp
lldb/test/API/commands/memory/write/Makefile
lldb/test/API/commands/memory/write/TestMemoryWrite.py
lldb/test/API/commands/memory/write/file.txt
lldb/test/API/commands/memory/write/main.c
Index: lldb/test/API/commands/memory/write/main.c
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/main.c
@@ -0,0 +1,7 @@
+#include
+
+int main() {
+ char my_string[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 0};
+ printf("my_string=%s\n", my_string); // Set break point at this line.
+ return 0;
+}
Index: lldb/test/API/commands/memory/write/file.txt
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/file.txt
@@ -0,0 +1 @@
+abcdefg
Index: lldb/test/API/commands/memory/write/TestMemoryWrite.py
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/TestMemoryWrite.py
@@ -0,0 +1,83 @@
+"""
+Test the 'memory write' command.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class MemoryWriteTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set break point at this line.')
+
+def build_run_stop(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Break in main() after the variables are assigned values.
+lldbutil.run_break_set_by_file_and_line(self,
+"main.c",
+self.line,
+num_expected_locations=1,
+loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list",
+STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped', 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+lldbutil.check_breakpoint(self, bpno = 1, expected_hit_count = 1)
+
+@no_debug_info_test
+def test_memory_write(self):
+"""Test the 'memory write' command for writing values and file contents."""
+self.build_run_stop()
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+self.expect(
+"memory write --format c --size 7 `&my_string` ABCDEFG")
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['ABCDEFG'])
+
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string`",
+substrs=['7 bytes were written'])
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string` ABCDEFG", error=True,
+substrs=['error: memory write takes only a destination address when writing file contents'])
+
+self.expect(
+"memory write --infile file.txt --size 7", error=True,
+substrs=['error: memory write takes a destination address when writing file contents'])
+
+@no_debug_info_test
+def test_memory_write_command_usage_syntax(self):
+"""Test that 'memory write' command usage syntax shows it does not take values when writing file contents."""
+self.expect(
+"help memory write",
+substrs=[
+"memory write [-f ] [-s ] [ [...]]",
+"memory write -i [-s ] [-o ] "])
Index: lldb/test/API/commands/memory/write/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -454,6 +454,9 @@
opt_set
[Lldb-commits] [PATCH] D90876: [lldb] [test] Improve comment on expr-after-step-after-crash tests
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Getting this off my queue. I think the change is fine, but I can also live with the original comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90876/new/ https://reviews.llvm.org/D90876 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89335: [lldb] Generic support for caching register set reads/writes
labath requested changes to this revision. labath added a comment. This revision now requires changes to proceed. I guess this slipped off my radar What's the state of this? Are you still interested in the patch? If yes, we can try again after rebasing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89335/new/ https://reviews.llvm.org/D89335 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114008: Draft PR for the deque, stack, queue lldb data formatters
danilashtefan added a comment.
Hi @labath, Thank you so much for your comment and correction. The initial
version of the test was the following, with array of ValueCheck objects:
`def check(self, var_name, size):
var = self.findVariable(var_name)
self.assertEqual(var.GetNumChildren(), size)
children = []
for i in range(100):
children.insert(0,ValueCheck(value={i, i + 1, i + 2}))
children.append(ValueCheck(value={-i, -(i + 1), -(i + 2)}))
self.expect_var_path(var_name, type=self.getVariableType(var_name),
children=children)`
However, the following error pops up in the `expected_var_path`:
`AssertionError: {99, 100, 101} != None : Checking child with index 0:`. The
reason is that `child.GetValue()`that is invoked inside gives None in case it
is a complex structure. This is why me and @wallace decided to go with a
printing test.
If I did not get you right, I would be glad if you could describe your solution
more, so I can implement it. Many thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114008/new/
https://reviews.llvm.org/D114008
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D112824: [lldb][AArch64] Add MakeTaggedRanges to MemoryTagManager
DavidSpickett updated this revision to Diff 389988.
DavidSpickett added a comment.
- Range based for loop over memory regions
- Reduce number of ->GetRange calls
- Check for overlapping regions up front and error if founda
- Added test that the range invesion check removes ignores tags
- Added the same test to existing MakeTaggedRange tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112824/new/
https://reviews.llvm.org/D112824
Files:
lldb/include/lldb/Target/MemoryTagManager.h
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -165,6 +165,14 @@
llvm::FailedWithMessage(
"End address (0x0) must be greater than the start address (0x1)"));
+ // The inversion check ignores tags in the addresses (MTE tags start at bit
+ // 56).
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRange((lldb::addr_t)1 << 56,
+ ((lldb::addr_t)2 << 56) + 0x10, memory_regions),
+ llvm::FailedWithMessage(
+ "Address range 0x0:0x10 is not in a memory tagged region"));
+
// Adding a single region to cover the whole range
memory_regions.push_back(MakeRegionInfo(0, 0x1000, true));
@@ -247,6 +255,111 @@
ASSERT_EQ(*got, expected_range);
}
+TEST(MemoryTagManagerAArch64MTETest, MakeTaggedRanges) {
+ MemoryTagManagerAArch64MTE manager;
+ MemoryRegionInfos memory_regions;
+
+ // Note that MakeTaggedRanges takes start/end address.
+ // Whereas TagRanges and regions take start address and size.
+
+ // Range must not be inverted
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRanges(1, 0, memory_regions),
+ llvm::FailedWithMessage(
+ "End address (0x0) must be greater than the start address (0x1)"));
+
+ // We remove tags before doing the inversion check, so this is not an error.
+ // Also no regions means no tagged regions returned.
+ // (bit 56 is where MTE tags begin)
+ llvm::Expected> got =
+ manager.MakeTaggedRanges((lldb::addr_t)2 << 56,
+ ((lldb::addr_t)1 << 56) + 0x10, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, std::vector{});
+
+ // Cover whole range, untagged. No ranges returned.
+ memory_regions.push_back(MakeRegionInfo(0, 0x20, false));
+ got = manager.MakeTaggedRanges(0, 0x20, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, std::vector{});
+
+ // Overlapping regions is an error.
+ memory_regions.push_back(MakeRegionInfo(0x10, 0x20, false));
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRanges(0, 1, memory_regions),
+ llvm::FailedWithMessage("Cannot calculate tagged ranges because there "
+ "are overlapping memory regions."));
+ memory_regions.pop_back();
+
+ // Make the region tagged and it'll be the one range returned.
+ memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+ got = manager.MakeTaggedRanges(0, 0x20, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, std::vector{
+ MemoryTagManager::TagRange(0, 0x20)});
+
+ // This region will be trimmed if it's larger than the whole range.
+ memory_regions.clear();
+ memory_regions.push_back(MakeRegionInfo(0, 0x40, true));
+ got = manager.MakeTaggedRanges(0x10, 0x30, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, std::vector{
+ MemoryTagManager::TagRange(0x10, 0x20)});
+
+ memory_regions.clear();
+
+ // Only start of range is tagged, only that is returned.
+ // Start the region just before the requested range to check
+ // we limit the result to the requested range.
+ memory_regions.push_back(MakeRegionInfo(0, 0x20, true));
+ got = manager.MakeTaggedRanges(0x10, 0x100, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, std::vector{
+ MemoryTagManager::TagRange(0x10, 0x10)});
+
+ // Add a tagged region at the end, now we get both
+ // and the middle is untagged.
+ // The range added here is deliberately over the end of the
+ // requested range to show that we trim the end.
+ memory_regions.push_back(MakeRegionInfo(0xE0, 0x40, true));
+ got = manager.MakeTaggedRanges(0x10, 0x110, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+
+ std::vector expected{
+ MemoryTagManager::TagRange(0x10, 0x10),
+ MemoryTagManager::TagRange(0xE0, 0x30)};
+ ASSERT_EQ(*got, expected);
+
+ // Now add a middle tagged region.
+ memory_regions.p
[Lldb-commits] [PATCH] D89335: [lldb] Generic support for caching register set reads/writes
mgorny added a comment. I don't think I really want to work on this right now. Even if it still works reliably, I'm not sure if the gain is really worth the added complexity (and risks). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89335/new/ https://reviews.llvm.org/D89335 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D112824: [lldb][AArch64] Add MakeTaggedRanges to MemoryTagManager
DavidSpickett marked 2 inline comments as done. DavidSpickett added inline comments. Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:148 + // For the logic to work regions must be in ascending order. + // We're going to assume that there are no overlapping regions + // and that each region is granule aligned already. omjavaid wrote: > so what would be the behavior if regions do overlap. Should we return an > error rather than keep going? I've addressed this by checking up front for overlaps in the list. We might see this if: * there is invalid data from the remote * the remote is using a memory protection unit that allows overlaps (Arm M class allows this) Afaik no hosted OS would let you have two mappings that use the same range of virtual memory. There's no good way to reconcile the overlap so an error is what I've gone with. I'll handle this error in the later patch to DumpDataExtractor. It is a shame to walk the regions up front but as mentioned in the comment I found some corner cases that lead to some ugly code that would have its own drawbacks. Given that the eventual goal here is "memory read <...> --show-tags" which is opt in, and we expect a fairly small amount of regions, I think the compromise is worth it. (if M class ever got memory tagging we can make this smarter overall) Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:160 + // we must use an untagged address. + MemoryRegionInfo::RangeType range(RemoveNonAddressBits(addr), len); + range = ExpandToGranule(range); omjavaid wrote: > RemoveNonAddressBits hard-coded but symbols may resolve to higher order bits > containing PACs. For now I only came across code pointers with PACs. But if > you suspect code regions can be inputs to this function then may be make > accommodating changes probably in separate patch. The end goal here is "memory read <...> --show-tags" and that command will have to use the ABI plugin in any case so the addresses this gets will be PAC/TBI/MTE free already. So for now this should be addressed by https://reviews.llvm.org/D103626. (once I convince myself which address should be show in the output but that's not for this issue) I agree that there is a bit of a conflict here with the ABI plugin removing everything vs the MemoryTagManager removing only the tag. However I think that it will work fine in this case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112824/new/ https://reviews.llvm.org/D112824 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D112824: [lldb][AArch64] Add MakeTaggedRanges to MemoryTagManager
DavidSpickett added inline comments. Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:160 + // we must use an untagged address. + MemoryRegionInfo::RangeType range(RemoveNonAddressBits(addr), len); + range = ExpandToGranule(range); DavidSpickett wrote: > omjavaid wrote: > > RemoveNonAddressBits hard-coded but symbols may resolve to higher order > > bits containing PACs. For now I only came across code pointers with PACs. > > But if you suspect code regions can be inputs to this function then may be > > make accommodating changes probably in separate patch. > The end goal here is "memory read <...> --show-tags" and that command will > have to use the ABI plugin in any case so the addresses this gets will be > PAC/TBI/MTE free already. > > So for now this should be addressed by https://reviews.llvm.org/D103626. > (once I convince myself which address should be show in the output but that's > not for this issue) > > I agree that there is a bit of a conflict here with the ABI plugin removing > everything vs the MemoryTagManager removing only the tag. However I think > that it will work fine in this case. Speaking of conflict, `MemoryTagManagerAArch64MTE::RemoveNonAddressBits` should probably be renamed to `RemoveTagBits` but I'll do that as another change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112824/new/ https://reviews.llvm.org/D112824 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114627: [lldb] add new overload for SymbolFile::FindTypes that accepts a scope
lassefolger created this revision.
lassefolger added reviewers: werat, teemperor.
lassefolger requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
Searching for fully qualified types in a SymbolFile can be inefficient
if it contains many types with the same basename. This is because the
SymbolFile does not offer and an interface to search for qualified
types. To get qualified typed you first need to query all types with the
same basename and then filter based on the scope.
This change introduces a new overload that accepts a scope which the
SymbolFile implementations can use to return filtered by scope.
This change also includes an implementation for DWARF that uses this
scope for filtering. All other implementation use the overload without
scope and filter afterwards.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114627
Files:
lldb/include/lldb/Symbol/SymbolFile.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/source/Symbol/SymbolFile.cpp
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -135,6 +135,19 @@
llvm::DenseSet &searched_symbol_files,
TypeMap &types) {}
+void SymbolFile::FindTypes(
+ConstString basename, ConstString scope,
+const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+TypeMap &types) {
+ FindTypes(basename, parent_decl_ctx, max_matches, searched_symbol_files,
+types);
+ TypeClass type_class = eTypeClassAny;
+ types.RemoveMismatchedTypes(std::string(basename.GetCString()),
+ std::string(scope.GetCString()), type_class,
+ true);
+}
+
void SymbolFile::FindTypes(llvm::ArrayRef pattern,
LanguageSet languages,
llvm::DenseSet &searched_symbol_files,
@@ -147,9 +160,11 @@
// We assert that we have to module lock by trying to acquire the lock from a
// different thread. Note that we must abort if the result is true to
// guarantee correctness.
- assert(std::async(std::launch::async,
-[this] { return this->GetModuleMutex().try_lock(); })
- .get() == false &&
+ assert(std::async(
+ std::launch::async,
+ [this] {
+ return this->GetModuleMutex().try_lock();
+ }).get() == false &&
"Module is not locked");
#endif
}
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -191,6 +191,13 @@
const std::string &scope_qualified_name,
std::vector &mangled_names) override;
+ void
+ FindTypes(lldb_private::ConstString name, lldb_private::ConstString scope,
+const lldb_private::CompilerDeclContext &parent_decl_ctx,
+uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+lldb_private::TypeMap &types) override;
+
void
FindTypes(lldb_private::ConstString name,
const lldb_private::CompilerDeclContext &parent_decl_ctx,
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -21,6 +21,7 @@
#include "lldb/Core/StreamFile.h"
#include "lldb/Core/Value.h"
#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/ConstString.h"
#include "lldb/Utility/RegularExpression.h"
#include "lldb/Utility/Scalar.h"
#include "lldb/Utility/StreamString.h"
@@ -1115,7 +1116,8 @@
if (const char *include_path = module_die.GetAttributeValueAsString(
DW_AT_LLVM_include_path, nullptr)) {
FileSpec include_spec(include_path, dwarf_cu->GetPathStyle());
-MakeAbsoluteAndRemap(include_spec, *dwarf_cu, m_objfile_sp->GetModule());
+MakeAbsoluteAndRemap(include_spec, *dwarf_cu,
+ m_objfile_sp->GetModule());
module.search_path = ConstString(include_spec.GetPath());
}
if (const char *sysroot = dwarf_cu->DIE().GetAttributeValueAsString(
@@ -1942,7 +1944,7 @@
block_die = function_die.LookupDeepestBlock(file_vm_addr);
}
- if (!sc.function || ! lookup_block)
+ if (!sc.function || !lookup_block)
return;
Block &block = sc.function->GetBlock(true);
@@ -2330,7 +2332,8 @@
if (log) {
GetObjectFile()->GetModule()->LogMessage(
log,
-"SymbolFileDWARF::FindFunctions (name=\"%s\", name_type_mask=0x%x, sc_list)",
+"SymbolFileDWAR
[Lldb-commits] [PATCH] D114627: [lldb] add new overload for SymbolFile::FindTypes that accepts a scope
lassefolger updated this revision to Diff 390001.
lassefolger added a comment.
rebased on nfc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114627/new/
https://reviews.llvm.org/D114627
Files:
lldb/include/lldb/Symbol/SymbolFile.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/source/Symbol/SymbolFile.cpp
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -135,6 +135,19 @@
llvm::DenseSet &searched_symbol_files,
TypeMap &types) {}
+void SymbolFile::FindTypes(
+ConstString basename, ConstString scope,
+const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+TypeMap &types) {
+ FindTypes(basename, parent_decl_ctx, max_matches, searched_symbol_files,
+types);
+ TypeClass type_class = eTypeClassAny;
+ types.RemoveMismatchedTypes(std::string(basename.GetCString()),
+ std::string(scope.GetCString()), type_class,
+ true);
+}
+
void SymbolFile::FindTypes(llvm::ArrayRef pattern,
LanguageSet languages,
llvm::DenseSet &searched_symbol_files,
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -191,6 +191,13 @@
const std::string &scope_qualified_name,
std::vector &mangled_names) override;
+ void
+ FindTypes(lldb_private::ConstString name, lldb_private::ConstString scope,
+const lldb_private::CompilerDeclContext &parent_decl_ctx,
+uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+lldb_private::TypeMap &types) override;
+
void
FindTypes(lldb_private::ConstString name,
const lldb_private::CompilerDeclContext &parent_decl_ctx,
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -21,6 +21,7 @@
#include "lldb/Core/StreamFile.h"
#include "lldb/Core/Value.h"
#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/ConstString.h"
#include "lldb/Utility/RegularExpression.h"
#include "lldb/Utility/Scalar.h"
#include "lldb/Utility/StreamString.h"
@@ -2419,6 +2420,15 @@
uint32_t max_matches,
llvm::DenseSet &searched_symbol_files,
TypeMap &types) {
+ FindTypes(name, ConstString(), parent_decl_ctx, max_matches,
+searched_symbol_files, types);
+}
+
+void SymbolFileDWARF::FindTypes(
+ConstString name, ConstString scope,
+const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+TypeMap &types) {
std::lock_guard guard(GetModuleMutex());
// Make sure we haven't already searched this SymbolFile before.
if (!searched_symbol_files.insert(this).second)
@@ -2445,10 +2455,16 @@
if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
return;
+ const char *sc = scope.AsCString("");
+ std::string s;
m_index->GetTypes(name, [&](DWARFDIE die) {
if (!DIEInDeclContext(parent_decl_ctx, die))
return true; // The containing decl contexts don't match
+const char *qn = die.GetQualifiedName(s);
+if (strncmp(sc, qn, strlen(sc)))
+ return true;
+
Type *matching_type = ResolveType(die, true, true);
if (!matching_type)
return true;
Index: lldb/include/lldb/Symbol/SymbolFile.h
===
--- lldb/include/lldb/Symbol/SymbolFile.h
+++ lldb/include/lldb/Symbol/SymbolFile.h
@@ -235,6 +235,15 @@
llvm::DenseSet &searched_symbol_files,
TypeMap &types);
+ // Find types in a specific scope.
+ // \param scope
+ // Must be either the fully qualified scope (without leading ::) or empty
+ virtual void
+ FindTypes(ConstString name, ConstString scope,
+const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+llvm::DenseSet &searched_symbol_files,
+TypeMap &types);
+
/// Find types specified by a CompilerContextPattern.
/// \param languages
/// Only return results in these languages.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114627: [lldb] add new overload for SymbolFile::FindTypes that accepts a scope
werat added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2464 +const char *qn = die.GetQualifiedName(s); +if (strncmp(sc, qn, strlen(sc))) Can `GetQualifiedName` return `NULL` string? Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2464-2465 +const char *qn = die.GetQualifiedName(s); +if (strncmp(sc, qn, strlen(sc))) + return true; werat wrote: > Can `GetQualifiedName` return `NULL` string? We can skip this check altogether if the `scope` is empty. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2465 +const char *qn = die.GetQualifiedName(s); +if (strncmp(sc, qn, strlen(sc))) + return true; There's no need to calculate length on every iteration. Also you can make `sc` an `llvm::StringRef` and do something like `StringRef(s).startswith(sc)`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114627/new/ https://reviews.llvm.org/D114627 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
RamNalamothu added inline comments. Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66 + 0, + eArgTypeCount, + "The number of total items to display."}, DavidSpickett wrote: > RamNalamothu wrote: > > DavidSpickett wrote: > > > Not sure if clang-format has done this, or just your preferred style. > > > Nothing against either but it makes it difficult to tell if anything has > > > changed in this particular part of the diff. > > > > > > In general we want clang-formatted code but if it makes the diff tricky > > > to read it's best done in a follow up change that just does formatting. > > The clang-format did it. Will mark this with `clang-format off/on`. > My bad, what I meant was that it's fine to format it in general but that in > this case it should be avoided to make the diff more readable. > > But now I see that this is essentially a new function so formatting it all is > fine. I now see the change you're making, the body of the array/function > isn't actually changing. You can remove the `/* clang-format...*/` and just > let it do its thing. > > That said, what is the reason that you couldn't re-use the option table? > > I have two thoughts about the function: > 1. Do we need to remake the array each time, vs the constexpr array from > before. > 2. We're returning an ArrayRef to a return value, which probably works in > some situations (if you immediately do a copy using the ref, then discard it) > but I think it's a lifetime issue overall. > > I'm assuming that ArrayRef is basically std::array& in a general sense, and > if you returned a std::array& to something on the stack, that's a lifetime > issue for sure. I looked at some other option groups and they all follow the > pattern of returning an array ref to a const array defined outside the > function. Ah, I could re-use the option table. Just an oversite from my end. Thank you for spotting that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114448/new/ https://reviews.llvm.org/D114448 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
RamNalamothu updated this revision to Diff 390022.
RamNalamothu added a comment.
Re-use the existing constexpr option table.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114448/new/
https://reviews.llvm.org/D114448
Files:
lldb/include/lldb/Interpreter/OptionGroupFormat.h
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/OptionGroupFormat.cpp
lldb/test/API/commands/help/TestHelp.py
Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
"help format",
matching=True,
substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f ( --format )", "Specify a format to be used for display.",
+"-s ( --size )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f ( --format )", "The format to use for each of the value to be written.",
+"-s ( --size )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,15 +16,7 @@
using namespace lldb;
using namespace lldb_private;
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
-: m_format(default_format, default_format),
- m_byte_size(default_byte_size, default_byte_size),
- m_count(default_count, default_count), m_prev_gdb_format('x'),
- m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+static constexpr OptionDefinition g_default_option_definitions[] = {
{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
nullptr, {}, 0, eArgTypeFormat,
"Specify a format to be used for display."},
@@ -39,8 +31,34 @@
"The number of total items to display."},
};
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
+: m_format(default_format, default_format),
+ m_byte_size(default_byte_size, default_byte_size),
+ m_count(default_count, default_count), m_prev_gdb_format('x'),
+ m_prev_gdb_size('w') {
+ // Copy the default option definitions.
+ std::copy(std::begin(g_default_option_definitions),
+std::end(g_default_option_definitions),
+std::begin(m_option_definitions));
+
+ for (auto usage_text_tuple : usage_text_vector) {
+switch (std::get<0>(usage_text_tuple)) {
+case eArgTypeFormat:
+ m_option_definitions[0].usage_text = std::get<1>(usage_text_tuple);
+ break;
+case eArgTypeByteSize:
+ m_option_definitions[2].usage_text = std::get<1>(usage_text_tuple);
+ break;
+default:
+ llvm_unreachable("Unimplemented option");
+}
+ }
+}
+
llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
- auto result = llvm::makeArrayRef(g_option_table);
+ auto result = llvm::makeArrayRef(m_option_definitions);
if (m_byte_size.GetDefaultValue() < UINT64_MAX) {
if (m_count.GetDefaultValue() < UINT64_MAX)
return result;
@@ -54,7 +72,7 @@
llvm::StringRef option_arg,
ExecutionContext *execution_context) {
Status error;
- const int short_option = g_option_table[option_idx].short_option;
+ const int short_option = m_option_definitions[option_idx].short_option;
switch (short_option) {
case 'f':
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1222,7 +1222,15 @@
interpreter, "memory write",
"Write to the memory of the current target process.", nullptr,
eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+m_option_group(),
+m_format_options(
+eFormatBytes, 1, UINT64_MAX,
+{std::make_tuple(
+ eArgTypeFo
[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
DavidSpickett accepted this revision. DavidSpickett added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114448/new/ https://reviews.llvm.org/D114448 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 7f05ff8 - [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
Author: Venkata Ramanaiah Nalamothu
Date: 2021-11-26T19:14:26+05:30
New Revision: 7f05ff8be481f6db23615c028280fd92c2080f5f
URL:
https://github.com/llvm/llvm-project/commit/7f05ff8be481f6db23615c028280fd92c2080f5f
DIFF:
https://github.com/llvm/llvm-project/commit/7f05ff8be481f6db23615c028280fd92c2080f5f.diff
LOG: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
Certain commands like 'memory write', 'register read' etc all use
the OptionGroupFormat options but the help usage text for those
options is not customized to those commands.
One such example is:
(lldb) help memory read
-s ( --size )
The size in bytes to use when displaying with the selected
format.
(lldb) help memory write
-s ( --size )
The size in bytes to use when displaying with the selected
format.
This patch allows such commands to overwrite the help text for the options
in the OptionGroupFormat group as needed and fixes help text of memory write.
llvm.org/pr49018.
Reviewed By: DavidSpickett
Differential Revision: https://reviews.llvm.org/D114448
Added:
Modified:
lldb/include/lldb/Interpreter/OptionGroupFormat.h
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/OptionGroupFormat.cpp
lldb/test/API/commands/help/TestHelp.py
Removed:
diff --git a/lldb/include/lldb/Interpreter/OptionGroupFormat.h
b/lldb/include/lldb/Interpreter/OptionGroupFormat.h
index 2d445b8a6c20a..551688b0d25f1 100644
--- a/lldb/include/lldb/Interpreter/OptionGroupFormat.h
+++ b/lldb/include/lldb/Interpreter/OptionGroupFormat.h
@@ -16,6 +16,9 @@
namespace lldb_private {
+typedef std::vector>
+OptionGroupFormatUsageTextVector;
+
// OptionGroupFormat
class OptionGroupFormat : public OptionGroup {
@@ -30,7 +33,10 @@ class OptionGroupFormat : public OptionGroup {
uint64_t default_byte_size =
UINT64_MAX, // Pass UINT64_MAX to disable the "--size" option
uint64_t default_count =
- UINT64_MAX); // Pass UINT64_MAX to disable the "--count" option
+ UINT64_MAX, // Pass UINT64_MAX to disable the "--count" option
+ OptionGroupFormatUsageTextVector usage_text_vector = {}
+ // Use to override default option usage text with the command specific
one
+ );
~OptionGroupFormat() override = default;
@@ -73,6 +79,7 @@ class OptionGroupFormat : public OptionGroup {
char m_prev_gdb_format;
char m_prev_gdb_size;
bool m_has_gdb_format;
+ OptionDefinition m_option_definitions[4];
};
} // namespace lldb_private
diff --git a/lldb/source/Commands/CommandObjectMemory.cpp
b/lldb/source/Commands/CommandObjectMemory.cpp
index ff508a939f0be..094ce6f8558fe 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1222,7 +1222,15 @@ class CommandObjectMemoryWrite : public
CommandObjectParsed {
interpreter, "memory write",
"Write to the memory of the current target process.", nullptr,
eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+m_option_group(),
+m_format_options(
+eFormatBytes, 1, UINT64_MAX,
+{std::make_tuple(
+ eArgTypeFormat,
+ "The format to use for each of the value to be written."),
+ std::make_tuple(
+ eArgTypeByteSize,
+ "The size in bytes to write from input file or each
value.")}),
m_memory_options() {
CommandArgumentEntry arg1;
CommandArgumentEntry arg2;
diff --git a/lldb/source/Interpreter/OptionGroupFormat.cpp
b/lldb/source/Interpreter/OptionGroupFormat.cpp
index 1cc5e70282c1a..a2ca9ff398189 100644
--- a/lldb/source/Interpreter/OptionGroupFormat.cpp
+++ b/lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,15 +16,7 @@
using namespace lldb;
using namespace lldb_private;
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
-: m_format(default_format, default_format),
- m_byte_size(default_byte_size, default_byte_size),
- m_count(default_count, default_count), m_prev_gdb_format('x'),
- m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+static constexpr OptionDefinition g_default_option_definitions[] = {
{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
nullptr, {}, 0, eArgTypeFormat,
"Specify a format to be used for display."},
@@ -39,8 +31,34 @@ static constexpr OptionDefinition g_option_table[] = {
"The number of total items to display."},
};
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t defau
[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f05ff8be481: [Bug 49018][lldb] Fix incorrect help text for
'memory write' command (authored by ramana-nvr, committed by
RamNalamothu).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114448/new/
https://reviews.llvm.org/D114448
Files:
lldb/include/lldb/Interpreter/OptionGroupFormat.h
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Interpreter/OptionGroupFormat.cpp
lldb/test/API/commands/help/TestHelp.py
Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
"help format",
matching=True,
substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f ( --format )", "Specify a format to be used for display.",
+"-s ( --size )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f ( --format )", "The format to use for each of the value to be written.",
+"-s ( --size )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,15 +16,7 @@
using namespace lldb;
using namespace lldb_private;
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
-: m_format(default_format, default_format),
- m_byte_size(default_byte_size, default_byte_size),
- m_count(default_count, default_count), m_prev_gdb_format('x'),
- m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+static constexpr OptionDefinition g_default_option_definitions[] = {
{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
nullptr, {}, 0, eArgTypeFormat,
"Specify a format to be used for display."},
@@ -39,8 +31,34 @@
"The number of total items to display."},
};
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
+: m_format(default_format, default_format),
+ m_byte_size(default_byte_size, default_byte_size),
+ m_count(default_count, default_count), m_prev_gdb_format('x'),
+ m_prev_gdb_size('w') {
+ // Copy the default option definitions.
+ std::copy(std::begin(g_default_option_definitions),
+std::end(g_default_option_definitions),
+std::begin(m_option_definitions));
+
+ for (auto usage_text_tuple : usage_text_vector) {
+switch (std::get<0>(usage_text_tuple)) {
+case eArgTypeFormat:
+ m_option_definitions[0].usage_text = std::get<1>(usage_text_tuple);
+ break;
+case eArgTypeByteSize:
+ m_option_definitions[2].usage_text = std::get<1>(usage_text_tuple);
+ break;
+default:
+ llvm_unreachable("Unimplemented option");
+}
+ }
+}
+
llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
- auto result = llvm::makeArrayRef(g_option_table);
+ auto result = llvm::makeArrayRef(m_option_definitions);
if (m_byte_size.GetDefaultValue() < UINT64_MAX) {
if (m_count.GetDefaultValue() < UINT64_MAX)
return result;
@@ -54,7 +72,7 @@
llvm::StringRef option_arg,
ExecutionContext *execution_context) {
Status error;
- const int short_option = g_option_table[option_idx].short_option;
+ const int short_option = m_option_definitions[option_idx].short_option;
switch (short_option) {
case 'f':
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1222,7 +1222,15 @@
interpreter, "memory write",
"Write to the memory of the current target process.", nullptr,
eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+m_option_group(),
+m_format_option
[Lldb-commits] [PATCH] D114008: Draft PR for the deque, stack, queue lldb data formatters
labath added a comment. You need to construct issue appropriately nested ValueCheck objects. Something like this: children.insert(0,ValueCheck(children=[ValueCheck(value=i), ValueCheck(value=i+1), ValueCheck(i + 2)]) If that doesn't work then that's probably a bug in the ValueCheck matching implementation, as it was definitely written with that mind. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114008/new/ https://reviews.llvm.org/D114008 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 0df5229 - Revert "Reland "[lldb] Remove non address bits when looking up memory regions""
Author: David Spickett
Date: 2021-11-26T15:35:02Z
New Revision: 0df522969a7a0128052bd79182c8d58e00556e2f
URL:
https://github.com/llvm/llvm-project/commit/0df522969a7a0128052bd79182c8d58e00556e2f
DIFF:
https://github.com/llvm/llvm-project/commit/0df522969a7a0128052bd79182c8d58e00556e2f.diff
LOG: Revert "Reland "[lldb] Remove non address bits when looking up memory
regions""
This reverts commit fac3f20de55769d028bd92220e74f22fa57dd4b2.
I found this has broken how we detect the last memory region in
GetMemoryRegions/"memory region" command.
When you're debugging an AArch64 system with pointer authentication,
the ABI plugin will remove the top bit from the end address of the last
user mapped area.
(lldb)
[0xfffdf000-0x0001) rw- [stack]
ABI plugin removes anything above the 48th bit (48 bit virtual addresses
by default on AArch64, leaving an address of 0.
(lldb)
[0x-0x0040) ---
You get back a mapping for 0 and get into an infinite loop.
Added:
Modified:
lldb/include/lldb/Target/Process.h
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/source/Plugins/Process/minidump/ProcessMinidump.h
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
lldb/source/Plugins/Process/scripted/ScriptedProcess.h
lldb/source/Target/Process.cpp
Removed:
lldb/test/API/linux/aarch64/tagged_memory_region/Makefile
lldb/test/API/linux/aarch64/tagged_memory_region/TestAArch64LinuxTaggedMemoryRegion.py
lldb/test/API/linux/aarch64/tagged_memory_region/main.c
diff --git a/lldb/include/lldb/Target/Process.h
b/lldb/include/lldb/Target/Process.h
index 4627502abd25d..e27cb8cbf2aa4 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -1762,7 +1762,7 @@ class Process : public
std::enable_shared_from_this,
///
/// If load_addr is within the address space the process has mapped
/// range_info will be filled in with the start and end of that range as
- /// well as the permissions for that range and range_info. GetMapped will
+ /// well as the permissions for that range and range_info.GetMapped will
/// return true.
///
/// If load_addr is outside any mapped region then range_info will have its
@@ -1771,21 +1771,23 @@ class Process : public
std::enable_shared_from_this,
/// there are no valid mapped ranges between load_addr and the end of the
/// process address space.
///
- /// GetMemoryRegionInfo calls DoGetMemoryRegionInfo. Override that function
in
- /// process subclasses.
+ /// GetMemoryRegionInfo will only return an error if it is unimplemented for
+ /// the current process.
///
/// \param[in] load_addr
- /// The load address to query the range_info for. May include non
- /// address bits, these will be removed by the the ABI plugin if there is
- /// one.
+ /// The load address to query the range_info for.
///
/// \param[out] range_info
/// An range_info value containing the details of the range.
///
/// \return
/// An error value.
- Status GetMemoryRegionInfo(lldb::addr_t load_addr,
- MemoryRegionInfo &range_info);
+ virtual Status GetMemoryRegionInfo(lldb::addr_t load_addr,
+ MemoryRegionInfo &range_info) {
+Status error;
+error.SetErrorString("Process::GetMemoryRegionInfo() not supported");
+return error;
+ }
/// Obtain all the mapped memory regions within this process.
///
@@ -2605,26 +2607,6 @@ void PruneThreadPlans();
virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
Status &error) = 0;
- /// DoGetMemoryRegionInfo is called by GetMemoryRegionInfo after it has
- /// removed non address bits from load_addr. Override this method in
- /// subclasses of Process.
- ///
- /// See GetMemoryRegionInfo for details of the logic.
- ///
- /// \param[in] load_addr
- /// The load address to query the range_info for. (non address bits
- /// removed)
- ///
- /// \param[out] range_info
- /// An range_info value containing the details of the range.
- ///
- /// \return
- /// An error value.
- virtual Status DoGetMemoryRegionInfo(lldb::addr_t load_addr,
- MemoryRegionInfo &range_info) {
-return Status("Process::DoGetMemoryRegi
[Lldb-commits] [PATCH] D112825: [lldb] Add MemoryTagMap class
DavidSpickett updated this revision to Diff 390073.
DavidSpickett added a comment.
- Move to Target/
- Doxygen style comments at top of class
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112825/new/
https://reviews.llvm.org/D112825
Files:
lldb/include/lldb/Target/MemoryTagMap.h
lldb/source/Target/CMakeLists.txt
lldb/source/Target/MemoryTagMap.cpp
lldb/unittests/Target/CMakeLists.txt
lldb/unittests/Target/MemoryTagMapTest.cpp
Index: lldb/unittests/Target/MemoryTagMapTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryTagMapTest.cpp
@@ -0,0 +1,81 @@
+//===-- MemoryTagMapTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/MemoryTagMap.h"
+#include "Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+// In these tests we use the AArch64 MTE tag manager because it is the only
+// implementation of a memory tag manager. MemoryTagMap itself is generic.
+
+TEST(MemoryTagMapTest, EmptyTagMap) {
+ MemoryTagManagerAArch64MTE manager;
+ MemoryTagMap tag_map(&manager);
+
+ tag_map.InsertTags(0, {});
+ ASSERT_TRUE(tag_map.empty());
+ tag_map.InsertTags(0, {0});
+ ASSERT_FALSE(tag_map.empty());
+}
+
+TEST(MemoryTagMapTest, GetTags) {
+ using TagsVec = std::vector>;
+
+ MemoryTagManagerAArch64MTE manager;
+ MemoryTagMap tag_map(&manager);
+
+ // No tags for an address not in the map
+ ASSERT_TRUE(tag_map.GetTags(0, 16).empty());
+
+ tag_map.InsertTags(0, {0, 1});
+
+ // No tags if you read zero length
+ ASSERT_TRUE(tag_map.GetTags(0, 0).empty());
+
+ EXPECT_THAT(tag_map.GetTags(0, 16), ::testing::ContainerEq(TagsVec{0}));
+
+ EXPECT_THAT(tag_map.GetTags(0, 32), ::testing::ContainerEq(TagsVec{0, 1}));
+
+ // Last granule of the range is not tagged
+ EXPECT_THAT(tag_map.GetTags(0, 48),
+ ::testing::ContainerEq(TagsVec{0, 1, llvm::None}));
+
+ EXPECT_THAT(tag_map.GetTags(16, 32),
+ ::testing::ContainerEq(TagsVec{1, llvm::None}));
+
+ // Reading beyond that address gives you no tags at all
+ EXPECT_THAT(tag_map.GetTags(32, 16), ::testing::ContainerEq(TagsVec{}));
+
+ // Address is granule aligned for you
+ // The length here is set such that alignment doesn't produce a 2 granule
+ // range.
+ EXPECT_THAT(tag_map.GetTags(8, 8), ::testing::ContainerEq(TagsVec{0}));
+
+ EXPECT_THAT(tag_map.GetTags(30, 2), ::testing::ContainerEq(TagsVec{1}));
+
+ // Here the length pushes the range into the next granule. When aligned
+ // this produces 2 granules.
+ EXPECT_THAT(tag_map.GetTags(30, 4),
+ ::testing::ContainerEq(TagsVec{1, llvm::None}));
+
+ // A range can also have gaps at the beginning or in the middle.
+ // Add more tags, 1 granule away from the first range.
+ tag_map.InsertTags(48, {3, 4});
+
+ // Untagged first granule
+ EXPECT_THAT(tag_map.GetTags(32, 32),
+ ::testing::ContainerEq(TagsVec{llvm::None, 3}));
+
+ // Untagged middle granule
+ EXPECT_THAT(tag_map.GetTags(16, 48),
+ ::testing::ContainerEq(TagsVec{1, llvm::None, 3}));
+}
Index: lldb/unittests/Target/CMakeLists.txt
===
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -3,6 +3,7 @@
DynamicRegisterInfoTest.cpp
ExecutionContextTest.cpp
MemoryRegionInfoTest.cpp
+ MemoryTagMapTest.cpp
ModuleCacheTest.cpp
PathMappingListTest.cpp
RemoteAwarePlatformTest.cpp
Index: lldb/source/Target/MemoryTagMap.cpp
===
--- /dev/null
+++ lldb/source/Target/MemoryTagMap.cpp
@@ -0,0 +1,64 @@
+//===-- MemoryTagMap.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/MemoryTagMap.h"
+
+using namespace lldb_private;
+
+MemoryTagMap::MemoryTagMap(const MemoryTagManager *manager)
+: m_manager(manager) {
+ assert(m_manager && "valid tag manager required to construct a MemoryTagMap");
+}
+
+void MemoryTagMap::InsertTags(lldb::addr_t addr,
+ const std::vector tags) {
+ // We're assuming that addr has no non address bits and is granule aligned.
+ size_t granule_size = m_manager->GetGranuleSize();
+ for (
[Lldb-commits] [PATCH] D107140: [lldb] Add option to show memory tags in memory read output
DavidSpickett updated this revision to Diff 390076.
DavidSpickett added a comment.
Rebase after changes to previous patches.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107140/new/
https://reviews.llvm.org/D107140
Files:
lldb/include/lldb/Core/DumpDataExtractor.h
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Commands/Options.td
lldb/source/Core/DumpDataExtractor.cpp
lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
Index: lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
===
--- lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
+++ lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
@@ -280,3 +280,128 @@
"\[0x[0-9A-Fa-f]+10, 0x[0-9A-Fa-f]+20\): 0x3 \(mismatch\)\n"
"\[0x[0-9A-Fa-f]+20, 0x[0-9A-Fa-f]+30\): 0x3 \(mismatch\)\n"
"\[0x[0-9A-Fa-f]+30, 0x[0-9A-Fa-f]+40\): 0x0$"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_memory_read_tag_display(self):
+self.setup_mte_test()
+
+# Reading from an untagged range should not be any different
+self.expect("memory read non_mte_buf non_mte_buf+16",
+substrs=["tag"], matching=False)
+
+# show-tags option is required
+self.expect("memory read mte_buf mte_buf+32 -f \"x\" -l 1 -s 16",
+patterns=["tag"], matching=False)
+
+# Reading 16 bytes per line means 1 granule and so 1 tag per line
+self.expect("memory read mte_buf mte_buf+32 -f \"x\" -l 1 -s 16 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+00: 0x0+ \(tag: 0x0\)\n"
+"0x[0-9A-fa-f]+10: 0x0+ \(tag: 0x1\)"
+])
+
+# If bytes per line is > granule size then you get multiple tags
+# per line.
+self.expect("memory read mte_buf mte_buf+32 -f \"x\" -l 1 -s 32 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+00: 0x0+ \(tags: 0x0 0x1\)\n"
+])
+
+# Reading half a granule still shows you the tag for that granule
+self.expect("memory read mte_buf mte_buf+8 -f \"x\" -l 1 -s 8 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+00: 0x0+ \(tag: 0x0\)\n"
+])
+
+# We can read a whole number of granules but split them over more lines
+# than there are granules. Tags are shown repeated for each applicable line.
+self.expect("memory read mte_buf+32 mte_buf+64 -f \"x\" -l 1 -s 8 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+20: 0x0+ \(tag: 0x2\)\n"
+"0x[0-9A-fa-f]+28: 0x0+ \(tag: 0x2\)\n"
+"0x[0-9A-fa-f]+30: 0x0+ \(tag: 0x3\)\n"
+"0x[0-9A-fa-f]+38: 0x0+ \(tag: 0x3\)"
+])
+
+# Also works if we misalign the start address. Note the first tag is shown
+# only once here and we have a new tag on the last line.
+# (bytes per line == the misalignment here)
+self.expect("memory read mte_buf+32+8 mte_buf+64+8 -f \"x\" -l 1 -s 8 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+28: 0x0+ \(tag: 0x2\)\n"
+"0x[0-9A-fa-f]+30: 0x0+ \(tag: 0x3\)\n"
+"0x[0-9A-fa-f]+38: 0x0+ \(tag: 0x3\)\n"
+"0x[0-9A-fa-f]+40: 0x0+ \(tag: 0x4\)"
+])
+
+# We can do the same thing but where the misaligment isn't equal to
+# bytes per line. This time, some lines cover multiple granules and
+# so show multiple tags.
+self.expect("memory read mte_buf+32+4 mte_buf+64+4 -f \"x\" -l 1 -s 8 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+24: 0x0+ \(tag: 0x2\)\n"
+"0x[0-9A-fa-f]+2c: 0x0+ \(tags: 0x2 0x3\)\n"
+"0x[0-9A-fa-f]+34: 0x0+ \(tag: 0x3\)\n"
+"0x[0-9A-fa-f]+3c: 0x0+ \(tags: 0x3 0x4\)"
+])
+
+# If you read a range that includes non tagged areas those areas
+# simply aren't annotated.
+
+# Initial part of range is untagged
+self.expect("memory read mte_buf-16 mte_buf+32 -f \"x\" -l 1 -s 16 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+f0: 0x0+\n"
+"0x[0-9A-fa-f]+00: 0x0+ \(tag: 0x0\)\n"
+"0x[0-9A-fa-f]+10: 0x0+ \(tag: 0x1\)"
+])
+
+# End of range is untagged
+self.expect("memory read mte_buf+page_size-16 mte_buf+page_size+16 -f \"x\" -l 1 -s 16 --show-tags",
+patterns=[
+"0x[0-9A-fa-f]+f0:
