[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

wallace added inline comments. Comment at: lldb/include/lldb/Target/Target.h:1105 + void SetTrace(const lldb::TraceSP _sp); + wallace wrote: > JDevlieghere wrote: > > Who owns the trace? If there's a 1:1 relationship between a trace and a > > target, can we

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

wallace added a comment. Thanks for the review! Those are very useful comments. So, I'll split the parsing out from the Trace object. Regarding a possible inconsistent state, you are right. That could happen. The targets, modules and processes used in the parsing are all created there, and it

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

wallace added inline comments. Comment at: lldb/include/lldb/Target/Target.h:1105 + void SetTrace(const lldb::TraceSP _sp); + JDevlieghere wrote: > Who owns the trace? If there's a 1:1 relationship between a trace and a > target, can we make the target its

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

kastiglione added inline comments. Comment at: lldb/source/API/SBPlatform.cpp:59 + +if (command_interpreter && command_interpreter[0]) { + full_command += command_interpreter; JDevlieghere wrote: > Given that this pattern repeats a few times in this

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

JDevlieghere added inline comments. Comment at: lldb/source/API/SBPlatform.cpp:59 + +if (command_interpreter && command_interpreter[0]) { + full_command += command_interpreter; Given that this pattern repeats a few times in this struct, maybe a small

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

JDevlieghere added inline comments. Comment at: lldb/include/lldb/Target/Target.h:1105 + void SetTrace(const lldb::TraceSP _sp); + Who owns the trace? If there's a 1:1 relationship between a trace and a target, can we make the target its owner? I'm trying to

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

JDevlieghere added a comment. In D85705#2240249 , @wallace wrote: > Given that this feature is intended to be used by non debugger developers, I > want to stick to using JSON as a default input format, as it's more > ubiquitous than YAML and I prefer to

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

wallace created this revision. wallace added a reviewer: clayborg. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. wallace requested review of this revision. Herald added a subscriber: JDevlieghere. Depends on D85705 . The previous diff left

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

aprantl added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257 +} else if (const char *str = const_value_form.AsCString()) { + uint32_t string_length = strlen(str) + 1; + location = DWARFExpression(

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

aprantl added a comment. Sorry, I missed this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86662/new/ https://reviews.llvm.org/D86662 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

mib updated this revision to Diff 288147. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86667/new/ https://reviews.llvm.org/D86667 Files: lldb/bindings/interface/SBPlatform.i lldb/include/lldb/API/SBPlatform.h

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

mib created this revision. mib added reviewers: labath, JDevlieghere, jingham. mib added a project: LLDB. Herald added subscribers: lldb-commits, dang. mib requested review of this revision. This patch adds the ability to use a custom interpreter with the `platform shell` command. If the user

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

comex added a comment. Disabling the thread list while the target is running sounds like a pretty complex change. For example, what should happen if a Python script calls `lldb.process.GetThreadAtIndex(n)` while the target is running, which currently works? And is it really the right

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

wallace updated this revision to Diff 288145. wallace added a comment. - As error messaging is important when parsing, I create a new set of functions in StructuredData for parsing that return an llvm::Expected. This allowed me to move the helper functions for parsing from the Trace file to

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc55db4600b4b: Load correct module for linux and android when duplicates exist in minidump. (authored by clayborg). Changed prior to commit:

[Lldb-commits] [lldb] c55db46 - Load correct module for linux and android when duplicates exist in minidump.

Author: Greg Clayton Date: 2020-08-26T15:48:34-07:00 New Revision: c55db4600b4bdc5664784983fefb82bd8189bafc URL: https://github.com/llvm/llvm-project/commit/c55db4600b4bdc5664784983fefb82bd8189bafc DIFF: https://github.com/llvm/llvm-project/commit/c55db4600b4bdc5664784983fefb82bd8189bafc.diff

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

shafik accepted this revision. shafik added a comment. LGTM with minor comments. Thank you for these fixes, they are awesome! Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3213 + bool use_type_size_for_value = false; + if

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

wallace added inline comments. Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:44 +# symbol_regex = re.compile(r"Symbols loaded. \([0-9]+(\.[0-9]*)?[KMG]?B\)") +symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

aprantl added a comment. Could you please also include a testcase? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86662/new/ https://reviews.llvm.org/D86662 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

jingham added a comment. We should be able to calculate ShouldReportRun before we actually set the run going. That's better than just querying potentially stale threads. It would also be good to find a way to prevent ourselves from consulting the thread list after we've decided to invalidate

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. This change is fine for what it does, but I don't think the model that it allows is really supportable. If you have multiple listeners on the process events, and Listener A wants

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

wallace requested changes to this revision. wallace added a comment. This revision now requires changes to proceed. For the record, this came out of discussion in which the consensus was to display less characters and maximize the relevant information, which in this case is just the debug info

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

aelitashen created this revision. aelitashen added reviewers: wallace, clayborg. Herald added subscribers: lldb-commits, aprantl. Herald added a project: LLDB. aelitashen requested review of this revision. Herald added a subscriber: JDevlieghere. The Symbol Status in modules view is simplified so

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

wallace added a comment. Given that this feature is intended to be used by non debugger developers, I want to stick to using JSON as a default input format, as it's more ubiquitous than YAML and I prefer to make this decision based on the user experience than on the technical solution. I like

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik created this revision. shafik added reviewers: martong, teemperor. Herald added a subscriber: rnkovacs. shafik requested review of this revision. When we fixed `ImportDeclContext(...)` in D71378 to make sure we complete each `FieldDecl` of a `RecordDecl`

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

JDevlieghere added a comment. In D85705#2237506 , @clayborg wrote: > In D85705#2237397 , @JDevlieghere > wrote: > >> A large part of this patch is concerned with parsing which worries me from a >> maintenance

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

ilya-nozhkin created this revision. ilya-nozhkin added reviewers: jingham, clayborg, labath. ilya-nozhkin added a project: LLDB. Herald added subscribers: lldb-commits, JDevlieghere. ilya-nozhkin requested review of this revision. Process does something with its state and threads when an event is

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

JDevlieghere updated this revision to Diff 288051. JDevlieghere added a comment. - Address code review feedback - Run the verifier when replaying a reproducer - Add SBReplayOptions instead of adding another overload - Add `--reproducer-skip-verify` flag CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

JosephTremoulet added a comment. To clarify, the failure did not reproduce for me in the SUSE 15.02 container. I don't know what I'm doing differently. To create the environment, I just did `docker run ... opensuse/leap:15.2`, and then `zypper in gcc-c++ python3-devel` inside the container.

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

teemperor requested changes to this revision. teemperor added a comment. This revision now requires changes to proceed. As discussed offline, I think the warnings here seem also useful when just replaying. I guess we should make it clear that things like missing files are fine when replaying a

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. What a function :-) Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3134 if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) || (tag ==

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

JosephTremoulet added a comment. FWIW, I tried running the test in an SUSE 15.2 docker container (on an Ubuntu 18.04 host), and here's what I'm seeing. `raise` and `abort` indeed do not indicate that they had the `S` set in the augmentation: (lldb) bt * thread #1, name = 'a.out', stop

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

sivachandra accepted this revision. sivachandra added a comment. Libc change LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86616/new/ https://reviews.llvm.org/D86616 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D86292: [LLDB][RISCV] Distinguish between riscv32 and riscv64 based on ELF class

jrtc27 added a comment. Not so silly; gdb (well, the names are inherited from bfd) has `set architecture riscv:rv32`/`set architecture riscv:rv64` :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86292/new/ https://reviews.llvm.org/D86292

[Lldb-commits] [PATCH] D85820: Use find_library for ncurses

phosek added a comment. In D85820#2235461 , @haampie wrote: > @gkistanova It's true that this change has lead to more issues I could ever > imagine, but I think the link you provided is the last remaining problem. > > Pinging @phosek for a similar issue

[Lldb-commits] [PATCH] D86521: Revert "Use find_library for ncurses"

gkistanova accepted this revision. gkistanova added a comment. This revision is now accepted and ready to land. I have checked this patch with http://lab.llvm.org:8011/builders/lld-perf-testsuite bot. It makes it green. Thanks for reverting, Harmen! Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D86292: [LLDB][RISCV] Distinguish between riscv32 and riscv64 based on ELF class

luismarques added a comment. In D86292#2237143 , @jrtc27 wrote: > Not so silly; gdb (well, the names are inherited from bfd) has `set > architecture riscv:rv32`/`set architecture riscv:rv64` :) Ha! We're in good company then. Thanks for sharing,

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

JosephTremoulet added a comment. > eh_frame augmented UnwindPlan: > ... > This UnwindPlan is for a trap handler function: yes. From memory, I'd have expected that for `__restore_rt`, but not for `raise` or `abort`. Can you dump the augmentation fields from the CIEs (or instrument

[Lldb-commits] [lldb] 4a15f51 - [lldb][NFC] Simplify string literal in GDBRemoteCommunicationClient

Author: Raphael Isemann Date: 2020-08-26T16:25:11+02:00 New Revision: 4a15f51a4f7726e12c327fa30e76d90a2b90430b URL: https://github.com/llvm/llvm-project/commit/4a15f51a4f7726e12c327fa30e76d90a2b90430b DIFF:

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

teemperor accepted this revision. teemperor added a comment. LGTM too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86616/new/ https://reviews.llvm.org/D86616 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. LGTM, but I'm not an owner for any of the projects touched by this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86616/new/

[Lldb-commits] [PATCH] D86436: [lldb] Fix Type::GetByteSize for pointer types

labath added a comment. In D86436#2234110 , @davide wrote: > @labath something I noticed when finding this (and related bugs) is that > `frame var` carries a decent diagnostic > > (int *) l_125 = > > and the expression parser returns something not

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

labath created this revision. labath added reviewers: ldionne, beanz. Herald added subscribers: libc-commits, lldb-commits, dexonsmith, hiraditya, mgorny. Herald added a reviewer: bollu. Herald added a reviewer: DavidTruby. Herald added projects: LLDB, libc-project, LLVM. labath requested review

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

labath created this revision. labath added reviewers: aprantl, shafik. Herald added a project: LLDB. labath requested review of this revision. Herald added a subscriber: JDevlieghere. Class-level static constexpr variables can have both DW_AT_const_value (in the "declaration") and a

[Lldb-commits] [lldb] 642cb78 - Copy m_plan_is_for_signal_trap member.

Author: Benjamin Kramer Date: 2020-08-26T13:27:01+02:00 New Revision: 642cb7865f35ad7dbac78d716fcddaff561e8639 URL: https://github.com/llvm/llvm-project/commit/642cb7865f35ad7dbac78d716fcddaff561e8639 DIFF:

[Lldb-commits] [PATCH] D86348: [lldb/DWARF] More DW_AT_const_value fixes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG82982304d709: [lldb/DWARF] More DW_AT_const_value fixes (authored by labath). Changed prior to commit:

[Lldb-commits] [lldb] 8298230 - [lldb/DWARF] More DW_AT_const_value fixes

Author: Pavel Labath Date: 2020-08-26T13:17:26+02:00 New Revision: 82982304d7095891b10faacdbf9b4eb73e92a92f URL: https://github.com/llvm/llvm-project/commit/82982304d7095891b10faacdbf9b4eb73e92a92f DIFF: https://github.com/llvm/llvm-project/commit/82982304d7095891b10faacdbf9b4eb73e92a92f.diff

[Lldb-commits] [PATCH] D80659: [lldb-vscode] Redirect stderr and stdout to DAPs console message

labath added a comment. Sorry, I missed this patch. Modulo the inline comments and the missing windows support and tests, I think this patch is ok-ish. However, I am still not convinced of its usefulness. Why would we be doing something (particularly a thing which will be hard to do in a

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

This revision was automatically updated to reflect the committed changes. Closed by commit rG9ad5d37fd917: [lldb] Correct wording of EXP_MSG (authored by DavidSpickett). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86603/new/

[Lldb-commits] [lldb] 9ad5d37 - [lldb] Correct wording of EXP_MSG

Author: David Spickett Date: 2020-08-26T11:52:30+01:00 New Revision: 9ad5d37fd917a5b8b3ffe5c12145c85021ee2578 URL: https://github.com/llvm/llvm-project/commit/9ad5d37fd917a5b8b3ffe5c12145c85021ee2578 DIFF:

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

teemperor added a comment. That's true, `EXP_MSG` is not used like the other messages so the only problem with it is the confusing error message. IMHO this is still good to land as it improves the error message. If you want to implement Pavel's much nicer error message in a follow-up patch,

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

DavidSpickett added a comment. Ok to land this as is just to remove some confusion? I agree it could be much improved, I'll look into that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86603/new/ https://reviews.llvm.org/D86603

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Thanks for the clarification. The new version looks great. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86375/new/

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

labath added a comment. Though these extra messages are often superfluous in general, I don't think that's the case for this particular message. This macro is only used in the `expect` and `match` checks. These are the most common way of asserting something right now, so I think it's

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

teemperor accepted this revision. teemperor added a comment. This revision is now accepted and ready to land. FWIW, I actually think this patch itself is perfectly fine. As these messages are only displayed when an assert failed they should be worded with the assumption that the assert did

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

teemperor added a reviewer: LLDB. teemperor added a comment. If you look at the other "assert messages" above and below your patch you'll see that all (most?) of them use this very confusing "Say the opposite of what actually happened" style. Weirdly enough only around 70% of the "assertion

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

DavidSpickett created this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. DavidSpickett requested review of this revision. Herald added a subscriber: JDevlieghere. EXP_MSG generates a message to show on assert failure. Currently it looks like: AssertionError:

[Lldb-commits] [PATCH] D82865: [LLDB] Add GetByteOffset to SBValue interface for reading register offset

labath added a comment. Sounds good. FWIW, I wouldn't be opposed to some CLI command which would dump some low-level details of the register context, similar to how we have the "target modules dump" command tree for Module objects... CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D82855: [LLDB] Send SVE vg register in custom expedited registerset

labath added a comment. This looks good, though it would be nice to add a test for it. It looks like TestGdbRemoteExpeditedRegisters.py has a lot of functionality that could be reused for this purpose. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82855/new/

[Lldb-commits] [PATCH] D82853: [LLDB] Support custom expedited register set in gdb-remote

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I like this a lot. LGTM with some small fixes inline. Comment at: lldb/source/Host/common/NativeRegisterContext.cpp:428-434 +static const uint32_t

[Lldb-commits] [lldb] 7518006 - [lldb] XFAIL TestMemoryHistory on Linux

Author: Raphael Isemann Date: 2020-08-26T10:24:13+02:00 New Revision: 7518006d75accd21325747430d6bced66b2c5ada URL: https://github.com/llvm/llvm-project/commit/7518006d75accd21325747430d6bced66b2c5ada DIFF:

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

clayborg updated this revision to Diff 287853. clayborg added a comment. Added a better algorithm for detecting if an address from the module list for linux processes with valid /proc//maps file is executable. We now search consective addresses that match the path to the module for any that are

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

llunak added a comment. In D86417#2237602 , @jasonmolenda wrote: > lldb thinks that both frames 1 & 2 are trap handler frames. They have full > register context available for the frame above them on the stack (that is, > frames 2 & 3) and frames 2 & 3