[Lldb-commits] [PATCH] D148063: [lldb] Parse the crashlog only once

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG88f409194d5a: [lldb] Parse the crashlog only once (authored 
by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148063

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h

Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -47,7 +47,7 @@
 
   Status DoLaunch(Module *exe_module, ProcessLaunchInfo _info) override;
 
-  void DidLaunch() override;
+  void DidResume() override;
 
   Status DoResume() override;
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -168,7 +168,7 @@
   return {};
 }
 
-void ScriptedProcess::DidLaunch() {
+void ScriptedProcess::DidResume() {
   m_pid = GetInterface().GetProcessID();
   GetLoadedDynamicLibrariesInfos();
 }
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -9,20 +9,18 @@
 from lldb.macosx.crashlog import CrashLog,CrashLogParser
 
 class CrashLogScriptedProcess(ScriptedProcess):
-def parse_crashlog(self):
-crashlog_parser = CrashLogParser.create(self.dbg, self.crashlog_path, False)
-crash_log = crashlog_parser.parse()
-
-self.pid = crash_log.process_id
-self.addr_mask = crash_log.addr_mask
-self.crashed_thread_idx = crash_log.crashed_thread_idx
+def set_crashlog(self, crashlog):
+self.crashlog = crashlog
+self.pid = self.crashlog.process_id
+self.addr_mask = self.crashlog.addr_mask
+self.crashed_thread_idx = self.crashlog.crashed_thread_idx
 self.loaded_images = []
-self.exception = crash_log.exception
+self.exception = self.crashlog.exception
 self.app_specific_thread = None
-if hasattr(crash_log, 'asi'):
-self.metadata['asi'] = crash_log.asi
-if hasattr(crash_log, 'asb'):
-self.extended_thread_info = crash_log.asb
+if hasattr(self.crashlog, 'asi'):
+self.metadata['asi'] = self.crashlog.asi
+if hasattr(self.crashlog, 'asb'):
+self.extended_thread_info = self.crashlog.asb
 
 def load_images(self, images):
 #TODO: Add to self.loaded_images and load images in lldb
@@ -38,12 +36,12 @@
 else:
 self.loaded_images.append(image)
 
-for thread in crash_log.threads:
+for thread in self.crashlog.threads:
 if self.load_all_images:
-load_images(self, crash_log.images)
+load_images(self, self.crashlog.images)
 elif thread.did_crash():
 for ident in thread.idents:
-load_images(self, crash_log.find_images_with_identifier(ident))
+load_images(self, self.crashlog.find_images_with_identifier(ident))
 
 if hasattr(thread, 'app_specific_backtrace') and thread.app_specific_backtrace:
 # We don't want to include the Application Specific Backtrace
@@ -92,7 +90,6 @@
 self.crashed_thread_idx = 0
 self.exception = None
 self.extended_thread_info = None
-self.parse_crashlog()
 
 def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
 # NOTE: CrashLogs don't contain any memory.
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -,12 +,17 @@
 launch_info.SetProcessPluginName("ScriptedProcess")
 launch_info.SetScriptedProcessClassName("crashlog_scripted_process.CrashLogScriptedProcess")
 launch_info.SetScriptedProcessDictionary(structured_data)
+launch_info.SetLaunchFlags(lldb.eLaunchFlagStopAtEntry)
+
 error = lldb.SBError()
 process = target.Launch(launch_info, error)
 
 if not process or error.Fail():
 raise InteractiveCrashLogException("couldn't launch Scripted Process", error)
 
+process.GetScriptedImplementation().set_crashlog(crashlog)
+process.Continue()
+
 if not options.skip_status:

[Lldb-commits] [lldb] 88f4091 - [lldb] Parse the crashlog only once

2023-04-11 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2023-04-11T17:05:15-07:00
New Revision: 88f409194d5ac84eb19ee91260a17c6c8b992eda

URL: 
https://github.com/llvm/llvm-project/commit/88f409194d5ac84eb19ee91260a17c6c8b992eda
DIFF: 
https://github.com/llvm/llvm-project/commit/88f409194d5ac84eb19ee91260a17c6c8b992eda.diff

LOG: [lldb] Parse the crashlog only once

Now that we can pass Python objects to the scripted process instance, we
don't need to parse the crashlog twice anymore.

Differential revision: https://reviews.llvm.org/D148063

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/examples/python/scripted_process/crashlog_scripted_process.py
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
lldb/source/Plugins/Process/scripted/ScriptedProcess.h

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index b09c03a50bb7..68ead43ce869 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -,12 +,17 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options, result)
 launch_info.SetProcessPluginName("ScriptedProcess")
 
launch_info.SetScriptedProcessClassName("crashlog_scripted_process.CrashLogScriptedProcess")
 launch_info.SetScriptedProcessDictionary(structured_data)
+launch_info.SetLaunchFlags(lldb.eLaunchFlagStopAtEntry)
+
 error = lldb.SBError()
 process = target.Launch(launch_info, error)
 
 if not process or error.Fail():
 raise InteractiveCrashLogException("couldn't launch Scripted Process", 
error)
 
+process.GetScriptedImplementation().set_crashlog(crashlog)
+process.Continue()
+
 if not options.skip_status:
 @contextlib.contextmanager
 def synchronous(debugger):

diff  --git 
a/lldb/examples/python/scripted_process/crashlog_scripted_process.py 
b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
index 8c32b40d8c8a..b8c05717bb3a 100644
--- a/lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -9,20 +9,18 @@
 from lldb.macosx.crashlog import CrashLog,CrashLogParser
 
 class CrashLogScriptedProcess(ScriptedProcess):
-def parse_crashlog(self):
-crashlog_parser = CrashLogParser.create(self.dbg, self.crashlog_path, 
False)
-crash_log = crashlog_parser.parse()
-
-self.pid = crash_log.process_id
-self.addr_mask = crash_log.addr_mask
-self.crashed_thread_idx = crash_log.crashed_thread_idx
+def set_crashlog(self, crashlog):
+self.crashlog = crashlog
+self.pid = self.crashlog.process_id
+self.addr_mask = self.crashlog.addr_mask
+self.crashed_thread_idx = self.crashlog.crashed_thread_idx
 self.loaded_images = []
-self.exception = crash_log.exception
+self.exception = self.crashlog.exception
 self.app_specific_thread = None
-if hasattr(crash_log, 'asi'):
-self.metadata['asi'] = crash_log.asi
-if hasattr(crash_log, 'asb'):
-self.extended_thread_info = crash_log.asb
+if hasattr(self.crashlog, 'asi'):
+self.metadata['asi'] = self.crashlog.asi
+if hasattr(self.crashlog, 'asb'):
+self.extended_thread_info = self.crashlog.asb
 
 def load_images(self, images):
 #TODO: Add to self.loaded_images and load images in lldb
@@ -38,12 +36,12 @@ def load_images(self, images):
 else:
 self.loaded_images.append(image)
 
-for thread in crash_log.threads:
+for thread in self.crashlog.threads:
 if self.load_all_images:
-load_images(self, crash_log.images)
+load_images(self, self.crashlog.images)
 elif thread.did_crash():
 for ident in thread.idents:
-load_images(self, 
crash_log.find_images_with_identifier(ident))
+load_images(self, 
self.crashlog.find_images_with_identifier(ident))
 
 if hasattr(thread, 'app_specific_backtrace') and 
thread.app_specific_backtrace:
 # We don't want to include the Application Specific Backtrace
@@ -92,7 +90,6 @@ def __init__(self, exe_ctx: lldb.SBExecutionContext, args : 
lldb.SBStructuredDat
 self.crashed_thread_idx = 0
 self.exception = None
 self.extended_thread_info = None
-self.parse_crashlog()
 
 def read_memory_at_address(self, addr: int, size: int, error: 
lldb.SBError) -> lldb.SBData:
 # NOTE: CrashLogs don't contain any memory.

diff  --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp 
b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index 5e7f88cc2d86..999edd8f8db7 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++

[Lldb-commits] [PATCH] D148063: [lldb] Parse the crashlog only once

2023-04-11 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D148063

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148063: [lldb] Parse the crashlog only once

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added a reviewer: mib.
Herald added a project: All.
JDevlieghere requested review of this revision.

Now that we can pass Python objects to the scripted process instance, we don't 
need to parse the crashlog twice anymore.


https://reviews.llvm.org/D148063

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h

Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -47,7 +47,7 @@
 
   Status DoLaunch(Module *exe_module, ProcessLaunchInfo _info) override;
 
-  void DidLaunch() override;
+  void DidResume() override;
 
   Status DoResume() override;
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -168,7 +168,7 @@
   return {};
 }
 
-void ScriptedProcess::DidLaunch() {
+void ScriptedProcess::DidResume() {
   m_pid = GetInterface().GetProcessID();
   GetLoadedDynamicLibrariesInfos();
 }
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -9,20 +9,18 @@
 from lldb.macosx.crashlog import CrashLog,CrashLogParser
 
 class CrashLogScriptedProcess(ScriptedProcess):
-def parse_crashlog(self):
-crashlog_parser = CrashLogParser.create(self.dbg, self.crashlog_path, False)
-crash_log = crashlog_parser.parse()
-
-self.pid = crash_log.process_id
-self.addr_mask = crash_log.addr_mask
-self.crashed_thread_idx = crash_log.crashed_thread_idx
+def set_crashlog(self, crashlog):
+self.crashlog = crashlog
+self.pid = self.crashlog.process_id
+self.addr_mask = self.crashlog.addr_mask
+self.crashed_thread_idx = self.crashlog.crashed_thread_idx
 self.loaded_images = []
-self.exception = crash_log.exception
+self.exception = self.crashlog.exception
 self.app_specific_thread = None
-if hasattr(crash_log, 'asi'):
-self.metadata['asi'] = crash_log.asi
-if hasattr(crash_log, 'asb'):
-self.extended_thread_info = crash_log.asb
+if hasattr(self.crashlog, 'asi'):
+self.metadata['asi'] = self.crashlog.asi
+if hasattr(self.crashlog, 'asb'):
+self.extended_thread_info = self.crashlog.asb
 
 def load_images(self, images):
 #TODO: Add to self.loaded_images and load images in lldb
@@ -38,12 +36,12 @@
 else:
 self.loaded_images.append(image)
 
-for thread in crash_log.threads:
+for thread in self.crashlog.threads:
 if self.load_all_images:
-load_images(self, crash_log.images)
+load_images(self, self.crashlog.images)
 elif thread.did_crash():
 for ident in thread.idents:
-load_images(self, crash_log.find_images_with_identifier(ident))
+load_images(self, self.crashlog.find_images_with_identifier(ident))
 
 if hasattr(thread, 'app_specific_backtrace') and thread.app_specific_backtrace:
 # We don't want to include the Application Specific Backtrace
@@ -92,7 +90,6 @@
 self.crashed_thread_idx = 0
 self.exception = None
 self.extended_thread_info = None
-self.parse_crashlog()
 
 def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
 # NOTE: CrashLogs don't contain any memory.
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -,12 +,17 @@
 launch_info.SetProcessPluginName("ScriptedProcess")
 launch_info.SetScriptedProcessClassName("crashlog_scripted_process.CrashLogScriptedProcess")
 launch_info.SetScriptedProcessDictionary(structured_data)
+launch_info.SetLaunchFlags(lldb.eLaunchFlagStopAtEntry)
+
 error = lldb.SBError()
 process = target.Launch(launch_info, error)
 
 if not process or error.Fail():
 raise InteractiveCrashLogException("couldn't launch Scripted Process", error)
 
+process.GetScriptedImplementation().set_crashlog(crashlog)
+process.Continue()
+
 if not options.skip_status:
 @contextlib.contextmanager

[Lldb-commits] [PATCH] D148062: [lldb] Make ObjectFileJSON loadable as a module

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added reviewers: mib, jingham.
Herald added a project: All.
JDevlieghere requested review of this revision.

This patch adds support for creating modules from JSON object files. This is 
necessary for crashlogs (see c17a1f3df70b 
 for more 
details) where we don't have neither the module nor the symbol file.


https://reviews.llvm.org/D148062

Files:
  lldb/include/lldb/Core/Section.h
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Core/Section.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
  lldb/source/Symbol/ObjectFile.cpp
  lldb/test/API/macosx/symbols/TestObjectFileJSON.py

Index: lldb/test/API/macosx/symbols/TestObjectFileJSON.py
===
--- /dev/null
+++ lldb/test/API/macosx/symbols/TestObjectFileJSON.py
@@ -0,0 +1,100 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+import json
+import uuid
+import os
+import shutil
+
+
+class TestObjectFileJSOn(TestBase):
+TRIPLE = "arm64-apple-macosx13.0.0"
+
+def setUp(self):
+TestBase.setUp(self)
+self.source = "main.c"
+
+def emitJSON(self, data, path):
+json_object = json.dumps(data, indent=4)
+json_object_file = self.getBuildArtifact("a.json")
+with open(json_object_file, "w") as outfile:
+outfile.write(json_object)
+
+def toModuleSpec(self, path):
+module_spec = lldb.SBModuleSpec()
+module_spec.SetFileSpec(lldb.SBFileSpec(path))
+return module_spec
+
+@no_debug_info_test
+def test_target(self):
+data = {
+"triple": self.TRIPLE,
+"uuid": str(uuid.uuid4()),
+"type": "executable",
+}
+
+json_object_file = self.getBuildArtifact("a.json")
+self.emitJSON(data, json_object_file)
+
+target = self.dbg.CreateTarget(json_object_file)
+self.assertTrue(target.IsValid())
+self.assertEqual(target.GetTriple(), self.TRIPLE)
+
+@no_debug_info_test
+def test_module(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+target = self.dbg.CreateTarget(exe)
+
+data = {
+"triple": self.TRIPLE,
+"uuid": str(uuid.uuid4()),
+}
+
+json_object_file = self.getBuildArtifact("a.json")
+self.emitJSON(data, json_object_file)
+
+module = target.AddModule(self.toModuleSpec(json_object_file))
+self.assertFalse(module.IsValid())
+
+data = {
+"triple": self.TRIPLE,
+"uuid": str(uuid.uuid4()),
+"type": "sharedlibrary",
+"sections": [
+{
+"name": "__TEXT",
+"type": "code",
+"address": 0,
+"size": 0x222,
+}
+],
+"symbols": [
+{
+"name": "foo",
+"address": 0x100,
+"size": 0x11,
+}
+],
+}
+self.emitJSON(data, json_object_file)
+
+module = target.AddModule(self.toModuleSpec(json_object_file))
+self.assertTrue(module.IsValid())
+
+section = module.GetSectionAtIndex(0)
+self.assertTrue(section.IsValid())
+self.assertEqual(section.GetName(), "__TEXT")
+self.assertEqual(section.file_addr, 0x0)
+self.assertEqual(section.size, 0x222)
+
+symbol = module.FindSymbol("foo")
+self.assertTrue(symbol.IsValid())
+self.assertEqual(symbol.addr.GetFileAddress(), 0x100)
+self.assertEqual(symbol.GetSize(), 0x11)
+
+error = target.SetSectionLoadAddress(section, 0x1000)
+self.assertSuccess(error)
+self.assertEqual(symbol.addr.GetLoadAddress(target), 0x1100)
Index: lldb/source/Symbol/ObjectFile.cpp
===
--- lldb/source/Symbol/ObjectFile.cpp
+++ lldb/source/Symbol/ObjectFile.cpp
@@ -761,3 +761,34 @@
   m_cache_hash = llvm::djbHash(strm.GetString());
   return *m_cache_hash;
 }
+
+namespace llvm {
+namespace json {
+
+bool fromJSON(const llvm::json::Value ,
+  lldb_private::ObjectFile::Type , llvm::json::Path path) {
+  if (auto str = value.getAsString()) {
+type = llvm::StringSwitch(*str)
+   .Case("corefile", ObjectFile::eTypeCoreFile)
+   .Case("executable", ObjectFile::eTypeExecutable)
+   .Case("debuginfo", ObjectFile::eTypeDebugInfo)
+   .Case("dynamiclinker", ObjectFile::eTypeDynamicLinker)
+   .Case("objectfile", ObjectFile::eTypeObjectFile)
+   .Case("sharedlibrary", ObjectFile::eTypeSharedLibrary)
+

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

2023-04-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda updated this revision to Diff 512610.
jasonmolenda added a comment.

Update the patch to incorporate David's first feedback.  I know this is a bit 
of a tricky patch to review because it is 95% comments/documentation/renaming 
of variables, followed by the one key change of removing `core >= 
ArchSpec::eCore_arm_generic && core <= ArchSpec::eCore_arm_aarch64)` which 
means ARM systems won't follow this MIPS false-watchpoint-hit behavior any 
more, for a watchpoint trap address outside the address range being watched 
(but DOES access the watched range on ARM).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Target/StopInfo.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Target/StopInfo.cpp

Index: lldb/source/Target/StopInfo.cpp
===
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -666,9 +666,8 @@
 WatchpointSP watchpoint_sp;
   };
 
-  StopInfoWatchpoint(Thread , break_id_t watch_id,
- lldb::addr_t watch_hit_addr)
-  : StopInfo(thread, watch_id), m_watch_hit_addr(watch_hit_addr) {}
+  StopInfoWatchpoint(Thread , break_id_t watch_id, bool silently_skip_wp)
+  : StopInfo(thread, watch_id), m_silently_skip_wp(silently_skip_wp) {}
 
   ~StopInfoWatchpoint() override = default;
 
@@ -893,27 +892,9 @@
 
 WatchpointSentry sentry(process_sp, wp_sp);
 
-/*
- * MIPS: Last 3bits of the watchpoint address are masked by the kernel.
- * For example:
- * 'n' is at 0x120010d00 and 'm' is 0x120010d04. When a watchpoint is
- * set at 'm', then
- * watch exception is generated even when 'n' is read/written. To handle
- * this case,
- * server emulates the instruction at PC and finds the base address of
- * the load/store
- * instruction and appends it in the description of the stop-info
- * packet. If watchpoint
- * is not set on this address by user then this do not stop.
-*/
-if (m_watch_hit_addr != LLDB_INVALID_ADDRESS) {
-  WatchpointSP wp_hit_sp =
-  thread_sp->CalculateTarget()->GetWatchpointList().FindByAddress(
-  m_watch_hit_addr);
-  if (!wp_hit_sp) {
-m_should_stop = false;
-wp_sp->IncrementFalseAlarmsAndReviseHitCount();
-  }
+if (m_silently_skip_wp) {
+  m_should_stop = false;
+  wp_sp->IncrementFalseAlarmsAndReviseHitCount();
 }
 
 if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
@@ -1035,7 +1016,17 @@
   
   bool m_should_stop = false;
   bool m_should_stop_is_valid = false;
-  lldb::addr_t m_watch_hit_addr;
+  // A false watchpoint hit has happened -
+  // the thread stopped with a watchpoint
+  // hit notification, but the watched region
+  // was not actually accessed (as determined
+  // by the gdb stub we're talking to).
+  // Continue past this watchpoint without
+  // notifying the user; on some targets this
+  // may mean disable wp, instruction step,
+  // re-enable wp, continue.
+  // On others, just continue.
+  bool m_silently_skip_wp = false;
   bool m_step_over_plan_complete = false;
   bool m_using_step_over_plan = false;
 };
@@ -1372,10 +1363,11 @@
   return StopInfoSP(new StopInfoBreakpoint(thread, break_id, should_stop));
 }
 
-StopInfoSP
-StopInfo::CreateStopReasonWithWatchpointID(Thread , break_id_t watch_id,
-   lldb::addr_t watch_hit_addr) {
-  return StopInfoSP(new StopInfoWatchpoint(thread, watch_id, watch_hit_addr));
+StopInfoSP StopInfo::CreateStopReasonWithWatchpointID(Thread ,
+  break_id_t watch_id,
+  bool silently_continue) {
+  return StopInfoSP(
+  new StopInfoWatchpoint(thread, watch_id, silently_continue));
 }
 
 StopInfoSP StopInfo::CreateStopReasonWithSignal(Thread , int signo,
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1749,33 +1749,60 @@
 } else if (reason == "trap") {
   // Let the trap just use the standard signal stop reason below...
 } else if (reason == "watchpoint") {
+  // We will have between 1 and 3 fields in the description.
+  //
+  // \a wp_addr which is the original start address that
+  // lldb requested be watched, or an address that the
+  // hardware reported.  This address should be within the
+

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

2023-04-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added inline comments.



Comment at: lldb/docs/lldb-gdb-remote.txt:1609
+//   There may be false-positive watchpoint hits on AArch64 as well,
+//   in the SVE Streaming Mode, but that is less common (v. ESR 
+//   register flag "WPF", "Watchpoint might be False-Positive") and

DavidSpickett wrote:
> What does this "v." mean? Is it short for "see here " or some kind of 
> citation?
Yeah, maybe not widely known.  "v." short for "vide" latin, see. cf 
https://latinlexicon.org/LNS_abbreviations.php ;)



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:62
+  //   \a address is 0x1000
+  //   size is 8
+  //   If a one-byte write to 0x1006 is the most recent watchpoint trap,

DavidSpickett wrote:
> Worth noting why here? (minimum watch size and alignment I guess)
I was taking notes as I read the source to understand how arm linux behaved, 
and left those notes in the header, maybe I should just remove them. The DREG 
struct has three addresses in it, and it wasn't really clear to me what was 
stored in them.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2232-2235
+// Rewrite this to mimic the "reason:watchpoint" and
+// "description:ADDR" entries in the
+// stop-reply packet, handled in
+// ProcessGDBRemote::SetThreadStopInfo

DavidSpickett wrote:
> Is this a note to self or is this code now actually doing that?
Sorry that wasn't clear.  We can receive watchpoint notifications one of three 
ways:  `watch`/`awatch`/`rwatch` (standard gdb RSP), `reason:watchpoint` with a 
`description` asciihex string of 1-to-3 numbers, and on Darwin systems, a mach 
exception.  ProcessGDBRemote rewrites watch/awatch/rwatch to 
`reason:watchpoint` + `description` with only the one address, and that's the 
only thing decoded into a StopInfo object.



Comment at: lldb/source/Target/StopInfo.cpp:1019
   bool m_should_stop_is_valid = false;
-  lldb::addr_t m_watch_hit_addr;
+  bool m_silently_skip = false;
   bool m_step_over_plan_complete = false;

DavidSpickett wrote:
> Please document this here as well.
> 
> I am not 100% whether silently skip true means always silently skip this 
> watchpoint, or silently skip out of range hits attributed to this watchpoint.
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148053: [lldb] Parse image high address from JSON crashlogs

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb67b7bb2f735: [lldb] Parse image high address from JSON 
crashlogs (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148053

Files:
  lldb/examples/python/crashlog.py


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -510,7 +510,8 @@
 for json_image in json_images:
 img_uuid = uuid.UUID(json_image['uuid'])
 low = int(json_image['base'])
-high = int(0)
+high = low + int(
+json_image['size']) if 'size' in json_image else low
 name = json_image['name'] if 'name' in json_image else ''
 path = json_image['path'] if 'path' in json_image else ''
 version = ''


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -510,7 +510,8 @@
 for json_image in json_images:
 img_uuid = uuid.UUID(json_image['uuid'])
 low = int(json_image['base'])
-high = int(0)
+high = low + int(
+json_image['size']) if 'size' in json_image else low
 name = json_image['name'] if 'name' in json_image else ''
 path = json_image['path'] if 'path' in json_image else ''
 version = ''
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] b67b7bb - [lldb] Parse image high address from JSON crashlogs

2023-04-11 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2023-04-11T14:46:49-07:00
New Revision: b67b7bb2f73568efac077de189f86e86d9e010d6

URL: 
https://github.com/llvm/llvm-project/commit/b67b7bb2f73568efac077de189f86e86d9e010d6
DIFF: 
https://github.com/llvm/llvm-project/commit/b67b7bb2f73568efac077de189f86e86d9e010d6.diff

LOG: [lldb] Parse image high address from JSON crashlogs

Use the base + size to correctly populate the image high address when
parsing JSON crashlogs.

Differential revision: https://reviews.llvm.org/D148053

Added: 


Modified: 
lldb/examples/python/crashlog.py

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index 6a5c560238eeb..b09c03a50bb75 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -510,7 +510,8 @@ def parse_images(self, json_images):
 for json_image in json_images:
 img_uuid = uuid.UUID(json_image['uuid'])
 low = int(json_image['base'])
-high = int(0)
+high = low + int(
+json_image['size']) if 'size' in json_image else low
 name = json_image['name'] if 'name' in json_image else ''
 path = json_image['path'] if 'path' in json_image else ''
 version = ''



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148053: [lldb] Parse image high address from JSON crashlogs

2023-04-11 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D148053

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148053: [lldb] Parse image high address from JSON crashlogs

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added a reviewer: mib.
Herald added a project: All.
JDevlieghere requested review of this revision.

Correctly parse the image high address from JSON crashlogs.

This is invisible in the output but necessary for a follow up patch that will 
test the new behavior.


https://reviews.llvm.org/D148053

Files:
  lldb/examples/python/crashlog.py


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -510,7 +510,8 @@
 for json_image in json_images:
 img_uuid = uuid.UUID(json_image['uuid'])
 low = int(json_image['base'])
-high = int(0)
+high = low + int(
+json_image['size']) if 'size' in json_image else low
 name = json_image['name'] if 'name' in json_image else ''
 path = json_image['path'] if 'path' in json_image else ''
 version = ''


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -510,7 +510,8 @@
 for json_image in json_images:
 img_uuid = uuid.UUID(json_image['uuid'])
 low = int(json_image['base'])
-high = int(0)
+high = low + int(
+json_image['size']) if 'size' in json_image else low
 name = json_image['name'] if 'name' in json_image else ''
 path = json_image['path'] if 'path' in json_image else ''
 version = ''
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148050: [lldb] Change formatter helper function parameter list to remove ConstString

2023-04-11 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib accepted this revision.
mib 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/D148050/new/

https://reviews.llvm.org/D148050

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147820: debugserver: move AArch64 watchpoint traps within a watchpointed region, parse ESR flags and send them to lldb

2023-04-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda updated this revision to Diff 512577.
jasonmolenda added a comment.

Update patch to incorporate Jonas' & David's feedback.  I'm holding this patch 
until I can get the lldb changes to fix the reason:watchpoint `description` 
handling corrected, or the test case in this patch will fail.  This patch 
changes debugserver's watchpoint notification to be the same as AArch64 
lldb-server, but that currently skips over watchpoint traps that originate 
outside the watched region and the test case will fail with that behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147820

Files:
  lldb/test/API/commands/watchpoints/unaligned-watchpoint/Makefile
  
lldb/test/API/commands/watchpoints/unaligned-watchpoint/TestUnalignedWatchpoint.py
  lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c
  lldb/tools/debugserver/source/DNBBreakpoint.cpp
  lldb/tools/debugserver/source/DNBBreakpoint.h
  lldb/tools/debugserver/source/DNBDefs.h
  lldb/tools/debugserver/source/MacOSX/MachException.cpp
  lldb/tools/debugserver/source/MacOSX/MachProcess.h
  lldb/tools/debugserver/source/MacOSX/MachProcess.mm
  lldb/tools/debugserver/source/RNBRemote.cpp

Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -2841,11 +2841,21 @@
   InitializeRegisters();
 
 if (g_reg_entries != NULL) {
+  auto interesting_regset = [](int regset) -> bool {
+#if defined(__arm64__) || defined(__aarch64__)
+// GPRs and exception registers, helpful for debugging
+// from packet logs.
+return regset == 1 || regset == 3;
+#else
+return regset == 1;
+#endif
+  };
+
   DNBRegisterValue reg_value;
   for (uint32_t reg = 0; reg < g_num_reg_entries; reg++) {
 // Expedite all registers in the first register set that aren't
 // contained in other registers
-if (g_reg_entries[reg].nub_info.set == 1 &&
+if (interesting_regset(g_reg_entries[reg].nub_info.set) &&
 g_reg_entries[reg].nub_info.value_regs == NULL) {
   if (!DNBThreadGetRegisterValueByID(
   pid, tid, g_reg_entries[reg].nub_info.set,
@@ -2860,6 +2870,50 @@
 
 if (did_exec) {
   ostrm << "reason:exec;";
+} else if (tid_stop_info.reason == eStopTypeWatchpoint) {
+  ostrm << "reason:watchpoint;";
+  ostrm << "description:";
+  std::ostringstream wp_desc;
+  wp_desc << tid_stop_info.details.watchpoint.addr << " ";
+  wp_desc << tid_stop_info.details.watchpoint.hw_idx << " ";
+  wp_desc << tid_stop_info.details.watchpoint.mach_exception_addr;
+  append_hexified_string(ostrm, wp_desc.str());
+  ostrm << ";";
+
+  // Temporarily, print all of the fields we've parsed out of the ESR
+  // on a watchpoint exception.  Normally this is something we would
+  // log for LOG_WATCHPOINTS only, but this was implemented from the
+  // ARM ARM spec and hasn't been exercised on real hardware that can
+  // set most of these fields yet.  It may need to be debugged in the
+  // future, so include all of these purely for debugging by human reasons.
+  ostrm << "watch_addr:" << std::hex
+<< tid_stop_info.details.watchpoint.addr << ";";
+  ostrm << "me_watch_addr:" << std::hex
+<< tid_stop_info.details.watchpoint.mach_exception_addr << ";";
+  ostrm << "wp_hw_idx:" << std::hex
+<< tid_stop_info.details.watchpoint.hw_idx << ";";
+  if (tid_stop_info.details.watchpoint.esr_fields_set) {
+ostrm << "wp_esr_iss:" << std::hex
+  << tid_stop_info.details.watchpoint.esr_fields.iss << ";";
+ostrm << "wp_esr_wpt:" << std::hex
+  << tid_stop_info.details.watchpoint.esr_fields.wpt << ";";
+ostrm << "wp_esr_wptv:"
+  << tid_stop_info.details.watchpoint.esr_fields.wptv << ";";
+ostrm << "wp_esr_wpf:"
+  << tid_stop_info.details.watchpoint.esr_fields.wpf << ";";
+ostrm << "wp_esr_fnp:"
+  << tid_stop_info.details.watchpoint.esr_fields.fnp << ";";
+ostrm << "wp_esr_vncr:"
+  << tid_stop_info.details.watchpoint.esr_fields.vncr << ";";
+ostrm << "wp_esr_fnv:"
+  << tid_stop_info.details.watchpoint.esr_fields.fnv << ";";
+ostrm << "wp_esr_cm:" << tid_stop_info.details.watchpoint.esr_fields.cm
+  << ";";
+ostrm << "wp_esr_wnr:"
+  << tid_stop_info.details.watchpoint.esr_fields.wnr << ";";
+ostrm << "wp_esr_dfsc:" << std::hex
+  << tid_stop_info.details.watchpoint.esr_fields.dfsc << ";";
+  }
 } else if (tid_stop_info.details.exception.type) {
   ostrm << "metype:" << std::hex << tid_stop_info.details.exception.type
 <<

[Lldb-commits] [PATCH] D148050: [lldb] Change formatter helper function parameter list to remove ConstString

2023-04-11 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, jingham, mib, jasonmolenda.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

All of these functions take a ConstString for the type_name,
but this isn't really needed for two reasons:
1.) This parameter is always constructed from a static c-string

  constant.

2.) They are passed along to to `AddTypeSummary` as a StringRef anyway.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148050

Files:
  lldb/include/lldb/DataFormatters/FormattersHelpers.h
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/FormattersHelpers.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -297,23 +297,23 @@
   objc_flags.SetSkipPointers(true);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("struct objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector"), objc_flags);
-  AddCXXSummary(
-  objc_category_sp, lldb_private::formatters::ObjCSELSummaryProvider,
-  "SEL summary provider", ConstString("objc_selector *"), objc_flags);
+"SEL summary provider", "SEL", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "struct objc_selector", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "objc_selector", objc_flags);
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCSELSummaryProvider,
-"SEL summary provider", ConstString("SEL *"), objc_flags);
+"SEL summary provider", "objc_selector *", objc_flags);
+  AddCXXSummary(objc_category_sp,
+lldb_private::formatters::ObjCSELSummaryProvider,
+"SEL summary provider", "SEL *", objc_flags);
 
   AddCXXSummary(objc_category_sp,
 lldb_private::formatters::ObjCClassSummaryProvider,
-"Class summary provider", ConstString("Class"), objc_flags);
+"Class summary provider", "Class", objc_flags);
 
   SyntheticChildren::Flags class_synth_flags;
   class_synth_flags.SetCascades(true).SetSkipPointers(false).SetSkipReferences(
@@ -321,61 +321,62 @@
 
   AddCXXSynthetic(objc_category_sp,
   lldb_private::formatters::ObjCClassSyntheticFrontEndCreator,
-  "Class synthetic children", ConstString("Class"),
-  class_synth_flags);
+  "Class synthetic children", "Class", class_synth_flags);
 
   objc_flags.SetSkipPointers(false);
   objc_flags.SetCascades(true);
   objc_flags.SetSkipReferences(false);
 
   AddStringSummary(objc_category_sp, "${var.__FuncPtr%A}",
-   ConstString("__block_literal_generic"), objc_flags);
+   "__block_literal_generic", objc_flags);
 
-  AddStringSummary(objc_category_sp, "${var.years} years, ${var.months} "
- "months, ${var.days} days, ${var.hours} "
- "hours, ${var.minutes} minutes "
- "${var.seconds} seconds",
-   ConstString("CFGregorianUnits"), objc_flags);
   AddStringSummary(objc_category_sp,
-   "location=${var.location} length=${var.length}",
-   ConstString("CFRange"), objc_flags);
+   "${var.years} years, ${var.months} "
+   "months, ${var.days} days, ${var.hours} "
+   "hours, ${var.minutes} minutes "
+   "${var.seconds} seconds",
+   "CFGregorianUnits", objc_flags);
+  AddStringSummary(objc_category_sp,
+   "location=${var.location} length=${var.length}", "CFRange",
+   objc_flags);
 
   AddStringSummary(objc_category_sp,
-   "location=${var.location}, length=${var.length}",
-   ConstString("NSRange"), objc_flags);
+   "location=${var.location}, length=${var.length}", "NSRange",
+   objc_flags);
   AddStringSummary(objc_category_sp, "(${var.origin}, ${var.size}), ...",
-

[Lldb-commits] [PATCH] D147820: debugserver: move AArch64 watchpoint traps within a watchpointed region, parse ESR flags and send them to lldb

2023-04-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added inline comments.



Comment at: lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c:8-11
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);

DavidSpickett wrote:
> These do what exactly?
These are doing 64-bit writes to `u64_p` which is pointing to the start of the 
`uint8_t buf[8]`, while I'm watching 1 byte in the middle, so the FAR address I 
get is the start of the buffer (and outside of the address range I'm watching). 
 This is the one that lldb currently skips over silently on AArch64 
Linux/FreeBSD.  Doing two of these writes on each source line was something I 
did while testing something else, I didn't really need to write it this way for 
this test case.



Comment at: lldb/tools/debugserver/source/RNBRemote.cpp:2888
+  // set most of these fields yet.  It may need to be debugged in the
+  // future, so include all of these purely for debugging by human reasons.
+  ostrm << "watch_addr:" << std::hex

DavidSpickett wrote:
> "by humans"?
"by jason", really. ;)  I was mostly trying to aid Future Jason who looks at 
this five years from now and wonders why the heck debugserver is sending all of 
these fields to lldb where none of them are used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147820

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 1fa26e6 - [PATCH][lldb] Fix dereference of null pointer.

2023-04-11 Thread Caroline Tice via lldb-commits 

Author: Caroline Tice
Date: 2023-04-11T13:33:03-07:00
New Revision: 1fa26e64fd87c848ff54d08e9a14ea03e01ae645

URL: 
https://github.com/llvm/llvm-project/commit/1fa26e64fd87c848ff54d08e9a14ea03e01ae645
DIFF: 
https://github.com/llvm/llvm-project/commit/1fa26e64fd87c848ff54d08e9a14ea03e01ae645.diff

LOG: [PATCH][lldb] Fix dereference of null pointer.

The function DWARFASTParserClang::ParsePointerToMemberType attempts to make
two pointers and then immediately tries to dereference them, without
verifying that the pointesr were successfully created. Sometimes the pointer
creation fails, and the dereference then causes a segfault. This add a check
that the pointers are non-null before attempting to dereference them.

Added: 
lldb/test/Shell/SymbolFile/DWARF/x86/class-type-nullptr-deref.s

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index cf794854c8431..e6921ca9cacdb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1351,6 +1351,11 @@ TypeSP DWARFASTParserClang::ParsePointerToMemberType(
   Type *class_type =
   dwarf->ResolveTypeUID(attrs.containing_type.Reference(), true);
 
+  // Check to make sure pointers are not NULL before attempting to
+  // dereference them.
+  if ((class_type == nullptr) || (pointee_type == nullptr))
+return nullptr;
+
   CompilerType pointee_clang_type = pointee_type->GetForwardCompilerType();
   CompilerType class_clang_type = class_type->GetForwardCompilerType();
 

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/x86/class-type-nullptr-deref.s 
b/lldb/test/Shell/SymbolFile/DWARF/x86/class-type-nullptr-deref.s
new file mode 100644
index 0..610b45823458b
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/class-type-nullptr-deref.s
@@ -0,0 +1,66 @@
+# Test to verify that, if a class type pointer creation fails (pointer is
+# null), LLDB does not try to dereference the null pointer.
+
+# RUN: llvm-mc --triple x86_64-pc-linux %s --filetype=obj -o %t
+# RUN: %lldb %t -o "target variable x" -o exit 2>&1
+
+# This tests a fix for a crash. If things are working we don't get a segfault.
+
+.type   x,@object   # @x
+.bss
+.globl  x
+x:
+.quad   0   # 0x0
+.size   x, 8
+
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   37  # DW_AT_producer
+.byte   8   # DW_FORM_string
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   2   # Abbreviation Code
+.byte   52  # DW_TAG_variable
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   2   # DW_AT_location
+.byte   24  # DW_FORM_exprloc
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   3   # Abbreviation Code
+.byte   31  # DW_TAG_ptr_to_member_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   29  # DW_AT_containing_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   0   # EOM(3)
+.section.debug_info,"",@progbits
+.Lcu_begin0:
+.long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+.short  5   # DWARF version number
+.byte   1   # DWARF Unit Type
+.byte   8   # Address Size (in bytes)
+.long   .debug_abbrev   # Offset Into Abbrev. Section
+.byte   1   # Abbrev [1] 
DW_TAG_compile_unit

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

2023-04-11 Thread Alex Langford via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6ebf1bc66b89: [lldb] Change return type of 
EventData::GetFlavor (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Utility/Event.h
  lldb/source/API/SBEvent.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/Watchpoint.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/Event.cpp

Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -114,12 +114,9 @@
 
 EventDataBytes::~EventDataBytes() = default;
 
-ConstString EventDataBytes::GetFlavorString() {
-  static ConstString g_flavor("EventDataBytes");
-  return g_flavor;
-}
+llvm::StringRef EventDataBytes::GetFlavorString() { return "EventDataBytes"; }
 
-ConstString EventDataBytes::GetFlavor() const {
+llvm::StringRef EventDataBytes::GetFlavor() const {
   return EventDataBytes::GetFlavorString();
 }
 
@@ -182,6 +179,10 @@
   m_bytes.swap(new_bytes);
 }
 
+llvm::StringRef EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}
+
 #pragma mark -
 #pragma mark EventStructuredData
 
@@ -200,7 +201,7 @@
 
 // EventDataStructuredData member functions
 
-ConstString EventDataStructuredData::GetFlavor() const {
+llvm::StringRef EventDataStructuredData::GetFlavor() const {
   return EventDataStructuredData::GetFlavorString();
 }
 
@@ -280,7 +281,6 @@
 return StructuredDataPluginSP();
 }
 
-ConstString EventDataStructuredData::GetFlavorString() {
-  static ConstString s_flavor("EventDataStructuredData");
-  return s_flavor;
+llvm::StringRef EventDataStructuredData::GetFlavorString() {
+  return "EventDataStructuredData";
 }
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -150,9 +150,8 @@
 
 // Thread Event Data
 
-ConstString Thread::ThreadEventData::GetFlavorString() {
-  static ConstString g_flavor("Thread::ThreadEventData");
-  return g_flavor;
+llvm::StringRef Thread::ThreadEventData::GetFlavorString() {
+  return "Thread::ThreadEventData";
 }
 
 Thread::ThreadEventData::ThreadEventData(const lldb::ThreadSP thread_sp)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4786,9 +4786,8 @@
 
 Target::TargetEventData::~TargetEventData() = default;
 
-ConstString Target::TargetEventData::GetFlavorString() {
-  static ConstString g_flavor("Target::TargetEventData");
-  return g_flavor;
+llvm::StringRef Target::TargetEventData::GetFlavorString() {
+  return "Target::TargetEventData";
 }
 
 void Target::TargetEventData::Dump(Stream *s) const {
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3929,12 +3929,11 @@
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
-ConstString Process::ProcessEventData::GetFlavorString() {
-  static ConstString g_flavor("Process::ProcessEventData");
-  return g_flavor;
+llvm::StringRef Process::ProcessEventData::GetFlavorString() {
+  return "Process::ProcessEventData";
 }
 
-ConstString Process::ProcessEventData::GetFlavor() const {
+llvm::StringRef Process::ProcessEventData::GetFlavor() const {
   return ProcessEventData::GetFlavorString();
 }
 
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -23,12 +23,11 @@
   return nullptr;
 }
 
-ConstString ProgressEventData::GetFlavorString() {
-  static ConstString g_flavor("ProgressEventData");
-  return g_flavor;
+llvm::StringRef ProgressEventData::GetFlavorString() {
+  return "ProgressEventData";
 }
 
-ConstString ProgressEventData::GetFlavor() const {
+llvm::StringRef ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
@@ -94,12 +93,11 @@
   s->Flush();
 }
 
-ConstString DiagnosticEventData::GetFlavorString() {
-  static ConstString g_flavor("DiagnosticEventData");
-  return g_flavor;
+llvm::StringRef DiagnosticEventData::GetFlavorString() {
+  return "DiagnosticEventData";
 }
 
-ConstString DiagnosticEventData::GetFlavor() const {
+llvm::StringRef DiagnosticEventData::GetFlavor()

[Lldb-commits] [lldb] 6ebf1bc - [lldb] Change return type of EventData::GetFlavor

2023-04-11 Thread Alex Langford via lldb-commits 

Author: Alex Langford
Date: 2023-04-11T10:49:17-07:00
New Revision: 6ebf1bc66b898e942f0f483aba200211c6c8ef31

URL: 
https://github.com/llvm/llvm-project/commit/6ebf1bc66b898e942f0f483aba200211c6c8ef31
DIFF: 
https://github.com/llvm/llvm-project/commit/6ebf1bc66b898e942f0f483aba200211c6c8ef31.diff

LOG: [lldb] Change return type of EventData::GetFlavor

There's no reason these strings need to be in the ConstString
StringPool, they're already string literals with static lifetime.

I plan on addressing other similar functions in follow up commits.

Differential Revision: https://reviews.llvm.org/D147833

Added: 


Modified: 
lldb/include/lldb/Breakpoint/Breakpoint.h
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/include/lldb/Core/DebuggerEvents.h
lldb/include/lldb/Target/Process.h
lldb/include/lldb/Target/Target.h
lldb/include/lldb/Target/Thread.h
lldb/include/lldb/Utility/Event.h
lldb/source/API/SBEvent.cpp
lldb/source/Breakpoint/Breakpoint.cpp
lldb/source/Breakpoint/Watchpoint.cpp
lldb/source/Core/DebuggerEvents.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp
lldb/source/Target/Thread.cpp
lldb/source/Utility/Event.cpp

Removed: 




diff  --git a/lldb/include/lldb/Breakpoint/Breakpoint.h 
b/lldb/include/lldb/Breakpoint/Breakpoint.h
index bd5c377e5014..f2380157f111 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -105,11 +105,11 @@ class Breakpoint : public 
std::enable_shared_from_this,
 
 ~BreakpointEventData() override;
 
-static ConstString GetFlavorString();
-
+static llvm::StringRef GetFlavorString();
+
 Log *GetLogChannel() override;
 
-ConstString GetFlavor() const override;
+llvm::StringRef GetFlavor() const override;
 
 lldb::BreakpointEventType GetBreakpointEventType() const;
 

diff  --git a/lldb/include/lldb/Breakpoint/Watchpoint.h 
b/lldb/include/lldb/Breakpoint/Watchpoint.h
index a5a72e3ad5a1..037be4539066 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -31,9 +31,9 @@ class Watchpoint : public 
std::enable_shared_from_this,
 
 ~WatchpointEventData() override;
 
-static ConstString GetFlavorString();
+static llvm::StringRef GetFlavorString();
 
-ConstString GetFlavor() const override;
+llvm::StringRef GetFlavor() const override;
 
 lldb::WatchpointEventType GetWatchpointEventType() const;
 

diff  --git a/lldb/include/lldb/Core/DebuggerEvents.h 
b/lldb/include/lldb/Core/DebuggerEvents.h
index f9ae67efac32..982b9701 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -7,7 +7,6 @@
 
//===--===//
 
 #include "lldb/Core/ModuleSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Event.h"
 #include "lldb/Utility/StructuredData.h"
 
@@ -27,9 +26,9 @@ class ProgressEventData : public EventData {
 m_id(progress_id), m_completed(completed), m_total(total),
 m_debugger_specific(debugger_specific) {}
 
-  static ConstString GetFlavorString();
+  static llvm::StringRef GetFlavorString();
 
-  ConstString GetFlavor() const override;
+  llvm::StringRef GetFlavor() const override;
 
   void Dump(Stream *s) const override;
 
@@ -93,8 +92,8 @@ class DiagnosticEventData : public EventData {
 
   void Dump(Stream *s) const override;
 
-  static ConstString GetFlavorString();
-  ConstString GetFlavor() const override;
+  static llvm::StringRef GetFlavorString();
+  llvm::StringRef GetFlavor() const override;
 
   static const DiagnosticEventData *
   GetEventDataFromEvent(const Event *event_ptr);
@@ -116,8 +115,8 @@ class SymbolChangeEventData : public EventData {
   SymbolChangeEventData(lldb::DebuggerWP debugger_wp, ModuleSpec module_spec)
   : m_debugger_wp(debugger_wp), m_module_spec(std::move(module_spec)) {}
 
-  static ConstString GetFlavorString();
-  ConstString GetFlavor() const override;
+  static llvm::StringRef GetFlavorString();
+  llvm::StringRef GetFlavor() const override;
 
   static const SymbolChangeEventData *
   GetEventDataFromEvent(const Event *event_ptr);

diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 1857dd0a7ada..255ba8cd4a92 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -411,9 +411,9 @@ class Process : public 
std::enable_shared_from_this,
 
 ~ProcessEventData() override;
 
-static ConstString GetFlavorString();
+static llvm::StringRef GetFlavorString();
 
-ConstString GetFlavor() const override;
+llvm::StringRef GetFlavor() const override;
 
 lldb::ProcessSP GetProcessSP() const { return m_process_wp.lock(); }
 

diff  --git a/lldb/include/lldb/Target/Target.h

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

2023-04-11 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere 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/D147833/new/

https://reviews.llvm.org/D147833

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

2023-04-11 Thread Michael Buch via Phabricator via lldb-commits 
Michael137 added subscribers: aprantl, dblaikie.
Michael137 added inline comments.



Comment at: lldb/test/API/lang/cpp/no_unique_address/main.cpp:38
+ long v = 42;
+} _f3;
+

I haven't checked the reworked logic yet, but it still crashes on this:

```
self.expect_expr("_f3")
```

I'm leaning towards not trying to support older compilers if it gets too 
complicated. Proposing a `DW_AT_no_unique_address` seems like the best option 
to me since that's pretty trivial to implement and requires almost no support 
on the LLDB side.

CC: @dblaikie @aprantl (since both were part of the [[ 
https://www.mail-archive.com/dwarf-discuss@lists.dwarfstd.org/msg00876.html | 
original discussion ]])


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

2023-04-11 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

Thanks for looking into all this, it makes my head hurt thinking about it :)

It will likely take us (Linaro) a while to do the lldb-server changes, but I do 
want to see this fixed eventually. We have some work upcoming for memcopy 
operations that will need special care for watchpoints, so that is a good 
excuse to do it.




Comment at: lldb/docs/lldb-gdb-remote.txt:1609
+//   There may be false-positive watchpoint hits on AArch64 as well,
+//   in the SVE Streaming Mode, but that is less common (v. ESR 
+//   register flag "WPF", "Watchpoint might be False-Positive") and

What does this "v." mean? Is it short for "see here " or some kind of citation?



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:62
+  //   \a address is 0x1000
+  //   size is 8
+  //   If a one-byte write to 0x1006 is the most recent watchpoint trap,

Worth noting why here? (minimum watch size and alignment I guess)



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:64
+  //   If a one-byte write to 0x1006 is the most recent watchpoint trap,
+  //   \a hit_addr is 0x1006
   struct DREG {

Also does this need to use `///` for the doxygen `\a` thing to work?

(I've no idea, Jonas is forever telling me to use `///` though :) )



Comment at: 
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2232-2235
+// Rewrite this to mimic the "reason:watchpoint" and
+// "description:ADDR" entries in the
+// stop-reply packet, handled in
+// ProcessGDBRemote::SetThreadStopInfo

Is this a note to self or is this code now actually doing that?



Comment at: lldb/source/Target/StopInfo.cpp:1019
   bool m_should_stop_is_valid = false;
-  lldb::addr_t m_watch_hit_addr;
+  bool m_silently_skip = false;
   bool m_step_over_plan_complete = false;

Please document this here as well.

I am not 100% whether silently skip true means always silently skip this 
watchpoint, or silently skip out of range hits attributed to this watchpoint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147820: debugserver: move AArch64 watchpoint traps within a watchpointed region, parse ESR flags and send them to lldb

2023-04-11 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

> This patch takes the trap address (FAR register) and finds the nearest 
> watchpoint if it is not contained in any watched region.

From what I remember, the Linux kernel also does this.




Comment at: 
lldb/test/API/commands/watchpoints/unaligned-watchpoint/TestUnalignedWatchpoint.py:5
+associating the fault with the set watchpoint.
+"""
+

Can you add here why "correctly associating" is not as easy as it sounds.

"This requires lldb to handle...".



Comment at: lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c:8-11
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);
+   printf("%lld %lld\n", (*u64_p)++, (*u64_p)++);

These do what exactly?



Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:1425
 
+// return 1 if bit "BIT" is set in "value"
+static uint32_t bit(uint32_t value, uint32_t bit) {

`"bit"`



Comment at: lldb/tools/debugserver/source/RNBRemote.cpp:2888
+  // set most of these fields yet.  It may need to be debugged in the
+  // future, so include all of these purely for debugging by human reasons.
+  ostrm << "watch_addr:" << std::hex

"by humans"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147820

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146965: [lldb] Add support for MSP430 in LLDB.

2023-04-11 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

> Looked into core dumping in LLDB a bit, I think it pretty much requires that 
> there is an OS, and there needs to be an explicit support for the 
> architecture in question. Plus, mspdebug doesn't actually support core files, 
> there is no use for it now even if LLDB can generate it.

That makes sense. I was more thinking about carving useful memory out of a raw 
dump of the RAM in any case, which is exactly what you've done in the test you 
added.

If you wanted to have a corefile like experience I guess you could do that RAM 
dump, load it back into a simulator and connect to that. But regardless, out of 
scope here.




Comment at: 
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py:6
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+class MyResponder(MockGDBServerResponder):

Please add a comment along the lines of: This test ensures that lldb correctly 
handles packets sent by .

Ideally with a link to that stub or its source or the vendor's website, 
whatever's available.

If you can say how you captured the packets that would be good too. I assume 
just connecting and turning on packet logging but worth mentioning in case you 
had to do something unique.



Comment at: 
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py:23
+case 3:
+return 
"T0500:1605;01:baff;02:0500;03:;04:;05:;06:;07:;08:;09:;0a:;0b:;0c:;0d:;0e:;0f:;"
+

Can you generate these with a helper function? Seems like the first bit changes 
and the rest are blanks you could add each time.

Also could you yield each time? Been a long time since I did serious Python but 
I think that could remove the counter.



Comment at: 
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py:29
+def readMemory(self, addr, length):
+match addr:
+case 0x0400:

I don't know that we have recent enough Pythons to use match, when was it added?

I don't have the minimum versions for lldb to hand, I would guess it is 3.7.



Comment at: 
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py:33
+case 0xfe00:
+return 
"28050a050005"
+

For these can you write a helper to generate the bulk of it? And ideally 
describe which bit is the important part. I guess some of this is the stack.



Comment at: 
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py:96
+self.assertEqual(crt0_addr, 0x50a)
+
+

Could you also check that the disassembler works?

I mistakenly thought that msp430 didn't have an llvm backend but it seems that 
it does so it should "just work".



Comment at: lldb/test/API/functionalities/gdb_remote_client/msp430.yaml:1
+--- !ELF
+FileHeader:

I forget if/how you add comments to a YAML file but however it's done, please 
add the C source that was used to generate the program file as a comment at the 
top.


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

https://reviews.llvm.org/D146965

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy added a comment.

In D147606#4246832 , @JDevlieghere 
wrote:

> The change looks fine and regardless of whether this makes sense or even 
> complies with the standard, we should be resilient against it. I would like 
> to see a test though.

It seems that DWARFv5 indeed specifies how to deal with these empty ranges

> A bounded range entry whose beginning and ending address offsets are equal
> (including zero) indicates an empty range and may be ignored.

Also, I kind of searched through the codebase, and noticed that there are 
multiple places like this, and they adapt a similar approach discarding those 
empty ranges.
e.g. in `DWARFDebugRanges.cpp`

// Filter out empty ranges
if (begin < end)
  range_list.Append(DWARFRangeList::Entry(begin + base_addr, end - begin));
  }


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

https://reviews.llvm.org/D147606

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy updated this revision to Diff 512362.
jwnhy added a comment.

Updated the patch to remove unncessary checks and added a testcase.


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

https://reviews.llvm.org/D147606

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s
@@ -0,0 +1,481 @@
+# Test handling of empty DWARF ranges generated by GCC
+
+# RUN: %clang --target=x86_64-pc-linux -o %t %s 
+# RUN: %lldb %t -o "b func" -o "r" -o "expression -T -- i" \
+# RUN: -o "exit" | FileCheck %s
+
+# Failing case was:
+# error: expression failed to parse:
+# error: :1:1: use of undeclared identifier 'i'
+# CHECK: (int) $0 = 10
+	.file	"r.c"
+	.file 1 "r.c"
+	.text
+.Ltext0:
+	.file 0 "" "r.c"
+	.globl	main
+	.type	main, @function
+main:
+.LFB1:
+	.file 1 "r.c"
+	.loc 1 12 12
+	.cfi_startproc
+	.loc 1 12 14
+	movl	b(%rip), %eax
+.LBB5:
+.LBB6:
+	.loc 1 4 3
+.LVL0:
+	.loc 1 5 3
+	.loc 1 5 12
+.LBE6:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB7:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE7:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB8:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE8:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB9:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE9:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB10:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE10:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB11:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE11:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB12:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE12:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB13:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE13:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB14:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE14:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB15:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE15:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB16:
+	.loc 1 5 19
+	.loc 1 5 12
+	.loc 1 9 3
+	.loc 1 9 5 is_stmt 0
+	movl	$0, a(%rip)
+	.loc 1 10 3 is_stmt 1
+.LVL1:
+.LBE16:
+.LBE5:
+	.loc 1 12 26 is_stmt 0
+	movl	$0, %eax
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	main, .-main
+	.globl	b
+	.bss
+	.align 4
+	.type	b, @object
+	.size	b, 4
+b:
+	.zero	4
+	.globl	a
+	.align 4
+	.type	a, @object
+	.size	a, 4
+a:
+	.zero	4
+	.text
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0xd9
+	.value	0x5
+	.byte	0x1
+	.byte	0x8
+	.long	.Ldebug_abbrev0
+	.uleb128 0x3
+	.long	.LASF2
+	.byte	0x1d
+	.long	.LASF0
+	.long	.LASF1
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.long	.Ldebug_line0
+	.uleb128 0x1
+	.string	"a"
+	.byte	0x1
+	.byte	0x5
+	.long	0x41
+	.uleb128 0x9
+	.byte	0x3
+	.quad	a
+	.uleb128 0x4
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.uleb128 0x5
+	.long	0x41
+	.uleb128 0x1
+	.string	"b"
+	.byte	0x2
+	.byte	0xe
+	.long	0x48
+	.uleb128 0x9
+	.byte	0x3
+	.quad	b
+	.uleb128 0x6
+	.long	.LASF3
+	.byte	0x1
+	.byte	0xc
+	.byte	0x5
+	.long	0x41
+	.quad	.LFB1
+	.quad	.LFE1-.LFB1
+	.uleb128 0x1
+	.byte	0x9c
+	.long	0xb0
+	.uleb128 0x7
+	.long	0xb0
+	.quad	.LBB5
+	.quad	.LBE5-.LBB5
+	.byte	0x1
+	.byte	0xc
+	.byte	0xe
+	.uleb128 0x8
+	.long	0xbd
+	.uleb128 0x9
+	.long	.LLRL0
+	.uleb128 0xa
+	.long	0xc7
+	.long	.LLST1
+	.byte	0
+	.byte	0
+	.byte	0
+	.uleb128 0xb
+	.long	.LASF4
+	.byte	0x1
+	.byte	0x3
+	.byte	0xc
+	.long	0x41
+	.byte	0x1
+	.uleb128 0xc
+	.string	"b"
+	.byte	0x1
+	.byte	0x3
+	.byte	0x18
+	.long	0x41
+	.uleb128 0x2
+	.string	"i"
+	.byte	0x4
+	.byte	0x7
+	.long	0x41
+	.uleb128 0xd
+	.uleb128 0x2
+	.string	"c"
+	.byte	0x6
+	.byte	0x9
+	.long	0x41
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0x21
+	.sleb128 1
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x2
+	.uleb128 0x18
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0x21
+	.sleb128 1
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x1f
+	.uleb128 0x1b
+	.uleb128 0x1f
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x10
+	.uleb128 0x17
+	.byte	0
+	.byte	0
+	.uleb128 0x4
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.uleb128 0x5
+	.uleb128 0x35
+	.byte	0
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.uleb128 0x2e
+	.byte	0x1
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x40
+	.uleb128 0x18
+	.uleb128 0x7a
+

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D147606#4249283 , @JDevlieghere 
wrote:

> In D147606#4247462 , @jwnhy wrote:
>
>> In D147606#4246832 , @JDevlieghere 
>> wrote:
>>
>>> The change looks fine and regardless of whether this makes sense or even 
>>> complies with the standard, we should be resilient against it. I would like 
>>> to see a test though.
>>
>> Thanks a lot for the comment, I am new to lldb community, and got one thing 
>> a bit silly to ask.
>> Up to now, a few patches I submitted is kind of "depending on the 
>> compiler-generated" binary?
>> What am I supposed to do to **ensure the compiler generates these 
>> "easy-to-fault" binaries in the test?**
>>
>> Like in this one, not every compiler will generate "empty ranges", and in 
>> the other one that is "DW_OP_div"...
>
> Yes, this would require a test that checks in what the compiler generates (as 
> opposed to the majority of our "API tests" that build test cases from 
> source). This can either be an assembly file (something like 
> `dwarf5-implicit-const.s`) or a YAML file created with `obj2yaml` (something 
> like `section-overlap.yaml`).

You may be able to adapt/extend 
`lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges.s`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

2023-04-11 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy updated this revision to Diff 512350.
jwnhy added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Thanks a lot for the suggestion.
I added a assembly testcase for "DW_OP_div".
Also, since I don't have the permission to push,
if the patch is good to you, could you please 
help me merge it?

Ping.


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

https://reviews.llvm.org/D147370

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/DW_OP_div-with-signed.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/DW_OP_div-with-signed.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/DW_OP_div-with-signed.s
@@ -0,0 +1,448 @@
+  # Test handling of values represented via DW_OP_div
+
+  # RUN: %clang --target=x86_64-pc-linux -o %t %s
+  # RUN: %lldb %t -o "b f" -o "r" -o "c" -o "c" -o "expression -T -- i" \
+  # RUN: -o "exit" | FileCheck %s
+
+  # Failing case was:
+  # (uint32_t) $0 = 0
+  # CHECK: (uint32_t) $0 = 2
+	.text
+	.file	"signed_dw_op_div.c"
+	.file	1 "/usr/local/include/bits" "types.h" md5 0x96c0983c9cdaf387938a8268d00aa594
+	.file	2 "/usr/local/include/bits" "stdint-uintn.h" md5 0xbedfab747425222bb150968c14e40abd
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.loc	0 13 0  # signed_dw_op_div.c:13:0
+	.cfi_startproc
+# %bb.0:
+	movl	$3, %eax
+	#DEBUG_VALUE: f:i <- 0
+	.p2align	4, 0x90
+.LBB0_1:# =>This Inner Loop Header: Depth=1
+.Ltmp0:
+	#DEBUG_VALUE: f:i <- [DW_OP_consts 3, DW_OP_minus, DW_OP_consts 18446744073709551615, DW_OP_div, DW_OP_stack_value] $eax
+	.loc	0 8 7 prologue_end  # signed_dw_op_div.c:8:7
+	incq	g(%rip)
+.Ltmp1:
+	#DEBUG_VALUE: f:i <- [DW_OP_consts 3, DW_OP_minus, DW_OP_consts 18446744073709551615, DW_OP_div, DW_OP_consts 1, DW_OP_plus, DW_OP_stack_value] $eax
+	.loc	0 7 20  # signed_dw_op_div.c:7:20
+	decl	%eax
+.Ltmp2:
+	.loc	0 7 5 is_stmt 0 # signed_dw_op_div.c:7:5
+	jne	.LBB0_1
+.Ltmp3:
+# %bb.2:
+	.loc	0 15 5 is_stmt 1# signed_dw_op_div.c:15:5
+	xorl	%eax, %eax
+	retq
+.Ltmp4:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+	.file	3 "/usr/local/include/bits" "stdint-intn.h" md5 0x90039fb90b44dcbf118222513050fe57
+# -- End function
+	.type	g,@object   # @g
+	.local	g
+	.comm	g,8,8
+	.section	.debug_loclists,"",@progbits
+	.long	.Ldebug_list_header_end0-.Ldebug_list_header_start0 # Length
+.Ldebug_list_header_start0:
+	.short	5   # Version
+	.byte	8   # Address size
+	.byte	0   # Segment selector size
+	.long	1   # Offset entry count
+.Lloclists_table_base0:
+	.long	.Ldebug_loc0-.Lloclists_table_base0
+.Ldebug_loc0:
+	.byte	4   # DW_LLE_offset_pair
+	.uleb128 .Ltmp0-.Lfunc_begin0   #   starting offset
+	.uleb128 .Ltmp1-.Lfunc_begin0   #   ending offset
+	.byte	16  # Loc expr size
+	.byte	112 # DW_OP_breg0
+	.byte	0   # 0
+	.byte	16  # DW_OP_constu
+	.byte	255 # 4294967295
+	.byte	255 # 
+	.byte	255 # 
+	.byte	255 # 
+	.byte	15  # 
+	.byte	26  # DW_OP_and
+	.byte	17  # DW_OP_consts
+	.byte	3   # 3
+	.byte	28  # DW_OP_minus
+	.byte	17  # DW_OP_consts
+	.byte	127 # -1
+	.byte	27  # DW_OP_div
+	.byte	159 # DW_OP_stack_value
+	.byte	4   # DW_LLE_offset_pair
+	.uleb128 .Ltmp1-.Lfunc_begin0   #   starting offset
+	.uleb128 .Ltmp2-.Lfunc_begin0   #   ending offset
+	.byte	19  # Loc expr size
+	.byte	112 # DW_OP_breg0
+	.byte	0   # 0
+	.byte	16  # DW_OP_constu
+	.byte	255 # 4294967295
+	.byte	255 # 
+	.byte	255 # 
+	.byte	255 # 
+	.byte	15  # 
+	.byte	26  # DW_OP_and
+	.byte	17  # DW_OP_consts
+	.byte	3   # 3
+	.byte	28  # DW_OP_minus
+	.byte	17  #