User Details
- User Since
- Mar 19 2018, 2:57 AM (218 w, 5 d)
Thu, May 26
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.
Tue, May 24
Fri, May 20
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.
Tue, May 17
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?
Fri, May 13
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.
Thu, May 12
Tue, May 3
/me squints,
Mon, May 2
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
Fri, Apr 29
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.
Thu, Apr 28
Apr 27 2022
Apr 26 2022
Apr 25 2022
(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.
Apr 21 2022
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.
Apr 19 2022
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.
Apr 13 2022
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,
Apr 12 2022
/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).
Apr 6 2022
Dropping in with the addition of a null-check in SelectionDAGISel::SelectAllBasicBlocks, not all targets actaully create a FastISel instance when createFastISel is called.
Apr 4 2022
Green dragon mailed me a test failure:
Apr 1 2022
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).
Mar 17 2022
Mar 4 2022
Mar 1 2022
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)
Feb 28 2022
clang-format and downgrade arguments to "process" to being ValueTable pointers -- no need to pass in the whole UniquePtr. Add const too.
Feb 23 2022
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.
Feb 21 2022
LGTM
Feb 11 2022
LGTM with query; More restructuring to avoid more indentation would be nice, but here we are ._.
Feb 10 2022
Feb 9 2022
LGTM
Feb 8 2022
Feb 7 2022
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.
Feb 4 2022
Feb 3 2022
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"
Feb 2 2022
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.