[lldb] Fix bug where memory read --outfile is not truncating the file

[JDevlieghere]

Summary

The memory read --outfile command should truncate the output when unless --append-outfile. Fix the bug and add a test.

rdar://76062318

[teemperor]

Thanks for the patch! I'll be extra nit-picky about the test to make up for the great meme steal of 2021.

lldb/test/API/functionalities/memory/read/TestMemoryRead.py

146

Single use var (I guess you can't reuse that as a common prefix as the address expression has to be the last arg).

149

self.assertTrue(res.Succeeded(), "memory read failed:" + res.GetError())

154

I believe usually we use self.getBuildArtifact("memory-read-output") for that. If you open a named temp file here (and don't close it), then the second open can fail on Windows.

166

Can we ever hit this? We just get an exception in the for loop above (and the exception won't be very useful to diagnose the failure).

You can do this:

lines = [s.strip() for s in lines]
expected = [s.strip() for s in expected]
self.assertEqual(lines, expected)

(assertEqual actually explains which elements are missing/additional/different for lists)

173

Could you write some garbage into the file between the two tests here?

[JDevlieghere]

In D99890#2671042, @teemperor wrote:

Thanks for the patch! I'll be extra nit-picky about the test to make up for the great meme steal of 2021.

Credit was given, so I do not consider it theft.

[teemperor]

LGTM now beside the now redundant import

lldb/test/API/functionalities/memory/read/TestMemoryRead.py

7

No longer needed

168

I see you have taken my suggestion literally

lldb/source/Commands/CommandObjectMemory.cpp

#undef ALL_KEYWORDS
     std::unique_ptr<Stream> output_stream_storage;
     Stream *output_stream_p = nullptr;
     const FileSpec &outfile_spec =
         m_outfile_options.GetFile().GetCurrentValue();
 
     std::string path = outfile_spec.GetPath();
     if (outfile_spec) {
 
-      auto open_options = File::eOpenOptionWrite | File::eOpenOptionCanCreate;
+      File::OpenOptions open_options =
+          File::eOpenOptionWrite | File::eOpenOptionCanCreate;
       const bool append = m_outfile_options.GetAppend().GetCurrentValue();
-      if (append)
-        open_options |= File::eOpenOptionAppend;
+      open_options |=
+          append ? File::eOpenOptionAppend : File::eOpenOptionTruncate;
 
       auto outfile = FileSystem::Instance().Open(outfile_spec, open_options);
 
       if (outfile) {
         auto outfile_stream_up =
             std::make_unique<StreamFile>(std::move(outfile.get()));
         if (m_memory_options.m_output_as_binary) {
           const size_t bytes_written =

lldb/test/API/functionalities/memory/read/TestMemoryRead.py

 """
 Test the 'memory read' command.
 """
 
-
-
 import lldb
-from lldbsuite.test.lldbtest import *
 import lldbsuite.test.lldbutil as lldbutil
+import tempfile
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
 
 
 class MemoryReadTestCase(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.cpp', '// Set break point at this line.')
 
-    def test_memory_read(self):
-        """Test the 'memory read' command with plain and vector formats."""
+    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.cpp", self.line, num_expected_locations=1, loc_exact=True)
+        lldbutil.run_break_set_by_file_and_line(self,
+                                                "main.cpp",
+                                                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,
+        self.expect("thread list",
+                    STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped', 'stop reason = breakpoint'])
 
         # The breakpoint should have a hit count of 1.
-        self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+        self.expect("breakpoint list -f",
+                    BREAKPOINT_HIT_ONCE,
                     substrs=[' resolved, hit count = 1'])
 
-        # Test the memory read commands.
+    @no_debug_info_test
+    def test_memory_read(self):
+        """Test the 'memory read' command with plain and vector formats."""
+        self.build_run_stop()
 
         # (lldb) memory read -f d -c 1 `&argc`
         # 0x7fff5fbff9a0: 1
         self.runCmd("memory read -f d -c 1 `&argc`")
 
         # Find the starting address for variable 'argc' to verify later that the
         # '--format uint32_t[] --size 4 --count 4' option increments the address
         # correctly.
def test_memory_read(self):
           lines = self.res.GetOutput().splitlines()
           objects_read = []
           for l in lines:
               objects_read.extend(l.split(':')[1].split())
           # Check that we got back 4 0x0000 etc bytes
           for o in objects_read:
               self.assertEqual(len(o), expected_object_length)
           self.assertEquals(len(objects_read), 4)
+
+    @no_debug_info_test
+    def test_memory_read_file(self):
+        self.build_run_stop()
+        cmd = "memory read -f d -c 1 `&argc`"
+        res = lldb.SBCommandReturnObject()
+        self.ci.HandleCommand(cmd, res)
+        self.assertTrue(res.Succeeded())
+
+        # Record golden output.
+        golden_output = res.GetOutput()
+
+        temp_file = tempfile.NamedTemporaryFile().name
+
+        def check_file_content(expected):
+            with open(temp_file) as f:
+                lines = f.readlines()
+                for i, line in enumerate(lines):
+                    expected_line = expected[i].strip()
+                    actual_line = line.strip()
+                    self.assertEqual(
+                        expected_line, actual_line,
+                        "Line {} doesn't match: expected '{}' but got '{}'".
+                        format(i, expected_line, actual_line))
+                self.assertEqual(len(expected), len(lines))
+
+        # Sanity check.
+        self.runCmd("memory read -f d -c 1 -o '{}' `&argc`".format(temp_file))
+        check_file_content([golden_output])
+
+        # Make sure the file is truncated when we run the command again.
+        self.runCmd("memory read -f d -c 1 -o '{}' `&argc`".format(temp_file))
+        check_file_content([golden_output])
+
+        # Make sure the file is appended when we run the command with --append-outfile.
+        self.runCmd(
+            "memory read -f d -c 1 -o '{}' --append-outfile `&argc`".format(
+                temp_file))
+        check_file_content([golden_output, golden_output])