This is an archive of the discontinued LLVM Phabricator instance.

Move Args::StringToAddress to Target::EvaluateAddressExpression
ClosedPublic

Authored by labath on Mar 9 2018, 7:42 AM.

Details

Summary

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).

Event Timeline

labath created this revision.Mar 9 2018, 7:42 AM

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.

labath planned changes to this revision.Mar 13 2018, 3:36 AM

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.

labath updated this revision to Diff 138418.Mar 14 2018, 11:13 AM

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.

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.

include/lldb/Interpreter/OptionArgParser.h
18–19 ↗(On Diff #138418)

Should be ToAddress.

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.

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.

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).

labath updated this revision to Diff 138540.Mar 15 2018, 6:14 AM

s/toAddress/ToAddress/

So, any objections to this patch?

labath added a comment.Apr 9 2018, 9:36 AM

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.

jingham accepted this revision.Apr 9 2018, 11:36 AM

Yes, with your updated naming, I am fine with this.

This revision is now accepted and ready to land.Apr 9 2018, 11:36 AM

Thank you.

PS: I will need someone to update the XCode project after this.

This revision was automatically updated to reflect the committed changes.