[Lldb-commits] [PATCH] D119548: [lldb] Fix race condition between lldb-vscode and stop hooks executor

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin created this revision.
ilya-nozhkin added reviewers: jingham, clayborg.
ilya-nozhkin requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The race is between these two pieces of code that are executed in two separate
lldb-vscode threads (the first is in the main thread and another is in the
event-handling thread):

  // lldb-vscode.cpp
  g_vsc.debugger.SetAsync(false);
  g_vsc.target.Launch(launch_info, error);
  g_vsc.debugger.SetAsync(true);



  // Target.cpp
  bool old_async = debugger.GetAsyncExecution();
  debugger.SetAsyncExecution(true);
  debugger.GetCommandInterpreter().HandleCommands(GetCommands(), exc_ctx,
  options, result);
  debugger.SetAsyncExecution(old_async);

The sequence that leads to the bug is this one:

1. Main thread enables synchronous mode and launches the process.
2. When the process is launched, it generates the first stop event.
3. This stop event is catched by the event-handling thread and DoOnRemoval is 
invoked.
4. Inside DoOnRemoval, this thread runs stop hooks. And before running stop 
hooks, the current synchronization mode is stored into old_async (and right now 
it is equal to "false").
5. The main thread finishes the launch and returns to lldb-vscode, the 
synchronization mode is restored to asynchronous by lldb-vscode.
6. Event-handling thread finishes stop hooks processing and restores the 
synchronization mode according to old_async (i.e. makes the mode synchronous)
7. And now the mode is synchronous while lldb-vscode expects it to be 
asynchronous. Synchronous mode forbids the process to broadcast public stop 
events, so, VS Code just hangs because lldb-vscode doesn't notify it about 
stops.

So, this diff introduces a lock of synchronization mode for cases when it should
be saved before some action and then restored.

The bug is only present on Windows but it seems to be just a coincidence. On
Linux, as I understand, the first stop event is intercepted by hijacking
listener and is never popped out of the queue, so, stop hooks are not executed
and the race doesn't happen.

So, this diff also fixes some problems with lldb-vscode tests on Windows to make
it possible to run the related test. Other tests still can't be enabled because
the debugged program prints something into stdout and LLDB can't intercept this
output and redirect it to lldb-vscode properly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119548

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/source/Core/Debugger.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
  lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
  lldb/test/API/tools/lldb-vscode/stop-hooks/main.c

Index: lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
@@ -0,0 +1,35 @@
+"""
+Test stop hooks
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbvscode_testcase
+
+
+class TestVSCode_stop_hooks(lldbvscode_testcase.VSCodeTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfRemote
+def test_stop_hooks_before_run(self):
+'''
+Test that there is no race condition between lldb-vscode and
+stop hooks executor
+'''
+program = self.getBuildArtifact("a.out")
+preRunCommands = ['target stop-hook add -o help']
+self.build_and_launch(program, stopOnEntry=True, preRunCommands=preRunCommands)
+
+# The first stop is on entry.
+self.continue_to_next_stop()
+
+breakpoint_ids = self.set_function_breakpoints(['main'])
+# This request hangs if the race happens, because, in that case, the
+# command interpreter is in synchronous mode while lldb-vscode expects
+# it to be in asynchronous mode, so, the process doesn't send the stop
+# event to "lldb.Debugger" listener (which is monitored by lldb-vscode).
+self.continue_to_breakpoints(breakpoint_ids)
+
+self.continue_to_exit()
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Ind

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Replace interactive mode by CrashLogScriptedProcess

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a subscriber: clayborg.
mib added a comment.

@clayborg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Replace interactive mode by CrashLogScriptedProcess

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

@clayborg I heard from Jim that you might be interested by this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I was hoping you would add a positive test as well like the one I came up with 
that covered the new code as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113498

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

[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D113498#3314729 , @shafik wrote:

> I was hoping you would add a positive test as well like the one I came up 
> with that covered the new code as well.

Oh, whoops, sorry, I don’t know how I missed your point :)
I will add a test, thanks for a code sample!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113498

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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Replace interactive mode by CrashLogScriptedProcess

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I used this back when I worked at Apple to load thousands of crash logs and get 
information about the versions of a library that were all involved in the 
crash. From the Apple crash reporter site, download the zip file with all crash 
logs, load them all into, use interactive mode to say "image LLDB" and I could 
see if any of the crashes were in current versions of LLDB after we submitted a 
fix for something. I am no longer at Apple, so find to remove this feature if 
no one uses it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Replace interactive mode by CrashLogScriptedProcess

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D119501#3314991 , @clayborg wrote:

> I used this back when I worked at Apple to load thousands of crash logs and 
> get information about the versions of a library that were all involved in the 
> crash. From the Apple crash reporter site, download the zip file with all 
> crash logs, load them all into, use interactive mode to say "image LLDB" and 
> I could see if any of the crashes were in current versions of LLDB after we 
> submitted a fix for something. I am no longer at Apple, so find to remove 
> this feature if no one uses it.

We're not really removing it, we're replacing it with a (imho much better) 
implementation backed by the scripted processes. This gives a "native" way to 
interactive inspect the crashlog with `bt` and `frame select` etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119504: [lldb/crashlog] Fix exception signal parsing

[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




Comment at: lldb/examples/python/crashlog.py:471
 exception_extra = ""
-return "{} ({}){}".format(exception_type, exception_signal,
   exception_extra)

mib wrote:
> @JDevlieghere Here it's only prepended by a space, which is honored in my 
> change since I concatenate `"({})".format(json_exception['signal'])` to 
> `exception_signal` (which is initialized with a space).
> 
> I don't think this should have a trailing space because it's not guaranteed 
> that we get something in `exception_extra`. 
Thanks, I missed the `+` in `+=`, I thought it was just an assignment. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119504

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

[Lldb-commits] [PATCH] D119388: [lldb/Plugin] Add artificial stackframe loading in ScriptedThread

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D119388

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

[Lldb-commits] [PATCH] D119388: [lldb/Plugin] Add artificial stackframe loading in ScriptedThread

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:159
+
+  size_t arr_size = arr_sp->GetSize();
+  if (arr_size > std::numeric_limits::max())

`const`



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:195
+
+lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+bool cfa_is_valid = false;

`const`



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:196
+lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+bool cfa_is_valid = false;
+const bool behaves_like_zeroth_frame = false;

`const`



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:273
   GetRegisterContext()->InvalidateIfNeeded(/*force=*/false);
+  LoadArtificialStackFrames();
 }

mib wrote:
> jingham wrote:
> > LoadArtificialStackFrames returns a bool for success/failure.  It always 
> > bugs me when returns signifying errors get dropped on the floor like this.
> `Thread::RefreshStateAfterStop` doesn't return anything, and the failure case 
> is already handled in `LoadArtificialStackFrames` by logging an error message 
> in the appropriate channel.
> 
> Do you have any other suggestion on how I could improve this ?
If we are never going to check the return value, we should not have one, it is 
meaningless.


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

https://reviews.llvm.org/D119388

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

[Lldb-commits] [PATCH] D119389: [lldb/crashlog] Add CrashLogScriptedProcess and resurrect_crashlog method

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:1098
 
+class ResurrectCrashProcess:
+def __init__(self, debugger, internal_dict):

Do we need this at all if we integrate it in the existing crashlog command?



Comment at: lldb/examples/python/crashlog.py:1301-1302
 'command script add -c lldb.macosx.crashlog.Symbolicate crashlog')
+debugger.HandleCommand(
+'command script add -c lldb.macosx.crashlog.ResurrectCrashProcess 
resurrect_crashlog')
 debugger.HandleCommand(

I expect the scripted process implementation to take the place of the existing 
interactive mode (D119501) so I don't think we need a new command.



Comment at: 
lldb/examples/python/scripted_process/crashlog_scripted_process.py:12
+class CrashLogScriptedProcess(ScriptedProcess):
+# NOTE: https://at.apple.com/json-crashlog-spec
+def parse_crashlog(self):

This is an internal link :-) 


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

https://reviews.llvm.org/D119389

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

[Lldb-commits] [PATCH] D119389: [lldb/crashlog] Add CrashLogScriptedProcess and resurrect_crashlog method

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:1301-1302
 'command script add -c lldb.macosx.crashlog.Symbolicate crashlog')
+debugger.HandleCommand(
+'command script add -c lldb.macosx.crashlog.ResurrectCrashProcess 
resurrect_crashlog')
 debugger.HandleCommand(

JDevlieghere wrote:
> I expect the scripted process implementation to take the place of the 
> existing interactive mode (D119501) so I don't think we need a new command.
Let's just merge this patch with D119501


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

https://reviews.llvm.org/D119389

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

[Lldb-commits] [lldb] af96914 - [lldb] Pin the shared cache when iterating over its images

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-02-11T15:26:33-08:00
New Revision: af969141fa285157044e34fb6b27963c3278241b

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

LOG: [lldb] Pin the shared cache when iterating over its images

Use the dyld_shared_cache_(un)pin_mapping SPI to map the whole shared
cache in memory (if possible) to avoid repeated calls to mmap.

rdar://81189015

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 10137e714861d..131d1932fe142 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -2711,7 +2711,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
 if (process_shared_cache_uuid.IsValid() &&
   process_shared_cache_uuid != UUID::fromOptionalData(&cache_uuid, 16))
 return;
-
+  const bool pinned = dyld_shared_cache_pin_mapping(shared_cache);
   dyld_shared_cache_for_each_image(shared_cache, ^(dyld_image_t image) {
 uuid_t dsc_image_uuid;
 if (found_image)
@@ -2768,6 +2768,8 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
   nlist_count = nlistCount;
 });
   });
+  if (pinned)
+dyld_shared_cache_unpin_mapping(shared_cache);
 });
 if (nlist_buffer) {
   DataExtractor dsc_local_symbols_data(nlist_buffer,



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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess to replace interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 408081.
mib retitled this revision from "[lldb/crashlog] Replace interactive mode by 
CrashLogScriptedProcess" to "[lldb/crashlog] Add CrashLogScriptedProcess to 
replace interactive mode".
mib edited the summary of this revision.
mib added a comment.
Herald added a subscriber: mgorny.

Merge D119389  into this and address 
@JDevlieghere comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -303,6 +303,9 @@
 
   StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
+  // FIXME: Need to sort the dictionary otherwise the thread ids won't match the
+  // thread indices.
+
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- /dev/null
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -0,0 +1,148 @@
+import os,json,struct,signal
+
+from typing import Any, Dict
+
+import lldb
+from lldb.plugins.scripted_process import ScriptedProcess
+from lldb.plugins.scripted_process import ScriptedThread
+
+from lldb.macosx.crashlog import CrashLog,CrashLogParser
+
+class CrashLogScriptedProcess(ScriptedProcess):
+def parse_crashlog(self):
+try:
+crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
+except Exception as e:
+return
+
+self.pid = crash_log.process_id
+self.crashed_thread_idx = crash_log.crashed_thread_idx
+self.loaded_images = []
+
+for thread in crash_log.threads:
+if thread.did_crash():
+for ident in thread.idents:
+images = crash_log.find_images_with_identifier(ident)
+if images:
+for image in images:
+#TODO: Add to self.loaded_images and load images in lldb
+err = image.add_module(self.target)
+if err:
+print(err)
+else:
+self.loaded_images.append(image)
+self.threads[thread.index] = CrashLogScriptedThread(self, None, thread)
+
+def __init__(self, target: lldb.SBTarget, args : lldb.SBStructuredData):
+super().__init__(target, args)
+
+if not self.target or not self.target.IsValid():
+return
+
+self.crashlog_path = None
+
+crashlog_path = args.GetValueForKey("crashlog_path")
+if crashlog_path and crashlog_path.IsValid():
+if crashlog_path.GetType() == lldb.eStructuredDataTypeString:
+self.crashlog_path = crashlog_path.GetStringValue(100)
+
+if not self.crashlog_path:
+return
+
+self.pid = super().get_process_id()
+self.crashed_thread_idx = 0
+self.parse_crashlog()
+
+def get_memory_region_containing_address(self, addr: int) -> lldb.SBMemoryRegionInfo:
+return None
+
+def get_thread_with_id(self, tid: int):
+return {}
+
+def get_registers_for_thread(self, tid: int):
+return {}
+
+def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+# NOTE: CrashLogs don't contain any memory.
+return lldb.SBData()
+
+def get_loaded_images(self):
+# TODO: Iterate over corefile_target modules and build a data structure
+# from it.
+return self.loaded_images
+
+def get_process_id(self) -> int:
+return self.pid
+
+def should_stop(self) -> bool:
+return True
+
+def is_alive(self) -> bool:
+return True
+
+def get_scripted_thread_plugin(self):
+return CrashLogScriptedThread.__module__ + "." + CrashLogScriptedThread.__name__
+
+class CrashLogScriptedThread(ScriptedThread):
+def create_register_ctx(self):
+if not self.has_crashed:
+return dict.fromkeys([*map(lambda reg: reg['name'], self.register_info['registers'])] , 0)
+
+if not self.backing_thread or not len(self.backing_thread.registers):
+return dict.fromkeys([*map(lambda reg: reg['name'], self.register_info['registers'])] , 0)
+
+for reg in self.register_info['registers']:
+reg_name = reg['name']
+if r

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess to replace interactive mode

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Test?




Comment at: lldb/examples/python/crashlog.py:990
+res = lldb.SBCommandReturnObject()
+ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', res)
+if not res.Succeeded():

Do we need to have LLDB import it? Can we import it here straight from python?



Comment at: lldb/examples/python/crashlog.py:1148-1157
+if options.interactive:
+load_crashlog_in_scripted_process(debugger, crash_log_file)
+elif options.batch:
+crash_log = CrashLogParser().parse(debugger, crash_log_file, 
options.verbose)
+SymbolicateCrashLog(crash_log, options)
+elif ci and ci.IsInteractive():
+load_crashlog_in_scripted_process(debugger, crash_log_file)

I'd extract a helper to compute if we need the interactive or batch crashlog to 
avoid the duplication across the clauses. 



Comment at: lldb/examples/python/crashlog.py:1153-1154
+SymbolicateCrashLog(crash_log, options)
+elif ci and ci.IsInteractive():
+load_crashlog_in_scripted_process(debugger, crash_log_file)
+else:

Let's hold off on making the interactive one the default for now. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119602: Add additional method for ObjectFileMachO to find the first loadable segment in a Mach-O binary

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: JDevlieghere.
jasonmolenda added a project: LLDB.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

When lldb is processing the first couple of binaries in a process launch, it 
needs to compute load addresses before the files' sections have been added to 
the TargetSectionList, so our normal mechanism is not sufficient.  
ObjectFileMachO::ParseSymtab() calls GetMachHeaderSection() to find that 
segment, then uses the difference between that segment and the LINKEDIT 
segment, plus the Mach-O header load address that we were given, to calculate 
the load address of LINKEDIT.

This scheme works until one of the binaries we're treating in this special way 
is in the shared cache.  GetMachHeaderSection() explicitly looks for a segment 
at file offset 0, as a way of implying the __TEXT segment without hardcoding 
its name.  In a file in the shared cache, it will have a non-zero file address. 
 It's laudable that ParseSymtab already handles the offset calculation 
correctly for a non-zero file offset -- but it won't match a segment which is 
nonzero.

This patch adds a backup mechanism if we find no segment at file offset 0, to 
find the __TEXT segment and return that.

Outside of an environment where one of these very initial binaries are in the 
shared cache, this can't be reproduced.  Individual Mach-O binaries have file 
__TEXT with a file offset of 0.  I haven't thought of a way to construct a well 
formed Mach-O binary that behaves like this, outside of the shared cache binary 
blob.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119602

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6100,6 +6100,15 @@
 if (section->GetFileOffset() == 0 && SectionIsLoadable(section))
   return section;
   }
+
+  // We may have a binary with in the shared cache that has a non-zero
+  // file address for its first segment, traditionally the __TEXT segment.
+  // Search for it by name and return it as our next best guess.
+  SectionSP text_segment_sp =
+  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   return nullptr;
 }
 


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6100,6 +6100,15 @@
 if (section->GetFileOffset() == 0 && SectionIsLoadable(section))
   return section;
   }
+
+  // We may have a binary with in the shared cache that has a non-zero
+  // file address for its first segment, traditionally the __TEXT segment.
+  // Search for it by name and return it as our next best guess.
+  SectionSP text_segment_sp =
+  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   return nullptr;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D119602: Add additional method for ObjectFileMachO to find the first loadable segment in a Mach-O binary

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 408091.
jasonmolenda added a comment.

Fix typeo in comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119602

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6100,6 +6100,15 @@
 if (section->GetFileOffset() == 0 && SectionIsLoadable(section))
   return section;
   }
+
+  // We may have a binary in the shared cache that has a non-zero
+  // file address for its first segment, traditionally the __TEXT segment.
+  // Search for it by name and return it as our next best guess.
+  SectionSP text_segment_sp =
+  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   return nullptr;
 }
 


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6100,6 +6100,15 @@
 if (section->GetFileOffset() == 0 && SectionIsLoadable(section))
   return section;
   }
+
+  // We may have a binary in the shared cache that has a non-zero
+  // file address for its first segment, traditionally the __TEXT segment.
+  // Search for it by name and return it as our next best guess.
+  SectionSP text_segment_sp =
+  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   return nullptr;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess to replace interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked an inline comment as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:990
+res = lldb.SBCommandReturnObject()
+ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', res)
+if not res.Succeeded():

JDevlieghere wrote:
> Do we need to have LLDB import it? Can we import it here straight from python?
`crashlog.py` can be imported outside of the debugger, so in my understanding, 
if we're importing it here directly, lldb's script interpreter might not find 
the module and class used to create the ScriptedProcess object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess & remove interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 408107.
mib marked an inline comment as done.
mib retitled this revision from "[lldb/crashlog] Add CrashLogScriptedProcess to 
replace interactive mode" to "[lldb/crashlog] Add CrashLogScriptedProcess & 
remove interactive mode".
mib added a comment.

Address @JDevlieghere comments. Still working the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -303,6 +303,9 @@
 
   StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
+  // FIXME: Need to sort the dictionary otherwise the thread ids won't match the
+  // thread indices.
+
   if (!thread_info_sp)
 return ScriptedInterface::ErrorWithMessage(
 LLVM_PRETTY_FUNCTION,
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- /dev/null
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -0,0 +1,148 @@
+import os,json,struct,signal
+
+from typing import Any, Dict
+
+import lldb
+from lldb.plugins.scripted_process import ScriptedProcess
+from lldb.plugins.scripted_process import ScriptedThread
+
+from lldb.macosx.crashlog import CrashLog,CrashLogParser
+
+class CrashLogScriptedProcess(ScriptedProcess):
+def parse_crashlog(self):
+try:
+crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
+except Exception as e:
+return
+
+self.pid = crash_log.process_id
+self.crashed_thread_idx = crash_log.crashed_thread_idx
+self.loaded_images = []
+
+for thread in crash_log.threads:
+if thread.did_crash():
+for ident in thread.idents:
+images = crash_log.find_images_with_identifier(ident)
+if images:
+for image in images:
+#TODO: Add to self.loaded_images and load images in lldb
+err = image.add_module(self.target)
+if err:
+print(err)
+else:
+self.loaded_images.append(image)
+self.threads[thread.index] = CrashLogScriptedThread(self, None, thread)
+
+def __init__(self, target: lldb.SBTarget, args : lldb.SBStructuredData):
+super().__init__(target, args)
+
+if not self.target or not self.target.IsValid():
+return
+
+self.crashlog_path = None
+
+crashlog_path = args.GetValueForKey("crashlog_path")
+if crashlog_path and crashlog_path.IsValid():
+if crashlog_path.GetType() == lldb.eStructuredDataTypeString:
+self.crashlog_path = crashlog_path.GetStringValue(100)
+
+if not self.crashlog_path:
+return
+
+self.pid = super().get_process_id()
+self.crashed_thread_idx = 0
+self.parse_crashlog()
+
+def get_memory_region_containing_address(self, addr: int) -> lldb.SBMemoryRegionInfo:
+return None
+
+def get_thread_with_id(self, tid: int):
+return {}
+
+def get_registers_for_thread(self, tid: int):
+return {}
+
+def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+# NOTE: CrashLogs don't contain any memory.
+return lldb.SBData()
+
+def get_loaded_images(self):
+# TODO: Iterate over corefile_target modules and build a data structure
+# from it.
+return self.loaded_images
+
+def get_process_id(self) -> int:
+return self.pid
+
+def should_stop(self) -> bool:
+return True
+
+def is_alive(self) -> bool:
+return True
+
+def get_scripted_thread_plugin(self):
+return CrashLogScriptedThread.__module__ + "." + CrashLogScriptedThread.__name__
+
+class CrashLogScriptedThread(ScriptedThread):
+def create_register_ctx(self):
+if not self.has_crashed:
+return dict.fromkeys([*map(lambda reg: reg['name'], self.register_info['registers'])] , 0)
+
+if not self.backing_thread or not len(self.backing_thread.registers):
+return dict.fromkeys([*map(lambda reg: reg['name'], self.register_info['registers'])] , 0)
+
+for reg in self.register_info['registers']:
+reg_name = reg['name']
+if reg_name in self.backing_thread.registers:
+self.register_ctx

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess & remove interactive mode

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:990
+res = lldb.SBCommandReturnObject()
+ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', res)
+if not res.Succeeded():

mib wrote:
> JDevlieghere wrote:
> > Do we need to have LLDB import it? Can we import it here straight from 
> > python?
> `crashlog.py` can be imported outside of the debugger, so in my 
> understanding, if we're importing it here directly, lldb's script interpreter 
> might not find the module and class used to create the ScriptedProcess object.
But that's not relevant for the (new) interactive mode? If you run crashlog on 
the command line it should behave like it does today. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119501: [lldb/crashlog] Add CrashLogScriptedProcess & remove interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:990
+res = lldb.SBCommandReturnObject()
+ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', res)
+if not res.Succeeded():

JDevlieghere wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > Do we need to have LLDB import it? Can we import it here straight from 
> > > python?
> > `crashlog.py` can be imported outside of the debugger, so in my 
> > understanding, if we're importing it here directly, lldb's script 
> > interpreter might not find the module and class used to create the 
> > ScriptedProcess object.
> But that's not relevant for the (new) interactive mode? If you run crashlog 
> on the command line it should behave like it does today. 
It's actually only relevant for the new interactive mode, since this will 
import the crashlog scripted process blueprint. Otherwise, launching the 
process won't work (see line 999) because lldb won't find the managing class 
module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119501

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

[Lldb-commits] [PATCH] D119616: [lldb] Replace asserts on .Success() with assertSuccess()

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added a reviewer: jingham.
Herald added a subscriber: wenlei.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Replace forms of `assertTrue(x.Success())` with `assertSuccess(x)`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119616

Files:
  lldb/test/API/commands/expression/context-object-objc/TestContextObjectObjc.py
  lldb/test/API/commands/platform/basic/TestPlatformPython.py
  lldb/test/API/commands/watchpoints/multiple_hits/TestMultipleHits.py
  
lldb/test/API/commands/watchpoints/step_over_watchpoint/TestStepOverWatchpoint.py
  
lldb/test/API/functionalities/breakpoint/auto_continue/TestBreakpointAutoContinue.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommandsFromPython.py
  lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py
  
lldb/test/API/functionalities/breakpoint/serialize/TestBreakpointSerialization.py
  
lldb/test/API/functionalities/dlopen_other_executable/TestDlopenOtherExecutable.py
  lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  
lldb/test/API/functionalities/gdb_remote_client/TestJLink6Armv7RegisterDefinition.py
  lldb/test/API/functionalities/gdb_remote_client/TestMemoryRegionDirtyPages.py
  lldb/test/API/functionalities/launch_stop_at_entry/TestStopAtEntry.py
  lldb/test/API/functionalities/load_using_paths/TestLoadUsingPaths.py
  lldb/test/API/functionalities/paths/TestPaths.py
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py
  lldb/test/API/functionalities/process_crash_info/TestProcessCrashInfo.py
  lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
  
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
  lldb/test/API/functionalities/return-value/TestReturnValue.py
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
  lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
  lldb/test/API/functionalities/step_scripted/TestStepScripted.py
  
lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py
  
lldb/test/API/functionalities/thread/state_after_expression/TestStateAfterExpression.py
  lldb/test/API/functionalities/var_path/TestVarPath.py
  lldb/test/API/lang/c/local_types/TestUseClosestType.py
  lldb/test/API/lang/cpp/incomplete-types/TestCppIncompleteTypes.py
  lldb/test/API/lang/cpp/trivial_abi/TestTrivialABI.py
  lldb/test/API/lang/cpp/type_lookup/TestCppTypeLookup.py
  lldb/test/API/lang/objc/blocks/TestObjCIvarsInBlocks.py
  lldb/test/API/lang/objc/global_ptrs/TestGlobalObjects.py
  lldb/test/API/lang/objc/objc-checker/TestObjCCheckers.py
  lldb/test/API/lang/objc/objc-ivar-offsets/TestObjCIvarOffsets.py
  lldb/test/API/lang/objc/objc-ivar-stripped/TestObjCIvarStripped.py
  lldb/test/API/lang/objc/objc-property/TestObjCProperty.py
  lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
  lldb/test/API/macosx/function-starts/TestFunctionStarts.py
  lldb/test/API/macosx/profile_vrs_detach/TestDetachVrsProfile.py
  lldb/test/API/macosx/thread_suspend/TestInternalThreadSuspension.py
  lldb/test/API/macosx/universal/TestUniversal.py
  lldb/test/API/python_api/debugger/TestDebuggerAPI.py
  lldb/test/API/python_api/file_handle/TestFileHandle.py
  lldb/test/API/python_api/process/read-mem-cstring/TestReadMemCString.py
  lldb/test/API/python_api/sbdata/TestSBData.py
  lldb/test/API/python_api/sbmodule/TestSBModule.py
  lldb/test/API/python_api/sbplatform/TestSBPlatform.py
  lldb/test/API/python_api/sbstructureddata/TestStructuredDataAPI.py
  lldb/test/API/python_api/target/TestTargetAPI.py
  lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
  lldb/test/API/sample_test/TestSampleTest.py
  lldb/test/API/sample_test/main.c

Index: lldb/test/API/sample_test/main.c
===
--- lldb/test/API/sample_test/main.c
+++ lldb/test/API/sample_test/main.c
@@ -9,7 +9,7 @@
   printf ("Set a breakpoint here: %d.\n", test_var);
   //% test_var = self.frame().FindVariable("test_var")
   //% test_value = test_var.GetValueAsUnsigned()
-  //% self.assertTrue(test_var.GetError().Success(), "Failed to fetch test_var")
+  //% self.assertSuccess(test_var.GetError(), "Failed to fetch test_var")
   //% self.assertEqual(test_value, 10, "Failed to get the right value for test_var")
   return global_test_var;
 }
Index: lldb/test/API/sample_test/TestSampleTest.py
===
--- lldb/test/API/sample_test/TestSampleTest.py
+++ lldb/test/API/sample_test/TestSampleTest.py
@@ -42,7 +42,7 @@
 
   

[Lldb-commits] [PATCH] D119616: [lldb] Replace asserts on .Success() with assertSuccess()

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Can you please provide a motivation in the summary, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119616

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