[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

We really do want to pack the four mutually exclusive cases into two bits.  I have tried to give more explicit comments inline to explain how you would do this.  It really should work fine, recognizing that the "not defaulted" case is not explicitly represented in the textual IR because it uses a zero value in the defaulted/deleted subfield of SPFlags.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1619
+      else {
+        SPFlags |= llvm::DISubprogram::SPFlagNotDefaulted;
+      }
----------------
SouraVX wrote:
> Previously SPFlagNotDefaulted is setted to SPFlagZero as it's normal value is, to save a bit. Hence in generated IR this flag is not getting set. instead 0 is getting emitted.
> As a result, test cases checking DISPFlagNotDefaulted in IR are failing.
Given that DISPFlagNotDefaulted is represented by the absence of the other related flags, that makes sense.  Those tests would verify the DISPFlagNotDefaulted case by showing none of those flags are present.


================
Comment at: clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp:60
 // HAS-ATTR-DAG: DISubprogram(name: "declaration2", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
-// HAS-ATTR-DAG: DISubprogram(name: "struct1", {{.*}}, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+// HAS-ATTR-DAG: DISubprogram(name: "struct1", {{.*}}, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized | DISPFlagNotDefaulted)
 // HAS-ATTR-DAG: DISubprogram(name: "struct1", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
----------------
Because DISPFlagNotDefaulted has a zero value, the unmodified test correctly verifies that no other defaulted/deleted flags are present.


================
Comment at: clang/test/CodeGenCXX/debug-info-not-defaulted.cpp:9
+
+// ATTR: DISubprogram(name: "not_defaulted", {{.*}}, flags: DIFlagPublic | DIFlagPrototyped, spFlags: DISPFlagNotDefaulted
+// ATTR: DISubprogram(name: "not_defaulted", {{.*}}, flags: DIFlagPublic | DIFlagPrototyped, spFlags: DISPFlagNotDefaulted
----------------
SouraVX wrote:
> SouraVX wrote:
> > This test case is failing, checking DISPFlagNotDefaulted.
> Please note here that, backend and llvm-dwarfdump is fine without this. 
> Since it's value is '0' , we are able to query this using isNotDefaulted() -- hence attribute 
> DW_AT_defaulted having value DW_DEFAULTED_no is getting set and emitted and dumped fine by llvm-dwarfdump.
DISPFlagNotDefaulted is not explicitly represented in the textual IR; it is implied by the absence of any of the other deleted/defaulted values.  The test needs to verify that spFlags is omitted from these DISubprogram entries; or if there are other spFlags present, it must verify that the other deleted/defaulted values are not present.


================
Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:93
+HANDLE_DISP_FLAG((1u << 10), DefaultedInClass)
+HANDLE_DISP_FLAG((1u << 11), DefaultedOutOfClass)
 
----------------
There are 4 mutually exclusive cases, which can be handled using 4 values in a 2-bit field.  We will give NotDefaulted the zero value, so it is not explicitly defined here.  So we would have:
```
HANDLE_DISP_FLAG((1u << 9), Deleted)
HANDLE_DISP_FLAG((2u << 9), DefaultedInClass)
HANDLE_DISP_FLAG((3u << 9), DefaultedOutOfClass)
```


================
Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:98
 // NOTE: Always must be equal to largest flag, check this when adding new flags.
-HANDLE_DISP_FLAG((1 << 8), Largest)
+HANDLE_DISP_FLAG((1 << 11), Largest)
 #undef DISP_FLAG_LARGEST_NEEDED
----------------
This can be 10, because we used only 2 bits for deleted/defaulted.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:1615
     SPFlagVirtuality = SPFlagVirtual | SPFlagPureVirtual,
+    SPFlagDefaultedInOrOutOfClass =
+        SPFlagDefaultedInClass | SPFlagDefaultedOutOfClass,
----------------
I would call this SPFlagDeletedOrDefaulted.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:1632
+  static DISPFlags
+  toSPFlags(bool IsLocalToUnit, bool IsDefinition, bool IsOptimized,
+            unsigned Virtuality = SPFlagNonvirtual,
----------------
No, you don't want to modify this function.  It is for converting from older bitcode formats that did not have a DISPFlags field.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:1777
+  }
+  bool isDeleted() const { return getSPFlags() & SPFlagDeleted; }
 
----------------
With all 4 values encoded in one field, isDeleted would become
```
return (getSPFlags() & SPFlagDefaultedOrDeleted) == SPFlagDeleted;
```
and of course the others would use the new mask name as well.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1309
+              dwarf::DW_DEFAULTED_no);
+    if (SP->isDeleted())
+      addFlag(SPDie, dwarf::DW_AT_deleted);
----------------
`else if` here.  It cannot be both defaulted and deleted.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:603
   case SPFlagVirtuality:
+  case SPFlagDefaultedInOrOutOfClass:
     return "";
----------------
This would go away, if we pack 4 values into 2 bits.


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

https://reviews.llvm.org/D68117