[lldb] Fix 'memory write' to not allow specifying values when writing file contents

[RamNalamothu]

Summary

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.

[DavidSpickett]

lldb/source/Interpreter/CommandObject.cpp

457

(like I said on the other diff)

What does it mean for an arg_entry to be empty? (please add the answer as a comment in the code as well)

I think this is avoiding trying to index [0] on an empty arg lower down:

} else {
  StreamString names;
  for (int j = 0; j < num_alternatives; ++j) {
    if (j > 0)
      names.Printf(" | ");
    names.Printf("%s", GetArgumentName(arg_entry[j].arg_type));
  }

  std::string name_str = std::string(names.GetString());
  switch (arg_entry[0].arg_repetition) {

Which is fine but I'm not sure if arg_entry being empty is unusual enough to prefer an assert rather than skipping over it.

lldb/test/API/commands/memory/write/TestMemoryWrite.py

2

I wasn't expecting such thorough tests, this is great!

50

You don't need to add these comments. The output is clear enough from the expect parameters.

58

I think error=False is the default so you can leave it out here. We tend to add error=True for the few times we want a failure.

82

So here I'd remove the comment and add "error: " to the start of the substr.

95

Again this is the default, the vast majority of the time we want a match so we only call out the few where we don't.

97

You could skip this line. Small thing but we're not testing the general usage printing infrastructure here just the specifics in the following 2 lines.

lldb/test/API/commands/memory/write/main.cpp

1 ↗

(On Diff #389533)

Trivial thing, but this file has no need to be .cpp since it's plain c.

[RamNalamothu]

Address review comments.

[RamNalamothu]

Thanks for the comments.

I wasn't familiar with the Pyhton test infrastructure so I took the files from commands/memory/read and updated as needed here. All the comments etc came from that.

I think I addressed all the feedback.

[DavidSpickett]

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.

lldb/test/API/commands/memory/write/Makefile

3

I was gonna say you can drop this but I see a few other tests with it, and it won't harm anything.

lldb/source/Commands/CommandObjectMemory.cpp

CommandObjectMemoryWrite(CommandInterpreter &interpreter)
 
     // There is only one variant this argument could be; put it into the
     // argument entry.
     arg1.push_back(addr_arg);
 
     // 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.
     arg2.push_back(value_arg);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg1);
     m_arguments.push_back(arg2);
bool DoExecute(Args &command, CommandReturnObject &result) override {
 
     if (m_memory_options.m_infile) {
       if (argc < 1) {
         result.AppendErrorWithFormat(
             "%s takes a destination address when writing file contents.\n",
             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",
           m_cmd_name.c_str());
       return false;
     }
 
     StreamString buffer(

lldb/source/Interpreter/CommandObject.cpp

void CommandObject::GetFormattedCommandArguments(Stream &str,
   int num_args = m_arguments.size();
   for (int i = 0; i < num_args; ++i) {
     if (i > 0)
       str.Printf(" ");
     CommandArgumentEntry arg_entry =
         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)) {
       const char *first_name = GetArgumentName(arg_entry[0].arg_type);
       const char *second_name = GetArgumentName(arg_entry[1].arg_type);
       switch (arg_entry[0].arg_repetition) {
       case eArgRepeatPairPlain:
         str.Printf("<%s> <%s>", first_name, second_name);

lldb/test/API/commands/memory/write/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

lldb/test/API/commands/memory/write/TestMemoryWrite.py

+"""
+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 <format>] [-s <byte-size>] <address> <value> [<value> [...]]",
+                "memory write -i <filename> [-s <byte-size>] [-o <offset>] <address>"])

lldb/test/API/commands/memory/write/file.txt

+abcdefg

lldb/test/API/commands/memory/write/main.c

+#include <stdio.h>
+
+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;
+}