Thank you for demonstrating this! I agree that the patch also introduces lots of opportunities for subtle bugs. It also increases the memory footprint in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support. This patch looks much more palatable to me now and I'd be okay with taking it if @dblaikie is okay with it. Is there danger in using the same function name or should we call the 64-bit variants something more explicit?

In a few cases it may be beneficial to reorder struct elements such that groups of 32-bit or smaller elements aren't interrupted by a 64-bit element for better packing.

We need to come up with a better error handling strategy before this can be landed.

The important part is that the error must be checked (which isn't enforceable through bool returns). Whether it's after each function call or later, you probably have a better idea about.

JF is currently working on making BitStream error-safe over in https://reviews.llvm.org/D63518. Perhaps there are some patterns that are worth copying.

In D56151#1557861, @jmorse wrote:

Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?

The patch effectively makes the codegen backend stricter about what DBG_VALUE insts it will accept, i.e. they have to refer to vregs that are alive. If it helps, this is already something that's enforced a few optimisation passes later in LiveDebugVariables [0], this patch just brings that check further forwards, due to the errors that can be introduced (described above). What's unfortunate is that this also prevents occasions where register coalescing re-enlivens DBG_VALUEs with a vreg that has the right value in it, occasions which can be generated due to limitations in SelectionDAG.

This then becomes a trade-off (see immediately preceeding comment) of whether the problem is so bad that we should trade some lost locations for getting rid of placeDbgValues. IMHO: yes, because we're trading definitely-incorrect locations for some more being optimized-out, a net reduction in error, to me. Other opinions most welcome.

[0] https://github.com/llvm/llvm-project/blob/b9f1e7b16ed2341e54b4e2033d111e7a2ca19b9a/llvm/lib/CodeGen/LiveDebugVariables.cpp#L624

Thanks!

looks good!

A rebase shouldn't affect the spirit of the change :-)

Thanks for your help!

Thanks, this addresses my concern!

I put up a Verifier patch at https://reviews.llvm.org/D63499. Once that patch has landed, this patch should be safe to re-apply.

This seems to be a case of having old, incorrect, metadata baked into the test.
The 'startLoc' (!680) and 'endLoc' (!681) in loop metadta for loop !679 are both
missing an 'inlinedAt' node.

I think this is good to go.

Address review feedback.

Thanks!

One last nit inline. Thanks!

Thanks for the quick response! Please let me know if I can help debugging this.

In D63171#1539070, @clayborg wrote:

In D63171#1539049, @aprantl wrote:

I suppose one could compile Objective-C code on Linux using GCC.

Will GCC not set the language to ObjC or ObjC++?

This patch broke our internal CI. You can reproduce the issue by downloading https://gist.github.com/adrian-prantl/ba88912878db855ec96534e6510246e6 (this is AArch64 bitcode for a file in the Swift stdlib) and running

I suppose one could compile Objective-C code on Linux using GCC.

Hmm, I'm not sure what else we need aside from the existing isValid() check in Verifier::visitDIExpression.

I think this is starting to look good!

Stylistic nit: unless this will push a lot of lines > 80 columns and kill readability, I would prefer Fragment over Frag.

Can you please add this to either LangRef.rst or SourceLevelDebugging.rst and also explain how this operation composes with other DIExpressions? I'm thinking particularly about DW_OP_LLVM_fragment.
This also needs an assembler round-trip-test and probably some checks in Verifier.cpp.

dwarfdump(-classic) on macOS also supported the --out-file option and on macOS we strive for compatibility with the older tool.

In D63013#1536440, @jhenderson wrote:

In D63013#1535260, @aprantl wrote:

I'm mildly against this change. IIRC, the idea was that --out-file is a long-form option that nobody uses in practice and we wanted people to prefer -o instead, which is what people are most familiar with working with compilers. So if -o shows up in the --help output, I'd leave it at that.

Could we just delete --out-file, if we don't want people to use it?

It's my belief that that's how SROA works, but I don't think it's a documented guarantee. SROA::splitAlloca certainly seems to completely rewrite dbg.declare's into ones that all use fragments. The verifier doesn't attempt to enforce this rule though.

That said, if clang also documents -o as --out-file, I think consistency wins.

I'm mildly against this change. IIRC, the idea was that --out-file is a long-form option that nobody uses in practice and we wanted people to prefer -o instead, which is what people are most familiar with working with compilers. So if -o shows up in the --help output, I'd leave it at that.

(marking as "request changes", so I don't forget to reply to the question, otherwise this will get lost in my inbox)

Thanks, that looks like an acceptable performance/accuracy tradeoff.

And thanks for doing the measurements!

Thanks!

I'm kind of curious where the performance is gained back, but generally, if the performance is the same, we can tentatively land this. Please be quick to revert if complaints about the performance resurface.

In D62650#1527632, @aprantl wrote:

Have you looked at the performance of the example posted in https://bugs.llvm.org/show_bug.cgi?id=33730 ?

Have you looked at the performance of the example posted in https://bugs.llvm.org/show_bug.cgi?id=33730 ?

Sorry, I got more comments :-)

The assumption that stack variable locations run to the end of a function, even over the frame destruction, is seen in GCC here [0].

In D61600#1527182, @dstenb wrote:

In D61600#1527114, @NikolaPrica wrote:

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

I think that the idea was to see if this patch increases the compilation time when building such a Clang binary.

(Just make sure to use the RelWithDebInfo build type also.)

I'm reposting your reply with an easier to read format:

log(before this patch) :

512B bb.2.for.body:

520B renamable $x8 = MOVaddr target-flags(aarch64-page) @array, target-flags(aarch64-pageoff, aarch64-nc) @array, debug-location !54; test_debug_val.cpp:16:17
524B renamable $w9 = MOVi32imm 56, debug-location !54; test_debug_val.cpp:16:17

DBG_VALUE $x19, $noreg, !"i", !DIExpression(), debug-location !51; test_debug_val.cpp:0 line no:14
544B STRWroX killed renamable $w9, killed renamable $x8, renamable $x19, 0, 1, debug-location !54 :: (store 4 into %ir.scevgep, !tbaa !38); test_debug_val.cpp:16:17

!51 = !DILocation(line: 0, scope: !35)
!52 = !DILocation(line: 14, column: 4, scope: !35)
!53 = !DILocation(line: 21, column: 4, scope: !14)
!54 = !DILocation(line: 16, column: 8, scope: !55)

short log :

512B bb.2.for.body:

520B $x8 = MOVaddr scope: !55
524B $w9 = MOVi32imm scope: !55

DBG_VALUE $x19, $noreg, !"i" scope: !35
544B STRWroX killed renamable $w9 scope: !55

i.e. DBG_VALUE was inserted into enclosed scope.

The reason for this is that scope identified by MachineInstr :

LiveDebugVariables.cpp:902

const InsnRange &Range : Scope->getRanges()

using InsnRange = std::pair<const MachineInstr *, const MachineInstr *>;

At the moment when scope was created the very first instruction was STRWroX.
So it become start of range. Start slot index(with type of Slot_Block) for
DBG_VALUE was trimmed to this instruction range. But after Register Allocator
there were inserted new MOV instructions before STRWroX with the same scope as STRWroX.
But Slot indices for DBG_VALUE was not updated and scope boundaries was not updated.

Actually Slot indices for DBG_VALUE looks correct without trimming : 512B,848B.
512B points to start of basic block:

512B bb.2.for.body:

which match with definition of slot index of type Slot_Block:

Basic block boundary. Used for live ranges entering and leaving a
/ block without being live in the layout neighbor. Also used as the
/// def slot of PHI-defs.

i.e. 512B,848B actually match with lexical scope and does not require trimming.

Also, trimming this slot index and leaving it of type Slot_Block is a error. Slot_Block should point to start of basic block.

I.setStartUnchecked(RStart); <<< trimmed I.start() is a Slot_Block

This concrete problem could be solved by avoiding trimming for Slot_Block only.

But it looks like trimming also is not required for the original problem test case : live-debug-variables.ll. With and without trimming it generates the same DBG_VALUE instructions :

DBG_VALUE $ecx, $noreg, !"i4", !DIExpression(), debug-location !14; foo.c:3:35 @[ foo.c:8:10 ] line no:3
DBG_VALUE $ebx, $noreg, !"i4", !DIExpression(), debug-location !14; foo.c:3:35 @[ foo.c:8:10 ] line no:3

So in short :

For the problem from D62650: SlotIndex was trimmed incorrectly. It does not require trimming and has incorrect value for Slot_Block after trimming.
Trimming for testcase from D35953 looks unnecessary. With and without trimming it generates the same DBG_VALUE instructions.
Thus it looks like trimming could be removed at all.