This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Support DW_AT_defaulted
AbandonedPublic

Authored by probinson on Sep 17 2021, 8:29 AM.

Details

Reviewers
dblaikie
aprantl
Summary

DWARF v5 added the DW_AT_defaulted attribute, which describes whether
a C++ subprogram has = default on its in-class or an out-of-class
declaration. Pass this info to LLVM for the special member functions,
and have LLVM emit the attribute.

The metadata encoding doesn't permit distinguishing between "not defaulted"
and older bitcode that doesn't describe the defaulted state, so we don't
emit "not defaulted" in the DWARF.

Diff Detail

Event Timeline

probinson created this revision.Sep 17 2021, 8:29 AM
probinson requested review of this revision.Sep 17 2021, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2021, 8:29 AM

I think this is the last of the new DWARF v5 tags/attributes.

Any particular use case in mind for this debug info? (generally I'd rather we not emit extra DWARF we don't have some use case for in mind)

(& if/when this is committed, should be in 3 parts - dwarfdump support, IR/codegen support (can be split into two if you like, but doesn't have to be), and Clang support)

clang/lib/CodeGen/CGDebugInfo.cpp
1792–1811

Looks like this'll do the right thing, but probably want to make sure there's a test case for "deleted as default".

something like:

struct t1 {
t1() = delete;
};
struct t2 {
  t1 v1;
  t2() = default; // this is deleted
};

at least I /think/ that'd tickle the case.

llvm/lib/IR/DebugInfoMetadata.cpp
854–855

How does Virtuality work? Can we avoid the special case for DefaultedOrDeleted in the same way that it is avoided for Virtuality?

llvm/test/DebugInfo/X86/DW_AT_defaulted.s
1

I'd tend to put pure dwarfdump tests in llvm/test/tools/llvm-dwarfdump (but I get that it's all a bit mixed/ambiguous - I know you mentioned a while back about the idea of keeping test/DebugInfo for the actual libDebugInfo tests, but it'd sort of already sailed/had lots of LLVM codegen tests in there - and equally in theory llvm/test/tools/llvm-dwarfdump could be just for tests of llvm-dwarfdump.cpp and related files, not the library implementation... ) ah well. Just a thought.

Any particular use case in mind for this debug info? (generally I'd rather we not emit extra DWARF we don't have some use case for in mind)

I don't think Sony has a use for it. The description in section 5.7.8 suggests in/out of class defaulting can affect calling conventions, and I see gcc emitting it, so I figured there was likely some use for it. I suppose I could see if any gdb tests would start working because of this, but apart from that, no I don't have a use-case in mind.

(& if/when this is committed, should be in 3 parts - dwarfdump support, IR/codegen support (can be split into two if you like, but doesn't have to be), and Clang support)

I did originally have this as separate LLVM and Clang commits, happy to split it back up. The dwarfdump support already existed, there just wasn't a test, so I tucked that into the IR/codegen changes; I guess adding the dump test could be its own thing. CodeGen is what makes use of the IR flags, splitting them means one patch defines new IR flags that... aren't used for anything? I figure they're better off in one patch.

clang/lib/CodeGen/CGDebugInfo.cpp
1792–1811

I didn't realize these things stacked that way. But yes, t2's default ctor is flagged as deleted.

llvm/lib/IR/DebugInfoMetadata.cpp
854–855

Virtuality occupies the low two bits of the flags, those flags carefully being defined to match the DW_VIRTUALITY_* constants; 00 = none, 01 = virtual, 10 = pure virtual. There's no special case here because 11 doesn't mean something; so, this splitFlags method, which essentially iterates over the flag bits, can iterate over the Virtuality flags without having anything go wrong.

We could do that for Defaulted as well, if we're willing to use a separate 2-bit field for it, because then like Virtuality there would be no special case for the 11 value. That would also make it feasible to avoid a switch statement in DwarfUnit.cpp, because we can just extract the field and (if it's nonzero) stuff it into the attribute.

Flag bits are not an endless resource, and so I followed the suggestion in the comment in DebugInfoFlags.def to have a single 2-bit field handle both the Defaulted and (mutually exclusive) Deleted cases. If you're willing to burn the extra bit and avoid a couple of special cases, I'm okay with doing it that way.

llvm/test/DebugInfo/X86/DW_AT_defaulted.s
1

Yeah, this is a long-standing thing and nobody is willing to go clean out the stables. This particular test placement is consistent with "code changed in DebugInfo, test goes in DebugInfo."
Recognizing that the *other* test is for a code change in CodeGen, and I'd be open to moving that test from DebugInfo to CodeGen, even though I'm just fiddling a test that was already there.

Any particular use case in mind for this debug info? (generally I'd rather we not emit extra DWARF we don't have some use case for in mind)

I don't think Sony has a use for it. The description in section 5.7.8 suggests in/out of class defaulting can affect calling conventions,

I think that came out of a misunderstanding that ultimately resulted in a more robust/complete solution involving the DW_AT_calling_convention attribute.

and I see gcc emitting it, so I figured there was likely some use for it. I suppose I could see if any gdb tests would start working because of this, but apart from that, no I don't have a use-case in mind.

Yeah, if there's any evidence this is being used, that'd be helpful I think - otherwise I'd be inclined to leave it until there's a motivating use case (especially given, to the best of my understanding of the conversations at the time, where this came out of & that DW_AT_calling_convention ended up being necessary anyway)

(& if/when this is committed, should be in 3 parts - dwarfdump support, IR/codegen support (can be split into two if you like, but doesn't have to be), and Clang support)

I did originally have this as separate LLVM and Clang commits, happy to split it back up.

I don't mind/think it's good to review it together, but separable parts should be committed separately when it comes to that.

The dwarfdump support already existed, there just wasn't a test, so I tucked that into the IR/codegen changes; I guess adding the dump test could be its own thing.

Yeah, that'd be great if you could commit the test coverage separately/ahead of the rest of any of this.

CodeGen is what makes use of the IR flags, splitting them means one patch defines new IR flags that... aren't used for anything? I figure they're better off in one patch.

Yeah, that's a marginal area where splitting may or may not be done - we're not especially serious about that. Though it can help separate a lot of mechanical work to plumb the stuff through IR (& that work will have its own test coverage - IR parsing/writing/bitcode/etc roundtripping) - even if it has no observable effect on the whole invocation of clang/llc/etc. Separating all that mechanical stuff from a few lines of semantic/more nuanced code in lib/CodeGen/AsmPrinter and testing those pieces separately.

probinson abandoned this revision.Sep 20 2021, 12:12 PM

I've poked around in the copy of gdb that we're using for testing; I see it noting DW_AT_defaulted, and factoring it into pass by reference/value decisions; but, if DW_AT_calling_convention is present, the latter takes precedence.
It looks like CGDebugInfo::CreateLimitedType will always set the CC flags for a CXXRecordDecl, and I think we always go through that path for complete type descriptions, so I accept that DW_AT_defaulted isn't really necessary.

On those grounds, I'll commit the dumper test (under the test/tools tree, not DebugInfo) but abandon the rest of it.

I've poked around in the copy of gdb that we're using for testing; I see it noting DW_AT_defaulted, and factoring it into pass by reference/value decisions; but, if DW_AT_calling_convention is present, the latter takes precedence.
It looks like CGDebugInfo::CreateLimitedType will always set the CC flags for a CXXRecordDecl, and I think we always go through that path for complete type descriptions, so I accept that DW_AT_defaulted isn't really necessary.

On those grounds, I'll commit the dumper test (under the test/tools tree, not DebugInfo) but abandon the rest of it.

Sounds good, thanks for checking around & for the extra test coverage!