Page MenuHomePhabricator

[DebugInfo] Add assert for variable size when creating fragments.
Needs ReviewPublic

Authored by akhuang on Sep 14 2020, 3:30 PM.

Details

Reviewers
dblaikie
aprantl
Summary

Recently I added the size to the debug info for class declarations, so
that the correct fragments are created when a class is not defined.

This patch asserts that the variable size exists when creating
fragments, since I think there shouldn't be any cases where we want to
create fragments for a variable with no size.t

Bug: https://bugs.llvm.org/show_bug.cgi?id=47338

Diff Detail

Event Timeline

akhuang created this revision.Sep 14 2020, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2020, 3:30 PM
akhuang requested review of this revision.Sep 14 2020, 3:30 PM

I'll leave this one to @aprantl - though I am a bit concerned about whether this should be a verifier error if SROA is going to assume it's an invariant. Some discussion on the bug about whether this SROA code is only reachable for some subset of types that do have sizes, while some other types won't have sizes. (variable length arrays being a fairly fundamental example, since they can't have a known length - but I would worry that SROA might be able to mess with known portions of them, maybe?) I mean, I'm usually all for "throw an assert in, and if it ever fails then we'll have figured out what case we didn't consider earlier", though just a bit hesitant on this at the moment. Probably no big deal, though.

Ah, yeah. I'm not really sure if there could be cases where there is no size. If there are, I guess we shouldn't make any changes here.

I'm expecting this to cause all sorts of non-clang frontends to trigger this assertion, since we don't require types to have sizes. In fact, I have an open bug for the Swift frontend to try removing the static sizes from types, since LLDB prefers the dynamic runtime size anyway.
If we want to make this a requirement, we should probably implement it as a verifier check instead of an assertion. But I'm not sure if we want to require this for all frontends.

I'm expecting this to cause all sorts of non-clang frontends to trigger this assertion, since we don't require types to have sizes. In fact, I have an open bug for the Swift frontend to try removing the static sizes from types, since LLDB prefers the dynamic runtime size anyway.
If we want to make this a requirement, we should probably implement it as a verifier check instead of an assertion. But I'm not sure if we want to require this for all frontends.

An alternative question then: Should this constraint exist? Or should it be valid to create a fragment that is the same size as the object? (we currently assert in the verifier if the size is available and the fragment covers the whole size: AssertDI(FragSize != *VarSize, "fragment covers entire variable", Desc, &V);)