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

[Amjad Aboud via llvm-commits]

aaboud marked 2 inline comments as done.
aaboud added a comment.

Thanks for the comments.
Answer inline.


================
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:
> This shouldn't be a FIXME. There is nothing we can do in the frontend when the type definition is unavailable, as in the example you used in your test case. MSVC gives the array size zero, which is what your code does. You should reword the comment to document that this is intended behavior.
Actually, it is a bug. However, my example was not correct.
I fixed the example to show the issue.

It is true that incomplete structure has no size.
However if we:
1. Declare an incomplete structure.
2. Define the structure later.
3. Declare an array of that defined structure.
Then the array will have no size, though it will be pointing to the defined complete structure.

This leads the assertion to be hit. (for incomplete structure without definition we do not hit assertion)

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:881
@@ +880,3 @@
+
+    // FIXME: this is a WA solution until solving dynamic array boundary.
+    if (Count == -1) {
----------------
majnemer wrote:
> rnk wrote:
> > This comment needs work. It's worth mentioning that a count of -1 means that this is a VLA, and that it is not currently representable in CodeView. Actually, why not just let it go through as ~0U?
> Can't we use LF_DIMVARLU for VLAs?
> This comment needs work. It's worth mentioning that a count of -1 means that this is a VLA, and that it is not currently representable in CodeView. Actually, why not just let it go through as ~0U?
I will fix the comment.
output ~0U will sign to the debugger that you have a large array what might cause accessing out-of-scope memory.
Better assign the minimum size of '1' till we fix the LLVM Ir and use LF_DIMVARLU as David suggested. 


> Can't we use LF_DIMVARLU for VLAs?
We should do that once front-end supports sub-ranges of variable values.
To do this we need to introduce a new debug intrinsic, as we cannot point to an instruction from external metadata.




http://reviews.llvm.org/D21526