MemoryRegion: Print "don't know" permission values as such

[labath]

Summary

The permissions in a memory region have ternary states (yes, no, don't
know), but the memory region command only prints in binary, treating
"don't know" as "yes", which is particularly confusing as for instance
the unwinder will treat an unknown value as "no".

This patch makes is so that we distinguish all three states when
printing the values, using "?" to indicate the lack of information. It
is implemented via a special argument to the format provider for the
OptionalBool enumeration.

[clayborg]

Looks good. Do we maybe want to use "unknown" instead of "don't know" when printing out the long for of a MemoryRegionInfo::OptionalBool? Or maybe "???"?

test/Shell/Minidump/memory-region-from-module.yaml

22 ↗

(On Diff #225410)

How do we get two responses for a single address here?

[labath]

In D69106#1712949, @clayborg wrote:

Looks good. Do we maybe want to use "unknown" instead of "don't know" when printing out the long for of a MemoryRegionInfo::OptionalBool? Or maybe "???"?

"don't know" is the name of the enum, and the same thing that the formatter printed previously. I don't think this value is be printed anywhere right now (except maybe some test failure messages), but if you want (I agree the name is somewhat informal), I can rename the change the enum and the formatter string at the same time.

test/Shell/Minidump/memory-region-from-module.yaml

22 ↗

(On Diff #225410)

These are two separate checks for the two FileCheck invocations above. The test runs the same lldb commands on two minidumps (their yaml below) which differ only in that the second one has a MemoryList stream covering the 0x4000 area. The test shows that in the first case (no MemoryList) we get the permissions from the module while in the second one, the minidump data "wins". (In this case I'm not really concerned with who wins, as that shouldn't happen in practice -- I'm mainly interested in making sure that we don't end up with a corrupted/overlapping memory regions)

[clayborg]

Sounds good, thanks for doing this

lldb/include/lldb/Target/MemoryRegionInfo.h

 public:
   using std::vector<lldb_private::MemoryRegionInfo>::vector;
 };
 
 }
 
 namespace llvm {
 template <>
+/// If Options is empty, prints a textual representation of the value. If
+/// Options is a single character, it uses that character for the "yes" value,
+/// while "no" is printed as "-", and "don't know" as "?". This can be used to
+/// print the permissions in the traditional "rwx" form.
 struct format_provider<lldb_private::MemoryRegionInfo::OptionalBool> {
   static void format(const lldb_private::MemoryRegionInfo::OptionalBool &B,
-                     raw_ostream &OS, StringRef Options) {
-    switch(B) {
-    case lldb_private::MemoryRegionInfo::eNo:
-      OS << "no";
-      return;
-    case lldb_private::MemoryRegionInfo::eYes:
-      OS << "yes";
-      return;
-    case lldb_private::MemoryRegionInfo::eDontKnow:
-      OS << "don't know";
-      return;
-    }
-  }
+                     raw_ostream &OS, StringRef Options);
 };
 }
 
 #endif // #ifndef lldb_MemoryRegionInfo_h

lldb/source/Commands/CommandObjectMemory.cpp

if (process_sp) {
             SectionSP section_sp(addr.GetSection());
             if (section_sp) {
               // Got the top most section, not the deepest section
               while (section_sp->GetParent())
                 section_sp = section_sp->GetParent();
               section_name = section_sp->GetName();
             }
           }
-          result.AppendMessageWithFormat(
-              "[0x%16.16" PRIx64 "-0x%16.16" PRIx64 ") %c%c%c%s%s%s%s\n",
+          result.AppendMessageWithFormatv(
+              "[{0:x16}-{1:x16}) {2:r}{3:w}{4:x}{5}{6}{7}{8}\n",
               range_info.GetRange().GetRangeBase(),
-              range_info.GetRange().GetRangeEnd(),
-              range_info.GetReadable() ? 'r' : '-',
-              range_info.GetWritable() ? 'w' : '-',
-              range_info.GetExecutable() ? 'x' : '-', name ? " " : "",
-              name.AsCString(""), section_name ? " " : "",
-              section_name.AsCString(""));
+              range_info.GetRange().GetRangeEnd(), range_info.GetReadable(),
+              range_info.GetWritable(), range_info.GetExecutable(),
+              name ? " " : "", name, section_name ? " " : "", section_name);
           m_prev_end_addr = range_info.GetRange().GetRangeEnd();
           result.SetStatus(eReturnStatusSuccessFinishResult);
         } else {
           result.SetStatus(eReturnStatusFailed);
           result.AppendErrorWithFormat("%s\n", error.AsCString());
         }
       }
     } else {

lldb/source/Target/MemoryRegionInfo.cpp

 //===-- MemoryRegionInfo.cpp ------------------------------------*- C++ -*-===//
 //
 // 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/MemoryRegionInfo.h"
 
+using namespace lldb_private;
+
 llvm::raw_ostream &lldb_private::operator<<(llvm::raw_ostream &OS,
                                             const MemoryRegionInfo &Info) {
-  return OS << llvm::formatv("MemoryRegionInfo([{0}, {1}), {2}, {3}, {4}, {5}, "
-                             "`{6}`, {7}, {8})",
+  return OS << llvm::formatv("MemoryRegionInfo([{0}, {1}), {2:r}{3:w}{4:x}, "
+                             "{5}, `{6}`, {7}, {8})",
                              Info.GetRange().GetRangeBase(),
                              Info.GetRange().GetRangeEnd(), Info.GetReadable(),
                              Info.GetWritable(), Info.GetExecutable(),
                              Info.GetMapped(), Info.GetName(), Info.GetFlash(),
                              Info.GetBlocksize());
 }
+
+void llvm::format_provider<MemoryRegionInfo::OptionalBool>::format(
+    const MemoryRegionInfo::OptionalBool &B, raw_ostream &OS,
+    StringRef Options) {
+  assert(Options.size() <= 1);
+  bool Empty = Options.empty();
+  switch (B) {
+  case lldb_private::MemoryRegionInfo::eNo:
+    OS << (Empty ? "no" : "-");
+    return;
+  case lldb_private::MemoryRegionInfo::eYes:
+    OS << (Empty ? "yes" : Options);
+    return;
+  case lldb_private::MemoryRegionInfo::eDontKnow:
+    OS << (Empty ? "don't know" : "?");
+    return;
+  }
+}

lldb/test/Shell/Minidump/memory-region-from-module.yaml

 # RUN:   -o "memory region 0" -o "memory region 0x4000" \
 # RUN:   -o "memory region 0x5000" -o "memory region 0x6000" \
 # RUN:   | FileCheck --check-prefix=ALL --check-prefix=CHECK2 %s
 
 # ALL-LABEL: (lldb) memory region 0
 # ALL: [0x0000000000000000-0x0000000000004000) ---
 # ALL-LABEL: (lldb) memory region 0x4000
 # CHECK1: [0x0000000000004000-0x00000000000040b0) r-x {{.*}}memory-region-from-module.exe PT_LOAD[0]
-# TODO: This output does not give any indication that the region is only "potentially" writable.
-# CHECK2: [0x0000000000004000-0x0000000000004010) rwx
+# CHECK2: [0x0000000000004000-0x0000000000004010) r??
 # ALL-LABEL: (lldb) memory region 0x5000
 # ALL: [0x0000000000005000-0x000000000000505c) rw- {{.*}}memory-region-from-module.exe PT_LOAD[1]
 # ALL-LABEL: (lldb) memory region 0x6000
 # ALL: [0x000000000000505c-0xffffffffffffffff) ---
 
 --- !minidump
 Streams:
   - Type:            ModuleList