This seems fine to me -- something very similar happens with NRVO in C++, a dbg.declare(..., DW_OP_deref) is created for the implicit output argument. There's a test demonstrating this in clang/test/CodeGenCXX/debug-info-nrvo.cpp.

In D125953#3527758, @jmorse wrote:

Additionally, the test file should be MIR and only run livedebugvalues, to reduce the surface of things being tested.

IIRC this is sound, from our discussion elsewhere the problem is that we do not eliminate a PHI if the incoming values agree, but one value comes from a PHI and the other does not. IMO: this should also get a unit test entry to ease the identification of errors in the future. Additionally, the test file should be MIR and only run livedebugvalues, to reduce the surface of things being tested.

Also, many thanks for driving this. After much introspection, I think that every time I came back to this document I tried to rewrite it from scratch, which is kind of a blocker against forward progress :(.

All those revisions / updates look good to me -- @StephenTozer @Orlando would you be able to give another look-over?

While the patch does fix the immediate issue, I'm not certain that disabling the valueCoversEntireFragment check in the context of pointers is a general solution. Otherwise generic code that does something strange (like breaking up allocas) would expect dbg.value's to become undef, and not doing that for certain types risks causing unexpected behaviour in the future. It's also good practice to be conservative in what input we accept here.

/me squints,

Thanks for the reproducer; while it's mildly hard to interpret what's going on, I think a large block of local stack memory that's the subject of a dbg.declare, is being recorded by a

Thinking about it, actually we can easily support the llvm.dbg.addr with complex-expression scenario, just by weakening the assertion that's firing. The code as written will cope just fine with dbg.addrs that perform a computation, the true problem is when there's a DW_OP_stack_value in the expression. That's what this assertion should have been testing for in the first place. It doesn't make sense for a variable to both be indirect, and computed on the DWARF expression stack.

To explain my thinking -- coroutines recently started using dbg.addr, and can be salvaged in 0b647fc529, which will create complex DIExpressions (aka anything with a DW_OP_stack_value). I think this might be the first scenario where dbg.addrs are generated and can be salvaged, which then messes up other code I've written in SelectionDAG, that assumes a dbg.addr can be treated as a more specific dbg.declare. If so, the fix is probably to flush the indirectness out of the intrinsics/instructions at SelectionDAG, seeing how everyone wants the IsIndirect flag gone in the long run.

Thanks for reverting; I think this has uncovered some changes in my assumptions of how some variable locations are described. Would you happen to know whether the source being compiled contains coroutines? There's been some work there lately which might have had an effect.

In D113586#3480392, @jryans wrote:

@jmorse It would be great to get this landed, as it's quite a useful reference to have around. 😄 If the main blocker is lack of time, I'd be happy to take over and work through the review comments so this can move forward.

(Addressing inline comments -- note that they've now shifted down a few lines).

Refreshed with a more comprehensive test plan -- see deref-spills-with-size.mir where there's a truth-table (well, kind of) of what inputs should lead to what output expressions, for:

• Different sizes of value on the stack,
• The differing presence of stack values / no stack value,
• Whether the variable is a scalar or a fragment.

The aforementioned problems are addressed in D124184, this patch revises the test to cover scenarios where DBG_PHIs refer to the stack, and ensure that the size operand of DBG_PHI is considered.

Drive by review from a debug-info perspective -- I'm not a RISC-V person so don't feel confident enough to approve this, but I can see how the removed code will crash in the presence of a block only containing debug instructions, and that the new code will not crash. The fix looks correct in that sense, but someone else should confirm that it's not going to change codegen.

Stone the crows, of the Dexter tests we have that originally detected this problem, another has found a problem in this patch -- for values defined in a stack slot, InstrRefBasedLDV currently guesses the size [0] because it can't work it out so late in compilation. In a piece of code dealing with vectors:

• The variable is a "vector" class with a single array of 4 f32's, that get promoted and vectorised it seems,
• The vectorised value is subject to a PHI at some point,
• A DBG_INSTR_REF refers to the PHI,
• This becomes a DBG_PHI referring to a stack slot after regalloc,
• The code in [0] guesses that the stack slot contains 64 bits, where in reality it's 128 bits,
• 64 bits is not the size of the variable, so a DW_OP_deref_size and DW_OP_stack_value is used, and we lose half the vector.

Drive-by comment on the topic of the gnu pubnames error,

/me squints -- looks like the test doesn't cover the case of expressions with fragments, which I'll revise in later. (The patch has a case for that).

Dropping in with the addition of a null-check in SelectionDAGISel::SelectAllBasicBlocks, not all targets actaully create a FastISel instance when createFastISel is called.

Green dragon mailed me a test failure:

This has come up on my radar, still applies cleanly, and passes all the debug-info tests. I'm out of time in my timezone, but without objection will commit next week (unless someone else gets there first).

A sample of things are D63083, https://bugs.llvm.org/show_bug.cgi?id=40188#c8 , D61940. Although looking back, they all appear to be about not making complicated location-list entries in the prologue, rather than the epilogue.

(Remembering to upload the modified tests this time)

clang-format and downgrade arguments to "process" to being ValueTable pointers -- no need to pass in the whole UniquePtr. Add const too.

Switch to passing an empty unique_ptr for value table arguments, instead of an optional raw pointer to a unique_ptr. This avoids un-necessary raw pointer usage.

LGTM

LGTM with query; More restructuring to avoid more indentation would be nice, but here we are ._.

In D118998#3304180, @Orlando wrote:

LGTM. Converting badly-formed variable locations into "no location" rather than crashing the compiler SGTM. Is it worth fixing the defect in the bug report too in a separate patch?

LGTM

In D95617#3300552, @ro wrote:

no-empty-child-vars.ll FAILs on SPARC: the first

; CHECK:     DW_TAG_subprogram
; CHECK:       DW_AT_abstract_origin (0x{{[0-9a-f]*}} "foo")

is missing completely and both qux and croix lack DW_TAG_inlined_subroutine.

Any suggestion what to do about this?

LGTM, given the long list of other XFails.

SGTM with a couple of nits. From the discussion on discourse, I think it's a deliberate choice that there's going to be incompatible configurations out there, in which case as you say the least-worst-option is xfailing these tests.

Jeremy's journey:

• Orlando: "This allocation/free code looks sketchy now,"
• dblaikie: "You should use std::unique_ptr!,"
• asan buildbots: "YOU SHOULD REALLY USE STD::UNIQUE_PTR"

Sitrep: I've re-enabled instr-ref for x86_64 in rG6e03a68b776d. I believe all the issues reported here so far are fixed / optimised further, so I've filed a cherry-pick request in https://github.com/llvm/llvm-project/issues/53548 , which I believe is the correct procedure.

In D118715#3289898, @StephenTozer wrote:

Is there any particular reason that the undef value test couldn't exist within the original function, or are they separated to look cleaner? YMMV and I don't feel strongly either way, so LGTM.