[PATCH] D109970: [DebugInfo] Support DW_AT_defaulted

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

In D109970#3006806 <https://reviews.llvm.org/D109970#3006806>, @dblaikie wrote:

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



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1792-1811
+  // We're checking for deleted/defaulted C++ special member functions
+  // [Ctors, Dtors, Copy/Move].
+  auto checkDeletedOrDefaulted = [&](const auto *Method) {
+    // '= delete' must be on the first declaration, so checking the
+    // canonical decl is sufficient.
+    if (Method->getCanonicalDecl()->isDeleted()) {
       SPFlags |= llvm::DISubprogram::SPFlagDeleted;
----------------
dblaikie wrote:
> 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.
I didn't realize these things stacked that way.  But yes, t2's default ctor is flagged as deleted.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:854-855
+  // Multi-bit fields can require special handling. In our case, there
+  // are two: DeletedOrDefaulted, and Virtuality.  We special-case one
+  // value for DeletedOrDefaulted, but everything else happens to have
   // single-bit values, so the right behavior just falls out.
----------------
dblaikie wrote:
> How does Virtuality work? Can we avoid the special case for DefaultedOrDeleted in the same way that it is avoided for Virtuality?
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.


================
Comment at: llvm/test/DebugInfo/X86/DW_AT_defaulted.s:1
+## Demonstrate dumping DW_AT_defaulted.
+## Any ELF-target triple will work.
----------------
dblaikie wrote:
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109970/new/

https://reviews.llvm.org/D109970