[Lldb-commits] [PATCH] D87442: [lldb][AArch64/Linux] Show memory tagged memory regions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Hopefully last round of cosmetic fixes, and then this should be good.



================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:15
 #include "lldb/Utility/RangeMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatProviders.h"
----------------
I guess this is not used anymore..


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py:737
                     "name",
-                    "error"])
+                    "error"], "Unexpected key \"%s\"" % key)
             self.assertIsNotNone(val)
----------------
I guess you could change this to `self.assertIn(needle, haystack)`


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1309-1313
+    llvm::handleAllErrors(Info.takeError(),
+                          [&Result](const llvm::StringError &e) {
+                            Result.SetErrorToGenericError();
+                            Result.SetErrorString(e.getMessage());
+                          });
----------------
Status has an llvm::Error constructor. Some variation on `Result = Info.takeError()` should work.


================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:196
+    callback(*region);
+  }
+}
----------------
[[ http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | no braces for simple statements]]


================
Comment at: lldb/source/Plugins/Process/minidump/MinidumpParser.cpp:270-272
+                         llvm::handleAllErrors(
+                             region.takeError(),
+                             [](const llvm::StringError &e) {});
----------------
`llvm::consumeError(region.takeError())`, though maybe it would be better to actually log it (LLDB_LOG_ERROR)


================
Comment at: lldb/test/API/linux/aarch64/mte_memory_region/TestAArch64LinuxMTEMemoryRegion.py:31
+        # 47 is our magic status meaning MTE isn't available
+        if "exited with status = 47" in self.res.GetOutput():
+            self.skipTest("MTE must be available in toolchain and on target")
----------------
`if self.process().GetState() == lldb.eStateExited and self.process().GetExitStatus() == 47` would be nicer


================
Comment at: lldb/test/API/linux/aarch64/mte_memory_region/TestAArch64LinuxMTEMemoryRegion.py:34-38
+        lldbutil.run_break_set_by_file_and_line(self, "main.c",
+            line_number('main.c', '// Set break point at this line.'),
+            num_expected_locations=1)
+
+        self.runCmd("run", RUN_SUCCEEDED)
----------------
I think you could just run this once, and then choose to skip the test or not depending on whether the process exited, or hit a breakpoint.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87442/new/

https://reviews.llvm.org/D87442