This is an archive of the discontinued LLVM Phabricator instance.

Introduce a TypeSystem interface to support adding non-clang languages.
ClosedPublic

Authored by ribrdb on Mar 30 2015, 3:17 PM.

Diff Detail

Event Timeline

ribrdb updated this revision to Diff 22911.Mar 30 2015, 3:17 PM
ribrdb retitled this revision from to Introduce a TypeSystem interface to support adding non-clang languages..
ribrdb updated this object.
ribrdb edited the test plan for this revision. (Show Details)
ribrdb added a reviewer: clayborg.
ribrdb set the repository for this revision to rL LLVM.
ribrdb added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Mar 30 2015, 4:05 PM
clayborg edited edge metadata.

Overall a good start. I am confused as to why there were a lot of:

clang_type.method(...)

changed to

type_system->method(clang_type, ...)

Seems like we should keep the old API as the ClangASTType contains a "TypeSystem*" and it would make the diffs a lot less noisy in SymbolFileDWARF. These are all of the "Why is this no longer a method on ..." inline code notations.

It also seems like anything that is actually creating a ClangASTTypeSystem specific type should have the creation functions only in ClangASTTypeSystem and not make all of them pure virtual in the TypeSystem definition. Then each language will have its own TypeSystem subclass. See inlined notes that say "move to ClangASTTypeSystem.h?". If we do this, then each module needs to be able to get one of the TypeSystem specific subclasses from the module:

ClangASTTypeSystem *
Module::GetTypeSystemClang ();

ClangASTTypeGo *
Module::GetTypeSystemGo ();

ClangASTTypeSwift *
Module::GetTypeSystemSwift ();

Then the DWARF parser would grab the right TypeSystem subclass from the module in SymbolFileDWARF and use that to create types. Each TypeSystem subclass can have its own way to create native types. All of which must return ClangASTType (this really should be renamed CompilerType) types.

So the changes I would like to see is:
1 - Make ClangASTContext inherit from TypeSystem
2 - Try and isolate all of the language specific type creation routines into ClangASTContext (since it now inherits from TypeSystem) and let it create native clang types. This way not all languages have to make hundreds of pure virtual functions that they will never support. If any are generic enough, we can include them, but it would be cleaner to allow each langue to have its own language specific create/addField/addIVar, etc routines
3 - Add get_as<T>() to the TypeSystem and allow people to dyn_cast their TypeSystem into more specific subclasses where needed and where the pure virtual stuff falls down.

include/lldb/Symbol/ClangASTContext.h
39

Shouldn't this inherit from TypeSystem?

58–59

If ClangASTContext inherits from TypeSystem, this isn't needed.

509

Shouldn't ClangASTContext inherit from TypeSystem? If so, this isn't needed.

include/lldb/Symbol/ClangASTType.h
20

Put this in lldb-forward.h and remove any other forward references to TypeSystem.

38

This should be "TypeSystem* type_system, void *type".

233–235

We shouldn't need this we should just be able to use the GetTypeSystem() above. We might need to implement llvm style dyn_cast<> operators on TypeSystem.

264

This should be "TypeSystem* type_system, void *type".

include/lldb/Symbol/TypeSystem.h
190–212

move to ClangASTTypeSystem.h?

We might not need these in the base class since they are used to create ClangASTTypeSystem based types. Each language should have their own creation types that not every language should have to implement.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1715

Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type?

1817

Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type?

1817

We should make a common name for any TypeSystem variable like "type_system" and use these common names everywhere. "types" seems like the wrong variable name here for clarity sake.

2006

Why is this no longer a method on "class_clang_type"?

2124

Why is this no longer a method on "class_clang_type"?

2174

Why is this no longer a method on "class_clang_type"?

2322

Why is this no longer a method on "class_clang_type"?

2326

Why is this no longer a method on base_class_clang_type?

2492

Why is this no longer a method on clang_type?

2528

Why no longer a method on the object like before?

This revision now requires changes to proceed.Mar 30 2015, 4:05 PM
ribrdb updated this revision to Diff 23189.Apr 2 2015, 5:19 PM
ribrdb edited edge metadata.

I think I've addressed most of your comments, except removing more methods from TypeSystem.h. I've added TODOs for the methods I think should be removed, and the ones I'm unsure about.
I've also changed the style for the ClangASTContext specific methods.
Instead of ast->Foo(type.GetOpaqueQualType()), it's now ClangASTContext::Foo(type). I think this is cleaner and safer.
If you like this style and agree on which methods to keep I'll go ahead and move the methods.

clayborg requested changes to this revision.Apr 3 2015, 10:58 AM
clayborg edited edge metadata.

Looks great. Very few changes needed. See inline comments.

include/lldb/Symbol/ClangASTType.h
443–447

This shouldn't be here, it should probably be moved as a static function to where the above functions moved.

include/lldb/lldb-forward.h
225

Fix spacing to match. If this is a tab, this should be spaces as we don't use tabs in LLDB sources.

This revision now requires changes to proceed.Apr 3 2015, 10:58 AM
ribrdb updated this revision to Diff 23289.Apr 6 2015, 1:29 PM
ribrdb edited edge metadata.
ribrdb updated this revision to Diff 23817.Apr 15 2015, 5:59 PM
ribrdb edited edge metadata.

Please take another look. I've moved around some methods between ClangASTType, ClangASTContext, and TypeSystem based on trying to implement a TypeSystem for go.

clayborg accepted this revision.May 12 2015, 1:25 PM
clayborg edited edge metadata.

Update to top of tree and you should be good to go as long as the test suite passes everything.

This revision is now accepted and ready to land.May 12 2015, 1:25 PM
ribrdb updated this revision to Diff 25728.May 13 2015, 1:14 PM
ribrdb edited edge metadata.

Update to top of tree

I don't have commit access, can you submit this?

ribrdb updated this revision to Diff 27328.Jun 8 2015, 1:21 PM

Sync to head

ribrdb updated this revision to Diff 27338.Jun 8 2015, 3:12 PM

Another try at updating.

ovyalov added a subscriber: ovyalov.Jun 8 2015, 4:51 PM

Looks like your CL caused some test regression - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/3040
Please fix or revert the CL.

ribrdb updated this revision to Diff 27412.Jun 9 2015, 4:20 PM

Oops, I guess building the "run testsuite" scheme in xcode doesn't actually run the tests.
This version fixes the tests on my linux machine.

ribrdb updated this revision to Diff 29820.Jul 15 2015, 1:14 PM

Sync to head

clayborg closed this revision.Aug 11 2015, 2:51 PM

Committed an updated version of this with the following two revisions:

Committed revision 244679.
Committed revision 244681.

source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp