Without this patch LLDB will naively interpret the DIE offset 0x51 as the static size of the array, which is clearly wrong.
This patch uses LLDB's dynamic type mechanism to re-parse VLA types with an optional execution context, to dynamically resolve the size of the array correctly. These dynamic types are not being cached, since they are only valid in a single execution context.

Parsing a type shouldn't need an execution context and we shouldn't be re-parsing a type over and over for each frame. We should be encoding the array expression somewhere that we can access it when we need to get the number of children using the current execution context.

The way I would prefer to see this:

Revert all SymbolFile changes that added an execution context to type parsing

Change the type parsing logic in SymbolFileDWARF to store the array count expression in some format in the TypeSystem (ClangASTContext.cpp) that is associated with the clang opaque type via clang type metadata maybe?

Change "virtual uint32_t TypeSystem::GetNumChildren(...);" and "virtual CompilerType TypeSystem::GetChildCompilerTypeAtIndex(...);" to take an execution context that defaults to nothing like you did with the type parsing in the first patch. This execution context can be used to evaluate the count expression as needed. We can attempt to grab the count expression from the clang opaque type that we stored in the above step, and if one exists, use the current frame to evaluate it correctly.

My understanding is that it is not possible today to get here with a swift type. In fact, GetDynamicTypeAndAddress in swift will be always called on a SwiftLanguageRuntime.

That said, I'm not necessarily advocating against the abstraction, just pointing out that this is just a theoretical concern, and given this is an internal API in the debugger, it could be possibly enforced by an assertion to make sure people don't change the status quo accidentally.

I now spent (way too much :-) time experimenting with alternative implementations of this patch. In https://reviews.llvm.org/D53961 there is a version that changes GetNumChildren() to take an execution context. I don't think that doing it this way is a good solution either and here's why:

If we move the logic to GetNumChildren; we can print the array elements, but the type remains unchanged, which makes for a worse user experience.

With VLAs implemented as dynamic types we get

(lldb) fr v vla
(int [4]) vla = ([0] = 1, [1] = 2)

if we only change GetNumChildren() we get

(lldb) fr v vla
(int []) vla = ([0] = 1, [1] = 2)

which is less nice, since we no longer see the size of the array.

What i'm proposing is to keep the implementation as a dynamic type in ItaniumABILanguageRuntime.cpp but reduce the surface area of the execution context pass-through in DWARFASTParserClang.cpp. Similar to what I did in https://reviews.llvm.org/D53961 we can add a ResolveDynamicArrayUID(uid, exe_ctx) method that only creates an array type. Since we need to re-create a fresh clang type at every execution context anyway, going through the single-function array-only DWARF parser seems like the right trade-off to me. Let me know what you think!

The only other thing you would need to change to get the usability back in check when doing things in GetNumChildren() would be to have the function that gets the typename take on optional execution context for dynamic types. The ValueObject can easily pass its execution context when getting the typename. Anyone who doesn't would continue to get the "int []" as the typename which isn't really lying either way. Thoughts?

To me a VLA fulfills all properties of a dynamic type so not modeling it as a dynamic type feels backwards to me. But not having to deal with temporary clang types might be worth the trade-off.

The only other thing you would need to change to get the usability back in check when doing things in GetNumChildren() would be to have the function that gets the typename take on optional execution context for dynamic types. The ValueObject can easily pass its execution context when getting the typename. Anyone who doesn't would continue to get the "int []" as the typename which isn't really lying either way. Thoughts?

I didn't realize that the string int [] is produced by ValueObject itself; I think this makes this option more palatable to me. I'll give it a try.

I didn't realize that the string int [] is produced by ValueObject itself; I think this makes this option more palatable to me. I'll give it a try.

So, it turns out it isn't. The only way to get the length into the typename is to hack ClangASTContext::GetTypeName to replace the training "[]" of what clang returns as the typename with a different string. The main problem with this is that this will only work for outermost types.

Something that has been requested in the past is to support C structs with trailing array members, such as

struct variable_size {
unsigned length;
int __attribute__((size=.length)) elements[]; // I just made up this attribute, but you get the basic idea.
};

in a similar fashion. When printing such a struct, there's no way of safely injecting the size into array type string any more.
If we dynamically created the correctly-sized array type instead, this would work just fine.

I haven't yet understood the motivation behind why overriding GetNumChildren/GetTypeName/GetChildAtIndex is preferable over creating a dynamic type in the language runtime. Is there something that I need to know?

I didn't realize that the string int [] is produced by ValueObject itself; I think this makes this option more palatable to me. I'll give it a try.

So, it turns out it isn't. The only way to get the length into the typename is to hack ClangASTContext::GetTypeName to replace the training "[]" of what clang returns as the typename with a different string. The main problem with this is that this will only work for outermost types.

Something that has been requested in the past is to support C structs with trailing array members, such as

struct variable_size {
unsigned length;
int __attribute__((size=.length)) elements[]; // I just made up this attribute, but you get the basic idea.
};

in a similar fashion. When printing such a struct, there's no way of safely injecting the size into array type string any more.
If we dynamically created the correctly-sized array type instead, this would work just fine.

I haven't yet understood the motivation behind why overriding GetNumChildren/GetTypeName/GetChildAtIndex is preferable over creating a dynamic type in the language runtime. Is there something that I need to know?

Creating a new dynamic type every time you stop seems like a lot of work and a possible waste of memory. You will need to see if you already have such a type ("int[4]" already exists) and use it if it does each time you stop. Any type that can change dynamically should easily be able to be detected by the type system in GetNumChildren(...) with the right execution context and and a dynamic type name can also easily be calculated in the type system. One option would be to just display "int[]" and have a summary for any dynamic array types that says "size = 4". Then we don't have to touch the typename.

For dotest style tests, the debug format to test is chosen by the test driver. All the tests should run with any format, but sometimes there are bugs in one reader or another (or in one version of DWARF or another) so you can skip or xfail a test based on format. Sounds like this test should be skipped when the debug format is PDB, since it seems like either the PDB format doesn't express a way to find the actual size of the vla (or the current PDB reader doesn't process those records).

There is a regression in trunk clang+lldb on Fedora 29 x86_64. Using either 7.0 clang or 7.0 lldb makes the testcases PASS but both trunk clang and trunk lldb makes the testcase FAIL.
This commit (rL346165) is the regressing one for LLDB, commit rL349207 by @dblaikie is the regressing one for clang.