Fix handling of multi-dimensional arrays.

We are going to move forward with this approach (after dealing with the multi-dimensional array case) temporarily. We are seeing crash bugs from this from users and we want to fix it while we explore the solution space more.

LGTM

@martong I have been experimenting w/ how we pass around ImportDefinitionKind and pushing it through to more places does not help. I also tried defaulting most locations to IDK_Everything to experiment to see if maybe I was missing some crucial point and no luck. So while it seemed like a good direction it has not worked out, unless I missed something in your idea.

LGTM with minor comments. Thank you for these fixes, they are awesome!

-Add REQUIRES x86

• Fix-up the comments in the test

Thank you finding these additional fixed, it looks good but I don't know some of the details as well as Adrian so I would prefer he gives the LGTM.

This is nice catch! LGTM

Minor comment but LGTM, nice fix!

LGTM, it is sad we lose the deterministic ordering though. Would it be efficient enough to use a vector but keep the set to prevent duplicates?

Update test name

I am curious about constexpr static as well here.

Updated test using more compact code from Fred.

Replacing python test with Shell test

We found a way to hand modify the assembly and it looks good, I just need to convert it to a test.

In D85376#2209638, @labath wrote:

This manifested itself for variables in registers because those end up being described as eValueTypeScalar, is that so?

If that's the case, then I think this would also manifest for variables described via DW_OP_constu 0xdead, DW_OP_stack_value. And if *that* is true, then this could be tested a lot simpler by having a global variable described by DW_OP_stack_value and "target variable"-ing it -- no makefiles, no python, just a simple .s file. And it would run on all platforms, not just darwin.

test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s tests a very similar thing in precisely this way. I suspect you could just take that test as a template and replace the struct definition with one that contains bitfieds.

Updated to use llvm.org clang, remove header files from the main.c and add the commit hash for the clang so that it is easier to replicate the main.s in the future.

• Add more tests

• Refactored the Makefile and test based on offline comments from Adrian.

Removing use of -O1 from Makefile.

Note: for the test I did not want to rely on clang choosing to pass the union by register, so I used assembly which ensures I will obtain the behavior I am looking to capture for the test.

Do we understand how to fix the crash in the case where we get a pair back? I am not sure how frequent the use case is.

Updating diff since the parent landed.

LGTM

I updated to include the radar number.

In D84272#2168172, @davide wrote:

why? Do you have a testcase?

Removing text->GetValueAsUnsigned(0) == 0 check.

I was testing out how NSStringSummaryProvider deals w/ NULL using NSString *foo = nullptr and we filter out NULL values in ValueObjectPrinter::GetValueSummaryError(...) :

Thank you for this fix!

LGTM but I want @martong to take a look as well and accept.

@labath why do we need two copies of use_lldb_suite.py?

Added clarifying comment.

It does indeed solve the immediate problem but if those namespaces aren't really meant to be shared across translation units they should have different names but that is for another PR.

Curious, why didn't we do it this way before?

@aprantl I think this Objective-C Runtime Programming Guide: Bye Encodings entry and this sample program answer the rest of your questions:

We can never know the offsets statically b/c of how Objective-C deals with the fragile base class problem the runtime may have to shift fields over and we can not calculate this statically.

Moved from shell test

• Removing spurious local variables in test
• Simplifying the test

In D83008#2136303, @teemperor wrote:

I think we are talking about different things. My question was why is this a 'Shell' test (like, a test in the Shell directory that uses FileCheck) and not a test in the API directory (using Python). An 'API' test could use the proper expression testing tools. And it could actually run when doing on-device testing (which is to my knowledge not supported for Shell tests) which seems important for a test concerning a bug that only triggers when doing on-device testing (beside that one ubuntu ARM bot).

In D83008#2131776, @teemperor wrote:

Thanks for tracking this down, this is a really nasty bug...

The fix itself is obviously fine, but I think I'm out of the loop regarding the testing strategy. We talked about adding a Clang test for this with the help of this layout overwrite JSON file. I assume that extending this to cover virtual bases turned out to be more complicated than expected? FWIW, I'm not necessarily the biggest fan of this Clang test option so I would be fine if we leave it as-is.

I think having an LLDB test is a good idea, but it's not clear why it's a Shell test. If I understand correctly this test requires running on arm64 (so, a remote test target), but from what I remember all the remote platform support is only in dotest.py? Also pretty much all other expression evaluation tests and the utilities for that are in the Python/API test infrastructure, so it's a bit out of place.

Also I think the test can be in general much simpler than an arm64-specific test. We get all base class offsets wrong in LLDB, so we can just write a simple test where you change the layout of some structs in a way that it doesn't fit the default layout. E.g., just putting alignas on a base class and then reading fields should be enough to trigger the same bug.

Adding a second test that is not arm64 specific.

LGTM

LGTM

So with the debug info before this fix what behavior does gdb and lldb have?