Generate DW_TAG_class_type when calling DIBuilder::createClassType().

[aprantl]

What's the motivation behind this change? Most language frontends are not using this API but instead use createReplaceableCompositeType() to deal with recursive data structures. However, we still need to be careful, since this has the potential to break many downstream language frontends that rely on the current behavior. Also, the patch would need a testcase.

[koutheir]

What's the motivation behind this change?

Correctness.

Most language frontends are not using this API but instead use createReplaceableCompositeType() to deal with recursive data structures.

At a certain point, I considered using this API. If you believe the API should never be used, then maybe it should be deprecated then removed.

However, we still need to be careful, since this has the potential to break many downstream language frontends that rely on the current behavior.

The current behavior produces wrong metadata, as far as DWARF is concerned. Relying on the old behavior will break things, but it also means producing correct metadata in the future.

Also, the patch would need a testcase.

Once the reviewers agree this change makes sense, I'll create a small test case to avoid future regressions, and update this patch.

[aprantl]

No language frontend in llvm-project is using this API, and the Swift compiler isn't either, so there is probably not much harm in making this change. It may still cause some fallout from other downstream frontends that I'm not aware of. I'm happy to accept the patch if it comes with a testcase.

[dexonsmith]

In D105604#2917564, @aprantl wrote:

No language frontend in llvm-project is using this API, and the Swift compiler isn't either, so there is probably not much harm in making this change. It may still cause some fallout from other downstream frontends that I'm not aware of. I'm happy to accept the patch if it comes with a testcase.

Can/should it be removed (or deprecated in some way) instead of changed?

[aprantl]

Can/should it be removed (or deprecated in some way) instead of changed?

If @koutheir has a use-case for it and needs it to be exposed in the C API, I don't see a problem with keeping it.

@koutheir are you sure this API is useful to you or do you have to deal with circular definitions that will necessitate a switch to createReplaceableCompositeType later on anyway?

[koutheir]

@koutheir are you sure this API is useful to you or do you have to deal with circular definitions that will necessitate a switch to createReplaceableCompositeType later on anyway?

I'm not currently using this API, as I switched to using createReplaceableCompositeType() instead. When I previously considered using it, I found this bug, and that's what I'm attempting to fix with this change.

[koutheir]

In D105604#2917587, @dexonsmith wrote:

Can/should it be removed (or deprecated in some way) instead of changed?

@aprantl, @dexonsmith, we still need to figure this out before I create tests for this change. Are we deprecating this API?

[aprantl]

I would be in favor of removing the API based on the assumption that every sufficiently complex language frontend will want to createReplaceableCompositeType() instead to deal with circular dependencies in the type graph. If you have a use-case for this API I'm fine keeping it, but I would be curious to learn how you deal with circular dependencies then.

[dexonsmith]

SGTM

[koutheir]

Given that @aprantl and @dexonsmith both agree that this API should be deprecated then removed, I'm abandoning this patch.

llvm/lib/IR/DIBuilder.cpp

DICompositeType *DIBuilder::createClassType(
     DIScope *Context, StringRef Name, DIFile *File, unsigned LineNumber,
     uint64_t SizeInBits, uint32_t AlignInBits, uint64_t OffsetInBits,
     DINode::DIFlags Flags, DIType *DerivedFrom, DINodeArray Elements,
     DIType *VTableHolder, MDNode *TemplateParams, StringRef UniqueIdentifier) {
   assert((!Context || isa<DIScope>(Context)) &&
          "createClassType should be called with a valid Context");
 
   auto *R = DICompositeType::get(
-      VMContext, dwarf::DW_TAG_structure_type, Name, File, LineNumber,
+      VMContext, dwarf::DW_TAG_class_type, Name, File, LineNumber,
       getNonCompileUnitScope(Context), DerivedFrom, SizeInBits, AlignInBits,
       OffsetInBits, Flags, Elements, 0, VTableHolder,
       cast_or_null<MDTuple>(TemplateParams), UniqueIdentifier);
   trackIfUnresolved(R);
   return R;
 }
 
 DICompositeType *DIBuilder::createStructType(