[PATCH] D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1237
+    return *VarSize;
+  llvm_unreachable("Unkonwn size of the variable described by DII");
+}
----------------
bjope wrote:
> aprantl wrote:
> > Unknown
> > 
> > Is this really unreachable, i.e.: would the Verifier reject a variable with a size of zero? In C (but not C++) I believe the size of `struct Empty {};` is zero. In Swift you can also have zero-sized types.
> We only end up in the unreachable if getSizeInBits failed to determine the size (it returns an Optional<uint64_t>). So *VarSize could still be zero.
> 
> But maybe we need to handle the situation more gracefully. I should probably let this method return an Optional<uint64_t> as well, and then deal with it somehow in IsVariableCoveredByType.
Oh, I didn't realize that this was an Optional! That said, the new code must not throw an unreachable for any input that is accepted by the Verifier. Both making the new code more permissive or tightening the Verifier are acceptable solutions.


================
Comment at: lib/Transforms/Utils/Local.cpp:1242
+/// \p DII isn't greater than the size of \p Ty.
+static bool IsVariableCoveredByType(DbgInfoIntrinsic *DII, Type *Ty) {
+  const DataLayout &DL = DII->getModule()->getDataLayout();
----------------
bjope wrote:
> aprantl wrote:
> > How about: `fragmentCoversEntireVariable()`
> In my world the variable and the fragment is both related to the same thing.
> But I can change it to: valueCoversEntireFragment(Type *Ty, DbgInfoIntrinsic *DII)
That name makes sense to me, perhaps the doxygen comment can be improved, too, by removing the negation. 


Repository:
  rL LLVM

https://reviews.llvm.org/D48024