- User Since
- Mar 2 2013, 8:12 AM (333 w, 1 d)
Tue, Jul 2
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.
Mon, Jul 1
Sat, Jun 29
Fri, Jun 28
We need to come up with a better error handling strategy before this can be landed.
Thu, Jun 27
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.
Wed, Jun 26
Tue, Jun 25
Mon, Jun 24
A rebase shouldn't affect the spirit of the change :-)
Jun 19 2019
Thanks for your help!
Jun 18 2019
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.
Jun 17 2019
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.
Jun 14 2019
Address review feedback.
Jun 13 2019
Jun 12 2019
One last nit inline. Thanks!
Thanks for the quick response! Please let me know if I can help debugging this.
Jun 11 2019
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.
Jun 10 2019
dwarfdump(-classic) on macOS also supported the --out-file option and on macOS we strive for compatibility with the older tool.
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.
Jun 8 2019
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.
Jun 7 2019
(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.
Jun 4 2019
And thanks for doing the measurements!
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.
Jun 3 2019
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 .
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:
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.