[Lldb-commits] [lldb] [lldb] Omit --show-globals in `help target var` (PR #85855)
https://github.com/bulbazord commented: Big fan of centralizing the options here. I had one question about automating another portion of this, but otherwise LGTM. https://github.com/llvm/llvm-project/pull/85855 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Show module name in progress update for downloading symbols (PR #85342)
https://github.com/bulbazord approved this pull request. +1 to Adrian's suggestion. LGTM since you've addressed that now https://github.com/llvm/llvm-project/pull/85342 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add ability to detect darwin host linker version to xfail tests (PR #83941)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/83941 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add ability to detect darwin host linker version to xfail tests (PR #83941)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/83941
>From 89d1c201636403bb26f12cf9681cbaf86b5c943b Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 4 Mar 2024 14:17:20 -0800
Subject: [PATCH] [lldb] Add ability to detect darwin host linker version to
xfail tests
When Apple released its new linker, it had a subtle bug that caused
LLDB's TLS tests to fail. Unfortunately this means that TLS tests
are not going to work on machines that have affected versions of the
linker, so we should annotate the tests so that they only work when we
are confident the linker has the required fix.
I'm not completely satisfied with this implementation. That being said,
I believe that adding suport for linker versions in general is a
non-trivial change that would require far more thought. There are a few
challenges involved:
- LLDB's testing infra takes an argument to change the compiler, but
there's no way to switch out the linker.
- There's no standard way to ask a compiler what linker it will use.
- There's no standard way to ask a linker what its version is. Many
platforms have the same name for their linker (ld).
- Some platforms automatically switch out the linker underneath you. We
do this for Windows tests (where we use LLD no matter what).
Given that this is affecting the tests on our CI, I think this is an
acceptable solution in the interim.
---
.../Python/lldbsuite/test/lldbplatformutil.py | 27 +++
.../API/lang/c/tls_globals/TestTlsGlobals.py | 1 +
2 files changed, 28 insertions(+)
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index c4d063d3cc77ef..187d16aa1baa68 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -3,6 +3,7 @@
# System modules
import itertools
+import json
import re
import subprocess
import sys
@@ -16,6 +17,7 @@
from . import lldbtest_config
import lldbsuite.test.lldbplatform as lldbplatform
from lldbsuite.test.builders import get_builder
+from lldbsuite.test.lldbutil import is_exe
def check_first_register_readable(test_case):
@@ -333,3 +335,28 @@ def expectedCompiler(compilers):
return True
return False
+
+
+# This is a helper function to determine if a specific version of Xcode's
linker
+# contains a TLS bug. We want to skip TLS tests if they contain this bug, but
+# adding a linker/linker_version conditions to a decorator is challenging due
to
+# the number of ways linkers can enter the build process.
+def xcode15LinkerBug():
+"""Returns true iff a test is running on a darwin platform and the host
linker is between versions 1000 and 1109."""
+darwin_platforms = lldbplatform.translate(lldbplatform.darwin_all)
+if getPlatform() not in darwin_platforms:
+return False
+
+try:
+raw_version_details = subprocess.check_output(
+("xcrun", "ld", "-version_details")
+)
+version_details = json.loads(raw_version_details)
+version = version_details.get("version", "0")
+version_tuple = tuple(int(x) for x in version.split("."))
+if (1000,) <= version_tuple <= (1109,):
+return True
+except:
+pass
+
+return False
diff --git a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
index dfe29b451df0a6..2bffd2eea123a6 100644
--- a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
+++ b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
@@ -40,6 +40,7 @@ def setUp(self):
@skipIfWindows
@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
@skipIf(oslist=no_match([lldbplatformutil.getDarwinOSTriples(), "linux"]))
+@expectedFailureIf(lldbplatformutil.xcode15LinkerBug())
def test(self):
"""Test thread-local storage."""
self.build()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix MSVC build issues (PR #84362)
@@ -168,8 +168,8 @@ class ConstString {
// Implicitly convert \class ConstString instances to \class StringRef.
operator llvm::StringRef() const { return GetStringRef(); }
- // Implicitly convert \class ConstString instances to \class
std::string_view.
- operator std::string_view() const {
+ // Explicitly convert \class ConstString instances to \class
std::string_view.
+ explicit operator std::string_view() const {
bulbazord wrote:
I think that would be nice to do. FWIW, I tried making the StringRef conversion
operator explicit a few months ago and found that it was a pretty large change.
We use the implicitness of the conversion in a lot of places.
https://github.com/llvm/llvm-project/pull/84362
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add ability to detect darwin host linker version to xfail tests (PR #83941)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/83941
>From a72d9d259b441c338399340d630ed7a64c1e228a Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 4 Mar 2024 14:17:20 -0800
Subject: [PATCH] [lldb] Add ability to detect darwin host linker version to
xfail tests
When Apple released its new linker, it had a subtle bug that caused
LLDB's TLS tests to fail. Unfortunately this means that TLS tests
are not going to work on machines that have affected versions of the
linker, so we should annotate the tests so that they only work when we
are confident the linker has the required fix.
I'm not completely satisfied with this implementation. That being said,
I believe that adding suport for linker versions in general is a
non-trivial change that would require far more thought. There are a few
challenges involved:
- LLDB's testing infra takes an argument to change the compiler, but
there's no way to switch out the linker.
- There's no standard way to ask a compiler what linker it will use.
- There's no standard way to ask a linker what its version is. Many
platforms have the same name for their linker (ld).
- Some platforms automatically switch out the linker underneath you. We
do this for Windows tests (where we use LLD no matter what).
Given that this is affecting the tests on our CI, I think this is an
acceptable solution in the interim.
---
.../Python/lldbsuite/test/lldbplatformutil.py | 25 +++
.../API/lang/c/tls_globals/TestTlsGlobals.py | 1 +
2 files changed, 26 insertions(+)
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index c4d063d3cc77ef..03e28fe6ba7795 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -3,6 +3,7 @@
# System modules
import itertools
+import json
import re
import subprocess
import sys
@@ -16,6 +17,7 @@
from . import lldbtest_config
import lldbsuite.test.lldbplatform as lldbplatform
from lldbsuite.test.builders import get_builder
+from lldbsuite.test.lldbutil import is_exe
def check_first_register_readable(test_case):
@@ -333,3 +335,26 @@ def expectedCompiler(compilers):
return True
return False
+
+
+# This is a helper function to determine if a specific version of Xcode's
linker
+# contains a TLS bug. We want to skip TLS tests if they contain this bug, but
+# adding a linker/linker_version conditions to a decorator is challenging due
to
+# the number of ways linkers can enter the build process.
+def xcode15LinkerBug():
+"""Returns true iff a test is running on a darwin platform and the host
linker is between versions 1000 and 1109."""
+darwin_platforms = lldbplatform.translate(lldbplatform.darwin_all)
+if getPlatform() not in darwin_platforms:
+return False
+
+try:
+raw_version_details = subprocess.check_output(("xcrun", "ld",
"-version_details"))
+version_details = json.loads(raw_version_details)
+version = version_details.get("version", "0")
+version_tuple = tuple(int(x) for x in version.split("."))
+if (1000,) <= version_tuple <= (1109,):
+return True
+except:
+pass
+
+return False
diff --git a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
index dfe29b451df0a6..2bffd2eea123a6 100644
--- a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
+++ b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
@@ -40,6 +40,7 @@ def setUp(self):
@skipIfWindows
@skipIf(oslist=["linux"], archs=["arm", "aarch64"])
@skipIf(oslist=no_match([lldbplatformutil.getDarwinOSTriples(), "linux"]))
+@expectedFailureIf(lldbplatformutil.xcode15LinkerBug())
def test(self):
"""Test thread-local storage."""
self.build()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Disable shell tests affected by ld_new bug (PR #84246)
bulbazord wrote: > Called it `ld_new-bug`. If it's not the TLS bug, I wonder if the version > range is correct. The TLS bug in the new linker is explicitly from versions 1000 to 1109. If this isn't related to TLS, we'll need to confirm with the linker folks on our side. https://github.com/llvm/llvm-project/pull/84246 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add ability to detect darwin host linker version to xfail tests (PR #83941)
bulbazord wrote: > To start with option #1, I created #84246 Just took a look, thanks for taking care of that. I'll address your feedback (or just look at what you did in the other PR) and update this tomorrow. https://github.com/llvm/llvm-project/pull/83941 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Disable shell tests affected by ld64 bug (PR #84246)
https://github.com/bulbazord approved this pull request. Thanks for taking care of that! https://github.com/llvm/llvm-project/pull/84246 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Remove unused #includes in ClangModulesDeclVendor.cpp (PR #84262)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/84262 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Change the return type of CalculateNumChildren to uint32_t. (PR #83501)
https://github.com/bulbazord approved this pull request. I'm a little disappointed but I think this is the right step to take. Thanks! https://github.com/llvm/llvm-project/pull/83501 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add ability to detect darwin host linker version to xfail tests (PR #83941)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/83941
When Apple released its new linker, it had a subtle bug that caused LLDB's TLS
tests to fail. Unfortunately this means that TLS tests are not going to work on
machines that have affected versions of the linker, so we should annotate the
tests so that they only work when we are confident the linker has the required
fix.
I'm not completely satisfied with this implementation. That being said, I
believe that adding suport for linker versions in general is a non-trivial
change that would require far more thought. There are a few challenges involved:
- LLDB's testing infra takes an argument to change the compiler, but there's no
way to switch out the linker.
- There's no standard way to ask a compiler what linker it will use.
- There's no standard way to ask a linker what its version is. Many platforms
have the same name for their linker (ld).
- Some platforms automatically switch out the linker underneath you. We do this
for Windows tests (where we use LLD no matter what).
Given that this is affecting the tests on our CI, I think this is an acceptable
solution in the interim.
>From 480252e4b0eba8f631568b6da4e48f657c516576 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 4 Mar 2024 14:17:20 -0800
Subject: [PATCH] [lldb] Add ability to detect darwin host linker version to
xfail tests
When Apple released its new linker, it had a subtle bug that caused
LLDB's TLS tests to fail. Unfortunately this means that TLS tests
are not going to work on machines that have affected versions of the
linker, so we should annotate the tests so that they only work when we
are confident the linker has the required fix.
I'm not completely satisfied with this implementation. That being said,
I believe that adding suport for linker versions in general is a
non-trivial change that would require far more thought. There are a few
challenges involved:
- LLDB's testing infra takes an argument to change the compiler, but
there's no way to switch out the linker.
- There's no standard way to ask a compiler what linker it will use.
- There's no standard way to ask a linker what its version is. Many
platforms have the same name for their linker (ld).
- Some platforms automatically switch out the linker underneath you. We
do this for Windows tests (where we use LLD no matter what).
Given that this is affecting the tests on our CI, I think this is an
acceptable solution in the interim.
---
.../Python/lldbsuite/test/lldbplatformutil.py | 40 +++
.../API/lang/c/tls_globals/TestTlsGlobals.py | 1 +
2 files changed, 41 insertions(+)
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index c4d063d3cc77ef..acb1a801e7f638 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -3,6 +3,7 @@
# System modules
import itertools
+import json
import re
import subprocess
import sys
@@ -16,6 +17,7 @@
from . import lldbtest_config
import lldbsuite.test.lldbplatform as lldbplatform
from lldbsuite.test.builders import get_builder
+from lldbsuite.test.lldbutil import is_exe
def check_first_register_readable(test_case):
@@ -333,3 +335,41 @@ def expectedCompiler(compilers):
return True
return False
+
+
+# This is a helper function to determine if a specific version of Xcode's
linker
+# contains a TLS bug. We want to skip TLS tests if they contain this bug, but
+# adding a linker/linker_version conditions to a decorator is challenging due
to
+# the number of ways linkers can enter the build process.
+def darwinLinkerHasTLSBug():
+"""Returns true iff a test is running on a darwin platform and the host
linker is between versions 1000 and 1109."""
+darwin_platforms = lldbplatform.translate(lldbplatform.darwin_all)
+if getPlatform() not in darwin_platforms:
+return False
+
+linker_path = (
+subprocess.check_output(["xcrun", "--find",
"ld"]).rstrip().decode("utf-8")
+)
+if not is_exe(linker_path):
+return False
+
+raw_linker_info = (
+subprocess.check_output([linker_path, "-version_details"])
+.rstrip()
+.decode("utf-8")
+)
+parsed_linker_info = json.loads(raw_linker_info)
+if "version" not in parsed_linker_info:
+return False
+
+raw_version = parsed_linker_info["version"]
+version = None
+try:
+version = int(raw_version)
+except:
+return False
+
+if version is None:
+return False
+
+return 1000 <= version and version <= 1109
diff --git a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
index dfe29b451df0a6..526e49b68162f3 100644
--- a/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
+++ b/lldb/test/API/lang/c/tls_globals/TestTlsGlobals.py
@@ -40,6
[Lldb-commits] [lldb] Change the return type of CalculateNumChildren to uint32_t. (PR #83501)
bulbazord wrote: > I would almost vote to change everything to `uint64_t` except for the public > API since we can't change the API without breaking. Though I winder if we can > actually change this one: > > ``` > uint64_t SBValue::GetNumChildren(); > ``` > > Since the return value isn't mangled into the function name? > > The reason I mention the uint64_t is `lldb::SBValue` and > `lldb_private::ValueObject*` can represent _any_ object that can be expanded. > We could have a `lldb::SBValue` that represents all of memory in a process > where each object can represent an area in memory as a specific format and > size. So a 64 bit process _could_ have a `SBValue` with `UINT64_MAX` children > available if we wanted to have a `SBValue` that represented memory as > `uint8_t` objects. > > So if we are going to change stuff around, I would vote to use `uint64_t` > instead of `uint32_t` Changing the type of a return value if it changes the size/layout is ABI breaking. The mangled name wouldn't change but it may silently break something later without us realizing. https://github.com/llvm/llvm-project/pull/83501 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Change the return type of CalculateNumChildren to uint32_t. (PR #83501)
bulbazord wrote: Why not increase TypeSystem::GetNumChildren to return a size_t instead? https://github.com/llvm/llvm-project/pull/83501 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported (PR #82593)
https://github.com/bulbazord approved this pull request. Looks great!!! Thanks for fixing the bug and gardening! https://github.com/llvm/llvm-project/pull/82593 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][progress][NFC] Fix Doxygen information (PR #83502)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/83502 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test][NFC] Add option to exclude third_party packages (PR #83191)
bulbazord wrote: @DavidSpickett If you haven't already, might I suggest looking into a CMake setting I implemented last year? `LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS` -- this will cause your CMake configure step to fail if your bots are missing python packages so that you know your bots are running in a reasonable state. :) https://github.com/llvm/llvm-project/pull/83191 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Ignore swig warnings about shadowed overloads (PR #83317)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/83317 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix interactive use of "command script add". (PR #83350)
https://github.com/bulbazord approved this pull request. Thinkos are the most insidious bugs. Nice catch. https://github.com/llvm/llvm-project/pull/83350 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Change `image list -r` so that it actually shows the ref count and whether the image is in the shared cache. (PR #83341)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/83341 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Remove -d(ebug) mode from the lldb driver (PR #83330)
https://github.com/bulbazord approved this pull request. :) https://github.com/llvm/llvm-project/pull/83330 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Ignore swig warnings about shadowed overloads (PR #83317)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/83317
This specifically addresses the warnings:
$LLVM/lldb/include/lldb/API/SBCommandReturnObject.h:119: Warning 509:
Overloaded method lldb::SBCommandReturnObject::PutCString(char const *)
effectively ignored, $LLVM/lldb/include/lldb/API/SBCommandReturnObject.h:119:
Warning 509: as it is shadowed by lldb::SBCommandReturnObject::PutCString(char
const *,int).
There is exactly one declaration of SBCommandReturnObject::PutCString. The
second parameter (of type `int`) has default value `-1`. Without investigating
why SWIG believes there are 2 method declarations, I believe it is safe to
ignore this warning. It does not appear to actually impact functionality in any
way.
rdar://117744660
>From 76a634a1ad00e90983391cdd04588f92b5f15432 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Wed, 28 Feb 2024 10:58:33 -0800
Subject: [PATCH] [lldb] Ignore swig warnings about shadowed overloads
This specifically addresses the warnings:
$LLVM/lldb/include/lldb/API/SBCommandReturnObject.h:119: Warning 509:
Overloaded method lldb::SBCommandReturnObject::PutCString(char const *)
effectively ignored,
$LLVM/lldb/include/lldb/API/SBCommandReturnObject.h:119: Warning 509: as it is
shadowed by lldb::SBCommandReturnObject::PutCString(char const *,int).
There is exactly one declaration of SBCommandReturnObject::PutCString.
The second parameter (of type `int`) has default value `-1`. Without
investigating why SWIG believes there are 2 method declarations, I
believe it is safe to ignore this warning. It does not appear to
actually impact functionality in any way.
rdar://117744660
---
lldb/bindings/CMakeLists.txt | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lldb/bindings/CMakeLists.txt b/lldb/bindings/CMakeLists.txt
index b44ed59aa662b2..296eae1ae77f86 100644
--- a/lldb/bindings/CMakeLists.txt
+++ b/lldb/bindings/CMakeLists.txt
@@ -23,7 +23,11 @@ endif()
set(SWIG_COMMON_FLAGS
-c++
- -w361,362 # Ignore warnings about ignored operator overloads
+ # Ignored warnings:
+ # 361: operator! ignored.
+ # 362: operator= ignored.
+ # 509: Overloaded method declaration effectively ignored, shadowed by
previous declaration.
+ -w361,362,509
-features autodoc
-I${LLDB_SOURCE_DIR}/include
-I${CMAKE_CURRENT_SOURCE_DIR}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test] Exclude third_party packages by default (PR #83191)
https://github.com/bulbazord approved this pull request. LGTM @JDevlieghere @adrian-prantl we should consider adding this to the list of required python modules on our bots too and enforce it with `LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS`? Probably not in this PR, but after we can get our bots configured correctly. https://github.com/llvm/llvm-project/pull/83191 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Use CreateOptionParsingError in CommandObjectBreakpoint (PR #83086)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/83086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Use CreateOptionParsingError in CommandObjectBreakpoint (PR #83086)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/83086
This updates the remaining SetOptionValue methods in CommandObjectBreakpoint to
use CreateOptionParsingError.
I found a few minor bugs that were fixed during this refactor (e.g. using the
wrong flag in an error message). That is one of the benefits of centralizing
error message creation.
I also found some option parsing code that is written incorrectly. I do not
make an attempt to update those here because this PR is primarily about
changing existing error handling code, not adding new error handling code.
>From feb4588cd12a5f513061dffa99f6370067f91463 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 26 Feb 2024 16:01:24 -0800
Subject: [PATCH] [lldb] Use CreateOptionParsingError in
CommandObjectBreakpoint
This updates the remaining SetOptionValue methods in
CommandObjectBreakpoint to use CreateOptionParsingError.
I found a few minor bugs that were fixed during this refactor (e.g.
using the wrong flag in an error message). That is one of the benefits
of centralizing error message creation.
I also found some option parsing code that is written incorrectly. I do
not make an attempt to update those here because this PR is primarily
about changing existing error handling code, not adding new error
handling code.
---
lldb/include/lldb/Interpreter/Options.h | 2 +
.../Commands/CommandObjectBreakpoint.cpp | 101 +-
2 files changed, 54 insertions(+), 49 deletions(-)
diff --git a/lldb/include/lldb/Interpreter/Options.h
b/lldb/include/lldb/Interpreter/Options.h
index 18a87e49deee5c..9a6a17c2793fa4 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -368,6 +368,8 @@ static constexpr llvm::StringLiteral
g_bool_parsing_error_message =
"Failed to parse as boolean";
static constexpr llvm::StringLiteral g_int_parsing_error_message =
"Failed to parse as integer";
+static constexpr llvm::StringLiteral g_language_parsing_error_message =
+"Unknown language";
} // namespace lldb_private
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp
b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index fc2217608a0bb9..cfb0b87a59e640 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -266,6 +266,8 @@ class CommandObjectBreakpointSet : public
CommandObjectParsed {
Status error;
const int short_option =
g_breakpoint_set_options[option_idx].short_option;
+ const char *long_option =
+ g_breakpoint_set_options[option_idx].long_option;
switch (short_option) {
case 'a': {
@@ -284,13 +286,15 @@ class CommandObjectBreakpointSet : public
CommandObjectParsed {
case 'u':
if (option_arg.getAsInteger(0, m_column))
- error.SetErrorStringWithFormat("invalid column number: %s",
- option_arg.str().c_str());
+ error =
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message);
break;
case 'E': {
LanguageType language =
Language::GetLanguageTypeFromString(option_arg);
+llvm::StringRef error_context;
switch (language) {
case eLanguageTypeC89:
case eLanguageTypeC:
@@ -308,19 +312,18 @@ class CommandObjectBreakpointSet : public
CommandObjectParsed {
m_exception_language = eLanguageTypeObjC;
break;
case eLanguageTypeObjC_plus_plus:
- error.SetErrorStringWithFormat(
- "Set exception breakpoints separately for c++ and objective-c");
+ error_context =
+ "Set exception breakpoints separately for c++ and objective-c";
break;
case eLanguageTypeUnknown:
- error.SetErrorStringWithFormat(
- "Unknown language type: '%s' for exception breakpoint",
- option_arg.str().c_str());
+ error_context = "Unknown language type for exception breakpoint";
break;
default:
- error.SetErrorStringWithFormat(
- "Unsupported language type: '%s' for exception breakpoint",
- option_arg.str().c_str());
+ error_context = "Unsupported language type for exception breakpoint";
}
+if (!error_context.empty())
+ error = CreateOptionParsingError(option_arg, short_option,
+ long_option, error_context);
} break;
case 'f':
@@ -336,9 +339,9 @@ class CommandObjectBreakpointSet : public
CommandObjectParsed {
bool success;
m_catch_bp = OptionArgParser::ToBoolean(option_arg, true, );
if (!success)
- error.SetErrorStringWithFormat(
- "Invalid boolean value for on-catch option: '%s'",
-
[Lldb-commits] [lldb] [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported (PR #82593)
@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
// The order of these checks is important.
if (process_does_not_exist (pid_attaching_to)) {
DNBLogError("Tried to attach to pid that doesn't exist");
- std::string return_message = "E96;";
- return_message += cstring_to_asciihex_string("no such process.");
+ std::string return_message = "E96";
bulbazord wrote:
Sounds great to me
https://github.com/llvm/llvm-project/pull/82593
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][docs] Remove/update docs pointing to unittest2 (PR #82672)
https://github.com/bulbazord approved this pull request. LGTM! https://github.com/llvm/llvm-project/pull/82672 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test] Remove vendored packages `unittest2` and `progress` (PR #82670)
https://github.com/bulbazord approved this pull request. 拾 Please wait a little bit for others to look. Thanks for taking care of this! https://github.com/llvm/llvm-project/pull/82670 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported (PR #82593)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/82593 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported (PR #82593)
@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
// The order of these checks is important.
if (process_does_not_exist (pid_attaching_to)) {
DNBLogError("Tried to attach to pid that doesn't exist");
- std::string return_message = "E96;";
- return_message += cstring_to_asciihex_string("no such process.");
+ std::string return_message = "E96";
bulbazord wrote:
I see this pattern in lots of places, is it worth abstracting out? Something
like:
```
std::string CreateAttachError(const char *additional_context = nullptr) {
std::string message = "E96";
if (additional_context)
message += ";" + cstring_to_asciihex_string(additional_context);
return message;
}
```
What do you think?
https://github.com/llvm/llvm-project/pull/82593
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported (PR #82593)
https://github.com/bulbazord approved this pull request. LGTM, seems like a good fix. One question/suggestion. https://github.com/llvm/llvm-project/pull/82593 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/82273 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test] Modernize assertEqual(value, bool) (PR #82526)
https://github.com/bulbazord approved this pull request. Thanks! https://github.com/llvm/llvm-project/pull/82526 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
bulbazord wrote: I've added a doxygen comment to the new function I introduced. I plan on landing this later today if there are no objections. https://github.com/llvm/llvm-project/pull/82273 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/82273
>From 790810f9318c7947fe2edd187f60425a85c949b5 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Thu, 15 Feb 2024 17:39:42 -0800
Subject: [PATCH 1/2] [lldb] Standardize command option parsing error messages
I have been looking to simplify parsing logic and improve the interfaces
so that they are both easier to use and harder to abuse. To be specific,
I am referring to functions such as `OptionArgParser::ToBoolean`: I
would like to go from its current interface to something more like
`llvm::Error ToBoolean(llvm::StringRef option_arg)`.
Through working on that, I encountered 2 inconveniences:
1. Option parsing code is not uniform. Every function writes a slightly
different error message, so incorporating an error message from the
`ToBoolean` implementation is going to be laborious as I figure out
what exactly needs to change or stay the same.
2. Changing the interface of `ToBoolean` would require a global atomic
change across all of the Command code. This would be quite
frustrating to do because of the non-uniformity of our existing code.
To address these frustrations, I think it would be easiest to first
standardize the error reporting mechanism when parsing options in
commands. I do so by introducing `CreateOptionParsingError` which will
create an error message of the shape:
Invalid valud ('${option_arg}') for -${short_value} ('${long_value}'):
${additional_context}
Concretely, it would look something like this:
(lldb) breakpoint set -n main -G yay
error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean
After this, updating the interfaces for parsing the values themselves
should become simpler. Because this can be adopted incrementally, this
should be able to done over the course of time instead of all at once as
a giant difficult-to-review change. I've changed exactly one function
where this function would be used as an illustration of what I am
proposing.
---
lldb/include/lldb/Interpreter/Options.h | 10 +
.../Commands/CommandObjectBreakpoint.cpp | 37 ++-
lldb/source/Interpreter/Options.cpp | 13 +++
lldb/unittests/Interpreter/CMakeLists.txt | 1 +
lldb/unittests/Interpreter/TestOptions.cpp| 29 +++
5 files changed, 73 insertions(+), 17 deletions(-)
create mode 100644 lldb/unittests/Interpreter/TestOptions.cpp
diff --git a/lldb/include/lldb/Interpreter/Options.h
b/lldb/include/lldb/Interpreter/Options.h
index bf74927cf99db8..3351fb517d4df3 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -336,6 +336,16 @@ class OptionGroupOptions : public Options {
bool m_did_finalize = false;
};
+llvm::Error CreateOptionParsingError(llvm::StringRef option_arg,
+ const char short_option,
+ llvm::StringRef long_option = {},
+ llvm::StringRef additional_context = {});
+
+static constexpr llvm::StringLiteral g_bool_parsing_error_message =
+"Failed to parse as boolean";
+static constexpr llvm::StringLiteral g_int_parsing_error_message =
+"Failed to parse as integer";
+
} // namespace lldb_private
#endif // LLDB_INTERPRETER_OPTIONS_H
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp
b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 3fdf5cd3cd43d2..fc2217608a0bb9 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -64,6 +64,8 @@ class lldb_private::BreakpointOptionGroup : public
OptionGroup {
Status error;
const int short_option =
g_breakpoint_modify_options[option_idx].short_option;
+const char *long_option =
+g_breakpoint_modify_options[option_idx].long_option;
switch (short_option) {
case 'c':
@@ -84,18 +86,17 @@ class lldb_private::BreakpointOptionGroup : public
OptionGroup {
case 'G': {
bool value, success;
value = OptionArgParser::ToBoolean(option_arg, false, );
- if (success) {
+ if (success)
m_bp_opts.SetAutoContinue(value);
- } else
-error.SetErrorStringWithFormat(
-"invalid boolean value '%s' passed for -G option",
-option_arg.str().c_str());
+ else
+error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message);
} break;
case 'i': {
uint32_t ignore_count;
if (option_arg.getAsInteger(0, ignore_count))
-error.SetErrorStringWithFormat("invalid ignore count '%s'",
- option_arg.str().c_str());
+error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message);
else
[Lldb-commits] [lldb] [lldb][test] Modernize asserts (PR #82503)
@@ -456,8 +457,9 @@ def queues_with_libBacktraceRecording(self): "doing_the_work_2", "queue 2's pending item #0 should be doing_the_work_2", ) -self.assertTrue( -queue_performer_2.GetPendingItemAtIndex().IsValid() == False, +self.assertEqual( +queue_performer_2.GetPendingItemAtIndex().IsValid(), bulbazord wrote: Might be useful to do an additional pass over these later. I think we could use `assertFalse` here, for example. https://github.com/llvm/llvm-project/pull/82503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test] Modernize asserts (PR #82503)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/82503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][test] Modernize asserts (PR #82503)
https://github.com/bulbazord approved this pull request. I did a quick manual inspection of all of them, the transformation produced by Teyit looks correct to me. I did notice a few places where we were doing convoluted checks (I left a comment on one) but those should be addressed in follow-ups I think. https://github.com/llvm/llvm-project/pull/82503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
bulbazord wrote: > > > This LGTM! > > > I don't think I can see far enough ahead on what you are planning here, > > > but I was just wondering if the ultimate goal is to have the > > > `option_arg.getAsT` return an `Expected`. In this case, wouldn't all > > > these arguments (short option, long option, additional context) have to > > > be part of the interface of `getAsT`? I suspect this is not your goal, > > > but I can't see a way around that > > > > > > The ultimate goal is to have option parsing be much more streamlined and > > automated. The majority of option parsing is just taking some primitive > > value and trying to grab its value from a string. There are places where we > > need additional context (e.g. in this PR, there's a place where we want to > > select a thread ID so there's more validation needed). I want to be able to > > write those pretty naturally too. > > The first step in my plan is to centralize how we create error messages. > > The majority of parsing errors will be "I couldn't turn this into a > > boolean/integer/address/other", so I'm going to create something that is > > better than what we have today. The step after that will be to revamp the > > parsing step. Ideally instead of writing `OptionArgParser::ToBoolean` the > > way we do today, there will be something like `llvm::Expected > > value_or_error = ParseAsBool(option);` where `option` is the actual > > OptionDefinition. I'm not tied to these abstractions, but I am committed to > > the path of making this easier to write and maintain. > > Ohhh I think I see it now! > > > where `option` is the actual OptionDefinition > > And the `option` itself would know how to report / construct the error > strings? I would like it to be that way. Let's see if it works out! https://github.com/llvm/llvm-project/pull/82273 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
bulbazord wrote: > This LGTM! > > I don't think I can see far enough ahead on what you are planning here, but I > was just wondering if the ultimate goal is to have the `option_arg.getAsT` > return an `Expected`. In this case, wouldn't all these arguments (short > option, long option, additional context) have to be part of the interface of > `getAsT`? I suspect this is not your goal, but I can't see a way around that The ultimate goal is to have option parsing be much more streamlined and automated. The majority of option parsing is just taking some primitive value and trying to grab its value from a string. There are places where we need additional context (e.g. in this PR, there's a place where we want to select a thread ID so there's more validation needed). I want to be able to write those pretty naturally too. The first step in my plan is to centralize how we create error messages. The majority of parsing errors will be "I couldn't turn this into a boolean/integer/address/other", so I'm going to create something that is better than what we have today. The step after that will be to revamp the parsing step. Ideally instead of writing `OptionArgParser::ToBoolean` the way we do today, there will be something like `llvm::Expected value_or_error = ParseAsBool(option);` where `option` is the actual OptionDefinition. I'm not tied to these abstractions, but I am committed to the path of making this easier to write and maintain. https://github.com/llvm/llvm-project/pull/82273 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Centralize the handling of completion for simple argument lists. (PR #82085)
https://github.com/bulbazord approved this pull request. LGTM, thanks! https://github.com/llvm/llvm-project/pull/82085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Standardize command option parsing error messages (PR #82273)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/82273
I have been looking to simplify parsing logic and improve the interfaces so
that they are both easier to use and harder to abuse. To be specific, I am
referring to functions such as `OptionArgParser::ToBoolean`: I would like to go
from its current interface to something more like `llvm::Error
ToBoolean(llvm::StringRef option_arg)`.
Through working on that, I encountered 2 inconveniences:
1. Option parsing code is not uniform. Every function writes a slightly
different error message, so incorporating an error message from the `ToBoolean`
implementation is going to be laborious as I figure out what exactly needs to
change or stay the same.
2. Changing the interface of `ToBoolean` would require a global atomic change
across all of the Command code. This would be quite frustrating to do because
of the non-uniformity of our existing code.
To address these frustrations, I think it would be easiest to first standardize
the error reporting mechanism when parsing options in commands. I do so by
introducing `CreateOptionParsingError` which will create an error message of
the shape:
Invalid valud ('${option_arg}') for -${short_value} ('${long_value}'):
${additional_context}
Concretely, it would look something like this:
(lldb) breakpoint set -n main -G yay
error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean
After this, updating the interfaces for parsing the values themselves should
become simpler. Because this can be adopted incrementally, this should be able
to done over the course of time instead of all at once as a giant
difficult-to-review change. I've changed exactly one function where this
function would be used as an illustration of what I am proposing.
>From 062ec05006182838c8b0043051741ae6c26c2520 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Thu, 15 Feb 2024 17:39:42 -0800
Subject: [PATCH] [lldb] Standardize command option parsing error messages
I have been looking to simplify parsing logic and improve the interfaces
so that they are both easier to use and harder to abuse. To be specific,
I am referring to functions such as `OptionArgParser::ToBoolean`: I
would like to go from its current interface to something more like
`llvm::Error ToBoolean(llvm::StringRef option_arg)`.
Through working on that, I encountered 2 inconveniences:
1. Option parsing code is not uniform. Every function writes a slightly
different error message, so incorporating an error message from the
`ToBoolean` implementation is going to be laborious as I figure out
what exactly needs to change or stay the same.
2. Changing the interface of `ToBoolean` would require a global atomic
change across all of the Command code. This would be quite
frustrating to do because of the non-uniformity of our existing code.
To address these frustrations, I think it would be easiest to first
standardize the error reporting mechanism when parsing options in
commands. I do so by introducing `CreateOptionParsingError` which will
create an error message of the shape:
Invalid valud ('${option_arg}') for -${short_value} ('${long_value}'):
${additional_context}
Concretely, it would look something like this:
(lldb) breakpoint set -n main -G yay
error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean
After this, updating the interfaces for parsing the values themselves
should become simpler. Because this can be adopted incrementally, this
should be able to done over the course of time instead of all at once as
a giant difficult-to-review change. I've changed exactly one function
where this function would be used as an illustration of what I am
proposing.
---
lldb/include/lldb/Interpreter/Options.h | 10 +
.../Commands/CommandObjectBreakpoint.cpp | 37 ++-
lldb/source/Interpreter/Options.cpp | 13 +++
lldb/unittests/Interpreter/CMakeLists.txt | 1 +
lldb/unittests/Interpreter/TestOptions.cpp| 29 +++
5 files changed, 73 insertions(+), 17 deletions(-)
create mode 100644 lldb/unittests/Interpreter/TestOptions.cpp
diff --git a/lldb/include/lldb/Interpreter/Options.h
b/lldb/include/lldb/Interpreter/Options.h
index bf74927cf99db8..3351fb517d4df3 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -336,6 +336,16 @@ class OptionGroupOptions : public Options {
bool m_did_finalize = false;
};
+llvm::Error CreateOptionParsingError(llvm::StringRef option_arg,
+ const char short_option,
+ llvm::StringRef long_option = {},
+ llvm::StringRef additional_context = {});
+
+static constexpr llvm::StringLiteral g_bool_parsing_error_message =
+"Failed to parse as boolean";
+static constexpr llvm::StringLiteral g_int_parsing_error_message =
+"Failed to parse as
[Lldb-commits] [lldb] [lldb] Don't rely on ScriptInterpreterPythonImpl::Initialize in the unit tests (PR #82096)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/82096 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't rely on ScriptInterpreterPythonImpl::Initialize in the unit tests (PR #82096)
bulbazord wrote: > The downside of doing the initialization manually is that we do lose a bit of > test coverage. For example, issue #70453 also manifested itself in the unit > tests. I think this is an acceptable tradeoff. The unit tests are for testing LLDB's python internals, not for testing LLDB's python interface and integration. https://github.com/llvm/llvm-project/pull/82096 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Call Import_AppendInittab exactly once before Py_Initialize (PR #82095)
https://github.com/bulbazord approved this pull request. Makes sense to me. https://github.com/llvm/llvm-project/pull/82095 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Centralize the handling of completion for simple argument lists. (PR #82085)
@@ -1139,6 +1097,15 @@ class CommandObjectPlatformProcessLaunch : public
CommandObjectParsed {
m_arguments.push_back({run_arg_arg});
}
+ void
+ HandleArgumentCompletion(CompletionRequest ,
+ OptionElementVector _element_vector) override {
bulbazord wrote:
Any way you could move this to the generic CommandObject implementation?
https://github.com/llvm/llvm-project/pull/82085
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Centralize the handling of completion for simple argument lists. (PR #82085)
@@ -305,6 +305,42 @@ void CommandObject::HandleCompletion(CompletionRequest
) {
}
}
+void
+CommandObject::HandleArgumentCompletion(CompletionRequest ,
+ OptionElementVector _element_vector) {
+ size_t num_arg_entries = GetNumArgumentEntries();
+ if (num_arg_entries != 1)
+return;
+
+ CommandArgumentEntry *entry_ptr = GetArgumentEntryAtIndex(0);
+ if (!entry_ptr)
+return; // Maybe this should be an assert, this shouldn't be possible.
bulbazord wrote:
If it shouldn't be possible, I think an assert is warranted here. I would say
add the assertion above the early return (and keep the early return since the
assertion will be compiled out in release builds).
https://github.com/llvm/llvm-project/pull/82085
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Centralize the handling of completion for simple argument lists. (PR #82085)
@@ -243,7 +243,7 @@ static constexpr CommandObject::ArgumentTableEntry
g_argument_table[] = {
{ lldb::eArgTypeLogCategory, "log-category",
lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "The name of a
category within a log channel, e.g. all (try \"log list\" to see a list of all
channels and their categories." },
{ lldb::eArgTypeLogChannel, "log-channel",
lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "The name of a log
channel, e.g. process.gdb-remote (try \"log list\" to see a list of all
channels and their categories)." },
{ lldb::eArgTypeMethod, "method", lldb::CompletionType::eNoCompletion, {},
{ nullptr, false }, "A C++ method name." },
-{ lldb::eArgTypeName, "name", lldb::eTypeCategoryNameCompletion, {}, {
nullptr, false }, "Help text goes here." },
bulbazord wrote:
Help text DOES go here! :)
https://github.com/llvm/llvm-project/pull/82085
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
bulbazord wrote: > @clayborg @bulbazord we have extension in lldb to support cobol code > debugging, we require byteswapping there. upstream version of lldb does not > do byteswapping on scalar so no problem seen. Maybe not, but this should still be testable with a unit test. If somebody modifies or refactors this code in the future without a test, this might break in a similar way and you would have to do all this work again. :) https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
https://github.com/bulbazord commented: Idea looks ok, the naming could use a bit of work. Right now it feels a bit generic and I'm not sure what "loading" means. I like Greg's suggestion here. https://github.com/llvm/llvm-project/pull/81706 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Report only loaded debug info in statistics dump (PR #81706)
@@ -224,6 +224,7 @@ llvm::json::Value DebuggerStats::ReportStatistics(
const lldb_private::StatisticsOptions ) {
const bool summary_only = options.summary_only;
+ const bool force_laoding = options.force_loading;
bulbazord wrote:
typo in this one
https://github.com/llvm/llvm-project/pull/81706
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
@@ -1666,33 +1646,52 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = {};
+std::optional pass_action = {};
+std::optional notify_action = {};
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
- result.AppendError("Invalid argument for command option --stop; must be "
- "true or false.\n");
- return;
+if (!m_options.stop.empty()) {
+ bool success = false;
+ bool value = OptionArgParser::ToBoolean(m_options.stop, false, );
bulbazord wrote:
My plan (that I wrote up 2 weeks ago) was to remove `VerifyCommandOptionValue`
entirely in this commit and then refactor `ToBoolean` the way you suggested as
a follow-up to this change. Does that sound good to you?
https://github.com/llvm/llvm-project/pull/79901
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
bulbazord wrote: ping @clayborg How does this look now? https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
bulbazord wrote: > > This uses `DataExtractor::GetMaxU64` which already does this under the > > hood. What does this do that isn't already being done? It may help if you > > add a test case to show what you are trying to fix. > > @clayborg @bulbazord The problem with GetMaxU64 is that it always returns > uint64_t even though actual type was uint32_t, so when byteswap is performed > it becomes invalid integer, to fixed this we need to store correct bitwidth > not higher. i.e. Suppose there is actual 32 bit integer i.e. 0x > `GetMaxU64` will return 0x (prmoted to uint64_t from > uint32_t) and when performing byteswap on this it becomes 0x > which is invalid. That makes sense to me, but how are you encountering this behavior? What specifically is this fixing? If you can write a test case that fails without your change but works with your change, that would help us understand what you're trying to fix. Correctness is important, and tests help us verify that we're not inadvertently introducing regressions. https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Detect a Darwin kernel issue and work around it (PR #81573)
@@ -79,6 +79,11 @@ class StopInfo : public
std::enable_shared_from_this {
virtual bool IsValidForOperatingSystemThread(Thread ) { return true; }
+ /// A Continue operation can result in a false stop event
+ /// before any execution has happened in certain cases on Darwin.
+ /// We need to silently continue again time.
bulbazord wrote:
"continue again time." -> "continue again one more time."
Is this what you meant?
https://github.com/llvm/llvm-project/pull/81573
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Detect a Darwin kernel issue and work around it (PR #81573)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/81573 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Detect a Darwin kernel issue and work around it (PR #81573)
https://github.com/bulbazord commented: Makes sense to me! https://github.com/llvm/llvm-project/pull/81573 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Detect a Darwin kernel issue and work around it (PR #81573)
@@ -825,6 +806,56 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( break; } - return StopInfoSP(new StopInfoMachException(thread, exc_type, exc_data_count, - exc_code, exc_sub_code)); + return StopInfoSP(new StopInfoMachException( bulbazord wrote: Since you're already here: `make_shared`? https://github.com/llvm/llvm-project/pull/81573 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Add header guard to PlatformRemoteAppleXR.h (PR #81565)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/81565 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Add header guard to PlatformRemoteAppleXR.h (PR #81565)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/81565
>From a1bb825c34a951a0d99ecf03fe86545ed43bbe42 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 12 Feb 2024 19:05:55 -0800
Subject: [PATCH 1/2] [lldb][NFCI] Add header guard to PlatformRemoteAppleXR.h
---
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
index 4fed6e15eda31c..a3e83b217149a0 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
@@ -6,6 +6,9 @@
//
//===--===//
+#ifdef LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
+#define LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
+
#include "PlatformRemoteDarwinDevice.h"
namespace lldb_private {
@@ -36,3 +39,5 @@ class PlatformRemoteAppleXR : public
PlatformRemoteDarwinDevice {
llvm::StringRef GetPlatformName() override;
};
} // namespace lldb_private
+
+#endif // LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
>From 916014411b73cc3132f0d2cc64438ca080d67b3e Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 12 Feb 2024 19:14:07 -0800
Subject: [PATCH 2/2] ifdef -> ifndef
---
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
index a3e83b217149a0..2fbb6caad8110f 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
@@ -6,7 +6,7 @@
//
//===--===//
-#ifdef LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
+#ifndef LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
#define LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
#include "PlatformRemoteDarwinDevice.h"
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Add header guard to PlatformRemoteAppleXR.h (PR #81565)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/81565
None
>From a1bb825c34a951a0d99ecf03fe86545ed43bbe42 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 12 Feb 2024 19:05:55 -0800
Subject: [PATCH] [lldb][NFCI] Add header guard to PlatformRemoteAppleXR.h
---
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
index 4fed6e15eda31c..a3e83b217149a0 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleXR.h
@@ -6,6 +6,9 @@
//
//===--===//
+#ifdef LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
+#define LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
+
#include "PlatformRemoteDarwinDevice.h"
namespace lldb_private {
@@ -36,3 +39,5 @@ class PlatformRemoteAppleXR : public
PlatformRemoteDarwinDevice {
llvm::StringRef GetPlatformName() override;
};
} // namespace lldb_private
+
+#endif // LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMREMOTEAPPLEXR_H
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Store proper integer bitwidth in Scalar Type (PR #81451)
https://github.com/bulbazord requested changes to this pull request. This uses `DataExtractor::GetMaxU64` which already does this under the hood. What does this do that isn't already being done? It may help if you add a test case to show what you are trying to fix. https://github.com/llvm/llvm-project/pull/81451 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][Docs] Replace LLDB_RELOCATABLE_PYTHON with LLDB_EMBED_PYTHON_HOME (PR #81310)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/81310 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] checks beforehand if lldb can trace/attach a process on FreeBSD. (PR #79662)
https://github.com/bulbazord approved this pull request. LGTM but I'm not really an expert on FreeBSD. @emaste ? https://github.com/llvm/llvm-project/pull/79662 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
https://github.com/bulbazord approved this pull request. Ok, I think this is good to go in. There are things that can be improved but it would be better to land this and change things in follow up PRs instead of continually adjusting this one. https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -0,0 +1,315 @@ +""" +This module implements a couple of utility classes to make writing +lldb parsed commands more Pythonic. +The way to use it is to make a class for you command that inherits from ParsedCommandBase. +That will make an LLDBOVParser which you will use for your +option definition, and to fetch option values for the current invocation bulbazord wrote: I don't remember why I suggested this, it looks pretty clear to me after another pass. https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [NFC] Remove min pkt size for compression setting (PR #81075)
https://github.com/bulbazord approved this pull request. LGTM. I also noticed there's no tests for this functionality... https://github.com/llvm/llvm-project/pull/81075 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Refactor GetFormatFromCString to always check for partial matches (NFC) (PR #81018)
https://github.com/bulbazord approved this pull request. Makes sense to me. If the test suite is happy then I think this is fine. https://github.com/llvm/llvm-project/pull/81018 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add comment to ParseInternal in FormatEntity (NFC) (PR #81018)
@@ -2204,7 +2204,9 @@ static Status ParseInternal(llvm::StringRef ,
Entry _entry,
return error;
}
} else if (FormatManager::GetFormatFromCString(
- entry.printf_format.c_str(), true, entry.fmt)) {
+ entry.printf_format.c_str(), true,
+ entry.fmt)) { // Try GetFormatFromCString again,
bulbazord wrote:
Is the ordering important here? Why not try partial formatting first from the
start? The code paths are nearly identical except for one parameter... The
comment definitely helps disambiguate but it would be nicer if we could just
rewrite this entirely.
https://github.com/llvm/llvm-project/pull/81018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Don't count all the frames just to skip the current inlined ones. (PR #80918)
@@ -608,11 +608,10 @@ static bool Evaluate_DW_OP_entry_value(std::vector
,
StackFrameSP parent_frame = nullptr;
addr_t return_pc = LLDB_INVALID_ADDRESS;
uint32_t current_frame_idx = current_frame->GetFrameIndex();
- uint32_t num_frames = thread->GetStackFrameCount();
- for (uint32_t parent_frame_idx = current_frame_idx + 1;
- parent_frame_idx < num_frames; ++parent_frame_idx) {
+
+ for (uint32_t parent_frame_idx = current_frame_idx + 1;;parent_frame_idx++) {
bulbazord wrote:
Suggestion: If you initialize `parent_frame` to
`thread->GetStackFrameAtIndex(current_frame_idx + 1)` and move the
`parent_frame = ...` bit to the end of the loop, you can have the loop
condition be `parent_frame != nullptr` instead of relying on a break statement.
https://github.com/llvm/llvm-project/pull/80918
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
https://github.com/bulbazord approved this pull request. I'm happy w/ the name. Thanks! https://github.com/llvm/llvm-project/pull/80890 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
@@ -5,10 +5,11 @@ let Definition = "modulelist" in {
Global,
DefaultTrue,
Desc<"Control the use of external tools and repositories to locate symbol
files. Directories listed in target.debug-file-search-paths and directory of
the executable are always checked first for separate debug info files. Then
depending on this setting: On macOS, Spotlight would be also used to locate a
matching .dSYM bundle based on the UUID of the executable. On NetBSD, directory
/usr/libdata/debug would be also searched. On platforms other than NetBSD
directory /usr/lib/debug would be also searched. If all other methods fail
there may be symbol-locator plugins that, if configured properly, will also
attempt to acquire symbols. The debuginfod plugin defaults to the
DEGUFINFOD_URLS environment variable which is configurable through the
'plugin.symbol-locator.debuginfod.server_urls' setting.">;
- def EnableBackgroundLookup: Property<"enable-background-lookup", "Boolean">,
+ def LazySymbolLookup: Property<"lazy-lookup", "Enum">,
bulbazord wrote:
`symbol-loading-behavior`? `symbol-lookup-strategy`?
https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Expand background symbol lookup (PR #80890)
@@ -5,10 +5,11 @@ let Definition = "modulelist" in {
Global,
DefaultTrue,
Desc<"Control the use of external tools and repositories to locate symbol
files. Directories listed in target.debug-file-search-paths and directory of
the executable are always checked first for separate debug info files. Then
depending on this setting: On macOS, Spotlight would be also used to locate a
matching .dSYM bundle based on the UUID of the executable. On NetBSD, directory
/usr/libdata/debug would be also searched. On platforms other than NetBSD
directory /usr/lib/debug would be also searched. If all other methods fail
there may be symbol-locator plugins that, if configured properly, will also
attempt to acquire symbols. The debuginfod plugin defaults to the
DEGUFINFOD_URLS environment variable which is configurable through the
'plugin.symbol-locator.debuginfod.server_urls' setting.">;
- def EnableBackgroundLookup: Property<"enable-background-lookup", "Boolean">,
+ def LazySymbolLookup: Property<"lazy-lookup", "Enum">,
bulbazord wrote:
I think the name should probably be different. "LazySymbolLookup" being set to
off makes me think it will look things up eagerly. Looking up the possible
values of "off", "background", and "foreground" also don't really illustrate
what they mean either.
I would propose the setting name "external-symbol-load-behavior" with the
enumeration values "off", "background", and "blocking" or something to this
effect.
https://github.com/llvm/llvm-project/pull/80890
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add a new SBProcess:: GetCoreFile() API (PR #80767)
bulbazord wrote: I'm not sure I understand. Why do you need to get the path to the core file through the SBAPI? Didn't you also load it through the SBAPI too? https://github.com/llvm/llvm-project/pull/80767 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
@@ -241,7 +241,7 @@ class DWARFUnit : public UserID {
FileSpec GetFile(size_t file_idx);
FileSpec::Style GetPathStyle();
- SymbolFileDWARFDwo *GetDwoSymbolFile();
+ SymbolFileDWARFDwo *GetDwoSymbolFile(bool load_if_needed = true);
bulbazord wrote:
Instead of having a bool here, you could make this an enum with 2 values.
Something like this:
```
enum LoadMode : bool { eDoNotForceLoad, eForceLoad };
```
https://github.com/llvm/llvm-project/pull/80745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
@@ -86,9 +86,13 @@ class LLDB_API SBTarget {
/// Returns a dump of the collected statistics.
///
+ /// \param[in] summary_only
+ /// If true, only report high level summary statistics without
+ /// targets/modules/breakpoints etc.. details.
+ ///
/// \return
/// A SBStructuredData with the statistics collected.
- lldb::SBStructuredData GetStatistics();
bulbazord wrote:
This is an ABI-breaking change. You'll want to add a new method entirely.
https://github.com/llvm/llvm-project/pull/80745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
@@ -186,6 +186,10 @@ class SymbolFileDWARF : public SymbolFileCommon {
GetMangledNamesForFunction(const std::string _qualified_name,
std::vector _names) override;
+ // Return total currently loaded debug info.
+ // For cases like .dwo files, the debug info = skeleton debug info + all dwo
+ // debug info where .dwo files might not be loaded yet. Calling this function
+ // will not force the loading of any .dwo files.
bulbazord wrote:
Could you make this into a proper doxygen comment? With `///` instead of `//`
https://github.com/llvm/llvm-project/pull/80745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/80745 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Support statistics dump summary only mode (PR #80745)
https://github.com/bulbazord requested changes to this pull request. Haven't looked over everything yet but this has an ABI-breaking change. Please revert the existing changes to SBTarget. https://github.com/llvm/llvm-project/pull/80745 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add QSupported key to report watchpoint types supported (PR #80376)
https://github.com/bulbazord approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/80376 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add QSupported key to report watchpoint types supported (PR #80376)
bulbazord wrote: > @bulbazord in the most recent commit I moved this internal-only enum from > lldb-enumerations.h to lldb-private-enumerations.h, but I need to use the > constexpr templatey thing that LLDB_MARK_AS_BITMASK_ENUM() defines for the > enum so I can use bit-wise | & operations without casting everywhere; that's > defined in lldb-enumerations.h so I included the public enums in the > lldb-private-enumerations.h. It seems like it's probably not a great choice, > but the other one is breaking out this and FLAGS_ENUM etc into a little > lldb-common-enumerations.h or something. What do you think? Not the greatest thing in the world but it's not terrible either. We use the lldb public enumerations everywhere in private code, so as long as we're not going the other way this is ok to do. I will say, by moving the definitions from `lldb-enumerations.h` to `lldb-private-enumerations.h` you have removed these values from the python bindings. Technically that's an API break, but I'm not sure where anyone could have used these values otherwise. LGTM. https://github.com/llvm/llvm-project/pull/80376 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Remove unused private TypeCategoryMap methods (NFC) (PR #80602)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/80602 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove m_being_created from Breakpoint classes (PR #79716)
https://github.com/bulbazord closed https://github.com/llvm/llvm-project/pull/79716 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove m_being_created from Breakpoint classes (PR #79716)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/79716
>From f7bc2e013fb4a5cac93d2c5856983796459fb84b Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Sat, 27 Jan 2024 15:54:16 -0800
Subject: [PATCH 1/4] [lldb][NFCI] Remove m_being_created from Breakpoint
classes
The purpose of m_being_created in these classes was to prevent
broadcasting an event related to these Breakpoints during the creation
of the breakpoint (i.e. in the constructor). In Breakpoint and
Watchpoint, m_being_created had no effect. That is to say, removing it
does not change behavior.
However, BreakpointLocation does still use m_being_created. In the
constructor, SetThreadID is called which does broadcast an event only if
`m_being_created` is false. Instead of having this logic be roundabout,
the constructor instead calls `SetThreadIDInternal`, which actually
changes the thread ID. `SetThreadID` also will call
`SetThreadIDInternal` in addition to broadcasting a changed event.
---
lldb/include/lldb/Breakpoint/Breakpoint.h | 1 -
.../lldb/Breakpoint/BreakpointLocation.h | 3 +-
lldb/include/lldb/Breakpoint/Watchpoint.h | 2 --
lldb/source/Breakpoint/Breakpoint.cpp | 22 +---
lldb/source/Breakpoint/BreakpointLocation.cpp | 34 ++-
lldb/source/Breakpoint/Watchpoint.cpp | 8 ++---
6 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h
b/lldb/include/lldb/Breakpoint/Breakpoint.h
index 8c4308ab0bc12..0b6bf593be37a 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -637,7 +637,6 @@ class Breakpoint : public
std::enable_shared_from_this,
Breakpoint(Target _target, const Breakpoint _to_copy_from);
// For Breakpoint only
- bool m_being_created;
bool
m_hardware; // If this breakpoint is required to use a hardware
breakpoint
Target _target; // The target that holds this breakpoint.
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 2a4f9fc01bf32..273132c950825 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -313,6 +313,8 @@ class BreakpointLocation
void UndoBumpHitCount();
+ void SetThreadIDInternal(lldb::tid_t thread_id);
+
// Constructors and Destructors
//
// Only the Breakpoint can make breakpoint locations, and it owns them.
@@ -337,7 +339,6 @@ class BreakpointLocation
bool check_for_resolver = true);
// Data members:
- bool m_being_created;
bool m_should_resolve_indirect_functions;
bool m_is_reexported;
bool m_is_indirect;
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h
b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 22fdfd686c3f1..fc11a466ba605 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -227,8 +227,6 @@ class Watchpoint : public
std::enable_shared_from_this,
WatchpointOptions
m_options; // Settable watchpoint options, which is a delegate to handle
// the callback machinery.
- bool m_being_created;
-
std::unique_ptr m_condition_up; // The condition to test.
void SetID(lldb::watch_id_t id) { m_id = id; }
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp
b/lldb/source/Breakpoint/Breakpoint.cpp
index af5dcc9cd88d4..ae845e92762b9 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -44,17 +44,14 @@ const char
*Breakpoint::g_option_names[static_cast(
Breakpoint::Breakpoint(Target , SearchFilterSP _sp,
BreakpointResolverSP _sp, bool hardware,
bool resolve_indirect_symbols)
-: m_being_created(true), m_hardware(hardware), m_target(target),
- m_filter_sp(filter_sp), m_resolver_sp(resolver_sp), m_options(true),
- m_locations(*this), m_resolve_indirect_symbols(resolve_indirect_symbols),
- m_hit_counter() {
- m_being_created = false;
-}
+: m_hardware(hardware), m_target(target), m_filter_sp(filter_sp),
+ m_resolver_sp(resolver_sp), m_options(true), m_locations(*this),
+ m_resolve_indirect_symbols(resolve_indirect_symbols), m_hit_counter() {}
Breakpoint::Breakpoint(Target _target, const Breakpoint _bp)
-: m_being_created(true), m_hardware(source_bp.m_hardware),
- m_target(new_target), m_name_list(source_bp.m_name_list),
- m_options(source_bp.m_options), m_locations(*this),
+: m_hardware(source_bp.m_hardware), m_target(new_target),
+ m_name_list(source_bp.m_name_list), m_options(source_bp.m_options),
+ m_locations(*this),
m_resolve_indirect_symbols(source_bp.m_resolve_indirect_symbols),
m_hit_counter() {}
@@ -979,9 +976,8 @@ bool
Breakpoint::EvaluatePrecondition(StoppointCallbackContext ) {
void Breakpoint::SendBreakpointChangedEvent(
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
bulbazord wrote: @clayborg how does this look to you? https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for large watchpoints in lldb (PR #79962)
@@ -0,0 +1,38 @@
+//===-- WatchpointAlgorithms.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_BREAKPOINT_WATCHPOINTALGORITHMS_H
+#define LLDB_BREAKPOINT_WATCHPOINTALGORITHMS_H
+
+#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointAlgorithms {
+
+public:
+ static std::vector AtomizeWatchpointRequest(
bulbazord wrote:
Can you add some documentation for these static functions? In the form of
doxygen comments preferrably
https://github.com/llvm/llvm-project/pull/79962
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for large watchpoints in lldb (PR #79962)
@@ -0,0 +1,38 @@ +//===-- WatchpointAlgorithms.h *- C++ bulbazord wrote: The header needs adjustment https://github.com/llvm/llvm-project/pull/79962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for large watchpoints in lldb (PR #79962)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/79962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for large watchpoints in lldb (PR #79962)
@@ -0,0 +1,146 @@
+//===-- WatchpointAlgorithms.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/Breakpoint/WatchpointAlgorithms.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/ArchSpec.h"
+
+#include
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+std::vector
+WatchpointAlgorithms::AtomizeWatchpointRequest(
+addr_t addr, size_t size, bool read, bool write,
+WatchpointHardwareFeature supported_features, ArchSpec ) {
+
+ std::vector> entries;
+
+ if (supported_features &
+ WatchpointHardwareFeature::eWatchpointHardwareArmMASK) {
+entries =
+PowerOf2Watchpoints(addr, size,
+/* min_byte_size */ 1,
+/* max_byte_size */ 2147483648,
bulbazord wrote:
Is this just INT32_MAX? Probably better to use a constant for clarity.
https://github.com/llvm/llvm-project/pull/79962
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for large watchpoints in lldb (PR #79962)
https://github.com/bulbazord commented: I haven't looked at the algorithm in great detail yet but a lot of the surrounding stuff looks pretty reasonable I think. https://github.com/llvm/llvm-project/pull/79962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/79901
>From 3cd6afa16fb8b282712b1d3cf103abbd3329fc17 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 29 Jan 2024 13:15:24 -0800
Subject: [PATCH 1/3] [lldb][NFCI] Minor refactor to
CommandObjectProcessHandle::VerifyCommandOptionValue
I was refactoring something else but ran into this function. It was
somewhat confusing to read through and understand, but it boils down to
two steps:
- First we try `OptionArgParser::ToBoolean`. If that works, then we're
good to go.
- Second, we try `llvm::to_integer` to see if it's an integer. If it
parses to 0 or 1, we're good.
- Failing either of the steps above means we cannot parse it into a
bool.
Instead of having an integer out param and a bool return value, it seems
like the interface is better served with an optional -- Either it
parses into true or false, or you get back nothing (nullopt).
---
lldb/source/Commands/CommandObjectProcess.cpp | 103 --
1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp
b/lldb/source/Commands/CommandObjectProcess.cpp
index c7b874d197937..f089a86275dc6 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
Options *GetOptions() override { return _options; }
- bool VerifyCommandOptionValue(const std::string , int _value) {
-bool okay = true;
+ std::optional VerifyCommandOptionValue(const std::string ) {
+if (option.empty())
+ return std::nullopt;
+
bool success = false;
bool tmp_value = OptionArgParser::ToBoolean(option, false, );
+if (success)
+ return tmp_value;
+
+int parsed_value = -1;
+if (llvm::to_integer(option, parsed_value)) {
+ if (parsed_value != 0 && parsed_value != 1)
+return std::nullopt;
-if (success && tmp_value)
- real_value = 1;
-else if (success && !tmp_value)
- real_value = 0;
-else {
- // If the value isn't 'true' or 'false', it had better be 0 or 1.
- if (!llvm::to_integer(option, real_value))
-real_value = 3;
- if (real_value != 0 && real_value != 1)
-okay = false;
+ return parsed_value == 0 ? false : true;
}
-return okay;
+return std::nullopt;
}
void PrintSignalHeader(Stream ) {
@@ -1666,33 +1666,31 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = VerifyCommandOptionValue(m_options.stop);
+std::optional pass_action = VerifyCommandOptionValue(m_options.pass);
+std::optional notify_action =
+VerifyCommandOptionValue(m_options.notify);
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
+if (!m_options.stop.empty() && !stop_action.has_value()) {
result.AppendError("Invalid argument for command option --stop; must be "
"true or false.\n");
return;
}
-if (!m_options.notify.empty() &&
-!VerifyCommandOptionValue(m_options.notify, notify_action)) {
- result.AppendError("Invalid argument for command option --notify; must "
- "be true or false.\n");
+if (!m_options.pass.empty() && !pass_action.has_value()) {
+ result.AppendError("Invalid argument for command option --pass; must be "
+ "true or false.\n");
return;
}
-if (!m_options.pass.empty() &&
-!VerifyCommandOptionValue(m_options.pass, pass_action)) {
- result.AppendError("Invalid argument for command option --pass; must be "
- "true or false.\n");
+if (!m_options.notify.empty() && !notify_action.has_value()) {
+ result.AppendError("Invalid argument for command option --notify; must "
+ "be true or false.\n");
return;
}
-bool no_actions = (stop_action == -1 && pass_action == -1
-&& notify_action == -1);
+bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
+ !notify_action.has_value());
if (m_options.only_target_values && !no_actions) {
result.AppendError("-t is for reporting, not setting, target values.");
return;
@@ -1732,14 +1730,14 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
// Casting the actions as bools here should be okay, because
// VerifyCommandOptionValue guarantees the value is either 0
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
bulbazord wrote: Okay, here's what I'll do then: 1. I will remove `VerifyCommandOptionValue` and replace uses of it with `OptionArgParser::ToBoolean`. 2. Afterwards, I will change the interface of `OptionArgParser::ToBoolean`. https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
Options *GetOptions() override { return _options; }
- bool VerifyCommandOptionValue(const std::string , int _value) {
-bool okay = true;
+ std::optional VerifyCommandOptionValue(const std::string ) {
bulbazord wrote:
I was actually in the middle of doing that when I found
`VerifyCommandOptionValue`. I was going to upload a PR for it after removing
this.
https://github.com/llvm/llvm-project/pull/79901
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
bulbazord wrote: > This version is still a little awkward because there's an ambiguity about > what the incoming string being empty means. In the VerifyCommandOptionValue, > it means "No Value" and the return is not distinguishable from "error" - but > in a bunch of the places where you use VerifyCommandOptionValue an empty > string just means a value wasn't provided, so you end up with constructs > where you set the optional (checking the incoming string for `empty`) then > check the input string again so you can tell empty apart from error, THEN > make up the error on the outside w/o knowing what actually was wrong with the > string. I did this intentionally so that `VerifyCommandOptionValue` would not attempt to interpret the result. The caller knows whether or not its input is empty, so it's able to interpret the results of `VerifyCommandOptionValue` however it wants. Maybe `VerifyCommandOptionValue` is not a good name for this? It seems to just be trying to take a string and trying to get a bool out of it. Thinking about it further, I'm starting to question the value of `VerifyCommandOptionValue`... Maybe we should be using `OptionArgParser::ToBoolean`? That seems to be the end goal anyway. > I wonder if it would be better to add a Status to the call. Then empty input > -> "Empty Optional but no error"; error in parsing -> "Empty Optional but > error" and result means success. Then you wouldn't have to do two checks on > the input string in different places, or make up the error string. https://github.com/llvm/llvm-project/pull/79901 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][NFCI] Remove m_being_created from Breakpoint classes (PR #79716)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/79716
>From 1f81c3e01a12682ad514038d2e9f1baea80dcf03 Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Sat, 27 Jan 2024 15:54:16 -0800
Subject: [PATCH 1/3] [lldb][NFCI] Remove m_being_created from Breakpoint
classes
The purpose of m_being_created in these classes was to prevent
broadcasting an event related to these Breakpoints during the creation
of the breakpoint (i.e. in the constructor). In Breakpoint and
Watchpoint, m_being_created had no effect. That is to say, removing it
does not change behavior.
However, BreakpointLocation does still use m_being_created. In the
constructor, SetThreadID is called which does broadcast an event only if
`m_being_created` is false. Instead of having this logic be roundabout,
the constructor instead calls `SetThreadIDInternal`, which actually
changes the thread ID. `SetThreadID` also will call
`SetThreadIDInternal` in addition to broadcasting a changed event.
---
lldb/include/lldb/Breakpoint/Breakpoint.h | 1 -
.../lldb/Breakpoint/BreakpointLocation.h | 3 +-
lldb/include/lldb/Breakpoint/Watchpoint.h | 2 --
lldb/source/Breakpoint/Breakpoint.cpp | 22 +---
lldb/source/Breakpoint/BreakpointLocation.cpp | 34 ++-
lldb/source/Breakpoint/Watchpoint.cpp | 8 ++---
6 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h
b/lldb/include/lldb/Breakpoint/Breakpoint.h
index 8c4308ab0bc12..0b6bf593be37a 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -637,7 +637,6 @@ class Breakpoint : public
std::enable_shared_from_this,
Breakpoint(Target _target, const Breakpoint _to_copy_from);
// For Breakpoint only
- bool m_being_created;
bool
m_hardware; // If this breakpoint is required to use a hardware
breakpoint
Target _target; // The target that holds this breakpoint.
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 2a4f9fc01bf32..273132c950825 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -313,6 +313,8 @@ class BreakpointLocation
void UndoBumpHitCount();
+ void SetThreadIDInternal(lldb::tid_t thread_id);
+
// Constructors and Destructors
//
// Only the Breakpoint can make breakpoint locations, and it owns them.
@@ -337,7 +339,6 @@ class BreakpointLocation
bool check_for_resolver = true);
// Data members:
- bool m_being_created;
bool m_should_resolve_indirect_functions;
bool m_is_reexported;
bool m_is_indirect;
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h
b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 22fdfd686c3f1..fc11a466ba605 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -227,8 +227,6 @@ class Watchpoint : public
std::enable_shared_from_this,
WatchpointOptions
m_options; // Settable watchpoint options, which is a delegate to handle
// the callback machinery.
- bool m_being_created;
-
std::unique_ptr m_condition_up; // The condition to test.
void SetID(lldb::watch_id_t id) { m_id = id; }
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp
b/lldb/source/Breakpoint/Breakpoint.cpp
index af5dcc9cd88d4..ae845e92762b9 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -44,17 +44,14 @@ const char
*Breakpoint::g_option_names[static_cast(
Breakpoint::Breakpoint(Target , SearchFilterSP _sp,
BreakpointResolverSP _sp, bool hardware,
bool resolve_indirect_symbols)
-: m_being_created(true), m_hardware(hardware), m_target(target),
- m_filter_sp(filter_sp), m_resolver_sp(resolver_sp), m_options(true),
- m_locations(*this), m_resolve_indirect_symbols(resolve_indirect_symbols),
- m_hit_counter() {
- m_being_created = false;
-}
+: m_hardware(hardware), m_target(target), m_filter_sp(filter_sp),
+ m_resolver_sp(resolver_sp), m_options(true), m_locations(*this),
+ m_resolve_indirect_symbols(resolve_indirect_symbols), m_hit_counter() {}
Breakpoint::Breakpoint(Target _target, const Breakpoint _bp)
-: m_being_created(true), m_hardware(source_bp.m_hardware),
- m_target(new_target), m_name_list(source_bp.m_name_list),
- m_options(source_bp.m_options), m_locations(*this),
+: m_hardware(source_bp.m_hardware), m_target(new_target),
+ m_name_list(source_bp.m_name_list), m_options(source_bp.m_options),
+ m_locations(*this),
m_resolve_indirect_symbols(source_bp.m_resolve_indirect_symbols),
m_hit_counter() {}
@@ -979,9 +976,8 @@ bool
Breakpoint::EvaluatePrecondition(StoppointCallbackContext ) {
void Breakpoint::SendBreakpointChangedEvent(
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
https://github.com/bulbazord updated
https://github.com/llvm/llvm-project/pull/79901
>From 7ce769f1a5bde83e0ff7ae36852c7a742bbb990b Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 29 Jan 2024 13:15:24 -0800
Subject: [PATCH 1/2] [lldb][NFCI] Minor refactor to
CommandObjectProcessHandle::VerifyCommandOptionValue
I was refactoring something else but ran into this function. It was
somewhat confusing to read through and understand, but it boils down to
two steps:
- First we try `OptionArgParser::ToBoolean`. If that works, then we're
good to go.
- Second, we try `llvm::to_integer` to see if it's an integer. If it
parses to 0 or 1, we're good.
- Failing either of the steps above means we cannot parse it into a
bool.
Instead of having an integer out param and a bool return value, it seems
like the interface is better served with an optional -- Either it
parses into true or false, or you get back nothing (nullopt).
---
lldb/source/Commands/CommandObjectProcess.cpp | 103 --
1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp
b/lldb/source/Commands/CommandObjectProcess.cpp
index c7b874d19793770..f089a86275dc694 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
Options *GetOptions() override { return _options; }
- bool VerifyCommandOptionValue(const std::string , int _value) {
-bool okay = true;
+ std::optional VerifyCommandOptionValue(const std::string ) {
+if (option.empty())
+ return std::nullopt;
+
bool success = false;
bool tmp_value = OptionArgParser::ToBoolean(option, false, );
+if (success)
+ return tmp_value;
+
+int parsed_value = -1;
+if (llvm::to_integer(option, parsed_value)) {
+ if (parsed_value != 0 && parsed_value != 1)
+return std::nullopt;
-if (success && tmp_value)
- real_value = 1;
-else if (success && !tmp_value)
- real_value = 0;
-else {
- // If the value isn't 'true' or 'false', it had better be 0 or 1.
- if (!llvm::to_integer(option, real_value))
-real_value = 3;
- if (real_value != 0 && real_value != 1)
-okay = false;
+ return parsed_value == 0 ? false : true;
}
-return okay;
+return std::nullopt;
}
void PrintSignalHeader(Stream ) {
@@ -1666,33 +1666,31 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = VerifyCommandOptionValue(m_options.stop);
+std::optional pass_action = VerifyCommandOptionValue(m_options.pass);
+std::optional notify_action =
+VerifyCommandOptionValue(m_options.notify);
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
+if (!m_options.stop.empty() && !stop_action.has_value()) {
result.AppendError("Invalid argument for command option --stop; must be "
"true or false.\n");
return;
}
-if (!m_options.notify.empty() &&
-!VerifyCommandOptionValue(m_options.notify, notify_action)) {
- result.AppendError("Invalid argument for command option --notify; must "
- "be true or false.\n");
+if (!m_options.pass.empty() && !pass_action.has_value()) {
+ result.AppendError("Invalid argument for command option --pass; must be "
+ "true or false.\n");
return;
}
-if (!m_options.pass.empty() &&
-!VerifyCommandOptionValue(m_options.pass, pass_action)) {
- result.AppendError("Invalid argument for command option --pass; must be "
- "true or false.\n");
+if (!m_options.notify.empty() && !notify_action.has_value()) {
+ result.AppendError("Invalid argument for command option --notify; must "
+ "be true or false.\n");
return;
}
-bool no_actions = (stop_action == -1 && pass_action == -1
-&& notify_action == -1);
+bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
+ !notify_action.has_value());
if (m_options.only_target_values && !no_actions) {
result.AppendError("-t is for reporting, not setting, target values.");
return;
@@ -1732,14 +1730,14 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
// Casting the actions as bools here should be okay, because
// VerifyCommandOptionValue guarantees the value is either
[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)
https://github.com/bulbazord created
https://github.com/llvm/llvm-project/pull/79901
I was refactoring something else but ran into this function. It was somewhat
confusing to read through and understand, but it boils down to two steps:
- First we try `OptionArgParser::ToBoolean`. If that works, then we're good to
go.
- Second, we try `llvm::to_integer` to see if it's an integer. If it parses to
0 or 1, we're good.
- Failing either of the steps above means we cannot parse it into a bool.
Instead of having an integer out param and a bool return value, the interface
is better served with an optional -- Either it parses into true or false,
or you get back nothing (nullopt).
>From 7ce769f1a5bde83e0ff7ae36852c7a742bbb990b Mon Sep 17 00:00:00 2001
From: Alex Langford
Date: Mon, 29 Jan 2024 13:15:24 -0800
Subject: [PATCH] [lldb][NFCI] Minor refactor to
CommandObjectProcessHandle::VerifyCommandOptionValue
I was refactoring something else but ran into this function. It was
somewhat confusing to read through and understand, but it boils down to
two steps:
- First we try `OptionArgParser::ToBoolean`. If that works, then we're
good to go.
- Second, we try `llvm::to_integer` to see if it's an integer. If it
parses to 0 or 1, we're good.
- Failing either of the steps above means we cannot parse it into a
bool.
Instead of having an integer out param and a bool return value, it seems
like the interface is better served with an optional -- Either it
parses into true or false, or you get back nothing (nullopt).
---
lldb/source/Commands/CommandObjectProcess.cpp | 103 --
1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp
b/lldb/source/Commands/CommandObjectProcess.cpp
index c7b874d19793770..f089a86275dc694 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
Options *GetOptions() override { return _options; }
- bool VerifyCommandOptionValue(const std::string , int _value) {
-bool okay = true;
+ std::optional VerifyCommandOptionValue(const std::string ) {
+if (option.empty())
+ return std::nullopt;
+
bool success = false;
bool tmp_value = OptionArgParser::ToBoolean(option, false, );
+if (success)
+ return tmp_value;
+
+int parsed_value = -1;
+if (llvm::to_integer(option, parsed_value)) {
+ if (parsed_value != 0 && parsed_value != 1)
+return std::nullopt;
-if (success && tmp_value)
- real_value = 1;
-else if (success && !tmp_value)
- real_value = 0;
-else {
- // If the value isn't 'true' or 'false', it had better be 0 or 1.
- if (!llvm::to_integer(option, real_value))
-real_value = 3;
- if (real_value != 0 && real_value != 1)
-okay = false;
+ return parsed_value == 0 ? false : true;
}
-return okay;
+return std::nullopt;
}
void PrintSignalHeader(Stream ) {
@@ -1666,33 +1666,31 @@ class CommandObjectProcessHandle : public
CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();
-int stop_action = -1; // -1 means leave the current setting alone
-int pass_action = -1; // -1 means leave the current setting alone
-int notify_action = -1; // -1 means leave the current setting alone
+std::optional stop_action = VerifyCommandOptionValue(m_options.stop);
+std::optional pass_action = VerifyCommandOptionValue(m_options.pass);
+std::optional notify_action =
+VerifyCommandOptionValue(m_options.notify);
-if (!m_options.stop.empty() &&
-!VerifyCommandOptionValue(m_options.stop, stop_action)) {
+if (!m_options.stop.empty() && !stop_action.has_value()) {
result.AppendError("Invalid argument for command option --stop; must be "
"true or false.\n");
return;
}
-if (!m_options.notify.empty() &&
-!VerifyCommandOptionValue(m_options.notify, notify_action)) {
- result.AppendError("Invalid argument for command option --notify; must "
- "be true or false.\n");
+if (!m_options.pass.empty() && !pass_action.has_value()) {
+ result.AppendError("Invalid argument for command option --pass; must be "
+ "true or false.\n");
return;
}
-if (!m_options.pass.empty() &&
-!VerifyCommandOptionValue(m_options.pass, pass_action)) {
- result.AppendError("Invalid argument for command option --pass; must be "
- "true or false.\n");
+if (!m_options.notify.empty() && !notify_action.has_value()) {
+ result.AppendError("Invalid argument for command option --notify; must "
+ "be true or false.\n");
return;
}
-bool no_actions = (stop_action == -1 && pass_action == -1
-
[Lldb-commits] [lldb] [lldb][NFCI] Remove m_being_created from Breakpoint classes (PR #79716)
@@ -313,6 +313,8 @@ class BreakpointLocation void UndoBumpHitCount(); + void SetThreadIDInternal(lldb::tid_t thread_id); bulbazord wrote: @jimingham how does this look to you? https://github.com/llvm/llvm-project/pull/79716 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
