In D67563#1675356, @aprantl wrote:

How do Fortran compilers represent regular versus inout arguments in DWARF?

How do Fortran compilers represent regular versus inout arguments in DWARF?

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.

After some consideration I concluded that this isn't the best approach either. [There's no plan to do that, but] If we ever want to serialize Swift types into DWARF then an attribute on (local) variables isn't enough to describe let-members of structs. So instead I'm currently preparing a patch (no LLVM involved) that will wrap let-bound types in DW_TAG_const_type (like the const qualifier in C).

Please be sure the watch the lldb (and other debugger) bots after committing this..

I'm afraid I'm going to give up on fixing the AST and return to my debuginfo-only patch.

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 :)

-> https://docs.swift.org/swift-book/ReferenceManual/Types.html

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?

Okay. Here is an alternative patch that implements a flag DW_AT_LLVM_constant. "The Swift Programming Language" calls let-bindings "constants" and constant seems to be a generic enough term to be generally useful.

In D67500#1669456, @jmorse wrote:

In D67500#1668097, @aprantl wrote:

I'm assuming that at the beginning all in-locations are unknown and only when all incoming basic blocks have outgoing locations, join() adds new incoming locations to be processed.

Ah, this is where the implementation diverges, in fact if all incoming blocks have the same outgoing location *or* unknown for a variable, join() will consider it a valid live-in location. I think it has to be this way because of loops -- for a location that is live throughout a loop, there's a circular dependency between the outgoing location for the back-edge and the live-in location for the loop header. Right now we assume the location is correct for any unknown blocks, and have to invalidate locations later if that doesn't turn out to be true. See for example D66599.

I guess this boils down to how we are not really modeling "unknown" currently?

It turns out to be even more exciting than that -- consider this IR [0] which has badly behaved, but completely legal control flow. It's illustrated in this highly sophisticated diagram [1]. A single variable location (toast==0) is created in the entry block, and then invalidated with an undef dbg.value in loop 3. Because every non-entry/non-exit block is reachable from every other block, the location-invalidation has to be propagated to them all. However, it takes at least one LiveDebugValues iteration for the invalidation to traverse a back-edge, so it takes two LiveDebugvalues iterations for the invalidation to move up the CFG and reach loop1. In that time, all the "unknown" blocks have been processed, and as a result locations have to transition from "true" to "false", rather than "unknown" to "false". It might be possible to generate a situation where something transitions from "false" to "true" too, but I haven't looked closely at that yet.

I think the fact that this is happening means I've made LiveDebugValues depart from peoples expectations, you wrote in the 2016 mailing list dicsussion [2]

the information at the nodes can only move in one direction in the lattice and can change at most once.

which isn't true any more because of D66599 removing locations. I suspect I've turned LiveDebugValues into a data-flow-like algorithm rather than a joining-lattices algorithm. Whether this is a good idea or not probably needs discussing; it's all been in aid of getting rid of invalid locations though.

[0] https://reviews.llvm.org/P8163
[1] https://imgur.com/a/YdE9kN7
[2] https://groups.google.com/forum/#!search/llvm-dev/llvm-dev/nlgxIfDQnhs/4zsGbuoTAAAJ

Before creating this patch I had considered 4 options:

Normally I would insist on a bitcode upgrade, but since this feature is so new I think we can skip that. However, because of bitcode compatability, we might want to get the semantics right from the start. Otherwise we might have to allocate a new opcode and implement an upgrade when we need to change it in the future.

minor nits inline

Looks mostly good.

just don't record transfers until the in-locations are all known to be correct

Having suggested this, I think this is a good idea ;-)

Found one more stale decl.

Good point!

My plan is to reuse the bit for an attribute that does not appear inside of DICompositeType, so we could leave the Verifier check in indefinitely. I should have said so though.

Thanks, that's exactly what I was looking for!

Thanks!

Otherwise this LGTM now.

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 D67410#1665698, @djtodoro wrote:

An idea: After this got implemented, we can add a new sub-directory inside the debuginfo-tests project called DOC (Debugging Optimized Code) and start testing (and tracking) improvements in that area. WDYT?

Looking great! I have one more question inline, but that is mostly about documenting the algorithm.

Okay, we should make sure that we do get the design right. I don't quite get the argument made in SourceLevelDebugInfo.rst for why the semantics are different:

As stated in the docs here [0], the meaning of a DBG_VALUE instruction changes after LiveDebugValues runs

In D66121#1656442, @rjmccall wrote:

Can you prepare an NFC patch with just the changes relating to adding ObjCPropertyImplDecl::get{Getter,Setter}MethodDecl?

In D67373#1663924, @dblaikie wrote:

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.

This is very exciting!

This is a strict improvement and as such is fine, but there's some general cleanups that are uncovered by this that we should also be doing.

Nice! Are you planning to address the FIXME's in a later update of this patch?

In D64595#1657960, @jmorse wrote:

Enjoyable leaps in variable-coverage / bytes-coverage on the far right of these graphs:

http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4&highlight_run=128792
http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4&highlight_run=128792

Thanks for digging into this!

I see we would have to do an abstract interpretation of a DIExpression together with the implicit register operand bound by the DBG_VALUE to computes all stack slots that my be touched by the expression, assuming we even know where non-frame-pointer registers point. Could you add an explanation to the comment that lays out why this isn't feasible?

This looks like a very good idea!

lgtm with oustanding comments addressed!

Thanks for implementing the error handling! There is one more comment inline.

Here is a work-in-progress alternative patch that attacks the problem by changing the AST generation to have an ObjCMethodDecl for the property accessors inside the implementation as suggested by @rjmccall.

If it helps...
Is there anything else we can print for a crashed dotest?

This makes sense, I'm kind of shocked that me missed something that obvious. Is there a big drop in variable locations with this (i.e., an improvement in accuracy)?

Currently, if a stack spill location is overwritten to by another spill instruction, nothing notices and any variables located in that slot are not terminated.