Page MenuHomePhabricator

[lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)
Needs RevisionPublic

Authored by kastiglione on Mar 15 2021, 12:51 PM.

Details

Summary

frame variable contextually supports accessing ivars via a language's implicit
instance variable, ex this in C++ or self in ObjC.

It has been assumed that the instance variable is a pointer, resulting in this-> or
self-> prefixes. However some languages have a reference based instance variable
instead of a pointer. An example of this is Swift.

This changes DeclContextIsClassMethod and a few of its callers to return (via an out
pointer) whether the instance variable is a pointer or reference. This information is
used in GetValueForVariableExpressionPath to construct a language appropriate
prefix for the ivar.

Some cleanup included:

  1. The language parameter wasn't used and has been removed
  2. The is_instance_method parameter is redundant and has been removed -- a non-empty instance_var_name indicates the context is an instance method
  3. IsClassMethod's parameters have been declared with default values of nullptr
  4. Renamed some variables/parameters

Diff Detail

Event Timeline

kastiglione requested review of this revision.Mar 15 2021, 12:51 PM
kastiglione created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 12:51 PM
kastiglione edited the summary of this revision. (Show Details)Mar 15 2021, 12:57 PM
kastiglione retitled this revision from [lldb] Refactor to support non-pointer instance variables (NFC) to [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC).Mar 15 2021, 1:07 PM
shafik added a subscriber: shafik.Mar 15 2021, 2:30 PM
shafik added inline comments.Mar 15 2021, 2:33 PM
lldb/include/lldb/Symbol/CompilerDeclContext.h
77

If we are going to refactor this, this change does not feel very C++y passing around pointers. I know we want a way to call this w/o any arguments but perhaps we can write an overload for that case?

Does instance_var_name_ptr need to be a string? Maybe we can encode it using an enum, we don't have a lot of cases this, self, maybe even not a pointer as well and get ride of instance_is_pointer_ptr.

kastiglione added inline comments.Mar 15 2021, 3:03 PM
lldb/include/lldb/Symbol/CompilerDeclContext.h
77

Something like?

enum InstanceVariable {
   ThisPointer,
   SelfPointer,
   Self,
};
teemperor added inline comments.Mar 16 2021, 4:17 AM
lldb/include/lldb/Symbol/CompilerDeclContext.h
77

We could also make this function that is something like llvm::Optional<SelfRef> GetCurrentObjectRef and SelfRef is just ConstString + enum if it's a pointer/ref/whatever.

FWIW, encoding the string inside an enum doesn't seem to fit with the idea that the TypeSystem interface just needs to be implemented (but not extended) when adding a new language plugin (if the language uses a different name like $this or Self then the enum needs to be expanded). Also not sure what use this has to the caller (I don't see how the callers do anything else with this enum then translating it to the actual string and checking if it's a pointer, both are more complicated with an enum).

kastiglione added inline comments.Mar 16 2021, 11:22 AM
lldb/include/lldb/Symbol/CompilerDeclContext.h
77

I like this approach. Before I make the change, some questions/thoughts.

I'm thinking the second field will be a bool, ex: is_pointer. The reason for bool and not enum is that I don't know if it's worth the complexity of trying to distinguish between reference and value. In Swift, the self variable could be reference (class) or value (struct, enum…).

Instead of SelfRef I'm thinking {This,Self,Instance}Variable, since it's info about the variable (name, pointer-ness).

Do we need to return a ConstString, or can it be const char * and let the caller do what it wants. It seems it will always be a string literal, and const char * is a lower common denominator. I guess I'm ultimately unclear on when, if ever, to not use ConstString?

jingham added inline comments.Mar 16 2021, 11:42 AM
lldb/include/lldb/Symbol/CompilerDeclContext.h
77

ConstString's main purpose is to hold strings we're likely to compare against a lot. For instance, if you take a symbol name and you are going to look it up everywhere, it's appropriate for that to be a ConstString since we're going to turn it into that anyway to do the searches.

Since a caller is likely to turn around and look up "this" having gotten that name back, a ConstString seems an okay choice. Another way to do this would be to make function statics with ConstStrings for "this" and "self". When you make a ConstString from a c-string we have to hash it look for it in the string pool. Copying a ConstString is just copying a pointer. So if you have just a couple of options, making static ConstStrings makes returning the result cheap. And since ConstString's are all null-terminated C-strings, ConstString -> cstring is cheap.

aprantl added inline comments.Mar 24 2021, 3:58 PM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
9505

Another nicer way to implement an API with multiple return values would be to return a struct ClassMethodDescriptor or something like that.

lldb/source/Target/StackFrame.cpp
566

Ideally we would call into the LanguageRuntime here and ask it what the syntax for member access is in this language.

teemperor requested changes to this revision.Fri, Apr 30, 3:42 AM

(Just pushing this back into Dave's review queue)

lldb/include/lldb/Symbol/CompilerDeclContext.h
77

FWIW, the available string types you could use would be:

  • llvm::StringRef -> no ownership and cheap
  • std::string -> ownership
  • ConstString -> Can be very cheap or expensive depending on how you use it.
  • const char * -> Nearly always a bad idea (exceptions are stuff that do C interop).
This revision now requires changes to proceed.Fri, Apr 30, 3:42 AM