[Lldb-commits] [PATCH] D112325: [lldb] Pass the target triple to the compiler when determining the DWARF version

vsk added a comment. Thanks! Lgtm as well, but with the same nits as Raphael. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112325/new/ https://reviews.llvm.org/D112325 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D112034: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12

This revision was automatically updated to reflect the committed changes. Closed by commit rG5e004b03f72a: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12 (authored by vsk). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D112034: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12

vsk created this revision. vsk added reviewers: JDevlieghere, kastiglione, kledzik. vsk requested review of this revision. In macOS 12, dyld switched to using chained fixups. As a result, all symbols are bound at launch and there are no lazy pointers any more. Since we wish to import/dlopen() a

[Lldb-commits] [PATCH] D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present

vsk added a comment. In D110013#3013733 , @aprantl wrote: > Without having dug into this deeper, do you think not specifying the arch > could cause any problems with universal binaries? I guess not, because we > still have the UUID, and the UUID is per

[Lldb-commits] [PATCH] D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present

This revision was automatically updated to reflect the committed changes. Closed by commit rGe31b2d7d7be9: [lldb][crashlog] Avoid specifying arch for image when a UUID is present (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present

vsk created this revision. vsk added reviewers: jasonmolenda, jingham, JDevlieghere. Herald added subscribers: pengfei, kristof.beyls. vsk requested review of this revision. Herald added a project: LLDB. When adding an image to a target for crashlog purposes, avoid specifying the architecture of

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7eb67748f9d7: [MachCore] Report arm64 thread exception state (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk updated this revision to Diff 373016. vsk added a comment. - Add missing skipIf(arch=no_match(['arm64', 'arm64e'])) change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109795/new/ https://reviews.llvm.org/D109795 Files:

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk updated this revision to Diff 373014. vsk added a comment. - Limit testing to arm64/arm64e only. - Set a stop reason for crashing threads only (and not for threads waiting on a syscall to finish). - Use @jasonmolenda's multi-threaded test case; make sure we only select the crashing thread.

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk added inline comments. Comment at: lldb/test/API/macosx/corefile-exception-reason/TestCorefileExceptionReason.py:19 +@skipUnlessDarwin +@skipIf(archs=['i386','x86_64']) # exception codes not yet supported for Intel macs +def test(self):

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk updated this revision to Diff 372836. vsk added a comment. - Add a test (thanks @jasonmolenda!). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109795/new/ https://reviews.llvm.org/D109795 Files:

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk added a comment. FTR, I was able to write a test using `process save-core`, but even with `-s dirty` this is generating gigabytes of data and would presumably be super-flaky on the bots. I also tried `-s stack`, but for the test program I passed in (just some 2-line .c file that

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk added inline comments. Comment at: lldb/include/lldb/Target/AppleArm64ExceptionClass.h:14 + +enum class AppleArm64ExceptionClass : unsigned { +#define APPLE_ARM64_EXCEPTION_CLASS(Name, Code) Name = Code, shafik wrote: > We should use fixed sized integer

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk updated this revision to Diff 372809. vsk marked 3 inline comments as done. vsk added a comment. Address review feedback: - Include far register in output, which now looks like e.g.: (lldb) bt all * thread #1, stop reason = ESR_EC_DABORT_EL0 (fault address: 0x6261747563657860) *

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk updated this revision to Diff 372591. vsk added a comment. - Add license header to the .def file. - (I'm not sure whether/how this change can be tested, any pointers appreciated.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109795/new/

[Lldb-commits] [PATCH] D109795: [MachCore] Report arm64 thread exception state

vsk created this revision. vsk added reviewers: jasonmolenda, jingham, JDevlieghere. Herald added subscribers: zzheng, omjavaid, pengfei, kristof.beyls. vsk requested review of this revision. Herald added a project: LLDB. A MachO userspace corefile may contain LC_THREAD commands which specify

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG66902a32c838: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D105741: [trace] Introduce Hierarchical Trace Representation (HTR) and add `thread trace export ctf` command for Intel PT trace visualization

vsk added a comment. Hey @jj10306, thanks for working on this. @wallace @clayborg -- It seems like there are a ton of logic changes introduced in this patch that are tested at too coarse a level. I'm not confident in the changes being made here. For example, there's a bunch of subtle work

[Lldb-commits] [PATCH] D106584: [lldb] Improve checking of file cache read eligibility for mach-O

vsk added a comment. Hey Augusto, thanks for tackling this, I'm just now slowly paging things in. Is this a correct statement of the problem: LLDB is failing to disable its file cache optimization when reading writable segments (say, __DATA) from a MachO sourced from the shared cache? If

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk updated this revision to Diff 360180. vsk added a comment. Drop unneeded braces in switch cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102428/new/ https://reviews.llvm.org/D102428 Files: lldb/include/lldb/Core/Address.h

[Lldb-commits] [PATCH] D105030: [lldb/Interpreter] Add setting to set session transcript save directory

vsk added a comment. I love the save-session feature. Just checking, has it already been release-noted on llvm.org (and for that matter Xcode)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105030/new/ https://reviews.llvm.org/D105030

[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

vsk added a comment. FTR, I'm quite happy with the new direction. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104422/new/ https://reviews.llvm.org/D104422 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D103588: [trace] Create a top level ThreadTrace class

vsk added a comment. I think this is moving in the right direction! However, the way ThreadTrace is specified still raises the same scalability concerns (see inline). The "instruction index" concept doesn't seem sufficient. Decoding needs to be done in two stages: a separate "trace index"

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

vsk added a comment. In D103588#2806894 , @wallace wrote: > I've been thinking about what you said and I'm having second thoughts on my > implementation. I'll share more context: > > - I want to work in the short term on reverse debugging and

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

vsk added a comment. In D103588#2806611 , @wallace wrote: >> This approach - of prototyping trace analyses on a vector >> representation and then rewriting them later when scaling issues arise - >> doesn't sound good to me. Even for something simple,

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

vsk added a comment. In D103588#2806512 , @wallace wrote: > Right now we haven't implemented lazy decoding; we are decoding the entire > trace. But that's just because so far we have been working with small traces. > As we progress in this work, we'll

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

vsk requested changes to this revision. vsk added a comment. This revision now requires changes to proceed. I'm quite concerned about the design decision to represent a trace as a vector of TraceInstruction. This bakes considerations that appear to be specific to Facebook's usage of IntelPT way

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

vsk requested changes to this revision. vsk added a subscriber: mib. vsk added a comment. This revision now requires changes to proceed. Thanks, excited to see this work progress! Comment at: lldb/include/lldb/Target/Target.h:1129 /// The trace object. It might be

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk added a comment. Friendly ping. Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:126 +Address brk_address; +if (!target.ResolveLoadAddress(fixed_bad_address, brk_address)) + return false; DavidSpickett wrote: > vsk

[Lldb-commits] [PATCH] D103020: [lldb] Add missing mutex guards to TargetList::CreateTarget

vsk accepted this revision as: vsk. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103020/new/ https://reviews.llvm.org/D103020 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk added inline comments. Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:126 +Address brk_address; +if (!target.ResolveLoadAddress(fixed_bad_address, brk_address)) + return false; DavidSpickett wrote: > vsk wrote: > >

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk updated this revision to Diff 345940. vsk added a comment. Address review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102428/new/ https://reviews.llvm.org/D102428 Files: lldb/include/lldb/Core/Address.h

[Lldb-commits] [PATCH] D102624: [lldb] Optimize expressions

vsk added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:409 + // static MyClass *__lldb_expr_result_ptr = // Points to stack. + // __lldb_expr_result_ptr(__lldb_expr_result_ptr); + // } // End of expression

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk added a comment. Thanks for the review! Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/BLRAA_error/blraa.c:19 + +// Before: +#if 0 DavidSpickett wrote: > What is the purpose of the `// Before:` blocks here? Simply to

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk updated this revision to Diff 345513. vsk added a comment. Address code review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102428/new/ https://reviews.llvm.org/D102428 Files: lldb/include/lldb/Core/Address.h

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

vsk created this revision. vsk added reviewers: JDevlieghere, jingham, jasonmolenda. Herald added subscribers: omjavaid, kristof.beyls. vsk requested review of this revision. Herald added a project: LLDB. Upstream lldb support for summarizing BLRAx and LDRAx auth failures. rdar://41615322

[Lldb-commits] [PATCH] D100212: [lldb] Delete dead StackFrameList::Merge

vsk accepted this revision as: vsk. vsk added a comment. This revision is now accepted and ready to land. +1 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100212/new/ https://reviews.llvm.org/D100212 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D99746: [lldb/test] Respect --apple-sdk path when querying SDK info

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7d15fb577945: [lldb/test] Respect --apple-sdk path when querying SDK info (authored by vsk). Changed prior to commit:

[Lldb-commits] [PATCH] D99746: [lldb/test] Respect --apple-sdk path when querying SDK info

vsk added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/lldbutil.py:69 +# use it. +if configuration.apple_sdk is not None: +return configuration.apple_sdk JDevlieghere wrote: > This will also exclude the empty string. Thanks,

[Lldb-commits] [PATCH] D99746: [lldb/test] Respect --apple-sdk path when querying SDK info

vsk created this revision. vsk added a reviewer: JDevlieghere. vsk requested review of this revision. Herald added a project: LLDB. Respect --apple-sdk if it's specified. If the SDK simply mounted from some disk image, and not actually installed, this is the only way to use it. Repository:

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk added a comment. Sorry about that. The file was removed in 8248dd91d7f042893d4a605e98d19cb1b89a44d4 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk updated this revision to Diff 332024. vsk added a comment. Add asserts checking that the library paths exist, and restrict the test to Darwin. See https://bugs.llvm.org/show_bug.cgi?id=49656 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcb8c1ee269da: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk updated this revision to Diff 331999. vsk added a comment. - Delete an unused '#undef DLOPEN_OPTIONS' Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879 Files:

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk updated this revision to Diff 331998. vsk added a comment. - Define RTLD_LAZY in the expression as suggested Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879 Files:

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk updated this revision to Diff 331695. vsk added a comment. - Make the test generic (not Darwin-specific) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879 Files:

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk updated this revision to Diff 331694. vsk added a comment. - Add a test. It's Darwin-specific as I couldn't work out how to pass the .dylib path to the linker in a platform-agnostic way. - Trim the comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk added a comment. The best way I can think of to test this is to: - Make a library, T1, out of: % cat t1.c extern void use(); void f1() {} void f2() { use(); } - Make a library, T2, out of: % cat t2.c void use() {} - Link T1.dylib against T2.dylib, so that the reference to

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk planned changes to this revision. vsk added a comment. Herald added a subscriber: JDevlieghere. This needs a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

vsk created this revision. vsk added a reviewer: jingham. Herald added a subscriber: emaste. vsk requested review of this revision. Herald added a project: LLDB. In general, it seems like the debugger should allow programs to load & run with libraries as far as possible, instead of defaulting to

[Lldb-commits] [PATCH] D98272: [lldb/Platform] Skip very slow xcrun queries for simulator platforms, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGac29c35207a5: [lldb/Platform] Skip very slow xcrun queries for simulator platforms, NFC (authored by vsk). Changed prior to commit:

[Lldb-commits] [PATCH] D98272: [lldb/Platform] Skip very slow xcrun queries for simulator platforms, NFC

vsk updated this revision to Diff 329733. vsk added a comment. Use a helper function, respect the force argument. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98272/new/ https://reviews.llvm.org/D98272 Files:

[Lldb-commits] [PATCH] D98272: [lldb/Platform] Skip very slow xcrun queries for simulator platforms, NFC

vsk added inline comments. Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp:534 +if (arch && arch->IsValid() && !arch->TripleEnvironmentWasSpecified()) + return nullptr; // Avoid very slow xcrun query for non-simulator archs.

[Lldb-commits] [PATCH] D98272: [lldb/Platform] Skip very slow xcrun queries for simulator platforms, NFC

vsk created this revision. vsk added reviewers: aprantl, JDevlieghere. vsk requested review of this revision. Herald added a project: LLDB. GetXcodeSDK() consistently takes over 1 second to complete if the queried SDK is missing, because `xcrun` doesn't cache negative lookups. Because there are

[Lldb-commits] [PATCH] D97701: [lldb] Remove XPCServices symlinking

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97701/new/ https://reviews.llvm.org/D97701 ___

[Lldb-commits] [PATCH] D85770: Build a flat LLDB.framework for embedded Darwin targets

vsk added inline comments. Comment at: lldb/cmake/modules/LLDBFramework.cmake:142-147 + add_custom_command(TARGET liblldb POST_BUILD +COMMAND ${CMAKE_COMMAND} -E create_symlink +Versions/Current/XPCServices +

[Lldb-commits] [PATCH] D96549: Support dereferencing a DWARF scalar stack value

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:347 + ExecutionContext exe_ctx(process_sp); + EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4,

[Lldb-commits] [PATCH] D96549: Support dereferencing a DWARF scalar stack value

vsk added a comment. The logic change seems reasonable to me. Any chance we can provide a mock memory in the DwarfExpression unit test, or check in assembly for a program that uses such an entry value expression? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96549/new/

[Lldb-commits] [PATCH] D95710: [lldb/Commands] Add command options for ScriptedProcess to ProcessLaunch

vsk added inline comments. Comment at: lldb/include/lldb/Host/ProcessLaunchInfo.h:188 + bool m_scripted_process; + llvm::StringRef m_scripted_process_class_name; + StructuredData::DictionarySP m_scripted_process_dictionary_sp; Might be best to use a

[Lldb-commits] [PATCH] D70277: [Signal] Allow llvm clients to opt into one-shot SIGPIPE handling

vsk added a comment. The issue is that InitLLVM initializes its private PrettyStackPrinter before installing the one-shot default SIGPIPE handler: the PrettyStackPrinter itself eventually calls RegisterHandlers(), which runs before the SIGPIPE handler is installed and therefore doesn't

[Lldb-commits] [PATCH] D70277: [Signal] Allow llvm clients to opt into one-shot SIGPIPE handling

vsk added a comment. Unfortunately I can’t get to this right now, but will take a look first thing tomorrow morning. A shot in the dark: looks like your workflow causes a clang process to be sent SIGPIPE more than once. The second time, it exits with ioerr. I expected this to be the pre-patch

[Lldb-commits] [PATCH] D91220: [ThreadPlan] Add a test for `thread step-in -r`, NFC

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGae3640e386cc: [ThreadPlan] Add a test for `thread step-in -r`,

[Lldb-commits] [PATCH] D91220: [ThreadPlan] Add a test for `thread step-in -r`, NFC

vsk created this revision. vsk added a reviewer: jingham. Herald added a project: LLDB. vsk requested review of this revision. Herald added a subscriber: JDevlieghere. Adds test coverage for ThreadPlanStepInRange::SetAvoidRegexp. See:

[Lldb-commits] [PATCH] D90895: [TargetList] Delete the destructor

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG65d15fefe339: [TargetList] Delete the destructor (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D90895: [TargetList] Delete the destructor

vsk created this revision. vsk added reviewers: teemperor, JDevlieghere, jingham. Herald added a project: LLDB. vsk requested review of this revision. AFAICT, ~TargetList simply implements the default destructor, plus some locking. The history is murky, so I'm not sure why we do this locking.

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG16e5a347e70b: [TargetList] Simplify dummy target creation (authored by vsk). Changed prior to commit:

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk added inline comments. Comment at: lldb/source/Core/Debugger.cpp:685 - m_dummy_target_sp = m_target_list.GetDummyTarget(*this); + { +ArchSpec arch(Target::GetDefaultArchitecture()); teemperor wrote: > Maybe have a small comment here that summarizes

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk added inline comments. Comment at: lldb/source/Target/TargetList.cpp:293 // FIXME: Maybe the dummy target should be per-Debugger - if (!m_dummy_target_sp || !m_dummy_target_sp->IsValid()) { + if (!m_dummy_target_sp) { ArchSpec

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk updated this revision to Diff 303252. vsk marked an inline comment as done. vsk added a comment. Make the dummy target per-debugger. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90872/new/ https://reviews.llvm.org/D90872 Files:

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk marked an inline comment as done. vsk added inline comments. Comment at: lldb/source/Target/TargetList.cpp:293 // FIXME: Maybe the dummy target should be per-Debugger - if (!m_dummy_target_sp || !m_dummy_target_sp->IsValid()) { + if (!m_dummy_target_sp) { ArchSpec

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk updated this revision to Diff 303223. vsk added a comment. Delete some dead code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90872/new/ https://reviews.llvm.org/D90872 Files: lldb/include/lldb/Target/TargetList.h

[Lldb-commits] [PATCH] D90872: [TargetList] Simplify dummy target creation

vsk created this revision. vsk added reviewers: teemperor, JDevlieghere, jingham. Herald added a project: LLDB. vsk requested review of this revision. Factor out dummy target creation from CreateTargetInternal. This makes it impossible for dummy target creation to accidentally fail due to

[Lldb-commits] [PATCH] D90706: [crashlog] Pass the debugger around instead of relying on lldb.debugger

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90706/new/ https://reviews.llvm.org/D90706 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D90706: [crashlog] Pass the debugger around instead of relying on lldb.debugger

vsk added inline comments. Comment at: lldb/examples/python/crashlog.py:853 +# can rely on lldb.debugger being set. +SymbolicateCrashLogs(lldb.debugger, shlex.split(command)) except Exception as e: Why does crashlog.Symbolicate accept a

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:26 +public: + IntelPTError() = default; + Do we need a default constructor for this pure-virtual class? Comment at:

[Lldb-commits] [PATCH] D90276: [lldb/utils] Add the lldb-env tool

vsk abandoned this revision. vsk added a comment. Jonas walked me through some of the issues here offline: - Looks like this makes testing depend on having a configured lldb-env tool, this will make it hard to run testing against an lldb root. - If we need to launch ./bin/lldb in some other

[Lldb-commits] [PATCH] D90276: [lldb/utils] Add the lldb-env tool

vsk added a comment. >> Re: making the lit.site.cfg self-contained, IIUC the situation is that there >> are a couple places where we shell out via `subprocess`, but that you'd like >> to get rid of these? I wonder whether we could bundle up the necessary >> scripts along with lit.site.cfg

[Lldb-commits] [PATCH] D90276: [lldb/utils] Add the lldb-env tool

vsk added a comment. In D90276#2357978 , @JDevlieghere wrote: > Can you explain the added value of the tool a bit more. I can see how it's > nice to be able to run `export $(lldb-env); path/to/lldb` but I wonder how > often you'd end up doing that.

[Lldb-commits] [PATCH] D90276: [lldb/utils] Add the lldb-env tool

vsk created this revision. vsk added reviewers: JDevlieghere, teemperor, mgorny. Herald added a project: LLDB. vsk requested review of this revision. Introduce lldb-env, a tool that prints out the environment variables needed to launch lldb. Usually, no environment variables need to be set to

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. @wallace thanks for winnowing the test objects. I left an inline suggestion about simplifying the error-handling in IntelPTInstruction. Other than that, mechanically this is looking good. Thanks! Comment at:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. In D89283#2336120 , @wallace wrote: > @vsk, I agree with you regarding the files. At the moment our implementation > of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. > Then, we'll be able to generate

[Lldb-commits] [PATCH] D89408: [trace] rename ThreadIntelPT to ThreadTrace

vsk added inline comments. Comment at: lldb/include/lldb/Target/ThreadTrace.h:34 + /// this thread was being executed. + ThreadTrace(Process , lldb::tid_t tid, const FileSpec trace_file) + : Thread(process, tid), m_trace_file(trace_file) {} wallace

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. I'm not sure if the current revision of the patch reflects the long-term testing strategy. But if so, I'm quite concerned about the proliferation of large binary files in the repo (like ld.so, or the raw trace binary itself). These are opaque blobs that are hard to

[Lldb-commits] [PATCH] D89408: [trace] rename ThreadIntelPT to ThreadTrace

vsk added inline comments. Comment at: lldb/include/lldb/Target/ThreadTrace.h:34 + /// this thread was being executed. + ThreadTrace(Process , lldb::tid_t tid, const FileSpec trace_file) + : Thread(process, tid), m_trace_file(trace_file) {} `const

[Lldb-commits] [PATCH] D89215: [lldb] Add a page to the documentation with (external) links on how to use LLDB

vsk added a comment. + 1, this looks good to me. I think there might be room to include a short (~1 sentence) summary next to each extension entry, so readers know what to expect before clicking on one. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89215/new/

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

vsk added a comment. It seems like calling any 'mmap' definition should work. Is the interposed mmap implementation failing and correctly returning -1, or is it succeeding and incorrectly returning -1? In either case, it seems like it's worth digging into why it's failing / returning the wrong

[Lldb-commits] [PATCH] D85770: Build a flat LLDB.framework for embedded Darwin targets

This revision was automatically updated to reflect the committed changes. Closed by commit rGd49aedd315e3: Build a flat LLDB.framework for embedded Darwin targets (authored by vsk). Changed prior to commit: https://reviews.llvm.org/D85770?vs=284849=285174#toc Repository: rG LLVM Github

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

vsk added a comment. In D85705#2211607 , @clayborg wrote: > In D85705#2211073 , @vsk wrote: > >> This looks very cool, thanks @clayborg! I think using JSON to describe the >> trace data (what kind of trace is

[Lldb-commits] [PATCH] D85770: Build a flat LLDB.framework for embedded Darwin targets

vsk added inline comments. Comment at: lldb/cmake/modules/LLDBFramework.cmake:6-37 +if(NOT APPLE_EMBEDDED) + set_target_properties(liblldb PROPERTIES +FRAMEWORK ON +FRAMEWORK_VERSION ${LLDB_FRAMEWORK_VERSION} - OUTPUT_NAME LLDB - VERSION ${LLDB_VERSION} -

[Lldb-commits] [PATCH] D85770: Build a flat LLDB.framework for embedded Darwin targets

vsk created this revision. vsk added reviewers: JDevlieghere, davide, friss. Herald added a subscriber: mgorny. Herald added a project: LLDB. vsk requested review of this revision. This patch configures LLDB.framework to build as a flat unversioned framework on non-macOS Darwin targets, which

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

vsk added a comment. This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable. > For "trace load", I get the plugin for the JSON file by matching it up with > the "name" field in the JSON, but I

[Lldb-commits] [PATCH] D85243: Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC)

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85243/new/ https://reviews.llvm.org/D85243 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D85243: Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC)

vsk added a comment. I gave this another look-over, and while I didn't spot anything troubling, I'm not terribly familiar with this code. I'll give this a third pass later today if review is still needed. Comment at:

[Lldb-commits] [PATCH] D85243: [WIP] Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC)

vsk added inline comments. Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp:313 +#if defined(__APPLE__) + // Only accept "unknown" for the vendor if the host is Apple and it + // "unknown" wasn't specified (it was just returned because it was

[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

vsk added a comment. @phosek I suspect this is causing a cmake error on the lldb standalone bot, would you mind taking a look? http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake-standalone/1858/ CMake Error at

[Lldb-commits] [PATCH] D83359: [Function] Lock the function when parsing call site info

This revision was automatically updated to reflect the committed changes. Closed by commit rG6cfc90b9b791: [Function] Lock the function when parsing call site info (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83359/new/

[Lldb-commits] [PATCH] D83359: [Function] Lock the function when parsing call site info

vsk added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:661 + std::mutex + m_call_edges_lock; ///< Exclusive lock that controls read/write + /// access to m_call_edges and m_call_edges_resolved. aprantl wrote: >

[Lldb-commits] [PATCH] D83359: [SymbolFileDWARF] Lock the module when parsing call site info

vsk updated this revision to Diff 276280. vsk added a comment. Move locking up into lldb::Function, and leave a comment in SymbolFileDWARF explaining why. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83359/new/ https://reviews.llvm.org/D83359

[Lldb-commits] [PATCH] D83359: [SymbolFileDWARF] Lock the module when parsing call site info

vsk planned changes to this revision. vsk added a comment. Hm, this doesn't totally fix the race. If the mutex is contested, the Function instance may overwrite its m_call_edges vector. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83359/new/

[Lldb-commits] [PATCH] D83359: [SymbolFileDWARF] Lock the module when parsing call site info

vsk created this revision. vsk added reviewers: jasonmolenda, friss, jingham. Herald added a subscriber: aprantl. Herald added a project: LLDB. DWARF-parsing methods in SymbolFileDWARF which update module state should take the module lock. Have ParseCallEdgesInFunction do this. This could

  1   2   3   4   5   >