[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added subscribers: omjavaid, DavidSpickett.
DavidSpickett added a reviewer: omjavaid.
DavidSpickett added a comment.

Do we support 32 bit Arm specifically? (I'm not very familiar with Windows 
targets)

@omjavaid Has done some testing on the latest Windows On Arm which is AArch64. 
I think you should probably say "ARM and AArch64 support is more" but he can 
comment on how well it works.




Comment at: lldb/docs/index.rst:81
+expected to work, with functionality improving rapidly. ARM support is
+more experimmental, with more known issues than the others.
 

"experimental"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96840

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

[Lldb-commits] [PATCH] D96463: [LLDB] Arm64/Linux test case for MTE and Pointer Authentication regset

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

We are using SVE read/write in this test to make sure we do not overlap 
register offsets while calculating offsets for the dynamic registers. Further 
dynamic register sets should also be readable.




Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:35
+
+p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
+for i in range(16):

DavidSpickett wrote:
> Can you explain the logic for the values here?
P reg sets predicate lanes. P0 will have all lanes set while P5 will have no 
lanes set. These are just random values testing read/write.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:39
+' '.join(p_value_bytes[i % 5] for _ in range(p_reg_size)) + '}'
+self.expect("register read " + 'p%i' % (i), substrs=[p_regs_value])
+

DavidSpickett wrote:
> You don't need the brackets around i for `% (i)`.
Ack.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:49
+
+for i in range(32):
+self.runCmd('register write ' + 'z%i' %

DavidSpickett wrote:
> Is it slightly more strict if we have one loop that reads then writes?
> ```
> for i in range(32):
>write
>read
> ```
> 
> Since we write the same value, if you write them all then read them all you 
> aren't totally sure that each one is lined up right. Then again I'm sure 
> you've tested sve register writes elsewhere.
> 
> Anyway, a single loop for each would be a bit neater.
I think you are right. If we do a read after write in a single loop we ll be 
doing 64 ptrace calls on lldb-server side. More the better.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:111
+
+if not self.isAArch64SVE() and sve_regset_available:
+self.fail(

DavidSpickett wrote:
> If you move all this code from after the for, into the for loop, you could 
> skip having the bools per register set.
I was actually trying to see whether what cpuinfo reports and what is reported 
by prctl is same or not.
I am going to revisit this piece and adjust.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:14
+  asm volatile("setffr\n\t");
+  asm volatile("ptrue p0.b\n\t");
+  asm volatile("ptrue p1.h\n\t");

DavidSpickett wrote:
> (I'm not familiar with SVE assembly but anyway..)
> 
> Is the .b/.h/.s etc. pattern specific or does it not matter?
Replied in comment below.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:31-32
+
+  asm volatile("cpy  z0.b, p0/z, #1\n\t");
+  asm volatile("cpy  z1.b, p5/z, #2\n\t");
+  asm volatile("cpy  z2.b, p10/z, #3\n\t");

DavidSpickett wrote:
> Same here, is the p0/p5/p10/p15 pattern affecting values used in the test or 
> just for fun? (which isn't a bad thing)
I copied this from SVE test case that I wrote last year. 

P is a predicate register and ptrue/pfalse instruction is used to set a 
predicate lane with a pattern. pattern is decide based on size specifier, b, h, 
s and d.

if size specified is b all lanes will be set to 1. which is needed to set al 
bytes in a  Z registers to the specified value.

We are setting p0, p5, p10 and p15 to enable all lanes other p registers are 
not enabling all lanes thats why they were not used as predicate for setting Z 
registers in following lines which set a constant value to Z register byte size 
elements.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:68
+  unsigned int mte_is_enabled = 0;
+  unsigned int pauth_is_enabled = 0;
+

DavidSpickett wrote:
> This appears to be defined but not set.
Ack. I ll adjust these as per your suggestion below.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:73
+
+  if (hwcap & HWCAP_SVE) // check if SVE is present
+set_sve_registers();

DavidSpickett wrote:
> Should there be a `#ifndef HWCAP_SVE` above too?
Most distros have now backported ptrace.h to include HWCAP_SVE but other two 
are still in the process.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:82
+  if (hwcap2 & HWCAP2_MTE)
+mte_is_enabled = 1;
+

DavidSpickett wrote:
> Would be neat to do these vars like:
> ```
> unsigned int mte_is_enabled = hwcap2 & HWCAP2_MTE;
> ```
Cool!


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

https://reviews.llvm.org/D96463

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

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D96840#2567890 , @DavidSpickett 
wrote:

> Do we support 32 bit Arm specifically? (I'm not very familiar with Windows 
> targets)

Yes, I implemented support for both ARM and AArch64 in late 2019.

> @omjavaid Has done some testing on the latest Windows On Arm which is 
> AArch64. I think you should probably say "ARM and AArch64 support is more" 
> but he can comment on how well it works.

Possibly yes. I was referring to an issue for the ARM part that I never got 
time to fix properly, see D70840 . Other than 
that, I don't know of any issues with the AArch64 part, but I haven't used it 
very much either, fwiw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96840

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

Re: [Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Omair Javaid via lldb-commits]

On Wed, 17 Feb 2021 at 15:10, Martin Storsjö via Phabricator <
revi...@reviews.llvm.org> wrote:

> mstorsjo added a comment.
>
> In D96840#2567890 ,
> @DavidSpickett wrote:
>
> > Do we support 32 bit Arm specifically? (I'm not very familiar with
> Windows targets)
>
> Yes, I implemented support for both ARM and AArch64 in late 2019.
>
> > @omjavaid Has done some testing on the latest Windows On Arm which is
> AArch64. I think you should probably say "ARM and AArch64 support is more"
> but he can comment on how well it works.
>
> Possibly yes. I was referring to an issue for the ARM part that I never
> got time to fix properly, see D70840 .
> Other than that, I don't know of any issues with the AArch64 part, but I
> haven't used it very much either, fwiw.
>

HI  mstorsjo, did you manage to run LLDB testsuite with python support
enabled on windows. Can you please share steps to build and test LLDB on
windows on Arm/AArch64 if you have them handy somewhere. I ll check if
there is something missing on our part which needs to be done to get LLDB
in reasonable shape on windows on Arm/AArch64.

IMO, the biggest hurdle right now is swig/API generation and python support
enablement. If you think that is working already with some hack, then
please share it with me. I am using MS Surface Arm64 for the build.


>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D96840/new/
>
> https://reviews.llvm.org/D96840
>
>

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

Re: [Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Martin Storsjö via lldb-commits]

On Wed, 17 Feb 2021, Omair Javaid wrote:


HI  mstorsjo, did you manage to run LLDB testsuite with python support
enabled on windows. Can you please share steps to build and test LLDB on
windows on Arm/AArch64 if you have them handy somewhere. I ll check if there
is something missing on our part which needs to be done to get LLDB in
reasonable shape on windows on Arm/AArch64.

IMO, the biggest hurdle right now is swig/API generation and python support
enablement. If you think that is working already with some hack, then please
share it with me. I am using MS Surface Arm64 for the build.


Hi,

I never ran the full testsuite, and I built with python disabled.

I did manage to run some testcases, but I'm cross compiling (building on 
x86_64 linux, targeting aarch64 windows), so I transplanted parts of the 
testsuite to the target system and ran it via WSL - not very smooth, but 
it helped create and verify some test cases like 
test/Shell/Register/aarch64-fp-read.test at least.


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

Re: [Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Omair Javaid via lldb-commits]

On Wed, 17 Feb 2021 at 15:26, Martin Storsjö  wrote:

> On Wed, 17 Feb 2021, Omair Javaid wrote:
>
> > HI  mstorsjo, did you manage to run LLDB testsuite with python support
> > enabled on windows. Can you please share steps to build and test LLDB on
> > windows on Arm/AArch64 if you have them handy somewhere. I ll check if
> there
> > is something missing on our part which needs to be done to get LLDB in
> > reasonable shape on windows on Arm/AArch64.
> >
> > IMO, the biggest hurdle right now is swig/API generation and python
> support
> > enablement. If you think that is working already with some hack, then
> please
> > share it with me. I am using MS Surface Arm64 for the build.
>
> Hi,
>
> I never ran the full testsuite, and I built with python disabled.
>
> I did manage to run some testcases, but I'm cross compiling (building on
> x86_64 linux, targeting aarch64 windows), so I transplanted parts of the
> testsuite to the target system and ran it via WSL - not very smooth, but
> it helped create and verify some test cases like
> test/Shell/Register/aarch64-fp-read.test at least.
>

Yes this was what I expected.

>
> // Martin
>


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

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.



Comment at: lldb/docs/index.rst:80
 (*) Support for Windows is under active development. Basic functionality is
-expected to work, with functionality improving rapidly.
+expected to work, with functionality improving rapidly. ARM support is
+more experimmental, with more known issues than the others.

s/ARM/ARM and AArch64/ 

Also add a line to reflect lack of python support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96840

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

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/docs/index.rst:80
 (*) Support for Windows is under active development. Basic functionality is
-expected to work, with functionality improving rapidly.
+expected to work, with functionality improving rapidly. ARM support is
+more experimmental, with more known issues than the others.

omjavaid wrote:
> s/ARM/ARM and AArch64/ 
> 
> Also add a line to reflect lack of python support.
Sure, I can label the AArch64 bit experimental too.

I wouldn't include comments about python here though. The fact that it's not 
easy to build LLDB with python for windows/aarch64 isn't an intrinsic feature 
of LLDB or something that is missing in LLDB, that's just python.

It is actually possible to cross compile LLDB with python support for 
windows/aarch64, at least for the mingw target. Python itself is a bit tricky 
to cross compile, but once that cross compilation itself works, building it for 
windows/aarch64 isn't any worse than for x86. In 
https://github.com/mstorsjo/llvm-mingw/issues/73 there's ongoing efforts in 
building and packaging LLDB for windows with python enabled. As it's primarily 
cross compiled, it's a bit tricky for running tests though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96840

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

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo updated this revision to Diff 324250.
mstorsjo added a comment.

Fixed the typo, labeling the AArch64 support experimental too.


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

https://reviews.llvm.org/D96840

Files:
  lldb/docs/index.rst


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -74,10 +74,11 @@
 * iOS, tvOS, and watchOS device debugging on ARM and AArch64
 * Linux user-space debugging for i386, x86_64 and PPC64le
 * FreeBSD user-space debugging for i386 and x86_64
-* Windows user-space debugging for i386 (*)
+* Windows user-space debugging for i386, x86_64, ARM and AArch64 (*)
 
 (*) Support for Windows is under active development. Basic functionality is
-expected to work, with functionality improving rapidly.
+expected to work, with functionality improving rapidly. ARM and AArch64 support
+is more experimental, with more known issues than the others.
 
 Get Involved
 


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -74,10 +74,11 @@
 * iOS, tvOS, and watchOS device debugging on ARM and AArch64
 * Linux user-space debugging for i386, x86_64 and PPC64le
 * FreeBSD user-space debugging for i386 and x86_64
-* Windows user-space debugging for i386 (*)
+* Windows user-space debugging for i386, x86_64, ARM and AArch64 (*)
 
 (*) Support for Windows is under active development. Basic functionality is
-expected to work, with functionality improving rapidly.
+expected to work, with functionality improving rapidly. ARM and AArch64 support
+is more experimental, with more known issues than the others.
 
 Get Involved
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D96463: [LLDB] Arm64/Linux test case for MTE and Pointer Authentication regset

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py:35
+
+p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
+for i in range(16):

omjavaid wrote:
> DavidSpickett wrote:
> > Can you explain the logic for the values here?
> P reg sets predicate lanes. P0 will have all lanes set while P5 will have no 
> lanes set. These are just random values testing read/write.
I would put that in a comment to save some time for those unfamiliar with SVE.



Comment at: 
lldb/test/API/commands/register/register/aarch64_dynamic_regset/main.c:31-32
+
+  asm volatile("cpy  z0.b, p0/z, #1\n\t");
+  asm volatile("cpy  z1.b, p5/z, #2\n\t");
+  asm volatile("cpy  z2.b, p10/z, #3\n\t");

omjavaid wrote:
> DavidSpickett wrote:
> > Same here, is the p0/p5/p10/p15 pattern affecting values used in the test 
> > or just for fun? (which isn't a bad thing)
> I copied this from SVE test case that I wrote last year. 
> 
> P is a predicate register and ptrue/pfalse instruction is used to set a 
> predicate lane with a pattern. pattern is decide based on size specifier, b, 
> h, s and d.
> 
> if size specified is b all lanes will be set to 1. which is needed to set al 
> bytes in a  Z registers to the specified value.
> 
> We are setting p0, p5, p10 and p15 to enable all lanes other p registers are 
> not enabling all lanes thats why they were not used as predicate for setting 
> Z registers in following lines which set a constant value to Z register byte 
> size elements.
I'd put that in a comment. Enough for someone triaging a failure to know what's 
arbitrary numbers and what's very specific.


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

https://reviews.llvm.org/D96463

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha added a comment.

CRTP was my first implementation, however, I discarded it as more bug-prone. 
Virtual Clone function at the interface is so common that, I believe, everyone 
knows it must be overridden by a new derived class. The necessity of inheriting 
from base_clone_helper is not so obvious.




Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173
+  // Trigger the callback second time.
+  file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"),
+ eVarSetOperationReplace);

dblaikie wrote:
> Generally it shouldn't be necessary to write `llvm::StringRef(...)` around a 
> string literal - StringRef is implicitly convertible from a string literal.
OptionValueProperties and OptionValueFileSpecList enforce using StringRef 
explicitly by specifying the overload SetValueFromString(const char*) as 
deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173
+  // Trigger the callback second time.
+  file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"),
+ eVarSetOperationReplace);

tatyana-krasnukha wrote:
> dblaikie wrote:
> > Generally it shouldn't be necessary to write `llvm::StringRef(...)` around 
> > a string literal - StringRef is implicitly convertible from a string 
> > literal.
> OptionValueProperties and OptionValueFileSpecList enforce using StringRef 
> explicitly by specifying the overload SetValueFromString(const char*) as 
> deleted.
So I was curious why we added these deleted overloads and from what I can see 
this was done as part of some C-string -> StringRef conversion in D24847. I 
don't think these overloads serve any purpose anymore, so I made a review for 
removing them: D96861


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [lldb] 8bcc037 - [lldb][NFC] Delete deleted const char* overloads of SetValueFromString

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-02-17T16:13:20+01:00
New Revision: 8bcc03767e440f229749d101f470f73b8e1cd2e5

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

LOG: [lldb][NFC] Delete deleted const char* overloads of SetValueFromString

This came up during the review of D96817 because those deleted overloads force
the caller to explicitly create a StringRef when passing a string literal.

It seems they were added as some kind of help while migrating the code base to
StringRef in D24847, but I don't think they have any use these days and make
these functions awkward to use.

This patch just removes all the deleted overloads.

Reviewed By: tatyana-krasnukha

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

Added: 


Modified: 
lldb/include/lldb/Interpreter/OptionValueArch.h
lldb/include/lldb/Interpreter/OptionValueArray.h
lldb/include/lldb/Interpreter/OptionValueBoolean.h
lldb/include/lldb/Interpreter/OptionValueChar.h
lldb/include/lldb/Interpreter/OptionValueEnumeration.h
lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
lldb/include/lldb/Interpreter/OptionValueFileSpec.h
lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
lldb/include/lldb/Interpreter/OptionValueFormat.h
lldb/include/lldb/Interpreter/OptionValueFormatEntity.h
lldb/include/lldb/Interpreter/OptionValueLanguage.h
lldb/include/lldb/Interpreter/OptionValuePathMappings.h
lldb/include/lldb/Interpreter/OptionValueRegex.h
lldb/include/lldb/Interpreter/OptionValueSInt64.h
lldb/include/lldb/Interpreter/OptionValueString.h
lldb/include/lldb/Interpreter/OptionValueUInt64.h
lldb/include/lldb/Interpreter/OptionValueUUID.h

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/OptionValueArch.h 
b/lldb/include/lldb/Interpreter/OptionValueArch.h
index 809261ef22c3..5047db775fc2 100644
--- a/lldb/include/lldb/Interpreter/OptionValueArch.h
+++ b/lldb/include/lldb/Interpreter/OptionValueArch.h
@@ -43,9 +43,6 @@ class OptionValueArch : public OptionValue {
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;

diff  --git a/lldb/include/lldb/Interpreter/OptionValueArray.h 
b/lldb/include/lldb/Interpreter/OptionValueArray.h
index 4546bbb80394..14bc28404ec3 100644
--- a/lldb/include/lldb/Interpreter/OptionValueArray.h
+++ b/lldb/include/lldb/Interpreter/OptionValueArray.h
@@ -32,9 +32,6 @@ class OptionValueArray : public OptionValue {
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_values.clear();

diff  --git a/lldb/include/lldb/Interpreter/OptionValueBoolean.h 
b/lldb/include/lldb/Interpreter/OptionValueBoolean.h
index 1af14a4980ed..6b58eb94b826 100644
--- a/lldb/include/lldb/Interpreter/OptionValueBoolean.h
+++ b/lldb/include/lldb/Interpreter/OptionValueBoolean.h
@@ -33,9 +33,6 @@ class OptionValueBoolean : public OptionValue {
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;

diff  --git a/lldb/include/lldb/Interpreter/OptionValueChar.h 
b/lldb/include/lldb/Interpreter/OptionValueChar.h
index a8ecf507a4cf..b5b39b77b9b7 100644
--- a/lldb/include/lldb/Interpreter/OptionValueChar.h
+++ b/lldb/include/lldb/Interpreter/OptionValueChar.h
@@ -34,9 +34,6 @@ class OptionValueChar : public OptionValue {
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;

diff  --git a/lldb/include/lldb/Interpreter/OptionValueEnumeration.h 
b/lldb/include/lldb/Interpreter/OptionValueEnumeration.h
index 12c6473c7f1c..5ef6fa54ab82 100644
--- a/lldb/include/lldb/Interpreter/OptionValueEnumeration.h
+++ b/lldb/include/lldb/Interpreter/OptionValueEnumeration.h
@@ -43,9 +43,6 @@ class OptionValueEnumeration : public OptionValue {
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperat

[Lldb-commits] [PATCH] D96861: [lldb][NFC] Delete deleted const char* overloads of SetValueFromString

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8bcc03767e44: [lldb][NFC] Delete deleted const char* 
overloads of SetValueFromString (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96861

Files:
  lldb/include/lldb/Interpreter/OptionValueArch.h
  lldb/include/lldb/Interpreter/OptionValueArray.h
  lldb/include/lldb/Interpreter/OptionValueBoolean.h
  lldb/include/lldb/Interpreter/OptionValueChar.h
  lldb/include/lldb/Interpreter/OptionValueEnumeration.h
  lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
  lldb/include/lldb/Interpreter/OptionValueFileSpec.h
  lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
  lldb/include/lldb/Interpreter/OptionValueFormat.h
  lldb/include/lldb/Interpreter/OptionValueFormatEntity.h
  lldb/include/lldb/Interpreter/OptionValueLanguage.h
  lldb/include/lldb/Interpreter/OptionValuePathMappings.h
  lldb/include/lldb/Interpreter/OptionValueRegex.h
  lldb/include/lldb/Interpreter/OptionValueSInt64.h
  lldb/include/lldb/Interpreter/OptionValueString.h
  lldb/include/lldb/Interpreter/OptionValueUInt64.h
  lldb/include/lldb/Interpreter/OptionValueUUID.h

Index: lldb/include/lldb/Interpreter/OptionValueUUID.h
===
--- lldb/include/lldb/Interpreter/OptionValueUUID.h
+++ lldb/include/lldb/Interpreter/OptionValueUUID.h
@@ -32,9 +32,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_uuid.Clear();
Index: lldb/include/lldb/Interpreter/OptionValueUInt64.h
===
--- lldb/include/lldb/Interpreter/OptionValueUInt64.h
+++ lldb/include/lldb/Interpreter/OptionValueUInt64.h
@@ -43,9 +43,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;
Index: lldb/include/lldb/Interpreter/OptionValueString.h
===
--- lldb/include/lldb/Interpreter/OptionValueString.h
+++ lldb/include/lldb/Interpreter/OptionValueString.h
@@ -81,9 +81,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;
Index: lldb/include/lldb/Interpreter/OptionValueSInt64.h
===
--- lldb/include/lldb/Interpreter/OptionValueSInt64.h
+++ lldb/include/lldb/Interpreter/OptionValueSInt64.h
@@ -46,9 +46,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_current_value = m_default_value;
Index: lldb/include/lldb/Interpreter/OptionValueRegex.h
===
--- lldb/include/lldb/Interpreter/OptionValueRegex.h
+++ lldb/include/lldb/Interpreter/OptionValueRegex.h
@@ -32,9 +32,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_regex = RegularExpression(m_default_regex_str);
Index: lldb/include/lldb/Interpreter/OptionValuePathMappings.h
===
--- lldb/include/lldb/Interpreter/OptionValuePathMappings.h
+++ lldb/include/lldb/Interpreter/OptionValuePathMappings.h
@@ -31,9 +31,6 @@
   Status
   SetValueFromString(llvm::StringRef value,
  VarSetOperationType op = eVarSetOperationAssign) override;
-  Status
-  SetValueFromString(const char *,
- VarSetOperationType = eVarSetOperationAssign) = delete;
 
   void Clear() override {
 m_path_mappings.Clear(m_notify_changes);
Index: lldb/include/lldb/Interpreter/OptionValueLanguage.h
===
--- lldb/include/lldb/Interpreter/OptionValueLanguage.h
+++ lldb/include/lldb/

[Lldb-commits] [PATCH] D96833: [lldb] Print a useful error message when trying to import modules with dots or dashes

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D96833

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

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Dave Lee via Phabricator via lldb-commits]

kastiglione accepted this revision.
kastiglione added a comment.
This revision is now accepted and ready to land.

"Asynced" sounds funny to me, but I don't have suggestions other than "Async".

Looks good to me.




Comment at: lldb/source/Target/RegisterContextUnwind.cpp:558
+  m_full_unwind_plan_sp = async_plan_sp;
+  if (active_row.get() && log) {
+StreamString active_row_strm;

Looks like active_row will always be valid here, as the code is currently 
constructed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D96779: [lldb] Fix shared library directory computation on windows

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova accepted this revision.
stella.stamenova added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96779

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

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:177
 
+  // A language runtime may be able to provide a special UnwindPlan for
+  // the frame represented by the register contents \a regctx when that

Please use `///` so this becomes a Doxygen comment. 



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:187
+  static lldb::UnwindPlanSP
+  GetAsyncedFrameUnwindPlan(lldb_private::Thread &thread,
+lldb_private::RegisterContext *regctx);

`s/Asynced/Async/` maybe?



Comment at: lldb/source/Target/RegisterContextUnwind.cpp:294
+  //
+  // A Swift async function will have a normal UnwindPlan when it is 
+  // executing, but it may be unscheduled (or on a different thread)

nit: This sounds like is supposed to be generic, I'd drop the Swift here, you 
already gave that as an example above. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

To be honest, I'm not sure how to reproduce this kind of debug info. I've tried 
a few examples (e.g. force inline the function from another CU) , but it didn't 
work.

Should be postpone this patch until someone can figure out the test case then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:187
+  static lldb::UnwindPlanSP
+  GetAsyncedFrameUnwindPlan(lldb_private::Thread &thread,
+lldb_private::RegisterContext *regctx);

JDevlieghere wrote:
> `s/Asynced/Async/` maybe?
Maybe "Pending"?  From the standpoint of the API, it doesn't really matter why 
there are some not-currently executing stacks, just that they are not currently 
executing.  Pending describes the state, not the why, so might be more 
appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D96537: Make the error condition in Value::ValueType explicit (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

aprantl closed this revision.
aprantl added a comment.

This landed as 057efa9916cdc354ef4653bcd128a578cc43125e 
 I messed 
up the commit message.


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

https://reviews.llvm.org/D96537

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

[Lldb-commits] [lldb] d6e8057 - [lldb] Improve error message for modules with dots or dashes

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-02-17T10:00:29-08:00
New Revision: d6e80578fc5e5199d5783671bd0c8ce1050925f9

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

LOG: [lldb] Improve error message for modules with dots or dashes

LLDB does not like to import Python files with dashes or dots in their
name. While the former are technically allowed, it is discouraged. Dots
are allowed for subpackages but not in module names. This patch improves
the user experience by printing a useful error.

Before this patch:

  error: module importing failed: SyntaxError('invalid syntax',
  ('', 1, 11, 'import foo-bar\n'))

After this patch:

  error: module importing failed: Python discourages dashes in module
  names: foo-bar

rdar://74263511

[1] https://www.python.org/dev/peps/pep-0008/#package-and-module-names

Differential revision: https://reviews.llvm.org/D96833

Added: 
lldb/test/Shell/ScriptInterpreter/Python/command_import.test

Modified: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 6b53bd3a2edc..c2fd1f266f07 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2781,6 +2781,7 @@ bool ScriptInterpreterPythonImpl::LoadScriptingModule(
   };
 
   std::string module_name(pathname);
+  bool possible_package = false;
 
   if (extra_search_dir) {
 if (llvm::Error e = ExtendSysPath(extra_search_dir.GetPath())) {
@@ -2805,6 +2806,7 @@ bool ScriptInterpreterPythonImpl::LoadScriptingModule(
 return false;
   }
   // Not a filename, probably a package of some sort, let it go through.
+  possible_package = true;
 } else if (is_directory(st) || is_regular_file(st)) {
   if (module_file.GetDirectory().IsEmpty()) {
 error.SetErrorString("invalid directory name");
@@ -2831,6 +2833,18 @@ bool ScriptInterpreterPythonImpl::LoadScriptingModule(
   module_name.resize(module_name.length() - 4);
   }
 
+  if (!possible_package && module_name.find('.') != llvm::StringRef::npos) {
+error.SetErrorStringWithFormat(
+"Python does not allow dots in module names: %s", module_name.c_str());
+return false;
+  }
+
+  if (module_name.find('-') != llvm::StringRef::npos) {
+error.SetErrorStringWithFormat(
+"Python discourages dashes in module names: %s", module_name.c_str());
+return false;
+  }
+
   // check if the module is already import-ed
   StreamString command_stream;
   command_stream.Clear();

diff  --git a/lldb/test/Shell/ScriptInterpreter/Python/command_import.test 
b/lldb/test/Shell/ScriptInterpreter/Python/command_import.test
new file mode 100644
index ..737313bea226
--- /dev/null
+++ b/lldb/test/Shell/ScriptInterpreter/Python/command_import.test
@@ -0,0 +1,20 @@
+# REQUIRES: python
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: echo "print('Rene Magritte')" >> %t/foo.py
+# RUN: echo "print('Jan van Eyck')" >> %t/foo-bar.py
+# RUN: echo "print('Pieter Bruegel the Elder')" >> %t/foo.bar.py
+# RUN: echo "print('Pieter Bruegel the Younger')" >> %t/foo.bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.py' 
2>&1 | FileCheck %s --check-prefix MAGRITTE
+# MAGRITTE: Rene Magritte
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo-bar.py' 
2>&1 | FileCheck %s --check-prefix VANEYCK
+# VANEYCK-NOT: Jan van Eyck
+# VANEYCK: module importing failed: Python discourages dashes in module names: 
foo-bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar.py' 
2>&1 | FileCheck %s --check-prefix BRUEGEL
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar' 
2>&1 | FileCheck %s --check-prefix BRUEGEL
+# BRUEGEL-NOT: Pieter Bruegel the Elder
+# BRUEGEL-NOT: Pieter Bruegel the Younger
+# BRUEGEL: module importing failed: Python does not allow dots in module 
names: foo.bar



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

[Lldb-commits] [PATCH] D96833: [lldb] Print a useful error message when trying to import modules with dots or dashes

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd6e80578fc5e: [lldb] Improve error message for modules with 
dots or dashes (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D96833?vs=324169&id=324340#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96833

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/test/Shell/ScriptInterpreter/Python/command_import.test


Index: lldb/test/Shell/ScriptInterpreter/Python/command_import.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/command_import.test
@@ -0,0 +1,20 @@
+# REQUIRES: python
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: echo "print('Rene Magritte')" >> %t/foo.py
+# RUN: echo "print('Jan van Eyck')" >> %t/foo-bar.py
+# RUN: echo "print('Pieter Bruegel the Elder')" >> %t/foo.bar.py
+# RUN: echo "print('Pieter Bruegel the Younger')" >> %t/foo.bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.py' 
2>&1 | FileCheck %s --check-prefix MAGRITTE
+# MAGRITTE: Rene Magritte
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo-bar.py' 
2>&1 | FileCheck %s --check-prefix VANEYCK
+# VANEYCK-NOT: Jan van Eyck
+# VANEYCK: module importing failed: Python discourages dashes in module names: 
foo-bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar.py' 
2>&1 | FileCheck %s --check-prefix BRUEGEL
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar' 
2>&1 | FileCheck %s --check-prefix BRUEGEL
+# BRUEGEL-NOT: Pieter Bruegel the Elder
+# BRUEGEL-NOT: Pieter Bruegel the Younger
+# BRUEGEL: module importing failed: Python does not allow dots in module 
names: foo.bar
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2781,6 +2781,7 @@
   };
 
   std::string module_name(pathname);
+  bool possible_package = false;
 
   if (extra_search_dir) {
 if (llvm::Error e = ExtendSysPath(extra_search_dir.GetPath())) {
@@ -2805,6 +2806,7 @@
 return false;
   }
   // Not a filename, probably a package of some sort, let it go through.
+  possible_package = true;
 } else if (is_directory(st) || is_regular_file(st)) {
   if (module_file.GetDirectory().IsEmpty()) {
 error.SetErrorString("invalid directory name");
@@ -2831,6 +2833,18 @@
   module_name.resize(module_name.length() - 4);
   }
 
+  if (!possible_package && module_name.find('.') != llvm::StringRef::npos) {
+error.SetErrorStringWithFormat(
+"Python does not allow dots in module names: %s", module_name.c_str());
+return false;
+  }
+
+  if (module_name.find('-') != llvm::StringRef::npos) {
+error.SetErrorStringWithFormat(
+"Python discourages dashes in module names: %s", module_name.c_str());
+return false;
+  }
+
   // check if the module is already import-ed
   StreamString command_stream;
   command_stream.Clear();


Index: lldb/test/Shell/ScriptInterpreter/Python/command_import.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/command_import.test
@@ -0,0 +1,20 @@
+# REQUIRES: python
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: echo "print('Rene Magritte')" >> %t/foo.py
+# RUN: echo "print('Jan van Eyck')" >> %t/foo-bar.py
+# RUN: echo "print('Pieter Bruegel the Elder')" >> %t/foo.bar.py
+# RUN: echo "print('Pieter Bruegel the Younger')" >> %t/foo.bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.py' 2>&1 | FileCheck %s --check-prefix MAGRITTE
+# MAGRITTE: Rene Magritte
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo-bar.py' 2>&1 | FileCheck %s --check-prefix VANEYCK
+# VANEYCK-NOT: Jan van Eyck
+# VANEYCK: module importing failed: Python discourages dashes in module names: foo-bar
+
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar.py' 2>&1 | FileCheck %s --check-prefix BRUEGEL
+# RUN: %lldb --script-language python -o 'command script import %t/foo.bar' 2>&1 | FileCheck %s --check-prefix BRUEGEL
+# BRUEGEL-NOT: Pieter Bruegel the Elder
+# BRUEGEL-NOT: Pieter Bruegel the Younger
+# BRUEGEL: module importing failed: Python does not allow dots in module names: foo.bar
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInter

[Lldb-commits] [PATCH] D96861: [lldb][NFC] Delete deleted const char* overloads of SetValueFromString

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Nice catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96861

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

[Lldb-commits] [PATCH] D95712: [lldb/bindings] Add Python ScriptedProcess base class to lldb module

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/scripted_process/scripted_process.py:18
+
+Attributes:
+args (lldb.SBStructuredData): Dictionary holding arbitrary values

Doesn't the python documentation generator infer the attributes and methods 
from this file? If not, is it worth having to keep it in sync?



Comment at: lldb/examples/python/scripted_process/scripted_process.py:67-71
+self.process_id = 0
+self.memory_regions = []
+self.threads = []
+self.loaded_images = []
+self.stops = []

Personally I don't think these belongs in the base class:

 1. The corresponding methods don't return these lists. 
 2. More importantly, if they're attributes then that becomes part of the 
interface. Now every implementation needs to populate e.g. the `threads` list 
while they might want to compute that lazily, like some of the OS plugin 
implementations do. 



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:27
+def test_python_plugin_package(self):
+"""Test that the lldb python module has a `plugins.scripted_process`
+package."""

None of the expect calls seems to be checking that the package exists and can 
be imported? 



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:33
+self.expect('script dir(lldb.plugins)',
+patterns=["scripted_process"])
+

It's not clear to me what this is actually testing. The same is true for the 
other `expect` calls below. Maybe a comment or add a failure string to the 
expect calls. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95712

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

[Lldb-commits] [PATCH] D95711: [lldb/Interpreter] Add ScriptInterpreter Wrapper for ScriptedProcess

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:548
   lldb::ScriptLanguage m_script_lang;
+  lldb::ScriptedProcessInterfaceSP m_scripted_process_interface_sp;
 };

Why does this need to be a shared pointer and not a unique one? I think the 
script interpreter should have unique ownership over the scripted process 
interface. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95711

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

[Lldb-commits] [PATCH] D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:117
+  if (m_interpreter)
+m_pid = m_interpreter->GetScriptedProcessInterface().GetProcessID();
+}

`m_interpreter->GetScriptedProcessInterface()` seems to repeated a lot. Maybe 
it's worth having a private helper method that wraps it?

```
ScriptedProcess::GetInferface() { 
  return m_interpreter->GetScriptedProcessInterface();
}
```



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:153
+  if (!m_interpreter) {
+size = LLDB_INVALID_ADDRESS;
+error.SetErrorString("No interpreter.");

Remembering to set the size seems error prone. I would use a `ScopeExit` (and 
clear it at the end on success). 



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:47
+
+@skipUnlessDarwin
+def test_launch_scripted_process(self):

I think this should work on all platforms? 



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:54
+self.assertTrue(target, VALID_TARGET)
+scripted_process_example_relpath = 
"../../../../examples/python/scripted_process/my_scripted_process.py"
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'

can you make this more portable with `os.path.join` or `os.dirname`? 



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:55
+scripted_process_example_relpath = 
"../../../../examples/python/scripted_process/my_scripted_process.py"
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + 
os.path.join(self.getSourceDir(),

Why is this needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95713

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

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:187
+  static lldb::UnwindPlanSP
+  GetAsyncedFrameUnwindPlan(lldb_private::Thread &thread,
+lldb_private::RegisterContext *regctx);

jingham wrote:
> JDevlieghere wrote:
> > `s/Asynced/Async/` maybe?
> Maybe "Pending"?  From the standpoint of the API, it doesn't really matter 
> why there are some not-currently executing stacks, just that they are not 
> currently executing.  Pending describes the state, not the why, so might be 
> more appropriate.
Thanks for the comments all.  Yeah the wording Asynced is a little weird, but I 
was trying to get across the idea that a function is executing normally on the 
stack, then it is descheduled (or the function it calls is executed on another 
thread) and it has been async'ified from its previous running state, and will 
later revivify.  But it's probably more trouble than it's worth and I should 
just use Async here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: 
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:55
+scripted_process_example_relpath = 
"../../../../examples/python/scripted_process/my_scripted_process.py"
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + 
os.path.join(self.getSourceDir(),

JDevlieghere wrote:
> Why is this needed?
I use this to load the script without running the `process launch -C ` 
command.

You can see the rest of the logic in D95712, in the MyScriptedProcess example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95713

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

[Lldb-commits] [PATCH] D96548: [lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support

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

mgorny updated this revision to Diff 324393.
mgorny retitled this revision from "[lldb] [Process/FreeBSDRemote] Introduce 
aarch64 hw break/watchpoint support [WIP]" to "[lldb] [Process/FreeBSDRemote] 
Introduce aarch64 hw break/watchpoint support".
mgorny added a reviewer: mhorne.
mgorny added a comment.

Ok, the changes have landed in FreeBSD src, so I think we're ready to go here.

Changes from previous version:

- enabled hardware breakpoints on FreeBSD
- changed the existing names to use `hbp` consistently (instead of mix of `hbp` 
and `hbr`)
- added FreeBSD version checks


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

https://reviews.llvm.org/D96548

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
  
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h

Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h
@@ -0,0 +1,77 @@
+//===-- NativeRegisterContextBreakWatchpoint_arm64.h *- C++ -*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef lldb_NativeRegisterContextBreakWatchpoint_arm64_h
+#define lldb_NativeRegisterContextBreakWatchpoint_arm64_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+#include 
+
+namespace lldb_private {
+
+class NativeRegisterContextBreakWatchpoint_arm64
+: public virtual NativeRegisterContextRegisterInfo {
+public:
+  uint32_t NumSupportedHardwareBreakpoints() override;
+
+  uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override;
+
+  bool ClearHardwareBreakpoint(uint32_t hw_idx) override;
+
+  Status ClearAllHardwareBreakpoints() override;
+
+  Status GetHardwareBreakHitIndex(uint32_t &bp_index,
+  lldb::addr_t trap_addr) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  bool ClearHardwareWatchpoint(uint32_t hw_index) override;
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  lldb::addr_t GetWatchpointHitAddress(uint32_t wp_index) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t GetWatchpointSize(uint32_t wp_index);
+
+  bool WatchpointIsEnabled(uint32_t wp_index);
+
+  // Debug register type select
+  enum DREGType { eDREGTypeWATCH = 0, eDREGTypeBREAK };
+
+protected:
+  // Debug register info for hardware breakpoints and watchpoints management.
+  struct DREG {
+lldb::addr_t address;  // Breakpoint/watchpoint address value.
+lldb::addr_t hit_addr; // Address at which last watchpoint trigger exception
+   // occurred.
+lldb::addr_t real_addr; // Address value that should cause target to stop.
+uint32_t control;   // Breakpoint/watchpoint control value.
+  };
+
+  std::array m_hbp_regs; // hardware breakpoints
+  std::array m_hwp_regs; // hardware watchpoints
+
+  uint32_t m_max_hbp_supported;
+  uint32_t m_max_hwp_supported;
+
+  virtual llvm::Error ReadHardwareDebugInfo() = 0;
+  virtual llvm::Error WriteHardwareDebugRegs(DREGType hwbType) = 0;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_NativeRegisterContextBreakWatchpoint_arm64_h
Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
@@ -0,0 +1,458 @@
+//===-- NativeRegisterContextBreakWatchpoint_arm64.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 "NativeRegisterConte

[Lldb-commits] [PATCH] D96548: [lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support

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

mgorny added a comment.

Oh, and added the method to copy dbregs to new threads.


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

https://reviews.llvm.org/D96548

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

[Lldb-commits] [PATCH] D96548: [lldb] [Process/FreeBSDRemote] Introduce aarch64 hw break/watchpoint support

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

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

clang-format.


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

https://reviews.llvm.org/D96548

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
  
lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h

Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.h
@@ -0,0 +1,77 @@
+//===-- NativeRegisterContextBreakWatchpoint_arm64.h *- C++ -*-===//
+//
+// 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
+//
+//===--===//
+
+#ifndef lldb_NativeRegisterContextBreakWatchpoint_arm64_h
+#define lldb_NativeRegisterContextBreakWatchpoint_arm64_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+#include 
+
+namespace lldb_private {
+
+class NativeRegisterContextBreakWatchpoint_arm64
+: public virtual NativeRegisterContextRegisterInfo {
+public:
+  uint32_t NumSupportedHardwareBreakpoints() override;
+
+  uint32_t SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override;
+
+  bool ClearHardwareBreakpoint(uint32_t hw_idx) override;
+
+  Status ClearAllHardwareBreakpoints() override;
+
+  Status GetHardwareBreakHitIndex(uint32_t &bp_index,
+  lldb::addr_t trap_addr) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  bool ClearHardwareWatchpoint(uint32_t hw_index) override;
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  lldb::addr_t GetWatchpointHitAddress(uint32_t wp_index) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t GetWatchpointSize(uint32_t wp_index);
+
+  bool WatchpointIsEnabled(uint32_t wp_index);
+
+  // Debug register type select
+  enum DREGType { eDREGTypeWATCH = 0, eDREGTypeBREAK };
+
+protected:
+  // Debug register info for hardware breakpoints and watchpoints management.
+  struct DREG {
+lldb::addr_t address;  // Breakpoint/watchpoint address value.
+lldb::addr_t hit_addr; // Address at which last watchpoint trigger exception
+   // occurred.
+lldb::addr_t real_addr; // Address value that should cause target to stop.
+uint32_t control;   // Breakpoint/watchpoint control value.
+  };
+
+  std::array m_hbp_regs; // hardware breakpoints
+  std::array m_hwp_regs; // hardware watchpoints
+
+  uint32_t m_max_hbp_supported;
+  uint32_t m_max_hwp_supported;
+
+  virtual llvm::Error ReadHardwareDebugInfo() = 0;
+  virtual llvm::Error WriteHardwareDebugRegs(DREGType hwbType) = 0;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_NativeRegisterContextBreakWatchpoint_arm64_h
Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextBreakWatchpoint_arm64.cpp
@@ -0,0 +1,458 @@
+//===-- NativeRegisterContextBreakWatchpoint_arm64.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 "NativeRegisterContextBreakWatchpoint_arm64.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+using namespace lldb_private;
+
+uint32_t
+NativeRegisterContextBreakWatchpoint_arm64::NumSupportedHardwareBreakpoints() {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
+  llvm::Error error = ReadHardwareDebugInfo();
+  if (error) {
+LLDB_LOG_ERROR(log, std::move(error),
+   "failed to read debug registers: {0}");
+return 0;
+  }
+
+  retu

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Can each real stack frame inject N frames in the middle of a stack frame for 
these asynchronous functions? Or is this like the libdispatch stuff that show 
more frames at the end of a normal stack trace? Can you attach some sample 
backtraces with some annotations?




Comment at: lldb/include/lldb/Target/LanguageRuntime.h:21
 #include "lldb/Target/ExecutionContextScope.h"
+#include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/Runtime.h"

I don't think we need this in the header file as we only use pointers to a 
RegisterContext. So move this to the .cpp file?



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:188
+  GetAsyncedFrameUnwindPlan(lldb_private::Thread &thread,
+lldb_private::RegisterContext *regctx);
+

Any reason we are passing in the register context and the thread instead of a 
stack frame? a stack frame has access to the register context and to the thread 
it belongs to. Or is this register context just a raw register context that was 
extracted from somewhere in memory?



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:192
+  virtual lldb::UnwindPlanSP
+  GetAsyncedFrameForThisLanguage(lldb_private::Thread &thread,
+ lldb_private::RegisterContext *regctx) {

What does "ThisLanguage" this mean? Is it the language of the stack frame that 
owns "regctx"? Should we pass in the current stack frame instead of a register 
context?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> CRTP was my first implementation, however, I discarded it as more bug-prone. 
> Virtual Clone function at the interface is so common that, I believe, 
> everyone knows it must be overridden by a new derived class. The necessity of 
> inheriting from base_clone_helper is not so obvious.

I would've thought it'd be pretty easy to accidentally miss either of these - I 
think the CRTP helper ensures consistency of implementation (harder to 
accidentally slice/copy the wrong type/etc. But I'm not a code owner/major 
contributor to lldb specifically, so probably more up to other developers who 
are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2568794 , @werat wrote:

> To be honest, I'm not sure how to reproduce this kind of debug info. I've 
> tried a few examples (e.g. force inline the function from another CU) , but 
> it didn't work.
>
> Should we postpone this patch until someone can figure out the test case then?

How'd you come across the issue? Presumably it requires some dwz usage to get, 
but that's OK - if there's a small example using dwz we could probably recreate 
that with some hand crafted assembly without going to great lengcths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

The D91014  patch has been found by a 
testsuite regression when running in DWZ mode. This patch has only the same 
pattern in the code and currently it is unclear whether this change is really 
reproducible or it is in fact just NFC.
OTOH I would find this change even as a code cleanup as (1) the code has less 
letters and (2) the previous code may work or not but the new code definitely 
should work.
I can check whether I can make a testcase for this patch as I agree new 
testcases are always good (but today it is 11pm here already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96839: Add a callout to the LanguageRuntime to override the normal UnwindPlan used for a frame

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:188
+  GetAsyncedFrameUnwindPlan(lldb_private::Thread &thread,
+lldb_private::RegisterContext *regctx);
+

clayborg wrote:
> Any reason we are passing in the register context and the thread instead of a 
> stack frame? a stack frame has access to the register context and to the 
> thread it belongs to. Or is this register context just a raw register context 
> that was extracted from somewhere in memory?
I'll rethink this though, but I don't think we have a StackFrame yet.  We're 
constructing the register context and we'll use that in creating the stack 
frame in a bit.  I might be off-by-one, these parts of the unwinder can take 
some mental gymnastics to model properly when you're changing things.



Comment at: lldb/include/lldb/Target/LanguageRuntime.h:192
+  virtual lldb::UnwindPlanSP
+  GetAsyncedFrameForThisLanguage(lldb_private::Thread &thread,
+ lldb_private::RegisterContext *regctx) {

clayborg wrote:
> What does "ThisLanguage" this mean? Is it the language of the stack frame 
> that owns "regctx"? Should we pass in the current stack frame instead of a 
> register context?
Yeah it's not a great method name.  The LanguageRuntime base class has a list 
of registered LanguageRuntime plugins.  I want to iterate over them, seeing if 
any LanguageRuntime can provide one of these special one-off UnwindPlans.  Tbh 
the top-level method GetAsyncedFrameForThisLanguage could have gone in Process 
and it could call a method of the same name in all the LanguageRuntime plugins, 
would end up working the same.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96839

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

[Lldb-commits] [PATCH] D96637: Make sure the interpreter module was loaded before making checks against it

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D96637#2567269 , @aadsm wrote:

>> We should have a test for this.
>
> how do you recommend doing this? I spent a couple of hours on this but got no 
> where. From what I understood we should prefer lit tests, so I was thinking 
> of creating a binary that dlopens a module. However, I wasn't able to create 
> a binary that I can start and capture its pid address so that I can attach 
> to. Here's what I've tried so far:
>
>   // RUN: cp %s %s.cpp
>   // RUN: %clang -g -O0 --target=x86_64-linux-gnu %s.cpp -o %s.out
>   // RUN: PID=$(%s.out)
>   // RUN: %lldb -p $PID -b -o 'target list' | FileCheck %s
>   // RUN: kill -9 $PID
>   // CHECK: foo
>   
>   #include 
>   #include 
>   
>   int main() {
>   pid_t pid = fork();
>   if (pid > 0) {
>   // parent process, print child pid
>   printf("%d", pid);
>   return 0;
>   } else if (pid < 0) {
>   printf("Unable to fork\n");
>   return -1;
>   }
>   // child process
>   pause();
>   }
>
> The lit test get stuck on `// RUN: PID=$(%s.out)`. Not sure why, the parent 
> process shouldn't wait on its children..

I would do an end to end test for this. We have many attach tests that should 
be easy to modify and pause() and then try to load a local dylib that is 
dlopen'ed. Unless Pavel has a differing opinion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96637

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

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 324431.
shafik added a comment.

- Went with unnamed enums Vs anonymous enums


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

https://reviews.llvm.org/D96807

Files:
  clang/lib/AST/TypePrinter.cpp
  clang/test/AST/ast-dump-decl-json.c
  clang/test/AST/ast-dump-enum-json.cpp
  clang/test/AST/ast-dump-openmp-cancel.c
  clang/test/AST/ast-dump-openmp-cancellation-point.c
  clang/test/AST/ast-dump-openmp-distribute-parallel-for-simd.c
  clang/test/AST/ast-dump-openmp-distribute-parallel-for.c
  clang/test/AST/ast-dump-openmp-distribute-simd.c
  clang/test/AST/ast-dump-openmp-distribute.c
  clang/test/AST/ast-dump-openmp-for-simd.c
  clang/test/AST/ast-dump-openmp-for.c
  clang/test/AST/ast-dump-openmp-ordered.c
  clang/test/AST/ast-dump-openmp-parallel-for-simd.c
  clang/test/AST/ast-dump-openmp-parallel-for.c
  clang/test/AST/ast-dump-openmp-parallel-sections.c
  clang/test/AST/ast-dump-openmp-parallel.c
  clang/test/AST/ast-dump-openmp-section.c
  clang/test/AST/ast-dump-openmp-sections.c
  clang/test/AST/ast-dump-openmp-simd.c
  clang/test/AST/ast-dump-openmp-single.c
  clang/test/AST/ast-dump-openmp-target-data.c
  clang/test/AST/ast-dump-openmp-target-enter-data.c
  clang/test/AST/ast-dump-openmp-target-exit-data.c
  clang/test/AST/ast-dump-openmp-target-parallel-for-simd.c
  clang/test/AST/ast-dump-openmp-target-parallel-for.c
  clang/test/AST/ast-dump-openmp-target-parallel.c
  clang/test/AST/ast-dump-openmp-target-simd.c
  clang/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for-simd.c
  clang/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for.c
  clang/test/AST/ast-dump-openmp-target-teams-distribute-simd.c
  clang/test/AST/ast-dump-openmp-target-teams-distribute.c
  clang/test/AST/ast-dump-openmp-target-teams.c
  clang/test/AST/ast-dump-openmp-target-update.c
  clang/test/AST/ast-dump-openmp-target.c
  clang/test/AST/ast-dump-openmp-task.c
  clang/test/AST/ast-dump-openmp-taskgroup.c
  clang/test/AST/ast-dump-openmp-taskloop-simd.c
  clang/test/AST/ast-dump-openmp-taskloop.c
  clang/test/AST/ast-dump-openmp-teams-distribute-parallel-for-simd.c
  clang/test/AST/ast-dump-openmp-teams-distribute-parallel-for.c
  clang/test/AST/ast-dump-openmp-teams-distribute-simd.c
  clang/test/AST/ast-dump-openmp-teams-distribute.c
  clang/test/AST/ast-dump-openmp-teams.c
  clang/test/AST/ast-dump-records-json.cpp
  clang/test/AST/ast-dump-records.c
  clang/test/AST/ast-dump-records.cpp
  clang/test/AST/ast-dump-stmt-json.m
  clang/test/ASTMerge/struct/test.c
  clang/test/Analysis/cfg.cpp
  clang/test/Analysis/padding_c.c
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Layout/ms-x86-alias-avoidance-padding.cpp
  clang/test/PCH/stmt-openmp_structured_block-bit.cpp
  clang/test/Sema/assign.c
  clang/test/Sema/switch.c
  clang/test/SemaCXX/condition.cpp
  clang/test/SemaCXX/enum.cpp
  clang/test/SemaCXX/warn-sign-conversion.cpp
  
lldb/test/Shell/SymbolFile/DWARF/clang-ast-from-dwarf-unamed-and-anon-structs.cpp
  lldb/test/Shell/SymbolFile/DWARF/debug-types-missing-signature.test

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

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik marked an inline comment as done.
shafik added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1308
+} else if ((isa(D) && 
cast(D)->isAnonymousStructOrUnion()) ||
+isa(D)) {
   OS << "anonymous";

aaron.ballman wrote:
> I think `EnumDecl` should probably be `unnamed` rather than `anonymous` 
> because there's no term of art for an anonymous enumeration (there's only 
> anonymous structures and anonymous unions). WDYT?
I thought I saw that wording but when I looked back I realize you are correct.


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

https://reviews.llvm.org/D96807

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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha updated this revision to Diff 324435.
tatyana-krasnukha added a comment.

Removed explicit conversions to StringRef from the test with respect to D96861 
.


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

https://reviews.llvm.org/D96817

Files:
  lldb/include/lldb/Interpreter/OptionValue.h
  lldb/include/lldb/Interpreter/OptionValueArch.h
  lldb/include/lldb/Interpreter/OptionValueArgs.h
  lldb/include/lldb/Interpreter/OptionValueArray.h
  lldb/include/lldb/Interpreter/OptionValueBoolean.h
  lldb/include/lldb/Interpreter/OptionValueChar.h
  lldb/include/lldb/Interpreter/OptionValueDictionary.h
  lldb/include/lldb/Interpreter/OptionValueEnumeration.h
  lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
  lldb/include/lldb/Interpreter/OptionValueFileSpec.h
  lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
  lldb/include/lldb/Interpreter/OptionValueFormat.h
  lldb/include/lldb/Interpreter/OptionValueFormatEntity.h
  lldb/include/lldb/Interpreter/OptionValueLanguage.h
  lldb/include/lldb/Interpreter/OptionValuePathMappings.h
  lldb/include/lldb/Interpreter/OptionValueProperties.h
  lldb/include/lldb/Interpreter/OptionValueRegex.h
  lldb/include/lldb/Interpreter/OptionValueSInt64.h
  lldb/include/lldb/Interpreter/OptionValueString.h
  lldb/include/lldb/Interpreter/OptionValueUInt64.h
  lldb/include/lldb/Interpreter/OptionValueUUID.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Interpreter/OptionValue.cpp
  lldb/source/Interpreter/OptionValueArch.cpp
  lldb/source/Interpreter/OptionValueArgs.cpp
  lldb/source/Interpreter/OptionValueArray.cpp
  lldb/source/Interpreter/OptionValueBoolean.cpp
  lldb/source/Interpreter/OptionValueChar.cpp
  lldb/source/Interpreter/OptionValueDictionary.cpp
  lldb/source/Interpreter/OptionValueEnumeration.cpp
  lldb/source/Interpreter/OptionValueFileColonLine.cpp
  lldb/source/Interpreter/OptionValueFileSpec.cpp
  lldb/source/Interpreter/OptionValueFileSpecList.cpp
  lldb/source/Interpreter/OptionValueFormat.cpp
  lldb/source/Interpreter/OptionValueFormatEntity.cpp
  lldb/source/Interpreter/OptionValueLanguage.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/source/Interpreter/OptionValueRegex.cpp
  lldb/source/Interpreter/OptionValueSInt64.cpp
  lldb/source/Interpreter/OptionValueString.cpp
  lldb/source/Interpreter/OptionValueUInt64.cpp
  lldb/source/Interpreter/OptionValueUUID.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/test/API/commands/settings/TestSettings.py
  lldb/unittests/Interpreter/CMakeLists.txt
  lldb/unittests/Interpreter/TestOptionValue.cpp

Index: lldb/unittests/Interpreter/TestOptionValue.cpp
===
--- /dev/null
+++ lldb/unittests/Interpreter/TestOptionValue.cpp
@@ -0,0 +1,173 @@
+//===-- TestOptionValue.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/Interpreter/OptionValues.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+class Callback {
+public:
+  virtual void Invoke() const {}
+  void operator()() const { Invoke(); }
+};
+
+class MockCallback : public Callback {
+public:
+  MOCK_CONST_METHOD0(Invoke, void());
+};
+
+// Test a single-value class.
+TEST(OptionValueString, DeepCopy) {
+  OptionValueString str;
+  str.SetValueFromString("ab");
+
+  MockCallback callback;
+  str.SetValueChangedCallback([&callback] { callback(); });
+  EXPECT_CALL(callback, Invoke());
+
+  auto copy_sp = str.DeepCopy(nullptr);
+
+  // Test that the base class data members are copied/set correctly.
+  ASSERT_TRUE(copy_sp);
+  ASSERT_EQ(copy_sp->GetParent().get(), nullptr);
+  ASSERT_TRUE(copy_sp->OptionWasSet());
+  ASSERT_EQ(copy_sp->GetStringValue(), "ab");
+
+  // Trigger the callback.
+  copy_sp->SetValueFromString("c", eVarSetOperationAppend);
+  ASSERT_EQ(copy_sp->GetStringValue(), "abc");
+}
+
+// Test an aggregate class.
+TEST(OptionValueArgs, DeepCopy) {
+  OptionValueArgs args;
+  args.SetValueFromString("A B");
+
+  MockCallback callback;
+  args.SetValueChangedCallback([&callback] { callback(); });
+  EXPECT_CALL(callback, Invoke());
+
+  auto copy_sp = args.DeepCopy(nullptr);
+
+  // Test that the base class data members are copied/set correctly.
+  ASSERT_TRUE(copy_sp);
+  ASSERT_EQ(copy_sp->GetParent(), nullptr);
+  ASSERT_TRUE(copy_sp->OptionWasSet());
+
+  auto *args_copy_ptr = copy_sp->GetAsArgs();
+  ASSERT_EQ(args_copy_ptr->GetSize(), 2U);
+  ASSERT_EQ((*args_copy_ptr)[0]->GetParent(), copy_sp);
+  ASSERT_EQ((*args_copy_ptr)[0]->G

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D96817#2569595 , @dblaikie wrote:

>> CRTP was my first implementation, however, I discarded it as more bug-prone. 
>> Virtual Clone function at the interface is so common that, I believe, 
>> everyone knows it must be overridden by a new derived class. The necessity 
>> of inheriting from base_clone_helper is not so obvious.
>
> I would've thought it'd be pretty easy to accidentally miss either of these - 
> I think the CRTP helper ensures consistency of implementation (harder to 
> accidentally slice/copy the wrong type/etc. But I'm not a code owner/major 
> contributor to lldb specifically, so probably more up to other developers who 
> are.

Whatever is the safest and most llvm like is probably the best approach IMHO. 
So maybe stick with a solution that will be familiar to LLVM developers if this 
current approach isn't. I am open to other suggestions if others feel strongly 
otherwise.


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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96680: [lldb-vscode] Emit the breakpoint changed event on location resolved

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

needs a test. Just make a test app with a shared library that gets dlopen'ed in 
the main function. At first the breakpoint in the shared library will be 
unresolved, then after stepping over the dlopen call, it should get resolved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96680

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

[Lldb-commits] [PATCH] D96916: [lldb] Fix PushPlan to set subplan to private

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added a reviewer: jingham.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Call `SetPrivate(true)` for subplans pushed via `PushPlan()`, as described in 
its
docstring.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96916

Files:
  lldb/include/lldb/Target/ThreadPlan.h


Index: lldb/include/lldb/Target/ThreadPlan.h
===
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -478,7 +478,7 @@
   // another thread plan is never either of the above.
   void PushPlan(lldb::ThreadPlanSP &thread_plan_sp) {
 GetThread().PushPlan(thread_plan_sp);
-thread_plan_sp->SetPrivate(false);
+thread_plan_sp->SetPrivate(true);
 thread_plan_sp->SetIsMasterPlan(false);
   }
 


Index: lldb/include/lldb/Target/ThreadPlan.h
===
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -478,7 +478,7 @@
   // another thread plan is never either of the above.
   void PushPlan(lldb::ThreadPlanSP &thread_plan_sp) {
 GetThread().PushPlan(thread_plan_sp);
-thread_plan_sp->SetPrivate(false);
+thread_plan_sp->SetPrivate(true);
 thread_plan_sp->SetIsMasterPlan(false);
   }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D96917: [lldb] Rename {stop, run}_vote to report_{stop, run}_vote

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added a reviewer: jingham.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Rename `stop_vote` and `run_vote` to `report_stop_vote` and `report_run_vote`
respectively. These variables are limited to logic involving (event) reporting 
only.
This naming is intended to make their context more clear.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96917

Files:
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStepInstruction.h
  lldb/include/lldb/Target/ThreadPlanStepOut.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadPlan.cpp
  lldb/source/Target/ThreadPlanBase.cpp
  lldb/source/Target/ThreadPlanStepInstruction.cpp
  lldb/source/Target/ThreadPlanStepOut.cpp

Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -33,11 +33,11 @@
 // ThreadPlanStepOut: Step out of the current frame
 ThreadPlanStepOut::ThreadPlanStepOut(
 Thread &thread, SymbolContext *context, bool first_insn, bool stop_others,
-Vote stop_vote, Vote run_vote, uint32_t frame_idx,
+Vote report_stop_vote, Vote report_run_vote, uint32_t frame_idx,
 LazyBool step_out_avoids_code_without_debug_info,
 bool continue_to_next_branch, bool gather_return_value)
-: ThreadPlan(ThreadPlan::eKindStepOut, "Step out", thread, stop_vote,
- run_vote),
+: ThreadPlan(ThreadPlan::eKindStepOut, "Step out", thread, report_stop_vote,
+ report_run_vote),
   ThreadPlanShouldStopHere(this), m_step_from_insn(LLDB_INVALID_ADDRESS),
   m_return_bp_id(LLDB_INVALID_BREAK_ID),
   m_return_addr(LLDB_INVALID_ADDRESS), m_stop_others(stop_others),
Index: lldb/source/Target/ThreadPlanStepInstruction.cpp
===
--- lldb/source/Target/ThreadPlanStepInstruction.cpp
+++ lldb/source/Target/ThreadPlanStepInstruction.cpp
@@ -23,10 +23,11 @@
 ThreadPlanStepInstruction::ThreadPlanStepInstruction(Thread &thread,
  bool step_over,
  bool stop_other_threads,
- Vote stop_vote,
- Vote run_vote)
+ Vote report_stop_vote,
+ Vote report_run_vote)
 : ThreadPlan(ThreadPlan::eKindStepInstruction,
- "Step over single instruction", thread, stop_vote, run_vote),
+ "Step over single instruction", thread, report_stop_vote,
+ report_run_vote),
   m_instruction_addr(0), m_stop_other_threads(stop_other_threads),
   m_step_over(step_over) {
   m_takes_iteration_count = true;
Index: lldb/source/Target/ThreadPlanBase.cpp
===
--- lldb/source/Target/ThreadPlanBase.cpp
+++ lldb/source/Target/ThreadPlanBase.cpp
@@ -70,8 +70,8 @@
 }
 
 bool ThreadPlanBase::ShouldStop(Event *event_ptr) {
-  m_stop_vote = eVoteYes;
-  m_run_vote = eVoteYes;
+  m_report_stop_vote = eVoteYes;
+  m_report_run_vote = eVoteYes;
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
 
@@ -82,8 +82,8 @@
 case eStopReasonInvalid:
 case eStopReasonNone:
   // This
-  m_run_vote = eVoteNoOpinion;
-  m_stop_vote = eVoteNo;
+  m_report_run_vote = eVoteNoOpinion;
+  m_report_stop_vote = eVoteNo;
   return false;
 
 case eStopReasonBreakpoint:
@@ -106,11 +106,11 @@
   // with "restarted" so the UI will know to wait and expect the consequent
   // "running".
   if (stop_info_sp->ShouldNotify(event_ptr)) {
-m_stop_vote = eVoteYes;
-m_run_vote = eVoteYes;
+m_report_stop_vote = eVoteYes;
+m_report_run_vote = eVoteYes;
   } else {
-m_stop_vote = eVoteNo;
-m_run_vote = eVoteNo;
+m_report_stop_vote = eVoteNo;
+m_report_run_vote = eVoteNo;
   }
   return false;
 
@@ -156,9 +156,9 @@
 // We're not going to stop, but while we are here, let's figure out
 // whether to report this.
 if (stop_info_sp->ShouldNotify(event_ptr))
-  m_stop_vote = eVoteYes;
+  m_report_stop_vote = eVoteYes;
 else
-  m_stop_vote = eVoteNo;
+  m_report_stop_vote = eVoteNo;
   }
   return false;
 
@@ -167,8 +167,8 @@
 }
 
   } else {
-m_run_vote = eVoteNoOpinion;
-m_stop_vote = eVoteNo;
+m_report_run_vote = eVoteNoOpinion;
+m_report_stop_vote = eVoteNo;
   }
 
   // If there's no explicit reason to stop, then

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96817#2569852 , @clayborg wrote:

> In D96817#2569595 , @dblaikie wrote:
>
>>> CRTP was my first implementation, however, I discarded it as more 
>>> bug-prone. Virtual Clone function at the interface is so common that, I 
>>> believe, everyone knows it must be overridden by a new derived class. The 
>>> necessity of inheriting from base_clone_helper is not so obvious.
>>
>> I would've thought it'd be pretty easy to accidentally miss either of these 
>> - I think the CRTP helper ensures consistency of implementation (harder to 
>> accidentally slice/copy the wrong type/etc. But I'm not a code owner/major 
>> contributor to lldb specifically, so probably more up to other developers 
>> who are.
>
> Whatever is the safest and most llvm like is probably the best approach IMHO. 
> So maybe stick with a solution that will be familiar to LLVM developers if 
> this current approach isn't. I am open to other suggestions if others feel 
> strongly otherwise.

I think either would be legible/familiar to LLVM developers. I rather like the 
CRTP, but it's not a make-or-break.

I found a couple of instances of cloning APIs across LLVM (incomplete results - 
grep only gets one so far):

  clang/include/clang/Sema/TypoCorrection.h:  virtual 
std::unique_ptr clone() = 0;
  lldb/include/lldb/Core/SearchFilter.h:  virtual lldb::SearchFilterSP 
DoCreateCopy() = 0;

And neither uses a CRTP for the derive classes, FWIW.


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

https://reviews.llvm.org/D96817

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

[Lldb-commits] [PATCH] D96917: [lldb] Rename {stop, run}_vote to report_{stop, run}_vote

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

Sure, that's clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96917

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

[Lldb-commits] [lldb] fcdef15 - Add a new Row setting to mark all un-declared regs as Undefined

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2021-02-17T23:52:28-08:00
New Revision: fcdef15d77bde8a200cd28242077cd2496f138a4

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

LOG: Add a new Row setting to mark all un-declared regs as Undefined

Add a new state for UnwindPlan::Row which indicates that any
register not listed is not defined, and should not be found in
stack frames newer than this one and passed up the stack.  Mostly
intended for use with architectural default unwind plans that are
used for jitted stack frames, where we have no unwind information
or start address.  lldb has no way to tell if registers were
spilled in the jitted frame & overwritten, so passing register
values up the stack is not safe to show the user.

Architectural default unwind plans are also used as a fast unwind
plan on x86_64 in particular, and are used as the fallback unwind
plans when lldb thinks it may be able to work around a problem
which causes the unwinder to stop walking the stack early.

For fast unwind plans, when we don't find a register location in
the arch default unwind plan, we fall back to computing & using
the full unwind plan. One small part of this patch is to know that
a register marked as Undefined in the fast unwind plan is a special
case, and we should continue on to the full unwind plan to find what
the real unwind rule is for this register.

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


Added: 


Modified: 
lldb/include/lldb/Symbol/UnwindPlan.h
lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp
lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp
lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
lldb/source/Plugins/ABI/X86/ABIMacOSX_i386.cpp
lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp
lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
lldb/source/Symbol/UnwindPlan.cpp
lldb/source/Target/RegisterContextUnwind.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/UnwindPlan.h 
b/lldb/include/lldb/Symbol/UnwindPlan.h
index 40814da3de4a..06c76ca80796 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -360,6 +360,25 @@ class UnwindPlan {
 
 bool SetRegisterLocationToSame(uint32_t reg_num, bool must_replace);
 
+// When this UnspecifiedRegistersAreUndefined mode is
+// set, any register that is not specified by this Row will
+// be described as Undefined.
+// This will prevent the unwinder from iterating down the
+// stack looking for a spill location, or a live register value
+// at frame 0.
+// It would be used for an UnwindPlan row where we can't track
+// spilled registers -- for instance a jitted stack frame where
+// we have no unwind information or start address -- and registers
+// MAY have been spilled and overwritten, so providing the
+// spilled/live value from a newer frame may show an incorrect value.
+void SetUnspecifiedRegistersAreUndefined(bool unspec_is_undef) {
+  m_unspecified_registers_are_undefined = unspec_is_undef;
+}
+
+bool GetUnspecifiedRegistersAreUndefined() {
+  return m_unspecified_registers_are_undefined;
+}
+
 void Clear();
 
 void Dump(Stream &s, const UnwindPlan *unwind_plan, Thread *thread,
@@ -372,6 +391,7 @@ class UnwindPlan {
 FAValue m_cfa_value;
 FAValue m_afa_value;
 collection m_register_locations;
+bool m_unspecified_registers_are_undefined;
   }; // class Row
 
   typedef std::shared_ptr RowSP;

diff  --git a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp 
b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
index ea0c09a1596a..861310e3ea0c 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
@@ -384,6 +384,7 @@ bool ABIMacOSX_arm64::CreateDefaultUnwindPlan(UnwindPlan 
&unwind_plan) {
 
   row->GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 2 * ptr_size);
   row->SetOffset(0);
+  row->SetUnspecifiedRegistersAreUndefined(true);
 
   row->SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
   row->SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);

diff  --git a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp 
b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
index 5918e2ccb5fa..0f74c1f5c73b 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/A

[Lldb-commits] [PATCH] D96829: Add "Undeclared registers are marked as Undefined" to UnwindPlan, adopt it in arch default unwind plans

[Jason Molenda via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfcdef15d77bd: Add a new Row setting to mark all un-declared 
regs as Undefined (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96829

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
  lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
  lldb/source/Plugins/ABI/Hexagon/ABISysV_hexagon.cpp
  lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp
  lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
  lldb/source/Plugins/ABI/X86/ABIMacOSX_i386.cpp
  lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp
  lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
  lldb/source/Symbol/UnwindPlan.cpp
  lldb/source/Target/RegisterContextUnwind.cpp

Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -542,6 +542,8 @@
   StreamString active_row_strm;
   active_row->Dump(active_row_strm, m_fast_unwind_plan_sp.get(), &m_thread,
m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
+  UnwindLogMsg("Using fast unwind plan '%s'",
+   m_fast_unwind_plan_sp->GetSourceName().AsCString());
   UnwindLogMsg("active row: %s", active_row_strm.GetData());
 }
   } else {
@@ -556,6 +558,8 @@
 active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
  &m_thread,
  m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
+UnwindLogMsg("Using full unwind plan '%s'",
+ m_full_unwind_plan_sp->GetSourceName().AsCString());
 UnwindLogMsg("active row: %s", active_row_strm.GetData());
   }
 }
@@ -662,13 +666,6 @@
   *m_thread.CalculateTarget(), m_thread);
   if (unwind_plan_sp) {
 if (unwind_plan_sp->PlanValidAtAddress(m_current_pc)) {
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
-  if (log && log->GetVerbose()) {
-if (m_fast_unwind_plan_sp)
-  UnwindLogMsgVerbose("frame, and has a fast UnwindPlan");
-else
-  UnwindLogMsgVerbose("frame");
-  }
   m_frame_type = eNormalFrame;
   return unwind_plan_sp;
 } else {
@@ -1147,6 +1144,7 @@
 RegisterContextUnwind::SavedLocationForRegister(
 uint32_t lldb_regnum, lldb_private::UnwindLLDB::RegisterLocation ®loc) {
   RegisterNumber regnum(m_thread, eRegisterKindLLDB, lldb_regnum);
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
 
   // Have we already found this register location?
   if (!m_registers.empty()) {
@@ -1179,8 +1177,17 @@
(int)unwindplan_registerkind);
   return UnwindLLDB::RegisterSearchResult::eRegisterNotFound;
 }
+// The architecture default unwind plan marks unknown registers as
+// Undefined so that we don't forward them up the stack when a
+// jitted stack frame may have overwritten them.  But when the
+// arch default unwind plan is used as the Fast Unwind Plan, we
+// need to recognize this & switch over to the Full Unwind Plan
+// to see what unwind rule that (more knoweldgeable, probably)
+// UnwindPlan has.  If the full UnwindPlan says the register 
+// location is Undefined, then it really is.
 if (active_row->GetRegisterInfo(regnum.GetAsKind(unwindplan_registerkind),
-unwindplan_regloc)) {
+unwindplan_regloc) &&
+!unwindplan_regloc.IsUndefined()) {
   UnwindLogMsg(
   "supplying caller's saved %s (%d)'s location using FastUnwindPlan",
   regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
@@ -1191,8 +1198,11 @@
   if (!have_unwindplan_regloc) {
 // m_full_unwind_plan_sp being NULL means that we haven't tried to find a
 // full UnwindPlan yet
-if (!m_full_unwind_plan_sp)
+bool got_new_full_unwindplan = false;
+if (!m_full_unwind_plan_sp) {
   m_full_unwind_plan_sp = GetFullUnwindPlanForFrame();
+  got_new_full_unwindplan = true;
+}
 
 if (m_full_unwind_plan_sp) {
   RegisterNumber pc_regnum(m_thread, eRegisterKindGeneric,
@@ -1202,6 +1212,16 @@
   m_full_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
   unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
 
+  if (got_new_full_unwindplan && active_row.get() && log) {
+

[Lldb-commits] [PATCH] D96939: [lldb] Add a note to the core file loading error message that mentions archives

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: LLDB.
teemperor added a project: LLDB.
Herald added a subscriber: JDevlieghere.
teemperor requested review of this revision.

Core files are frequently compressed in some compressed archive and users 
aren't aware that LLDB can't
extract the core file for them. Because of this there is usually one question 
every day on why
LLDB is giving them a cryptic error message when they open their compressed 
core file:

  error: Unable to find process plug-in for core file '/tmp/core.gz

This just adds a note that compressed core files need to be decompressed first 
before LLDB can do anything
with them.


https://reviews.llvm.org/D96939

Files:
  lldb/source/Commands/CommandObjectTarget.cpp


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -427,6 +427,10 @@
   result.AppendErrorWithFormatv(
   "Unable to find process plug-in for core file '{0}'\n",
   core_file.GetPath());
+  result.AppendMessage(
+  "Note: If the passed file is a compressed file archive 
containing"
+  " a core file, you first need to manually extract the core file "
+  " from the archive before trying to load it.\n");
   result.SetStatus(eReturnStatusFailed);
 }
   } else {


Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -427,6 +427,10 @@
   result.AppendErrorWithFormatv(
   "Unable to find process plug-in for core file '{0}'\n",
   core_file.GetPath());
+  result.AppendMessage(
+  "Note: If the passed file is a compressed file archive containing"
+  " a core file, you first need to manually extract the core file "
+  " from the archive before trying to load it.\n");
   result.SetStatus(eReturnStatusFailed);
 }
   } else {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits