Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.

LGTM

In D51976#1237392, @vsk wrote:

I expect that extending dbg.value in this way will be fairly involved. There are a couple questions worth thinking about, imo:

• Would the extra SSA values need to be constants (or live in registers)? If not, how will their locations be represented in a TAG_variable entry?
• What's the expected memory size overhead of adding an extra operand to dbg.value? Does it need to be counterbalanced by introducing dbg.value.simple() [for the case where there is an empty DIExpression/extended SSA stack]?

In D51976#1236852, @CarlosAlbertoEnciso wrote:

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

In D51908#1233588, @dblaikie wrote:

Perhaps we could implement this behavior (pr: i.e., omitting the label if it has been optimized away) only if we're tuning for GDB & otherwise produce a label without a low_pc? Not sure how other folks (@aprantl @echristo @probinson @JDevlieghere) feel about this - apply the workaround generally, because it's not super important to have a label for name-lookup porpoises? or is it worth carrying the two codepaths to ensure that slightly more accurate debug info where it's possible (presuming LLDB can cope with it)?

Can you preserved the declaration DIE (at 0x0000007c) while omitting the definition DIE (at 0x00000063)? Does that still cause a problem for GDB?

+cfe-commits

Awesome thanks!

The indentation in the test looks a little odd. Please verify there are no hard tabs.
Otherwise LGTM.

I'm abandoning this minor tweak as I am convinced that there is something more fundamentally wrong here.
llvm-dwarfdump doesn't call the right API sequence to show the bug; it requires a DWP, looking up a specific unit by hash via the DWP index, and then scanning all the units. I can envision how to do a unittest for that, but the unittest infrastructure doesn't currently support building a DWP. I can't take the time right now to address this, but I'll keep it on The List.

How does this interact with v5 .debug_names?

Two totally optional style nits. The new class distinction seems like an improvement.

Using %t with suffixes instead of %T is the preferred idiom, glad you worked that out.
The more elaborate test depends on a set of Posix-y calls, but that's fine. If there are more environments that can't handle it, the bots will find them for you. :-)
LGTM with the fancier test, once you fix the missing RUN. (The RUN prefixes are how lit decides which lines are the test script.)

I think you forgot to subscribe llvm-commits to this review?

LGTM.

Rebase. Add to and improve commentary.

Rebase.

Rebase. Renamed 'parse' methods to start with 'addUnits' to reflect their cumulative nature.

Rebase. Update some commentary.

Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir and then verify the mode of the created directory?
The test would have to be flagged as unsupported on Windows but that seems fine.

As a partial and somewhat experimental feature, this should be controlled by a command-line option and off by default.
Even when it's complete it should probably still be under at least a tuning option, unless you can demonstrate the size cost is small.

Although I am far from expert in how Clang manages declarations, AFAICT this does what @rsmith requested, so LGTM.

In D49467#1176829, @jeroen.dobbelaere wrote:

Reassembled to reset the debug id's

In D49652#1172498, @alxu wrote:

my general theory is that it's better to have tests that test everything at once if possible, but I guess it's unlikely enough that the debug info path will be correct in the IR and then not correct in the generated file and that that won't be caught by LLVM tests.

In D49652#1172352, @alxu wrote:

I was just going to run llvm-dwarfdump.

Re. SmallVector versus std::vector, they are functionally similar but have different memory-allocation behaviors. SmallVector includes a vector of N elements (where N is the template parameter) so does no dynamic allocation until you have more than N elements; but it takes up that much more space as a member of a class. It's mainly intended for stack variables. Given that this particular option is used rarely, I would probably go with std::vector. The code change otherwise is pretty simple and looks fine.

cc llvm-commits. Please do this when first creating the review.

+ @clayborg who IIRC did the original generator

Also, please document the option in clang/docs/UsersManual.rst. It should have been added when the option was first added, but certainly with right-to-left behavior it needs a mention. Nearly all options work left-to-right, so even though it's the same as gcc, not everybody comes to clang from gcc.

Needs a test.

Sorry, lost track of this one. One comment suggestion and LGTM.

If the plan is for this to be relatively temporary then LGTM.

It seems like the metadata could be reduced further. In particular, do you really need all those different types? The dbg.value needs only !11, I think you should be able to remove !12 through !18 (and references to them).

Is this because type units depend on COMDAT support? I had a vague idea that COFF also supports COMDAT.

Please rename the new test to simply 'codegenprepare-and-debug.ll` (the convention of a date on the test name is obsolete, we don't do that for new tests anymore).

LGTM but I'll let @JDevlieghere have the last word. It looks like he still has one open question in DwarfDebug.cpp.

I agree the ordering test looks fragile. The only other option that comes to mind is trying to exercise the APIs directly from a unittest, but then you need enough scaffolding around it to capture at least the assembler output and verify it. Probably best to leave the test as it is.

@rsmith any further thoughts? We would really like to get this in before the LLVM 7.0 branch, currently scheduled for 1 August.

A few typos.

A bunch of style comments. Maybe try clang-format-diff.

I wonder if we should experiment with making the line-table builder complain about a .loc that doesn't increment the address. That would tell us whether these zero-length cases show up anywhere else. Or, if we decide the assembler should handle it correctly, whether anything slips through.

In D49426#1166535, @dblaikie wrote:

However, this particular piece of work addresses what I perceived to be an artefact, a .loc directive for a missing prologue.

I'm not sure it's really a bug here, though - if the assembler addressed the sub-optimality of producing zero-length regions. It seems to me like a natural implementation to change the location, emit the prologue, then change the location again - and if the prologue happens to be empty/zero-length, the correct output falls out naturally. Avoiding special casing the empty prologue keeps the code simple/easier to read/understand, I think.

I will try to look at this tomorrow if @vsk doesn't. Passes are not really my thing but as it's based on the existing opt code it should not be too hard to review.

The repeated address is syntactically valid per the rules for the line-number program, but semantically undefined per the rationale in the non-normative description of the table that the line-number program is supposed to be encoding (i.e., a table indexed by instruction address).

This is slightly complicated by the fact that the debug_names tables are
incompatible with the DWARF v4 type units (they assume that the type
units are in the debug_info section), and unfortunately, right now we
generate DWARF v4-style type units even for -gdwarf-5.

+llvm-commits.

+llvm-commits