Seems fine if we're already doing the same thing in a few places anyway.

Looks good - thanks!

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

In D67563#1675351, @aprantl wrote:

Swift's let is indeed closer to C's const than to a named constant in Pascal (https://wiki.freepascal.org/Constants). The Swift language spec call them constants, and they are effectively SSA variables (with storage). But (and this is where it differs from Pascal constants) let bindings can also appear inside of composite types (there are let struct and class members) and as function arguments (unless a parameter is explicitly marked as inout, similar to Fortran). I have found that DW_TAG_const is the only mechanism that works for all of these use-cases.

In D67668#1674807, @MaskRay wrote:

In D67668#1674393, @dblaikie wrote:

Could we have this as a standalone helper function (STLExtras or the like) rather than implementing it in every container?

We only need it in 2 unordered associative containers: DenseMap, and StringMap. Member methods have the advantage that they have better discoverability and can be easily be completed. This method is simple enough now. Placing it in STLExtras does not save much.

In D67563#1670227, @aprantl wrote:

In D67563#1670226, @dblaikie wrote:

Would seem to me (with an obvious C++ bias in my view of the world) that it maps best to i_am_a_constant being a DW_TAG_variable (or function_parameter) with a DW_TAG_const_type that has a base type of "Int" (however that's represented).

But I realize that might not be a good fit for Swift/its debuggers, etc, and certainly tend to leave this up to whatever you folks want to do with your language. Just my 2c :)

It's not elegant in LLDB to peel the constness out of the type and attach it to the variable (the code that takes a DWARF type and returns a Swift type would need to ignore it and the code that builds an lldb_private::Variable needs to look into the DW_AT_type), but certainly doable, and implementation details of one specific debugger shouldn't really affect this decision. The prevalence of let-bindings may cause debug info to blow up a bit with all the extra DW_TAG_const_types, but that's not going to be a showstopper either.

In D67647#1674781, @comex wrote:

In D67647#1674773, @dblaikie wrote:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

Yeah, arguments() returns an adapting iterator.

In D67647#1674745, @comex wrote:

Here's a new version of the patch that uses iterator ranges instead of ArrayRef, to avoid adding new uses of CallExpr::getArgs() in case it has to be removed in the future due to the strict aliasing issue.

In D67647#1673363, @comex wrote:

Ugh, it looks like getArgs() is a massive strict aliasing violation. And it's used in enough different places in Clang that fixing the violation may require extensive refactoring... I've reported the issue as https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it :)

However, I can at least update this patch to avoid using getArgs().

Could we have this as a standalone helper function (STLExtras or the like) rather than implementing it in every container?

As per the bug - I'm not super inclined towards a function attribute here (& even if it's going to be an inliner rather than debug info generation/backend thing (which I'm more on the fence about - think I'm happy with either direction there, really) - I'd still be inclined towards it living in the DICompileUnit with other debug info things, rather than a function attribute). But happy to hear what other folks (especially the usual debug info cabal - @aprantl @probinson @JDevlieghere etc) think.

In D67563#1670224, @aprantl wrote:

In D67563#1670183, @dblaikie wrote:

Out of curiosity does swift have const in the type system like C and C++? (I guess so) - and what would be the difference between a normal (non-let) variable of a const type, versus a let variable of a non-const type?

Swift does not have const in the type system. There are no real pointers in the language; the closest equivalent are probably constant struct members, such as:

struct MyStruct {
  let i_am_a_constant : Int = 42
  var i_am_mutable : String
}

Out of curiosity does swift have const in the type system like C and C++? (I guess so) - and what would be the difference between a normal (non-let) variable of a const type, versus a let variable of a non-const type?

In D67563#1669828, @vsk wrote:

IIUC @jingham makes the argument here that DW_TAG_constant isn't appropriate for swift's 'let', and instead suggests attaching DW_AT_const_value to the DW_TAG_variable (which lldb seems to already understand). Wdyt of that as an alternative?

What's a "true named constant" as opposed to, say, C++ (or C's) "const int X = 3;" (well, I guess const int X could be defined by a runtime expression, - OK, what about "constexpr int X = 3;" then?)

Some more ping (:

Oh, because copying the file is copying non-writable permissions? Fair enough.

I vaguely recall @lhames had some opinions on this layeering, might have another fix in mind.

In D65026#1666621, @w2yehia wrote:

Just FYI, that our (IBM's) internal bots are also breaking because the following tests are not cleaning the build/test/Reduce/tmp folder:

LLVM :: Reduce/remove-funcs.ll
LLVM :: Reduce/remove-metadata.ll

And we're forced to sometimes do a clean build.

In D67410#1666891, @vsk wrote:

The DWARFv5 forms were already the default choice when the tuning isn't set to GDB, this change is about allowing TAG_call_site_parameter to be emitted at all when the tuning is set to LLDB.

In D67410#1666856, @vsk wrote:

In D67410#1666505, @dblaikie wrote:

In D67410#1666370, @aprantl wrote:

I'm very curious what the effect of this patch on our green dragon statistics bot will be (http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/).

What sort of statistics do we track that would be impacted by this change?

None for now, because debug entry values are gated behind a CC1 flag the bot does not use.

In D67410#1666370, @aprantl wrote:

I'm very curious what the effect of this patch on our green dragon statistics bot will be (http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/).

In D65026#1666176, @thakis wrote:

The test added in this change has a file leak that causes all possible temp file patterns to be created, at which point it starts failing, see https://bugs.llvm.org/show_bug.cgi?id=43278 (Diego, I tried to cc you, but bugzilla didn't know your email address.) Can you please take a look, and revert if it takes a while to investigate? (remove-funcs seems to have the same problem.)

Might be worth having some more detail in the commit message to explain why -ffunction-sections/-gc-sections isn't sufficient to remove the binary size increase.

Seems good to me - thanks!

Have you got a link to the original thread where this was discussed/I mentioned it? Want to page in some context to double-check if I had any ideas that might've let this simplify things.

Fair enough - thanks!

Looks good to me - thanks!

In D66904#1661872, @jgorbe wrote:

It's just that it seemed weird/hacky to use asm just to stick assembler directives in the middle of the generated code instead of actual assembly, so I generated an assembly version and cleaned it up a bit. I'm happy to go back to the original shorter C++ version if that's not a concern.

Any chance of test coverage? (not sure how the rest of this analysis is tested (or how we test analyses in general))

In D66976#1660407, @grimar wrote:

In D66976#1659909, @dblaikie wrote:

In D66976#1658737, @MaskRay wrote:

In D66976#1658669, @dblaikie wrote:

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

rL370669 added a test

That doesn't seem to test this patch, though - if I revert this patch no tests fail.

Did you set LLVM_ENABLE_ABI_BREAKING_CHECKS to 1?

Before this patch NameOrErr was not consumed/ignored by consumeError.
If you revert it and run the test from rL370669, you should get a
"Expected<T> must be checked before access or destruction" failture.

In D67097#1661014, @probinson wrote:

In D67097#1659917, @dblaikie wrote:

Not sure I understand the snark at the C++ committee, but anyway.

Oh, because of their perennial willingness to change a released standard, with no way to identify revisions. The C++11 of today is not the same as the C++11 of 2011, for example. This is basic revision control, everybody (else) understands this. I mean, if we found a bug in Clang 8.0.0, we wouldn't overwrite the archived 8.0.0 to fix it. But that's effectively what the C++ committee does with its "defect repair."
The DWARF committee has the decency to provide a new version number when the interpretation of some attribute changes.

In D67276#1661066, @probinson wrote:

As written, this will work only for little-endian. There are other unittests that are bi-endian that you ought to be able to use for examples; if nothing else, bail out after checking sys::IsLittleEndianHost (but a bi-endian test would be better).

In D63672#1660282, @mgorny wrote:

In D63672#1659973, @dblaikie wrote:

In D63672#1657198, @mgorny wrote:

In D63672#1657183, @mgorny wrote:

Ok, I have an idea of using --test-arg but it would require the args to be passed before the filename. I'll post a patch.

Actually, I won't because I'm hitting some memory corruption in TestArgs and I don't really have time or motivation to fight it. Please fix your code.

Could you be more specific? I've tried running both tests under valgrind and haven't encountered any reported errors there.

In my case, passing --test-arg to the program caused TestArgs inside TestRunner to contain junk before the actual parameter strings.

Sounds good to me - thanks!

Seems fairly reasonable to me - test might be able to be cleaned up a bit. Code might be cleaned up a bit (either now or in a follow-up commit)