This is an archive of the discontinued LLVM Phabricator instance.

[codeview] Improved array type support (multi dimension array)
ClosedPublic

Authored by aaboud on Jun 20 2016, 12:06 PM.

Details

Summary

Added support for multi dimension array.

Also worked around some issues with:

  1. array of incomplete structure type.
  2. dynamic size array.

Diff Detail

Event Timeline

aaboud updated this revision to Diff 61282.Jun 20 2016, 12:06 PM
aaboud retitled this revision from to [codeview] Improved array type support (multi dimension array).
aaboud updated this object.
aaboud added reviewers: rnk, majnemer, amccarth.
aaboud added subscribers: llvm-commits, bwyma, smerritt.
majnemer added inline comments.Jun 20 2016, 12:29 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1022–1025

If a type is incomplete, how can we emit a size for it?

rnk edited edge metadata.Jun 20 2016, 12:51 PM

Thanks, seems like the right approach.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1021–1025

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.

1039

Why not just assert(Subrange->getLowerBound() == 0 && "codeview doesn't support subranges with lower bounds");? As is, your code will probably trigger unused variable warnings in NDEBUG builds.

1044

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?

1057

I think without adding (void)UndefinedSubrange outside the assert, you will get -Wunused-but-set warnings in an NDEBUG build.

majnemer added inline comments.Jun 20 2016, 12:57 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1044

Can't we use LF_DIMVARLU for VLAs?

aaboud marked 2 inline comments as done.Jun 21 2016, 2:14 PM

Thanks for the comments.
Answer inline.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1021–1025

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)

1044

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.

aaboud updated this revision to Diff 61446.Jun 21 2016, 2:15 PM
aaboud edited edge metadata.

Applied changes according to comments from Reid and David.

rnk added inline comments.Jun 21 2016, 3:49 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1021–1025

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.

1044

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.

Thanks Reid for the comments.
Please see answers below.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1021–1025

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?

1044

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?

rnk accepted this revision.Jun 24 2016, 8:51 AM
rnk edited edge metadata.

lgtm with a better comment about arrays of incomplete type.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1021–1025

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];
1044

Thanks for the explanation, makes sense.

This revision is now accepted and ready to land.Jun 24 2016, 8:51 AM
This revision was automatically updated to reflect the committed changes.
aaboud reopened this revision.Jun 28 2016, 6:48 AM

Patch was reverted at revision (273815) due to PR28311.

This revision is now accepted and ready to land.Jun 28 2016, 6:48 AM
aaboud updated this revision to Diff 62084.Jun 28 2016, 6:51 AM
aaboud edited edge metadata.
aaboud removed rL LLVM as the repository for this revision.

Fixed PR28311.
Now we take care of base element of the array, which is derived from other type (e.g. typedef).
These derived types might have no Size field, that is why we hit the assertion:
assert(UndefinedSubrange || ElementSize == (Ty->getSizeInBits() / 8));

rnk added inline comments.Jun 29 2016, 11:18 AM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
990

use clang-format to align Tag.

1005

use clang-format here

Can you give an example of when this would be needed? It would make a good test.

test/DebugInfo/COFF/types-array-advanced.ll
18

Maybe make it 'typedef const volatile int T_INT' to cover a few more codepaths?

aaboud marked 3 inline comments as done.Jul 5 2016, 6:55 AM
aaboud added inline comments.
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1005

I am not sure there is an example where a base type of an array is a reference or rvalue reference.
I took this code from DwarfUnit.cpp, where it was used to determine base size of member types.

I would remove this code for now, unless you have other opinion.

aaboud updated this revision to Diff 62743.Jul 5 2016, 6:56 AM
aaboud marked an inline comment as done.

Improved LIT test according to Reid comment.
Applied clang-format.
Update revision to top of trunk.

rnk added a comment.Jul 6 2016, 8:44 AM

Looks good after sharing common code

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1005

Oh, then let's move it to the common base class for both, DebugHandlerBase.

aaboud updated this revision to Diff 62943.Jul 6 2016, 12:54 PM
aaboud added a reviewer: dblaikie.

Applied Reid comment and moved getBaseTypeSize to DebugHandlerBase as it used by DwarfDebug and CodeViewDebug.

Added David Blaikie to review the change in DwarfDebug.

aaboud updated this revision to Diff 63077.Jul 7 2016, 8:39 AM

Updated getBaseTypeSize to match more the top of trunk version moved from DwarfUnit.cpp.

This revision was automatically updated to reflect the committed changes.