Yep

In D87442#2334786, @DavidSpickett wrote:

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!

In D89283#2336280, @vsk wrote:

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 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.

In D87868#2337537, @aadsm wrote:

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

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

In D83302#2337264, @jankratochvil wrote:

Fixed stale+unused DW_AT_object_pointer+DW_AT_specification by rGfa89f641cf9f.

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.

In D89334#2332452, @JDevlieghere wrote:

In D89334#2331903, @labath wrote:

In D89334#2330287, @JDevlieghere wrote:

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.

In D82863#2331892, @omjavaid wrote:

In D82863#2330122, @labath wrote:

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.

In D87442#2332244, @DavidSpickett wrote:

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.

In D89179#2332781, @tmsriram wrote:

In D89179#2329922, @labath wrote:

In D89179#2328195, @dblaikie wrote:

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!

address review feedback

In D89408#2333502, @wallace wrote:

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.

In D89334#2330287, @JDevlieghere wrote:

In D89334#2329667, @labath wrote:

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.

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. :(

Seems pretty straight forward.

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.

In D82863#2324560, @omjavaid wrote:

In D82863#2321647, @labath wrote:

In D82863#2321370, @omjavaid wrote:

In D82863#2320342, @jasonmolenda wrote:

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.

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?

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.

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;

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.

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

lg

In D89295#2327767, @teemperor wrote:

In D89295#2327751, @kastiglione wrote:

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).

• GetCurrentThread
• const

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

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.

In D89227#2327066, @DavidSpickett wrote:

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?

Looks straight-forward enough.

What exactly is the use case for this?

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

In D89124#2325008, @jankratochvil wrote:

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"

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

address review comments

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

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

Makes sense.

In D82863#2321370, @omjavaid wrote:

In D82863#2320342, @jasonmolenda wrote:

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 was waiting for the dependant patch to take shape. This looks ok to me -- just one small design question.

Looks good.

In D88967#2317851, @amccarth wrote:

In D88967#2317545, @labath wrote:

In D88967#2317522, @amccarth wrote:

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.

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).

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

/me parachutes into the discussion.

This looks fine. Thanks.

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.

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).

In D88967#2317522, @amccarth wrote:

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

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... :/

In D87442#2300987, @labath wrote:

I think this patch for its test coverage.

In D88728#2315648, @MaskRay wrote:

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

In D88906#2314777, @amccarth wrote:

Yes, I also (independently) discovered this problem.

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.

In D88840#2314860, @JDevlieghere wrote:

In D88840#2314594, @labath wrote:

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.

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

In D88840#2314587, @rupprecht wrote:

In D88840#2313331, @kastiglione wrote:

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.

In D88769#2314164, @labath wrote:

In D88769#2312482, @wallace wrote:

• 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).