[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D98482#2624973 , @labath wrote:

> I'm still bugged by the GetPidTid interface -- the pair of optionals is very 
> complicated. What if we had this function take a `default_pid` argument 
> (which would be equal to the currently selected process), and it returned 
> that as the process id, in case the packet does not contain one? Then the 
> interface could be something like `Optional> 
> GetPidTid(pid_t default_pid)` (because tid automatically defaults to -1), 
> which I think is more intuitive. In case we really need to differentiate 
> between a pid not being present and an implicitly chosen pid, then the 
> interface could be like `Optional, tid_t>> GetPidTid()`, 
> which still seems /slightly/ better as its easier to recognise invalid input. 
> WDYT?

I've originally wanted to precisely convey whatever was in the packet since it 
was a generic class, and I actually thought nested `Optional`s are going to be 
worse. But thinking about it, you're probably right — the spec explicitly says 
to assume `tid=-1` when only pid is provided, so I guess we can use a single 
`Optional` and just assume a default pid. However, I think I'm going to go a 
step further and eliminate `pair` as well, in favor of 
`llvm::Optional GetPidTid(lldb:pid_t& pid)`, with `pid` being 
modified if specified or otherwise left in its current (i.e. default) value.


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

https://reviews.llvm.org/D98482

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

[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

In D96458#2624962 , @labath wrote:

> Thanks. This looks much better, but there is still one thing which bugs me 
> about the register info constructor. Currently, there are three paths through 
> that constructor:
>
> 1. when we don't have any fancy registers (this is the original one)
> 2. when we have just SVE (added with the sve support)
> 3. when we have pauth et al. (added now)
>
> Do we need all three of them? Is there anything which makes SVE special that 
> it deserves its own register info array? Could it be just another "dynamic" 
> register set like the other features? (If that's true, I might consider also 
> removing the first code path, making it just a special case of an empty set 
> of dynamic features.)
>
> I also have some inline comments, but there are just simple stylistic issues, 
> I hope.

I agree with your about SVE being dynamic I just didn't go for the change 
because I was being a little conservative about changing existing 
functionality. But I think If we look at the changes now making SVE dynamic 
seems right thing to do. Let me make the change and update this rev.


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

https://reviews.llvm.org/D96458

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

[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

In D96458#2625554 , @omjavaid wrote:

> In D96458#2624962 , @labath wrote:
>
>> Thanks. This looks much better, but there is still one thing which bugs me 
>> about the register info constructor. Currently, there are three paths 
>> through that constructor:
>>
>> 1. when we don't have any fancy registers (this is the original one)
>> 2. when we have just SVE (added with the sve support)
>> 3. when we have pauth et al. (added now)
>>
>> Do we need all three of them? Is there anything which makes SVE special that 
>> it deserves its own register info array? Could it be just another "dynamic" 
>> register set like the other features? (If that's true, I might consider also 
>> removing the first code path, making it just a special case of an empty set 
>> of dynamic features.)
>>
>> I also have some inline comments, but there are just simple stylistic 
>> issues, I hope.
>
> I agree with your about SVE being dynamic I just didn't go for the change 
> because I was being a little conservative about changing existing 
> functionality. But I think If we look at the changes now making SVE dynamic 
> seems right thing to do. Let me make the change and update this rev.

@labath 
I forgot to mention but recalled after looking at the code that the major 
problem/hurdle is g_contained_* and *_invalidates reg list. In case of SVE V, D 
and S registers are contained by Z register and therefore we will have to 
update their contained and value reg list accordingly. Only registers 
declaration which we can skip out of the SVE's register info array are the GPRs 
X and W. Larger the register set more complicated handling of its dynamic 
register set will be.
We have three options here:

1. Keep current state and use separate register info array in case of SVE 
(Simple solution)
2. We skip the GPRs out of SVE's register info array and declare all vector 
registers in SVE's register info array using appropriate contained and 
invalidate register lists.

Looking at this now I dont see this much of advantage just for the sake of 
pulling out GPR declaration from SVE register info array. What do you think?


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

https://reviews.llvm.org/D96458

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid requested changes to this revision.
omjavaid added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp:63
+  // should be removed once full PAC support is added.
+  return pc & 0x000F;
+}

How did you come up with this 36bit mask for PC reg I think this mask is not 
appropriate for Linux virtual address space which is 52 or 48 bits in length.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp:63
+  // should be removed once full PAC support is added.
+  return pc & 0x000F;
+}

omjavaid wrote:
> How did you come up with this 36bit mask for PC reg I think this mask is not 
> appropriate for Linux virtual address space which is 52 or 48 bits in length.
> 
> 
If it's any help I looked for this too and found 
`aarch64/functions/pac/calcbottompacbit/CalculateBottomPACBit` pseudocode in 
the armarm. This is used by the PAC strip instructions. (though it is a bit 
mind bending to read)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

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

[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+return error;

omjavaid wrote:
> DavidSpickett wrote:
> > omjavaid wrote:
> > > DavidSpickett wrote:
> > > > DavidSpickett wrote:
> > > > > DavidSpickett wrote:
> > > > > > omjavaid wrote:
> > > > > > > ptrace request is a success if number of tags requested is not 
> > > > > > > equal to no of tags read? If not then this and following 
> > > > > > > condition may be redundant.
> > > > > > Well ptracewrapper doesn't check the iovec, but I'll check the 
> > > > > > kernel source to see if it's actually possible for it to fail that 
> > > > > > way.
> > > > > In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there is a 
> > > > > comment:
> > > > > ```
> > > > > +/*
> > > > > + * Access MTE tags in another process' address space as given in mm. 
> > > > > Update
> > > > > + * the number of tags copied. Return 0 if any tags copied, error 
> > > > > otherwise.
> > > > > + * Inspired by __access_remote_vm().
> > > > > + */
> > > > > ```
> > > > > 
> > > > > *any tags* being the key words.
> > > > > 
> > > > > So the scenario is:
> > > > > * ask to read from addr X in page 0, with length of pagesize+some so 
> > > > > the range spills into page 1
> > > > > * kernel can access page 0, reads tags until the end of the page
> > > > > * tries to access page 1 to read the rest, fails, returns 0 (success) 
> > > > > since *some* tags were read
> > > > > * we see the ptrace call succeeded but with less tags than we expected
> > > > > 
> > > > > I don't see it's worth dealing with this corner case here since lldb 
> > > > > will look before it leaps. It would have errored much earlier here 
> > > > > because either page 1 isn't in the tracee's memory regions or it 
> > > > > wasn't MTE enabled.
> > > > > 
> > > > > 
> > > > Added a comment in the code too.
> > > This means emitting less than requested number of tags is legit. However 
> > > we have set tags vector size equal to whatever we have requested. We set 
> > > error string which is actually not being used anywhere and can be removed 
> > > in favor of a log message to help with debugging if needed.
> > > 
> > > Also we need to resize the vector after ptrace request so we use this 
> > > size in gdb remote reply.
> > I'll log that error in in GDBRemoteCommunicationServerLLGS.cpp.
> > 
> > I'll do what you suggested to support partial read on the server side. Then 
> > lldb client can error if it doesn't get what it expected.
> > (testing partial reads on the lldb side is going to be very difficult 
> > anyway since we'd need a valid smaps entry to even issue one)
> If we are following an approach similar to m/M gdb remote packets for tags 
> then its ok to read partial data in case a part memory in requested address 
> range was inaccessible. May be make appropriate adjustment for command output 
> I dont recall what currently emit out for partial memory reads but should be 
> something like 
> 
> 
I did some digging and lldb-server does not return partial data when a read 
fails. 
```
  for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
Status error = NativeProcessLinux::PtraceWrapper(
PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
if (error.Fail())
  return error;

remainder = size - bytes_read;
<...>
dst += k_ptrace_word_size;
  }
  return Status();
```

I was able to test partial writes too. There too we don't attempt to restore if 
we only wrote a smaller amount, writing less than the total is a failure.

However, it is true that I'm not handling the syscall properly. I need to loop 
like readmemory does. So I'm going to do that.
Loop until we've read all tags, or return the error we get.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601

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

[Lldb-commits] [lldb] 0df28ac - [LLDB] Skip TestExitDuringExpression on arm/linux buildbot

[Muhammad Omair Javaid via lldb-commits]

Author: Muhammad Omair Javaid
Date: 2021-03-15T16:03:06+05:00
New Revision: 0df28acffb5637ffb9abc908ef638f29389e0012

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

LOG: [LLDB] Skip TestExitDuringExpression on arm/linux buildbot

TestExitDuringExpression test_exit_before_one_thread_unwind fails
sporadically on arm/linux. This seems like a thread timing issue.
I am marking it skip for now.

Added: 


Modified: 

lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py

Removed: 




diff  --git 
a/lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py
 
b/lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py
index 4c365b99a244..4ee65c85e8f1 100644
--- 
a/lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py
+++ 
b/lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py
@@ -15,6 +15,7 @@ class TestExitDuringExpression(TestBase):
 NO_DEBUG_INFO_TESTCASE = True
 
 @skipIfWindows
+@skipIf(oslist=["linux"], archs=["arm"], bugnumber="llvm.org/pr48414")
 @expectedFailureAll(oslist=["freebsd"], bugnumber="llvm.org/pr48414")
 @expectedFailureNetBSD
 def test_exit_before_one_thread_unwind(self):



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

[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

Or scratch that. What you propose is cleaner and requires less changes to tests 
;-).


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

https://reviews.llvm.org/D98482

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

[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+return error;

DavidSpickett wrote:
> omjavaid wrote:
> > DavidSpickett wrote:
> > > omjavaid wrote:
> > > > DavidSpickett wrote:
> > > > > DavidSpickett wrote:
> > > > > > DavidSpickett wrote:
> > > > > > > omjavaid wrote:
> > > > > > > > ptrace request is a success if number of tags requested is not 
> > > > > > > > equal to no of tags read? If not then this and following 
> > > > > > > > condition may be redundant.
> > > > > > > Well ptracewrapper doesn't check the iovec, but I'll check the 
> > > > > > > kernel source to see if it's actually possible for it to fail 
> > > > > > > that way.
> > > > > > In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there is 
> > > > > > a comment:
> > > > > > ```
> > > > > > +/*
> > > > > > + * Access MTE tags in another process' address space as given in 
> > > > > > mm. Update
> > > > > > + * the number of tags copied. Return 0 if any tags copied, error 
> > > > > > otherwise.
> > > > > > + * Inspired by __access_remote_vm().
> > > > > > + */
> > > > > > ```
> > > > > > 
> > > > > > *any tags* being the key words.
> > > > > > 
> > > > > > So the scenario is:
> > > > > > * ask to read from addr X in page 0, with length of pagesize+some 
> > > > > > so the range spills into page 1
> > > > > > * kernel can access page 0, reads tags until the end of the page
> > > > > > * tries to access page 1 to read the rest, fails, returns 0 
> > > > > > (success) since *some* tags were read
> > > > > > * we see the ptrace call succeeded but with less tags than we 
> > > > > > expected
> > > > > > 
> > > > > > I don't see it's worth dealing with this corner case here since 
> > > > > > lldb will look before it leaps. It would have errored much earlier 
> > > > > > here because either page 1 isn't in the tracee's memory regions or 
> > > > > > it wasn't MTE enabled.
> > > > > > 
> > > > > > 
> > > > > Added a comment in the code too.
> > > > This means emitting less than requested number of tags is legit. 
> > > > However we have set tags vector size equal to whatever we have 
> > > > requested. We set error string which is actually not being used 
> > > > anywhere and can be removed in favor of a log message to help with 
> > > > debugging if needed.
> > > > 
> > > > Also we need to resize the vector after ptrace request so we use this 
> > > > size in gdb remote reply.
> > > I'll log that error in in GDBRemoteCommunicationServerLLGS.cpp.
> > > 
> > > I'll do what you suggested to support partial read on the server side. 
> > > Then lldb client can error if it doesn't get what it expected.
> > > (testing partial reads on the lldb side is going to be very difficult 
> > > anyway since we'd need a valid smaps entry to even issue one)
> > If we are following an approach similar to m/M gdb remote packets for tags 
> > then its ok to read partial data in case a part memory in requested address 
> > range was inaccessible. May be make appropriate adjustment for command 
> > output I dont recall what currently emit out for partial memory reads but 
> > should be something like 
> > 
> > 
> I did some digging and lldb-server does not return partial data when a read 
> fails. 
> ```
>   for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
> Status error = NativeProcessLinux::PtraceWrapper(
> PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
> if (error.Fail())
>   return error;
> 
> remainder = size - bytes_read;
> <...>
> dst += k_ptrace_word_size;
>   }
>   return Status();
> ```
> 
> I was able to test partial writes too. There too we don't attempt to restore 
> if we only wrote a smaller amount, writing less than the total is a failure.
> 
> However, it is true that I'm not handling the syscall properly. I need to 
> loop like readmemory does. So I'm going to do that.
> Loop until we've read all tags, or return the error we get.
Considering gdb remote protocol this is not complying with 'm' packet whick 
says:
"The reply may contain fewer addressable memory units than requested if the 
server was able to read only part of the region of memory."

May be we can fix this in a separate patch where LLDB should emit proper error 
code based on which either we can completely fail or send partial read data. 
What do you think ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601

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

[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+return error;

omjavaid wrote:
> DavidSpickett wrote:
> > omjavaid wrote:
> > > DavidSpickett wrote:
> > > > omjavaid wrote:
> > > > > DavidSpickett wrote:
> > > > > > DavidSpickett wrote:
> > > > > > > DavidSpickett wrote:
> > > > > > > > omjavaid wrote:
> > > > > > > > > ptrace request is a success if number of tags requested is 
> > > > > > > > > not equal to no of tags read? If not then this and following 
> > > > > > > > > condition may be redundant.
> > > > > > > > Well ptracewrapper doesn't check the iovec, but I'll check the 
> > > > > > > > kernel source to see if it's actually possible for it to fail 
> > > > > > > > that way.
> > > > > > > In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there 
> > > > > > > is a comment:
> > > > > > > ```
> > > > > > > +/*
> > > > > > > + * Access MTE tags in another process' address space as given in 
> > > > > > > mm. Update
> > > > > > > + * the number of tags copied. Return 0 if any tags copied, error 
> > > > > > > otherwise.
> > > > > > > + * Inspired by __access_remote_vm().
> > > > > > > + */
> > > > > > > ```
> > > > > > > 
> > > > > > > *any tags* being the key words.
> > > > > > > 
> > > > > > > So the scenario is:
> > > > > > > * ask to read from addr X in page 0, with length of pagesize+some 
> > > > > > > so the range spills into page 1
> > > > > > > * kernel can access page 0, reads tags until the end of the page
> > > > > > > * tries to access page 1 to read the rest, fails, returns 0 
> > > > > > > (success) since *some* tags were read
> > > > > > > * we see the ptrace call succeeded but with less tags than we 
> > > > > > > expected
> > > > > > > 
> > > > > > > I don't see it's worth dealing with this corner case here since 
> > > > > > > lldb will look before it leaps. It would have errored much 
> > > > > > > earlier here because either page 1 isn't in the tracee's memory 
> > > > > > > regions or it wasn't MTE enabled.
> > > > > > > 
> > > > > > > 
> > > > > > Added a comment in the code too.
> > > > > This means emitting less than requested number of tags is legit. 
> > > > > However we have set tags vector size equal to whatever we have 
> > > > > requested. We set error string which is actually not being used 
> > > > > anywhere and can be removed in favor of a log message to help with 
> > > > > debugging if needed.
> > > > > 
> > > > > Also we need to resize the vector after ptrace request so we use this 
> > > > > size in gdb remote reply.
> > > > I'll log that error in in GDBRemoteCommunicationServerLLGS.cpp.
> > > > 
> > > > I'll do what you suggested to support partial read on the server side. 
> > > > Then lldb client can error if it doesn't get what it expected.
> > > > (testing partial reads on the lldb side is going to be very difficult 
> > > > anyway since we'd need a valid smaps entry to even issue one)
> > > If we are following an approach similar to m/M gdb remote packets for 
> > > tags then its ok to read partial data in case a part memory in requested 
> > > address range was inaccessible. May be make appropriate adjustment for 
> > > command output I dont recall what currently emit out for partial memory 
> > > reads but should be something like 
> > > 
> > > 
> > I did some digging and lldb-server does not return partial data when a read 
> > fails. 
> > ```
> >   for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
> > Status error = NativeProcessLinux::PtraceWrapper(
> > PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
> > if (error.Fail())
> >   return error;
> > 
> > remainder = size - bytes_read;
> > <...>
> > dst += k_ptrace_word_size;
> >   }
> >   return Status();
> > ```
> > 
> > I was able to test partial writes too. There too we don't attempt to 
> > restore if we only wrote a smaller amount, writing less than the total is a 
> > failure.
> > 
> > However, it is true that I'm not handling the syscall properly. I need to 
> > loop like readmemory does. So I'm going to do that.
> > Loop until we've read all tags, or return the error we get.
> Considering gdb remote protocol this is not complying with 'm' packet whick 
> says:
> "The reply may contain fewer addressable memory units than requested if the 
> server was able to read only part of the region of memory."
> 
> May be we can fix this in a separate patch where LLDB should emit proper 
> error code based on which either we can completely fail or send partial read 
> data. 
> What do you think ?
That would be the ideal thing to do, however I was wrong about lldb not 
supporting it at all. In fact memory read can do partial results:
```

(lldb) memory read 0xf7ff7000-16
lldb <  34> send packet: $qMemoryRegionInfo:f7ff9010#df
lldb <  55> read packet: 
$start:f7ff9000;size:1000;perm

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[David Zarzycki via Phabricator via lldb-commits]

davezarzycki added a comment.

Might somebody be willing to sign off on this change (this week or next)? I'd 
like to cherry-pick it to Swift's LLVM branch. Thanks for all the feedback so 
far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Justin Cohen via Phabricator via lldb-commits]

justincohen updated this revision to Diff 330665.
justincohen added a comment.

Move logic to ABIMacOSX_arm64.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

Files:
  lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
  lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h


Index: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
@@ -62,6 +62,12 @@
 return true;
   }
 
+  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override {
+// Short term workaround to remove any pointer authentication codes. This
+// should be removed once full PAC support is added.
+return pc & 0x000F;
+  }
+
   // Static Functions
 
   static void Initialize();
Index: lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
@@ -24,8 +24,6 @@
 
   uint32_t GetGenericNum(llvm::StringRef name) override;
 
-  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override;
-
   using lldb_private::MCBasedABI::MCBasedABI;
 };
 #endif
Index: lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
===
--- lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
+++ lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
@@ -56,9 +56,3 @@
   .Case("x7", LLDB_REGNUM_GENERIC_ARG8)
   .Default(LLDB_INVALID_REGNUM);
 }
-
-lldb::addr_t ABIAArch64::FixCodeAddress(lldb::addr_t pc) {
-  // Short term workaround to remove any pointer authentication codes. This
-  // should be removed once full PAC support is added.
-  return pc & 0x000F;
-}


Index: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
@@ -62,6 +62,12 @@
 return true;
   }
 
+  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override {
+// Short term workaround to remove any pointer authentication codes. This
+// should be removed once full PAC support is added.
+return pc & 0x000F;
+  }
+
   // Static Functions
 
   static void Initialize();
Index: lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIAArch64.h
@@ -24,8 +24,6 @@
 
   uint32_t GetGenericNum(llvm::StringRef name) override;
 
-  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override;
-
   using lldb_private::MCBasedABI::MCBasedABI;
 };
 #endif
Index: lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
===
--- lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
+++ lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp
@@ -56,9 +56,3 @@
   .Case("x7", LLDB_REGNUM_GENERIC_ARG8)
   .Default(LLDB_INVALID_REGNUM);
 }
-
-lldb::addr_t ABIAArch64::FixCodeAddress(lldb::addr_t pc) {
-  // Short term workaround to remove any pointer authentication codes. This
-  // should be removed once full PAC support is added.
-  return pc & 0x000F;
-}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Justin Cohen via Phabricator via lldb-commits]

justincohen added inline comments.



Comment at: lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp:63
+  // should be removed once full PAC support is added.
+  return pc & 0x000F;
+}

DavidSpickett wrote:
> omjavaid wrote:
> > How did you come up with this 36bit mask for PC reg I think this mask is 
> > not appropriate for Linux virtual address space which is 52 or 48 bits in 
> > length.
> > 
> > 
> If it's any help I looked for this too and found 
> `aarch64/functions/pac/calcbottompacbit/CalculateBottomPACBit` pseudocode in 
> the armarm. This is used by the PAC strip instructions. (though it is a bit 
> mind bending to read)
I'll move this to `lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h` to not 
impact Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

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

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

[Justin Cohen via Phabricator via lldb-commits]

justincohen updated this revision to Diff 330667.
justincohen added a comment.

squash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98529

Files:
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h


Index: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
@@ -62,6 +62,12 @@
 return true;
   }
 
+  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override {
+// Short term workaround to remove any pointer authentication codes. This
+// should be removed once full PAC support is added.
+return pc & 0x000F;
+  }
+
   // Static Functions
 
   static void Initialize();


Index: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
===
--- lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
+++ lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h
@@ -62,6 +62,12 @@
 return true;
   }
 
+  lldb::addr_t FixCodeAddress(lldb::addr_t pc) override {
+// Short term workaround to remove any pointer authentication codes. This
+// should be removed once full PAC support is added.
+return pc & 0x000F;
+  }
+
   // Static Functions
 
   static void Initialize();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 330671.
mgorny edited the summary of this revision.
mgorny added a comment.

Updated `GetPidTid()` to use `llvm::Optional>` return type with a default PID approach as suggested by @labath.

Updated `ReadTid()` to use `llvm::Expected`. Create a new 
`GDBRemoteError` class to pass gdb-remote `$E` codes through cleanly.


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

https://reviews.llvm.org/D98482

Files:
  lldb/include/lldb/Utility/GDBRemoteError.h
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/GDBRemoteError.cpp
  lldb/source/Utility/Status.cpp
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

Index: lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp
@@ -0,0 +1,167 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+#include "lldb/Utility/StringExtractorGDBRemote.h"
+#include "lldb/lldb-defines.h"
+
+namespace {
+class StringExtractorGDBRemoteTest : public ::testing::Test {};
+} // namespace
+
+TEST_F(StringExtractorGDBRemoteTest, GetPidTid) {
+  StringExtractorGDBRemote ex("");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // invalid/short values
+
+  ex.Reset("narf");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset(";1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset(".1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("pnarf");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p;1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p.1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.;1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p-2.1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // overflow
+
+  ex.Reset("p1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1.0");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p0.1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1.1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // pure thread id
+
+  ex.Reset("0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0));
+
+  ex.Reset("-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0x1234ULL));
+
+  ex.Reset("123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0x123456789ABCDEF0ULL));
+
+  // pure process id
+
+  ex.Reset("p0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(LLDB_INVALID_PROCESS_ID, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0x1234ULL, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0x123456789ABCDEF0ULL, LLDB_INVALID_THREAD_ID));
+
+  // combined thread id + process id
+
+  ex.Reset("p0.0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, 0));
+
+  ex.Reset("p0.-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, LLDB_INVALID_THREAD_ID));
+
+  // NB: technically, specific thread with unspecified process is invalid
+  // but we do not filter that in StringExtractor
+
+  ex.Reset("p0.1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, 0x1234ULL));
+
+  ex.Reset("p0.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p-1.0");
+  EXPECT_THAT(ex.GetPi

[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 330674.
mgorny added a comment.

clang-format.


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

https://reviews.llvm.org/D98482

Files:
  lldb/include/lldb/Utility/GDBRemoteError.h
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/GDBRemoteError.cpp
  lldb/source/Utility/Status.cpp
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

Index: lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp
@@ -0,0 +1,175 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+#include "lldb/Utility/StringExtractorGDBRemote.h"
+#include "lldb/lldb-defines.h"
+
+namespace {
+class StringExtractorGDBRemoteTest : public ::testing::Test {};
+} // namespace
+
+TEST_F(StringExtractorGDBRemoteTest, GetPidTid) {
+  StringExtractorGDBRemote ex("");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // invalid/short values
+
+  ex.Reset("narf");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset(";1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset(".1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("pnarf");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p;1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p.1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.;1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1234.-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p-2");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p-2.1234");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // overflow
+
+  ex.Reset("p1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1.0");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p0.1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  ex.Reset("p1.1");
+  EXPECT_EQ(ex.GetPidTid(0), llvm::None);
+
+  // pure thread id
+
+  ex.Reset("0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0));
+
+  ex.Reset("-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(100, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0x1234ULL));
+
+  ex.Reset("123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(100, 0x123456789ABCDEF0ULL));
+
+  // pure process id
+
+  ex.Reset("p0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(LLDB_INVALID_PROCESS_ID, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0x1234ULL, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0x123456789ABCDEF0ULL, LLDB_INVALID_THREAD_ID));
+
+  // combined thread id + process id
+
+  ex.Reset("p0.0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, 0));
+
+  ex.Reset("p0.-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0, LLDB_INVALID_THREAD_ID));
+
+  // NB: technically, specific thread with unspecified process is invalid
+  // but we do not filter that in StringExtractor
+
+  ex.Reset("p0.1234");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(0, 0x1234ULL));
+
+  ex.Reset("p0.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(0, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p-1.0");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pair(LLDB_INVALID_PROCESS_ID, 0));
+
+  ex.Reset("p-1.-1");
+  EXPECT_THAT(ex.GetPidTid(100).getValue(),
+  ::testing::Pai

[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 330684.
DavidSpickett added a comment.

Correctly handle the ptrace call by looping until we
get all tags as an error.

I've gone ahead and treated anything but all tags
as an error as far as lldb-server is concerned.

Tell me what you think of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/include/lldb/Host/linux/Ptrace.h
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/source/Host/common/NativeProcessProtocol.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/memory-tagging/Makefile
  lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
  lldb/test/API/tools/lldb-server/memory-tagging/main.c

Index: lldb/test/API/tools/lldb-server/memory-tagging/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-tagging/main.c
@@ -0,0 +1,55 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+int print_result(char *ptr) {
+  // Page size allows the test to try reading off of the end of the page
+  printf("buffer: %p page_size: 0x%x\n", ptr, sysconf(_SC_PAGESIZE));
+
+  // Exit after some time, so we don't leave a zombie process
+  // if the test framework lost track of us.
+  sleep(60);
+  return 0;
+}
+
+int main(int argc, char const *argv[]) {
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL,
+PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC |
+// Allow all tags to be generated by the addg
+// instruction __arm_mte_increment_tag produces.
+(0x << PR_MTE_TAG_SHIFT),
+0, 0, 0)) {
+return print_result(NULL);
+  }
+
+  size_t page_size = sysconf(_SC_PAGESIZE);
+  char *buf = mmap(0, page_size, PROT_READ | PROT_WRITE | PROT_MTE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return print_result(NULL);
+
+  // Set incrementing tags until end of the page
+  char *tagged_ptr = buf;
+  // This intrinsic treats the addresses as if they were untagged
+  while (__arm_mte_ptrdiff(tagged_ptr, buf) < page_size) {
+// This sets the allocation tag
+__arm_mte_set_tag(tagged_ptr);
+// Set the tag of the next granule (hence +16) to the next
+// tag value. Returns a new pointer with the new logical tag.
+// Tag values wrap at 0xF so it'll cycle.
+tagged_ptr = __arm_mte_increment_tag(tagged_ptr + 16, 1);
+  }
+
+  // lldb-server should be removing the top byte from addresses passed
+  // to ptrace. So put some random bits in there.
+  // ptrace expects you to remove them but it can still succeed if you
+  // don't. So this isn't proof that we're removing them, it's just a
+  // smoke test in case something didn't account for them.
+  buf = (char *)((size_t)buf | ((size_t)0xAA << 56));
+  return print_result(buf);
+}
Index: lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
@@ -0,0 +1,116 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteMemoryTagging(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def check_qmemtags_response(self, body, expected):
+self.test_sequence.add_log_lines(["read packet: $qMemTags:{}#00".format(body),
+  "send packet: ${}#00".format(expected),
+  ],
+ True)
+self.expect_gdbremote_sequence()
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_qmemtags_packets(self):
+""" Test that qMemTags packets are parsed correctly and/or rejected. """
+
+self.build()
+self.set_inferior_startup_launch()
+procs = self.prep_debug_monitor_and_inferior()
+
+# Run the process
+self.test_sequence.add_log_lines(
+[
+# Start running after initial stop
+

[Lldb-commits] [PATCH] D95602: [lldb][AArch64] Add MTE memory tag reading to lldb

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 330685.
DavidSpickett added a comment.

Update after changing parent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95602

Files:
  lldb/include/lldb/Core/Architecture.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp
  lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h
  lldb/source/Plugins/Architecture/AArch64/CMakeLists.txt
  lldb/source/Plugins/Architecture/CMakeLists.txt
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -657,3 +657,68 @@
   EXPECT_EQ(llvm::None, GetQOffsets("TextSeg=0x1234"));
   EXPECT_EQ(llvm::None, GetQOffsets("TextSeg=12345678123456789"));
 }
+
+static void
+check_qmemtags(TestClient &client, MockServer &server, size_t read_len,
+   const char *packet, llvm::StringRef response,
+   llvm::Optional> expected_tag_data) {
+  const auto &ReadMemoryTags = [&](size_t len, const char *packet,
+   llvm::StringRef response) {
+std::future result = std::async(std::launch::async, [&] {
+  return client.ReadMemoryTags(0xDEF0, read_len, 1);
+});
+
+HandlePacket(server, packet, response);
+return result.get();
+  };
+
+  auto result = ReadMemoryTags(0, packet, response);
+  if (expected_tag_data) {
+ASSERT_TRUE(result);
+llvm::ArrayRef expected(*expected_tag_data);
+llvm::ArrayRef got = result->GetData();
+ASSERT_THAT(expected, testing::ContainerEq(got));
+  } else {
+ASSERT_FALSE(result);
+  }
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, ReadMemoryTags) {
+  // Zero length reads are valid
+  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+ std::vector{});
+
+  // The client layer does not check the length of the received data.
+  // All we need is the "m" and for the decode to use all of the chars
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09",
+ std::vector{0x9});
+
+  // Zero length response is fine as long as the "m" is present
+  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+ std::vector{});
+
+  // Normal responses
+  check_qmemtags(client, server, 16, "qMemTags:def0,10:1", "m66",
+ std::vector{0x66});
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m0102",
+ std::vector{0x1, 0x2});
+
+  // Empty response is an error
+  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "", llvm::None);
+  // Usual error response
+  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "E01", llvm::None);
+  // Leading m missing
+  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "01", llvm::None);
+  // Anything other than m is an error
+  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "z01", llvm::None);
+  // Decoding tag data doesn't use all the chars in the packet
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09zz", llvm::None);
+  // Data that is not hex bytes
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "mhello",
+ llvm::None);
+  // Data is not a complete hex char
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m9", llvm::None);
+  // Data has a trailing hex char
+  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m01020",
+ llvm::None);
+}
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -6019,3 +6019,71 @@
 
   return false;
 }
+
+llvm::Expected
+Process::GetMemoryTagManager(lldb::addr_t addr, lldb::addr_t end_addr) {
+  Architecture *arch = GetTarget().GetArchitecturePlugin();
+  const MemoryTagManager *tag_manager =
+  arch ? arch->GetMemoryTagManager() : nullptr;
+  if (!arch || !tag_manager) {
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"This architecture does not support memory tagging",
+GetPluginName().GetCString());
+  }
+
+  if (!SupportsMemoryTagging()) {
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "Process does not support memory tagging");
+  }
+
+  ptrdiff_t len = tag_manager->AddressDiff(end_a

[Lldb-commits] [PATCH] D97285: [lldb][AArch64] Add "memory tag read" command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 330686.
DavidSpickett added a comment.

- Update after changing earlier revisions
- Read page size only once in the test file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97285

Files:
  lldb/source/Commands/CMakeLists.txt
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Commands/CommandObjectMemoryTag.h
  lldb/test/API/functionalities/memory/tag/Makefile
  lldb/test/API/functionalities/memory/tag/TestMemoryTag.py
  lldb/test/API/functionalities/memory/tag/main.cpp
  lldb/test/API/linux/aarch64/mte_tag_read/Makefile
  lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
  lldb/test/API/linux/aarch64/mte_tag_read/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_read/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_read/main.c
@@ -0,0 +1,60 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL,
+PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC |
+// Allow all tags to be generated by the addg
+// instruction __arm_mte_increment_tag produces.
+(0x << PR_MTE_TAG_SHIFT),
+0, 0, 0)) {
+return 1;
+  }
+
+  size_t page_size = sysconf(_SC_PAGESIZE);
+
+  // Allocate memory with MTE
+  char *buf = mmap(0, page_size, PROT_READ | PROT_WRITE | PROT_MTE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // And without MTE
+  char *non_mte_buf = mmap(0, page_size, PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (non_mte_buf == MAP_FAILED)
+return 1;
+
+  // Set incrementing tags until end of the page
+  char *tagged_ptr = buf;
+  // This ignores tag bits when subtracting the addresses
+  while (__arm_mte_ptrdiff(tagged_ptr, buf) < page_size) {
+// Set the allocation tag for this location
+__arm_mte_set_tag(tagged_ptr);
+// + 16 for 16 byte granules
+// Earlier we allowed all tag values, so this will give us an
+// incrementing pattern 0-0xF wrapping back to 0.
+tagged_ptr = __arm_mte_increment_tag(tagged_ptr + 16, 1);
+  }
+
+  // Tag the original pointer with 9
+  buf = __arm_mte_create_random_tag(buf, ~(1 << 9));
+  // A different tag so that buf_alt_tag > buf if you don't handle the tag
+  char *buf_alt_tag = __arm_mte_create_random_tag(buf, ~(1 << 10));
+
+  // lldb should be removing the whole top byte, not just the tags.
+  // So fill 63-60 with something non zero so we'll fail if we only remove tags.
+  size_t nibble = 0xA;
+  buf = (char *)((size_t)buf | (nibble << 60));
+  buf_alt_tag = (char *)((size_t)buf_alt_tag | (nibble << 60));
+
+  // Breakpoint here
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
@@ -0,0 +1,116 @@
+"""
+Test "memory tag read" command on AArch64 Linux with MTE.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagReadTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_read(self):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Argument validation
+self.expect("memory tag read",
+substrs=["error: wrong number of arguments; expected at least , "
+ "at most  "],
+error=True)
+self.expect("memory tag read buf buf+16 32",
+substrs=["error: wrong number of arguments; expected at least , "
+ "at most  "],
+error=True)
+self.expect("memory tag read not_a_sy

[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: 
lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py:102
+# Here we read from 1/2 way through a granule, into the next. Expands 
to 2 granules
+self.check_qmemtags_response("{:x},10:1".format(buf_address+64-8), 
"m0304")

DavidSpickett wrote:
> omjavaid wrote:
> > Do we test partial read case here? 
> Ack. No, but it should be a case of reading off of the end of the allocated 
> buffer by some amount.
This has been added at the end of the tests here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601

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

[Lldb-commits] [PATCH] D97285: [lldb][AArch64] Add "memory tag read" command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I still have one known (though rare) limitation to this command. That is, if 
you try to read across two neighbouring MTE regions it will fail because it 
thinks that the whole range isn't tagged. When it is, it's just split over two 
regions.

This would require you to manage to mmap them next to each other. So I'm 
looking for a way to do deliberately so it could be tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97285

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

[Lldb-commits] [PATCH] D98653: [lldb] Refactor to support non-pointer instance variables (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: aprantl, teemperor, jingham, JDevlieghere, labath.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

`frame variable` contextually supports accessing ivars via a language's implicit
instance variable, ex `this` in C++ or `self` in ObjC.

It has been assumed that the instance variable is a pointer, resulting in 
`this->` or
`self->` prefixes. However some languages have a reference based instance 
variable
instead of a pointer. An example of this is Swift.

This changes `DeclContextIsClassMethod` and a few of its callers to return (via 
an out
pointer) whether the instance variable is a pointer or reference.

Some cleanup included:

1. The `language` parameter wasn't used and has been removed
2. The `is_instance_method` parameter is redundant and has been removed -- a 
non-empty `instance_var_name` indicates the context is an instance method
3. `IsClassMethod`'s parameters have been declared with default values of 
`nullptr`


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98653

Files:
  lldb/include/lldb/Symbol/CompilerDeclContext.h
  lldb/include/lldb/Symbol/SymbolContext.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerDeclContext.cpp
  lldb/source/Symbol/SymbolContext.cpp
  lldb/source/Target/StackFrame.cpp

Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -553,16 +553,17 @@
 // Check for direct ivars access which helps us with implicit access to
 // ivars with the "this->" or "self->"
 GetSymbolContext(eSymbolContextFunction | eSymbolContextBlock);
-lldb::LanguageType method_language = eLanguageTypeUnknown;
-bool is_instance_method = false;
-ConstString method_object_name;
-if (m_sc.GetFunctionMethodInfo(method_language, is_instance_method,
-   method_object_name)) {
-  if (is_instance_method && method_object_name) {
-var_sp = variable_list->FindVariable(method_object_name);
+ConstString instance_var_name;
+bool instance_is_pointer = true;
+if (m_sc.GetFunctionMethodInfo(&instance_var_name, &instance_is_pointer)) {
+  if (instance_var_name) {
+var_sp = variable_list->FindVariable(instance_var_name);
 if (var_sp) {
   separator_idx = 0;
-  var_expr_storage = "->";
+  if (instance_is_pointer)
+var_expr_storage = "->";
+  else
+var_expr_storage = ".";
   var_expr_storage += var_expr;
   var_expr = var_expr_storage;
   synthetically_added_instance_object = true;
Index: lldb/source/Symbol/SymbolContext.cpp
===
--- lldb/source/Symbol/SymbolContext.cpp
+++ lldb/source/Symbol/SymbolContext.cpp
@@ -545,17 +545,13 @@
   return nullptr;
 }
 
-bool SymbolContext::GetFunctionMethodInfo(lldb::LanguageType &language,
-  bool &is_instance_method,
-  ConstString &language_object_name)
-
-{
+bool SymbolContext::GetFunctionMethodInfo(ConstString *instance_var_name,
+  bool *instance_is_pointer) {
   Block *function_block = GetFunctionBlock();
   if (function_block) {
 CompilerDeclContext decl_ctx = function_block->GetDeclContext();
 if (decl_ctx)
-  return decl_ctx.IsClassMethod(&language, &is_instance_method,
-&language_object_name);
+  return decl_ctx.IsClassMethod(instance_var_name, instance_is_pointer);
   }
   return false;
 }
Index: lldb/source/Symbol/CompilerDeclContext.cpp
===
--- lldb/source/Symbol/CompilerDeclContext.cpp
+++ lldb/source/Symbol/CompilerDeclContext.cpp
@@ -34,13 +34,11 @@
   return ConstString();
 }
 
-bool CompilerDeclContext::IsClassMethod(lldb::LanguageType *language_ptr,
-bool *is_instance_method_ptr,
-ConstString *language_object_name_ptr) {
+bool CompilerDeclContext::IsClassMethod(ConstString *instance_var_name_ptr,
+bool *instance_is_pointer_ptr) {
   if (IsValid())
 return m_type_system->DeclContextIsClassMethod(
-m_opaque_decl_ctx, language_ptr, is_instance_method_ptr,
-language_object_name_ptr);
+m_opaque_decl_ctx, instance_var_name_ptr, instance_is_pointer_ptr);
   return false;
 }
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
==

[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Right, I see. The contained lists are going to make this trickier than I 
expected, and might even make the code more complicated. (Though this tradeoff 
will change if we ever end up with two optional regsets that need to mess with 
contained lists.)

Could we at least then structure the constructor code to avoid checking for SVE 
twice. Ideally, we would make this a two-step process:

1. select the "base" register list (either gpr+fpr, or gpr+fpr+sve)
2. add optional sets

Something like:

  if (regsets & SVE) {
m_register_set_p = reginfos_with_sve;
// and similar for other variables
  } else {
m_register_set_p = reginfos_without_sve;
  }
  if (regsets & Dynamic) {
m_dynamic_reg_infos.assign(m_register_set_p, m_register_set_count)
if (regsets & pauth)
  m_dynamic_reg_infos.insert(end(), g_pauth_registers); // or whatever

m_register_set_p = m_dynamic_reg_infos.data();
// etc.
  }


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

https://reviews.llvm.org/D96458

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

[Lldb-commits] [PATCH] D98653: [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/include/lldb/Symbol/CompilerDeclContext.h:77
   /// in a struct, union or class.
-  bool IsClassMethod(lldb::LanguageType *language_ptr,
- bool *is_instance_method_ptr,
- ConstString *language_object_name_ptr);
+  bool IsClassMethod(ConstString *instance_var_name_ptr = nullptr,
+ bool *instance_is_pointer_ptr = nullptr);

If we are going to refactor this, this change does not feel very C++y passing 
around pointers. I know we want a way to call this w/o any arguments but 
perhaps we can write an overload for that case?

Does `instance_var_name_ptr` need to be a string? Maybe we can encode it using 
an enum, we don't have a lot of cases `this`, `self`, maybe even not a pointer 
as well and get ride of `instance_is_pointer_ptr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98653

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

[Lldb-commits] [PATCH] D98653: [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/include/lldb/Symbol/CompilerDeclContext.h:77
   /// in a struct, union or class.
-  bool IsClassMethod(lldb::LanguageType *language_ptr,
- bool *is_instance_method_ptr,
- ConstString *language_object_name_ptr);
+  bool IsClassMethod(ConstString *instance_var_name_ptr = nullptr,
+ bool *instance_is_pointer_ptr = nullptr);

shafik wrote:
> If we are going to refactor this, this change does not feel very C++y passing 
> around pointers. I know we want a way to call this w/o any arguments but 
> perhaps we can write an overload for that case?
> 
> Does `instance_var_name_ptr` need to be a string? Maybe we can encode it 
> using an enum, we don't have a lot of cases `this`, `self`, maybe even not a 
> pointer as well and get ride of `instance_is_pointer_ptr`.
Something like?

```
enum InstanceVariable {
   ThisPointer,
   SelfPointer,
   Self,
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98653

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

[Lldb-commits] [lldb] 34885bf - [lldb-vscode] Handle request_evaluate's context attribute

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-03-15T15:09:23-07:00
New Revision: 34885bffdf43920c0f011e17a65fd678100240dd

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

LOG: [lldb-vscode] Handle request_evaluate's context attribute

Summary:
The request "evaluate" supports a "context" attribute, which is sent by VSCode. 
The attribute is defined here 
https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Evaluate

The "clipboard" context is not yet supported by lldb-vscode, so we can forget 
about it for now. The 'repl' (i.e. Debug Console) and 'watch' (i.e. Watch 
Expression) contexts must use the expression parser in case the frame's 
variable path is not enough, as the user expects these expressions to never 
fail. On the other hand, the 'hover' expression is invoked whenever the user 
hovers on any keyword on the UI and the user is fine with the expression not 
being fully resolved, as they know that the 'repl' case is the fallback they 
can rely on.

Given that the 'hover' expression is invoked many many times without the user 
noticing it due to it being triggered by the mouse, I'm making it use only the 
frame's variable path functionality and not the expression parser. This should 
speed up tremendously the responsiveness of a debug session when the user only 
sets source breakpoints and inspect local variables, as the entire debug info 
is not needed to be parsed.

Regarding tests, I've tried to be as comprehensive as possible considering a 
multi-file project. Fortunately, the results from the "hover" case are enough 
most of the times.

Differential Revision: https://reviews.llvm.org/D98656

Added: 
lldb/test/API/tools/lldb-vscode/evaluate/Makefile
lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
lldb/test/API/tools/lldb-vscode/evaluate/foo.cpp
lldb/test/API/tools/lldb-vscode/evaluate/foo.h
lldb/test/API/tools/lldb-vscode/evaluate/main.cpp

Modified: 
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
lldb/tools/lldb-vscode/lldb-vscode.cpp

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
index 1582115ee40b..5a433f2baac2 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -139,7 +139,7 @@ def get_modules(self):
 for module in module_list:
 modules[module['name']] = module
 return modules
-
+
 def get_output(self, category, timeout=0.0, clear=True):
 self.output_condition.acquire()
 output = None
@@ -304,7 +304,7 @@ def send_recv(self, command):
 return response_or_request
 else:
 if response_or_request['command'] == 'runInTerminal':
-subprocess.Popen(response_or_request['arguments']['args'], 
+subprocess.Popen(response_or_request['arguments']['args'],
 env=response_or_request['arguments']['env'])
 self.send_packet({
 "type": "response",
@@ -317,7 +317,7 @@ def send_recv(self, command):
 else:
 desc = 'unkonwn reverse request "%s"' % 
(response_or_request['command'])
 raise ValueError(desc)
-
+
 return None
 
 def wait_for_event(self, filter=None, timeout=None):
@@ -567,13 +567,14 @@ def request_disconnect(self, terminateDebuggee=None):
 }
 return self.send_recv(command_dict)
 
-def request_evaluate(self, expression, frameIndex=0, threadId=None):
+def request_evaluate(self, expression, frameIndex=0, threadId=None, 
context=None):
 stackFrame = self.get_stackFrame(frameIndex=frameIndex,
  threadId=threadId)
 if stackFrame is None:
 return []
 args_dict = {
 'expression': expression,
+'context': context,
 'frameId': stackFrame['id'],
 }
 command_dict = {
@@ -787,7 +788,7 @@ def request_compileUnits(self, moduleId):
 }
 response = self.send_recv(command_dict)
 return response
-
+
 def request_completions(self, text):
 args_dict = {
 'text': text,

diff  --git a/lldb/test/API/tools/lldb-vscode/evaluate/Makefile 
b/lldb/test/API/tools/lldb-vscode/evaluate/Makefile
new file mode 100644
index ..7df22699c57d
--- /dev/null
+++ b/lldb/test/API/tools/lldb-vscode/evaluate/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp foo.cpp
+
+include Makefile.rules

diff  --git a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCo

[Lldb-commits] [PATCH] D98656: [lldb-vscode] Handle request_evaluate's context attribute

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG34885bffdf43: [lldb-vscode] Handle request_evaluate's 
context attribute (authored by Walter Erquinigo ).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98656

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/evaluate/Makefile
  lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
  lldb/test/API/tools/lldb-vscode/evaluate/foo.cpp
  lldb/test/API/tools/lldb-vscode/evaluate/foo.h
  lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1125,6 +1125,7 @@
   auto arguments = request.getObject("arguments");
   lldb::SBFrame frame = g_vsc.GetLLDBFrame(*arguments);
   const auto expression = GetString(arguments, "expression");
+  llvm::StringRef context = GetString(arguments, "context");
 
   if (!expression.empty() && expression[0] == '`') {
 auto result =
@@ -1133,13 +1134,17 @@
 body.try_emplace("variablesReference", (int64_t)0);
   } else {
 // Always try to get the answer from the local variables if possible. If
-// this fails, then actually evaluate an expression using the expression
-// parser. "frame variable" is more reliable than the expression parser in
+// this fails, then if the context is not "hover", actually evaluate an
+// expression using the expression parser.
+//
+// "frame variable" is more reliable than the expression parser in
 // many cases and it is faster.
 lldb::SBValue value = frame.GetValueForVariablePath(
 expression.data(), lldb::eDynamicDontRunTarget);
-if (value.GetError().Fail())
+
+if (value.GetError().Fail() && context != "hover")
   value = frame.EvaluateExpression(expression.data());
+
 if (value.GetError().Fail()) {
   response["success"] = llvm::json::Value(false);
   // This error object must live until we're done with the pointer returned
Index: lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
@@ -0,0 +1,29 @@
+#include "foo.h"
+
+static int static_int = 42;
+
+int non_static_int = 43;
+
+int a_function(int var) {
+  return var; // breakpoint 3
+}
+
+struct my_struct {
+  int foo;
+};
+
+int main(int argc, char const *argv[]) {
+  my_struct struct1 = {15};
+  my_struct *struct2 = new my_struct{16};
+  int var1 = 20;
+  int var2 = 21;
+  int var3 = static_int; // breakpoint 1
+  {
+int non_static_int = 10;
+int var2 = 2;
+int var3 = non_static_int; // breakpoint 2
+  }
+  a_function(var3);
+  foo_func();
+  return 0;
+}
Index: lldb/test/API/tools/lldb-vscode/evaluate/foo.h
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/evaluate/foo.h
@@ -0,0 +1,3 @@
+int foo_func();
+
+extern int foo_var;
Index: lldb/test/API/tools/lldb-vscode/evaluate/foo.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/evaluate/foo.cpp
@@ -0,0 +1,5 @@
+#include "foo.h"
+
+int foo_func() { return 43; }
+
+int foo_var = 44;
Index: lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
@@ -0,0 +1,157 @@
+"""
+Test lldb-vscode completions request
+"""
+
+
+import lldbvscode_testcase
+import unittest2
+import vscode
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def assertEvaluate(self, expression, regex):
+self.assertRegexpMatches(
+self.vscode.request_evaluate(expression, context=self.context)['body']['result'],
+regex)
+
+def assertEvaluateFailure(self, expression):
+self.assertNotIn('result',
+self.vscode.request_evaluate(expression, context=self.context)['body'])
+
+def isExpressionParsedExpected(self):
+return self.context != "hover"
+
+def run_test_evaluate_expressions(self, context=None):
+"""
+Tests the evaluate expression request at different breakpoints
+"""
+self.context = context
+program = self.getBuildArtifact("a.out")
+self.build_and_launch(p

[Lldb-commits] [lldb] b5657d1 - Fix 34885bffdf43920c0f011e17a65fd678100240dd

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-03-15T16:36:32-07:00
New Revision: b5657d1fbf77fc59d4c11476464547d850d9f48a

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

LOG: Fix 34885bffdf43920c0f011e17a65fd678100240dd

It failed https://lab.llvm.org/buildbot/#/builders/17/builds/5262 and
the fix is simply to relax a regex expression in a test.

Added: 


Modified: 
lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py

Removed: 




diff  --git a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py 
b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
index f496073d1b6d..c0ccd2ef43be 100644
--- a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
+++ b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
@@ -61,12 +61,12 @@ def run_test_evaluate_expressions(self, context=None):
 if self.isExpressionParsedExpected():
 self.assertEvaluate(
 "a_function",
-"0x.* \(a.out`a_function\(int\) at main.cpp:7\)")
+"0x.*a.out`a_function.*")
 self.assertEvaluate("a_function(1)", "1")
 self.assertEvaluate("var2 + struct1.foo", "36")
 self.assertEvaluate(
 "foo_func",
-"0x.* \(a.out`foo_func\(\) at foo.cpp:3\)")
+"0x.*a.out`foo_func.*")
 self.assertEvaluate("foo_var", "44")
 else:
 self.assertEvaluateFailure("a_function")
@@ -88,12 +88,12 @@ def run_test_evaluate_expressions(self, context=None):
 if self.isExpressionParsedExpected():
 self.assertEvaluate(
 "a_function",
-"0x.* \(a.out`a_function\(int\) at main.cpp:7\)")
+"0x.*a.out`a_function.*")
 self.assertEvaluate("a_function(1)", "1")
 self.assertEvaluate("var2 + struct1.foo", "17")
 self.assertEvaluate(
 "foo_func",
-"0x.* \(a.out`foo_func\(\) at foo.cpp:3\)")
+"0x.*a.out`foo_func.*")
 self.assertEvaluate("foo_var", "44")
 else:
 self.assertEvaluateFailure("a_function")
@@ -118,12 +118,12 @@ def run_test_evaluate_expressions(self, context=None):
 if self.isExpressionParsedExpected():
 self.assertEvaluate(
 "a_function",
-"0x.* \(a.out`a_function\(int\) at main.cpp:7\)")
+"0x.*a.out`a_function.*")
 self.assertEvaluate("a_function(1)", "1")
 self.assertEvaluate("var + 1", "43")
 self.assertEvaluate(
 "foo_func",
-"0x.* \(a.out`foo_func\(\) at foo.cpp:3\)")
+"0x.*a.out`foo_func.*")
 self.assertEvaluate("foo_var", "44")
 else:
 self.assertEvaluateFailure("a_function")



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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 330884.
nealsid added a comment.

Rebased on HEAD


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildcard}",
+"${thread.id}",
+"${thread.protocol_

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D98153#2610953 , @teemperor wrote:

> Thanks for cleaning this up! Are the `if (!sc) ...` stuff are missing nullptr 
> checks that affect a normal LLDB session (not sure if we can ever have no 
> SymbolContext) or is this just for the unit test?
>
> Anyway, I have some small inline comments but otherwise this looks pretty 
> good to me.

Thanks again for the review! I should have addressed everything with the new 
diff I just uploaded, but I'm still not sure about the pre-merge check failures 
because it looks (to me) like the build bot should have "lldb" in its cmake 
command, although I will freely admit zero experience with LLVM's build 
infrastructure :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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