[Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command
labath added a comment.
I think this would be a very nice feature for lldb. Thank you for working on
this.
However, I am somewhat worried about how you're hooking the expression
completer into the completion machinery. I think this should be cleaned up
first.
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:1
+"""
+Test the lldb command line completion mechanism for the 'expr' command.
Maybe put the test under the `expression_command` sub-tree?
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:177-215
+def generate_random_expr(self, run_index):
+"""
+Generates a random expression. run_index seeds the rng, so
+the output of this method is always the same for the same run_index
value
+"""
+# Some random tokens we built our expression from.
+tokens = [".", ",", "(", ")", "{", "}", "foo", "a", "some_expr",
I don't think a test like this has place in the test suite. It tests a
different thing every time it's run and it will be impossible to debug if it
starts to fail/be flaky on the build bots.
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:227
+
+assert num_matches == 0, "Got matches, but didn't expect any: " +
str(available_completions)
+
We normally use the unittest2 functions to do assertions
(`self.assertEquals(num_matches, 0)`)
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:246-264
+def complete_from_to(self, str_input, pattern):
+interp = self.dbg.GetCommandInterpreter()
+match_strings = lldb.SBStringList()
+num_matches = interp.HandleCompletion(str_input, len(str_input), 0,
-1, match_strings)
+common_match = match_strings.GetStringAtIndex(0)
+
+if num_matches == 0:
We already have this function in TestCompletions.py. We should move it so some
place that we can reuse it instead of reimplementing it.
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/main.cpp:1
+#include
+
The rest of this test is test is nicely self-contained. Could we avoid
std::string here and make it fully hermetic?
Comment at: source/Commands/CommandObjectExpression.cpp:419-423
+// We got the index of the arg and the cursor index inside that arg as
+// parameters, but for completing the whole expression we need to revert
+// this back to the absolute offset of the cursor in the whole expression.
+int absolute_pos =
+WordPosToAbsolutePos(arg, cursor_index, cursor_char_position);
Will this work correctly if the expression command contains arguments (`expr
-i0 -- my_expression`)? What about quoted strings (`expr "string with
spaces`)?
Have you looked at whether it would be possible to refactor the completion api
to provide the absolute position (it has to exist at some point), instead of
trying to reverse-engineer it here?
I think the problem here is that the completion api has this built-in
assumption that you're only ever going to be completing "parsed" commands,
which is why you get the input as an `Args` array and a two-dimensional cursor
position. "expr" command takes "raw" input, which means it does not go through
the usual word-splitting and getopt parsing. You can see that because
CommandObjectExpression::DoExecute takes a `const char *command`, whereas for
"parsed" commands (e.g. "frame variable") the DoExecute function takes the
argument as `Args &command`.
I think it would make sense to start this by refactoring the completion api to
behave the same way as the "DoExecute" api. For "raw" commands , the
HandleCompletion function would take a raw string + absolute position in that
string, and the parsed commands would get the an `Args` array plus the
two-dimensional position. Without that, you're going to get subtle differences
in how the expression is treated for completion purposes and for actual
evaluation.
https://reviews.llvm.org/D48465
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe
On Wed, 20 Jun 2018 at 23:21, Jim Ingham wrote: > > It is not uncommon that you would be parsing the DWARF for module A and find > a type that is only known as a forward declaration. In that case, lldb will > look through the other Modules' debug info for a real definition, parse that > and import it into module A. So you would need to suspend one task, start > another and wait on its completion. > Taking a step back, I was wondering what are the situations when we do this kind of cross-module debug info importing? I was under the impression that we don't do this kind importing precisely because the module can be shared between multiple targets (and we don't want information from a module which is not loaded in a given target to leak into it just because we happen to have a different target with that module around). I think these kinds of things came up during the discussions about why lldb behaves poorly under -fno-standalone-debug. Am I misunderstanding something here? Because if this is true (parsing debug info in a given module does not access other modules), then maybe we could solve this by just taking some kind of a module lock when parsing debug info in the given module. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
labath created this revision.
labath added reviewers: clayborg, sas, lemo, davide.
Herald added subscribers: arichardson, emaste.
Herald added a reviewer: espindola.
During the previous attempt to generalize the UUID class, it was
suggested that we represent invalid UUIDs as length zero (previously, we
used an all-zero UUID for that). This meant that some valid build-ids
could not be represented (it's possible however unlikely that a checksum of
some file would be zero) and complicated adding support for variable
length build-ids (should a 16-byte empty UUID compare equal to a 20-byte
empty UUID?).
This patch resolves these issues by introducing a canonical
representation for an invalid UUID. The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working, I have introduced an
additional argument to the UUID constructor, which specifies whether an
all-zero vector should be considered a valid UUID. For the usages where
the UUID data comes from a MachO file, I set this argument to false.
I did not introduce a similar argument to the string parsing function
with the rationalle that if somebody went through the trouble of
spelling it out as a bunch of zeroes, he probably really means that.
https://reviews.llvm.org/D48479
Files:
include/lldb/Utility/UUID.h
source/API/SBModuleSpec.cpp
source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Utility/DataExtractor.cpp
source/Utility/UUID.cpp
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
unittests/Utility/UUIDTest.cpp
Index: unittests/Utility/UUIDTest.cpp
===
--- unittests/Utility/UUIDTest.cpp
+++ unittests/Utility/UUIDTest.cpp
@@ -15,10 +15,10 @@
TEST(UUIDTest, RelationalOperators) {
UUID empty;
- UUID a16("1234567890123456", 16);
- UUID b16("1234567890123457", 16);
- UUID a20("12345678901234567890", 20);
- UUID b20("12345678900987654321", 20);
+ UUID a16("1234567890123456", 16, true);
+ UUID b16("1234567890123457", 16, true);
+ UUID a20("12345678901234567890", 20, true);
+ UUID b20("12345678900987654321", 20, true);
EXPECT_EQ(empty, empty);
EXPECT_EQ(a16, a16);
@@ -34,3 +34,40 @@
EXPECT_LT(a16, b16);
EXPECT_GT(a20, b20);
}
+
+TEST(UUIDTest, Validity) {
+ UUID empty;
+ std::vector zeroes(20, 0);
+ UUID a16(zeroes.data(), 16, true);
+ UUID a20(zeroes.data(), 20, true);
+ UUID a16_0(zeroes.data(), 16, false);
+ UUID a20_0(zeroes.data(), 20, false);
+ EXPECT_FALSE(empty);
+ EXPECT_TRUE(a16);
+ EXPECT_TRUE(a20);
+ EXPECT_FALSE(a16_0);
+ EXPECT_FALSE(a20_0);
+}
+
+TEST(UUIDTest, SetFromStringRef) {
+ UUID u;
+ EXPECT_EQ(32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+ EXPECT_EQ(36, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+ EXPECT_EQ(45, u.SetFromStringRef(
+"40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u);
+
+ EXPECT_EQ(0, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
+ EXPECT_EQ(0, u.SetFromStringRef("40x"));
+ EXPECT_EQ(0, u.SetFromStringRef(""));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u)
+ << "uuid was changed by failed parse calls";
+
+ EXPECT_EQ(
+ 32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+}
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -194,7 +194,7 @@
ASSERT_EQ(1u, result->size());
EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
- EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16), result.getValue()[0].GetUUID());
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), result.getValue()[0].GetUUID());
EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
}
@@ -218,7 +218,8 @@
ASSERT_EQ(1u, result->size());
EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
- EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20), result.getValue()[0].GetUUID());
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true),
+result.getValue()[0].GetUUID());
EXPECT_E
[Lldb-commits] [lldb] r335344 - Android.rules: Use libc++ by default
Author: labath
Date: Fri Jun 22 06:13:29 2018
New Revision: 335344
URL: http://llvm.org/viewvc/llvm-project?rev=335344&view=rev
Log:
Android.rules: Use libc++ by default
libstdc++ will soon be dropped from the android NDK. This patch makes
sure we are prepared for that by using libc++ in tests by default (i.e.,
except for libstdc++ data formatter tests).
Only a couple of small tweaks were needed to make this work:
- Add the libc++ include paths to CXXFLAGS only. This was necessary to
make the tests compile with -fmodules. The modules tests have been
disabled, but this way, they will be ready for them if they are
enabled.
- in one test I had to add an explicit std::string copy to make sure the
copy constructor is there for the expression evaluator to find it.
Modified:
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/auto/main.cpp
lldb/trunk/packages/Python/lldbsuite/test/make/Android.rules
lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
Modified: lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/auto/main.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/auto/main.cpp?rev=335344&r1=335343&r2=335344&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/auto/main.cpp (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/auto/main.cpp Fri Jun 22
06:13:29 2018
@@ -12,5 +12,10 @@
int main()
{
std::string helloworld("hello world");
+
+ // Ensure std::string copy constructor is present in the binary, as we will
+ // use it in an expression.
+ std::string other = helloworld;
+
return 0; // break here
}
Modified: lldb/trunk/packages/Python/lldbsuite/test/make/Android.rules
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/make/Android.rules?rev=335344&r1=335343&r2=335344&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/make/Android.rules (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Android.rules Fri Jun 22
06:13:29 2018
@@ -75,8 +75,15 @@ ARCH_CFLAGS += --sysroot=$(NDK_ROOT)/sys
-D__ANDROID_API__=$(API_LEVEL)
ARCH_LDFLAGS +=
--sysroot=$(NDK_ROOT)/platforms/android-$(API_LEVEL)/arch-$(SYSROOT_ARCH) -lm
-ifeq (1,$(USE_LIBCPP))
+ifeq (1,$(USE_LIBSTDCPP))
ARCH_CFLAGS += \
+ -isystem $(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/include \
+ -isystem
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/libs/$(STL_ARCH)/include \
+ -isystem
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/include/backward
+
+ ARCH_LDFLAGS +=
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/libs/$(STL_ARCH)/libgnustl_static.a
+else
+ ARCH_CXXFLAGS += \
-isystem $(NDK_ROOT)/sources/cxx-stl/llvm-libc++/include \
-isystem $(NDK_ROOT)/sources/android/support/include \
-isystem $(NDK_ROOT)/sources/cxx-stl/llvm-libc++abi/include
@@ -84,11 +91,4 @@ ifeq (1,$(USE_LIBCPP))
ARCH_LDFLAGS += \
-L$(NDK_ROOT)/sources/cxx-stl/llvm-libc++/libs/$(STL_ARCH) \
$(NDK_ROOT)/sources/cxx-stl/llvm-libc++/libs/$(STL_ARCH)/libc++.a
-else
- ARCH_CFLAGS += \
- -isystem $(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/include \
- -isystem
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/libs/$(STL_ARCH)/include \
- -isystem
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/include/backward
-
- ARCH_LDFLAGS +=
$(NDK_ROOT)/sources/cxx-stl/gnu-libstdc++/4.9/libs/$(STL_ARCH)/libgnustl_static.a
endif
Modified: lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules?rev=335344&r1=335343&r2=335344&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules Fri Jun 22
06:13:29 2018
@@ -247,7 +247,7 @@ ifeq "$(MAKE_GMODULES)" "YES"
CFLAGS += $(MANDATORY_MODULE_BUILD_CFLAGS)
endif
-CXXFLAGS += -std=c++11 $(CFLAGS)
+CXXFLAGS += -std=c++11 $(CFLAGS) $(ARCH_CXXFLAGS)
LD = $(CC)
LDFLAGS ?= $(CFLAGS)
LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command
> On Jun 22, 2018, at 3:19 AM, Pavel Labath via Phabricator
> wrote:
>
> labath added a comment.
>
> I think this would be a very nice feature for lldb. Thank you for working on
> this.
>
> However, I am somewhat worried about how you're hooking the expression
> completer into the completion machinery. I think this should be cleaned up
> first.
>
>
>
>
> Comment at:
> packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:1
> +"""
> +Test the lldb command line completion mechanism for the 'expr' command.
>
> Maybe put the test under the `expression_command` sub-tree?
>
>
>
> Comment at:
> packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:177-215
> +def generate_random_expr(self, run_index):
> +"""
> +Generates a random expression. run_index seeds the rng, so
> +the output of this method is always the same for the same run_index
> value
> +"""
> +# Some random tokens we built our expression from.
> +tokens = [".", ",", "(", ")", "{", "}", "foo", "a", "some_expr",
>
> I don't think a test like this has place in the test suite. It tests a
> different thing every time it's run and it will be impossible to debug if it
> starts to fail/be flaky on the build bots.
>
>
>
> Comment at:
> packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:227
> +
> +assert num_matches == 0, "Got matches, but didn't expect any: " +
> str(available_completions)
> +
>
> We normally use the unittest2 functions to do assertions
> (`self.assertEquals(num_matches, 0)`)
>
>
>
> Comment at:
> packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:246-264
> +def complete_from_to(self, str_input, pattern):
> +interp = self.dbg.GetCommandInterpreter()
> +match_strings = lldb.SBStringList()
> +num_matches = interp.HandleCompletion(str_input, len(str_input), 0,
> -1, match_strings)
> +common_match = match_strings.GetStringAtIndex(0)
> +
> +if num_matches == 0:
>
> We already have this function in TestCompletions.py. We should move it so
> some place that we can reuse it instead of reimplementing it.
>
>
>
> Comment at:
> packages/Python/lldbsuite/test/functionalities/expr_completion/main.cpp:1
> +#include
> +
>
> The rest of this test is test is nicely self-contained. Could we avoid
> std::string here and make it fully hermetic?
>
>
>
> Comment at: source/Commands/CommandObjectExpression.cpp:419-423
> +// We got the index of the arg and the cursor index inside that arg as
> +// parameters, but for completing the whole expression we need to revert
> +// this back to the absolute offset of the cursor in the whole
> expression.
> +int absolute_pos =
> +WordPosToAbsolutePos(arg, cursor_index, cursor_char_position);
>
> Will this work correctly if the expression command contains arguments (`expr
> -i0 -- my_expression`)? What about quoted strings (`expr "string with
> spaces`)?
>
> Have you looked at whether it would be possible to refactor the completion
> api to provide the absolute position (it has to exist at some point), instead
> of trying to reverse-engineer it here?
>
> I think the problem here is that the completion api has this built-in
> assumption that you're only ever going to be completing "parsed" commands,
> which is why you get the input as an `Args` array and a two-dimensional
> cursor position. "expr" command takes "raw" input, which means it does not go
> through the usual word-splitting and getopt parsing. You can see that because
> CommandObjectExpression::DoExecute takes a `const char *command`, whereas for
> "parsed" commands (e.g. "frame variable") the DoExecute function takes the
> argument as `Args &command`.
>
> I think it would make sense to start this by refactoring the completion api
> to behave the same way as the "DoExecute" api. For "raw" commands , the
> HandleCompletion function would take a raw string + absolute position in that
> string, and the parsed commands would get the an `Args` array plus the
> two-dimensional position. Without that, you're going to get subtle
> differences in how the expression is treated for completion purposes and for
> actual evaluation.
If you are going to do this, I think the job would be easier if we first make
the option parsing for raw commands be less ad hoc. Right now
CommandObjectExpression::DoExecute has the code to search for a -- and hand
parse the options up to the --. Then it goes on to deal with the raw part. If
you don't straighten this out, then even if you make a "HandleCompletion" for
raw commands, you will still have to hack around the option part.
[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync
teemperor added a reviewer: labath.
teemperor added a subscriber: labath.
teemperor added a comment.
Adding Pavel because he wrote the PrintAsync code.
Also @labath: Can you tell me what variables/functionality the `m_output_mutex`
in Editline.cpp is supposed to shield? I don't see any documentation for that.
The `m_output_mutex` name suggests its related to console output, but we
actually take the lock also when reading *input*. Especially in
`EditLine::GetLine` we take a guard on the lock but then somehow unlock the
guarded mutex from inside `Editline::GetCharacter` that we call afterwards
(which completely breaks this patch):
// This mutex is locked by our caller (GetLine). Unlock it while we read a
// character (blocking operation), so we do not hold the mutex
// indefinitely. This gives a chance for someone to interrupt us. After
// Read returns, immediately lock the mutex again and check if we were
// interrupted.
m_output_mutex.unlock();
int read_count = m_input_connection.Read(&ch, 1, llvm::None, status, NULL);
m_output_mutex.lock();
if (m_editor_status == EditorStatus::Interrupted) {
while (read_count > 0 && status == lldb::eConnectionStatusSuccess)
read_count = m_input_connection.Read(&ch, 1, llvm::None, status, NULL);
lldbassert(status == lldb::eConnectionStatusInterrupted);
return 0;
}
https://reviews.llvm.org/D48463
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe
> On Jun 22, 2018, at 4:05 AM, Pavel Labath wrote: > > On Wed, 20 Jun 2018 at 23:21, Jim Ingham wrote: >> >> It is not uncommon that you would be parsing the DWARF for module A and find >> a type that is only known as a forward declaration. In that case, lldb will >> look through the other Modules' debug info for a real definition, parse that >> and import it into module A. So you would need to suspend one task, start >> another and wait on its completion. >> > > Taking a step back, I was wondering what are the situations when we do > this kind of cross-module debug info importing? I was under the > impression that we don't do this kind importing precisely because the > module can be shared between multiple targets (and we don't want > information from a module which is not loaded in a given target to > leak into it just because we happen to have a different target with > that module around). I think these kinds of things came up during the > discussions about why lldb behaves poorly under -fno-standalone-debug. > > Am I misunderstanding something here? Because if this is true (parsing > debug info in a given module does not access other modules), then > maybe we could solve this by just taking some kind of a module lock > when parsing debug info in the given module. We only do this when we are parsing DWARF on behalf of the expression parser's AST context. That context is not shared among targets, so it doesn't cause the sort of problems you are worried about. The target AST context looks in itself for definitions and then dispatches to the module's AST contexts to look for, parse and import into itself definitions it doesn't know or hasn't completed yet. I don't remember whether we pause mid-stream to find unknown types in this case, or do it as a cleanup. I haven't looked at that code in years. Greg probably remembers it better than I do. Note, it is in fact a longstanding bug that, if you are in a frame of a module that only has a forward declaration of type Foo, which exists in full in another module, "expr" will helpfully print the full type, but "frame var" will not. That's tricky to solve for the reasons you specify. The target would need to keep a per-module side table of types that augment the ones in the modules and use that somehow. It's not clear how to make this work, and the bug has languished for most of the life of lldb... But it is not good for the debugger to have two ways to print values - both of which are in common usage because the "frame variable" path is how all GUI debuggers present local variables - which give different answers for the same value. So I still think the bug is important, and I don't want to make this more difficult. OTOH, however we solve it, the target-inferred extra types can't go into the Module based AST contexts for the reasons you outline, so it seems like a good assumption that if you are parsing module A, that can't cause you to parse module B. But we might have to introduce some more distinctions between how we ingest DWARF for the scratch AST contexts and for the module AST contexts. Jim ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
lemo added a comment.
> The slight complication here is that
> some clients (MachO) actually use the all-zero notation to mean "no UUID
> has been set". To keep this use case working, I have introduced an
> additional argument to the UUID constructor, which specifies whether an
> all-zero vector should be considered a valid UUID. For the usages where
> the UUID data comes from a MachO file, I set this argument to false.
What is the distinction between "no UUID has been set" and "invalid UUID"? I
find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?
Comment at: include/lldb/Utility/UUID.h:31
// Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
typedef uint8_t ValueType[20];
switch to llvm::SmallVector as suggested by Greg?
Comment at: include/lldb/Utility/UUID.h:42
- const UUID &operator=(const UUID &rhs);
+ UUID &operator=(const UUID &rhs) = default;
If we define this we should define at least the copy constructor as well (rule
of 3/5). In this case the cleanest solution may be to allow the implicit
definitions (which would also allow the implicit move operations - defining
assignment operator as defaulted would inhibit the implicit move SMFs)
Comment at: include/lldb/Utility/UUID.h:52
- bool IsValid() const;
+ explicit operator bool() const { return IsValid(); }
+ bool IsValid() const { return m_num_uuid_bytes > 0; }
is this really needed? I'd prefer the truly explicit (pun intended) IsValid()
Comment at: include/lldb/Utility/UUID.h:95
bool operator==(const UUID &lhs, const UUID &rhs);
bool operator!=(const UUID &lhs, const UUID &rhs);
these can be simplified if we use llvm::SmallVector (or std::vector)
https://reviews.llvm.org/D48479
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync
clayborg added inline comments.
Comment at: source/Core/Debugger.cpp:988-1004
+ bool should_forward = false;
+ {
+// We check if any user requested to delay output to a later time
+// (which is designated by m_delayed_output_counter being not 0).
+std::lock_guard guard(m_delayed_output_mutex);
+if (m_delayed_output_counter != 0) {
+ // We want to delay messages, so push them to the buffer.
Can this code become:
```
// We check if any user requested to delay output to a later time
// (which is designated by m_delayed_output_counter being not 0).
{
std::lock_guard guard(m_delayed_output_mutex);
if (m_delayed_output_counter != 0) {
// We want to delay messages, so push them to the buffer.
m_delayed_output.emplace_back(std::string(s, len), is_stdout);
return;
}
}
lldb::StreamFileSP stream = is_stdout ? GetOutputFile() : GetErrorFile();
m_input_reader_stack.PrintAsync(stream.get(), s, len);
```
Comment at: source/Core/IOHandler.cpp:358
+ // lldb code that will be called from here (possibly in another thread).
+ Debugger::MessageDelayScope buffer_scope(m_debugger);
+
So anytime we have a "(lldb)" prompt we won't be able to output something?
https://reviews.llvm.org/D48463
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`
davide created this revision.
davide added reviewers: friss, aprantl, clayborg, labath.
Herald added a subscriber: JDevlieghere.
To the best of my understanding modern compilers handle all these cases
correctly. So, I think this is basically dead code.
https://reviews.llvm.org/D48500
Files:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -307,14 +307,7 @@
decl.SetColumn(form_value.Unsigned());
break;
case DW_AT_name:
-
type_name_cstr = form_value.AsCString();
-// Work around a bug in llvm-gcc where they give a name to a
-// reference type which doesn't include the "&"...
-if (tag == DW_TAG_reference_type) {
- if (strchr(type_name_cstr, '&') == NULL)
-type_name_cstr = NULL;
-}
if (type_name_cstr)
type_name_const_str.SetCString(type_name_cstr);
break;
@@ -558,16 +551,9 @@
if (attributes.ExtractFormValueAtIndex(i, form_value)) {
switch (attr) {
case DW_AT_decl_file:
-if (die.GetCU()->DW_AT_decl_file_attributes_are_invalid()) {
- // llvm-gcc outputs invalid DW_AT_decl_file attributes that
- // always point to the compile unit file, so we clear this
- // invalid value so that we can still unique types
- // efficiently.
- decl.SetFile(FileSpec("", false));
-} else
- decl.SetFile(
- sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
- form_value.Unsigned()));
+decl.SetFile(
+ sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
+ form_value.Unsigned()));
break;
case DW_AT_decl_line:
@@ -2977,15 +2963,6 @@
class_language == eLanguageTypeObjC_plus_plus)
accessibility = eAccessNone;
-if (member_idx == 0 && !is_artificial && name &&
-(strstr(name, "_vptr$") == name)) {
- // Not all compilers will mark the vtable pointer member as
- // artificial (llvm-gcc). We can't have the virtual members in our
- // classes otherwise it throws off all child offsets since we end up
- // having and extra pointer sized member in our class layouts.
- is_artificial = true;
-}
-
// Handle static members
if (is_external && member_byte_offset == UINT32_MAX) {
Type *var_type = die.ResolveTypeUID(DIERef(encoding_form));
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -307,14 +307,7 @@
decl.SetColumn(form_value.Unsigned());
break;
case DW_AT_name:
-
type_name_cstr = form_value.AsCString();
-// Work around a bug in llvm-gcc where they give a name to a
-// reference type which doesn't include the "&"...
-if (tag == DW_TAG_reference_type) {
- if (strchr(type_name_cstr, '&') == NULL)
-type_name_cstr = NULL;
-}
if (type_name_cstr)
type_name_const_str.SetCString(type_name_cstr);
break;
@@ -558,16 +551,9 @@
if (attributes.ExtractFormValueAtIndex(i, form_value)) {
switch (attr) {
case DW_AT_decl_file:
-if (die.GetCU()->DW_AT_decl_file_attributes_are_invalid()) {
- // llvm-gcc outputs invalid DW_AT_decl_file attributes that
- // always point to the compile unit file, so we clear this
- // invalid value so that we can still unique types
- // efficiently.
- decl.SetFile(FileSpec("", false));
-} else
- decl.SetFile(
- sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
- form_value.Unsigned()));
+decl.SetFile(
+ sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
+ form_value.Unsigned()));
break;
case DW_AT_decl_line:
@@ -2977,15 +2963,6 @@
class_language == eLanguageTypeObjC_plus_plus)
accessibility = eAccessNone;
-if (member_idx == 0 && !is_arti
[Lldb-commits] [PATCH] D47992: [lldb-mi] Clean up and update a few MI commands.
apolyakov added a comment. Would be really nice to get review of this patch before this weekend. Thanks for your time, folks. https://reviews.llvm.org/D47992 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
labath added a comment.
In https://reviews.llvm.org/D48479#1140927, @lemo wrote:
> > The slight complication here is that
> > some clients (MachO) actually use the all-zero notation to mean "no UUID
> > has been set". To keep this use case working, I have introduced an
> > additional argument to the UUID constructor, which specifies whether an
> > all-zero vector should be considered a valid UUID. For the usages where
> > the UUID data comes from a MachO file, I set this argument to false.
>
> What is the distinction between "no UUID has been set" and "invalid UUID"? I
> find this subtlety confusing, can we simplify it to just valid/not-valid
> UUIDs?
That is what I am trying to do (although not completely successfully, it seems
;) ). Once you have a UUID object around there should be no distinction. You
either have a valid uuid (for now, consisting of 16 or 20 bytes), or you don't.
However, during parsing you need to know the meaning of a "...0" UUID. In a
MachO file (at least based on the comments in the code) this value is used to
denote the fact that the object file has no UUID. For elf, a "000..0" build-id
is a perfectly valid identifier (and the lack of a build-id is denoted by the
absence of the build-id section). The extra constructor argument is my way of
trying to support both scenarios. The other possibility I see is to have a some
kind of a factory function for one of the options (or both). I don't really
have a preference between the two.
Comment at: include/lldb/Utility/UUID.h:31
// Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
typedef uint8_t ValueType[20];
lemo wrote:
> switch to llvm::SmallVector as suggested by Greg?
I'm deliberately keeping that for a separate patch. Here, I just want to
prepare the ground by defining the "invalid" UUID more clearly. The part with
the arbitrary UUID length will come after that.
Comment at: include/lldb/Utility/UUID.h:42
- const UUID &operator=(const UUID &rhs);
+ UUID &operator=(const UUID &rhs) = default;
lemo wrote:
> If we define this we should define at least the copy constructor as well
> (rule of 3/5). In this case the cleanest solution may be to allow the
> implicit definitions (which would also allow the implicit move operations -
> defining assignment operator as defaulted would inhibit the implicit move
> SMFs)
Good point. I've deleted the copy constructor altogether.
Comment at: include/lldb/Utility/UUID.h:52
- bool IsValid() const;
+ explicit operator bool() const { return IsValid(); }
+ bool IsValid() const { return m_num_uuid_bytes > 0; }
lemo wrote:
> is this really needed? I'd prefer the truly explicit (pun intended) IsValid()
My main reason for an `operator bool` is that it allows the if-declaration
syntax (`if (UUID u = getUUID()) doSomethingUsefulWith(u);` The most llvm-y
solution would be not have neither of these methods and use Optional when
you don't know if you have one, but as far as operator bool vs. isValid goes,
both styles are used in llvm (and lldb).
https://reviews.llvm.org/D48479
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
labath updated this revision to Diff 152530.
labath added a comment.
Delete copy constructor.
https://reviews.llvm.org/D48479
Files:
include/lldb/Utility/UUID.h
source/API/SBModuleSpec.cpp
source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Utility/DataExtractor.cpp
source/Utility/UUID.cpp
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
unittests/Utility/UUIDTest.cpp
Index: unittests/Utility/UUIDTest.cpp
===
--- unittests/Utility/UUIDTest.cpp
+++ unittests/Utility/UUIDTest.cpp
@@ -15,10 +15,10 @@
TEST(UUIDTest, RelationalOperators) {
UUID empty;
- UUID a16("1234567890123456", 16);
- UUID b16("1234567890123457", 16);
- UUID a20("12345678901234567890", 20);
- UUID b20("12345678900987654321", 20);
+ UUID a16("1234567890123456", 16, true);
+ UUID b16("1234567890123457", 16, true);
+ UUID a20("12345678901234567890", 20, true);
+ UUID b20("12345678900987654321", 20, true);
EXPECT_EQ(empty, empty);
EXPECT_EQ(a16, a16);
@@ -34,3 +34,40 @@
EXPECT_LT(a16, b16);
EXPECT_GT(a20, b20);
}
+
+TEST(UUIDTest, Validity) {
+ UUID empty;
+ std::vector zeroes(20, 0);
+ UUID a16(zeroes.data(), 16, true);
+ UUID a20(zeroes.data(), 20, true);
+ UUID a16_0(zeroes.data(), 16, false);
+ UUID a20_0(zeroes.data(), 20, false);
+ EXPECT_FALSE(empty);
+ EXPECT_TRUE(a16);
+ EXPECT_TRUE(a20);
+ EXPECT_FALSE(a16_0);
+ EXPECT_FALSE(a20_0);
+}
+
+TEST(UUIDTest, SetFromStringRef) {
+ UUID u;
+ EXPECT_EQ(32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+ EXPECT_EQ(36, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+ EXPECT_EQ(45, u.SetFromStringRef(
+"40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u);
+
+ EXPECT_EQ(0, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
+ EXPECT_EQ(0, u.SetFromStringRef("40x"));
+ EXPECT_EQ(0, u.SetFromStringRef(""));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u)
+ << "uuid was changed by failed parse calls";
+
+ EXPECT_EQ(
+ 32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+}
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -194,7 +194,7 @@
ASSERT_EQ(1u, result->size());
EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
- EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16), result.getValue()[0].GetUUID());
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), result.getValue()[0].GetUUID());
EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
}
@@ -218,7 +218,8 @@
ASSERT_EQ(1u, result->size());
EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
- EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20), result.getValue()[0].GetUUID());
+ EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true),
+result.getValue()[0].GetUUID());
EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
}
Index: source/Utility/UUID.cpp
===
--- source/Utility/UUID.cpp
+++ source/Utility/UUID.cpp
@@ -21,29 +21,8 @@
using namespace lldb_private;
-UUID::UUID() { Clear(); }
-
-UUID::UUID(const UUID &rhs) {
- SetBytes(rhs.m_uuid, rhs.m_num_uuid_bytes);
-}
-
-UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes) {
- SetBytes(uuid_bytes, num_uuid_bytes);
-}
-
-const UUID &UUID::operator=(const UUID &rhs) {
- if (this != &rhs) {
-m_num_uuid_bytes = rhs.m_num_uuid_bytes;
-::memcpy(m_uuid, rhs.m_uuid, sizeof(m_uuid));
- }
- return *this;
-}
-
-UUID::~UUID() {}
-
-void UUID::Clear() {
- m_num_uuid_bytes = 16;
- ::memset(m_uuid, 0, sizeof(m_uuid));
+UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes, bool accept_zeroes) {
+ SetBytes(uuid_bytes, num_uuid_bytes, accept_zeroes);
}
std::string UUID::GetAsString(const char *separator) const {
@@ -74,36 +53,20 @@
s->PutCString(GetAsString().c_str());
}
-bool UUID::SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes) {
- if (uuid_bytes)
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
lemo added subscribers: clayborg, labath, sas. lemo added a comment. > However, during parsing you need to know the meaning of a "...0" UUID. > In a MachO file (at least based on the comments in the code) this value is > used to denote the fact that the object file has no UUID. For elf, a > "000..0" build-id is a perfectly valid identifier (and the lack of a > build-id is denoted by the absence of the build-id section). The extra > constructor argument is my way of trying to support both scenarios. The > other possibility I see is to have a some kind of a factory function for > one of the options (or both). I don't really have a preference between the > two. One solution might be to encapsulate the MachO convention in the MachO code: check in there (maybe through a helper function) if the UUID is "000...0" and map it to the empty UUID in that case. The UUID interface would not have to know/care about this convention. Would this work? https://reviews.llvm.org/D48479 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
>
> However, during parsing you need to know the meaning of a "...0" UUID.
> In a MachO file (at least based on the comments in the code) this value is
> used to denote the fact that the object file has no UUID. For elf, a
> "000..0" build-id is a perfectly valid identifier (and the lack of a
> build-id is denoted by the absence of the build-id section). The extra
> constructor argument is my way of trying to support both scenarios. The
> other possibility I see is to have a some kind of a factory function for
> one of the options (or both). I don't really have a preference between the
> two.
One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?
On Fri, Jun 22, 2018 at 12:02 PM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a comment.
>
> In https://reviews.llvm.org/D48479#1140927, @lemo wrote:
>
> > > The slight complication here is that
> > > some clients (MachO) actually use the all-zero notation to mean "no
> UUID
> > > has been set". To keep this use case working, I have introduced an
> > > additional argument to the UUID constructor, which specifies whether
> an
> > > all-zero vector should be considered a valid UUID. For the usages
> where
> > > the UUID data comes from a MachO file, I set this argument to false.
> >
> > What is the distinction between "no UUID has been set" and "invalid
> UUID"? I find this subtlety confusing, can we simplify it to just
> valid/not-valid UUIDs?
>
>
> That is what I am trying to do (although not completely successfully, it
> seems ;) ). Once you have a UUID object around there should be no
> distinction. You either have a valid uuid (for now, consisting of 16 or 20
> bytes), or you don't.
>
> However, during parsing you need to know the meaning of a "...0" UUID.
> In a MachO file (at least based on the comments in the code) this value is
> used to denote the fact that the object file has no UUID. For elf, a
> "000..0" build-id is a perfectly valid identifier (and the lack of a
> build-id is denoted by the absence of the build-id section). The extra
> constructor argument is my way of trying to support both scenarios. The
> other possibility I see is to have a some kind of a factory function for
> one of the options (or both). I don't really have a preference between the
> two.
>
>
>
>
> Comment at: include/lldb/Utility/UUID.h:31
>// Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
>typedef uint8_t ValueType[20];
>
>
> lemo wrote:
> > switch to llvm::SmallVector as suggested by Greg?
> I'm deliberately keeping that for a separate patch. Here, I just want to
> prepare the ground by defining the "invalid" UUID more clearly. The part
> with the arbitrary UUID length will come after that.
>
>
>
> Comment at: include/lldb/Utility/UUID.h:42
>
> - const UUID &operator=(const UUID &rhs);
> + UUID &operator=(const UUID &rhs) = default;
>
>
> lemo wrote:
> > If we define this we should define at least the copy constructor as well
> (rule of 3/5). In this case the cleanest solution may be to allow the
> implicit definitions (which would also allow the implicit move operations -
> defining assignment operator as defaulted would inhibit the implicit move
> SMFs)
> Good point. I've deleted the copy constructor altogether.
>
>
>
> Comment at: include/lldb/Utility/UUID.h:52
>
> - bool IsValid() const;
> + explicit operator bool() const { return IsValid(); }
> + bool IsValid() const { return m_num_uuid_bytes > 0; }
>
> lemo wrote:
> > is this really needed? I'd prefer the truly explicit (pun intended)
> IsValid()
> My main reason for an `operator bool` is that it allows the if-declaration
> syntax (`if (UUID u = getUUID()) doSomethingUsefulWith(u);` The most
> llvm-y solution would be not have neither of these methods and use
> Optional when you don't know if you have one, but as far as operator
> bool vs. isValid goes, both styles are used in llvm (and lldb).
>
>
> https://reviews.llvm.org/D48479
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
labath added a comment. In https://reviews.llvm.org/D48479#1141067, @lemo wrote: > One solution might be to encapsulate the MachO convention in the MachO > > code: check in there (maybe through a helper function) if the UUID is > "000...0" and map it to the empty UUID in that case. The UUID interface > would not have to know/care about this convention. Would this work? I'm not sure if there is a suitable place for that function. This is needed in "ObjectFileMachO" and two dynamic loader plugins. https://reviews.llvm.org/D48479 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
lemo added a comment.
> I'm not sure if there is a suitable place for that function. This is
> needed in "ObjectFileMachO" and two dynamic loader plugins.
Then your factory idea may be the next best thing. While we're at it, maybe
we can remove the UUID::SetBytes() from the public interface, and make the
UUID an immutable value type:
Ex. instead of:
UUID uuid;
...
uuid.SetBytes(...)
We'd have:
UUID uuid;
uuid = UUID(...);
// or
uuid = { ... };
// or
uuid = UUID::factory(...);
What do you think?
https://reviews.llvm.org/D48479
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
>
> I'm not sure if there is a suitable place for that function. This is
> needed in "ObjectFileMachO" and two dynamic loader plugins.
Then your factory idea may be the next best thing. While we're at it, maybe
we can remove the UUID::SetBytes() from the public interface, and make the
UUID an immutable value type:
Ex. instead of:
UUID uuid;
...
uuid.SetBytes(...)
We'd have:
UUID uuid;
uuid = UUID(...);
// or
uuid = { ... };
// or
uuid = UUID::factory(...);
What do you think?
On Fri, Jun 22, 2018 at 12:29 PM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a comment.
>
> In https://reviews.llvm.org/D48479#1141067, @lemo wrote:
>
> > One solution might be to encapsulate the MachO convention in the MachO
> >
> > code: check in there (maybe through a helper function) if the UUID is
> > "000...0" and map it to the empty UUID in that case. The UUID interface
> > would not have to know/care about this convention. Would this work?
>
>
> I'm not sure if there is a suitable place for that function. This is
> needed in "ObjectFileMachO" and two dynamic loader plugins.
>
>
> https://reviews.llvm.org/D48479
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r335386 - Mark this test as no debuginfo
Author: adrian Date: Fri Jun 22 13:26:53 2018 New Revision: 335386 URL: http://llvm.org/viewvc/llvm-project?rev=335386&view=rev Log: Mark this test as no debuginfo Modified: lldb/trunk/packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py Modified: lldb/trunk/packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py?rev=335386&r1=335385&r2=335386&view=diff == --- lldb/trunk/packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py (original) +++ lldb/trunk/packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py Fri Jun 22 13:26:53 2018 @@ -14,7 +14,7 @@ from lldbsuite.test import lldbutil class HelloWorldTestCase(TestBase): - +NO_DEBUG_INFO_TESTCASE = True mydir = TestBase.compute_mydir(__file__) def setUp(self): ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync
labath added a comment. In https://reviews.llvm.org/D48463#1140861, @teemperor wrote: > Adding Pavel because he wrote the PrintAsync code. > > Also @labath: Can you tell me what variables/functionality the > `m_output_mutex` in Editline.cpp is supposed to shield? I don't see any > documentation for that. > > The `m_output_mutex` name suggests its related to console output, but we > actually take the lock also when reading *input*. Especially in > `EditLine::GetLine` we take a guard on the lock but then somehow unlock the > guarded mutex from inside `Editline::GetCharacter` that we call afterwards > (which completely breaks this patch): If I remember correctly (it was a long time ago), the idea was behind this was to make sure that nobody prints anything to stdout while libedit is playing around with it. That's why it's called "output" mutex. The thing that this (admittedly strange) locking strategy achieves is that the the only time the `PrintAsync` function can write to stdout is while we are blocked waiting for input, as that's the place we can be sure we aren't writing to stdout -- before, or after that, we could be in libedit code and it could mess with stdout to write it's prompt or whatever. IIRC, PrintAsync should eventually end up taking the same mutex, and then depending on the current state of libedit machinery, do the "right thing" wrt. erasing the prompt et al. Hmm.. Now that I have spelled this out, it sounds like this same pattern could be used to achieve your goal too -- while you're inside the completion handler, you also shouldn't be doing any funny things with stdout. Maybe the solution would be do just drop the output mutex while the running the completion code. Could you try if something like that would work? > > > // This mutex is locked by our caller (GetLine). Unlock it while we read a > // character (blocking operation), so we do not hold the mutex > // indefinitely. This gives a chance for someone to interrupt us. After > // Read returns, immediately lock the mutex again and check if we were > // interrupted. This comment refers to the second purpose of the surrounding code, which is to make sure that any interrupt request is handled exactly once. https://reviews.llvm.org/D48463 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`
aprantl added a comment. I don't have a problem with dropping compatibility with llvm-gcc in LLDB, but I should point out that LLDB generally wants to be able to debug code produced by a wide range of compilers, including old ones. https://reviews.llvm.org/D48500 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`
davide added a comment. I think it's fair and correct supporting everything we can, but I guess `llvm-gcc` is largely dead at this point. https://reviews.llvm.org/D48500 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`
aprantl added a comment. People might have a legitimate reason to debug very old code, e.g., for backporting security fixes or similar. On the other hand one might argue that they could just do this with a debugger from the same era. https://reviews.llvm.org/D48500 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47992: [lldb-mi] Clean up and update a few MI commands.
aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.
Comment at: tools/lldb-mi/MICmdCmdExec.cpp:137
+ auto successHandler = [this] {
+// CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
+if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
Is this a copy&paste error?
Comment at: tools/lldb-mi/MICmdCmdExec.cpp:142
+ MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
+ this->m_cmdData.strMiCmd.c_str(),
+ rErrMsg.c_str()));
Might want to factor this out into a HandleBoolReturnValue helper if it appears
more than once.
https://reviews.llvm.org/D47992
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero
clayborg added a comment. Would love to remove the "accept_zeroes" argument everywhere. Too much matching happens in LLDB and we can't have multiple shared libraries claiming zeros as their UUID Comment at: include/lldb/Utility/UUID.h:54 + void SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes, +bool accept_zeroes); + void SetBytes(llvm::ArrayRef bytes, bool accept_zeroes); I am not sure accept_zeroes is a good idea. LLDB does a lot of matching based on UUID and we can't have multiple shared libraries claiming to have zeroes as their UUID. It will cause chaos. Zeroes as a build ID is not very useful https://reviews.llvm.org/D48479 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r335401 - Update cmdtemplate.py to use best pratices.
Author: gclayton
Date: Fri Jun 22 16:34:24 2018
New Revision: 335401
URL: http://llvm.org/viewvc/llvm-project?rev=335401&view=rev
Log:
Update cmdtemplate.py to use best pratices.
Fixes include:
- fix all lint errors
- add code that will automatically register and LLDB command classes by
detecting the classes and any classes that have a "register_lldb_command"
function
- automatically fill in the correct module name when registering commands
- automatically fill in the class name when registering command
Modified:
lldb/trunk/examples/python/cmdtemplate.py
Modified: lldb/trunk/examples/python/cmdtemplate.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/examples/python/cmdtemplate.py?rev=335401&r1=335400&r2=335401&view=diff
==
--- lldb/trunk/examples/python/cmdtemplate.py (original)
+++ lldb/trunk/examples/python/cmdtemplate.py Fri Jun 22 16:34:24 2018
@@ -1,69 +1,91 @@
#!/usr/bin/python
-#--
+# -
# Be sure to add the python path that points to the LLDB shared library.
#
# # To use this in the embedded python interpreter using "lldb" just
# import it with the full path using the "command script import"
# command
# (lldb) command script import /path/to/cmdtemplate.py
-#--
+# -
+import inspect
import lldb
-import commands
import optparse
import shlex
+import sys
+
class FrameStatCommand:
-def create_options(self):
+program = 'framestats'
+
+@classmethod
+def register_lldb_command(cls, debugger, module_name):
+parser = cls.create_options()
+cls.__doc__ = parser.format_help()
+# Add any commands contained in this module to LLDB
+command = 'command script add -c %s.%s %s' % (module_name,
+ cls.__name__,
+ cls.program)
+debugger.HandleCommand(command)
+print('The "{0}" command has been installed, type "help {0}" or "{0} '
+ '--help" for detailed help.'.format(cls.program))
+
+@classmethod
+def create_options(cls):
usage = "usage: %prog [options]"
-description = '''This command is meant to be an example of how to make
an LLDB command that
-does something useful, follows best practices, and exploits the SB API.
-Specifically, this command computes the aggregate and average size of the
variables in the current frame
-and allows you to tweak exactly which variables are to be accounted in the
computation.
-'''
-
-# Pass add_help_option = False, since this keeps the command in line
with lldb commands,
-# and we wire up "help command" to work by providing the long & short
help methods below.
-self.parser = optparse.OptionParser(
-description = description,
-prog = 'framestats',
-usage = usage,
-add_help_option = False)
+description = ('This command is meant to be an example of how to make '
+ 'an LLDB command that does something useful, follows '
+ 'best practices, and exploits the SB API. '
+ 'Specifically, this command computes the aggregate '
+ 'and average size of the variables in the current '
+ 'frame and allows you to tweak exactly which variables '
+ 'are to be accounted in the computation.')
+
+# Pass add_help_option = False, since this keeps the command in line
+# with lldb commands, and we wire up "help command" to work by
+# providing the long & short help methods below.
+parser = optparse.OptionParser(
+description=description,
+prog=cls.program,
+usage=usage,
+add_help_option=False)
-self.parser.add_option(
+parser.add_option(
'-i',
'--in-scope',
-action = 'store_true',
-dest = 'inscope',
-help = 'in_scope_only = True',
-default = True)
+action='store_true',
+dest='inscope',
+help='in_scope_only = True',
+default=True)
-self.parser.add_option(
+parser.add_option(
'-a',
'--arguments',
-action = 'store_true',
-dest = 'arguments',
-help = 'arguments = True',
-default = True)
+action='store_true',
+dest='arguments',
+help='arguments = True',
+default=True)
-self.parser.add_option(
+parser.add_option(
'-l',
'--loca
Re: [Lldb-commits] [lldb] r335401 - Update cmdtemplate.py to use best pratices.
> On Jun 22, 2018, at 5:20 PM, Jim Ingham wrote:
>
> This is very cool!
>
> Could we make a base class for lldb commands in the lldb module that provides
> register_lldb_command? Then you wouldn't have to copy and paste this
> boiler-plate in every command file. Now that you have a generic way to add
> the commands, we could just do that automatically in "command script import"
> on every class that derives from this base when we import the module, and you
> wouldn't need to add the __lldb_init_module either.
>
> We could also provide a table driven way to specify the options, then have
> the base class actually make them using opt parse. I'd like to hide this bit
> as much as possible, to prepare for eventually transitioning over to having
> the options be real lldb command options.
>
> Jim
>
>
>> On Jun 22, 2018, at 4:34 PM, Greg Clayton via lldb-commits
>> wrote:
>>
>> Author: gclayton
>> Date: Fri Jun 22 16:34:24 2018
>> New Revision: 335401
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=335401&view=rev
>> Log:
>> Update cmdtemplate.py to use best pratices.
>>
>> Fixes include:
>> - fix all lint errors
>> - add code that will automatically register and LLDB command classes by
>> detecting the classes and any classes that have a "register_lldb_command"
>> function
>> - automatically fill in the correct module name when registering commands
>> - automatically fill in the class name when registering command
>>
>>
>> Modified:
>> lldb/trunk/examples/python/cmdtemplate.py
>>
>> Modified: lldb/trunk/examples/python/cmdtemplate.py
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/examples/python/cmdtemplate.py?rev=335401&r1=335400&r2=335401&view=diff
>> ==
>> --- lldb/trunk/examples/python/cmdtemplate.py (original)
>> +++ lldb/trunk/examples/python/cmdtemplate.py Fri Jun 22 16:34:24 2018
>> @@ -1,69 +1,91 @@
>> #!/usr/bin/python
>>
>> -#--
>> +# -
>> # Be sure to add the python path that points to the LLDB shared library.
>> #
>> # # To use this in the embedded python interpreter using "lldb" just
>> # import it with the full path using the "command script import"
>> # command
>> # (lldb) command script import /path/to/cmdtemplate.py
>> -#--
>> +# -
>>
>> +import inspect
>> import lldb
>> -import commands
>> import optparse
>> import shlex
>> +import sys
>> +
>>
>> class FrameStatCommand:
>> -def create_options(self):
>> +program = 'framestats'
>> +
>> +@classmethod
>> +def register_lldb_command(cls, debugger, module_name):
>> +parser = cls.create_options()
>> +cls.__doc__ = parser.format_help()
>> +# Add any commands contained in this module to LLDB
>> +command = 'command script add -c %s.%s %s' % (module_name,
>> + cls.__name__,
>> + cls.program)
>> +debugger.HandleCommand(command)
>> +print('The "{0}" command has been installed, type "help {0}" or
>> "{0} '
>> + '--help" for detailed help.'.format(cls.program))
>> +
>> +@classmethod
>> +def create_options(cls):
>>
>>usage = "usage: %prog [options]"
>> -description = '''This command is meant to be an example of how to
>> make an LLDB command that
>> -does something useful, follows best practices, and exploits the SB API.
>> -Specifically, this command computes the aggregate and average size of the
>> variables in the current frame
>> -and allows you to tweak exactly which variables are to be accounted in the
>> computation.
>> -'''
>> -
>> -# Pass add_help_option = False, since this keeps the command in
>> line with lldb commands,
>> -# and we wire up "help command" to work by providing the long &
>> short help methods below.
>> -self.parser = optparse.OptionParser(
>> -description = description,
>> -prog = 'framestats',
>> -usage = usage,
>> -add_help_option = False)
>> +description = ('This command is meant to be an example of how to
>> make '
>> + 'an LLDB command that does something useful, follows
>> '
>> + 'best practices, and exploits the SB API. '
>> + 'Specifically, this command computes the aggregate '
>> + 'and average size of the variables in the current '
>> + 'frame and allows you to tweak exactly which
>> variables '
>> + 'are to be accounted in the computation.')
>> +
>> +# Pass add_help_option = False, since this keeps
