This is an archive of the discontinued LLVM Phabricator instance.

[Core] Generalize ValueObject::MaybeCalculateCompleteType
ClosedPublic

Authored by xiaobai on Jul 3 2019, 1:47 PM.

Details

Summary

Instead of hardcoding ClangASTContext and ObjCLanguageRuntime, we can
generalize this by creating the method CalculateCompleteType in
LanguageRuntime and moving the current MaybeCalculateCompleteType
implementation into ObjCLanguageruntime::CalculateCompleteType

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jul 3 2019, 1:47 PM
JDevlieghere added inline comments.Jul 3 2019, 3:15 PM
source/Target/ObjCLanguageRuntime.cpp
404 ↗(On Diff #207883)

I would move this down.

414 ↗(On Diff #207883)

and do return {} here, to make it clear that the object is empty.

417 ↗(On Diff #207883)

Same here.

421 ↗(On Diff #207883)

Maybe even better would be to return an llvm::Optional?

The main issue I have with this is the name of the function we are adding to LanguageRuntime. See inlined comments.

include/lldb/Target/ObjCLanguageRuntime.h
253 ↗(On Diff #207883)

Is this named correctly? Maybe this should be named "CompilerType GetRuntimeType(CompilerType base_type) override;"?

What this function does is gets the real definition from the objective C runtime at the moment. This name would better reflect what is going on and would be something we might ask of a runtime.

source/Target/ObjCLanguageRuntime.cpp
403 ↗(On Diff #207883)

So a main question for ObjC here: do we always want to show the runtime type? Should we not check if the class inside of "base_type" is the one true definition and skip grabbing the runtime type here and return {}?

404 ↗(On Diff #207883)

Remove this variable and just return {} everywhere

430–433 ↗(On Diff #207883)
if (is_pointer_type)
  return complete_class.GetPointerType();
else
  return complete_class;
436 ↗(On Diff #207883)
return {};
xiaobai marked 2 inline comments as done.Jul 8 2019, 2:31 PM
xiaobai added inline comments.
include/lldb/Target/ObjCLanguageRuntime.h
253 ↗(On Diff #207883)

I have no problem with renaming it "GetRuntimeType". Will do that.

source/Target/ObjCLanguageRuntime.cpp
403 ↗(On Diff #207883)

How do you know which is the "one true definition"?

jingham added inline comments.Jul 8 2019, 2:50 PM
source/Target/ObjCLanguageRuntime.cpp
403 ↗(On Diff #207883)

The only places you can add ivars to an ObjC class are in the @interface declaration (which is usually in the .h file for the class) and in the @implementation and the "class category" - which has to be in the same source file as the @implementation. So if you find debug information for the .m file that contains the @implementation you have seen all the ivars of the class. Clang marks that fact by putting "DW_AT_APPLE_objc_complete_type" with value "true" in the DW_TAG_structure_type die for the class. If you see a type definition so marked, that it the "one true definition".

xiaobai updated this revision to Diff 208841.Jul 9 2019, 4:50 PM
xiaobai marked an inline comment as done.

Switch to using llvm::Optional<CompilerType> for the return type
Rename method from CalculateCompleteType to GetRuntimeType

xiaobai added inline comments.Jul 9 2019, 4:51 PM
source/Target/ObjCLanguageRuntime.cpp
403 ↗(On Diff #207883)

This makes sense to me, but I'm not sure if there are any abstractions that support this today. I did find a method CompilerType::IsCompleteType but I don't think that does this. I think that the only way to accomplish this right now is to reach into the DWARF directly, which I do not want to have to do here. I think preserving existing behavior is okay for now.

clayborg accepted this revision.Jul 12 2019, 10:05 AM
This revision is now accepted and ready to land.Jul 12 2019, 10:05 AM
compnerd accepted this revision.Jul 12 2019, 10:12 AM
compnerd added a subscriber: compnerd.

Seems that all the comments have been addressed and this is purely code motion. LGTM

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 11:36 AM