[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via lldb-commits]

dexonsmith added a comment.

Given how large this is, would it be reasonable to split this up a bit more?

What I might do if this were my patch: get a review of the API change + the 
manual changes in one patch (assuming there aren't many manual changes), then 
land the remaining mechanical changes in chunks, perhaps vaguely by component, 
likely using post-commit review. The benefit of committing by chunks is that if 
there is some problem that comes up (even mechanical changes fail) there's more 
granularity for bisection (and less churn on revert). WDYT?

Also: is there a reference to this API in the ProgrammersManual? (I honestly 
don't know, but if there is, please be sure to update it as well.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/ADT/StringRef.h:192
 
-/// equals_lower - Check for string equality, ignoring case.
+/// equals_insensitive - Check for string equality, ignoring case.
 LLVM_NODISCARD

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

"Don’t duplicate function or class name at the beginning of the comment."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D104819#2837421 , @dexonsmith 
wrote:

> Given how large this is, would it be reasonable to split this up a bit more?
>
> What I might do if this were my patch: get a review of the API change + the 
> manual changes in one patch (assuming there aren't many manual changes), then 
> land the remaining mechanical changes in chunks, perhaps vaguely by 
> component, likely using post-commit review. The benefit of committing by 
> chunks is that if there is some problem that comes up (even mechanical 
> changes fail) there's more granularity for bisection (and less churn on 
> revert). WDYT?

That sounds like a good plan to me, thanks for the suggestion!

> Also: is there a reference to this API in the ProgrammersManual? (I honestly 
> don't know, but if there is, please be sure to update it as well.)

I don't think so, at least grepping for `_lower` didn't hit anything in 
llvm/docs (and manually browsing it now, the section on StringRef doesn't 
mention those methods).




Comment at: llvm/include/llvm/ADT/StringRef.h:192
 
-/// equals_lower - Check for string equality, ignoring case.
+/// equals_insensitive - Check for string equality, ignoring case.
 LLVM_NODISCARD

MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
> 
> "Don’t duplicate function or class name at the beginning of the comment."
Thanks, I'll fix those for the methods I'm renaming here, but I'll refrain from 
touching the other methods in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via lldb-commits]

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

LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.

(I also see that there are some clang-format suggestions in the unit tests; not 
sure any of them are actually improvements so maybe you left those out 
intentionally? Just be sure to clang-format when you do the mechanical changes 
in the follow up patches.)




Comment at: llvm/include/llvm/ADT/StringRef.h:198
 
+LLVM_NODISCARD
+bool equals_lower(StringRef RHS) const { return equals_insensitive(RHS); }

You could probably drop the `LLVM_NODISCARD` from the soon-to-be-deleted 
`_lower` variants, but maybe it's not worth fussing with (up to you).



Comment at: llvm/lib/Support/StringRef.cpp:37
 
-/// compare_lower - Compare strings, ignoring case.
-int StringRef::compare_lower(StringRef RHS) const {
+/// compare_insensitive - Compare strings, ignoring case.
+int StringRef::compare_insensitive(StringRef RHS) const {

You can drop these duplicate doxygen comments entirely from the `.cpp` file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via lldb-commits]

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

Reduced this patch only to updating of the StringRef class itself and its unit 
test, removing the mechanical changes. Removed superfluous method name 
duplication in doxygen comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

Files:
  llvm/include/llvm/ADT/StringRef.h
  llvm/lib/Support/StringRef.cpp
  llvm/unittests/ADT/StringRefTest.cpp

Index: llvm/unittests/ADT/StringRefTest.cpp
===
--- llvm/unittests/ADT/StringRefTest.cpp
+++ llvm/unittests/ADT/StringRefTest.cpp
@@ -98,15 +98,15 @@
   EXPECT_EQ( 1, StringRef("aab").compare("aa"));
   EXPECT_EQ( 1, StringRef("\xFF").compare("\1"));
 
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aAd"));
-  EXPECT_EQ( 0, StringRef("AaB").compare_lower("aab"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("AAA"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aaBb"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("bb"));
-  EXPECT_EQ( 1, StringRef("aaBb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("bb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("aA"));
-  EXPECT_EQ( 1, StringRef("\xFF").compare_lower("\1"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aAd"));
+  EXPECT_EQ( 0, StringRef("AaB").compare_insensitive("aab"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("AAA"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aaBb"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("bb"));
+  EXPECT_EQ( 1, StringRef("aaBb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("bb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("aA"));
+  EXPECT_EQ( 1, StringRef("\xFF").compare_insensitive("\1"));
 
   EXPECT_EQ(-1, StringRef("aab").compare_numeric("aad"));
   EXPECT_EQ( 0, StringRef("aab").compare_numeric("aab"));
@@ -366,14 +366,14 @@
   EXPECT_FALSE(Str.startswith("hi"));
 }
 
-TEST(StringRefTest, StartsWithLower) {
+TEST(StringRefTest, StartsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.startswith_lower(""));
-  EXPECT_TRUE(Str.startswith_lower("he"));
-  EXPECT_TRUE(Str.startswith_lower("hell"));
-  EXPECT_TRUE(Str.startswith_lower("HELlo"));
-  EXPECT_FALSE(Str.startswith_lower("helloworld"));
-  EXPECT_FALSE(Str.startswith_lower("hi"));
+  EXPECT_TRUE(Str.startswith_insensitive(""));
+  EXPECT_TRUE(Str.startswith_insensitive("he"));
+  EXPECT_TRUE(Str.startswith_insensitive("hell"));
+  EXPECT_TRUE(Str.startswith_insensitive("HELlo"));
+  EXPECT_FALSE(Str.startswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.startswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeFront) {
@@ -392,22 +392,22 @@
   EXPECT_TRUE(Str.consume_front(""));
 }
 
-TEST(StringRefTest, ConsumeFrontLower) {
+TEST(StringRefTest, ConsumeFrontInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
   EXPECT_EQ("heLLo", Str);
   EXPECT_FALSE(Str.consume_front("HEl"));
   EXPECT_EQ("heLLo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("HEl"));
+  EXPECT_TRUE(Str.consume_front_insensitive("HEl"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("loworld"));
+  EXPECT_FALSE(Str.consume_front_insensitive("loworld"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("ol"));
+  EXPECT_FALSE(Str.consume_front_insensitive("ol"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("lo"));
+  EXPECT_TRUE(Str.consume_front_insensitive("lo"));
   EXPECT_EQ("", Str);
-  EXPECT_FALSE(Str.consume_front_lower("o"));
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_FALSE(Str.consume_front_insensitive("o"));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
 }
 
 TEST(StringRefTest, EndsWith) {
@@ -419,14 +419,14 @@
   EXPECT_FALSE(Str.endswith("so"));
 }
 
-TEST(StringRefTest, EndsWithLower) {
+TEST(StringRefTest, EndsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.endswith_lower(""));
-  EXPECT_TRUE(Str.endswith_lower("lo"));
-  EXPECT_TRUE(Str.endswith_lower("LO"));
-  EXPECT_TRUE(Str.endswith_lower("ELlo"));
-  EXPECT_FALSE(Str.endswith_lower("helloworld"));
-  EXPECT_FALSE(Str.endswith_lower("hi"));
+  EXPECT_TRUE(Str.endswith_insensitive(""));
+  EXPECT_TRUE(Str.endswith_insensitive("lo"));
+  EXPECT_TRUE(Str.endswith_insensitive("LO"));
+  EXPECT_TRUE(Str.endswith_insensitive("ELlo"));
+  EXPECT_FALSE(Str.endswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.endswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeBack) {
@@ -445,22 +445,22 @@
   EXPECT_TRUE(Str.consume_back(""));
 }
 
-TEST(StringRefTest, ConsumeBackLower) {
+TEST(StringRefTest, ConsumeBackInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_back_lower(""));
+  EXPECT_TRUE(Str.consume_back_

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Chris Lattner via Phabricator via lldb-commits]

lattner accepted this revision.
lattner added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D104819#2839263 , @dexonsmith 
wrote:

> LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.
>
> (I also see that there are some clang-format suggestions in the unit tests; 
> not sure any of them are actually improvements so maybe you left those out 
> intentionally?

Yeah those seem to be intentionally vertically aligned that way, no big opinion 
either way

> Just be sure to clang-format when you do the mechanical changes in the follow 
> up patches.)

Hmm, those would have to be manually reviewed, but I guess I can try to do that 
(discarding changes where the surrounding code isn't too well formatted now 
already so it does more extensive reformattings other than for the renaming).




Comment at: llvm/lib/Support/StringRef.cpp:37
 
-/// compare_lower - Compare strings, ignoring case.
-int StringRef::compare_lower(StringRef RHS) const {
+/// compare_insensitive - Compare strings, ignoring case.
+int StringRef::compare_insensitive(StringRef RHS) const {

dexonsmith wrote:
> You can drop these duplicate doxygen comments entirely from the `.cpp` file.
Sure, yeah doxygen in implementation files make little sense anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via lldb-commits]

dexonsmith added a comment.

In D104819#2839315 , @mstorsjo wrote:

> In D104819#2839263 , @dexonsmith 
> wrote:
>
>> Just be sure to clang-format when you do the mechanical changes in the 
>> follow up patches.)
>
> Hmm, those would have to be manually reviewed, but I guess I can try to do 
> that (discarding changes where the surrounding code isn't too well formatted 
> now already so it does more extensive reformattings other than for the 
> renaming).

Sounds good. (I'm generally in favour of erring on the side of trusting 
clang-format, especially for large refactorings like this; I wouldn't spend too 
much time analyzing whether it's an improvement...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

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

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3eed57e7ef7d: [ADT] Rename StringRef case insensitive 
methods for clarity (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D104819?vs=354178&id=354355#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

Files:
  llvm/include/llvm/ADT/StringRef.h
  llvm/lib/Support/StringRef.cpp
  llvm/unittests/ADT/StringRefTest.cpp

Index: llvm/unittests/ADT/StringRefTest.cpp
===
--- llvm/unittests/ADT/StringRefTest.cpp
+++ llvm/unittests/ADT/StringRefTest.cpp
@@ -98,15 +98,15 @@
   EXPECT_EQ( 1, StringRef("aab").compare("aa"));
   EXPECT_EQ( 1, StringRef("\xFF").compare("\1"));
 
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aAd"));
-  EXPECT_EQ( 0, StringRef("AaB").compare_lower("aab"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("AAA"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aaBb"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("bb"));
-  EXPECT_EQ( 1, StringRef("aaBb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("bb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("aA"));
-  EXPECT_EQ( 1, StringRef("\xFF").compare_lower("\1"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aAd"));
+  EXPECT_EQ( 0, StringRef("AaB").compare_insensitive("aab"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("AAA"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aaBb"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("bb"));
+  EXPECT_EQ( 1, StringRef("aaBb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("bb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("aA"));
+  EXPECT_EQ( 1, StringRef("\xFF").compare_insensitive("\1"));
 
   EXPECT_EQ(-1, StringRef("aab").compare_numeric("aad"));
   EXPECT_EQ( 0, StringRef("aab").compare_numeric("aab"));
@@ -366,14 +366,14 @@
   EXPECT_FALSE(Str.startswith("hi"));
 }
 
-TEST(StringRefTest, StartsWithLower) {
+TEST(StringRefTest, StartsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.startswith_lower(""));
-  EXPECT_TRUE(Str.startswith_lower("he"));
-  EXPECT_TRUE(Str.startswith_lower("hell"));
-  EXPECT_TRUE(Str.startswith_lower("HELlo"));
-  EXPECT_FALSE(Str.startswith_lower("helloworld"));
-  EXPECT_FALSE(Str.startswith_lower("hi"));
+  EXPECT_TRUE(Str.startswith_insensitive(""));
+  EXPECT_TRUE(Str.startswith_insensitive("he"));
+  EXPECT_TRUE(Str.startswith_insensitive("hell"));
+  EXPECT_TRUE(Str.startswith_insensitive("HELlo"));
+  EXPECT_FALSE(Str.startswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.startswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeFront) {
@@ -392,22 +392,22 @@
   EXPECT_TRUE(Str.consume_front(""));
 }
 
-TEST(StringRefTest, ConsumeFrontLower) {
+TEST(StringRefTest, ConsumeFrontInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
   EXPECT_EQ("heLLo", Str);
   EXPECT_FALSE(Str.consume_front("HEl"));
   EXPECT_EQ("heLLo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("HEl"));
+  EXPECT_TRUE(Str.consume_front_insensitive("HEl"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("loworld"));
+  EXPECT_FALSE(Str.consume_front_insensitive("loworld"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("ol"));
+  EXPECT_FALSE(Str.consume_front_insensitive("ol"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("lo"));
+  EXPECT_TRUE(Str.consume_front_insensitive("lo"));
   EXPECT_EQ("", Str);
-  EXPECT_FALSE(Str.consume_front_lower("o"));
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_FALSE(Str.consume_front_insensitive("o"));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
 }
 
 TEST(StringRefTest, EndsWith) {
@@ -419,14 +419,14 @@
   EXPECT_FALSE(Str.endswith("so"));
 }
 
-TEST(StringRefTest, EndsWithLower) {
+TEST(StringRefTest, EndsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.endswith_lower(""));
-  EXPECT_TRUE(Str.endswith_lower("lo"));
-  EXPECT_TRUE(Str.endswith_lower("LO"));
-  EXPECT_TRUE(Str.endswith_lower("ELlo"));
-  EXPECT_FALSE(Str.endswith_lower("helloworld"));
-  EXPECT_FALSE(Str.endswith_lower("hi"));
+  EXPECT_TRUE(Str.endswith_insensitive(""));
+  EXPECT_TRUE(Str.endswith_insensitive("lo"));
+  EXPECT_TRUE(Str.endswith_insensitive("LO"));
+  EXPECT_TRUE(Str.endswith_insensitive("ELlo"));
+  EXPECT_FALSE(Str.endswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.endswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeBack) {
@@ -445,22 +445,22 @@
   EXPECT_TRUE(Str.consume_back(""));
 }
 
-TEST(StringRefTest, ConsumeBackLower) {
+TEST(StringRefTest, ConsumeBackInsensitive) {
   StringRef Str("heLLo");
-  

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: JDevlieghere, teemperor, jhenderson.
mib added projects: Zorg, LLDB.
Herald added a subscriber: mgorny.
Herald added a reviewer: gkistanova.
mib requested review of this revision.

To reflect changes announced in 
https://lists.llvm.org/pipermail/llvm-dev/2021-January/148048.html, this patch 
updates all occurrences of `debuginfo-tests` to `cross-project-debuginfo-tests`.

Signed-off-by: Med Ismail Bennani 


Repository:
  rZORG LLVM Github Zorg

https://reviews.llvm.org/D105134

Files:
  zorg/jenkins/build.py
  zorg/jenkins/jobs/jobs/lldb-cmake
  zorg/jenkins/jobs/jobs/llvm-coverage
  zorg/jenkins/monorepo_build.py

Index: zorg/jenkins/monorepo_build.py
===
--- zorg/jenkins/monorepo_build.py
+++ zorg/jenkins/monorepo_build.py
@@ -614,7 +614,7 @@
 '-v', 'check-lldb']
 else:
 test_command = ['/usr/bin/env', 'TERM=vt100', NINJA,
-'-v', 'check-debuginfo', 'check-lldb', '-k2']
+'-v', 'check-cross_project-debuginfo-tests', 'check-lldb', '-k2']
 run_cmd(conf.lldbbuilddir(), test_command)
 footer()
 
Index: zorg/jenkins/jobs/jobs/llvm-coverage
===
--- zorg/jenkins/jobs/jobs/llvm-coverage
+++ zorg/jenkins/jobs/jobs/llvm-coverage
@@ -1,7 +1,7 @@
 #!/usr/bin/env groovy
 pipeline {
 agent { label 'green-dragon-23' }
-
+
 parameters {
 string(name: 'GIT_REVISION', defaultValue: '*/main', description: 'Git revision to build')
 string(name: 'ARTIFACT', defaultValue: 'clang-stage1-RA/latest', description: 'Compiler artifact to use for building the project')
@@ -35,21 +35,21 @@
 sh '''
 set -u
 rm -rf build.properties
-
+
 cd llvm-project
 git tag -a -m "First Commit" first_commit 97724f18c79c7cc81ced24239eb5e883bf1398ef || true
-
+
 git_desc=$(git describe --match "first_commit")
-
+
 export GIT_DISTANCE=$(echo ${git_desc} | cut -f 2 -d "-")
-
+
 sha=$(echo ${git_desc} | cut -f 3 -d "-")
 export GIT_SHA=${sha:1}
-
+
 cd -
 
 export PATH=$PATH:/usr/bin:/usr/local/bin
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py cmake build \
   --cmake-build-target FileCheck \
   --cmake-build-target count \
@@ -59,10 +59,10 @@
   --projects "" \
   --noinstall \
   --noupload
-
+
 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake build \
   --assertions \
-  --projects="clang;libcxx;libcxxabi;lldb;debuginfo-tests"  \
+  --projects="clang;libcxx;libcxxabi;lldb;cross-project-tests"  \
   --cmake-flag="-DPYTHON_LIBRARY=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
   --cmake-flag="-DPYTHON_INCLUDE_DIR=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/include/python3.7m" \
   --cmake-flag="-DPYTHON_LIBRARY_DEBUG=/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/libpython3.7.dylib" \
@@ -85,7 +85,7 @@
 export PATH=$PATH:/usr/bin:/usr/local/bin
 
 rm -rf test/results.xml
-
+
 BUILD_DIR=$(pwd)/lldb-build
 FAST_BUILD_DIR=$(pwd)/clang-build
 REPORT_DIR=$(pwd)/coverage-reports
@@ -93,31 +93,31 @@
 LLVM_PROFDATA=$HOST/llvm-profdata
 LLVM_COV=$HOST/llvm-cov
 ARTIFACT_PREP_SCRIPT=$WORKSPACE/llvm-project/llvm/utils/prepare-code-coverage-artifact.py
-
+
 FAST_TOOLS=(FileCheck count not)
 for TOOL in ${FAST_TOOLS[@]}; do
   cp $FAST_BUILD_DIR/bin/$TOOL $BUILD_DIR/bin/$TOOL
 done
-
+
 # Clear out any stale profiles.
 rm -rf $BUILD_DIR/profiles
 
 # Run the tests.
 IGNORE_ERRORS_OVERRIDE=1 python llvm-zorg/zorg/jenkins/monorepo_build.py lldb-cmake testlong || echo "Some tests may have failed."
-
+
 cd $BUILD_DIR
 ninja -k 0 check-llvm check-clang || echo "Some tests may have failed."
 cd -
-
+
 COV_BINARIES=$(find $BUILD_DIR/bin $BUILD_DIR/lib -depth 1 -type f -exec file {} \\; | gre

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.

Looks good, with the exception of my inline comment.




Comment at: zorg/jenkins/build.py:619
 run_cmd(conf.lldbbuilddir(), ['/usr/bin/env', 'TERM=vt100', NINJA, '-v',
-  'check-debuginfo', 'check-lldb', '-k2'])
+  'check-cross_project-debuginfo-tests', 
'check-lldb', '-k2'])
 footer()

Either I made a typo somewhere, or my comments were unclear apparently. This 
should be one of:

`check-cross-project`

or

`check-debuginfo`

There is no need to change it from what was there before, if you wish to 
maintain the behaviour as it was. If you change it to `check-cross-project`, 
additional tests may start getting picked up as they are added that aren't 
specifically debug info tests.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D105134

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

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM!


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D105134

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

[Lldb-commits] [PATCH] D104914: [lldb] Correct format of qMemTags type field

[David Spickett via Phabricator via lldb-commits]

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

- Remove redundant check on type value
- Explain how we check the target has MTE


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104914

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include 
+#include 
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
@@ -468,19 +469,18 @@
 
 static void
 check_qmemtags(TestClient &client, MockServer &server, size_t read_len,
-   const char *packet, llvm::StringRef response,
+   int32_t type, const char *packet, llvm::StringRef response,
llvm::Optional> expected_tag_data) {
-  const auto &ReadMemoryTags = [&](size_t len, const char *packet,
-   llvm::StringRef response) {
+  const auto &ReadMemoryTags = [&]() {
 std::future result = std::async(std::launch::async, [&] {
-  return client.ReadMemoryTags(0xDEF0, read_len, 1);
+  return client.ReadMemoryTags(0xDEF0, read_len, type);
 });
 
 HandlePacket(server, packet, response);
 return result.get();
   };
 
-  auto result = ReadMemoryTags(0, packet, response);
+  auto result = ReadMemoryTags();
   if (expected_tag_data) {
 ASSERT_TRUE(result);
 llvm::ArrayRef expected(*expected_tag_data);
@@ -493,40 +493,52 @@
 
 TEST_F(GDBRemoteCommunicationClientTest, ReadMemoryTags) {
   // Zero length reads are valid
-  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+  check_qmemtags(client, server, 0, 1, "qMemTags:def0,0:1", "m",
  std::vector{});
 
+  // Type can be negative. Put into the packet as the raw bytes
+  // (as opposed to a literal -1)
+  check_qmemtags(client, server, 0, -1, "qMemTags:def0,0:", "m",
+ std::vector{});
+  check_qmemtags(client, server, 0, std::numeric_limits::min(),
+ "qMemTags:def0,0:8000", "m", std::vector{});
+  check_qmemtags(client, server, 0, std::numeric_limits::max(),
+ "qMemTags:def0,0:7fff", "m", std::vector{});
+
   // The client layer does not check the length of the received data.
   // All we need is the "m" and for the decode to use all of the chars
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09",
+  check_qmemtags(client, server, 32, 2, "qMemTags:def0,20:2", "m09",
  std::vector{0x9});
 
   // Zero length response is fine as long as the "m" is present
-  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+  check_qmemtags(client, server, 0, 0x34, "qMemTags:def0,0:34", "m",
  std::vector{});
 
   // Normal responses
-  check_qmemtags(client, server, 16, "qMemTags:def0,10:1", "m66",
+  check_qmemtags(client, server, 16, 1, "qMemTags:def0,10:1", "m66",
  std::vector{0x66});
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m0102",
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m0102",
  std::vector{0x1, 0x2});
 
   // Empty response is an error
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "", llvm::None);
   // Usual error response
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "E01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "E01",
+ llvm::None);
   // Leading m missing
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "01", llvm::None);
   // Anything other than m is an error
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "z01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "z01",
+ llvm::None);
   // Decoding tag data doesn't use all the chars in the packet
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09zz", llvm::None);
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m09zz",
+ llvm::None);
   // Data that is not hex bytes
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "mhello",
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "mhello",
  llvm::None);
   // Data is not a complete hex char
-  check_qmemtags(client, server, 32, 

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: danielkiss, kristof.beyls.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

In the latest Linux kernels synchronous tag faults
include the tag bits in their address.

Programs must enable the tagged address ABI to
receive these signals and are also opting into the
presence of these tag bits.
(asynchronous faults have no address)

Add logical and allocation tags to the description
of synchronous tag faults.

Process 1626 stopped

- thread #1, name = 'a.out', stop reason = signal SIGSEGV: sync tag check fault 
(fault address: 0x900f7ff9010 logical tag: 0x9 allocation tag: 0x0)

This extends the existing description and will
show as much as it can on the rare occasion something
fails.

This change supports AArch64 MTE only but other
architectures could be added by extending the
switch at the start of AnnotateSyncTagCheckFault.
The rest of the function is generic code.

Tests have been added for synchronous and asynchronous
MTE faults.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105178

Files:
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
  lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
  lldb/test/API/linux/aarch64/mte_tag_faults/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_faults/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/main.c
@@ -0,0 +1,49 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  // Only expect to get the fault type
+  if (argc != 2)
+return 1;
+
+  // Tagged address ABI also enables MTE faults
+  // Allow tags 9 and 10 to be generated
+  unsigned long prctl_arg2 =
+  PR_TAGGED_ADDR_ENABLE | ((3 << 9) << PR_MTE_TAG_SHIFT);
+  if (!strcmp(argv[1], "sync"))
+prctl_arg2 |= PR_MTE_TCF_SYNC;
+  else if (!strcmp(argv[1], "async"))
+prctl_arg2 |= PR_MTE_TCF_ASYNC;
+  else
+return 1;
+
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, prctl_arg2, 0, 0, 0))
+return 1;
+
+  char *buf = mmap(0, sysconf(_SC_PAGESIZE), PROT_MTE | PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // Tag ptr with 10
+  char *tagged_buf = __arm_mte_create_random_tag(buf, 1 << 9);
+  // Set first allocation tag to 10
+  __arm_mte_set_tag(tagged_buf);
+  // Retag the pointer with 9
+  tagged_buf = __arm_mte_create_random_tag(buf, 1 << 10);
+
+  // Breakpoint here
+  // Faults, 9 != 10
+  *(tagged_buf) = '?';
+
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
@@ -0,0 +1,59 @@
+"""
+Test reporting of MTE tag access faults.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagFaultsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_mte_test(self, fault_type):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run {}".format(fault_type), RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_sync(self):
+self.setup_mte_test("sync")
+# The logical tag should be included in the fault address
+# and we know what the bottom byte should be.
+self.expect("continue",
+patterns=[
+"\* thread #1, name = 'a.out', stop reason = signal SIGSEGV: "
+"sync tag check fault \(fault address: 0x9[0-9A-Fa-f]+00\ "
+"logical tag: 0x9 allocation tag: 0xa\)"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_async

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added reviewers: omjavaid, pcc.
DavidSpickett added a subscriber: pcc.
DavidSpickett added a comment.
Herald added a subscriber: JDevlieghere.

@pcc This makes use of the kernel changes you did, let me know if I have 
anything wrong.

Note that the situation where you can't read the allocation tag isn't testable, 
but the code will just show the logical tag in the event it did happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

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

[Lldb-commits] [PATCH] D105179: [lldb][AArch64] Add tag repeat and unpack to memory tag manager

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: danielkiss, kristof.beyls.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

PackTags is used by to compress tags to go in the QMemTags packet
and be passed to ptrace when writing memory tags.

The behaviour of RepeatTagsForRange matches that described for QMemTags
in the GDB documentation:
https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

In addition, unpacking tags with number of tags 0 now means
do not check that number of tags matches the range.
This will be used by lldb-server to unpack tags before repeating
them to fill the requested range.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105179

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
  lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -21,7 +21,7 @@
   manager.UnpackTagsData(input, 2),
   llvm::FailedWithMessage(
   "Packed tag data size does not match expected number of tags. "
-  "Expected 2 tag(s) for 2 granules, got 0 tag(s)."));
+  "Expected 2 tag(s) for 2 granule(s), got 0 tag(s)."));
 
   // This is out of the valid tag range
   input.push_back(0x1f);
@@ -41,6 +41,37 @@
   manager.UnpackTagsData(input, 2);
   ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
   ASSERT_THAT(expected, testing::ContainerEq(*got));
+
+  // Error for too much tag data
+  ASSERT_THAT_EXPECTED(
+  manager.UnpackTagsData(input, 1),
+  llvm::FailedWithMessage(
+  "Packed tag data size does not match expected number of tags. "
+  "Expected 1 tag(s) for 1 granule(s), got 2 tag(s)."));
+
+  // If granules is zero then we don't check number of tags
+  llvm::Expected> got_zero =
+  manager.UnpackTagsData(input, 0);
+  ASSERT_THAT_EXPECTED(got_zero, llvm::Succeeded());
+  ASSERT_THAT(expected, testing::ContainerEq(*got));
+}
+
+TEST(MemoryTagManagerAArch64MTETest, PackTags) {
+  MemoryTagManagerAArch64MTE manager;
+
+  llvm::Expected> invalid_tag_err =
+  manager.PackTags({0x10});
+  ASSERT_THAT_EXPECTED(
+  invalid_tag_err,
+  llvm::FailedWithMessage(
+  "Found tag 0x10 which is > max MTE tag value of 0xf."));
+
+  // 0xf here is the max tag value
+  std::vector tags{0, 1, 0xf};
+  std::vector expected{0, 1, 0xf};
+  llvm::Expected> packed = manager.PackTags(tags);
+  ASSERT_THAT_EXPECTED(packed, llvm::Succeeded());
+  ASSERT_THAT(expected, testing::ContainerEq(*packed));
 }
 
 TEST(MemoryTagManagerAArch64MTETest, GetLogicalTag) {
@@ -118,3 +149,51 @@
   ASSERT_EQ(-32, manager.AddressDiff(0x55114400, 0x44114420));
   ASSERT_EQ(65, manager.AddressDiff(0x99114441, 0x66114400));
 }
+
+static void
+test_repeating_tags(const std::vector &tags,
+MemoryTagManagerAArch64MTE::TagRange range,
+const std::vector &expected_tags) {
+  MemoryTagManagerAArch64MTE manager;
+  llvm::Expected> tags_or_err =
+  manager.RepeatTagsForRange(tags, range);
+  ASSERT_THAT_EXPECTED(tags_or_err, llvm::Succeeded());
+  ASSERT_THAT(expected_tags, testing::ContainerEq(*tags_or_err));
+}
+
+TEST(MemoryTagManagerAArch64MTETest, RepeatTagsForRange) {
+  MemoryTagManagerAArch64MTE manager;
+
+  // Must have some tags if your range is not empty
+  llvm::Expected> no_tags_err =
+  manager.RepeatTagsForRange({},
+ MemoryTagManagerAArch64MTE::TagRange{0, 16});
+  ASSERT_THAT_EXPECTED(
+  no_tags_err, llvm::FailedWithMessage(
+   "Expected some tags to cover given range, got zero."));
+
+  // If the range is empty, you get no tags back
+  test_repeating_tags({1, 2, 3}, MemoryTagManagerAArch64MTE::TagRange{0, 0},
+  {});
+  // And you don't need tags for an empty range
+  test_repeating_tags({}, MemoryTagManagerAArch64MTE::TagRange{0, 0}, {});
+
+  // A single tag will just be multiplied as many times as needed
+  test_repeating_tags({5}, MemoryTagManagerAArch64MTE::TagRange{0, 16}, {5});
+  test_repeating_tags({6}, MemoryTagManagerAArch64MTE::TagRange{0, 32}, {6, 6});
+
+  // If you've got as many tags as granules, it's a roundtrip
+  test_repeating_tags({7, 8}, MemoryTagManagerAArch64MTE::TagRange{0, 32},
+  {7, 8});
+
+  // If you've got fewer tags than granules, they repeat. Exactly or partially
+  // as needed.
+  test_repeating_tags({7, 8}, MemoryTagManagerAArch64MTE::TagRange{0, 64},
+  {7, 8, 

[Lldb-commits] [PATCH] D105180: [lldb][AArch64] Add memory tag writing to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: danielkiss, kristof.beyls.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This is implemented using the QMemTags packet, as specified
by GDB in:
https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

(recall that qMemTags was previously added to read tags)

On receipt of a valid packet lldb-server will:

- align the given address and length to granules (most of the time lldb will 
have already done this but the specification doesn't guarantee it)
- Repeat the supplied tags as many times as needed to cover the range. (if tags 
> range we just use as many as needed)
- Call ptrace POKEMTETAGS to write the tags.

The ptrace step will loop just like the tag read does,
until all tags are written or we get an error.
Meaning that if ptrace succeeds it could be a partial write.
So we call it again and if we then get an error, return an error to
lldb.

We are not going to attempt to restore tags after a partial
write followed by an error. This matches the behaviour of the
existing memory writes.

The lldb-server tests have been extended to include read and
write in the same test file. With some updated function names
since "qMemTags" vs "QMemTags" isn't very clear when they're
next to each other.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105180

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Host/common/NativeProcessProtocol.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py

Index: lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
===
--- lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
+++ lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
@@ -3,27 +3,40 @@
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
+"""
+Check that lldb-server correctly processes qMemTags and QMemTags packets.
+
+In the tests below E03 means the packet wasn't formed correctly
+and E01 means it was but we had some other error acting upon it.
+
+We do not test reading or writing over a page boundary
+within the same mapping. That logic is handled in the kernel
+so it's just a single ptrace call for lldb-server.
+"""
+
 class TestGdbRemoteMemoryTagging(gdbremote_testcase.GdbRemoteTestCaseBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
-def check_qmemtags_response(self, body, expected):
-self.test_sequence.add_log_lines(["read packet: $qMemTags:{}#00".format(body),
+def check_memtags_response(self, packet_name, body, expected):
+self.test_sequence.add_log_lines(["read packet: ${}:{}#00".format(packet_name, body),
   "send packet: ${}#00".format(expected),
   ],
  True)
 self.expect_gdbremote_sequence()
 
-@skipUnlessArch("aarch64")
-@skipUnlessPlatform(["linux"])
-@skipUnlessAArch64MTELinuxCompiler
-def test_qmemtags_packets(self):
-""" Test that qMemTags packets are parsed correctly and/or rejected. """
+def check_tag_read(self, body, expected):
+self.check_memtags_response("qMemTags", body, expected)
 
+def prep_memtags_test(self):
 self.build()
 self.set_inferior_startup_launch()
 procs = self.prep_debug_monitor_and_inferior()
 
+# We don't use isAArch64MTE here because we cannot do runCmd in an
+# lldb-server test. Instead we run the example and skip if it fails
+# to allocate an MTE buffer.
+
 # Run the process
 self.test_sequence.add_log_lines(
 [
@@ -56,61 +69,153 @@
 buf_address = int(buf_address, 16)
 page_size = int(page_size, 16)
 
-# In the tests below E03 means the packet wasn't formed correctly
-# and E01 means it was but we had some other error acting upon it.
+return buf_address, page_size
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_qMemTags_packets(self):
+""" Test that qMemTags packets are parsed correctly and/or rejected. """
+buf_address, page_size = self.prep_memtags_test()
 
 # Sanity check that address is correct
-self.check_qmemtags_response("{:x},20:1".format(buf_address), "m0001")
+self.

[Lldb-commits] [PATCH] D105181: [lldb][AArch64] Add memory tag writing to lldb

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: danielkiss, kristof.beyls.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This adds memory tag writing to Process and the
GDB remote code. Supporting work for the
"memory tag write" command. (to follow)

This command will sometimes only know the start
address and number of tags so a new way to get
the tag manager has been added.

GetMemoryTagManagerForGranules will work out
the range from a start address and some number
of granules.

The alignment behaviour of this method is different
from the existing method. In the case where start is
misaligned, we first align it down then add granules.
As opposed to adding granules first which would result
in an extra granule after alignment.
(this makes more sense for a situation like we
will have in the tag write command)

To save the commands then redoing that alignment
both memory tag manager methods now return a struct
of the range we checked and the manager itself.

(ReadMemoryTags and WriteMemoryTags now also use this
type instead of seperate range arguments)

Process WriteMemoryTags is similair to ReadMemoryTags.
It will pack the tags then call DoWriteMemoryTags.
That function will send the QMemTags packet to the gdb-remote.

The QMemTags packet follows the GDB specification in:
https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

Note that lldb-server will be treating partial writes as
complete failures. So lldb doesn't need to handle the partial
write case in any special way.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105181

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include 
+#include 
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
@@ -530,3 +531,51 @@
   check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m01020",
  llvm::None);
 }
+
+static void check_Qmemtags(TestClient &client, MockServer &server,
+   lldb::addr_t addr, size_t len, int32_t type,
+   const std::vector &tags, const char *packet,
+   llvm::StringRef response, bool should_succeed) {
+  const auto &WriteMemoryTags = [&]() {
+std::future result = std::async(std::launch::async, [&] {
+  return client.WriteMemoryTags(addr, len, type, tags);
+});
+
+HandlePacket(server, packet, response);
+return result.get();
+  };
+
+  auto result = WriteMemoryTags();
+  if (should_succeed)
+ASSERT_TRUE(result.Success());
+  else
+ASSERT_TRUE(result.Fail());
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, WriteMemoryTags) {
+  check_Qmemtags(client, server, 0xABCD, 0x20, 1,
+ std::vector{0x12, 0x34}, "QMemTags:abcd,20:1:1234",
+ "OK", true);
+
+  // The GDB layer doesn't care that the number of tags !=
+  // the length of the write.
+  check_Qmemtags(client, server, 0x4321, 0x20, 9, std::vector{},
+ "QMemTags:4321,20:9:", "OK", true);
+
+  check_Qmemtags(client, server, 0x8877, 0x123, 0x34,
+ std::vector{0x55, 0x66, 0x77},
+ "QMemTags:8877,123:34:556677", "E01", false);
+
+  // Type is a signed integer but is packed as its raw bytes,
+  // instead of having a +/-.
+  check_Qmemtags(client, server, 0x456789, 0, -1, std::vector{0x99},
+ "QMemTags:456789,0::99", "E03", false);
+  check_Qmemtags(client, server, 0x456789, 0,
+ std::numeric_limits::max(),
+ std::vector{0x99}, "QMemTags:456789,0:7fff:99",
+ "E03", false);
+  check_Qmemtags(client, server, 0x456789, 0,
+ std::numeric_limits::min(),
+ std::vector{0x99}, "QMemTags:456789,0:8000:99",
+ "E03", false);
+}
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -6066,8 +6066,45 @@
   return false;
 }
 
-llvm::Expected
+llvm::Expected
 

[Lldb-commits] [PATCH] D105182: [lldb] Add "memory tag write" command

[David Spickett via Phabricator via lldb-commits]

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

This adds a new command for writing memory tags.
It is based on the existing "memory write" command.

Syntax: memory tag write   [ [...]]
(where "value" is a tag value)

(lldb) memory tag write mte_buf 1 2
(lldb) memory tag read mte_buf mte_buf+32
Logical tag: 0x0
Allocation tags:
[0xf7ff9000, 0xf7ff9010): 0x1
[0xf7ff9010, 0xf7ff9020): 0x2

The range you are writing to will be calculated by
aligning the address down to a granule boundary then
adding as many granules as there are tags.

(a repeating mode with an end address will be in a follow
up patch)

This is why "memory tag write" uses GetMemoryTagManagerForGranules.
In contrast to "memory tag read" which knows the end of the
range from its arguments.

The command does all the usual argument validation:

- Address must evaluate
- You must supply at least one tag value (though lldb-server would just treat 
that as a nop anyway)
- Those tag values must be valid for your tagging scheme (e.g. for MTE the 
value must be > 0 and < 0xf)
- The calculated range must be memory tagged

That last error will show you the final range, not just
the start address you gave the command.

(lldb) memory tag write mte_buf_2+page_size-16 6
(lldb) memory tag write mte_buf_2+page_size-16 6 7
error: Address range 0xf7ffaff0:0xf7ffb010 is not in a memory tagged 
region

(note that we do not check if the region is writeable
since lldb can write to it anyway)

The read and write tag tests have been merged into
a single set of "tag access" tests as their test programs would
have been almost identical.
(also I have renamed some of the buffers to better
show what each one is used for)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105182

Files:
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/test/API/functionalities/memory/tag/TestMemoryTag.py
  lldb/test/API/linux/aarch64/mte_tag_access/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
  lldb/test/API/linux/aarch64/mte_tag_access/main.c
  lldb/test/API/linux/aarch64/mte_tag_read/Makefile
  lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
  lldb/test/API/linux/aarch64/mte_tag_read/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
===
--- lldb/test/API/linux/aarch64/mte_tag_read/TestAArch64LinuxMTEMemoryTagRead.py
+++ /dev/null
@@ -1,126 +0,0 @@
-"""
-Test "memory tag read" command on AArch64 Linux with MTE.
-"""
-
-
-import lldb
-from lldbsuite.test.decorators import *
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
-
-
-class AArch64LinuxMTEMemoryTagReadTestCase(TestBase):
-
-mydir = TestBase.compute_mydir(__file__)
-
-NO_DEBUG_INFO_TESTCASE = True
-
-@skipUnlessArch("aarch64")
-@skipUnlessPlatform(["linux"])
-@skipUnlessAArch64MTELinuxCompiler
-def test_mte_tag_read(self):
-if not self.isAArch64MTE():
-self.skipTest('Target must support MTE.')
-
-self.build()
-self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
-
-lldbutil.run_break_set_by_file_and_line(self, "main.c",
-line_number('main.c', '// Breakpoint here'),
-num_expected_locations=1)
-
-self.runCmd("run", RUN_SUCCEEDED)
-
-if self.process().GetState() == lldb.eStateExited:
-self.fail("Test program failed to run.")
-
-self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
-substrs=['stopped',
- 'stop reason = breakpoint'])
-
-# Argument validation
-self.expect("memory tag read",
-substrs=["error: wrong number of arguments; expected at least , "
- "at most  "],
-error=True)
-self.expect("memory tag read buf buf+16 32",
-substrs=["error: wrong number of arguments; expected at least , "
- "at most  "],
-error=True)
-self.expect("memory tag read not_a_symbol",
-substrs=["error: Invalid address expression, address expression \"not_a_symbol\" "
- "evaluation failed"],
-error=True)
-self.expect("memory tag read buf not_a_symbol",
-substrs=["error: Invalid end address expression, address expression \"not_a_symbol\" "
- "evaluation failed"],
-error=True)
-# Inverted range
-self.expect("memory tag read buf buf-16",
-patterns=["error: End address \(0x[A-Fa-f0-9]+\) must be "
-  "greater than the start address \(0x[A-Fa-f0-9]+\)"],
-error=True)
-# Range of len

[Lldb-commits] [PATCH] D105183: [lldb] Add "memory tag write" --end-addr option

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a subscriber: dang.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The default mode of "memory tag write" is to calculate the
range from the start address and the number of tags given.
(just like "memory write" does)

(lldb) memory tag write mte_buf 1 2
(lldb) memory tag read mte_buf mte_buf+48
Logical tag: 0x0
Allocation tags:
[0xf7ff9000, 0xf7ff9010): 0x1
[0xf7ff9010, 0xf7ff9020): 0x2
[0xf7ff9020, 0xf7ff9030): 0x0

This new option allows you to set an end address and have
the tags repeat until that point.

(lldb) memory tag write mte_buf 1 2 --end-addr mte_buf+64
(lldb) memory tag read mte_buf mte_buf+80
Logical tag: 0x0
Allocation tags:
[0xf7ff9000, 0xf7ff9010): 0x1
[0xf7ff9010, 0xf7ff9020): 0x2
[0xf7ff9020, 0xf7ff9030): 0x1
[0xf7ff9030, 0xf7ff9040): 0x2
[0xf7ff9040, 0xf7ff9050): 0x0

This is implemented using the QMemTags packet previously
added. We skip validating the number of tags in lldb and send
them on to lldb-server, which repeats them as needed.

Apart from the number of tags, all the other client side checks
remain. Tag values, memory range must be tagged, etc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105183

Files:
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Commands/Options.td
  
lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py

Index: lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
===
--- lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
+++ lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
@@ -216,3 +216,59 @@
 self.expect("memory tag write mte_buf 99",
 patterns=["error: Found tag 0x63 which is > max MTE tag value of 0xf."],
 error=True)
+
+# You can provide an end address and have lldb repeat the tags as needed
+# The range is checked in the same way it is for "memory tag read"
+self.expect("memory tag write mte_buf 9 -e",
+patterns=["error: last option requires an argument"],
+error=True)
+self.expect("memory tag write mte_buf 9 -e food",
+patterns=["error: address expression \"food\" evaluation failed"],
+error=True)
+self.expect("memory tag write mte_buf_2 9 --end-addr mte_buf_2",
+patterns=["error: End address \(0x[A-Fa-f0-9]+\) must be "
+  "greater than the start address \(0x[A-Fa-f0-9]+\)"],
+error=True)
+self.expect("memory tag write mte_buf_2 9 --end-addr mte_buf_2-16",
+patterns=["error: End address \(0x[A-Fa-f0-9]+\) must be "
+  "greater than the start address \(0x[A-Fa-f0-9]+\)"],
+error=True)
+self.expect("memory tag write mte_buf_2 9 --end-addr mte_buf_2+page_size+16",
+patterns=["error: Address range 0x[0-9A-fa-f]+00:0x[0-9A-Fa-f]+10 "
+  "is not in a memory tagged region"],
+error=True)
+
+# Tags are repeated across the range
+# For these we'll read one extra to make sure we don't over write
+self.expect("memory tag write mte_buf_2 4 5 --end-addr mte_buf_2+48")
+self.expect("memory tag read mte_buf_2 mte_buf_2+64",
+patterns=["Logical tag: 0x0\n"
+  "Allocation tags:\n"
+  "\[0x[0-9A-Fa-f]+00, 0x[0-9A-Fa-f]+10\): 0x4\n"
+  "\[0x[0-9A-Fa-f]+10, 0x[0-9A-Fa-f]+20\): 0x5\n"
+  "\[0x[0-9A-Fa-f]+20, 0x[0-9A-Fa-f]+30\): 0x4\n"
+  "\[0x[0-9A-Fa-f]+30, 0x[0-9A-Fa-f]+40\): 0x0$"])
+
+# Since this aligns like tag read does, the start is aligned down and the end up.
+# Meaning that start/end tells you the start/end granule that will be written.
+# This matters particularly if either are misaligned.
+
+# Here start moves down so the final range is mte_buf_2 -> mte_buf_2+32
+self.expect("memory tag write mte_buf_2+8 6 -end-addr mte_buf_2+32")
+self.expect("memory tag read mte_buf_2 mte_buf_2+48",
+patterns=["Logical tag: 0x0\n"
+  "Allocation tags:\n"
+  "\[0x[0-9A-Fa-f]+00, 0x[0-9A-Fa-f]+10\): 0x6\n"
+  "\[0x[0-9A-Fa-f]+10, 0x[0-9A-Fa-f]+20\): 0x6\n"
+  "\[0x[0-9A-Fa-f]+20, 0x[0-9A-Fa-f]+30\): 0x4$"])
+
+# If we do the same with a misaligned end, it also moves but upward.
+# The intial range is 2 granules but the final range is mte_buf_2 -> mte_buf_2+48
+self.expect("memory tag write mte_buf_2+8 3 -

[Lldb-commits] [PATCH] D103744: [lldb][docs] Remove mention of subversion. NFC.

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added a comment.

seems you forget to commit it, @brucem :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103744

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

[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

[Adam Brouwers-Harries via Phabricator via lldb-commits]

aharries-upmem added a comment.

In D93479#2841049 , @jingham wrote:

> Huh...  I fear you are going to have to debug this further on your end.  I 
> can't see anything else suspect in this patch.

Agreed. Sadly I need to prioritise another task in the short term, but I think 
I'm going to try slowly re-implementing the changes in the patch to see I can 
find a minimal change that triggers it when I have time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

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

[Lldb-commits] [lldb] 0591540 - [lldb] Replace SVE_PT* macros in NativeRegisterContextLinux_arm64.{cpp, h} with their equivalent defintions in LinuxPTraceDefines_arm64sve.h

[Caroline Tice via lldb-commits]

Author: Caroline Tice
Date: 2021-06-30T09:26:20-07:00
New Revision: 05915400b7f9933b95686116f2dc1370e7f96cfb

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

LOG: [lldb] Replace SVE_PT* macros in NativeRegisterContextLinux_arm64.{cpp,h} 
with their equivalent defintions in LinuxPTraceDefines_arm64sve.h

Commit 090306fc80dbf (August 2020) changed most of the arm64 SVE_PT*
macros, but apparently did not make the changes in the
NativeRegisterContextLinux_arm64.* files (or those files were pulled
over from someplace else after that commit). This change replaces the
macros NativeRegisterContextLinux_arm64.cpp with the replacement
definitions in LinuxPTraceDefines_arm64sve.h. It also includes
LinuxPTraceDefines_arm64sve.h in NativeRegisterContextLinux_arm64.h.

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

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
index f78c0d2bb32fe..a0672a635937f 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -58,7 +58,7 @@ 
NativeRegisterContextLinux::CreateHostNativeRegisterContextLinux(
   switch (target_arch.GetMachine()) {
   case llvm::Triple::arm:
 return std::make_unique(target_arch,
- native_thread);
+native_thread);
   case llvm::Triple::aarch64: {
 // Configure register sets supported by this AArch64 target.
 // Read SVE header to check for SVE support.
@@ -207,15 +207,15 @@ NativeRegisterContextLinux_arm64::ReadRegister(const 
RegisterInfo *reg_info,
   if (reg == GetRegisterInfo().GetRegNumFPSR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPSR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPSROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16);
+  offset = sve::ptrace_fpsimd_offset + (32 * 16);
   } else if (reg == GetRegisterInfo().GetRegNumFPCR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPCR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPCROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16) + 4;
+  offset = sve::ptrace_fpsimd_offset + (32 * 16) + 4;
   } else {
 // Extract SVE Z register value register number for this reg_info
 if (reg_info->value_regs &&
@@ -341,15 +341,15 @@ Status NativeRegisterContextLinux_arm64::WriteRegister(
   if (reg == GetRegisterInfo().GetRegNumFPSR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPSR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPSROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16);
+  offset = sve::ptrace_fpsimd_offset + (32 * 16);
   } else if (reg == GetRegisterInfo().GetRegNumFPCR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPCR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPCROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16) + 4;
+  offset = sve::ptrace_fpsimd_offset + (32 * 16) + 4;
   } else {
 // Extract SVE Z register value register number for this reg_info
 if (reg_info->value_regs &&
@@ -824,19 +824,21 @@ void 
NativeRegisterContextLinux_arm64::ConfigureRegisterContext() {
 if (error.Success()) {
   // If SVE is enabled thread can switch between SVEState::FPSIMD and
   // SVEState::Full on every stop.
-  if ((m_sve_header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD)
+  if ((m_sve_header.flags & sve::ptrace_regs_mask) ==
+  sve::ptrace_regs_fpsimd)
 m_sve_state = SVEState::FPSIMD;
-  else if ((m_sve_header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE)
+  else if ((m_sve_header.flags & sve::ptrace_regs_mask) ==
+   sve::ptrace_regs_sve)
 m_sve_state = SVEState::Full;
 
  

[Lldb-commits] [PATCH] D104826: Replace SVE_PT* macros in NativeRegisterContextLinux_arm64.{cpp, h} with their equivalent defintions in LinuxPTraceDefines_arm64sve.h

[Caroline Tice via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG05915400b7f9: [lldb] Replace SVE_PT* macros in 
NativeRegisterContextLinux_arm64.{cpp,h} with… (authored by cmtice).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D104826?vs=354109&id=355592#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104826

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
@@ -12,6 +12,7 @@
 #define lldb_NativeRegisterContextLinux_arm64_h
 
 #include "Plugins/Process/Linux/NativeRegisterContextLinux.h"
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
 #include "Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h"
 #include "Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h"
 
@@ -90,7 +91,7 @@
   m_fpr; // floating-point registers including extended register sets.
 
   SVEState m_sve_state;
-  struct user_sve_header m_sve_header;
+  struct sve::user_sve_header m_sve_header;
   std::vector m_sve_ptrace_payload;
 
   bool m_refresh_hwdebug_info;
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -58,7 +58,7 @@
   switch (target_arch.GetMachine()) {
   case llvm::Triple::arm:
 return std::make_unique(target_arch,
- native_thread);
+native_thread);
   case llvm::Triple::aarch64: {
 // Configure register sets supported by this AArch64 target.
 // Read SVE header to check for SVE support.
@@ -207,15 +207,15 @@
   if (reg == GetRegisterInfo().GetRegNumFPSR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPSR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPSROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16);
+  offset = sve::ptrace_fpsimd_offset + (32 * 16);
   } else if (reg == GetRegisterInfo().GetRegNumFPCR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPCR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPCROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16) + 4;
+  offset = sve::ptrace_fpsimd_offset + (32 * 16) + 4;
   } else {
 // Extract SVE Z register value register number for this reg_info
 if (reg_info->value_regs &&
@@ -341,15 +341,15 @@
   if (reg == GetRegisterInfo().GetRegNumFPSR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPSR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPSROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16);
+  offset = sve::ptrace_fpsimd_offset + (32 * 16);
   } else if (reg == GetRegisterInfo().GetRegNumFPCR()) {
 sve_reg_num = reg;
 if (m_sve_state == SVEState::Full)
-  offset = SVE_PT_SVE_FPCR_OFFSET(sve_vq_from_vl(m_sve_header.vl));
+  offset = sve::PTraceFPCROffset(sve::vq_from_vl(m_sve_header.vl));
 else if (m_sve_state == SVEState::FPSIMD)
-  offset = SVE_PT_FPSIMD_OFFSET + (32 * 16) + 4;
+  offset = sve::ptrace_fpsimd_offset + (32 * 16) + 4;
   } else {
 // Extract SVE Z register value register number for this reg_info
 if (reg_info->value_regs &&
@@ -824,19 +824,21 @@
 if (error.Success()) {
   // If SVE is enabled thread can switch between SVEState::FPSIMD and
   // SVEState::Full on every stop.
-  if ((m_sve_header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD)
+  if ((m_sve_header.flags & sve::ptrace_regs_mask) ==
+  sve::ptrace_regs_fpsimd)
 m_sve_state = SVEState::FPSIMD;
-  else if ((m_sve_header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE)
+  else if ((m_sve_header.flags & sve::ptrace_regs_mask) ==
+   sve::ptrace_regs_

[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D93479#2850104 , @aharries-upmem 
wrote:

> In D93479#2841049 , @jingham wrote:
>
>> Huh...  I fear you are going to have to debug this further on your end.  I 
>> can't see anything else suspect in this patch.
>
> Agreed. Sadly I need to prioritise another task in the short term, but I 
> think I'm going to try slowly re-implementing the changes in the patch to see 
> I can find a minimal change that triggers it when I have time.

Don't hesitate to reach out for questions, explanations, "what the heck"-s once 
you get back to this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

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

[Lldb-commits] [PATCH] D100262: [lldb] [gdb-remote client] Support switching PID along with TID

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:337
 
-  llvm::Optional SendSetCurrentThreadPacket(uint64_t tid, char type);
+  llvm::Optional>
+  SendSetCurrentThreadPacket(uint64_t tid, uint64_t pid, char type);

I generally prefer a struct with named fields over an unnamed pair. Do you 
think that would help readability here?



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:574-575
 
   lldb::pid_t m_curr_pid;
+  lldb::pid_t m_curr_pid_run;
   lldb::tid_t m_curr_tid; // Current gdb remote protocol thread index for all

Maybe a comment how these two are different?


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

https://reviews.llvm.org/D100262

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

[Lldb-commits] [PATCH] D100261: [lldb] [gdb-remote server] Support selecting process via Hg

[Jonas Devlieghere via Phabricator via lldb-commits]

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

I'm probably not the most qualified to sign off on this, but the review has 
stalled and I don't see anything obviously wrong here so LGTM.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2121
+return SendErrorResponse(llvm::make_error(
+inconvertibleErrorCode(), "Malformed thread-id"));
+

Would it be useful to say "Malformed thread-id for process-id {}"? 


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

https://reviews.llvm.org/D100261

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

[Lldb-commits] [PATCH] D93479: [lldb] Simplify the is_finalized logic in process and make it thread safe.

[Adam Brouwers-Harries via Phabricator via lldb-commits]

aharries-upmem added a comment.

> ! In D93479#2850445 , @jingham wrote:
>  Don't hesitate to reach out for questions, explanations, "what the heck"-s 
> once you get back to this.

Thanks Jim, that's very kind of you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93479

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

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

[Alex Langford via Phabricator via lldb-commits]

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

The only remaining plugin dependency in Mangled is CPlusPlusLanguage which it
uses to extract information from C++ mangled names. The static function
GetDemangledNameWithoutArguments is written specifically for C++, so it
would make sense for this specific functionality to live in a
C++-related plugin. In order to keep this functionality in Mangled
without maintaining this dependency, I added
`Language::GetDemangledFunctionNameWithoutArguments`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105215

Files:
  lldb/include/lldb/Target/Language.h
  lldb/source/Core/Mangled.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -106,6 +106,9 @@
 
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
+  ConstString
+  GetDemangledFunctionNameWithoutArguments(Mangled mangled) const override;
+
   static bool IsCPPMangledName(llvm::StringRef name);
 
   // Extract C++ context and identifier from a string using heuristic matching
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -64,6 +64,35 @@
   return mangled_name && CPlusPlusLanguage::IsCPPMangledName(mangled_name);
 }
 
+ConstString CPlusPlusLanguage::GetDemangledFunctionNameWithoutArguments(
+Mangled mangled) const {
+  const char *mangled_name_cstr = mangled.GetMangledName().GetCString();
+  ConstString demangled_name = mangled.GetDemangledName();
+  if (demangled_name && mangled_name_cstr && mangled_name_cstr[0]) {
+if (mangled_name_cstr[0] == '_' && mangled_name_cstr[1] == 'Z' &&
+(mangled_name_cstr[2] != 'T' && // avoid virtual table, VTT structure,
+// typeinfo structure, and typeinfo
+// mangled_name
+ mangled_name_cstr[2] != 'G' && // avoid guard variables
+ mangled_name_cstr[2] != 'Z'))  // named local entities (if we
+// eventually handle eSymbolTypeData,
+// we will want this back)
+{
+  CPlusPlusLanguage::MethodName cxx_method(demangled_name);
+  if (!cxx_method.GetBasename().empty()) {
+std::string shortname;
+if (!cxx_method.GetContext().empty())
+  shortname = cxx_method.GetContext().str() + "::";
+shortname += cxx_method.GetBasename().str();
+return ConstString(shortname);
+  }
+}
+  }
+  if (demangled_name)
+return demangled_name;
+  return mangled.GetMangledName();
+}
+
 // PluginInterface protocol
 
 lldb_private::ConstString CPlusPlusLanguage::GetPluginName() {
Index: lldb/source/Core/Mangled.cpp
===
--- lldb/source/Core/Mangled.cpp
+++ lldb/source/Core/Mangled.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Core/Mangled.h"
 
 #include "lldb/Core/RichManglingContext.h"
+#include "lldb/Target/Language.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
@@ -16,8 +17,6 @@
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-enumerations.h"
 
-#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
-
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/Support/Compiler.h"
@@ -34,35 +33,6 @@
   return Mangled::GetManglingScheme(s) != Mangled::eManglingSchemeNone;
 }
 
-static ConstString GetDemangledNameWithoutArguments(ConstString mangled,
-ConstString demangled) {
-  const char *mangled_name_cstr = mangled.GetCString();
-
-  if (demangled && mangled_name_cstr && mangled_name_cstr[0]) {
-if (mangled_name_cstr[0] == '_' && mangled_name_cstr[1] == 'Z' &&
-(mangled_name_cstr[2] != 'T' && // avoid virtual table, VTT structure,
-// typeinfo structure, and typeinfo
-// mangled_name
- mangled_name_cstr[2] != 'G' && // avoid guard variables
- mangled_name_cstr[2] != 'Z')) // named local entities (if we eventually
-   // handle eSymbolTypeData, we will want
-   // this back)
-{
-  CPlusPlusLanguage::MethodName cxx_method(demangled);
-  if (!cxx_met

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.
Herald added a subscriber: JDevlieghere.

I kind of feel that `Language::GetDemangledFunctionNameWithoutArguments` may be 
a bit too specific for a generalized language plugins. I think it may be worth 
it to make `Mangled` an interface that language plugins can implement (e.g. 
`CPlusPlusMangledName`) but I haven't totally thought out what the 
ramifications of that would be yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105215

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

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D105215#2850821 , @bulbazord wrote:

> I kind of feel that `Language::GetDemangledFunctionNameWithoutArguments` may 
> be a bit too specific for a generalized language plugins. I think it may be 
> worth it to make `Mangled` an interface that language plugins can implement 
> (e.g. `CPlusPlusMangledName`) but I haven't totally thought out what the 
> ramifications of that would be yet.

The name is unfortunate, but the notion that function types have an identifier, 
that is then decorated by arguments and maybe return types, seems pretty 
common.  So in this particular case, maybe we just need a better name?  
GetBaseName isn't right since this function also returns any namespace 
information.  Maybe GetFullyQualifiedBaseName?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105215

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

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 355672.
OmarEmaraDev added a comment.
This revision is now accepted and ready to land.

- Add contextual scrolling support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -71,6 +71,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 using namespace lldb;
@@ -85,6 +86,8 @@
 #define KEY_RETURN 10
 #define KEY_ESCAPE 27
 
+#define KEY_SHIFT_TAB (KEY_MAX + 1)
+
 namespace curses {
 class Menu;
 class MenuDelegate;
@@ -336,93 +339,52 @@
   int m_first_visible_line;
 };
 
-class Window {
+// A surface is an abstraction for something than can be drawn on. The surface
+// have a width, a height, a cursor position, and a multitude of drawing
+// operations. This type should be sub-classed to get an actually useful ncurses
+// object, such as a Window, SubWindow, Pad, or a SubPad.
+class Surface {
 public:
-  Window(const char *name)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(false),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
+  Surface() : m_window(nullptr) {}
 
-  Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(del),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-if (w)
-  Reset(w);
-  }
+  WINDOW *get() { return m_window; }
 
-  Window(const char *name, const Rect &bounds)
-  : m_name(name), m_window(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(true),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
-   bounds.origin.y));
-  }
+  operator WINDOW *() { return m_window; }
 
-  virtual ~Window() {
-RemoveSubWindows();
-Reset();
+  // Copy a region of the surface to another surface.
+  void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
+ Size size) {
+::copywin(m_window, target.get(), source_origin.y, source_origin.x,
+  target_origin.y, target_origin.x,
+  target_origin.y + size.height - 1,
+  target_origin.x + size.width - 1, false);
   }
 
-  void Reset(WINDOW *w = nullptr, bool del = true) {
-if (m_window == w)
-  return;
-
-if (m_panel) {
-  ::del_panel(m_panel);
-  m_panel = nullptr;
-}
-if (m_window && m_delete) {
-  ::delwin(m_window);
-  m_window = nullptr;
-  m_delete = false;
-}
-if (w) {
-  m_window = w;
-  m_panel = ::new_panel(m_window);
-  m_delete = del;
-}
-  }
+  int GetCursorX() const { return getcurx(m_window); }
+  int GetCursorY() const { return getcury(m_window); }
+  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
 
   void AttributeOn(attr_t attr) { ::wattron(m_window, attr); }
   void AttributeOff(attr_t attr) { ::wattroff(m_window, attr); }
-  void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
-::box(m_window, v_char, h_char);
-  }
-  void Clear() { ::wclear(m_window); }
-  void Erase() { ::werase(m_window); }
-  Rect GetBounds() const {
-return Rect(GetParentOrigin(), GetSize());
-  } // Get the rectangle in our parent window
-  int GetChar() { return ::wgetch(m_window); }
-  int GetCursorX() const { return getcurx(m_window); }
-  int GetCursorY() const { return getcury(m_window); }
-  Rect GetFrame() const {
-return Rect(Point(), GetSize());
-  } // Get our rectangle in our own coordinate system
-  Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
-  Size GetSize() const { return Size(GetWidth(), GetHeight()); }
-  int GetParentX() const { return getparx(m_window); }
-  int GetParentY() const { return getpary(m_window); }
+
   int GetMaxX() const { return getmaxx(m_window); }
   int GetMaxY() const { return getmaxy(m_window); }
   int GetWidth() const { return GetMaxX(); }
   int GetHeight() const { return GetMaxY(); }
-  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
-  void MoveWindow(int x, int y) { MoveWindow(Point(x, y)); }
-  void Resize(int w, int h) { ::wresize(m_window, h, w); }
-  void Resize(const Size &size) {
-::wresize(m_window, size.height, size.width);
- 

[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Full example:

F17684763: 20210630-220202.mp4 <https://reviews.llvm.org/F17684763>

Example API definition for the above example:

  class TestFormDelegate : public FormDelegate {
  public:
TestFormDelegate() {
  m_text_field = AddTextField("Text", "The big brown fox.");
  m_file_field = AddFileField("File", "/tmp/a");
  m_directory_field = AddDirectoryField("Directory", "/tmp/");
  m_pid_field = AddIntegerField("Number", 5);
  std::vector choices;
  choices.push_back(std::string("Choice 1"));
  choices.push_back(std::string("Choice 2"));
  choices.push_back(std::string("Choice 3"));
  choices.push_back(std::string("Choice 4"));
  choices.push_back(std::string("Choice 5"));
  m_choices_field = AddChoicesField("Choices", 3, choices);
  m_bool_field = AddBooleanField("Boolean", true);
  TextFieldDelegate default_field =
  TextFieldDelegate("Text", "The big brown fox.");
  m_text_list_field = AddListField("Text List", default_field);
  
  AddAction("Submit", [this](Window &window) { Submit(window); });
}
  
void Submit(Window &window) { SetError("An example error."); }
  
  protected:
TextFieldDelegate *m_text_field;
FileFieldDelegate *m_file_field;
DirectoryFieldDelegate *m_directory_field;
IntegerFieldDelegate *m_pid_field;
BooleanFieldDelegate *m_bool_field;
ChoicesFieldDelegate *m_choices_field;
ListFieldDelegate *m_text_list_field;
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

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

[Lldb-commits] [PATCH] D105166: Fix expression evaluation result expansion in lldb-vscode

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added inline comments.



Comment at: lldb/tools/lldb-vscode/VSCode.cpp:531
 
+void Variables::Clear() {
+  locals.Clear();

clayborg wrote:
> 
I am not sure. I like to call it Clear() in cause we will need to call it from 
other scenario beyond process continue.



Comment at: lldb/tools/lldb-vscode/VSCode.cpp:535
+  registers.Clear();
+  expandable_variables.clear();
+}

clayborg wrote:
> Do we want to set "next_temporary_var_ref" to VARREF_FIRST_VAR_IDX here too? 
> Ok if we don't.
Good question. I guess we can reset it if wanted but not matter much though.



Comment at: lldb/tools/lldb-vscode/VSCode.h:94
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+  llvm::DenseMap expandable_variables;
+  llvm::DenseMap expandable_permanent_variables;

clayborg wrote:
> rename to "temporary_variables" and add a comment
I kind of disagree with this. I think the keyword "expandable" is very 
important for reading. It immediately tells reader that the map only contains 
expandable variables instead of any other variables. 




Comment at: lldb/tools/lldb-vscode/VSCode.h:95
+  llvm::DenseMap expandable_variables;
+  llvm::DenseMap expandable_permanent_variables;
+

clayborg wrote:
> rename to "permanent_variables" and add comment
The same as my comment above.



Comment at: lldb/tools/lldb-vscode/VSCode.h:114
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};

clayborg wrote:
> clear sounds like you are clearing everything in the class. I would name this 
> willContinue() like the others
I am not sure. I like to call it Clear() in cause we will need to call it from 
other scenario beyond process continue. I do not think reader will be confused 
by permanent map is not cleared because the name permanent imply that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166

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

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 355735.
omjavaid added a comment.

This update makes native register context changes restricted to Linux/Arm64. 
Also tweeks test case a little bit.

PS: This test may fail on QEMU. There is QEMU bug that reports wrong watchpoint 
hit address in case top byte of watched address is tagged.


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

https://reviews.llvm.org/D101361

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.cpp
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/watchpoints/watch_tagged_addr/Makefile
  lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
  lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c

Index: lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c
===
--- /dev/null
+++ lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c
@@ -0,0 +1,22 @@
+#include 
+
+uint32_t global_var = 0; // Watchpoint variable declaration.
+
+int main(int argc, char **argv) {
+  int dummy = 0;
+  // Move address of global variable into tagged_ptr after tagging
+  // Simple tagging scheme where 62nd bit of tagged address is set
+  uint32_t *tagged_ptr = (uint32_t *)((uint64_t)&global_var | (1ULL << 62));
+
+  ++dummy; // Set break point at this line.
+
+  // Increment global_var
+  ++global_var;
+
+  ++dummy;
+
+  // Increment global_var using tagged_ptr
+  ++*tagged_ptr;
+
+  return 0;
+}
Index: lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
===
--- /dev/null
+++ lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
@@ -0,0 +1,130 @@
+"""
+Test LLDB can set and hit watchpoints on tagged addresses
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestWatchTaggedAddresses(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+# Set source filename.
+self.source = 'main.c'
+
+# Invoke the default build rule.
+self.build()
+
+# Get the path of the executable
+exe = self.getBuildArtifact("a.out")
+
+# Create a target by the debugger.
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+@skipIf(archs=no_match(["aarch64"]))
+@skipIf(oslist=no_match(['linux']))
+def test_watch_hit_tagged_ptr_access(self):
+"""
+Test that LLDB hits watchpoint installed on an untagged address with
+memory access by a tagged pointer.
+"""
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_symbol(self, 'main')
+
+# Run the program.
+self.runCmd("run", RUN_SUCCEEDED)
+
+# We should be stopped due to the breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Set the watchpoint variable declaration line number.
+self.decl = line_number(self.source,
+'// Watchpoint variable declaration.')
+
+# Now let's set a watchpoint on 'global_var'.
+self.expect(
+"watchpoint set variable global_var",
+WATCHPOINT_CREATED,
+substrs=[
+'Watchpoint created',
+'size = 4',
+'type = w',
+'%s:%d' %
+(self.source,
+ self.decl)])
+
+self.verify_watch_hits()
+
+@skipIf(archs=no_match(["aarch64"]))
+@skipIf(oslist=no_match(['linux']))
+def test_watch_set_on_tagged_ptr(self):
+"""Test that LLDB can install and hit watchpoint on a tagged address"""
+
+# Find the line number to break inside main().
+self.line = line_number(self.source, '// Set break point at this line.')
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_file_and_line(
+self, None, self.line, num_expected_locations=1)
+
+# Run the program.
+self.runCmd("run", RUN_SUCCEEDED)
+
+# We should be stopped due to the breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Now let's set a expression watchpoint on 'tagged_ptr'.
+self.expect(
+"watch