@bruno: When we last discussed this my plan was to avoid the stat() in lookupModuleFile() for files that were just added to the PCMCache by WriteAST() entirely, but ModuleManager::Modules is a DenseMap<FileEntry, ModuleFile *> and lookupModuleFile() is the easiest way to create a new FileEntry. It would be nice to find a way to avoid the stat() for a file that we just wrote, but it wasn't immediately obvious to me how to do that.

Looks good from my point of view.

Sorry, my mistake. I saw that you added a new field in the IR assembler and sloppily assumed that there is also a new bitcode record for it, but there isn't, so the whole point is moot.

Can you add a bitcode upgrade test?

I could - but I'm not sure it would test anything. The format has in some sense remained the same - this field is still zero/one before/after the change, but now new values are also valid.
Reckon it's still worth testing? What do you reckon that testing would cover?

Currently has no effect, but see above ^ - I reckon it probably should. Maybe we can generalize the name of the property? "nameTable: " {None, Default (v4 or v5 based on DWARF version), GNU (only makes sense for v4 though - ignored if you end up building v5? seems reasonable to me)}

That looks good!

Thanks, let's just give it try!

Do you have a testcase for this? You should be able to construct an artificial MIR testcase.

Thanks!

Looks right.

Thanks!

It would be cleanest to create a new, minimal test that only contains one global variable with a basic type. The resulting bitcode should only be a few bytes and then it would be obvious what the tests tests.

I think this is reasonable. Let's maybe give this another day to see if anyone sees a problem with this before landing it.

Thanks!

Hmm.. yeah, this looks more like a side-channel than a proper part of the MI protocol. That said, this is also what the original code was doing, so we can investigate the proper protocol separately.

The last thing that is missing is a bitcode upgrade test (unless you can show that one of the existing checked in .bc files in the testsuite also covers the upgrade path for the old, shorter DIBasicType).

Deleting dead code is always good; I'll let Jason sign this off though.

for the time being the basicType that maps to DW_TAG_base_type, but in future the variables created using that basictype or any other type with DW_TAG_variable and DW_TAG_formal_parameters tags.

Is it a property of the type or a property of the variable? I.e.: Can you have a struct with one LE member and one BE member?

It looks like my request wasn't addressed either. Can you please fix this soon?

Do you need to support endinaness attributes just on basic types or eventually on any type?
This is still missing an assembler round-trip test.

DILocation isn't only used for coverage, I think this should probably be a more general purpose flag, like "isImplicitCode", to indicate that the code was generated as part of a destructor cleanup, an epilogue, or something else that requires the compiler to generate IR that isn't obviously attributed to user source code. Then GCOV can decide for itself how it wants to generate coverage.

@aprantl @JDevlieghere - This looks good to me, but could you take a glance & see if it's reasonable for AArch64 (though this is for ELF, not MachO - but figure it might cut close enough you might want to check it)

lgtm with the two parameters moved from DIDumpOptions into explict arguments of dumpAddrSection.

sgtm.

Thanks!

Seems good otherwise.

Sorry about the late reply, I've been on vacation.

May I ask what the context of this is? Do you have a machine where some (but not all) basic types have a different endianity than the one specified by the ABI?

SGTM.

The de-templatization will reduce the code size and I doubt that the virtual method overhead will be measurable. Looks like a good direction to me.

@jingham: do you have any opinion about the right SBAPI for manipulating settings like Alexander outlined?

This looks like the right change as it makes the IR more regular.
Note that in the past this worked because allocas were implicitly indirect since they caused AsmPrinter to emit a what DWARF calls a "memory location". It's possible that that code path no longer exists or isn't triggered any more.

Thanks, I think this looks good!

Thanks!

A couple of cosmetic changes in line, but otherwise this LGTM.

This is definitely going to be very useful! It's not very large, but generally I wonder if llvm-developer-centric features like this should be made #ifndef NDEBUG-only to not weigh down clang releases' binary size?

Thanks, I think this looks good now!

That looks like a safe change to make.

Okay then let's not do this for now. It's fine to revisit this later if there turns out to be a good use-case for it, but every SBAPI call we introduce has to be supported indefinitely and can therefore be quite expensive to maintain.

Have you seen my earlier question:

Fre

Thanks. This is an obvious improvement. It begs the question whether there is a better / more principled condition rather than "the expression is empty" to decide whether the stack_value should be added or not. Let me know if you have any ideas.

I think we're almost there now!

I think this is starting to look good. I have a few more comments inline.

LGTM with the inline keyword removed.

This is going to be really nice!

This seems like a reasonable addition. Could you also add documentation for the new API?

Is your plan to add dwp as another dimension in the test matrix (an equal citizen of DWARF, dSYM, DWO) or something that would be on or off for an entire run of the suite, or something only exercised by few specialized testcases?

In other words, the mov lr, #2 is the *materialization* of the constant and is no longer associated with a specific source location.

Is the dummy target something we need to expose over the SBAPI?
I see that other places in lldb-mi query if (sbTarget == rSessionInfo.GetDebugger().GetDummyTarget()). Would that be sufficient?