[PATCH] D21526: [codeview] Improved array type support (multi dimension array)

[Reid Kleckner via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm with a better comment about arrays of incomplete type.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:858-862
@@ +857,7 @@
+
+  // FIXME:
+  // There is a bug in the front-end where an array of an incomplete structure
+  // declaration ends up not getting a size assigned to it.  This needs to
+  // be fixed in the front-end, but in the meantime we don't want to trigger an
+  // assertion because of this.
+  if (Ty->getSizeInBits() == 0) {
----------------
aaboud wrote:
> rnk wrote:
> > Nice find! However, let's file that bug in http://llvm.org/bugs, and once we fix it, this code doesn't need to change, so I don't see why we need a comment here about it. It's correct as written, assuming the frontend does the right thing.
> OK, will report a bug in LLVM.
> I can remove the comment, however, if we fix this bug we will not need the check below, that is why I added the comment in the first place.
> If you think that we should not assert that arrays with total size zero, does not sum into non-zero size, then we can let this check here exist. Otherwise, this check is a temporary one and should be removed once the above bug is fixed.
> 
> What is your preference?
OK, now I understand. I thought we needed this check to pass the assertion in the simple incomplete type case. I think the comment needs to elaborate that this is only a problem when the code builds an array of incomplete struct type, and then later completes the struct type. In fact, just putting the test case in the comments helps:
  struct A (*p)[3];
  struct A { int f; } a[3];

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:881
@@ +880,3 @@
+    // FIXME: Make front-end support VLA subrange and emit LF_DIMVARLU.
+    if (Count == -1) {
+      Count = 1;
----------------
aaboud wrote:
> rnk wrote:
> > VLA stands for "variable length array", not "very large array". :)
> > 
> > Why do we need to change the frontend? Don't we already know that `Count == -1` implies a VLA? This will be a backend change, not a frontend change.
> >VLA stands for "variable length array", not "very large array". :)
> Indeed, I thought I wrote it right :) will fix it.
> 
> >Why do we need to change the frontend? Don't we already know that Count == -1 implies a VLA? This will be a backend change, not a frontend change.
> 
> Because we should emit something like this:
> 
> 
> ```
> ; void foo(int x) {
> ;   int dyn_size_arr[x];
> ;   dyn_size_arr[0] = 0;
> ; }```
> 
> ```
> REFSYM (0x1003) {
>   TypeLeafKind: LF_REFSYM (0x20c)
>   Constant {
>     Type: long (0x112)
>     Value: 0
>   }
> }
> 
> REFSYM (0x1004) {
>   TypeLeafKind: LF_REFSYM (0x20c)
>   RegRelativeSym {
>     Offset: 0x40
>     Type: int (0x74)
>     Register: 0x14E
>     VarName: x
>   }
> }
> 
> DIMVARLU (0x1005) {
>   TypeLeafKind: LF_DIMVARLU (0x120a)
>   Rank: 1
>   IndexType: int (0x74)
>   Bounds: (0x1003:0x1004)
> }
> 
> Array (0x1006) {
>   TypeLeafKind: LF_DIMARRAY (0x1508)
>   ElementType: int (0x74)
>   DimensionInfo: 0x1005
>   Name:
> }
> ```
> 
> We need to track dynamic array range, which will be an LLVM IR value.
> Can we do this without adding a new dbg intrinsic?
> 
Thanks for the explanation, makes sense.


http://reviews.llvm.org/D21526