Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (384 w, 6 d)

Recent Activity

Today

labath accepted D89716: [LLDB][TestSuite] Improve skipIfWindowsAndNonEnglish in decorators.py.

Yep

Mon, Oct 19, 11:18 AM · Restricted Project
labath added a comment to D87442: [lldb] Show flags for memory regions.

Woa, back up. I though you were just going to add the one flag you need right now... :(

I was going for the showing flags as a feature of the "memory region" command then later adding the memory tagging flag to that.
I see your point though and yeah I don't need all the flags to unblock mte.

With the perspective I was coming from, adding a set of getter/setter for 20ish flags wasn't an appealing idea:

bool m_memory_tagged;
OptionalBool GetMemoryTagged() const { return m_memory_tagged; }
void SetMemoryTagged(bool v) { m_memory_tagged = v; }
<x20>

If it's just mt then I just add another set, can always merge it with a flags (plural) interface later if we accumulate more.

So if it makes sense to you, I will:

  • add only "mt" flag, with it's own getter/setter
  • keep the protocol changes (but only recognise the 1 flag)
  • keep the extra testing, use of expected etc. where it still applies
  • add tests to run on a memory tagging enabled kernel with qemu

Then we can at least agree in principle, even if this doesn't land until the new tagging commands have also been reviewed. (which is fine by me, but I don't have them ready yet)

Thanks for all your comments so far!

Mon, Oct 19, 6:50 AM · Restricted Project
labath added inline comments to D89477: [lldb] Port lldb gdb-server to libOption.
Mon, Oct 19, 6:35 AM · Restricted Project
labath added a comment to D89283: [trace][intel-pt] Implement the basic decoding functionality.
In D89283#2336280, @vsk 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 these trace files on the fly as the tests run, so I imagine I'll be deleting these binary files. For the time being I doubt I'll include any new binary, as what is included is more than enough to test the basic decoding functionalities.

That seems promising. Deleting those binary files after the fact doesn't address the issue, though, as they'd be part of the history. I have a question about that ld-2.17.so file in particular: is there no way to decoder/traverse a trace of a process that loads a dylib, without copying all of ld.so into the source tree? That seems very surprising -- I'd expect the decoder API to allow you to skip right over PC ranges that have nothing to do with the binary you want to debug.

Mon, Oct 19, 6:29 AM · Restricted Project, Restricted Project
labath accepted D89408: [trace] rename ThreadIntelPT to ThreadTrace.
Mon, Oct 19, 3:11 AM · Restricted Project
labath added inline comments to D87172: Check if debug line sequences are starting after the first code segment.
Mon, Oct 19, 3:01 AM · Restricted Project
labath added a comment to D87868: [RFC] When calling the process mmap try to call all found instead of just the first one.

Should I still go ahead with this since @labath implemented the memory allocation on lldb-server?

Mon, Oct 19, 1:48 AM · Restricted Project
labath accepted D89646: [nfc] [lldb] Move `LookupAddress` to `DWARFCompileUnit`.

This looks fine. The dwo construction code (SymbolFileDWARFDwo::GetDWOCompileUnitForHash) already ensures that the returned object is really a compile unit.

Mon, Oct 19, 1:45 AM · Restricted Project, Restricted Project
labath added a comment to D83302: [lldb/DWARF] Don't treat class declarations with children as definitions.

Fixed stale+unused DW_AT_object_pointer+DW_AT_specification by rGfa89f641cf9f.

Mon, Oct 19, 1:36 AM · Restricted Project
labath committed rGdfb226632876: [lldb] Make DW_AT_declaration-with-children.s test more realistic (authored by labath).
[lldb] Make DW_AT_declaration-with-children.s test more realistic
Mon, Oct 19, 1:34 AM
labath added a comment to D89600: [lldb] Move copying of reproducer out of process.

Hm... Are you sure that this will help with anything? IIUC, the serialization of the VFS file map still happens in the context of the crashing process. This requires walking a bunch of data structures with liberally-asserting code, which will abort on any inconsistency it runs into. If *that* succeeds, I'd expect that the copying of the files will succeed too.

Mon, Oct 19, 1:18 AM

Fri, Oct 16

labath added a comment to D89334: [lldb] Support Python imports relative the to the current file being sourced.

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I don't think the two options are mutually exclusive. I'm pretty sure this is just a limitation of our current importing code, which could be fixed to import awesome_backtrace_analyzer/main.py as awesome_backtrace_analyzer.main like it would be from python.

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

Fri, Oct 16, 7:51 AM
labath committed rGdaae4a84828b: [lldb] Modernize PseudoTerminal::OpenSecondary (authored by labath).
[lldb] Modernize PseudoTerminal::OpenSecondary
Fri, Oct 16, 6:23 AM
labath added a comment to D82863: [LLDB] Add support to resize SVE registers at run-time.

I started drafting an email to the gdb mailing list to try to clarify this thing. For that, I was re-reading the gdb docs for the qXfer:target.xml packet, I realized that the gdb interpretation is mostly unambiguous about this thing:

  • regnum: The register’s number. If omitted, <default value algorithm...>. This register number is used to read or write the register; e.g. it is used in the remote p and P packets, and registers appear in the g and G packets in order of increasing register number.

I'm sure that whoever wrote this did not have variable-length registers in mind. However, it's interpretation for variable-length registers is pretty clear: take each register, and dump its bytes in sequence (no matter many of them are in the register at this particular time). The reason this is unambiguous is because gdb does not have the "offset" attribute of the "reg" element, and so the only way to find its 'g' offset is via this algorithm. And given your explanation about "switching" target.xml descriptions at runtime, I'm would expect this is what is happening within gdb.

The problem is that he "offset" attribute (added by lldb) makes things ambiguous, as it provides another way to find a register in the 'g' blob. So how about we "fix" that ambiguity by having lldb-server omit the "offset" attribute for variable-length registers?

Sending it over is actively harmful, as the value is very likely to be wrong, and I believe that in your current implementation these numbers are pretty much ignored by the client anyway. Lldb should already have code to automatically compute the regiser offsets when the "offset" attribute is missing (though it's possible it may not handle partially missing values correctly). This code would need to be extended to recompute those offsets whenever register sizes change (which is roughly what this patch does). We could also extend the parser with a check to ensure that all registers with explicit offsets come before offset-less registers. That would ensure that the register offsets dynamically computed by the client never overlap with any registers whose offsets are explicitly given by the server.

How does that sound?

I think the algorithm you are suggestion is quite workable. Summarizing our discussion above:

  1. registers should be sent over by lldb-server (via qRegisterInfo packet or target XML packet) in order of increasing register numbers

Yep.

Fri, Oct 16, 6:13 AM
labath added a comment to D87442: [lldb] Show flags for memory regions.

If you are wondering, the average number of flags for a Linux hello world's memory regions is 8 so 8 lines of output. Here's an example I had that does madvise.

(lldb) memory region addr
[0x00007ffff7ed5000-0x00007ffff7fd5000) rw- /dev/zero (deleted)
flags:
readable
writeable
shared
may read
may write
may execute
may share
soft-dirty
(lldb) n
(lldb) n (over madvise(addr, len, MADV_DONTFORK);)
(lldb) memory region addr
[0x00007ffff7ed5000-0x00007ffff7fd5000) rw- /dev/zero (deleted)
flags:
readable
writeable
shared
may read
may write
may execute
may share
do not copy area on fork
soft-dirty

I could add this as a test case but it only really adds checking that the region cache is updated which I think is covered already.

Fri, Oct 16, 5:59 AM · Restricted Project
labath accepted D89179: New test to check if basic block sections produces the right back traces with lldb.

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

I am reading this as rewriting this as an API test would not help us much. If you see benefits with an API test, I am happy to change this or add that too. Thanks!

Fri, Oct 16, 5:50 AM · Restricted Project
labath updated the diff for D89477: [lldb] Port lldb gdb-server to libOption.

address review feedback

Fri, Oct 16, 5:46 AM · Restricted Project
labath added inline comments to D89477: [lldb] Port lldb gdb-server to libOption.
Fri, Oct 16, 5:46 AM · Restricted Project
labath committed rGe338ca7bced1: [lldb] Fix FreeBSD build for ea3a547 (authored by labath).
[lldb] Fix FreeBSD build for ea3a547
Fri, Oct 16, 4:52 AM
labath added a comment to D89408: [trace] rename ThreadIntelPT to ThreadTrace.

Addressed the issues that @labath pointed out regarding the TraceProcess and TraceThread:

  • These classes were not together
  • TraceProcess was an optional plug-in, which would break the Trace parsing logic if plugged-out

So I ended up moving TraceProcess and TraceThread next to Trace in LLDB core, guaranteeing that the "trace" process plug-in will always exist and now the source file structure looks better.

Fri, Oct 16, 4:45 AM · Restricted Project

Thu, Oct 15

labath requested review of D89477: [lldb] Port lldb gdb-server to libOption.
Thu, Oct 15, 8:56 AM · Restricted Project
labath added a comment to D89334: [lldb] Support Python imports relative the to the current file being sourced.

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Imagine the user has the following (reasonable, I think) file structure.

$ROOT/utils/consult_oracle.py
$ROOT/automatic_bug_finder/main.py # uses consult_oracle.py
$ROOT/awesome_backtrace_analyzer/main.py # uses consult_oracle.py
$ROOT/install_super_scripts.lldbinit # calls command script import awesome_backtrace_analyzer/main.py

If "command script import awesome_backtrace_analyzer/main.py" ends up adding $ROOT/awesome_backtrace_analyzer to the path, then this module will have a hard time importing the modules it depends on (it would either have to use weird relative imports, or mess with sys.path itself. If we add just $ROOT then it could simply import utils.consult_oracle.

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

Thu, Oct 15, 2:27 AM
labath added a comment to D89408: [trace] rename ThreadIntelPT to ThreadTrace.

Making this a separate patch is a good idea, but it does not materially address my objections to having ProcessTrace and ThreadTrace live in two different places. :(

Thu, Oct 15, 1:45 AM · Restricted Project
labath added a comment to D89413: [lldb] [Process/FreeBSDRemote] Initial multithreading support.

Seems pretty straight forward.

Thu, Oct 15, 1:35 AM · Restricted Project

Wed, Oct 14

labath added a comment to D89124: [lldb-server][linux] Add ability to allocate memory.

It looks like the windows lldb-server implementation does not support all permission combinations (ConvertLldbToWinApiProtect in ProcessDebugger.cpp). I've xfailed the test for now -- I'll try to add the missing bits tomorrow.

Wed, Oct 14, 11:46 AM · Restricted Project
labath committed rGa1ab2b773b6d: [lldb] More memory allocation test fixes (authored by labath).
[lldb] More memory allocation test fixes
Wed, Oct 14, 11:44 AM
labath committed rG36f22cd28d5e: [lldb] Fix TestGdbRemoteMemoryAllocation on windows (authored by labath).
[lldb] Fix TestGdbRemoteMemoryAllocation on windows
Wed, Oct 14, 7:49 AM
labath committed rGea3a547f0be2: [lldb] Remove bogus ProcessMonitor forward-decls (authored by labath).
[lldb] Remove bogus ProcessMonitor forward-decls
Wed, Oct 14, 7:44 AM
labath added a comment to D82863: [LLDB] Add support to resize SVE registers at run-time.

The original g/G packets were designed for little embedded systems where the stub had to be very small and dumb, and could just memcpy the payload in the packet into the register context & back out again. Any sensible design today would, at least, have some form of an array of regnum:regval to eliminate many of the problems.

Unless of course, we make sure SVE regs come last, but that imposes some constraints on future registers sets. I suppose we might be able to say that all variable-length or otherwise-funny registers must come last.

Yeah, Omair's patch currently assumes that the SVE regs come last already when they copy & truncate the registers context to heap. I fear that we'll get to armv12 and we'll be adding registers after the SVE and wonder why they're being truncated somewhere in lldb. :)

Well.. we could design it such that the SVE registers (and any other dynamically-sized regs) always come last. Then they wouldn't impact the offsets of static registers.

I don't think we have a requirement that newer register sets must be appended. Or at least, we shouldn't have, as now both intel and arm have cpu features (and registers) that may be arbitrarily combined in future processors.

For now I am considering variable sized registers i-e SVE Z and P registers to always come last which is currently the case and will be in future patches which add support for Pointer Authentication and MTE registers.

@omjavaid , what do you think about disabling g/G when we're working with SVE registers (GDBRemoteCommunicationClient::AvoidGPackets)? There are some gdb-remote stubs that can *only* read/write registers with g/G, but I think it's reasonable to say "you must implement p/P for a target with SVE", that's a generic packet shared by both lldb and gdb. We could add a more-modern g/G replacement packet, but no one would have that implemented, and if they were going to add anything, I'd have them implement p/P unless it's perf problems and they need a read-all-registers / write-all-registers packet that works with SVE.

Just scrolling through GDB log one of the answers about target XML is that target XML is not exchanged for native connection where gdbserver is not being used. For gdbserver connection, target xml is exchanged onces per connection and register sizes are updated based on vg after that internally. However testing gdb with same executable as the one I wrote for LLDB was having gdb remote connection crash which could be something to do with SVE or some other bug.

Wed, Oct 14, 7:36 AM
labath committed rG2c4226f8ac2c: [lldb-server][linux] Add ability to allocate memory (authored by labath).
[lldb-server][linux] Add ability to allocate memory
Wed, Oct 14, 6:02 AM
labath committed rG6bb123b819c6: [lldb] Modernize PseudoTerminal::OpenFirstAvailablePrimary (authored by labath).
[lldb] Modernize PseudoTerminal::OpenFirstAvailablePrimary
Wed, Oct 14, 6:02 AM
labath closed D89124: [lldb-server][linux] Add ability to allocate memory.
Wed, Oct 14, 6:02 AM · Restricted Project
labath added a comment to D89179: New test to check if basic block sections produces the right back traces with lldb.

I'm still trying to understand what platforms is this supposed to work on -- mainly to see if additional test annotations are needed. What's the story with non-x86 non-elf targets?

Wed, Oct 14, 5:24 AM · Restricted Project
labath added a comment to D89227: [lldb] Note difference in vFile:pread/pwrite format for lldb.

Yeah, using qSupport also works. It just seems like it would be possible to handle this particular issue without adding the generic framework, so I was throwing that idea out too.

Wed, Oct 14, 5:12 AM · Restricted Project
labath added a comment to D89335: [lldb] Generic support for caching register set reads/writes [WIP].

I'm wondering if this could be implemented as some kind of a separate class which you "mix" into your register context when you want to do caching. Possibly something like this:

class NRCLinux_MyArch: public NRCLinux, private RCCache::Backend {
  RCCache m_cache;
Wed, Oct 14, 5:09 AM
labath added a comment to D89334: [lldb] Support Python imports relative the to the current file being sourced.

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Wed, Oct 14, 2:50 AM
labath edited reviewers for D89357: [llvm] Make obj2yaml and yaml2obj LLVM utilities instead of tools, added: mgorny, tstellar; removed: labath.

I don't have a problem with this, but I don't really know what this implies. Adding some folks who package llvm downstream.

Wed, Oct 14, 1:50 AM · Restricted Project
labath accepted D89248: [lldb] [test/Register] Add read/write tests for multithreaded process.

lg

Wed, Oct 14, 1:39 AM · Restricted Project

Tue, Oct 13

labath added a comment to D89295: [lldb] Add $HOME to Python's sys.path.

For lldbinit files, and any file that gets command source'd, I think it would be useful if they could perform command script import some/path/to/command.py, where some is resolved relative to the dirname of the lldb file. For example, given an lldbinit file at my/project/scripts/project.lldb, it could load a python at my/project/scripts/commands/my.py by running command script import commands/my.py.

+1, I think that would be useful. Not sure about the right way to implement this though (and whether we should make this implicit a relative path vs for example some kind of placeholder 'variable' or something like that).

Tue, Oct 13, 9:17 AM
labath updated the diff for D89124: [lldb-server][linux] Add ability to allocate memory.
  • GetCurrentThread
  • const
Tue, Oct 13, 8:56 AM · Restricted Project
labath added inline comments to D89124: [lldb-server][linux] Add ability to allocate memory.
Tue, Oct 13, 8:52 AM · Restricted Project
labath added inline comments to D89179: New test to check if basic block sections produces the right back traces with lldb.
Tue, Oct 13, 8:02 AM · Restricted Project
labath added a comment to D89283: [trace][intel-pt] Implement the basic decoding functionality.

I'm still digesting this, but here's my first set of comments.

Tue, Oct 13, 7:52 AM · Restricted Project, Restricted Project
labath added a comment to D89295: [lldb] Add $HOME to Python's sys.path.

Interesting. I think that implementing a project-specific script repository would be indeed best done in xcode, since that's the thing which knows what a "project" is.

Tue, Oct 13, 6:32 AM
labath added a comment to D89227: [lldb] Note difference in vFile:pread/pwrite format for lldb.

Given how often the '0x' prefix on base16 numbers is dropped in the remote serial protocol

Now you mention it, I thought I knew how strtoull worked but I don't. (I assumed it was some best effort that would roll back if somehow the digits didn't fit the assumed base)

This is what a gdb pread looks like:

getpkt ("vFile:pread:6,3fff,9ab473c8");  [no ack sent]

So in fact it is doing exactly what you said and not including the 0x.

What do you think?

Yes I think sending hex with the 0x is the clearest indicator for implementers of their own lldb-servers. However gdb-server doesn't expect the leading 0x (I don't see the spec mandating it either way) so it doesn't solve that side, if that is a goal.

So would it make more sense to change lldb to send unprefixed hex like gdb does but update lldb-server to accept either? (by trying base 10 then base 16). Especially if implementers are going to prefer the packet specification over reading packet logs from lldb.

That's not perfect either, an older lldb-server with a new lldb client could happen to parse the number as base 10 but you'll get a unexpected result. (at least adding 0x would give you a hard error there) Do we commit to that kind of compatibility?

Tue, Oct 13, 6:12 AM · Restricted Project
labath accepted D89310: [lldb] Reject redefinitions of persistent variables.

Looks straight-forward enough.

Tue, Oct 13, 5:41 AM · Restricted Project
labath added a comment to D89295: [lldb] Add $HOME to Python's sys.path.

What exactly is the use case for this?

Tue, Oct 13, 5:37 AM
labath added a comment to D89179: New test to check if basic block sections produces the right back traces with lldb.

I think this looks reasonable. A couple of comments inline.

Tue, Oct 13, 5:19 AM · Restricted Project

Mon, Oct 12

labath added a comment to D89124: [lldb-server][linux] Add ability to allocate memory.

In file included from /home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/NativeProcessProtocol.cpp:9:
/home/jkratoch/redhat/llvm-monorepo/lldb/include/lldb/Host/common/NativeProcessProtocol.h:20:10: fatal error: 'lldb/Utility/UnimplementedError.h' file not found
#include "lldb/Utility/UnimplementedError.h"

Mon, Oct 12, 7:40 AM · Restricted Project
labath added inline comments to D89227: [lldb] Note difference in vFile:pread/pwrite format for lldb.
Mon, Oct 12, 6:45 AM · Restricted Project
labath requested review of D89236: [lldb] Fix bitfield "frame var" for pointers (pr47743).
Mon, Oct 12, 6:30 AM · Restricted Project
labath committed rGe2f1fe361a9c: [lldb/Utility] Introduce UnimplementedError (authored by labath).
[lldb/Utility] Introduce UnimplementedError
Mon, Oct 12, 4:58 AM
labath closed D89121: [lldb/Utility] Introduce UnimplementedError.
Mon, Oct 12, 4:58 AM · Restricted Project
labath accepted D89227: [lldb] Note difference in vFile:pread/pwrite format for lldb.

Ah, interesting. We already know of one incompatibility in vFile packets -- it seems you have found a second one.

Mon, Oct 12, 4:30 AM · Restricted Project
labath added inline comments to D89124: [lldb-server][linux] Add ability to allocate memory.
Mon, Oct 12, 2:44 AM · Restricted Project
labath updated the diff for D89124: [lldb-server][linux] Add ability to allocate memory.

address review comments

Mon, Oct 12, 2:44 AM · Restricted Project
labath added a comment to D89193: [lldb] [Process/FreeBSDRemote] Support YMM reg via PT_*XSTATE.

I assume this fixes some of the tests we have already, but it would be good to mention that in the description.

Mon, Oct 12, 1:10 AM · Restricted Project
labath added a comment to D89182: [lldb] [Process/FreeBSDRemote] Kill process via PT_KILL.

FWIW, PTHREAD_KILL is strongly discouraged on linux. But if the situation is different on freebsd, then fine.

Mon, Oct 12, 12:55 AM · Restricted Project
labath accepted D88769: [trace] Scaffold "thread trace dump instructions".
Mon, Oct 12, 12:54 AM · Restricted Project
labath accepted D89165: [nfc] [lldb] Simplify calling SymbolFileDWARF::GetDWARFCompileUnit.

Makes sense.

Mon, Oct 12, 12:51 AM · Restricted Project

Fri, Oct 9

labath added a comment to D82863: [LLDB] Add support to resize SVE registers at run-time.

The original g/G packets were designed for little embedded systems where the stub had to be very small and dumb, and could just memcpy the payload in the packet into the register context & back out again. Any sensible design today would, at least, have some form of an array of regnum:regval to eliminate many of the problems.

Unless of course, we make sure SVE regs come last, but that imposes some constraints on future registers sets. I suppose we might be able to say that all variable-length or otherwise-funny registers must come last.

Yeah, Omair's patch currently assumes that the SVE regs come last already when they copy & truncate the registers context to heap. I fear that we'll get to armv12 and we'll be adding registers after the SVE and wonder why they're being truncated somewhere in lldb. :)

Well.. we could design it such that the SVE registers (and any other dynamically-sized regs) always come last. Then they wouldn't impact the offsets of static registers.

Fri, Oct 9, 6:11 AM
labath added inline comments to D89124: [lldb-server][linux] Add ability to allocate memory.
Fri, Oct 9, 5:44 AM · Restricted Project
labath requested review of D89124: [lldb-server][linux] Add ability to allocate memory.
Fri, Oct 9, 5:39 AM · Restricted Project
labath requested review of D89121: [lldb/Utility] Introduce UnimplementedError.
Fri, Oct 9, 5:36 AM · Restricted Project
labath added a comment to D88769: [trace] Scaffold "thread trace dump instructions".

I was waiting for the dependant patch to take shape. This looks ok to me -- just one small design question.

Fri, Oct 9, 1:55 AM · Restricted Project
labath accepted D88841: [intel pt] Refactor parsing.

Looks good.

Fri, Oct 9, 1:40 AM · Restricted Project
labath committed rG0610a25a85a0: [lldb] Delete copy operations on PluginInterface class (authored by labath).
[lldb] Delete copy operations on PluginInterface class
Fri, Oct 9, 1:38 AM

Thu, Oct 8

labath committed rG19d64138e6a7: [lldb] Fix "frame var" for large bitfields (authored by labath).
[lldb] Fix "frame var" for large bitfields
Thu, Oct 8, 9:43 AM
labath committed rGd4a7c70751cf: [lldb] Add a cmake warning about the python/swig incompatibility (authored by labath).
[lldb] Add a cmake warning about the python/swig incompatibility
Thu, Oct 8, 9:43 AM
labath closed D88992: [lldb] Fix "frame var" for large bitfields.
Thu, Oct 8, 9:43 AM · Restricted Project
labath closed D88967: [lldb] Add a cmake warning about the python/swig incompatibility.
Thu, Oct 8, 9:43 AM · Restricted Project
labath added a comment to D88967: [lldb] Add a cmake warning about the python/swig incompatibility.

If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.

Is that problem only theoretical (like, "you shouldn't be doing that") or does it have some practical consequences (crashes, incorrect operation, etc.)?

My memory isn't that detailed. I think it was an argument being passed in as a single value but being referenced as though it was an array or list. Or vice versa.

Thu, Oct 8, 9:13 AM · Restricted Project
labath added a comment to D82863: [LLDB] Add support to resize SVE registers at run-time.

I haven't looked at the new version of the patch yet, partly because I'm busy (sorry), and partly because I'm not sure we have reached a consensus yet (or at least, I don't know what that consensus is).

Thu, Oct 8, 9:07 AM
labath added a comment to D88841: [intel pt] Refactor parsing.

I like this. A lot of comments, but they're all cosmetic.

Thu, Oct 8, 8:41 AM · Restricted Project
labath added a comment to D88387: Create "skinny corefiles" for Mach-O with process save-core / reading.

/me parachutes into the discussion.

Thu, Oct 8, 5:51 AM · Restricted Project
labath accepted D88975: [LLDB] On Windows, fix tests.

This looks fine. Thanks.

Thu, Oct 8, 5:10 AM · Restricted Project
labath added a comment to D89019: Change the default handling of SIGCONT to nostop/noprint.

I believe we should at least print/notify about the fact that the process has received a SIGCONT (like we do with SIGCHLD, which happens a lot more often). As for stopping, I don't really have a strong opinion.

Thu, Oct 8, 4:49 AM · Restricted Project

Wed, Oct 7

labath added a comment to D88975: [LLDB] On Windows, fix tests.

I really want to avoid having language specific strings in the tests. Skipping tests which depend on these is a reasonable approach (as you've discovered, there shouldn't be that many of them -- it's really just the tests which check that we actually can retrieve the error from the OS), and it could be later improved by switching to an english locale on a best-effort basis (i.e., if the locale is available).

Wed, Oct 7, 11:49 AM · Restricted Project
labath added a comment to D88967: [lldb] Add a cmake warning about the python/swig incompatibility.

If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.

Wed, Oct 7, 11:38 AM · Restricted Project
labath requested review of D88992: [lldb] Fix "frame var" for large bitfields.
Wed, Oct 7, 11:20 AM · Restricted Project
labath accepted D88940: Add regular expressions to and DWARF Call Frame Information tests in case the architecture specific target is not compiled into LLVM..
Wed, Oct 7, 10:44 AM · Restricted Project
labath added a comment to D88975: [LLDB] On Windows, fix tests.

Is there a way to force a program to use a specific locale, ignoring the system-wide setting (something like export LC_ALL=C on unix)? Hardcoding the error message for all locales is not a viable long term strategy... :/

Wed, Oct 7, 9:18 AM · Restricted Project
labath added a comment to D87442: [lldb] Show flags for memory regions.

I think this patch for its test coverage.

Wed, Oct 7, 8:10 AM · Restricted Project
labath requested review of D88967: [lldb] Add a cmake warning about the python/swig incompatibility.
Wed, Oct 7, 7:42 AM · Restricted Project
labath committed rG3dfb94986170: [lldb] Check for and use ptsname_r if available (authored by labath).
[lldb] Check for and use ptsname_r if available
Wed, Oct 7, 6:30 AM
labath committed rG029290f1a623: [lldb/docs] Clarify python/swig version incompatibility (authored by labath).
[lldb/docs] Clarify python/swig version incompatibility
Wed, Oct 7, 6:30 AM
labath closed D88728: [lldb] Check for and use ptsname_r if available.
Wed, Oct 7, 6:29 AM · Restricted Project
labath closed D88906: [lldb/docs] Clarify python/swig version incompatibility.
Wed, Oct 7, 6:29 AM · Restricted Project
labath added a comment to D88728: [lldb] Check for and use ptsname_r if available.

LGTM. Worth mentioning that this will be in POSIX issue 8 https://www.austingroupbugs.net/bug_view_page.php?bug_id=508

Wed, Oct 7, 6:24 AM · Restricted Project
labath added a comment to D88906: [lldb/docs] Clarify python/swig version incompatibility.

Yes, I also (independently) discovered this problem.

Wed, Oct 7, 6:20 AM · Restricted Project
labath added inline comments to D87868: [RFC] When calling the process mmap try to call all found instead of just the first one.
Wed, Oct 7, 4:55 AM · Restricted Project
labath added a comment to D88940: Add regular expressions to and DWARF Call Frame Information tests in case the architecture specific target is not compiled into LLVM..

This basically removes the testing for the original patch, since all of these tests would now pass even if the feature is completely reverted. Fortunately, I think the fix for that is easy. We don't need to add the regex to tests that already presuppose that the relevant llvm target is available. By the looks of things, that's everything except test/DebugInfo/dwarfdump-debug-frame-simple.test.

Wed, Oct 7, 4:41 AM · Restricted Project

Tue, Oct 6

labath added a comment to D88840: [dotest] Simplify logic to find the Python path.

One way this could be simplified further is to ditch -P and pass down the appropriate value from cmake/lit.

LLDB in Xcode supports both Python 2 and 3 and we use a default com.apple.dt.lldb DefaultPythonVersion to override the version. By using -P here we can run the test suite with both versions by just changing the value of the default without having to reconfigure CMake. I'm not saying we should not do this because our donwstream fork, just that it would complicate things for us.

Tue, Oct 6, 11:07 AM · Restricted Project
labath added inline comments to D87868: [RFC] When calling the process mmap try to call all found instead of just the first one.
Tue, Oct 6, 9:09 AM · Restricted Project
labath added a comment to D88906: [lldb/docs] Clarify python/swig version incompatibility.

Jonas, the notion that 3.7 is incompatible comes from ae7389116. Do you happen to remember what was the issue you ran into?

Tue, Oct 6, 8:47 AM · Restricted Project
labath requested review of D88906: [lldb/docs] Clarify python/swig version incompatibility.
Tue, Oct 6, 8:44 AM · Restricted Project
labath added a comment to D88840: [dotest] Simplify logic to find the Python path.

About -P, the man page for lldb and the driver's Options.td say it:

Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.

It can print the path, or it can print <COULD NOT FIND PATH> if e.g. lldb wasn't built with python support or has some non-standard python setup. So I think this is basically as simple as the parsing can get.

Tue, Oct 6, 8:40 AM · Restricted Project
labath added inline comments to D88796: [lldb] Initial version of FreeBSD remote process plugin.
Tue, Oct 6, 6:55 AM · Restricted Project
labath added inline comments to D88841: [intel pt] Refactor parsing.
Tue, Oct 6, 6:49 AM · Restricted Project
labath added a comment to D88769: [trace] Scaffold "thread trace dump instructions".
  • Comments on the architecture of classes are in that diff

Where would that be? I couldn't find anything that would answer my question from the previous comment?

The reason I want to know that is that if each TraceXXX "plugin" is supposed to have/use/need a ProcessTraceXXX plugin, then I'd rather organize them such that they are closer together. OTOH, if ProcessTrace is supposed to work with all kinds of traces, then the current design makes perfect sense (though I am having trouble imagining how would that work).

Tue, Oct 6, 6:22 AM · Restricted Project