akhuang added inline comments.

================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1035
+  const RecordDecl *D = RD->getDefinition();
+  if (D && D->isCompleteDefinition())
+    Size = CGM.getContext().getTypeSize(Ty);
----------------
dblaikie wrote:
> When is a definition not a complete definition? (sounds like a line from 
> Alice in Wonderland... but I'm genuinely curious/not sure I understand)
ha, good question, I copied this without really looking at it. I am also not 
sure when a definition would not be a complete definition.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4537-4538
         auto VarSize = DbgDeclare->getVariable()->getSizeInBits();
-        if (VarSize) {
-          if (Size > *VarSize)
-            Size = *VarSize;
-          if (Size == 0 || Start + Size > *VarSize)
-            continue;
-        }
+        assert(VarSize &&
+               "Any variable with a location must have a type with a size");
+        if (Size > *VarSize)
----------------
dblaikie wrote:
> This is probably a sligthly larger/separate project/patch - @aprantl would it 
> be reasonable to add this as an LLVM IR debug info metadata invariant/tested 
> in the debug info verifier?
> 
> (in terms of patch order, I guess:
> 1) ignoring the size, if present, on declarations in the backend
> 2) adding the size in the frontend
> 3) adding an invariant/verifier check that the size is always specified on 
> types of variables with locations - and changing this code here to assert 
> rather than test
> 
> But happy to review at least 1 and 2 in this review, though they could/should 
> still be committed separately - the third's probably involved enough to merit 
> a separate review, I'd hazard a guess)
I just tried adding a verifier check to see what happens, but it looks like 
incomplete arrays have locations but no size? 

I can split out 1 into another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87062

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to