[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
This revision was automatically updated to reflect the committed changes.
Closed by commit rL329677: Move Args::StringTo*** functions to a new
OptionArgParser class (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D44306
Files:
lldb/trunk/include/lldb/Interpreter/Args.h
lldb/trunk/include/lldb/Interpreter/OptionArgParser.h
lldb/trunk/include/lldb/Interpreter/Options.h
lldb/trunk/source/API/SBDebugger.cpp
lldb/trunk/source/Commands/CommandObjectBreakpoint.cpp
lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp
lldb/trunk/source/Commands/CommandObjectCommands.cpp
lldb/trunk/source/Commands/CommandObjectDisassemble.cpp
lldb/trunk/source/Commands/CommandObjectExpression.cpp
lldb/trunk/source/Commands/CommandObjectLog.cpp
lldb/trunk/source/Commands/CommandObjectMemory.cpp
lldb/trunk/source/Commands/CommandObjectProcess.cpp
lldb/trunk/source/Commands/CommandObjectSource.cpp
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Commands/CommandObjectThread.cpp
lldb/trunk/source/Commands/CommandObjectType.cpp
lldb/trunk/source/Commands/CommandObjectWatchpointCommand.cpp
lldb/trunk/source/Interpreter/Args.cpp
lldb/trunk/source/Interpreter/CMakeLists.txt
lldb/trunk/source/Interpreter/OptionArgParser.cpp
lldb/trunk/source/Interpreter/OptionGroupValueObjectDisplay.cpp
lldb/trunk/source/Interpreter/OptionGroupWatchpoint.cpp
lldb/trunk/source/Interpreter/OptionValueBoolean.cpp
lldb/trunk/source/Interpreter/OptionValueChar.cpp
lldb/trunk/source/Interpreter/OptionValueFormat.cpp
lldb/trunk/source/Interpreter/Property.cpp
lldb/trunk/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
lldb/trunk/source/Target/Process.cpp
lldb/trunk/unittests/Interpreter/CMakeLists.txt
lldb/trunk/unittests/Interpreter/TestArgs.cpp
lldb/trunk/unittests/Interpreter/TestOptionArgParser.cpp
Index: lldb/trunk/include/lldb/Interpreter/Options.h
===
--- lldb/trunk/include/lldb/Interpreter/Options.h
+++ lldb/trunk/include/lldb/Interpreter/Options.h
@@ -18,6 +18,7 @@
// Other libraries and framework includes
// Project includes
#include "lldb/Interpreter/Args.h"
+#include "lldb/Utility/Status.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-private.h"
Index: lldb/trunk/include/lldb/Interpreter/OptionArgParser.h
===
--- lldb/trunk/include/lldb/Interpreter/OptionArgParser.h
+++ lldb/trunk/include/lldb/Interpreter/OptionArgParser.h
@@ -0,0 +1,43 @@
+//===-- OptionArgParser.h ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLDB_INTERPRETER_OPTIONARGPARSER_H
+#define LLDB_INTERPRETER_OPTIONARGPARSER_H
+
+#include "lldb/lldb-private-types.h"
+
+namespace lldb_private {
+
+struct OptionArgParser {
+ static lldb::addr_t ToAddress(const ExecutionContext *exe_ctx,
+llvm::StringRef s, lldb::addr_t fail_value,
+Status *error);
+
+ static bool ToBoolean(llvm::StringRef s, bool fail_value, bool *success_ptr);
+
+ static char ToChar(llvm::StringRef s, char fail_value, bool *success_ptr);
+
+ static int64_t ToOptionEnum(llvm::StringRef s,
+ OptionEnumValueElement *enum_values,
+ int32_t fail_value, Status &error);
+
+ static lldb::ScriptLanguage ToScriptLanguage(llvm::StringRef s,
+ lldb::ScriptLanguage fail_value,
+ bool *success_ptr);
+
+ // TODO: Use StringRef
+ static Status ToFormat(const char *s, lldb::Format &format,
+ size_t *byte_size_ptr); // If non-NULL, then a
+ // byte size can precede
+ // the format character
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_INTERPRETER_OPTIONARGPARSER_H
Index: lldb/trunk/include/lldb/Interpreter/Args.h
===
--- lldb/trunk/include/lldb/Interpreter/Args.h
+++ lldb/trunk/include/lldb/Interpreter/Args.h
@@ -12,7 +12,6 @@
// C Includes
// C++ Includes
-#include
#include
#include
#include
@@ -22,7 +21,6 @@
#
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath added a comment. Thank you. PS: I will need someone to update the XCode project after this. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Yes, with your updated naming, I am fine with this. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath added a comment. Jim, judging by the comment in https://reviews.llvm.org/D44306#1037587 you were fine with this patch except the naming part. Now that that's fixed, I hope this should be fine. Unless I hear from you, I am going to commit this so I can continue with re-layering the `Args` class. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath added a comment. So, any objections to this patch? https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath updated this revision to Diff 138540.
labath added a comment.
s/toAddress/ToAddress/
https://reviews.llvm.org/D44306
Files:
include/lldb/Interpreter/Args.h
include/lldb/Interpreter/OptionArgParser.h
include/lldb/Interpreter/Options.h
source/API/SBDebugger.cpp
source/Commands/CommandObjectBreakpoint.cpp
source/Commands/CommandObjectBreakpointCommand.cpp
source/Commands/CommandObjectCommands.cpp
source/Commands/CommandObjectDisassemble.cpp
source/Commands/CommandObjectExpression.cpp
source/Commands/CommandObjectLog.cpp
source/Commands/CommandObjectMemory.cpp
source/Commands/CommandObjectProcess.cpp
source/Commands/CommandObjectSource.cpp
source/Commands/CommandObjectTarget.cpp
source/Commands/CommandObjectThread.cpp
source/Commands/CommandObjectType.cpp
source/Commands/CommandObjectWatchpointCommand.cpp
source/Interpreter/Args.cpp
source/Interpreter/CMakeLists.txt
source/Interpreter/OptionArgParser.cpp
source/Interpreter/OptionGroupValueObjectDisplay.cpp
source/Interpreter/OptionGroupWatchpoint.cpp
source/Interpreter/OptionValueBoolean.cpp
source/Interpreter/OptionValueChar.cpp
source/Interpreter/OptionValueFormat.cpp
source/Interpreter/Property.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
source/Target/Process.cpp
unittests/Interpreter/CMakeLists.txt
unittests/Interpreter/TestArgs.cpp
unittests/Interpreter/TestOptionArgParser.cpp
Index: unittests/Interpreter/TestOptionArgParser.cpp
===
--- /dev/null
+++ unittests/Interpreter/TestOptionArgParser.cpp
@@ -0,0 +1,121 @@
+//===-- ArgsTest.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+#include "lldb/Interpreter/OptionArgParser.h"
+
+using namespace lldb_private;
+
+TEST(OptionArgParserTest, toBoolean) {
+ bool success = false;
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("true"), false, nullptr));
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("on"), false, nullptr));
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("yes"), false, nullptr));
+ EXPECT_TRUE(OptionArgParser::ToBoolean(llvm::StringRef("1"), false, nullptr));
+
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("true"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("on"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("yes"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("1"), false, &success));
+ EXPECT_TRUE(success);
+
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("false"), true, nullptr));
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("off"), true, nullptr));
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("no"), true, nullptr));
+ EXPECT_FALSE(OptionArgParser::ToBoolean(llvm::StringRef("0"), true, nullptr));
+
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("false"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("off"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("no"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("0"), true, &success));
+ EXPECT_TRUE(success);
+
+ EXPECT_FALSE(
+ OptionArgParser::ToBoolean(llvm::StringRef("10"), false, &success));
+ EXPECT_FALSE(success);
+ EXPECT_TRUE(
+ OptionArgParser::ToBoolean(llvm::StringRef("10"), true, &success));
+ EXPECT_FALSE(success);
+ EXPECT_TRUE(OptionArgParser::ToBoolean(llvm::StringRef(""), true, &success));
+ EXPECT_FALSE(success);
+}
+
+TEST(OptionArgParserTest, toChar) {
+ bool success = false;
+
+ EXPECT_EQ('A', OptionArgParser::ToChar("A", 'B', nullptr));
+ EXPECT_EQ('B', OptionArgParser::ToChar("B", 'A', nullptr));
+
+ EXPECT_EQ('A', OptionArgParser::ToChar("A", 'B', &success));
+ EXPECT_TRUE(success);
+ EXPECT_EQ('B', OptionArgParser::ToChar("B", 'A', &success));
+ EXPECT_TRUE(success);
+
+ EXPECT_EQ('A', OptionArgParser::ToChar("", 'A', &success));
+ EXPECT_FALSE(success);
+ EXPECT_EQ('A', OptionArgParser::ToChar("ABC", 'A', &success));
+ EXPECT_FA
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath added a comment. I agree with Jim. I deliberately put here only the types that are used for command parsing, since I presume these are the things that the Command/Interpreter modules will depend on (it's not fully clear to me where to draw the line between these two, so it may end up needing to be moved from one to the other in the future, but I think that future will be pretty far away). I also thought about the case when something else, which is logically lower than Interpreter/Command needs the string conversion functions -- in this case we could put the conversion function in a different module, but still keep the function in OptionArgParser as a wrapper on top of that. This way you would still have all the argument parsing functions in one place, while the things in the other modules (which by definition will not be parsing option arguments, or else they would be in these modules), can call the lower level function directly. Ultimately, solving the layering of string conversion functions is not my goal here. I would be fully satisfied with breaking even. My goal is to move this stuff out of the Args class, so it can be moved down to Utility, as it is one of the main causes that everything depends on Interpreter (and then everything else). https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
jingham added a comment. As long as these functions are used for parsing options args in individual CommandObject implementations, and don't get dragged lower in the stack, I don't think that's a problem. By the time you're getting to individual commands you are on top of pretty much everything else in lldb, and can expect to rely on whatever. And the value of having all the converters gathered in one place so people writing new commands can easily do the right thing when ingesting arguments more than justifies this gathering of disparate elements IMO. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
zturner added a subscriber: labath. zturner added a comment. I think having all parsing functions in a single place will just move the layering problem elsewhere, since a bunch of conversion functions for objects from various libraries will be mushed together into one place. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
I think having all parsing functions in a single place will just move the
layering problem elsewhere, since a bunch of conversion functions for
objects from various libraries will be mushed together into one place.
On Wed, Mar 14, 2018 at 11:34 AM Jim Ingham via Phabricator <
revi...@reviews.llvm.org> wrote:
> jingham added a comment.
>
> Except for the to -> To to keep consistent with all the other lldb
> function naming this looks fine.
>
> Now that they are all together it's easy to see we haven't been consistent
> in these functions. We really should make ToFormat return the format &
> take an error reference, and have ToBoolean take an error so callers don't
> have to cons it up. But that's orthogonal to this patch.
>
>
>
>
> Comment at: include/lldb/Interpreter/OptionArgParser.h:18-19
> +struct OptionArgParser {
> + static lldb::addr_t toAddress(const ExecutionContext *exe_ctx,
> +llvm::StringRef s, lldb::addr_t
> fail_value,
> +Status *error);
>
> Should be ToAddress.
>
>
> https://reviews.llvm.org/D44306
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
jingham added a comment.
Except for the to -> To to keep consistent with all the other lldb function
naming this looks fine.
Now that they are all together it's easy to see we haven't been consistent in
these functions. We really should make ToFormat return the format & take an
error reference, and have ToBoolean take an error so callers don't have to cons
it up. But that's orthogonal to this patch.
Comment at: include/lldb/Interpreter/OptionArgParser.h:18-19
+struct OptionArgParser {
+ static lldb::addr_t toAddress(const ExecutionContext *exe_ctx,
+llvm::StringRef s, lldb::addr_t fail_value,
+Status *error);
Should be ToAddress.
https://reviews.llvm.org/D44306
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath updated this revision to Diff 138418.
labath added a comment.
Herald added a subscriber: mgorny.
This is a version that moves the StringTo*** functions to a new
"OptionArgParser" class. I'm not terribly proud of the name, but I couldn't
think of anything better -- we already have a OptionParser class, so I wanted
to make it clear that this is for parsing the *arguments* of the options.
I deliberately left out three functions out of this move (StringToVersion,
StringToGenericRegister and StringToVersion), as these are not used by the
Interpreter/Command libraries, but things like Host and ProcessGDBRemote, and
so I think they should have a different home.
Let me know what you think.
https://reviews.llvm.org/D44306
Files:
include/lldb/Interpreter/Args.h
include/lldb/Interpreter/OptionArgParser.h
include/lldb/Interpreter/Options.h
source/API/SBDebugger.cpp
source/Commands/CommandObjectBreakpoint.cpp
source/Commands/CommandObjectBreakpointCommand.cpp
source/Commands/CommandObjectCommands.cpp
source/Commands/CommandObjectDisassemble.cpp
source/Commands/CommandObjectExpression.cpp
source/Commands/CommandObjectLog.cpp
source/Commands/CommandObjectMemory.cpp
source/Commands/CommandObjectProcess.cpp
source/Commands/CommandObjectSource.cpp
source/Commands/CommandObjectTarget.cpp
source/Commands/CommandObjectThread.cpp
source/Commands/CommandObjectType.cpp
source/Commands/CommandObjectWatchpointCommand.cpp
source/Interpreter/Args.cpp
source/Interpreter/CMakeLists.txt
source/Interpreter/OptionArgParser.cpp
source/Interpreter/OptionGroupValueObjectDisplay.cpp
source/Interpreter/OptionGroupWatchpoint.cpp
source/Interpreter/OptionValueBoolean.cpp
source/Interpreter/OptionValueChar.cpp
source/Interpreter/OptionValueFormat.cpp
source/Interpreter/Property.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
source/Target/Process.cpp
unittests/Interpreter/CMakeLists.txt
unittests/Interpreter/TestArgs.cpp
unittests/Interpreter/TestOptionArgParser.cpp
Index: unittests/Interpreter/TestOptionArgParser.cpp
===
--- /dev/null
+++ unittests/Interpreter/TestOptionArgParser.cpp
@@ -0,0 +1,121 @@
+//===-- ArgsTest.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+#include "lldb/Interpreter/OptionArgParser.h"
+
+using namespace lldb_private;
+
+TEST(OptionArgParserTest, toBoolean) {
+ bool success = false;
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("true"), false, nullptr));
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("on"), false, nullptr));
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("yes"), false, nullptr));
+ EXPECT_TRUE(OptionArgParser::toBoolean(llvm::StringRef("1"), false, nullptr));
+
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("true"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("on"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("yes"), false, &success));
+ EXPECT_TRUE(success);
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("1"), false, &success));
+ EXPECT_TRUE(success);
+
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("false"), true, nullptr));
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("off"), true, nullptr));
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("no"), true, nullptr));
+ EXPECT_FALSE(OptionArgParser::toBoolean(llvm::StringRef("0"), true, nullptr));
+
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("false"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("off"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("no"), true, &success));
+ EXPECT_TRUE(success);
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("0"), true, &success));
+ EXPECT_TRUE(success);
+
+ EXPECT_FALSE(
+ OptionArgParser::toBoolean(llvm::StringRef("10"), false, &success));
+ EXPECT_FALSE(success);
+ EXPECT_TRUE(
+ OptionArgParser::toBoolean(llvm::StringRef("10"), true, &success));
+ EXPECT_FALSE(success);
+ EXPECT_TRUE(OptionArgParser::toBoolean(l
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath planned changes to this revision. labath added a comment. Yes, that thought occurred to me almost immediately after I submitted this. I'll try to implement something like that instead. Since this will be just a bunch of static functions with no state, I think we don't even have to put them into an existing class, but instead create a new file/class for holding just this. I just need to figure out a good name for that. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
jingham added a comment. I liked the fact that all these Argument input type string converters were all in one place, though arguably Args was not the right place. Among other things it made it easy to see that if you were adding a new command that took in an Address, AddressToString was the canonical way to do it. That's less obvious now that this one is a target function, and you would have to go look at another argument to see what it does. I wonder if this problem isn't better solved by moving all the parsing functions somewhere more appropriate (like CommandInterpreter?) and then it isn't surprising what they might do. https://reviews.llvm.org/D44306 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44306: Move Args::StringToAddress to Target::EvaluateAddressExpression
labath created this revision.
labath added reviewers: jingham, zturner.
StringToAddress could end up evaluating an expression, which is a somewhat
unexpected behavior of a seemingly simple method. I rename it to
EvaluateAddressExpression and move it to the Target class, next to the standard
EvaluateExpression function.
This functionality is now invoked via
execution_context->GetTargetPtr()->EvaluateAddressExpression(). To avoid null
checking the target pointer everywhere, I've added the eCommandRequiresTarget
flag to the commands which didn't have it, and which did not seem to be useful
without a target (disassemble && target modules list).
https://reviews.llvm.org/D44306
Files:
include/lldb/Interpreter/Args.h
include/lldb/Target/Target.h
source/Commands/CommandObjectBreakpoint.cpp
source/Commands/CommandObjectDisassemble.cpp
source/Commands/CommandObjectMemory.cpp
source/Commands/CommandObjectSource.cpp
source/Commands/CommandObjectTarget.cpp
source/Commands/CommandObjectThread.cpp
source/Interpreter/Args.cpp
source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2326,6 +2326,110 @@
return execution_results;
}
+lldb::addr_t Target::EvaluateAddressExpression(const ExecutionContext *exe_ctx,
+ llvm::StringRef expr,
+ lldb::addr_t fail_value,
+ Status *error_ptr) {
+ if (expr.empty()) {
+if (error_ptr)
+ error_ptr->SetErrorStringWithFormatv("invalid address expression \"{0}\"",
+ expr);
+return fail_value;
+ }
+
+ lldb::addr_t addr = LLDB_INVALID_ADDRESS;
+ if (!expr.getAsInteger(0, addr)) {
+if (error_ptr)
+ error_ptr->Clear();
+return addr;
+ }
+
+ // Try base 16 with no prefix...
+ if (!expr.getAsInteger(16, addr)) {
+if (error_ptr)
+ error_ptr->Clear();
+return addr;
+ }
+
+ Target *target = nullptr;
+ if (!exe_ctx || !(target = exe_ctx->GetTargetPtr())) {
+if (error_ptr)
+ error_ptr->SetErrorStringWithFormatv("invalid address expression \"{0}\"",
+ expr);
+return fail_value;
+ }
+
+ lldb::ValueObjectSP valobj_sp;
+ EvaluateExpressionOptions options;
+ options.SetCoerceToId(false);
+ options.SetUnwindOnError(true);
+ options.SetKeepInMemory(false);
+ options.SetTryAllThreads(true);
+
+ ExpressionResults expr_result = target->EvaluateExpression(
+ expr, exe_ctx->GetFramePtr(), valobj_sp, options);
+
+ bool success = false;
+ if (expr_result == eExpressionCompleted) {
+if (valobj_sp)
+ valobj_sp = valobj_sp->GetQualifiedRepresentationIfAvailable(
+ valobj_sp->GetDynamicValueType(), true);
+// Get the address to watch.
+if (valobj_sp)
+ addr = valobj_sp->GetValueAsUnsigned(fail_value, &success);
+if (success) {
+ if (error_ptr)
+error_ptr->Clear();
+ return addr;
+}
+if (error_ptr) {
+ error_ptr->SetErrorStringWithFormatv(
+ "address expression \"%{0}\" resulted in a value whose type "
+ "can't be converted to an address: {1}",
+ expr, valobj_sp->GetTypeName().GetCString());
+}
+return fail_value;
+ }
+
+ // Since the compiler can't handle things like "main + 12" we should
+ // try to do this for now. The compiler doesn't like adding offsets
+ // to function pointer types.
+ static RegularExpression g_symbol_plus_offset_regex(
+ "^(.*)([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*$");
+ RegularExpression::Match regex_match(3);
+ if (g_symbol_plus_offset_regex.Execute(expr, ®ex_match)) {
+uint64_t offset = 0;
+bool add = true;
+std::string name;
+std::string str;
+if (regex_match.GetMatchAtIndex(expr, 1, name)) {
+ if (regex_match.GetMatchAtIndex(expr, 2, str)) {
+add = str[0] == '+';
+
+if (regex_match.GetMatchAtIndex(expr, 3, str)) {
+ if (!llvm::StringRef(str).getAsInteger(0, offset)) {
+Status error;
+addr = EvaluateAddressExpression(exe_ctx, name,
+ LLDB_INVALID_ADDRESS, &error);
+if (addr != LLDB_INVALID_ADDRESS) {
+ if (add)
+return addr + offset;
+ else
+return addr - offset;
+}
+ }
+}
+ }
+}
+ }
+
+ if (error_ptr) {
+error_ptr->SetErrorStringWithFormatv(
+"address expression \"{0}\" evaluation failed", expr);
+ }
+ return fail_value;
+}
+
lldb::ExpressionVariableSP
Target::GetPersistentVariable(const ConstString &name) {
lldb::ExpressionVariableSP variable_sp;
Index: source/Plugins/LanguageRuntime/ObjC/Appl
