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

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Thanks Reid for the comments.
Please see answers below.


================
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) {
----------------
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?

================
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;
----------------
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?



http://reviews.llvm.org/D21526