Page MenuHomePhabricator

Generate DW_TAG_class_type when calling DIBuilder::createClassType().
AbandonedPublic

Authored by koutheir on Jul 7 2021, 8:06 PM.

Diff Detail

Event Timeline

koutheir created this revision.Jul 7 2021, 8:06 PM
koutheir requested review of this revision.Jul 7 2021, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 8:06 PM
lattner resigned from this revision.Jul 7 2021, 8:45 PM
aprantl requested changes to this revision.Jul 29 2021, 5:23 PM

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.

This revision now requires changes to proceed.Jul 29 2021, 5:23 PM

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.

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.

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?

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 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.

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?

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.

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

koutheir abandoned this revision.Aug 23 2021, 10:57 AM