In D91203#2391662, @aardappel wrote:

@yurydelendik note that wasm64 is an LLVM-level construct, in general Wasm we may one day have multiple memories, which means that "being 64-bit" is a property of each load/store instruction individually. So assuming we have a DWARF section associated with a possibly mixed mode Wasm module, there's no way to determine the size of DW_AT_low_pc or DW_FORM_addr from the module.

Granted, this is a little hypothetical, since we don't know if wasm-ld will ever support such mixed linking (or another linker will), or wether DWARF will be output by non-LLVM producers, but the fact remains that Wasm really doesn't have an "architecture" in the same way as other ISAs.

Thus, it seems easier to me to hard-code DW_FORM_addr to always be 32 or 64-bit for Wasm. Either choice is going to give the problem this patch is trying to address, though.

In D91203#2389882, @sbc100 wrote:

Given that we already have DWARF consumers of WebAssembly DWARF pointers I wonder how they handle this today?
For example 1, 2, and 3 are perfectly valid function addresses in user code but make no sense as bytecode offsets.

In D77353#1987324, @aardappel wrote:

@yurydelendik one thing about your patch: the variadic argument was previously declared as SignedSizeLEB and emitted with emitSigned, yet your new code reads it as getULEB128 and stores it in an array of uint64_t (which we can't change since its shared code). It doesn't matter much for the current use case, but we might want to be consistent about this storing signed or unsigned values? I see value in it being signed for future use cases, but given the existing DWARFExpression code it may well have to be unsigned.

In D77353#1972855, @aardappel wrote:

So far we only need 2 tags, for locals and globals, and maybe soon a third for stack-relative. I don't see us running out of 32 entries that fast :)

In D77353#1972809, @aardappel wrote:

@yurydelendik how would you change Descriptions[DW_OP_WASM_location] = Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSizeLEB); to make it variadic?

In D77353#1972706, @dschuff wrote:

Concretely you are proposing that the form of DW_OP_Wasm_location be variadic instead of fixed, right? i.e. it has different fields based on the value of the first field. I don't think there is any precedent for that in DWARF anywhere, is there?

If there are no further comments on improvements we can carry out right now, does anyone feel like approving this for merging? :)

I'm still curious if something like this will work:

In D73581#1845884, @dschuff wrote:

I'll go ahead and land this to reduce the number of cases where we crash; but I'd be interested in hearing if @yurydelendik has found any cases that were asserting before that this fixes, or if any of the wasm subscribers have other opinions.

Agree with all the changes

Closing in favor of D71681

rename addWasmLocation

Fix comments

add llvm-dwarfdump test

Use TargetIndex operands in DbgValue to track WebAssembly operands locations

In D69807#1734845, @dschuff wrote:

we'd need to ensure that the frame base stays in the designated local for the whole function, correct? i.e. we'd have to ensure it never gets stackified, which it currently could, right?

Rebase D52634: [WebAssembly] Add DBG_VALUE with local operands location in WebAssemblyExplicitLocals pass

In D58453#1585436, @jmorse wrote:

the WebAssemblyDebugValueManager constructor could just apply an additional filter on collected DbgValues, that the LiveInterval/ValueNumber the DBG_VALUE refers to is the defining instruction.

In D52634#1584117, @dschuff wrote:

Let's work toward getting this landed!
@yurydelendik Your note mentioned that you reverted the breg version based on some kind of incompatibility, can you say more about that?

Rebase

In D58453#1582934, @jmorse wrote:

I've added some changes to the patch that makes collectDebugValues behave in the way I described, returning all debug users of a VReg, optionally limited to only the basic block of the source MachineInst. This should be useful for testing; and it's immediately caused the test/DebugInfo/WebAssembly/dbg-value-move-reg-stackify.mir [0] test to fail. The output is:

%0:i64 = ARGUMENT_i64 0, implicit $arguments
dead %1:i32 = I32_WRAP_I64 %0, implicit-def dead $arguments
dead %1:i32 = CALL_i32 @bar, implicit-def dead $arguments, implicit $sp32, implicit $sp64
%2:i32 = CALL_i32 @bar, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def $value_stack, implicit $value_stack
DBG_VALUE %2, $noreg, <0x5490d00>, !DIExpression(), debug-location !DILocation(line: 359, column: 12, scope: <0x548f5f0>)
DBG_VALUE %2, $noreg, <0x5490cb0>, !DIExpression(), debug-location !DILocation(line: 358, column: 12, scope: <0x548f5f0>)
DBG_VALUE %2, $noreg, <0x5490a50>, !DIExpression(), debug-location !DILocation(line: 357, column: 12, scope: <0x548f5f0>)
CALL_VOID @foo, %2, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def $value_stack, implicit $value_stack
RETURN_VOID implicit-def dead $arguments

While I don't know webassembly, it seems clear that a) the test post-SSA, and b) the test expects collectDebugValues to only return DBG_VALUEs that refer to the same vreg def that the source instruction does.

Is that a fair assessment, or some kind of test artefact? That behaviour is almost certainly achievable after this patch lands, although it might require more plumbing somewhere.

[0] https://github.com/llvm/llvm-project/blob/a1d97a960e622ee21550d92809512cb0870be499/llvm/test/DebugInfo/WebAssembly/dbg-value-move-reg-stackify.mir

In D58453#1564940, @jmorse wrote:

Hi,

Awkwardly collectDebugValues relies on a behaviour that this patch will mostly undo, specifically, that DBG_VALUE insts are placed after the defining instruction. collectDebugValues will work sometimes, but will "miss" DBG_VALUEs much more often that it does now.

In D58453#1563748, @aheejin wrote:

@yurydelendik Would this change be OK for WebAssembly debug value handling?

Looks good. Thanks

rebase

@jingham is this patch good to land?

In D57689#1384148, @jingham wrote:

Could you add a test for this setting to the ./functionalities/jitloader_gdb/TestJITLoaderGDB.py test?

• Remove enable-jit-breakpoint option

• Add tests

• Change to on/off/default property.

Fix GetEnableLoaderForDarwin name

Minor changes to comment; move #include

Add simple test

• revert breg use due to incompatibility with DWARF definition
• rebase to git llvm-project

Top comment fix

• Prune unused #include
• Fix comments
• Reduce default size of vector to 2
• Reverse insert order for move/clone

• Fix WebAssemblyDebugValueManager::move
• Remove DebugValueManager::reMaterialize

In D56401#1354978, @aheejin wrote:

And another question: I haven't been a reviewer for your other debug info related CLs so I don't know, but are there any other places or your pending CLs that can make use of this new class?

• Fix trivially clonable instruction moving/cloning

Revert bad test change

In D56401#1353800, @aheejin wrote:

Oh and I think we need test cases for

1. Rematerializing a def in another BB that has DBG_VALUE attached

• Move WebAssembly::DebugValueManager into WebAssemblyUtilities.*
• Fix testing of dbg-value-move-reg-stackify.mir

Remove TII param from clone()

• Move MachineInstrAdjacentDebugValues
• Use CloneMachineInstr

In D56401#1350516, @aheejin wrote:

I haven't finished reading other parts, but I think I should understand this AttachedDebugValues constructor first to proceed.

• Use collectDebugValues
• Rename AdjacentDebugValues and DefDIs

fix code style

In D52634#1336518, @dschuff wrote:

Come to think of it, might we actually want the possibility of constant offset in addition to the "kind" and index field, e.g. for cases where the value is actually a pointer?

In D52634#1336455, @aprantl wrote:

In D52634#1335761, @dschuff wrote:

@aprantl Is the advantage of your suggested approach just that we don't have to define a new expression type? Obviously the interpretation is not the same as DW_OP_breg on other targets so as you say, either way there would have to be special logic in all the tools that consume it. Is this kind of repurposing of builtin primitives common?

I'm not against this approach, I was just trying to point out that an alternative less invasive encoding could be used that might be less confusing to existing consumers of debug information.

Replace DW_OP_wasm_location with DW_OP_breg

In D52634#1334758, @aprantl wrote:

I also found that the tools (e.g. LLVM or LLDB) are eager to know about number of registers (which we cannot determine) to allocate internal structures.

What I was thinking of was to define exactly three WASM DWARF register numbers: wasm_reg_local, wasm_reg_global, wasm_reg_stack and use DW_OP_breg regnum offset like you would use the new special operation introduced by this patch.

In D52634#1334701, @aprantl wrote:

In D52634#1334438, @yurydelendik wrote:

In D52634#1293526, @aprantl wrote:

Can you explain how this new operation is meant to work?

@aprantl, here is some description of what we are trying to achieve: https://github.com/WebAssembly/debugging/issues/1#issuecomment-448237641

In the nutshell, the WebAssembly extensions for the DWARF expression will allow us to express WebAssembly specific locations such as for locals, globals and operands stack. The consumers of this information will be limited to LLVM tools, web browser debuggers and AOT/JIT WebAssembly compilers.

Does WebAssembly have a concept of registers? You could just define three additional register numbers for WebAssembly and address a stack slot as DW_OP_breg [wasm_stack_register] + 3 etc.. This would need less support in all the involved tools. The link says that it was considered, but rejected, but there is no mention as to why.

In D52634#1293526, @aprantl wrote:

Can you explain how this new operation is meant to work?

Change DW_OP_WASM_location op to 0xED.

Can you explain how this new operation is meant to work?

There was a question about how easy it will be to reuse or modify LLDB to use with such extension (DW_OP_WASM_location). I looked at DWARFExpression::Evaluate and, I think, it will not be a problem to add support for that. It might be even easier than try to emulate registers mapping to some WASM local or stack operand.

Rebase

Further reduced by @aheejin test case

And I don't think we need the BEFORE check... what I meant by the 'before' check in the comment above was, before your fix, the wrong code sequence generated was like that.

Further reduction of cfg-stackify-dbg-skip.ll

In D52837#1255388, @sbc100 wrote:

Would bugpoint help here? Its specifically designed to minimize test case I think although I don't have a lot of experience with it myself.

Minimize test case; fix its comment

In D52837#1255219, @aheejin wrote:

.. you can pass multiple -run-pass flags to llc, like llc -run-pass wasm-reg-stackify -run-pass wasm-reg-coloring .... Maybe we can start from mir just before the RegStackify pass and make llc run all passes from RegStackify to CFGStackify. I think it's worth trying. If this fails for some other reason, we can bring back .ll test case...

Revert test to .ll form, but ensure valid input before wasm-cfg-stackify pass

Add mir test

In D52837#1254381, @sbc100 wrote:

How does the output of the test differ without this fix?

Add test comment

Skipping labels

In D52837#1254005, @aheejin wrote:

Why should we skip debug instructions when searching for block start?

In D49034#1246966, @sbc100 wrote:

We we revert this or get it fixed? Its currently breaking the wasm waterfall: https://wasm-stat.us/console