This is an archive of the discontinued LLVM Phabricator instance.

[Target] Hoist LanguageRuntime::GetDeclVendor
ClosedPublic

Authored by xiaobai on Jun 20 2019, 1:45 PM.

Details

Summary

It's possible that each LanguageRuntime could have its own DeclVendor,
so let's hoist that out of ObjCLanguageRuntime into LanguageRuntime.

Additionally, this gives the opportunity to remove SBTarget's dependency
on ObjCLanguageRuntime.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jun 20 2019, 1:45 PM

This change makes it clear that SBTarget::FindFirstType should take a language, but that is orthogonal to this change.

source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

As soon as you start iterating over all language runtimes, I don't think you can use ClangASTContext::GetTypeForDecl, can you? Not all language runtimes will be backed by a Clang AST.

xiaobai marked an inline comment as done.Jun 20 2019, 2:25 PM

This change makes it clear that SBTarget::FindFirstType should take a language, but that is orthogonal to this change.

Yep, it definitely should.

source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

In principle, this is wrong. But FindDecl's deals with clang NamedDecl's, so this still makes sense right now. In the future we will need to make DeclVendor more TypeSystem/compiler agnostic.

labath edited reviewers, added: jingham; removed: labath.Jun 20 2019, 11:58 PM

Seems like a reasonable thing to do, but I don't really know what this code does...

compnerd added inline comments.Jun 21 2019, 9:21 AM
include/lldb/Target/LanguageRuntime.h
137 ↗(On Diff #205896)

Can this not be const? Seems like retrieving the vendor should not mutate the runtime.

source/API/SBTarget.cpp
1850 ↗(On Diff #205896)

auto should be fine, the type is spelt out.

1851 ↗(On Diff #205896)

Can this not be const auto *runtime?

1912 ↗(On Diff #205896)

auto should be fine, the type is spelt out in the method.

jingham added inline comments.Jun 21 2019, 10:33 AM
source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

If that's true, then you should signal the restriction by limiting the iteration over language runtimes to C-family ones. Like:

// DeclVendor only works for C Family languages at present.
if (!Language::LanguageIsCFamily(runtime->GetLanguageType())
       continue;

Or maybe we need another "LanguageIsBackedByClangAST" check? That would be more accurate...

jingham added inline comments.Jun 21 2019, 10:35 AM
include/lldb/Target/LanguageRuntime.h
137 ↗(On Diff #205896)

The vendor is computed lazily, so calling this could cause the vendor to get built. So formally this is not a const operation.

Seems like a reasonable thing to do, but I don't really know what this code does...

The runtime DeclVendor gives runtimes a way to produce type information from runtime metadata. For instance, the ObjC runtime tables actually have fairly good type information for all ObjC classes - both instance variables, properties and methods. It is a little lossy, but for instance it knows the return types of methods so with this augmentation users can call ObjC methods without requiring casting... This is just the code to query the runtime's DeclVendors to see if they know anything about a given typename.

ObjC is a bit obnoxious, because you can define instance variables and methods in the implementation of a class as well as its interface. So even though you might have a debug info definition for the type - gotten from including the interface header file - that may not be as good as what you can get from the runtime. But the runtime type info is, as I said, lossy so if you DO have debug info it will be better. That means for ObjC you have to do some kind of merge operation to get the best information for a type. That complexity doesn't affect this patch, but I couldn't resist the opportunity to moan about it a bit since it's given US so much grief over the years!

xiaobai marked an inline comment as done.Jun 21 2019, 12:43 PM
xiaobai added inline comments.
source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

I could try to limit things based on language, but that makes it seem unlikely to get cleaned up in the future if DeclVendor is generalized beyond just using clang.

Another idea could be to introduce a method DeclVendor::GetTypes, returning a std::vector<CompilerType> which will do the job that is being done here. That will remove clang from the equation entirely. Of course, because of the way DeclVendor works now, that method will end up using clang internally, but then it will be easier to clean up when support for non-clang-based languages are added.

xiaobai marked an inline comment as done.Jun 21 2019, 1:02 PM
xiaobai added inline comments.
source/API/SBTarget.cpp
1851 ↗(On Diff #205896)

Not unless GetDeclVendor is marked as const as well, and if what Jim says is correct, then we cannot do this.

xiaobai updated this revision to Diff 206054.Jun 21 2019, 1:02 PM

Add a few autos

jingham added inline comments.Jun 21 2019, 1:34 PM
source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

The second idea is clearly what's going to have to get done to generalize this to other TypeSystems. I was thinking of limiting the language more as a signpost that this is not a truly general solution, just to decouple this patch from the work to generalize the DeclVendor interface. On second thought, a FIXME comment to that effect is probably a more explicit way to do this.

Of course, if you want to fix how the DeclVendors work to make them handle other TypeSystems as part of that patch, that would be double plus good.

xiaobai marked an inline comment as done.Jun 21 2019, 2:26 PM
xiaobai added inline comments.
source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

Okay, so then what I think I'm going to do is this:

  • Add a FIXME to this patch denoting that this is really will only work with clang at the moment. Unfortunate, but easily findable with a quick git grep FIXME.
  • Submit a patch that adds a method DeclVendor::GetTypes which will take on the burden of using clang. The FIXME will be moved there until I figure out how to generalize DeclVendor further.

Sound good to you?

jingham added inline comments.Jun 21 2019, 3:31 PM
source/API/SBTarget.cpp
1854–1859 ↗(On Diff #205896)

That sounds like a fine plan.

The runtime DeclVendor gives runtimes a way to produce type information from runtime metadata. For instance, the ObjC runtime tables actually have fairly good type information for all ObjC classes - both instance variables, properties and methods. It is a little lossy, but for instance it knows the return types of methods so with this augmentation users can call ObjC methods without requiring casting... This is just the code to query the runtime's DeclVendors to see if they know anything about a given typename.

ObjC is a bit obnoxious, because you can define instance variables and methods in the implementation of a class as well as its interface. So even though you might have a debug info definition for the type - gotten from including the interface header file - that may not be as good as what you can get from the runtime. But the runtime type info is, as I said, lossy so if you DO have debug info it will be better. That means for ObjC you have to do some kind of merge operation to get the best information for a type. That complexity doesn't affect this patch, but I couldn't resist the opportunity to moan about it a bit since it's given US so much grief over the years!

Thanks for the explanation/background. :)

include/lldb/Target/LanguageRuntime.h
137 ↗(On Diff #205896)

That depends on how you look at it. If something behaves like "const" from the outside (i.e. doesn't change the abstract state of the object), then I'd be fine with marking it as const even if it needs to modify some internal variables. Lazy evaluation/caching is one of the few valid uses for the mutable keyword.

However, that wouldn't be very consistent with how the this is done in the rest of lldb.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 24 2019, 1:34 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 1:34 PM